Sanitise user

[Wuntenn]

feat(users): Routes are currently leaking access tokens

Authentication routes use route /api/users/me and /api/users/ (for admins). Maybe a mock closer to passport like: https://gist.github.com/mweibel/5219403 would be nice. In the meanwhile this should make the returned user object safer. See:

https://www.facebook.com/FacebookforDevelopers/videos/10152795636318553/

(specifically the part about stealing access tokens). I re-used code from core:

modules/core/server/controllers/core.server.controller.js

Fixes n/a - general security improvement

[Wuntenn]

Currently whilst a user is logged in, the route /api/users/me can be navigated to which shows all the data of that user. If the user is an admin, navigating to /api/users/ gives away access tokens for all users in the db. I've removed the providerData for both.

Unlike the data attached to the window.user this data isn't sanitised. I've basically ripped code from the core.server.controller and used it to remove unnecessary fields.

If we could find a way to more closely test passport we could probably eliminate the api/users/me route for the user and restrict it to just admin.

[ilanbiala]

@Wuntenn can you take a look at the failing Travis build?

[Wuntenn]

@ilanbiala I've fixed the typos and pushed it the the branch again. It's my first rebase and hopefully first PR with anything useful.

I'm not sure what happens now. Will travis re-run the tests based on the code in the updated branch or do I need to re-submit the PR?

[mleanos]

@Wuntenn It looks like this branch has diffs that have already been merged into master. Can you ensure you've rebased your local master branch from upstream & then rebase your PR branch with your local master? It will be difficult to see what has changed here, otherwise.

You'll most likely get a "branch has diverged" message in your GIT console. Simply do a git push --force & it should overwrite your remote branch with exactly what your local branch looks like (including all the up to date GIT history).

Anytime you update this remote branch, Travis CI will automatically re-run the builds. So no need to re-submit a PR.

[Wuntenn]

Not sure why exactly @marianoqueirel has chosen to leave shoutouts in this thread, but thanks @mleanos, I would've definitely have gone the wrong/long way about rebasing had I not (re)read your comment.

So far, I thought that reading: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History gave me all I needed to know about rebasing.

Your comment didn't make sense to me initially because I didn't know that branches could be supplied to rebase to replay all the commits within them. Now reading: https://git-scm.com/docs/git-rebase instead :D

[marianoqueirel]

@Wuntenn
Docs for CONTRIBUTING
Docs for PR

[Wuntenn]

@marianoqueirel I'm sure your "Thanks you's" are well deserved. The thing is you sought out help and was given advice in other threads. I was hinting that (unless you felt that this should be the "rebase enlightenment thread", that) maybe you could/should thank people in those threads where you received help. Github's noisy enough as it is.

I didn't try to insult you. If you're upset about that then please explain.

Docs for CONTRIBUTING
Docs for PR

Is there something in particular that you want draw my attention to within those two documents that's relevant to my PR and/or the way that I've contributed?

[marianoqueirel]

@Wuntenn . No, for nothing, on the contrary, you explained me that isn't the place for this type of comments and I understood, and think so.

About the two docs,
Sanitise user could be feat(users): Routes are currently leaking access tokens for example ? for PR title.
I say this because I was just reading the ways to make contributions.

[Wuntenn]

About the two docs,
Sanitise user could be feat(users): Routes are currently leaking access tokens for example ? for > PR title.

@marianoqueirel I'm not so sure that would be better. As far as I'm aware the commit message should state in positive terms, the action(s) that it applies. To quote from the same Docs for PR document which you sent to me:

• The subject line should be clear and concise as to what is being accomplished in the commit.

Your suggestion is the state the conditions pre-commit. I can't see how that would be useful to someone looking back at the commit's. They wouldn't be able to ascertain (without reading the source code) which changes expect.

Take a look at: http://alistapart.com/article/the-art-of-the-commit

[Wuntenn]

@marianoqueirel - Sorry I've been an ass! I'm stressing about stuff, I should've ignored the comment earlier.

[mleanos]

@Wuntenn Can you explain the advantages for us to use our own passport verification here, as described in the mock example? How does this differ from our current Strategy implementation?

[lirantal]

@mleanos I don't think we need to create any mock. This looks just well.

[mleanos]

I agree. I was questioning the TODO note.

[mleanos]

I think this PR is related to, and similar to the issue, this PR is attempting to solve:
#1421

Perhaps, there can be some collaboration with both of these PR's?

[Wuntenn]

@mleanos The part which I re-used is the part used at the end of the passport authentication flow to remove the same fields, however this has nothing to do with passport.

My PR is related to the retrieval of users data via these routes. The user is currently able to show his/her own data via the route: /api/users/me. Admin are able to additionally list every users data from the route api/users. Both routes bypass any sanitation. My PR introduces it into these routes.

My PR also removes data which is not really necessary (IMO) to list or show, like the providerData which means that it's not so easy to get to access tokens in bulk.

/api/users/me is only used for testing, Passport doesn't use it for authentication. Whilst both routes remain in the code base, and in light of the attacks mentioned in the above linked videos, my PR is a precaution to minimise exposure of access tokens. This is also recommended in the same video.

My pull request has nothing to do with #1421. That PR is related to saving/updating. In that pull request @shanavas786 improves the way that we update/save the user data.

We currently secure the way we update the users by blacklisting changes of certain user attributes via deletion of these attributes before save. His changes propose that we use an array of safe-to-edit attributes that act instead as a whitelist. This list is iterated and attributes included in it are changed only if necessary.

[Wuntenn]

Thinking about it maybe sanitise user would be be better re-used as a service. WDYT?

[mleanos]

@Wuntenn I think both PR's are considering sanitation of User data. They both are attempting to achieve the protection of data. This one protects data sent back to the client, and the other protects data coming from the client (into our db). It seems there could be some collaboration between both attempts to solve the problem in a more modular & reusable manner.

See my comment on @shanavas786's PR. The same solution could be used to satisfy both issues.

Regarding having a reusable service, what did you have in mind? It seems this might be only needed by the profile server controller. One other option might be to explore using pre/post hooks on the User model to sanitize the data. I'm not sure how that would look though.

[Wuntenn]

I think I see where you're coming from. I need to improve engineering perspective. I'm looking at it in terms of functionality (data-structures and algorithms) as the (initial) fix. You're looking higher up (system level) as the long term solution and seeing duplication of functionality.

I like the idea of using pre/post hook. We'd have to consider the affect to the passport initial save from the provider and how we create admin roles. We may have to introduce alternate lesser used save methods which work like our current implantation for use by admins. All other CRUD would pass the pre/post hooks.

---

I got caught up thinking about DRYing up the solution in response to your suggestion to merge the two PR's that I ended up thinking to use a service, however that would put the sanitation in the front-end... not good!!

A middleware would however do the job. If used to strip the req.user or res.user of unwanted fields it would remove the need to pass around a service.

Either way it would the two PR's would become one solution achieving your objective if I've understood correctly.

[shanavas786]

@Wuntenn @mleanos I see some differences between sanitizing user input and database object.
PR #1421 write protect or update protect certain fields while this PR read protect. The list of fields differ. Fields updated and created should be write protected but not read protected.

What about adding a method getSafeObject to User schema which returns sanitized user object?

[mleanos]

@Wuntenn Could we do something similar to what was done in #1421?

Also, I'm not sure if the use of validator is necessary. The only reason we're doing this in our main app view, is because this object will reside in the browser (DOM). We should be returning just the json that represents our data, and not transforming it in any way.

[Wuntenn]

@mleanos I'll take a look at it this weekend hopefully. Can't make any promises. Things are a bit crazy for me right now.

[lirantal]

@meanjs/contributors guys this looks just fine to me. It's only about not exposing social tokens from the providerData.

Can someone sum up what are the objections/comments on not merging this?

[lirantal]

And also thank you @Wuntenn for contributing this :)

[mleanos]

@lirantal My main objection here was the use of the validator module. I don't see why it's necessary here. I understand why we need it for the User object being stored in the browser's window object (window.user). Shouldn't we be merely passing the non-transformed data (with just our desired field list) back to the client in the response?

With that said, I'm not opposed to this getting merged in it's current state. I think the use of validator here may be useful when we move to Token-based Authentication; I would just refactor this a little at that time.

So what I'm saying is LGTM, as long as no others have objections.

[lirantal]

Agree on the TODO and probably on the validator, but we need to test first.
Would you like to take it on a personal branch and look it up, then submit a PR?

[mleanos]

Would you like to take it on a personal branch and look it up, then submit a PR?

@lirantal Was that a question for me? If so, I don't follow what you mean.

[lirantal]

@mleanos yeah, do you want to finalize the work here with @Wuntenn and merge this in?

[mleanos]

Let's merge this in. Seems ready to me.