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

Handle tab in token authentication header. #14212

Merged
merged 1 commit into from
Dec 15, 2015

Conversation

tylerhunt
Copy link
Contributor

The HTTP spec allows for LWS to precede the header content, which
could include multiple SP and HT characters. Update the regex used to
match the Token authorization header to account for this, instead of
matching on a single SP.

See http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html and
http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2 for the relevant
sections of the specification.

The HTTP spec allows for LWS to precede the header content, which
could include multiple SP and HT characters. Update the regex used to
match the Token authorization header to account for this, instead of
matching on a single SP.

See http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html and
http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html for the relevant
parts of the specification.
@robin850
Copy link
Member

Hello @tylerhunt,

Thanks for the patch but I'm not sure that it is needed. If we try to run the given test without it, it passes. I think Action Pack is actually removing the extra tabs through AUTHN_PAIR_DELIMITERS in the raw_params method but I may be wrong.

@pixeltrix : would you mind having a look please ?

@tylerhunt
Copy link
Contributor Author

Just went back to verify myself, and if I revert change to the TOKEN regex, the test fails for me.

  1) Failure:
HttpTokenAuthenticationTest#test_authentication_request_with_tab_in_header [/[…]/rails/actionpack/test/controller/http_token_authentication_test.rb:94]:
Expected response to be a <success>, but was <401>

Additionally, I've seen an Authorization header coming into Rails from IE 8 that has just such a tab, and it causes the authentication to fail.

@robin850
Copy link
Member

@tylerhunt : Whups, sorry for the noise, I've tested with /^Token/ instead of /^Token / (without a space at the end). Maybe we can simply remove it instead of adding \s+ ? 😁

@tylerhunt
Copy link
Contributor Author

@robin850 The issue there is the potential for false positives with other authentication schemes. For instance, if I were to define a custom scheme called TokenAuth, matching on /^Token/ would cause my Authorization: AuthToken header to be picked up when it shouldn't have been. If you don't want to match the whitespace, I'd suggest using at least something like /^Token\b/.

@sgrif sgrif merged commit d5a0d71 into rails:master Dec 15, 2015
sgrif added a commit that referenced this pull request Dec 15, 2015
Handle tab in token authentication header.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants