fix: only proxy redirect on existing origin#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 history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 4 findings (2 P3, 2 Nit), COMMENT. Review Finding inventoryFindings
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. 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.
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.
| func TestHandleSubdomain_APIKeySmuggling_NoOpenRedirect(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cases := []struct { |
There was a problem hiding this comment.
P3 [CRF-1] The table covers tab but not newline or CR, despite both being named bypass vectors in the test comment (line 186: "Browsers additionally strip tab/newline characters") and modeled in the browser normalization step (line 223: strips \t, \n, \r). Two more rows would close the regression coverage gap:
{name: "LeadingNewline", path: "/\n/evil.com/phish"},
{name: "LeadingCR", path: "/\r/evil.com/phish"},The fix handles these correctly today (\n encodes to %0A, \r to %0D). The gap is regression coverage: if a future change broke encoding for one control character class but not another, only the tab path would catch it. (Bisky)
🤖
| path := r.URL.Path | ||
| if path == "" { | ||
| path = "/" | ||
| // Strip the smuggled API key query parameter and redirect back to the same |
There was a problem hiding this comment.
P3 [CRF-2] The PR title "fix: only proxy redirect on existing origin" does not name the vulnerability class. If this is squash-merged, the title becomes the commit subject, losing the clear naming from the individual commits. "Existing origin" is not standard URL terminology.
Suggested: fix(coderd/workspaceapps): prevent open redirect via smuggled API-key path (Leorio)
🤖
| // newSmugglingTestServer builds a workspaceapps.Server wired with a static | ||
| // encryption keycache and returns the server, a valid app subdomain host, and a | ||
| // freshly minted smuggled API key that decrypts against the server. Each caller | ||
| // gets its own server so parallel subtests never share mutable state. |
There was a problem hiding this comment.
Nit [CRF-4] newSmugglingTestServer duplicates the hostname pattern, dashboard URL, RealIPConfig, and server construction from TestHandleSubdomain_IgnoresUntrustedForwardedHost at line 42. The existing test needs a direct reference to its fakeSignedTokenProvider for call-count assertions, so it can't use this helper as-is. Not urgent with only two callers, but worth noting for future test additions in this file. (Robin)
🤖
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