879748 - Secure the /user/:id route in webmaker-loginapi

Status: RESOLVED FIXED

(Webmaker Graveyard :: Login, defect)

Description

[Chris DeCairos (:cade)]

This route makes user data publicly available (including username and account status). It's gotta go, or if someone argues for it, protected in some manner. As it stands now it steps around the protection we placed on the Make API to not publicly expose user email accounts to the world.

https://webmaker.mofostaging.net/user/cade
https://webmaker.mofostaging.net/user/dude
https://webmaker.mofostaging.net/user/Scott

Comment 1

[Matthew Schranz [:mjschranz]]

I'd much prefer the route of making it more secure. For example, with 878774 I use the username that this module tacks onto the sessions for author now with Popcorn Maker.

Can we just manually get it? Sure, but I like the idea of it simply being more secure or returning less important data. Infact, maybe it should be limited to tacking that username on session and not returning any actual data.

Comment 2

[Chris DeCairos (:cade)]

wow.

If I'm logged in as myself (cade), I can hit that route with another user's id and make the server change the username on my session to another username.

Comment 3

[Chris DeCairos (:cade)]

here's how it changed my cookie: http://dl.dropbox.com/u/86269810/Screenshots/z~2b.png

Comment 4

[Chris DeCairos (:cade)]

( It should be ["username":"cade", ..} )

Comment 5

[Kieran Sedgwick (:sedge)]

I think the best solution, that sticks to why this endpoint was first created, is to add an authentication middleware to the route in the webmaker-loginapi module. 

This middleware would:

a) check if a persona session exists i.e. `if ( req.session.email )`
b) use the webmaker-loginapi's `getUser()` method with `req.session.email` to retrieve the logged-in user's details
c) compare the retrieved details against the `:id` being requested in the route to confirm it's the same user

Otherwise, there are two possibilities:

1) We remove the route completely, and leave it to each app's developers to add the route manually (leads to inconsistency in return data and expected behavior = bad IMO)
2) Come up with a completely different solution that replaces the need for a '/user/:id' path on each server in the first place.

Comment 6

[Kieran Sedgwick (:sedge)]

Also, if we choose the middleware option, we can modify the route logic to return a safe subset of the user's information instead of the entire user object.

Comment 7

[Chris DeCairos (:cade)]

(In reply to Kieran Sedgwick (:sedge) from comment #5)
> a) check if a persona session exists i.e. `if ( req.session.email )`
> b) use the webmaker-loginapi's `getUser()` method with `req.session.email`
> to retrieve the logged-in user's details
> c) compare the retrieved details against the `:id` being requested in the
> route to confirm it's the same user

I can get behind this proposal.

Comment 8

[Jon Buckley [:jbuck]]

Yeah, I think that sounds sane. Put it on me for review when you've done that

Comment 9

[Kieran Sedgwick (:sedge)]

Attachment: [WIP] Pull request for this patch

https://github.com/mozilla/node-webmaker-loginapi/pull/14

Comment 10

[Jon Buckley [:jbuck]]

Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

r-, notes in the pull request

Comment 11

[Kieran Sedgwick (:sedge)]

Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

https://github.com/mozilla/node-webmaker-loginapi/pull/14

Comment 12

[Jon Buckley [:jbuck]]

Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

This still won't provide any protection against visiting http://example.org/user/ksedge when I'm logged in as myself. See the pull request for more details, but this is a fundamentally broken API if it'll accept an email address, a webmaker username, or a webmaker id.

Comment 13

[Kieran Sedgwick (:sedge)]

Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

https://github.com/mozilla/node-webmaker-loginapi/pull/14

Comment 14

[Jon Buckley [:jbuck]]

Attachment: Strawman patch

I don't think it event needs to be that complex. Take a look at this patch.

The important part of both patches is that we take the email arguments from the session, which we trust. Other than that, this route already does what we need it to.

Comment 15

[Kieran Sedgwick (:sedge)]

Okay, I can deal with that! If that's the case though, I would remove the ":userid" component of the route all together.

Comment 16

[Jon Buckley [:jbuck]]

I would too, but then all of our consumers of this route would break

Comment 17

[Kieran Sedgwick (:sedge)]

Ahhh touché! I'll leave it for backwards compatibility, and cut out the fluff in the implementation.

Comment 18

[Kieran Sedgwick (:sedge)]

Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

I cut out the userid stuff and touched up the readme.

Comment 19

[Jon Buckley [:jbuck]]

Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

r-, notes in pull request

Comment 20

[Kieran Sedgwick (:sedge)]

Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

https://github.com/mozilla/node-webmaker-loginapi/pull/14

Updated readme, fixed async return issues

Comment 21

[Jon Buckley [:jbuck]]

Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

r+ with nits, see PR