fix(server): charge workspace imports against the plan limit (BUG-2793) - #1219
Merged
Conversation
`POST /workspaces` enforces the user-scoped `workspaces` plan limit before creating. `POST /workspaces/import` did not, and it mints a workspace through the same store.CreateWorkspace — so a user at their plan's limit could exceed it by exporting any workspace and importing it back. Cloud only; enforceUserPlanLimit is a no-op when cloudMode is off, so self-hosted was never affected. Dave ruled the shape on day 63: an import IS a new workspace and counts, with no exemption for re-importing something you previously owned. Export provenance is not trustworthy enough to gate billing on, and the at-limit case that deserves relief — undoing a delete — is served by the restore endpoint, which mints nothing. The call is one line; the PLACEMENT is the fix. It sits beside the #1212 consent gate, ABOVE the Content-Type dispatch, for two reasons: - handleImportWorkspaceBundle is reachable only through that dispatch, so a gate below it would cover the JSON path and leave the tar.gz path — the one that carries attachments, and the one a real export produces — wide open. - Above either body read, so a refused caller never uploads. The two paths have very different size bounds; the gate precedes both. That is not a hypothetical: the mutation matrix includes it. Moving the gate below the dispatch fails ONLY the bundle test and leaves the JSON test green, which is exactly the false confidence a placement-blind fix would have shipped. Five tests, four of them controls, because a gate is easy to get green and hard to get right: the JSON path refuses at the limit, the bundle path refuses at the limit, under-the-limit is NOT refused (a gate wired to the wrong feature key would pass the first two), self-hosted is unaffected (this must not introduce a limit where there are no plans), and a request with no resolved user is not charged — mirroring the create side's `userID != ""` guard, which is not defensive padding but the difference between "no limit applies" and a nil lookup. This is the SECOND gate on workspace creation the import door skipped; the first was the OAuth consent gate (IDEA-2756, PR #1212). Two have now diverged this way, which is the argument for the shared pre-step helper — tracked separately rather than folded in here. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…-refusal (BUG-2793) Codex round 1. The three control tests were vacuous and I would have shipped them. WorkspaceExport.Version defaults to 0 and the import requires 1, so the under-the-limit, self-hosted, and no-resolved-user cases were all failing with a 500 long before they reached anything this change is about. They passed because they asserted only "not 403" — and a 500 is not a 403. That made all three useless in the same direction: a fix that broke imports outright, or a gate wired to refuse everything with a non-403 status, would have sailed through them while the two refusal tests stayed green. The controls existed precisely to catch that, and could not. Fixed by setting Version: 1 and asserting the real success status, 201. A control that cannot tell success from a server error controls nothing. Mutation matrix re-run after the change, because a matrix over vacuous tests proves nothing either: removing the gate still fails exactly the two refusal tests, and the three controls now pass on genuine imports rather than on identical 500s. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…ns (BUG-2793) Codex round 2 pointed out that this test locks in a 201 for a caller with no resolved user, and that a reader will take that as approval of userless workspace creation. It is not. The test pins the GUARD — that import behaves as create does when no user resolves — and it drives the handler directly, so it does not prove a real legacy workspace token reaches this code at all. Said so in the test rather than leaving the 201 to speak for itself, and pointed at BUG-2809 for the question it does not answer. Round 2's three findings are all real and all filed rather than folded, because this unit's ruling is specifically the plan limit on the import door: - BUG-2808 — enforceUserPlanLimit is check-then-act, so concurrent requests can exceed any cap. A property of the helper, shared with the create door and every other feature it gates; this change inherits it rather than introducing it. - BUG-2809 — import and create still enforce different preconditions on the same mint: required-name validation (a live defect — an empty name yields an empty SLUG, which is a routing key), settings normalization, source attribution, and the userless case above. Filed as a class because the mechanism is one thing and the record now shows it failing twice. The reviewer also confirmed two non-doors, which is the useful negative: autoCreateWorkspace is intentional first-workspace provisioning, and `pad db migrate-to-pg` calls ImportWorkspace directly as an operator-only migration outside HTTP entirely. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
… data (BUG-2793) Codex round 3, on the tests. Two ways they could pass without proving what their names say. 1. The JSON refusal test sends VALID json, so a gate placed after the decode would still return 403 and it would stay green. The bundle test covers the gzip half of the placement claim; nothing covered the JSON half, and the code comment claims the gate sits above EITHER body read. Added an at-limit case with an undecodable body: reaching 403 rather than a decode error is only possible if nothing read the body first. 2. The no-resolved-user test asserted only a 201. A regression that quietly attributed the import to the at-limit fixture user would also return 201 and pass. It now asserts the fact instead of inferring it — the user's workspace count is unchanged across the request, and the created workspace has no owner. The mutation matrix now separates the two placements, which is the point of having both tests: - gate below the Content-Type dispatch (still above the decode) -> only the BUNDLE test fails. - gate below the JSON decode -> the bundle test AND the new JSON test fail. Neither mutation is caught by the original refusal test, which is what "passes for the wrong reason" looked like here. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
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.
fix(server): charge workspace imports against the plan limit (BUG-2793)
POST /workspacesenforces the user-scopedworkspacesplan limit beforecreating.
POST /workspaces/importdid not, and it mints a workspace throughthe same store.CreateWorkspace — so a user at their plan's limit could exceed
it by exporting any workspace and importing it back. Cloud only;
enforceUserPlanLimit is a no-op when cloudMode is off, so self-hosted was never
affected.
Dave ruled the shape on day 63: an import IS a new workspace and counts, with
no exemption for re-importing something you previously owned. Export
provenance is not trustworthy enough to gate billing on, and the at-limit case
that deserves relief — undoing a delete — is served by the restore endpoint,
which mints nothing.
The call is one line; the PLACEMENT is the fix. It sits beside the #1212
consent gate, ABOVE the Content-Type dispatch, for two reasons:
gate below it would cover the JSON path and leave the tar.gz path — the one
that carries attachments, and the one a real export produces — wide open.
have very different size bounds; the gate precedes both.
That is not a hypothetical: the mutation matrix includes it. Moving the gate
below the dispatch fails ONLY the bundle test and leaves the JSON test green,
which is exactly the false confidence a placement-blind fix would have
shipped.
Five tests, four of them controls, because a gate is easy to get green and
hard to get right: the JSON path refuses at the limit, the bundle path refuses
at the limit, under-the-limit is NOT refused (a gate wired to the wrong feature
key would pass the first two), self-hosted is unaffected (this must not
introduce a limit where there are no plans), and a request with no resolved
user is not charged — mirroring the create side's
userID != ""guard, whichis not defensive padding but the difference between "no limit applies" and a
nil lookup.
This is the SECOND gate on workspace creation the import door skipped; the
first was the OAuth consent gate (IDEA-2756, PR #1212). Two have now diverged
this way, which is the argument for the shared pre-step helper — tracked
separately rather than folded in here.
Gates:
go test ./...under Postgres 17 EXIT=0; gofmt clean;make lint0issues.
BUG-2793
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN