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

[HttpClient] strengthen bearer validation #30561

Merged
merged 1 commit into from Mar 15, 2019

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 14, 2019

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Better be sure CR/LF/etc cannot be passed inside raw header values, opening potential security risks.

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 14, 2019

In my experience, when using bearer tokens and Guzzle I passed the token "as is". Will this change require us to do a manual base64_encode($token) whenever we want to use this Bearer authentication?

It looks cumbersome and too low-level. Also, Basic authentication is base64-encoded when including it as a HTTP header, but we don't force users to do a base64_encode($user) and base64_encode($password), right? This asymmetry looks confusing.

I agree with Nico's main argument about potential security issues ... but we can instead throw an error when the token doesn't include "normal" characters.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 14, 2019

we can instead throw an error when the token doesn't include "normal" characters.

that's exactly what happens here. the RFC says that only some characters are allowed, so this is what the regexp checks, only that.

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 14, 2019

@nicolas-grekas ah sorry! My confusion then is that the error message says that "this must be a base-64 encoded string" ... but that's "not true", at least not always. Maybe Kévin proposal is better? Thanks.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-bearer branch from fabbac1 to e6e1620 Mar 14, 2019

@nicolas-grekas nicolas-grekas merged commit e6e1620 into symfony:master Mar 15, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Mar 15, 2019

minor #30561 [HttpClient] strengthen bearer validation (nicolas-grekas)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[HttpClient] strengthen bearer validation

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Better be sure CR/LF/etc cannot be passed inside raw header values, opening potential security risks.

Commits
-------

e6e1620 [HttpClient] strengthen bearer validation

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:hc-bearer branch Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.