fix: prevent open redirect in proxy authentication flow - #26647
Conversation
… open redirect Adds TestHandleSubdomain_APIKeySmuggling_NoOpenRedirect, which mints a valid smuggled API key and drives HandleSubdomain with attacker-controlled paths that survive to r.URL.Path: //evil.com/..., /\evil.com/..., and a leading tab /<tab>/evil.com/.... After the key is consumed, HandleSubdomain issues a 303 redirect built from the uncleaned path, so the Location header resolves to an off-origin host once a browser normalizes tabs and backslashes. The leading-tab case is a real bypass over HTTP/1: http.Redirect emits the raw tab verbatim (url.Parse rejects the control byte and skips cleaning) and the header writer does not sanitize tabs. TestHandleSubdomain_APIKeySmuggling_PreservesPath locks in that legitimate paths still round-trip and that unrelated query params survive while the smuggled key is stripped. The security test fails on the current code. Refs ANT-2026-22456.
… path handleAPIKeySmuggling consumed the smuggled subdomain API-key query param, set the app session cookie, then issued a 303 redirect back to the same request. The target was built from the uncleaned, attacker-controlled, already-decoded r.URL.Path. http.Redirect parses the target with url.Parse, which treats a leading "//" as a scheme-relative URL with a host, and emits the target verbatim when url.Parse rejects it (e.g. a decoded control byte). Browsers additionally strip tab and newline characters and normalize a leading "/\" before resolving. So //evil.com, /\evil.com, and /<tab>/evil.com all turned a trusted app subdomain into an open redirect (the slash-collapsing middleware writes to chi's RoutePath, not r.URL.Path, so the raw path survived). Collapse any leading run of "/" and "\" into a single "/", then build the redirect through url.URL so the path is escaped: control bytes, backslashes, "?" and "#" become percent-encoded and can no longer introduce an authority, query, or fragment. The redirect now stays same-origin for every input. Fixes ANT-2026-22456.
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 4 | Last posted: Round 4, 6 findings (2 P3, 4 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledgedCRF-3 (Nit, proxy_test.go:1) - Commit subjects exceed 72-character limit
Round logRound 1Panel (18 reviewers). 0 P0, 0 P1, 2 P3, 2 Nit, 1 Note. 10 Gon P2 comment findings dropped (Leorio praised same comments). Reviewed against 51591e3..43f62ba. Round 2BLOCKED. CRF-3 (Nit, commit subject length) silent. CRF-1, CRF-2, CRF-4 addressed. No review. Round 3PROCEED. CRF-3 contested, panel closed (8/8 accept). CRF-1, CRF-2, CRF-4 verified fixed. Netero: 1 Nit (CRF-7). Panel: 1 new Nit (CRF-8). Reviewed against 51591e3..1ba1fe4. Round 4PROCEED. CRF-7, CRF-8 addressed. All findings resolved. Panel (10 reviewers): 0 new findings. Reviewed against 51591e3..550616c. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
A deep review of ANT-2026-22456 found the same open-redirect class in two more redirects in proxyWorkspaceApp that build the Location from the raw, attacker-controlled r.URL.Path: the trailing-slash redirect (r.URL.Path+"/") and the default-query redirect (r.URL.String()). These are reachable for coderd path apps because singleSlashMW cleans chi's RoutePath while leaving r.URL.Path raw, so a request like //@user/ws/apps/app can match the route yet redirect to a scheme-relative //@user/... Location (off-origin once a browser resolves it). Extract the same-origin normalization into originLocalPath and route all three redirects (including the API-key smuggling one) through it, building each Location via url.URL so control characters are escaped. Add an internal unit test covering the helper; the existing RedirectsWithSlash/RedirectsWithQuery integration tests confirm legitimate redirects are unchanged. Refs ANT-2026-22456.
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped security fix. The two-layer defense (TrimLeft for slash/backslash collapsing + url.URL.String() for control-character encoding) is the right approach, and both layers are independently necessary: TrimLeft alone misses the tab-stripping bypass, url.URL alone passes "//evil.com" verbatim. 18 reviewers found no correctness or security issues.
Severity count: 2 P3, 2 Nit.
"I tried to build a case against this change and couldn't. The problem is correctly understood, the solution is proportional, and the fix is at the right level." (Pariston)
coderd/workspaceapps/proxy_test.go:1
Nit [CRF-3] Both commit subjects exceed the 72-character limit (fix: 74, test: 83). Shorter alternatives:
fix(coderd/workspaceapps): prevent open redirect via API-key smuggling(71)test(coderd/workspaceapps): regression test for API-key open redirect(70)
(Leorio)
🤖
🤖 This review was automatically generated with Coder Agents.
Match the Go convention of naming an unexported-function test Test_<func> rather than Test<Func>. Refs ANT-2026-22456.
sreya
changed the title
Address review feedback on the API-key smuggling redirect tests: - Collapse newSmugglingTestServer/doSmugglingRequest and the two table-driven HandleSubdomain tests into a single focused wiring test constructed inline, matching the sibling TestHandleSubdomain_IgnoresUntrustedForwardedHost. There is no lighter shared fixture, and the full app test harness cannot model an attacker path that bypasses the slash-cleaning middleware, so the inline setup is the minimal option. - Move the exhaustive path-sanitization matrix to Test_originLocalPath (no server needed) and add an EscapesControlCharacters case asserting tab, newline, and CR are percent-encoded (%09/%0A/%0D), so a regression in any single control-character class is caught. Refs ANT-2026-22456.
Fold the API-key smuggling redirect test into a single TestHandleSubdomain parent alongside the forwarded-host test, trim the verbose comments, and drop the internal tracking reference.
Have the helper return the sanitized *url.URL directly instead of a path string, since every caller wrapped it in a url.URL anyway. Rename it to originLocalURL and assert on its String() output in the unit test.
…rowser The origin-local URL test reproduced browser URL normalization (stripping tab/newline/CR and flipping backslash to slash) to check the redirect target. That made test-only logic look like it belonged in production. Assert the invariant directly instead: the escaped Location parses as a relative, same-origin reference and contains no raw backslash or control byte that a browser could normalize back into an authority. url.URL.String() guarantees this by percent-encoding, so production needs no ad-hoc stripping.
|
/coder-agents-review |
There was a problem hiding this comment.
CRF-1, CRF-2, and CRF-4 are addressed. The test restructuring, originLocalURL extraction, and additional redirect-site hardening look solid at a glance. Full panel review is blocked on one silent finding:
CRF-3 (Nit): Commit subjects exceed the 72-character limit. The original commits (74, 83 chars) remain, and the new commit also exceeds it (75 chars: test(coderd/workspaceapps): assert escaping instead of simulating a browser). This was folded into the R1 review body (outside diff hunks), so it may not have been visible as a distinct action item.
To unblock: acknowledge, contest, or fix CRF-3. A one-line reply is sufficient.
🤖 This review was automatically generated with Coder Agents.
|
The commits are being squashed so the messages do not matter and the title of the PR will be used |
|
/coder-agents-review |
There was a problem hiding this comment.
R1 findings CRF-1, CRF-2, and CRF-4 verified fixed. CRF-3 (Nit, commit subject length) contested by author and closed by panel (8/8 accept: squash merge discards individual subjects; PR title at 55 chars is within the convention).
The originLocalURL extraction, additional redirect-site hardening, and test restructuring are all clean. 18 reviewers found no correctness or security issues with the updated code. All three redirect sites in proxy.go that derive their target from r.URL.Path now go through originLocalURL; the remaining redirect uses server-controlled s.AccessURL.String().
Severity count: 2 Nit (1 stale reference, 1 test style).
"I tried to construct a scenario where this fix fails to prevent an open redirect and could not. The differential diagnosis produced four framings; the author chose the right one." (Pariston)
🤖 This review was automatically generated with Coder Agents.
Wrap each table entry in t.Run so every malicious and legitimate path is an independent subtest, matching the sibling tests in this package. A failing vector no longer hides the rest, which matters for a security table. Also fix a stale Test_originLocalPath reference left by the rename to Test_originLocalURL.
|
/coder-agents-review |
There was a problem hiding this comment.
All findings resolved across 4 rounds. No new issues found by Netero or the 10-reviewer panel.
originLocalURL is a clean, minimal sanitizer applied at all three r.URL.Path-derived redirect sites. The two-layer defense (TrimLeft for slash/backslash collapsing + url.URL.String() for control-character encoding) eliminates the class of open redirect by construction. Test coverage is thorough: 18 attack vectors, 3 encoding assertions, 5 legitimate-path preservation cases at the unit level, plus an end-to-end integration test through the full handler with a real encrypted API key.
"I tried to build a case against this change and could not. The problem is correctly understood, the solution is proportional, and the fix is at the right causal level." (Pariston, R4)
🤖 This review was automatically generated with Coder Agents.
2 participants
After consuming a smuggled subdomain API-key, handleAPIKeySmuggling issued a 303 redirect back to the request path to strip the query param. The target was built from the raw, already-decoded r.URL.Path, which can be manipulated by a malicious user.
The redirect target is now built through a url.URL whose Path collapses any leading / and \ run to a single /. url.URL.String() escapes the path, so control characters, backslashes, ?, and # are percent-encoded and can no longer introduce an off-origin host, query, or fragment. The redirect stays same-origin for every input while legitimate paths and unrelated query params are preserved