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
Hide user gravatar when email not public. #4104
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4104 +/- ##
=======================================
Coverage 98.86% 98.86%
=======================================
Files 275 275
Lines 6254 6268 +14
=======================================
+ Hits 6183 6197 +14
Misses 71 71
|
20e8a1d
to
348310d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good to me 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for seeing this through!
348310d
to
02d76a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see this improvement. It seems like it's been requested for a long time. My review comment is not important by any means, just something I found useful in the past.
when :light then "/images/avatar.svg" | ||
when :dark then "/images/avatar_inverted.svg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've often seen SVGs styled directly in css to change the fill color. Is that worth it here? Something like this with invert() or even inlining the svg so it doesn't take an extra request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think doing it in the CSS makes sense, then the SVG will change without a page refresh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not related to dark mode, but I'll try to provide inline SVG with styles, that could be possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was tired when I wrote this. Don't invert()
it unless we want emerald users, haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a silly question, but I think we could do something like add a /users/:id/avatar_:size.png
endpoint that would proxy to gravatar, and then fastly would cache that, allowing us to continue showing avatars for everyone without leaking the email directly in the avatar link?
when :light then "/images/avatar.svg" | ||
when :dark then "/images/avatar_inverted.svg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think doing it in the CSS makes sense, then the SVG will change without a page refresh
I thought about the proxy as an option to hide the gravatar hash, and then I realized that if we are serving the image, anyone can download the image and reverse-image-search to find some other place serving the image with the hash in the URL. The information leakage remains the same as long as we serve the image at all, so I think we need to hide the image if the user wants to keep their email address private. |
There's all different levels of privacy. Not sharing email easily vs staying totally anonymous. Is there any evidence that there's overlap between the totally anonymous users and gravatar users? The only downside I can see is that a gem will look like it's published by a bunch of generic users if everyone has a private email. This feels like bikeshedding. I bet this current solution is going to solve for the private email use case and then we could deal with the subset of users that want a profile image, but not gravatar, after we get requests for that feature. |
ℹ️ I'll check out possible SVG inlining and open other PR if needed. |
Yeah I noticed this.. I thought something was broken, like some cache or something :) Just happened to see this PR. I understand the info leakage issue. I wonder if it was worth it. I think the avatars served a really good purpose. I imagine very few users will make their email public. Oh well... |
No worries, there are plans to update those pages to be more friendly again. This PR was just to fix the reported issue as quickly as possible for now. |
@segiddins is it ok to just add images to
public/images
folder? Would that be served by Fastly in production?default avatar examples
disabled avatar info