Repository navigation
fix: refuse cross-host redirects in OAuth token-exchange POST - #2224
Merged
Merged
Conversation
PostOAuthToken's request body carries subject_token (the caller's login JWT, or a jurisdiction-scoped exchange token) as a POST form field, not a header. Go's default http.Client only strips sensitive *headers* on a cross-host redirect (net/http's shouldCopyHeaderOnRedirect) — it copies the request body unconditionally regardless of host change. A malicious or compromised token-exchange endpoint issuing a 307/308 redirect could therefore have the CLI hand its login JWT to a different origin. Guard once in PostOAuthToken (the shared choke point both auth/cell_data_api.go's jurisdiction exchange and internal/coreapi/cross_juris_transport.go's cross-jurisdiction exchange already call through) rather than requiring every constructed *http.Client to opt in individually. Mirrors the same rejectCrossHostRedirect pattern cmd/entire/cli/api/client.go already uses for its own client construction. Added a two-server reproduction test proving the token never reaches a different host; verified it fails against the pre-fix code before confirming it passes with the fix.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new redirect test has an unsynchronized shared boolean accessed across goroutines, which will race under -race and should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the shared OAuth token exchange helper to prevent sensitive POST form data (notably subject_token) from being forwarded to a different origin via 307/308 redirects, addressing a redirect-handling gap in Go’s default http.Client behavior when secrets live in the request body.
Changes:
- Add a redirect guard (
CheckRedirect) inPostOAuthTokento refuse cross-host redirects while still allowing same-host redirects. - Add a regression test that reproduces a cross-host 307 redirect attempt using two
httptest.Serverinstances.
File summaries
| File | Description |
|---|---|
| internal/entireclient/httputil/oauth.go | Enforces same-host redirect policy for OAuth token POSTs by wrapping the provided http.Client with a guarded CheckRedirect. |
| internal/entireclient/httputil/oauth_test.go | Adds a two-server redirect reproduction test to ensure subject_token never reaches a different host. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses the two review comments on #2224, plus one they missed. The shallow-copy rationale said it avoided "overriding any redirect policy already set on" the client, which is the opposite of what the copy does: the guard takes precedence for this request. Say what the copy actually buys (the caller's shared instance is not mutated) and note the precedence, since a caller needing its own policy here wants a same-host-preserving hook rather than the guard removed. No caller sets CheckRedirect today. attackerSawToken is written on the handler goroutine and read on the test goroutine, so make it atomic.Bool. It does not race in practice -- 600 runs under -race across -cpu=1,4,8 report nothing, with and without the guard, because the handler never executes while the guard works -- but atomic keeps a future regression surfacing as a failed assertion instead of a race report. The leak check now runs BEFORE require.Error. It is the assertion that pins this test's headline claim, and require aborts, so in the old order every regression failed on the error assertion alone and never evaluated whether the token had reached the attacker. Verified by mutation: with the guard removed both assertions now fire; previously only the error one did. (assert.Error would be the other way to fix this, but testifylint's require-error rule forbids it.) Production change is comment-only. fmt + lint clean; package green 150x under -race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1H8GRDWRA2DR32PA79R4G4K
pjbgf
approved these changes
Sep 2, 2026
Soph
added a commit
that referenced
this pull request
Sep 7, 2026
Follow-up cleanup on the two preceding commits, from /simplify. internal/entireclient/httputil was left holding a single const plus a paragraph describing the RFC 8693 implementation that no longer lives there — the residue of a deletion that stopped one file early. Its three importers are gone: package auth already aliased the const as oauthClientID (so cell_data_api.go was the only site in the package reaching around its own alias), and internal/coreapi already imports cmd/entire/cli/auth, so exporting auth.OAuthClientID adds no package edge. The const now sits in provider.go's block beside oauthTokenPath, which is where the rest of the OAuth wiring is. exchangeJurisdictionToken takes an http.RoundTripper rather than an *http.Client. It only ever used the client's Transport, and the nil check guarding that was unreachable — every caller routes through cellExchangeHTTPClient, whose every branch returns a non-nil client. The signature now states what the function needs, so "the Timeout does not carry over" is true by construction rather than by comment. The coreapi test keeps the #2224/#2235 rationale, which is the load- bearing half, and loses the paragraph restating what its own name and its sibling's already say. Its third assertion went too: an attacker-issued token can only reach the core if the redirect was followed, which the first assertion already catches, so it was a second spelling of the headline claim rather than independent coverage. Its doc comment is also trimmed. Measured against the rest of the repo the rationale here had outgrown its neighbours, and the narrative it carried is already in the PR body; what stays is the part a reader of this function cannot recover elsewhere. Folded in while doing that: a note that sts narrows plain HTTP to loopback on top of AllowInsecureHTTP, so that is the effective policy regardless of --insecure-http-auth — the line otherwise reads as permitting more than it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1XPFQQK00625TT7SRA41VSY
Soph
added a commit
that referenced
this pull request
Sep 11, 2026
The existing TestRoundTripper_RejectsOffOrigin401ExchangeURL covers a hint that names a foreign host, which validateExchangeURL refuses up front. This is the case that check cannot see: an impeccable hint — same origin as the 401, https-or-loopback, path exactly /oauth/token — whose origin then answers the exchange POST with a 307 pointing elsewhere. Validation has already passed by then, so the only thing between the user's login JWT and the redirect target is the exchange client's redirect policy. net/http strips sensitive headers on a cross-host redirect but replays the body, and the body is where subject_token lives. The guard itself belongs in auth-go, but the test belongs here, because "the CLI reaches a guarded client" is a property of the CLI's wiring, not of the library: #2224 added the guard to httputil.PostOAuthToken, and #2235 moved this path off that helper twelve hours later without anyone noticing, precisely because #2224's regression test sits in httputil and kept passing. Anchored where the CLI actually makes the call, this fails against auth-go without the fix — reporting both the leaked JWT and the attacker-issued token being presented to the core. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1KDHGDWNKKJEEA6ZFSB4Z0A
Soph
added a commit
that referenced
this pull request
Sep 11, 2026
Follow-up cleanup on the two preceding commits, from /simplify. internal/entireclient/httputil was left holding a single const plus a paragraph describing the RFC 8693 implementation that no longer lives there — the residue of a deletion that stopped one file early. Its three importers are gone: package auth already aliased the const as oauthClientID (so cell_data_api.go was the only site in the package reaching around its own alias), and internal/coreapi already imports cmd/entire/cli/auth, so exporting auth.OAuthClientID adds no package edge. The const now sits in provider.go's block beside oauthTokenPath, which is where the rest of the OAuth wiring is. exchangeJurisdictionToken takes an http.RoundTripper rather than an *http.Client. It only ever used the client's Transport, and the nil check guarding that was unreachable — every caller routes through cellExchangeHTTPClient, whose every branch returns a non-nil client. The signature now states what the function needs, so "the Timeout does not carry over" is true by construction rather than by comment. The coreapi test keeps the #2224/#2235 rationale, which is the load- bearing half, and loses the paragraph restating what its own name and its sibling's already say. Its third assertion went too: an attacker-issued token can only reach the core if the redirect was followed, which the first assertion already catches, so it was a second spelling of the headline claim rather than independent coverage. Its doc comment is also trimmed. Measured against the rest of the repo the rationale here had outgrown its neighbours, and the narrative it carried is already in the PR body; what stays is the part a reader of this function cannot recover elsewhere. Folded in while doing that: a note that sts narrows plain HTTP to loopback on top of AllowInsecureHTTP, so that is the effective policy regardless of --insecure-http-auth — the line otherwise reads as permitting more than it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1XPFQQK00625TT7SRA41VSY
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.
https://entire.io/gh/entireio/cli/trails/1206
Summary
PostOAuthToken's request body carriessubject_token(the caller's login JWT, or a jurisdiction-scoped exchange token) as a POST form field, not a header. Go's defaulthttp.Clientonly strips sensitive headers on a cross-host redirect (net/http'sshouldCopyHeaderOnRedirect) — it copies the request body unconditionally regardless of host change. A malicious or compromised token-exchange endpoint issuing a 307/308 redirect could therefore have the CLI hand its login JWT to a different origin.PostOAuthToken(auth/cell_data_api.go's jurisdiction exchange,internal/coreapi/cross_juris_transport.go's cross-jurisdiction exchange) construct their own*http.Clientwith noCheckRedirect, so this affected both.PostOAuthTokenitself) rather than requiring every caller's client construction to opt in — mirrors therejectCrossHostRedirectpatterncmd/entire/cli/api/client.goalready uses for its own client.Verification
TestPostOAuthToken_RefusesCrossHostRedirect: a genuine two-httptest.Serverreproduction (origin redirects to attacker via 307) asserting the attacker server never receivessubject_token.mise run check(fmt + lint + test:ci) passes.Test plan
mise run checkgreen