Closed
Bug 917080
Opened 11 years ago
Closed 11 years ago
Don't confuse params for static paths in user/ and username/ routes
Categories
(Webmaker Graveyard :: Login, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: humph, Assigned: sedge)
References
Details
Attachments
(1 file, 3 obsolete files)
We had a case via the MakeAPI where someone was trying to get data from the server by doing bad things, specifically, trying to pass a username like this: /user//../../../../../../../../../../../etc/passwd Here, our Login code gets confused and thinks the extra /s mean it should look for a static route vs. use the entire string as a username. What we should do is take everything after the /user/* or /username/* as the entire username string, and let our validation and such deal with it. In express you do this by saying that a route is: app.get( '/routename/*', ...) Then, to get the value: var username = req.params[ 0 ]; I've got a patch for this that I'll post in a sec, and I probably need someone to shepherd it in tomorrow, since I'll be afk most of the day.
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #805738 -
Flags: review?(kieran.sedgwick)
Attachment #805738 -
Flags: review?(cade)
Reporter | ||
Comment 2•11 years ago
|
||
Test runs shows we might still have some issues with this, see stack traces (not sure what those are about): ✓ should successfully return an account when attempting the retrieve an existing user by id ◦ should error on attempting to retrieve a non-existant account: 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "GET /user/hloflax6@email.com HTTP/1.1" 404 58 "-" "-" ✓ should error on attempting to retrieve a non-existant account ◦ should deal with bogus usernames that look like routes: Error: Forbidden at SendStream.error (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/send/lib/send.js:145:16) at SendStream.pipe (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/send/lib/send.js:310:52) at Object.static (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/middleware/static.js:84:8) at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15) at Object.logger (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/middleware/logger.js:156:5) at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15) at Object.exports.allowCorsRequests [as handle] (/Users/dave/Sites/repos/login.webmaker.org/app/http/controllers/application.js:22:3) at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15) at Object.expressInit [as handle] (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/lib/middleware.js:31:5) at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15) 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "GET /user//../../../../../../../../../../../etc/passwd HTTP/1.1" 403 - "-" "-" ✓ should deal with bogus usernames that look like routes 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "DELETE /user/295 HTTP/1.1" 200 3 "-" "-" 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "DELETE /user/296 HTTP/1.1" 200 3 "-" "-" 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "DELETE /user/297 HTTP/1.1" 200 3 "-" "-" GET /isAdmin/:id info: HTTP server listening on port 3000. ◦ should successfully return when attempting to check an existing user: Sending the welcome email is disabled 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "POST /user HTTP/1.1" 200 415 "-" "-" 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "GET /isAdmin?id=hloflax7@email.com HTTP/1.1" 200 21 "-" "-" ✓ should successfully return when attempting to check an existing user ◦ should error on attempting to check a non-existant account: 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "GET /isAdmin?id=hloflax8@email.com HTTP/1.1" 404 78 "-" "-" ✓ should error on attempting to check a non-existant account 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "DELETE /user/298 HTTP/1.1" 200 3 "-" "-" GET /user/username/:userid info: HTTP server listening on port 3000. ◦ should error if no userid is passed: 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "GET /user/username/ HTTP/1.1" 400 27 "-" "-" ✓ should error if no userid is passed ◦ should return 200 if username in use: Sending the welcome email is disabled 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "POST /user HTTP/1.1" 200 416 "-" "-" 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "GET /user/username/hloflax9 HTTP/1.1" 200 56 "-" "-" ✓ should return 200 if username in use ◦ should return 403 if username is a badword: 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "GET /user/username/damn HTTP/1.1" 403 41 "-" "-" ✓ should return 403 if username is a badword ◦ should return 404 if username is available: 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "GET /user/username/hloflaxa HTTP/1.1" 404 43 "-" "-" ✓ should return 404 if username is available ◦ should deal with bogus usernames that look like routes: Error: Forbidden at SendStream.error (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/send/lib/send.js:145:16) at SendStream.pipe (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/send/lib/send.js:310:52) at Object.static (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/middleware/static.js:84:8) at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15) at Object.logger (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/middleware/logger.js:156:5) at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15) at Object.exports.allowCorsRequests [as handle] (/Users/dave/Sites/repos/login.webmaker.org/app/http/controllers/application.js:22:3) at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15) at Object.expressInit [as handle] (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/lib/middleware.js:31:5) at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15) 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "GET /user/username//../../../../../../../../../../../etc/passwd HTTP/1.1" 403 - "-" "-"
Assignee | ||
Comment 4•11 years ago
|
||
Updated patch based on Dave's work.
Assignee: nobody → kieran.sedgwick
Attachment #805738 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #805738 -
Flags: review?(kieran.sedgwick)
Attachment #805738 -
Flags: review?(cade)
Attachment #806032 -
Flags: review?(jon)
Attachment #806032 -
Flags: review?(cade)
Comment 5•11 years ago
|
||
Comment on attachment 806032 [details] [review] https://github.com/mozilla/login.webmaker.org/pull/162 r+
Attachment #806032 -
Flags: review?(jon)
Attachment #806032 -
Flags: review?(cade)
Attachment #806032 -
Flags: review+
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 806032 [details] [review] https://github.com/mozilla/login.webmaker.org/pull/162 You too please!
Attachment #806032 -
Flags: review?(jon)
Updated•11 years ago
|
Attachment #806032 -
Flags: review?(jon) → review+
Comment 7•11 years ago
|
||
Commits pushed to master at https://github.com/mozilla/login.webmaker.org https://github.com/mozilla/login.webmaker.org/commit/0e3114a20dfb97076ba98ac87f3ad4eaf8c8359d Fix Bug 917080 - Don't confuse params for static paths in user/ and username/ routes https://github.com/mozilla/login.webmaker.org/commit/8748086e16a96589acaa5bb5027928095437f934 Merge branch 'bug917080'
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 8•11 years ago
|
||
Commit pushed to master at https://github.com/mozilla/login.webmaker.org https://github.com/mozilla/login.webmaker.org/commit/bd9dcecb068eedaa615414674ef88d0477ccd4dc Revert "Fix Bug 917080 - Don't confuse params for static paths in user/ and username/ routes" This reverts commit 0e3114a20dfb97076ba98ac87f3ad4eaf8c8359d.
Comment 9•11 years ago
|
||
This patch prevents account deletions from occuring. I get the following error: 127.0.0.1 - - [Wed, 18 Sep 2013 16:45:16 GMT] "POST /account/delete HTTP/1.1" 500 - "http://localhost:9000/account" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0" TypeError: Cannot call method 'match' of undefined at parseQuery (/Users/jon/Sites/login.webmaker.org/app/models/user/sqlController.js:77:40) at Object.getUser (/Users/jon/Sites/login.webmaker.org/app/models/user/sqlController.js:140:27) at Object.getUser (/Users/jon/Sites/login.webmaker.org/app/models/user/index.js:17:17) at controller.del (/Users/jon/Sites/login.webmaker.org/app/http/controllers/user.js:63:16) at callbacks (/Users/jon/Sites/login.webmaker.org/node_modules/express/lib/router/index.js:161:37) at checkPersona (/Users/jon/Sites/login.webmaker.org/app/http/routes.js:82:7) at callbacks (/Users/jon/Sites/login.webmaker.org/node_modules/express/lib/router/index.js:161:37) at /Users/jon/Sites/login.webmaker.org/node_modules/express/node_modules/connect/lib/middleware/csrf.js:56:5 at callbacks (/Users/jon/Sites/login.webmaker.org/node_modules/express/lib/router/index.js:161:37) at param (/Users/jon/Sites/login.webmaker.org/node_modules/express/lib/router/index.js:135:11) Backing it out in the mean time
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 10•11 years ago
|
||
Sorry, I squashed it down. Look for my comment in the PR
Attachment #806773 -
Flags: review?(jon)
Comment 11•11 years ago
|
||
Comment on attachment 806773 [details] https://bugzilla.mozilla.org/show_bug.cgi?id=917080 This is not the attachment I'm looking for :)
Attachment #806773 -
Flags: review?(jon)
Assignee | ||
Comment 12•11 years ago
|
||
Hah. Hahaha. Oh Sedge...
Attachment #806032 -
Attachment is obsolete: true
Attachment #806773 -
Attachment is obsolete: true
Attachment #807193 -
Flags: review?(jon)
Updated•11 years ago
|
Attachment #807193 -
Flags: review?(jon) → review+
Comment 13•11 years ago
|
||
Commit pushed to master at https://github.com/mozilla/login.webmaker.org https://github.com/mozilla/login.webmaker.org/commit/a92facc09acc3d7252d2802e1485f3b1ecd4c1d2 Fix Bug 917080 - Don't confuse params for static paths in user/ and username/ routes
Updated•11 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Attachment mime type: text/plain text/plain text/plain → text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request
You need to log in
before you can comment on or make changes to this bug.
Description
•