Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect handling of complex & duplicate x-www-form-urlencoded parameters #20

Open
astiob opened this issue Jun 7, 2019 · 0 comments

Comments

@astiob
Copy link

astiob commented Jun 7, 2019

RFC 5849 §3.4.1.3.1 mandates that if the HTTP request contains a single valid application/x-www-form-urlencoded entity-body, its contents be parsed into key/value pairs and added to the list of parameter sources that are covered by the OAuth 1 signature.

Currently, passport-http-oauth checks whether the Content-Type is application/x-www-form-urlencoded and uses req.body if it is. This requires a separate middleware to be present in the chain to decode the entity-body to req.body. Furthermore, this requires said middleware to decode it to a simple list of key/value pairs to follow the spec. Unfortunately, the common body-parser middleware urlencoded() does not do this.

Consider:

POST /test HTTP/1.0
Content-Type: application/x-www-form-urlencoded
Content-Length: 15

foo=bar&foo=baz
const express = require('express')
const passport = require('passport')
const {ConsumerStrategy, TokenStrategy} = require('passport-http-oauth')

passport.use('consumer', new ConsumerStrategy(
	(consumerKey, done) => done(null, {}, 'secret'),
	(requestToken, done) => done(null, false),
))

const app = express()
app.post('/test',
	express.urlencoded({extended: true/false}),
	passport.authenticate('consumer', { session: false }),
	(req, res) => res.json('ok'))

As documented in the Node.js docs, querystring.parse, which is called if {extended: false}, parses this request body as:

req.body = {'foo': ['bar', 'baz']}

As a result, passport-http-oauth attempts to sign the string ...%26foo%3Dbar%252Cbaz instead of the expected ...%26foo%3Dbar%26foo%3Dbaz, and authentication fails when it should succeed (and succeeds when it should fail, if the client does the same!).

qs.parse, which is called if {extended: true}, doesn’t explicitly document this in the README, but as far as I can tell from the source code, it does the same and this cannot be disabled. Furthermore, it also parses several special syntaxes for complex objects and arrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant