Repository navigation
Conversation
cpojer
marked this pull request as ready for review
April 2, 2026 08:15
cpojer
pushed a commit
that referenced
this pull request
Sep 11, 2026
* fix(auth): reject deleted and suspended users in auth middleware
Both the API and dashboard auth middlewares only verified JWT signature
and expiry, never checking whether the user still existed in the
database. A deleted user retained full access for up to 30 days (the JWT
lifetime). The suspended_reason column was also never checked, so
suspended users could still call all API endpoints.
The admin auth middleware already performed this check correctly -- this
commit applies the same pattern to the regular auth middleware.
| Issue | Before | After |
|-------|--------|-------|
| Deleted user hits API with valid JWT | 200 OK -- middleware trusts JWT blindly | 401 user not found |
| Deleted user visits dashboard | Pages render with stale JWT data | Cookie cleared, redirect to /login |
| Suspended user hits API | 200 OK -- suspended_reason never checked | 403 account suspended |
| Suspended user visits dashboard | Pages render normally | Cookie cleared, redirect to /login |
| GET /usage with deleted user | 500 crash -- user!.billing_cycle_start on null | Blocked at middleware (401) |
| GET /projects/:id/usage with deleted user | 500 crash -- user!.billing_cycle_start on null | Blocked at middleware (401) |
| Redundant getUser() in route handlers | 4 extra DB queries per request across routes | Eliminated -- routes use c.get("user") from middleware |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(auth): distinguish admin vs usage-limit suspension in auth middleware
Usage-suspended users (requests_exceeded) must retain dashboard and API
access so they can inspect usage and upgrade their plan. Only
admin-suspended users should be fully blocked.
Adds isAdminSuspended() helper that treats known usage-limit reasons
as non-blocking, while any other suspended_reason (admin actions)
triggers a 403/redirect.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(api): simplify auth cleanup from review findings
- Remove redundant getUser() call in me.ts (middleware already provides it)
- Export REASON_REQUESTS_EXCEEDED constant and use it in usage-counter.ts
and index.ts instead of scattered string literals
- Fix seed.sql using wrong suspension reason (usage_limit_exceeded ->
requests_exceeded)
- Remove narration comment in dashboard middleware
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(api): remove redundant userId/userLogin context variables
The auth middleware now provides the full User object via c.get("user"),
making the separate userId and userLogin context variables redundant
state. All route handlers (regular + admin) now read from the user
object directly.
Also moves isAdminSuspended and REASON_REQUESTS_EXCEEDED out of env.ts
into a dedicated suspension.ts module, since suspension logic is domain
concern, not environment configuration.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* revert: keep original seed.sql suspension reason
The seed value usage_limit_exceeded was intentional — no reason to
change it to match the metering system constant.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cpojer
pushed a commit
that referenced
this pull request
Sep 11, 2026
…deploy) (#131) ## Summary Fixes the empirically-confirmed bug where **every sandbox project fails on its second deploy**, plus the latent silent misroute in deploys 1+ before PR #124 surfaced it. The bug: - Per-deploy WfP script names (`user-${slug}-${deployment.id}`) cause Cloudflare to mint a fresh DO `namespace_id` for the `Sandbox` class on every deploy. - Container apps' `durable_objects.namespace_id` is **immutable on PATCH**. - Deploy 1 binds the container to namespace NS-1; deploy 2 mints NS-2 and `assertNamespaceMatch` (PR #124) fails the deploy. - Pre-#124, the container kept routing traffic to the deploy-1 namespace silently; deploys "succeeded" but sandbox calls landed on stale instances. The fix: a stable per-project `do-host-${slug}` script owns the `Sandbox` class. User workers cross-script-bind to it. Container apps bind to the host's namespace once and live for the project's lifetime. Sandbox state survives redeploys. Design doc: [`meta/design-docs/0067-cross-script-do-host-architecture.md`](meta/design-docs/0067-cross-script-do-host-architecture.md). Two adversarial-review rounds before implementation; six review/fix rounds during implementation. ## What changes **Schema (additive, backward-compatible):** - `resources.metadata jsonb` — sandbox host info (`namespaceId`, `hostScriptName`) - `projects.status text` — gates new deploys against in-flight teardowns **New host script artifact:** - `packages/api/src/provisioning/sandbox-host-source/index.ts` — re-exports `Sandbox` from `@cloudflare/sandbox` (pinned `0.9.1`) - `packages/api/scripts/build-sandbox-host.ts` — tsdown bundler (`cloudflare:*` and `node:*` external; everything else bundled) - `packages/api/src/provisioning/sandbox-host-bundle.generated.ts` — committed deterministic artifact (BUNDLE_BYTES + SHA256 + COMPATIBILITY_DATE) - CI gate (`pnpm -C packages/api build:sandbox-host && git diff --exit-code`) catches drift on `@cloudflare/sandbox` bumps **`provisionSandboxHost` (the new core helper):** - Idempotent. Holds project-row `SELECT … FOR UPDATE` through CF reconciliation. - Honors `projects.status='deleting'` to abort safely (`ProjectDeletingError`). - Cached fast path verifies the live host's binding namespace; reconciles container config drift (image / instance_type / max_instances) via PATCH+rollout. - Create-or-recover handles fresh provisioning and partial-failure replay (read-tag-first PUT preserves namespace). When a same-name container app exists on a mismatched namespace (the pre-PR cutover state), throws a clear runbook-pointing error so operators can run a one-time `wrangler containers delete` per the cutover runbook — silently destroying a tenant's container state is exactly the failure mode this architecture exists to prevent. - Compensating DELETE if the resources UPSERT fails after the container POST — only deletes containers this attempt actually created. **Deploy pipeline integration:** - `provisionSandboxHost` runs early (after row insert, before metadata assembly), gated on `manifest.sandbox && hasWorkerFiles`. - Metadata-assembly helpers (`buildSandboxUploadBindings` / `buildSandboxUploadContainers` / `buildDurableObjectUploadMigrations`) accept a `crossScriptHost` arg: emit `script_name: do-host-${slug}` on the SANDBOX binding, force `class_name: 'Sandbox'` (the host's literal class), strip the user worker's `containers` block, drop the `Sandbox` migration. - Framework remains unchanged — local `wrangler dev` still uses same-script form. Platform rewrites at upload time. - Concurrent-deploy precheck restructured into an atomic `FOR UPDATE` with placeholder-row insert; second concurrent request observes the placeholder and 409s. **Project teardown (consolidated `teardownProject`):** - Used from public delete, admin project delete, admin user delete (one shared path; previously three divergent ones). - Takes `FOR UPDATE`, flips `status='deleting'`, marks all non-terminal deployments `'canceling'`, commits — then runs cleanup. Existing `assertDeploymentNotCanceled` calls in the deploy pipeline observe the cancellation. - Cleanup is split into **critical** (host script + container app — fatal on non-404) and **best-effort** (D1/KV/R2/queues/etc. — warnings). DB cascade only runs after critical succeeds; transient CF failures preserve DB pointers for retry. - Deterministic-name orphan recovery for containers without resource rows. **Route-flip race hardening:** - `assertProjectNotDeleting` helper called alongside every `assertDeploymentNotCanceled` (~15 sites); fails closed on missing project. - Final commit (deployment status='live' + project type) wrapped in a `FOR UPDATE` tx with post-lock 'deleting' check. - Catch path tracks every successful KV write (`writtenRoutingKeys`) — queue, slug, domain — and rolls back the exact set on teardown-race or pre-routing failure. Tracker is pushed AFTER each successful PUT (not before) so transient KV failures can't delete the previous deployment's live route. - Teardown-race branch also DELETEs the uploaded user worker to recover orphans. **Tests:** - 16 unit tests (`packages/api/test/unit/sandbox-host.test.ts`) — `provisionSandboxHost`, `putSandboxHostScript`, metadata helpers (test #5 asserts the cutover throw path, not auto-migration) - 11 integration tests + 1 intentional skip (`packages/api/test/integration/api-sandbox.test.ts`) — first deploy, redeploy, drift reconciliation, stale-host recovery, partial-failure replay, pre-PR cutover error event, concurrent-deploy 409, rollback preserves state, project teardown - Mock CF API extended with stateful host scripts + container apps + cross-script binding validation + call-order recording (455 existing integration tests still pass unchanged) **Documentation:** - `meta/lessons.md` — architectural lesson appended (notes the cutover is intentionally operator-driven) - `meta/operations.md` — "Pre-PR-#67 sandbox cutover" runbook + existing orphan-host runbook - Framework submodule bumped: `framework/docs/guide/sandboxes.md` rewritten to distinguish stable DO identity from ephemeral container state (cf. `@cloudflare/containers`'s `sleepAfter` default) ## Pre-PR cutover The 4 stuck sandbox tenants from before this PR (recappi, `dtnyzh0qckg3`, `2ves8mgqb4po`, `60c3lfqptd2i`) have stale CF container apps bound to per-deploy DO namespaces. Their next deploy after this PR ships will fail loudly with a runbook-pointing error. Operator runs the one-time cutover per [`meta/operations.md`](meta/operations.md) "Pre-PR-#67 sandbox cutover": notify each tenant 24h in advance, then `wrangler containers delete proj-<projectId>-void-sandbox` once per affected tenant before their next deploy. One-time sandbox state loss at cutover is unavoidable; documented. ## Out of scope (deferred follow-ups) - Silent DO storage rotation footgun for **non-sandbox** DO users (would need extending the host pattern to all user-declared DO classes) - Per-deploy churn for secrets, queue consumers, tail consumers, cron triggers - Pre-existing partial-routing-failure imperfection at `deploy.ts:3282-3297` - Slug rename for sandbox projects - `deploy.ts:85` has its own `DEFAULT_DISPATCH_NAMESPACE` literal (pre-existing duplicate of `constants.ts`)
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
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.
This PR fixes these CLI UX issues: