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

Improved validation of route method label #2402

Merged
merged 1 commit into from Feb 16, 2015

Conversation

@kanongil
Copy link
Contributor

@kanongil kanongil commented Feb 11, 2015

This patch improves route method option handling in 3 ways:

  1. It fixes a bug in the validation which allows a nested array (eg. ['GET', ['POST']]) but later crashes when creating the route.
  2. It enforces the non-HEAD method restriction from the API docs.
  3. It limits the allowed value to token characters from https://tools.ietf.org/html/rfc7230#section-3.2.6.

Finally, I have included a couple of tests for HEAD responses, validating the existing implementation.

@@ -35,11 +35,13 @@ exports = module.exports = internals.Route = function (options, connection, real
Hoek.assert(options.handler || (options.config && options.config.handler), 'Missing or undefined handler:', options.method, options.path);
Hoek.assert(!!options.handler ^ !!(options.config && options.config.handler), 'Handler must only appear once:', options.method, options.path); // XOR
Hoek.assert(options.path === '/' || options.path[options.path.length - 1] !== '/' || !connection.settings.router.stripTrailingSlash, 'Path cannot end with a trailing slash when connection configured to strip:', options.method, options.path);
Hoek.assert(/^[a-zA-Z0-9!#\$%&'\*\+\-\.^_`\|~]+$/.test(options.method), 'Invalid method name:', options.method, options.path);
Copy link
Contributor

@arb arb Feb 11, 2015

Wouldn't it be faster and probably easier to read to just have a method object and reference that?

var _methdods = { get: true, post: true...}
Hoek.assert(_methods[options.method.toLowerCase()])

or something along those lines?

Copy link
Contributor Author

@kanongil kanongil Feb 11, 2015

I don't know why you are concerned about speed in route creation. As for whitelisting, that would make sense as a future option but right now I think it is a breaking change.

This patch is mainly concerned with catching method names that should never be allowed, like GET /.

@hueniverse hueniverse added the bug label Feb 16, 2015
@hueniverse hueniverse added this to the 8.2.1 milestone Feb 16, 2015
@hueniverse hueniverse self-assigned this Feb 16, 2015
hueniverse pushed a commit that referenced this issue Feb 16, 2015
Improved validation of route method label
@hueniverse hueniverse merged commit 9317fd1 into hapijs:master Feb 16, 2015
1 check passed
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants