-
Notifications
You must be signed in to change notification settings - Fork 46
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
get user/:userId exposes user info that should be hidden #70
Comments
Are there sensitive fields that need to be hidden? |
As a user I probably woudln't want my email exposed and I don't think the password hash should be exposed. There may be others as well... it probably would be worth explicitly setting things as private (email) vs public (username, profile image, bio). Having users decide what is private/public (some may want email/full name as public, others not) increases complexity but may be worth considering in future. |
Good point. Adding @jasonify for visibility. |
Ah yeah, we don't want to be exposing either of these. For now we can omit the password hash + emails by default as a quick solution. Though if we add a note to the email being public (like on github for example), that might be okay too. Not sure how much of a problem email scrapers are these days. |
The difference is that you can opt out of public email on Github. |
True, had that same thought after I commented. We should hide them for now until we add that functionality. |
This wasn't a problem previously, because
/users
wasn't used inindex.route.js
. But now that it is, a call toapi/users/:userId
will return all user info even if you're not that same user or even logged in.Something that might address this as well as support issues #36 and #56 going forward, could be a
toAuthJSON
andtoProfileJSON
functions which would only expose those elements to the same authenticated user and to anyone respectively. Here is a similar example.Because there is both auth and user controller it seems it would be easiest to put in model but its worth discussing.
As a stop gap - simply disabling the route might be the best choice (my guess is it isn't used anywhere, because the /users was only recently exposed)
The text was updated successfully, but these errors were encountered: