Handle tab in token authentication header. #14212

Merged
merged 1 commit into from Dec 15, 2015

Conversation

Projects
None yet
3 participants
@tylerhunt
Contributor

tylerhunt commented Feb 26, 2014

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.

Handle tab in token authentication header.
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 robin850 added the actionpack label Feb 26, 2014

@robin850

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Feb 27, 2014

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 ?

Member

robin850 commented Feb 27, 2014

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

This comment has been minimized.

Show comment Hide comment
@tylerhunt

tylerhunt Feb 27, 2014

Contributor

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.

Contributor

tylerhunt commented Feb 27, 2014

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

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Feb 27, 2014

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+ ? 😁

Member

robin850 commented Feb 27, 2014

@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

This comment has been minimized.

Show comment Hide comment
@tylerhunt

tylerhunt Feb 27, 2014

Contributor

@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/.

Contributor

tylerhunt commented Feb 27, 2014

@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

1 check passed

default The Travis CI build passed
Details

sgrif added a commit that referenced this pull request Dec 15, 2015

Merge pull request #14212 from tylerhunt/fix-token-regex
Handle tab in token authentication header.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment