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

Anonymize default profile photo url calls #940

Merged
merged 4 commits into from
Jan 7, 2022

Conversation

geisi
Copy link
Contributor

@geisi geisi commented Jan 7, 2022

Hello and Happy New Year!

This pr only affects instances with ProfilePhoto feature enabled .

At a security and privacy review of one of our Jetstream applications it was criticised that the applications sends the whole user name to an external service (https://ui-avatars.com/) on every request. So every single name of your application users potentially gets exposed to ui-avatars.com.

I think if someone randomly asks you to hand over all of your applications user names you won't approve that.

I want to clarify that this is no criticism on services provided by ui-avatars.com they are awesome and we can be glad that these services exist.

Since ui-avatars.com is only using the first letters of a users given name to generate its avatars this is completely unnecessary. So this PR corrects the application behaviour so that only the first letters of the name are added to the users default profile photo url.

I know that the rest of the world outside of Europe does not care that much about privacy concerns. But for all european and UK users this is a well being change.

DefaultUserPhotoUrl Before this PR:
https://ui-avatars.com/api/?name=Taylor+Otwell&color=7F9CF5&background=EBF4FF

And After:
https://ui-avatars.com/api/?name=T+O&color=7F9CF5&background=EBF4FF

Thank you for your amazing work and have a nice weekend!

@geisi geisi changed the title Anonymize default photo url calls Anonymize default profile photo url calls Jan 7, 2022
@taylorotwell
Copy link
Member

I know that the rest of the world outside of Europe does not care that much about privacy concerns. But for all european and UK users this is a well being change.

I do not think Europe is the only entity that cares about privacy.

@geisi
Copy link
Contributor Author

geisi commented Jan 7, 2022

I know that the rest of the world outside of Europe does not care that much about privacy concerns. But for all european and UK users this is a well being change.

I do not think Europe is the only entity that cares about privacy.

Don't get me wrong I don't want to criticise or judge about other countries regularities. And I did not want to give this PR a political touch. :D

@@ -66,7 +67,11 @@ public function getProfilePhotoUrlAttribute()
*/
protected function defaultProfilePhotoUrl()
{
return 'https://ui-avatars.com/api/?name='.urlencode($this->name).'&color=7F9CF5&background=EBF4FF';
$name = trim(collect(explode(' ', $this->name))->map(function ($segment) {
return $segment[0] ?? '';
Copy link

@nvi9 nvi9 Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taylorotwell, @geisi as mentioned in a comment at laravel/framework#40381, this implementation doesn't work as expected with multi-byte letters (eg. Á).

$segment[0] should be either changed to Str::substr($segment, 0, 1), or the linked PR should be included in the framework package and its new function should be used here like this:

return 'https://ui-avatars.com/api/?name='.urlencode(Str::initials($this->name)).'&color=7F9CF5&background=EBF4FF';

GeoSot pushed a commit to GeoSot/jetstream that referenced this pull request Jan 27, 2022
* anonymize defaultPhotoUrl() calls

* refactor to Str macro

* simplify

* remove import

Co-authored-by: Taylor Otwell <[email protected]>
DSpeichert added a commit to DSpeichert/filament that referenced this pull request May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants