fix: stop OAuth2Extractor from letting a non-Bearer header shadow access_token#512
Open
ultramcu wants to merge 1 commit into
Open
fix: stop OAuth2Extractor from letting a non-Bearer header shadow access_token#512ultramcu wants to merge 1 commit into
ultramcu wants to merge 1 commit into
Conversation
…ess_token OAuth2Extractor used AuthorizationHeaderExtractor, which returns any non-empty Authorization header value (its "Bearer " prefix strip is a no-op for other schemes). Because MultiExtractor stops at the first non-empty token, a present but non-Bearer header (e.g. "Basic ...") was returned as the token and the documented 'access_token' fallback only ran when the Authorization header was entirely absent. A request carrying both HTTP Basic auth and a valid ?access_token=<jwt> therefore failed to authenticate, with a confusing "invalid number of segments" error. Use BearerExtractor as the first sub-extractor: it returns ErrNoTokenInRequest for a non-Bearer header, so MultiExtractor correctly falls through to access_token. Adds TestOAuth2Extractor covering precedence and fallthrough.
oxisto
reviewed
May 25, 2026
| // that it does not shadow a valid 'access_token' argument. | ||
| var OAuth2Extractor = &MultiExtractor{ | ||
| AuthorizationHeaderExtractor, | ||
| BearerExtractor{}, |
Collaborator
There was a problem hiding this comment.
where is BearerExtractor defined?
Author
There was a problem hiding this comment.
BearerExtractor is the existing exported type in this package — request/extractor.go:84 (type BearerExtractor struct{}, with an ExtractToken method). I reused it here precisely because it returns ErrNoTokenInRequest when the Authorization header isn't Bearer …, so MultiExtractor falls through to the access_token form value instead of being shadowed by a non-Bearer header (e.g. Basic …). The previous code used stripBearerPrefixFromTokenString, which returns the header unchanged for a non-bearer value, which is what stopped the fallthrough.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
OAuth2Extractoris documented as "looks in 'Authorization' header then 'access_token' argument for a token". In practice, a present but non-BearerAuthorizationheader silently shadows a validaccess_token:The cause is the composition:
AuthorizationHeaderExtractor'sstripBearerPrefixFromTokenStringreturns the header value unchanged when there is no"Bearer "prefix, andMultiExtractorstops at the first non-empty token, so theaccess_tokenfallback only runs when theAuthorizationheader is entirely absent. A request that carries both HTTP Basic auth (e.g. a browser re-sending cached credentials, or a proxy) and a valid?access_token=<jwt>therefore fails to authenticate, surfacing a confusingtoken is malformed: token contains an invalid number of segmentserror.Fix
Use
BearerExtractor{}as the first sub-extractor. It already returnsErrNoTokenInRequestfor a non-Bearer header, soMultiExtractorcorrectly falls through toaccess_token:Behavior change
A token placed directly in the
Authorizationheader without theBearerscheme prefix is no longer extracted byOAuth2Extractor(it previously was, as a side effect of the lenient strip). This matches RFC 6750, which mandates theBearerscheme for OAuth2 access tokens. A realBearer <token>header is unaffected, andAuthorizationHeaderExtractoritself is unchanged for anyone using it directly.Tests
Adds
TestOAuth2Extractorcovering: Bearer header, Bearer precedence overaccess_token,access_tokenfallback with no header, and the regression cases — a non-Bearer header (Basic ...,Token ...) falling through toaccess_token, and a non-Bearer header with noaccess_tokenreturningErrNoTokenInRequest. The fallthrough cases fail before this change and pass after.go test -race ./...,go vet ./...andgofmtare clean; no existing test changed.