Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpClient] Allow bearer token with colon#38248
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[HttpClient] Allow bearer token with colon#38248
Uh oh!
There was an error while loading.Please reload this page.
Conversation
wouterj commentedSep 20, 2020
Seems like the RFC 6750 bearer format was used here:https://tools.ietf.org/html/rfc6750#section-2.1 But I see no reason to prevent any other token format here, "bearer" isn't directly coupled to OAuth2.0, right? |
| if (isset($options['auth_bearer']) && (!\is_string($options['auth_bearer']) || !preg_match('{^[-._=~+/0-9a-zA-Z]++$}',$options['auth_bearer']))) { | ||
| if (isset($options['auth_bearer']) && (!\is_string($options['auth_bearer']) || !preg_match('{^[-._=:~+/0-9a-zA-Z]++$}',$options['auth_bearer']))) { | ||
| thrownewInvalidArgumentException(sprintf('Option "auth_bearer" must be a string containing only characters from the base 64 alphabet,'.(\is_string($options['auth_bearer']) ?'invalid string given.' :'"%s" given.'),\gettype($options['auth_bearer']))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The message should be updated. It's now the base 64 alphabet with the colon!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reverted, no need to advertise the colon, its non-standard
OskarStark commentedSep 22, 2020
Could you add a testcase? |
stof commentedSep 22, 2020
I think this still deserves reporting to Jetbrains that their OAuth tokens don't actually respect the OAuth stack. But we might indeed be less strict here, accepting more things that servers provide as tokens. |
stephanvierkant commentedSep 22, 2020
|
8fb7e41 to82ed1ecCompare| if (isset($options['auth_bearer']) && (!\is_string($options['auth_bearer']) || !preg_match('{^[-._=~+/0-9a-zA-Z]++$}',$options['auth_bearer']))) { | ||
| if (isset($options['auth_bearer']) && (!\is_string($options['auth_bearer']) || !preg_match('{^[-._=:~+/0-9a-zA-Z]++$}',$options['auth_bearer']))) { | ||
| thrownewInvalidArgumentException(sprintf('Option "auth_bearer" must be a string containing only characters from the base 64 alphabet,'.(\is_string($options['auth_bearer']) ?'invalid string given.' :'"%s" given.'),\gettype($options['auth_bearer']))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reverted, no need to advertise the colon, its non-standard
nicolas-grekas commentedSep 24, 2020
Thank you@stephanvierkant. |
The JetBrains Hub (YouTrack API) creates tokens with a
perm:prefix. This doesn't work right now, because HttpClient doesn't allow a colon in the bearer token.As far as I can see, there is no reason to disallow the use of the semicolon in the bearer token, so this PR fixes it.
Example of a token:
perm:c3RlcGhhbg==.NTUtMw==.NiZw16agafhsQAShTvclhb78hyJh2H