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

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cade, Assigned: sedge)

Tracking

Details

(Whiteboard: u=dev c=login p=1 s=2013w23)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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
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.
(Reporter)

Comment 2

5 years ago
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.
(Reporter)

Comment 3

5 years ago
here's how it changed my cookie: http://dl.dropbox.com/u/86269810/Screenshots/z~2b.png
(Reporter)

Comment 4

5 years ago
( It should be ["username":"cade", ..} )
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
(Reporter)

Comment 7

5 years ago
(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

5 years ago
Yeah, I think that sounds sane. Put it on me for review when you've done that
(Assignee)

Updated

5 years ago
Assignee: nobody → kieran.sedgwick
Summary: Remove the /user/:id route from webmaker-loginapi → Secure the /user/:id route in webmaker-loginapi
Whiteboard: u=dev c=login p=1 s=2013w23
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

5 years ago
Created attachment 758621 [details] [review]
[WIP] Pull request for this patch

https://github.com/mozilla/node-webmaker-loginapi/pull/14
Attachment #758621 - Flags: review?(jon)
Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

r-, notes in the pull request
Attachment #758621 - Flags: review?(jon) → review-
(Assignee)

Comment 11

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

https://github.com/mozilla/node-webmaker-loginapi/pull/14
Attachment #758621 - Flags: review- → review?(jon)
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.
Attachment #758621 - Flags: review?(jon) → review-
(Assignee)

Comment 13

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

https://github.com/mozilla/node-webmaker-loginapi/pull/14
Attachment #758621 - Flags: review- → review?(jon)
Created attachment 758816 [details]
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.

Updated

5 years ago
Attachment #758621 - Flags: review?(jon) → review-
(Assignee)

Comment 15

5 years ago
Okay, I can deal with that! If that's the case though, I would remove the ":userid" component of the route all together.
I would too, but then all of our consumers of this route would break
(Assignee)

Comment 17

5 years ago
Ahhh touché! I'll leave it for backwards compatibility, and cut out the fluff in the implementation.
(Assignee)

Comment 18

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

I cut out the userid stuff and touched up the readme.
Attachment #758621 - Flags: review- → review?(jon)
Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

r-, notes in pull request
Attachment #758621 - Flags: review?(jon) → review-
(Assignee)

Comment 20

5 years ago
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
Attachment #758621 - Flags: review- → review?(jon)
Comment on attachment 758621 [details] [review]
[WIP] Pull request for this patch

r+ with nits, see PR
Attachment #758621 - Flags: review?(jon) → review+
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.