Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix avatar sanitize and cleanup code & css #1786

Merged
merged 1 commit into from
Sep 6, 2020
Merged

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Sep 5, 2020

  • Cleanup code & template
  • Move css
  • Cleanup css
  • Fix inline js event xss

Xss svg examples (safe, fire an alert)

  1.  <svg xmlns="http://www.w3.org/2000/svg" height="256" width="256" onload="alert('xss')">
     	<rect ry=".5" rx=".5" height="256" width="256" fill="#0082c9"/>
     	<g transform="translate(0 64)" stroke-width="22" fill="none" stroke="#fff">
     		<circle r="26" cy="64" cx="40"/>
     		<circle r="26" cy="64" cx="216"/>
     		<circle r="46" cy="64" cx="128"/>
     	</g>
     </svg>
  2.  <svg xmlns="http://www.w3.org/2000/svg" height="256" width="256">
     	<rect ry=".5" rx=".5" height="256" width="256" fill="#0082c9"/>
     	<g transform="translate(0 64)" stroke-width="22" fill="none" stroke="#fff">
     		<circle r="26" cy="64" cx="40"/>
     		<circle r="26" cy="64" cx="216"/>
     		<circle r="46" cy="64" cx="128"/>
     	</g>
     	<script>alert('xss')</script>
     </svg>

Capture d’écran_2020-09-06_12-03-16
Capture d’écran_2020-09-06_12-03-28
Capture d’écran_2020-09-06_12-03-38

@skjnldsv skjnldsv added 3. to review Waiting for reviews bug Something isn't working high High priority security Pull requests that address a security vulnerability labels Sep 5, 2020
@codecov
Copy link

codecov bot commented Sep 5, 2020

Codecov Report

Merging #1786 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1786   +/-   ##
=========================================
  Coverage     34.38%   34.38%           
  Complexity      148      148           
=========================================
  Files            19       19           
  Lines           442      442           
=========================================
  Hits            152      152           
  Misses          290      290           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c00530f...fbb5858. Read the comment docs.

@skjnldsv skjnldsv self-assigned this Sep 5, 2020
@call-me-matt
Copy link
Member

Seems the image-button-overlay now is visible all the time. It was only on mouse-over before. Is that on purpose?

@skjnldsv
Copy link
Member Author

skjnldsv commented Sep 6, 2020

Seems the image-button-overlay now is visible all the time. It was only on mouse-over before. Is that on purpose?

Accessibility for keyboard and mobile users. If it's hidden, you don't know it's there. :)
Just pushed a fix for keyboard visual feedback that wasn't visible

@skjnldsv skjnldsv force-pushed the fix/avatar-sanitize branch from 121a2e7 to 6b14f68 Compare September 6, 2020 10:02
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the fix/avatar-sanitize branch from 6b14f68 to fbb5858 Compare September 6, 2020 10:05
@call-me-matt
Copy link
Member

Thought that would be the reason we keep the second menu in the modal. But if its on purpose, that's fine for me.

@skjnldsv
Copy link
Member Author

skjnldsv commented Sep 6, 2020

Thought that would be the reason we keep the second menu in the modal. But if its on purpose, that's fine for me.

Right! Let's ping @jancborchardt for when he gets back and let get this in :)
I'll handle the previous behaviour rollback if necessary! 🚀

@skjnldsv skjnldsv merged commit ead1bfa into master Sep 6, 2020
@skjnldsv skjnldsv deleted the fix/avatar-sanitize branch September 6, 2020 14:16
@jancborchardt
Copy link
Member

Seems the image-button-overlay now is visible all the time. It was only on mouse-over before. Is that on purpose?

Accessibility for keyboard and mobile users. If it's hidden, you don't know it's there. :)
Just pushed a fix for keyboard visual feedback that wasn't visible

I’d agree it would be nicer to only show it on hover/focus. Also showing it on focus makes sure it’s also visible to keyboard users. On mobile, I’d say people expect something to happen on tap of the avatar if they want to change it.
And if not avatar is set for a person, the button can still be shown by default, centered on top of the avatar – that way people also know it’s there. :)

@call-me-matt
Copy link
Member

I'll handle the previous behaviour rollback if necessary!

@skjnldsv if you find the time, I would also prefer the hover behaviour.

@skjnldsv skjnldsv added this to the 3.4.0 milestone Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working high High priority security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants