fix(coderd): prevent cross-tenant workspace app rebinding#26103
Conversation
UpsertWorkspaceApp's ON CONFLICT (id) DO UPDATE rebound an existing app's agent_id to any incoming agent, keyed only on the app id. A malicious build could reuse a victim's app id and point it at the attacker's agent, proxying the victim's app traffic across tenants (SEC-91/PLAT-262). Guard the update in SQL so an app only rebinds when the existing and incoming agents resolve to the same owning workspace. Apps whose existing agent resolves to no workspace, such as template-import or dry-run agents, may be claimed because they are not cross-tenant victims. Once an app belongs to a workspace, rebinds to agents from other workspaces or agents that resolve to no workspace are rejected. insertAgentApp translates the resulting sql.ErrNoRows into a clear cross-workspace rejection that fails the build.
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 1 | Last posted: Round 1, 8 findings (2 P2, 3 P3, 2 Nit, 1 Note), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel. 2 P2, 3 P3, 2 Nit, 1 Note posted. 2 dropped (P4). Reviewed against d852116..2b7eb36. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Solid security fix. The SQL-level WHERE guard on the ON CONFLICT is the right structural decision: it makes cross-workspace rebinding impossible regardless of caller context, and there is no TOCTOU window because PostgreSQL evaluates the guard atomically under the row-level lock. The two-branch NOT EXISTS / OR EXISTS pattern handles all five ownership transitions correctly, and the workspace_builds.job_id UNIQUE constraint guarantees deterministic agent-to-workspace resolution. Test density is 91.4% (445 test lines for 42 production lines), covering all guard branches at the query level and all three job types through the full CompleteJob flow.
Severity counts: 2 P2, 3 P3, 2 Nit, 1 Note.
"I tried to build a case against this change and could not." (Pariston)
coderd/agentapi/subagent.go:293
P3 [CRF-3] This PR changes the contract of UpsertWorkspaceApp: on ID conflict, it now returns sql.ErrNoRows when the cross-workspace guard rejects the rebind. The insertAgentApp caller at provisionerdserver.go:3705 handles this with a descriptive error. This caller wraps all errors generically ("insert workspace app: %w"), passing through the raw sql.ErrNoRows.
Currently safe because line 263 always passes uuid.New(), so ON CONFLICT never fires. But the comment on that same line reads: "NOTE: we may need to maintain the app's ID here for stability." When stable IDs land, collisions will produce a confusing "insert workspace app: sql: no rows in result set" instead of the clear "owned by another workspace" message.
A defensive errors.Is(err, sql.ErrNoRows) check matching what insertAgentApp does, or at minimum a comment noting the changed contract, would close the gap.
(Knuckle P3, Mafuuu, Knov, Kite, Ryosuke, Meruem, Zoro, Hisoka)
🤖
🤖 This review was automatically generated with Coder Agents.
2b7eb36 to
6ceaacc
Compare
jdomeracki-coder
left a comment
There was a problem hiding this comment.
Approved, I like the fail-close approach
Can't really comment on code quality / sticking to convention
Fixes SEC-91 / PLAT-262 by preventing
UpsertWorkspaceAppfrom rebinding an existing app ID to an agent owned by another workspace. The upsert now only rebinds apps when the existing and incoming agents resolve to the same workspace, while still allowing unowned import or dry-run apps to be claimed.Adds query and
provisionerdservercoverage for cross-workspace rejection, same-workspace rebuilds, unowned app claims, and workspace-owned app rebinds to agents without a workspace.Coder Agents generated.