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

No longer allow lowercase HTTP methods #170

Merged
merged 1 commit into from Aug 16, 2017
Merged

Conversation

@bertjwregeer
Copy link
Member

@bertjwregeer bertjwregeer commented Aug 16, 2017

RFC 7231 specifies that methods are case sensitive, and by convention are all uppercase: https://tools.ietf.org/html/rfc7231#section-4.1

This specification defines a number of standardized methods that are
commonly used in HTTP, as outlined by the following table. By
convention, standardized methods are defined in all-uppercase
US-ASCII letters.

https://tools.ietf.org/html/rfc7231#section-8.1 specifies that there is a registry for all of the HTTP methods that exist, which is available here: https://www.iana.org/assignments/http-methods/http-methods.xhtml (in other news, there are a lot more there than I was aware of :P)

All of them are uppercase.

I recently ran into an interesting issue, I was using the new fetch() function in JavaScript and it allowed me to send a post request instead of a POST request. nginx dropped the request on the floor, but waitress locally was allowing it by just calling .upper() on the method.

This change brings things more in line with all other HTTP servers, and disallows anything put an uppercase HTTP method thereby hopefully making it harder to have differences between prod and devel...

@bertjwregeer
Copy link
Member Author

@bertjwregeer bertjwregeer commented Aug 16, 2017

Closes #154

@bertjwregeer bertjwregeer requested a review from mmerickel Aug 16, 2017
Copy link
Member

@mmerickel mmerickel left a comment

As a counter point <form method=post> is valid... but that's the html spec, not http. Anyway I don't think it's important for waitress to accept non-compliant verbs.

command = m.group(1).upper()
method = m.group(1)

# the

This comment has been minimized.

@mmerickel

mmerickel Aug 16, 2017
Member

This comment could use a couple more iterations and design meetings.

This comment has been minimized.

@bertjwregeer

bertjwregeer Aug 16, 2017
Author Member

Seems I forgot to commit after adding the comment. Done now.


# the
if method != method.upper():
raise ParsingError('Malformed HTTP method "%s"' % tostr(method))

This comment has been minimized.

@mmerickel

mmerickel Aug 16, 2017
Member

Is there a reason to force it to be upper specifically instead of just passing it through untouched?

This comment has been minimized.

@bertjwregeer

bertjwregeer Aug 16, 2017
Author Member

Yeah, nginx and other servers drop the incoming request on the floor. This caused me a lot of headache and debugging issues when I was testing with waitress locally and using nginx in production.

All currently defined HTTP methods are uppercase, I don't think it is a bad idea to enforce that.

@bertjwregeer bertjwregeer force-pushed the bugfix/uppercase_verb branch from 70e916f to e3ad2a3 Aug 16, 2017
@bertjwregeer
Copy link
Member Author

@bertjwregeer bertjwregeer commented Aug 16, 2017

As a counter point <form method=post>

Yes, and browsers unconditionally turn that into a POST request. The HTML standard specifies that get and post map to GET and POST respectively: https://www.w3.org/TR/html5/forms.html#attr-fs-method

@bertjwregeer bertjwregeer merged commit c18aa5e into master Aug 16, 2017
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@bertjwregeer bertjwregeer deleted the bugfix/uppercase_verb branch Aug 16, 2017
clrpackages pushed a commit to clearlinux-pkgs/waitress that referenced this pull request Oct 11, 2017
…1.1.0

1.1.0 (2017-10-10)
------------------

Features
~~~~~~~~

- Waitress now has a __main__ and thus may be called with ``python -mwaitress``

Bugfixes
~~~~~~~~

- Waitress no longer allows lowercase HTTP verbs. This change was made to fall
  in line with most HTTP servers. See Pylons/waitress#170

- When receiving non-ascii bytes in the request URL, waitress will no longer

(NEWS truncated at 15 lines)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.