feat(server): refuse workspace creation without may_create_workspaces consent (IDEA-2756) - #1212
Merged
Merged
Conversation
… consent (IDEA-2756)
The OAuth consent screen's "Let this app create new workspaces" checkbox
gated only the post-creation auto-add. A connection whose user left it
unticked could still create workspaces; it simply could not then see
them. A permission that does not prevent the action it names is a
consent mismatch.
Dave ruled it: the checkbox is a permission on whether the connected
token may CREATE, and it has to be true to what a user would honestly
expect from the option. The behaviour-change-for-existing-connections
argument loses to honest consent semantics.
Adds Server.requireWorkspaceCreationConsent, a shared gate at the top of
both endpoints that mint a workspace under the caller's account:
POST /api/v1/workspaces handleCreateWorkspace
POST /api/v1/workspaces/import handleImportWorkspace
Import reaches CreateWorkspace via store.ImportWorkspace, so it is the
same permission at a second door — lead-ruled as an application of the
same rationale, not a new decision. The gate sits above the Content-Type
dispatch, so it covers the tar.gz bundle path (whose only route is that
handler) and refuses before the 64 MiB body read.
Refusal is a 403, mirroring handleAuditLog's consent refusal (BUG-2102):
a hard decline rather than a narrowed response, because there is no
narrower version of creating a workspace.
Three non-refusal cases and one refusal, all but the last with a test:
- not an OAuth grant (PAT, CLI session, local stdio) — creation rides
on ordinary account authority
- ErrOAuthConnectionNotFound (pre-Phase-C grant) — ALLOW, matching the
backfill's may_create_workspaces=ON default. Deliberately asymmetric
with maybeAutoAddCreatorConnection's not-found branch, which declines
a convenience where this one would invent a refusal
- flag set — proceeds; the auto-add is unchanged
- a store I/O error — REFUSED, failing closed with a 500, because
allowing the create when the deciding state could not be read grants
a declined permission on the strength of a database blip. This is
the one branch with no test: injecting a store read failure needs a
fault-injecting store the package does not have, so it is reasoned
rather than measured
Population enumerated before the fix (CONVE-18): five CreateWorkspace
call sites, two of them HTTP endpoints reachable by an OAuth token (both
gated). Excluded with reasons: autoCreateWorkspace (signup-time, no
connection in context), workspace restore (un-deletes an existing
workspace), /oauth/claim (grants access, does not mint), cmd_db.go
(local store copy, no HTTP). Search boundary: the sweep traced
Store.CreateWorkspace callers and did not look for a path that inserts a
workspace by raw SQL.
Ten tests, all driving the real router rather than calling handlers
directly (CONVE-19). Every refusal leg asserts that no workspace of that
name exists afterwards, not merely the status code (CONVE-12) — a guard
that 403s after the write passes a status-only assertion. Seven mutants,
seven detected, including both guard-placement mutations.
MCP tool surface 0.25 -> 0.26. Behaviour bump on the v0.9/v0.16/v0.25
grounds: no tool name, action enum or param shape changed, but
pad_workspace.create now refuses a call it used to permit. Closest
precedent is v0.10; unlike v0.10 there is deliberately no escape-hatch
param, because the gate encodes a decision the USER made at consent time
and a bypass flag would be the app overriding its own grant.
CONVE-23 sweep for prose the change falsified: instructions.md told
agents the create still succeeds and to use the claim flow (it would
have sent them to claim something that was never created); the
TASK-2753 allow-list guard entry asserted the same and posed IDEA-2756
as open; the MCP catalog and CLI help described only the flag=true path;
maybeAutoAddCreatorConnection's flag-off branch is now unreachable from
its sole caller and is documented as dead code kept for contract, to be
deleted only with the guard. CLAUDE.md was already stale at v0.24 (v0.25
bumped the constant without it) — brought to v0.26 with a backfilled
v0.25 line.
The consent screen and console copy are unchanged: they were the
misleading half of this bug, and the fix makes them true.
The import-side gate is correct but currently unexercised in production, and the first framing of this change did not say so. WithMCPTokenIdentity is stashed by exactly one middleware, MCPBearerAuth, mounted on /mcp alone. An OAuth connection reaches an /api/v1 handler only through the in-process MCP dispatcher, and that dispatcher's route table has a workspace create action but no workspace import. So no OAuth-bound caller can reach handleImportWorkspace today. The gate stays, and the comment now says why: adding that action later must not silently reopen the door, which is the state a create-only fix would have left armed. Found on a verify pass reading the middleware mount points, not by the tests — they synthesize the OAuth identity into the request context, so they prove the handler's behaviour GIVEN an identity and have no opinion about which routes supply one (CONVE-19). Codex round 2 reached the same conclusion independently.
…2756)
All five were mine, all P2, none changing the gate's behaviour — four are
claims that were broader than the code, one is a test that proved less
than its name.
1. "Only re-authorization lifts it" was wrong in five places (version.go,
README, CLAUDE.md, the MCP catalog description, CLI help). A user can
also enable the flag on the EXISTING connection via
PATCH /connected-apps/{id}/flags, which the console page drives —
instructions.md said so and contradicted the others. All five now name
both remedies, and both are still the user's, which is the part that
matters: neither is reachable by the app.
2. "This branch is UNREACHABLE ... it is dead code" on
maybeAutoAddCreatorConnection's flag-off branch was false. The gate
reads the connection and that function reads it AGAIN after creation;
a user revoking creation power from the console between those two
reads lands exactly there. It is a real second check across a real
TOCTOU window, failing in the safe direction. The claim was written
from the call graph, which cannot see a concurrent write between two
reads.
3. handlers_import_bundle.go's "Auth: any authenticated user" was made
false by this change and the concept sweep never had a chance at it —
it greps may_create / auto-add / creation power, and that sentence
contains none of them. Corrected in place.
4. The two NonOAuthCallerUnaffected tests claimed PAT, CLI session and
local stdio; each drives one PAT. The comments now state the fixture's
real scope and why one caller stands for the class (the guard branches
on an identity only MCPBearerAuth sets, so callers that skipped it are
indistinguishable) rather than implying three fixtures.
5. The JSON import refusal leg would have passed with the gate below
decodeJSONWithLimit — only the bundle leg pinned placement, and only
for gzip. Adds TestImportWorkspace_ConsentRefusalPrecedesBodyDecode
(malformed body: 400 if the gate is late, 403 if it is early),
mirroring the create-side ordering legs.
Mutation matrix now 9 mutants, 9 detected. M8 (guard below the JSON
decode) is killed by the bundle leg too, so it shows the new test is
covered rather than necessary; M9 gates the bundle path and moves only
the JSON path's guard, and dies to the new leg ALONE. That is the mutant
that justifies the test.
…close it (BUG-2792) Round 3 caught me calling maybeAutoAddCreatorConnection's flag-off branch dead code. The replacement comment then claimed the branch means a revoked grant cannot silently gain a workspace — which is more safety than the code delivers, and round 4 caught that. The read and the AddConnectionWorkspace insert below it are separate unconditional statements, so a revocation landing BETWEEN them still adds the workspace. The check narrows the window; it does not close it. Filed as BUG-2792 rather than folded in: the race is pre-existing and unchanged by IDEA-2756, and closing it needs an atomic check-and-insert at the store layer, written and gated for both dialects — materially more diff and risk than this handler-level guard. Both mistakes were the same shape in opposite directions: a claim about concurrency derived from reading the call graph, which cannot see a concurrent write between two reads.
gofmt wants blank lines between list items once one item spans multiple paragraphs, which the BUG-2792 note made true. My error, and worth naming exactly: I ran build, vet and the targeted tests on this commit but not lint, because lint had passed on the PREVIOUS commit and the change was 'only a comment'. The gate has to run on the tree being pushed, not on an earlier one that resembles it. CI's golangci-lint is pinned to the same v2.11.4 the Makefile installs, so there was no version skew to blame — the local gate would have caught this in 51 seconds.
…(IDEA-2756) Round 8 reviewed only the prose this change adds. Ten claims were broader than the code. All ten are mine; none changes behaviour. Rounds 3, 4 and 7 each caught one of these, which is why round 8 was pointed at the class rather than at a new dimension. The substantive ones: - "gates every endpoint that MINTS a workspace" — autoCreateWorkspace mints from registration, bootstrap and oauth-login and is deliberately outside this gate. The helper doc and the test header now name the two callers and the exclusion instead of claiming universality. - "the agent was handed a workspace it could not then see" (version.go, README, CLAUDE.md) — only true for a connection with an EXPLICIT allow-list. An all_current_workspaces=true connection is not gated per slug and could see what it made. The consent mismatch is the constant; the invisibility was its most visible symptom, not its definition. - "ErrOAuthConnectionNotFound — a pre-Phase-C grant" asserted a cause the code cannot know: ANY missing row takes that branch. Now stated as the expected cause, with the limit of what the code can tell. - "above the 64 MiB body read" conflated the two import paths. 64 MiB is the JSON decode's bound; the bundle path has its own, much larger. The gate precedes both, which is the property that actually matters. - "the request context is decorated AFTER TokenAuth runs" was false, and inherited verbatim from the sibling helper this was modelled on (handlers_oauth_claim_test.go's doClaim), where it is also false. The wrapper sets the identity BEFORE ServeHTTP; it survives because nothing on the /api/v1 chain writes that key. - "lets CreateWorkspace normalize it" — CreateWorkspace slugifies only when the supplied slug is EMPTY, and import supplies a non-empty one, so an imported workspace keeps the ?name= value verbatim. - "The PAT needs a workspace to bind to" — CreateAPIToken takes WorkspaceID as optional. And one where the first fix was worse than the finding: - "Every refusal leg asserts no workspace exists afterwards" was false — the two ordering legs assert status only. My first correction ADDED those assertions, which is the trap the finding was pointing at: a malformed body and an empty name are rejected before creation under every guard placement, so "no such workspace exists" is true of broken and working code alike. Reverted; the header now states which legs carry the counterfactual, and why the ordering legs discriminate on status instead. Gates re-run on the tree being pushed, not an earlier one: gofmt clean, lint 0 issues, internal/server and internal/mcp green, mutation matrix still 9/9.
No code change. The Go job on cb47c76 failed on BUG-2786 (the recurring internal/events subscribe-confirm guard, which fails by asserting its own premise: 'the acknowledgement never landed before the mark; this test could not have discriminated'). CONVE-11 owes that failure a re-run before it can be called a flake. rerun-failed-jobs produced attempt 2 = startup_failure with the Go job stuck in 'queued' — a GitHub infrastructure fault, not a test result — after which the run refuses further retries ('This workflow run cannot be retried'). The CI workflow has no workflow_dispatch trigger, so a push is the only way to get a fresh run. Evidence the failure is unrelated to this branch, gathered before re-running rather than after: the branch touches 0 files under internal/events (11 files total, none in that package), and the parent tip c681850 had Go: SUCCESS with the only non-comment Go difference being one added assertion in this branch's own test file. Go (PostgreSQL) also passed on cb47c76, exercising the same package.
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
1 participant
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.
Closes IDEA-2756.
The defect
The OAuth consent screen carries a checkbox labelled "Let this app create new workspaces". It gated only
maybeAutoAddCreatorConnection's allow-list insert — so a connection whose user left it unticked could still create workspaces throughPOST /api/v1/workspaces; it simply could not then see what it made. A permission that does not prevent the action it names is a consent mismatch, and the user's natural reading of an unticked box is "this app cannot make workspaces in my account".Ruled by Dave (day-51, on IDEA-2756's trail): the checkbox is a permission on whether the connected token has the right to CREATE, and it has to be true to what a user would honestly expect from the option. The behaviour-change-for-existing-connections argument loses to honest consent semantics.
This is not, and never was, a security finding. Creation cannot disclose anything — it mints a new workspace rather than reading an existing one. It is a consent-honesty fix.
The population, enumerated before the fix
The item's disposition named one site. The class is endpoints that MINT a workspace under the caller's account and are reachable by an OAuth-connection token. Enumerated from all five non-test
CreateWorkspace(call sites ininternal/andcmd/, then traced to routes:POST /api/v1/workspaces→handleCreateWorkspacepad_workspace action=createand both CLI paths (pad init,pad workspace init) POST here — one guard, three surfacesPOST /api/v1/workspaces/import→handleImportWorkspaceCreateWorkspaceviastore.ImportWorkspace. The gzip branch dispatches tohandleImportWorkspaceBundlefrom inside this handler, and that handler has no other route, so one guard at the top covers both request bodies. See the reachability note below — no OAuth caller can reach this route todayautoCreateWorkspace(handlers_cloud.go), reached from/auth/bootstrap,/auth/register,/auth/oauth-loginPOST /workspaces/{slug}/restorePOST /oauth/claimcmd_db.goImportWorkspaceImport was lead-ruled in as an application of Dave's stated rationale rather than a new decision: a user who left the box unticked does not expect the app to mint a workspace by importing one.
Reachability, stated precisely — a correction to my own first framing
I originally wrote that both endpoints are "reachable by an OAuth-connection token". That is true of create and false of import, and I found it on my own verify pass after opening this PR; Codex round 2 reached the same conclusion independently.
WithMCPTokenIdentityis stashed by exactly one middleware,MCPBearerAuth, mounted on/mcpalone. An OAuth connection therefore reaches an/api/v1handler only through the in-process MCP dispatcher — and that dispatcher's route table has aworkspace createaction but noworkspace import.So:
pad_workspace.create;pad initandpad workspace initPOST to the same endpoint under ordinary account authority and take the non-OAuth branch)workspace importMCP action later cannot silently reopen the door — which is precisely the state a create-only fix would have left armedWorth being explicit that the new tests could not have caught this: they synthesize the OAuth identity into the request context, so they prove the handler's behaviour given an identity and have no opinion about which routes ever supply one. That is the wiring-is-a-claim blind spot (CONVE-19), and the answer here came from reading the mount points, not from a green suite.
Search boundary: the sweep traced
Store.CreateWorkspacecallers. It did not search for a path that inserts a workspace by raw SQL; if one exists it is outside what this enumeration could find.The change
One shared helper,
Server.requireWorkspaceCreationConsent, called at the top of both handlers. Shape mirrorshandleAuditLog's consent refusal (BUG-2102): a hard 403 rather than a narrowed response, because there is no narrower version of creating a workspace.Three non-refusal cases and one refusal:
request_id) → creation rides on ordinary account authority; this flag has no opinionErrOAuthConnectionNotFound(pre-Phase-C grant, not yet backfilled) → allow. The backfill mints those rows with the flag ON, so allowing is that default applied early. Deliberately asymmetric withmaybeAutoAddCreatorConnection's not-found branch: that path declines a convenience, this one would invent a refusalstored_state_unreadable— the state is not unreadable-in-principle; the read failed, the fault is ours, and a retry can legitimately succeedThe gate sits above the body decode on both handlers, so a refusal never depends on body validity — and on import, above the 64 MiB read, so a refused caller never uploads a bundle.
No escape-hatch parameter, deliberately (contrast v0.10's
allow_draft): the gate expresses a decision the user made at consent time, so a bypass flag would be the app overriding its own grant. The remedy is re-authorization, which only the user can perform.Tests — 10 new, all through the real router
internal/server/handlers_workspace_create_consent_test.godrivessrv.ServeHTTP, so the tests have an opinion about the ROUTE and not only the function (CONVE-19).Every refusal leg asserts the wrong behaviour's observable consequence — that no workspace of that name exists afterwards — not merely the status code (CONVE-12). A guard that 403s after
store.CreateWorkspacepasses a status-only assertion while leaving the workspace behind.Both control directions are covered: flag-set still creates and auto-adds; PAT callers are unaffected on both endpoints. Without those, a guard that refused every OAuth caller — or every caller — would pass all the refusal legs.
Mutation matrix — 7 mutants, 7 detected
Harness asserts anchor uniqueness, requires each mutant to compile before its run counts as evidence, requires
--- FAIL: <name>for every predicted test, and verifies the restore byte-for-byte.The uniqueness assertion earned itself twice: on the first run M5's anchor matched two functions (the guard and
maybeAutoAddCreatorConnection's own check), and on the re-run after a message edit it matched zero, which is the state that would otherwise report as a clean matrix.Not covered, stated rather than implied: the I/O-error branch has no test. Injecting a store read failure needs a fault-injecting store the package does not have, so that branch is reasoned, not measured.
MCP surface: v0.25 → v0.26
pad_workspace.action=createnow refuses a call it used to permit. BEHAVIOR bump on the v0.9 / v0.16 / v0.25 grounds — no tool name, action enum, or param shape changed. Closest precedent is v0.10, which likewise turned a server-side gate into a structured refusal.tool_surface_drift_test.gocaughtinstructions.mdandREADME.mdimmediately; both updated.CONVE-23 sweep — prose this change falsified
Grepped the concept (
may_create,auto-add,creation power) across.go,.md,.svelte,.ts,.html, then re-read each site after editing:internal/mcp/instructions.md— said "if that scope was declined the create call still succeeds but the workspace doesn't auto-join — direct the user to the claim flow". False in both halves, and it is the artifact an MCP agent reads: it would have sent the agent to claim a workspace that was never created. Rewritten to say the call is refused, retrying won't help, and name the two real remedies.internal/mcp/dispatch_http_allowlist_guard_test.go— TASK-2753'sworkspace createguard entry asserted "with it unset the create still succeeds" and posed IDEA-2756 as the open question. Now answered; classification staysexempt, exactly as the item predicted, because refusing to create is still not a disclosure question.internal/mcp/catalog_workspace.goand 4.cmd/pad/cmd_workspace.go— both described only the flag=true path. Not false, but incomplete in the direction that matters to an agent deciding whether to retry.internal/mcp/dispatch_http_routes.go— the OAuth identity now decides the call, not just the side effect.maybeAutoAddCreatorConnection's flag-off branch is now unreachable from its sole caller. Kept, with a comment saying so: the function's contract is "add only when the grant permits", and a helper whose safety depends on its caller checking first is one refactor from being wrong. Documented as dead code to be deleted with the guard, not as redundant defence in depth.Checked and deliberately unchanged: the consent screen copy and the console toggle. Both were the misleading half of this bug — the fix makes existing copy true rather than needing new copy. The refusal message quotes that label verbatim, with a comment binding the two.
Found in passing and fixed:
CLAUDE.mdstill described the catalog as v0.24 in three places — v0.25 (TASK-2657) bumped the constant without it, and nothing enforces CLAUDE.md the way the drift test enforces the other two documents. Brought to v0.26 with a backfilled v0.25 line. The drift test's coverage gap is worth its own item.Deploy note, and one review finding declined on technical grounds
Codex round 6 raised as P1 that a mixed fleet is not policy-consistent: during a rolling deploy, old instances still answer
201for a flag-off connection while new ones answer403, so the same call succeeds or fails by load-balancer routing, andtool_surface_versionreads0.25or0.26per instance. It suggested draining old instances before exposing the new behaviour, and noted the commit leaves no marker or cleanup path for workspaces the old instances created.Declined as a blocker, recorded so it is not re-litigated:
The inconsistency window is a property of deploying any behaviour change to a fleet, not of this diff. Every prior
ToolSurfaceVersionbump had the same per-instance split.The failure mode inside the window is the old behaviour — a workspace gets created that the new gate would refuse. That is what happens today, all the time, and has for months.
Corrected after round 7 argued against this, and it was right: my original wording said the window "does not create a new harm", which overstates. This is an authorization change, so during the window old nodes honour a permission the user explicitly declined, and a refused caller can simply retry until it lands on one. That is a different shape from passive uneven arrival — it is briefly bypassable by anything motivated to bypass it. The conclusion stands (what a bypass yields is exactly what the app could do freely until this PR, over a window measured in minutes), but the reasoning had to be fixed, not just restated.
No schema, no migration, so rollback is clean. Rollback reopening creation is what rollback means here.
A cleanup path for workspaces created under the old behaviour is deliberately not in scope. Those are real workspaces the user can see, owned by them, some presumably wanted. Deleting or marking them would be a destructive data operation nobody asked for, decided by an agent, on the strength of a consent flag's state at some past moment we did not record.
What is worth acting on, upgraded from round 7's argument: this is an authorization change with no schema component. Deploy it as a fast rollout rather than a long canary, and prefer draining old instances to running a mixed fleet for an extended period — not because the window is dangerous, but because "an enforcement gap you can retry into" is worth minutes rather than hours. There is nothing to sequence and no migration to order against.
Adjacent defects found and filed, not folded in
Three preconditions/side-effects now differ between the two doors onto
CreateWorkspace. This PR closes the first; the other two are filed rather than folded, because each needs a decision this PR's ruling does not cover:workspacesplan limitCreateWorkspace)Three is a pattern rather than a coincidence:
handleImportWorkspacewas written as "restore a workspace from an artifact" andhandleCreateWorkspaceas "mint a workspace for this caller", so everything the second learns about being a caller-facing creation endpoint has to be re-learned by the first, one incident at a time. Both filed items argue for a shared pre/post step over a third one-off patch.Also filed: BUG-2792, the residual TOCTOU window inside the auto-add itself.
Gates
make test(SQLite): 28 packages ok, 0 failures, exit 0internal/storetook 351s vs 76s on SQLite, so the PG legs demonstrably ran rather than skippingmake lint: 0 issuesmake vuln: no vulnerabilities affecting this code