From 710fe44721da60f86c4c133c5bcfb7e53e53a19a Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 02:59:23 +0000 Subject: [PATCH 1/4] fix(server): charge workspace imports against the plan limit (BUG-2793) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- ...ndlers_workspace_import_plan_limit_test.go | 159 ++++++++++++++++++ internal/server/handlers_workspaces.go | 28 +++ 2 files changed, 187 insertions(+) create mode 100644 internal/server/handlers_workspace_import_plan_limit_test.go diff --git a/internal/server/handlers_workspace_import_plan_limit_test.go b/internal/server/handlers_workspace_import_plan_limit_test.go new file mode 100644 index 00000000..d0db06f4 --- /dev/null +++ b/internal/server/handlers_workspace_import_plan_limit_test.go @@ -0,0 +1,159 @@ +package server + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// BUG-2793. `POST /workspaces` enforces the user-scoped `workspaces` plan +// limit; `POST /workspaces/import` did not, and it mints a workspace through +// the same store.CreateWorkspace. A user at their plan's limit could exceed it +// by exporting any workspace and importing it back. +// +// This is the SECOND gate on workspace creation the import door skipped — the +// first was the OAuth consent gate (IDEA-2756, PR #1212). The tests below are +// written against the door rather than against the limiter, because the +// limiter was never broken: what was missing was the call, and where it sits. + +// importLimitFixture builds a cloud-mode server and a user who owns exactly +// `owned` workspaces. +func importLimitFixture(t *testing.T, owned int) (*Server, *models.User) { + t.Helper() + srv := testServer(t) + srv.cloudMode = true + + user, err := srv.store.CreateUser(models.UserCreate{ + Email: "owner@example.com", Name: "Owner", + Password: "correct-horse-battery-staple", Role: "member", + }) + if err != nil { + t.Fatalf("create user: %v", err) + } + for i := 0; i < owned; i++ { + if _, err := srv.store.CreateWorkspace(models.WorkspaceCreate{ + Name: "Owned " + string(rune('A'+i)), OwnerID: user.ID, + }); err != nil { + t.Fatalf("create workspace %d: %v", i, err) + } + } + return srv, user +} + +// importRequest drives handleImportWorkspace directly with the caller +// attached, which is how the router presents an authenticated request. +// contentType selects the JSON path or the tar.gz bundle path. +func importRequest(t *testing.T, srv *Server, user *models.User, contentType string, body []byte) *httptest.ResponseRecorder { + t.Helper() + r := httptest.NewRequest("POST", "/api/v1/workspaces/import", bytes.NewReader(body)) + r.Header.Set("Content-Type", contentType) + r.RemoteAddr = "192.0.2.1:1234" + if user != nil { + r = r.WithContext(WithCurrentUser(r.Context(), user)) + } + rr := httptest.NewRecorder() + srv.handleImportWorkspace(rr, r) + return rr +} + +func exportBody(t *testing.T) []byte { + t.Helper() + b, err := json.Marshal(models.WorkspaceExport{ + Workspace: models.WorkspaceExportMeta{Name: "Imported", Slug: "imported"}, + }) + if err != nil { + t.Fatalf("marshal export: %v", err) + } + return b +} + +// TestImportWorkspace_EnforcesThePlanLimit is the defect itself, on the JSON +// path. The free plan allows store.DefaultFreeLimits.Workspaces; a user who +// already owns that many must be refused. +func TestImportWorkspace_EnforcesThePlanLimit(t *testing.T) { + atLimit := store.DefaultFreeLimits.Workspaces + srv, user := importLimitFixture(t, atLimit) + + rr := importRequest(t, srv, user, "application/json", exportBody(t)) + + if rr.Code != http.StatusForbidden { + t.Fatalf("import at the plan limit returned %d, want 403: %s", rr.Code, rr.Body.String()) + } + if b := rr.Body.String(); !strings.Contains(b, "plan_limit_exceeded") { + t.Errorf("response lacks the code a client switches on: %s", b) + } +} + +// TestImportWorkspace_EnforcesThePlanLimitOnTheBundlePathToo is the reason the +// gate's PLACEMENT is the load-bearing part rather than the call. +// +// handleImportWorkspaceBundle is reachable only through this handler's +// Content-Type dispatch. A gate added below that dispatch 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, while the test above stayed green. +// +// The body is deliberately not a valid bundle: the refusal must happen before +// anything reads it, so an invalid body reaching a 403 rather than a parse +// error is itself the assertion. +func TestImportWorkspace_EnforcesThePlanLimitOnTheBundlePathToo(t *testing.T) { + atLimit := store.DefaultFreeLimits.Workspaces + srv, user := importLimitFixture(t, atLimit) + + rr := importRequest(t, srv, user, "application/gzip", []byte("not a real gzip bundle")) + + if rr.Code != http.StatusForbidden { + t.Fatalf("bundle-path import at the plan limit returned %d, want 403 — a gate below the "+ + "Content-Type dispatch would leave this path open: %s", rr.Code, rr.Body.String()) + } + if b := rr.Body.String(); !strings.Contains(b, "plan_limit_exceeded") { + t.Errorf("response lacks the plan-limit code: %s", b) + } +} + +// TestImportWorkspace_UnderTheLimitIsNotRefused is the control. Without it, a +// gate that refused every import — or one wired to the wrong feature key — +// passes both tests above while breaking the feature outright. +func TestImportWorkspace_UnderTheLimitIsNotRefused(t *testing.T) { + srv, user := importLimitFixture(t, store.DefaultFreeLimits.Workspaces-1) + + rr := importRequest(t, srv, user, "application/json", exportBody(t)) + + if rr.Code == http.StatusForbidden { + t.Fatalf("import UNDER the plan limit was refused: %s", rr.Body.String()) + } +} + +// TestImportWorkspace_SelfHostedIsUnaffected pins the other half of the +// obligation: enforceUserPlanLimit is a no-op off cloud, and this fix must not +// quietly introduce a limit on self-hosted instances, which have no plans. +func TestImportWorkspace_SelfHostedIsUnaffected(t *testing.T) { + atLimit := store.DefaultFreeLimits.Workspaces + srv, user := importLimitFixture(t, atLimit) + srv.cloudMode = false // the only difference from the refusing case + + rr := importRequest(t, srv, user, "application/json", exportBody(t)) + + if rr.Code == http.StatusForbidden { + t.Fatalf("self-hosted import was refused by a plan limit that should not apply: %s", rr.Body.String()) + } +} + +// TestImportWorkspace_NoResolvedUserIsNotCharged mirrors the create side's +// `userID != ""` guard. A legacy workspace token resolves no user, and there +// is nobody to charge — the guard is not defensive padding, it is the +// difference between "no limit applies" and a nil lookup. +func TestImportWorkspace_NoResolvedUserIsNotCharged(t *testing.T) { + srv, _ := importLimitFixture(t, store.DefaultFreeLimits.Workspaces) + + rr := importRequest(t, srv, nil, "application/json", exportBody(t)) + + if rr.Code == http.StatusForbidden { + t.Fatalf("an import with no resolved user was charged against a plan: %s", rr.Body.String()) + } +} diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index 92f743b2..9491c51f 100644 --- a/internal/server/handlers_workspaces.go +++ b/internal/server/handlers_workspaces.go @@ -826,6 +826,34 @@ func (s *Server) handleImportWorkspace(w http.ResponseWriter, r *http.Request) { return } + // Plan limit — the SECOND gate on workspace creation this door used to + // skip (BUG-2793). An import 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. + // + // Dave's day-63 ruling: 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 does not mint anything. + // + // Placed here for the same two reasons as the consent gate above it, and + // the placement is the load-bearing part rather than the call: ABOVE the + // Content-Type dispatch, so the tar.gz bundle path is covered by the same + // line rather than needing its own, and above either body read, so a + // refused caller never uploads. Self-hosted is unaffected — + // enforceUserPlanLimit returns true when cloudMode is off. + // + // The `userID != ""` guard mirrors the create side exactly. It is not + // defensive padding: a legacy workspace token resolves no user, and + // charging an unattributable import against nobody's plan is not a + // limit, it is a crash waiting for a nil. + if userID := currentUserID(r); userID != "" { + if !s.enforceUserPlanLimit(w, userID, "workspaces") { + return + } + } + // Content-Type dispatch: // application/gzip / application/x-gzip / application/x-tar // → tar.gz bundle path (TASK-885) — handles attachments. From f9ed39b7e85cc752ef17f81f233df6066e9c7ede Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 03:11:55 +0000 Subject: [PATCH 2/4] test(server): make the import controls assert success, not merely non-refusal (BUG-2793) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...ndlers_workspace_import_plan_limit_test.go | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/internal/server/handlers_workspace_import_plan_limit_test.go b/internal/server/handlers_workspace_import_plan_limit_test.go index d0db06f4..2966fdfd 100644 --- a/internal/server/handlers_workspace_import_plan_limit_test.go +++ b/internal/server/handlers_workspace_import_plan_limit_test.go @@ -64,7 +64,12 @@ func importRequest(t *testing.T, srv *Server, user *models.User, contentType str func exportBody(t *testing.T) []byte { t.Helper() + // Version 1 is REQUIRED. Without it the import fails with a 500 before it + // reaches anything this file is about — and the success controls below, + // which assert only "not 403", passed on that 500 (codex round 1). A + // control that cannot tell success from a server error controls nothing. b, err := json.Marshal(models.WorkspaceExport{ + Version: 1, Workspace: models.WorkspaceExportMeta{Name: "Imported", Slug: "imported"}, }) if err != nil { @@ -124,8 +129,9 @@ func TestImportWorkspace_UnderTheLimitIsNotRefused(t *testing.T) { rr := importRequest(t, srv, user, "application/json", exportBody(t)) - if rr.Code == http.StatusForbidden { - t.Fatalf("import UNDER the plan limit was refused: %s", rr.Body.String()) + if rr.Code != http.StatusCreated { + t.Fatalf("import UNDER the plan limit returned %d, want 201 — asserting merely "+ + "\"not 403\" would pass on a 500 and prove nothing: %s", rr.Code, rr.Body.String()) } } @@ -139,8 +145,9 @@ func TestImportWorkspace_SelfHostedIsUnaffected(t *testing.T) { rr := importRequest(t, srv, user, "application/json", exportBody(t)) - if rr.Code == http.StatusForbidden { - t.Fatalf("self-hosted import was refused by a plan limit that should not apply: %s", rr.Body.String()) + if rr.Code != http.StatusCreated { + t.Fatalf("self-hosted import returned %d, want 201 — a plan limit must not apply where "+ + "there are no plans: %s", rr.Code, rr.Body.String()) } } @@ -153,7 +160,8 @@ func TestImportWorkspace_NoResolvedUserIsNotCharged(t *testing.T) { rr := importRequest(t, srv, nil, "application/json", exportBody(t)) - if rr.Code == http.StatusForbidden { - t.Fatalf("an import with no resolved user was charged against a plan: %s", rr.Body.String()) + if rr.Code != http.StatusCreated { + t.Fatalf("import with no resolved user returned %d, want 201 — there is nobody to charge: %s", + rr.Code, rr.Body.String()) } } From 6c40d1df2861f875e7855e7d5e7b8240da990b7d Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 03:19:55 +0000 Subject: [PATCH 3/4] docs(server): scope what the no-resolved-user import test actually pins (BUG-2793) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../server/handlers_workspace_import_plan_limit_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/server/handlers_workspace_import_plan_limit_test.go b/internal/server/handlers_workspace_import_plan_limit_test.go index 2966fdfd..4a715945 100644 --- a/internal/server/handlers_workspace_import_plan_limit_test.go +++ b/internal/server/handlers_workspace_import_plan_limit_test.go @@ -155,6 +155,13 @@ func TestImportWorkspace_SelfHostedIsUnaffected(t *testing.T) { // `userID != ""` guard. A legacy workspace token resolves no user, and there // is nobody to charge — the guard is not defensive padding, it is the // difference between "no limit applies" and a nil lookup. +// +// SCOPE, stated because this test locks in a 201 and someone will read that as +// approval: it pins the GUARD's behaviour, not a judgement that userless +// workspace creation is fine. It also drives the handler directly rather than +// through a real legacy token, so it does not prove that token shape reaches +// here — only that the guard does what create's does when no user resolves. +// Whether these doors should mint unowned workspaces at all is BUG-2809. func TestImportWorkspace_NoResolvedUserIsNotCharged(t *testing.T) { srv, _ := importLimitFixture(t, store.DefaultFreeLimits.Workspaces) From 813fcfcf1a525ff2cc020287295b0bf12ca43f84 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 03:32:57 +0000 Subject: [PATCH 4/4] test(server): pin the JSON-path placement and assert "not charged" as data (BUG-2793) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...ndlers_workspace_import_plan_limit_test.go | 50 ++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/internal/server/handlers_workspace_import_plan_limit_test.go b/internal/server/handlers_workspace_import_plan_limit_test.go index 4a715945..146f6478 100644 --- a/internal/server/handlers_workspace_import_plan_limit_test.go +++ b/internal/server/handlers_workspace_import_plan_limit_test.go @@ -121,6 +121,30 @@ func TestImportWorkspace_EnforcesThePlanLimitOnTheBundlePathToo(t *testing.T) { } } +// TestImportWorkspace_EnforcesThePlanLimitBeforeReadingTheJSONBody is the +// JSON-path half of the placement claim, and the refusal test above cannot +// make it: that one sends VALID JSON, so a gate placed after the decode would +// still return 403 and it would stay green (codex round 3). +// +// The code comment claims the gate sits above EITHER body read. The bundle +// test proves it for gzip. This proves it for JSON, by sending a body that +// cannot be decoded: reaching 403 rather than a decode error is only possible +// if nothing read the body first. +func TestImportWorkspace_EnforcesThePlanLimitBeforeReadingTheJSONBody(t *testing.T) { + atLimit := store.DefaultFreeLimits.Workspaces + srv, user := importLimitFixture(t, atLimit) + + rr := importRequest(t, srv, user, "application/json", []byte("{this is not json")) + + if rr.Code != http.StatusForbidden { + t.Fatalf("at-limit import with an undecodable body returned %d, want 403 — the gate is "+ + "running after the JSON decode: %s", rr.Code, rr.Body.String()) + } + if b := rr.Body.String(); !strings.Contains(b, "plan_limit_exceeded") { + t.Errorf("refused for the wrong reason — want the plan-limit code, got: %s", b) + } +} + // TestImportWorkspace_UnderTheLimitIsNotRefused is the control. Without it, a // gate that refused every import — or one wired to the wrong feature key — // passes both tests above while breaking the feature outright. @@ -163,7 +187,12 @@ func TestImportWorkspace_SelfHostedIsUnaffected(t *testing.T) { // here — only that the guard does what create's does when no user resolves. // Whether these doors should mint unowned workspaces at all is BUG-2809. func TestImportWorkspace_NoResolvedUserIsNotCharged(t *testing.T) { - srv, _ := importLimitFixture(t, store.DefaultFreeLimits.Workspaces) + srv, user := importLimitFixture(t, store.DefaultFreeLimits.Workspaces) + + before, err := srv.store.CheckUserLimit(user.ID, "workspaces") + if err != nil { + t.Fatalf("read the user's limit: %v", err) + } rr := importRequest(t, srv, nil, "application/json", exportBody(t)) @@ -171,4 +200,23 @@ func TestImportWorkspace_NoResolvedUserIsNotCharged(t *testing.T) { t.Fatalf("import with no resolved user returned %d, want 201 — there is nobody to charge: %s", rr.Code, rr.Body.String()) } + + // "Not charged" asserted as a FACT about the data, not inferred from a + // status code (codex round 3). A regression that quietly attributed the + // import to the at-limit fixture user would return 201 too, and pass on + // the check above alone. + after, err := srv.store.CheckUserLimit(user.ID, "workspaces") + if err != nil { + t.Fatalf("re-check the user's limit: %v", err) + } + if after.Current != before.Current { + t.Errorf("the fixture user's workspace count moved %d -> %d; an import with no resolved "+ + "user was attributed to them", before.Current, after.Current) + } + + var created models.Workspace + parseJSON(t, rr, &created) + if created.OwnerID != "" { + t.Errorf("workspace created with owner %q by a caller with no resolved user", created.OwnerID) + } }