security(oauth): encode social-login state instead of concatenating it - #760
Merged
Conversation
/oauth_login built the provider `state` by joining four values with
"___" and /oauth_callback split them back apart. The FIRST of them is
supplied by the caller:
state + "___" + redirectURI + "___" + roles + "___" + scope
A caller whose state contains the delimiter shifts every later field
left. Sending state=A___https://evil.example___admin___openid makes the
callback take its redirect URI from the caller's segment and its ROLES
from the next one.
Two things make that more than untidy:
- The two sides validate roles differently. /oauth_login checks the roles
query param against Config.Roles + ProtectedRoles; the callback checks
only ProtectedRoles, so any role that merely is not marked protected is
accepted. An attacker registering themselves through social login
chooses their own roles.
- AllowedOrigins defaults to ["*"], so an injected redirect URI passes
validation and the code lands wherever the caller asked.
The RFC 9700 browser binding does not help: the state is bound to the
attacker's own browser, and the attack is on their own signup.
The same bug has a benign face that is far more common. A caller's state
is typically base64url, whose alphabet includes "_", so a token that
merely ENDS in one produces "____", splits a character early, and yields
a redirect URI with a leading underscore - a hard "invalid redirect uri"
on roughly 1 in 64 social logins, for every provider. That is what made
the social e2e specs look flaky; the trace showed the mangled state.
The fix is not a better delimiter. NOTHING the caller controls travels to
the provider now: `state` is an opaque 32-byte handle and the four values
live server-side in the state store, which already held an entry per
login. There is no format for a caller to collide with because there is
nothing to parse on the way back.
Encoding the fields instead (base64 per field, joined by a character
outside the alphabet) would also have closed the injection, but it
inflates the state by ~28% - and X/Twitter documents a 100-character
limit on `state`, which a realistic redirect URI already approaches. A
fixed 43-character handle is shorter than what this server sent before,
so no provider limit gets closer. It also stops the redirect URI, roles
and scope passing through a third party at all.
Decoding fails closed on any store entry this server did not write,
including the previous release's bare provider name.
BREAKING: states issued by an older instance are not accepted by a new
one. Social logins in flight across a rolling deploy fail and must be
retried. Keeping the old parser as a fallback would keep the
vulnerability, so it is not accepted.
lakhansamani
force-pushed
the
security/oauth-state-encoding
branch
from
August 13, 2026 07:34
3b45c9b to
03fc8c0
Compare
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.
Fixes a field-injection flaw in the social-login
state, present onmain(not introduced by #758).The flaw
/oauth_loginbuilt the providerstateby concatenating four values, and/oauth_callbacksplit them back apart:The first field is caller-supplied. A caller whose state contains the delimiter shifts every later field left, so the callback reads:
[1]redirect URI[2]rolesTwo details turn that from untidy into exploitable:
oauth_login.gochecks therolesquery parameter againstConfig.Roles + ProtectedRoles. The callback checksinputRolesonly againstProtectedRoles— so any role that merely isn't marked protected is accepted. Someone registering themselves through social login picks their own roles.AllowedOriginsdefaults to["*"], so an injected redirect URI passes validation and the authorization code is delivered wherever the caller asked.The RFC 9700 browser-binding check added earlier does not mitigate this: the state is bound to the attacker's own browser, and the attack is against their own signup.
The same bug, benign face
A caller's
stateis typically base64url, whose alphabet includes_. A token that merely ends in one yields____, splits a character early, and produces a redirect URI with a leading underscore — a hardinvalid redirect urion roughly 1 in 64 social logins, for every provider.That is what made the social e2e specs look flaky. It was diagnosed from a Playwright trace showing the mangled state:
→
redirectURL = "_http://authorizer:8080/app"→ 400.The fix
Each field is base64url-encoded before joining with
., which is outside the base64url alphabet — so no encoded field can contain the delimiter, whatever the caller sends. Decoding requires exactly four fields and fails closed; a lenient parse is precisely how caller input became privileges.New
internal/http_handlers/oauth_state.go, wired into both handlers. The___format is gone.Tests
oauth_state_test.gocovers the adversarial shapes directly — a token ending in_, a token containing___, and a token forging every following field — each asserting the redirect URI and roles still come from the server. Plus malformed-input rejection and URL-safety of the encoding.Verification
go build/go vet/make lint(0 issues)internal/http_handlersand the integration suite passStates issued by an older instance are not accepted by a newer one, and vice versa. Social logins in flight across a deploy fail and must be retried — states are short-lived, so the window is small. Keeping the old parser as a fallback would keep the vulnerability, so it is deliberately not accepted.