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

Hapi not rejecting non-URL-encoded data on route that allows 'application/x-www-form-urlencoded' content type #3422

Closed
memughal opened this issue Jan 9, 2017 · 5 comments
Assignees
Labels
non issue Issue is not a problem or requires changes

Comments

@memughal
Copy link

memughal commented Jan 9, 2017

I am trying the following test and I am sending non-urlencoded data to a route that 'allows' url encoded data. Since content type of this request is set to 'application/x-www-form-urlencoded', it should encode the payload but it does not.

            context('with invalid payload (not URL-encoded)', () => {
                it('returns 400', () => {
                    return server.inject({
                        url: '/books',
                        method: 'POST',
                        headers: {
                            'Content-Type': 'application/x-www-form-urlencoded',
                            Authorization: internals.headers.Authorization
                        },
                        payload: 'title=💣'
                    }).then(res => {
                        expect(res.statusCode).to.equal(400);
                    });
                });
            });

and the following routes config:

       this.server.route({
            method: 'POST',
            path: '/books',
            config: {
                handler: (request, reply) => {
                    this.handleBooks(request, response);
                },
                description: 'Create a new book',
                payload: {
                    allow: 'application/x-www-form-urlencoded'
                },
                validate: {
                    payload: {
                        title: Joi.string().required().description('Title of the new book')
                    }
                }
            }
        })

This results in 200 status code instead of 400, since routes should not let users post not encoded characters by default.

Context

  • node version: v7.1.0
  • hapi version: 16.0.2
  • os: MacOS Sierra v 10.12
@hueniverse
Copy link
Contributor

We use node's querystring to parse the payload. If it passes that, we use it. While you are correct that this payload violates the spec, I don't see a point in working to restrict that.

@hueniverse hueniverse self-assigned this Jan 10, 2017
@hueniverse hueniverse added the non issue Issue is not a problem or requires changes label Jan 10, 2017
@nmoskopp
Copy link

nmoskopp commented Jan 10, 2017

@hueniverse Every piece of software that takes input contains a de facto recognizer for accepting valid input and rejecting invalid input. Parser differentials – when two programs parse things differently, one accepting data and another rejecting it – silently invalidate assumptions programmers have about data safety and can lead to security issues. I think a good example of what this can result in is the Android master key vulnerability.

For more information why accepting invalid input is a bad idea, read The Seven Turrets of Babel and other LANGSEC papers.

@nmoskopp
Copy link

nmoskopp commented Jan 10, 2017

@hueniverse please reopen.

@sirgallifrey
Copy link
Contributor

Some relevant info on querystring:

querystring.unescape() v7.4.0

The querystring.unescape() method is used by querystring.parse() and is generally not expected to be used directly. It is exported primarily to allow application code to provide a replacement decoding implementation if necessary by assigning querystring.unescape to an alternative function.
By default, the querystring.unescape() method will attempt to use the JavaScript built-in decodeURIComponent() method to decode. If that fails, a safer equivalent that does not throw on malformed URLs will be used.

So in theory you can make your own querystring.unescape implementation which will not fallback to a safer decode. https://github.com/nodejs/node/blob/master/lib/querystring.js#L90

But I don't know if this you give the 400 error you are expecting or a 500 o even more nasty error...
This is something you cant test or investigate... And one more thing, if you want this behavior only in routes with allow: 'application/x-www-form-urlencoded' then I don't know if this is a good workaround...

@hueniverse
Copy link
Contributor

@nmoskopp I don't make decision based on general concepts. If you want to make the case we should write our own query string parser, you would need to be a lot more specific.

The reasons I closed this issue are:

  • I have no interest in writing my own query string parser.
  • By using node's parser as-is, we are matching the expectations of node developers already familiar with it. Those expectations are far more important to meet than comparing a hapi server to some completely different stack.
  • Clients can still pass invalid inputs properly encoded (e.g. 'title=%F0%9F%92%A3'). The result request.payload will be identical. Your application still has to perform the exact same validation logic.

If you really need a spec compliant parser you can solve this issue by writing your own. I am happy to add a note to the documentation to state that the node parser is very flexible in its interpretation.

And last, if you feel strongly about this, I would suggest opening an issue directly with node core. After all, it's their issue.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
non issue Issue is not a problem or requires changes
Projects
None yet
Development

No branches or pull requests

4 participants