fix(security): fixing possible xss issue in parsed objects

[lirantal]

To handle potential security issues with XSS - the following PR introduces:

• refactoring the user object that gets populated in the server-side to achieve: 1. a pre-defined sub-set of the user object will be passed to the client side for usage; 2. escaping possible malicious XSS strings such as username, displayName, etc.

Fixes #1106

[lirantal]

With this fix, there is still some meddling required on the angular side though.
For example, check out the edit profile view:

[mleanos]

I started playing around with this branch, and noticed a couple of issues..

1. The Authentication service will need to be updated to check that we have a populated user object. Currently, it assigns the user from $window.user which will exist even when there's no user logged in; thus, the application thinks the user is logged in. We may want to change some of the logic around this service.

2. In Chrome, if not logged in & I navigate to the articles list, the browser's user object gets set to my previously logged in user. Firefox doesn't behave this way. Fixing the first issue should help with this, but I still find it concerning (& confusing) that this is happening.

Test this with both logged in users, and non authenticated user, to see these behaviors.

As for the issue of injecting that script into the username field, we can add some validation logic using Angular, right?

[mleanos]

Where is this user object coming from? Is it getting it from the session middleware when the page is rendered?

If would be nice to have some sort of condition here that would check if the user data is populated. If it's not then set this new user object to undefined or null.

Is that possible here? Perhaps, with a locals variable?

[lirantal]

the user object comes from the server's response object/express variables.
that view is server-side view, not angular.

on your comment with not setting it, how about setting to an empty object rather than null/undefined? is that safer?

[mleanos]

This part of the express middleware is still a bit elusive to me; meaning I don't understand it that well..

This is how I'm perceiving it to be working, in regards to the Authentication service. When this object is set on the browser, it becomes accessible through $window.user. Is that correct?

With the above assumption..

I think we can just set this variable to an empty object. However, how would we do this? In the current state of this PR, if the user isn't authenticated then the express variable is undefined. Thus, this variable is set to the following..

{
  displayName: "",
  provider: "",
  username: "",
  created: "",
  roles: "",
  profileImageURL: "",
  email: "",
  lastName: "",
  firstName: ""
}

This is probably sufficient, and we'd avoid making significant changes to the backend request/express user variable. However, we need to adjust the logic in the Authentication service. At this line https://github.com/meanjs/mean/blob/master/modules/users/client/services/authentication.client.service.js#L7, we probably can change it to..
user: $window.user.username.length ? $window.user : {}, or something similar

[lirantal]

@mleanos I didn't get your 2nd bullet about seeing another user's session when not logged in... sounds very bad. Can you explain better?

Also, you can go ahead and commit to my branch all the changes concerning the angular part.

[mleanos]

@lirantal Regarding my 2nd point..

I was monitoring what the browser's user object was getting set to, by viewing the source of the page. When I wasn't logged in, I navigated to the Articles list view & viewed the page source. The user object was set to my previously logged in user. My guess, is that it pulled it from the browser cookie.

I only experienced this in Chrome, and not Firefox; I only tested with these two browsers.

Can you try to reproduce?

[mleanos]

@lirantal Also, what changes do you see needed on the Angular side of things? I'm more than happy to submit them to your branch.

Some of these changes could include the following..

1. Update Authentication service to account for new empty object structure in $window.user
2. Add client-side validation that would prohibit malicious field data. I may need guidance on this, as to what these guidelines should be (and what potential data is malicious.) Is it merely prohibiting anything that has >script and similar?

Anything else?

[codydaig]

@lirantal @mleanos I know the second was an issue in the past but I was under the impression that we fixed that a while back.

[lirantal]

@mleanos about the actual changes required - I refactored so that the change is only happening in the server-side, including the sanitizing before echo'ing back to the frontend to render.
I also left the original null value to be returned and not an empty object because that really messed things up with the tests and the UI.

On the Angular part - see my screenshot above - it requires "re-encoding" the data back on the view layer so that it doesn't show the HTML entities decoded. EDIT - actually re-testing this it looks ok.

[lirantal]

@codydaig @mleanos as to the other weird issue reported - I'm able to reproduce this:

1. login with user to meanjs
2. browse some stuff, then press CTRL+F5 to re-load the page
3. browse reloads the page from scratch and shows user as anonymous, but for some reason the user object is still populated on the page

any ideas?

[mleanos]

@lirantal Nice. Yea, keeping the changes on the server-side is the best way to go. Also, so much of the framework expects null (or undefined) for the user object when not logged in. It will take some work to refactor everything to expect an empty object. If we decide an empty object is better, we can tackle that in a new PR.

I've re-synced to your latest change, and tested. I can verify the E2E tests are passing now. And now there's no need to refactor the Authentication service.

I'm about to try to reproduce your issue. Did you have any success reproducing what I reported above?

Quick question: Only applying the validator.escape on the displayName is required?

[mleanos]

@lirantal I wasn't able to reproduce the issue you described. I was experiencing odd-ish behavior with the user object on the page, and it appeared to be related to previous browsing history. I cleared my cookies & browsing history for my local meanjs host. After that, the odd behavior went away. Perhaps try to do the same, and see if you still experience your issue.

After removing history/cookies in Chrome, I'm still seeing the same behavior I reported above. However, once I do a hard refresh (CTRL+F5) of the page, the data isn't there anymore. It's weird tho, that even after clearing the cookies, the data would still be there until I do the hard refresh.

I think we can chalk these issues up to browser (cache) issues. WDYT?

@codydaig

[lirantal]

Thanks for the feedback @mleanos
About the validation you're right, it's not yet complete and I will update the rest of the fields too to be escaped properly.

[lirantal]

@mleanos addressed changes.

[mleanos]

LGTM, other than the coverage. It looks like we just don't have tests specifically for the core server controller; the little coverage that file has is coming from other tests.

[lirantal]

Right, the code coverage is a bit harder in that area because if you inspect the missing statements at https://coveralls.io/builds/4587050/source?filename=..%2Fmodules%2Fcore%2Fserver%2Fcontrollers%2Fcore.server.controller.js then you clearly see these are basically the actual interaction with the page and error handling.

[ilanbiala]

@lirantal for me it says source is unavailable. Does that link work for you?

[lirantal]

@ilanbiala try this: https://coveralls.io/builds/4587050 and scroll down to click on the actual controller file

[codydaig]

@ilanbiala @lirantal I'm getting the same error "Source not available" even with @lirantal's other link.

[ilanbiala]

Strangely, this link works. https://coveralls.io/builds/4594977/source?filename=..%2Fmodules%2Fcore%2Fserver%2Fcontrollers%2Fcore.server.controller.js

[ilanbiala]

For these statements, can't we actually make a request and test the header and content-type? Then it will test these lines, no?

[mleanos]

That's really odd, about not being able to see the coverage on that file.

@ilanbiala I think you're right. We can just a request, and test the header & content-type. We'll need to have tests for both logged in users, and non-authenticated users.

I think renderServerError will be a little more tricky, but we should still be able increase converage, without testing this method.

[lirantal]

The server-side route tests simply don't touch these areas, I don't think it's related to header or content-type.

I'll try to work on adding these for these areas too.

[lirantal]

Merging fix for now.