From fbfd0ccb5c91b16b64bfe82365af8bbf9a987d36 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Fri, 24 Jul 2026 15:09:54 -0400 Subject: [PATCH] fix: require an explicit workspace for admin credential issuance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four ADMIN_TOKEN-gated token/enrollment routes defaulted an omitted `workspace` to the communal `default` workspace, and `uploads admin invite create` hardcoded the same fallback. A forgotten field therefore issued a credential against the shared tenant instead of erroring. That is the accidental-co-tenancy path: enrollment redemption mints a token without creating an org membership (`redeemEnrollment`), so the recipient got files:read/files:write on `default`'s objects while appearing in no member list and in no account UI. - apps/api: `POST/GET/DELETE /admin/tokens` and `POST /admin/enrollments` now 400 `workspace_required` when the field is absent or blank. Malformed slugs keep the distinct `invalid_workspace` code so "you forgot it" doesn't read as "you typo'd it". Naming `default` explicitly still works. - CLI: `uploads admin invite create` requires `--workspace`. - /console: the workspace field no longer prefills `default`, and the invite handler refuses to submit a blank one rather than falling back. - Docs: admin-tokens.md documented the old default; ops.md's example now uses a named workspace. Prod state at time of change: no accidental co-tenancy exists — `default` has one member (the operator), every other workspace has one member except the team org, and there are zero pending invites and zero enrollment rows. This closes the path before it is used, rather than cleaning up after it. Verification: pnpm test 3115/3115, pnpm typecheck clean, changed files format-clean. Refs #505 --- .changeset/tidy-moons-invite.md | 11 ++ .../routes/admin-workspace-required.test.ts | 162 ++++++++++++++++++ apps/api/src/routes/admin.ts | 22 ++- apps/api/test/routes-auth.test.ts | 13 +- apps/web/src/pages/console.astro | 16 +- docs/admin-tokens.md | 17 +- docs/ops.md | 2 +- .../uploads/src/commands/admin-enrollment.ts | 12 +- packages/uploads/test/commands-login.test.ts | 36 +++- 9 files changed, 255 insertions(+), 36 deletions(-) create mode 100644 .changeset/tidy-moons-invite.md create mode 100644 apps/api/src/routes/admin-workspace-required.test.ts diff --git a/.changeset/tidy-moons-invite.md b/.changeset/tidy-moons-invite.md new file mode 100644 index 00000000..7a91cba8 --- /dev/null +++ b/.changeset/tidy-moons-invite.md @@ -0,0 +1,11 @@ +--- +"@buildinternet/uploads": minor +--- + +`uploads admin invite create` now requires `--workspace`. It previously defaulted +to the communal `default` workspace, so a forgotten flag issued an invite +granting `files:read`/`files:write` on the shared tenant — and because +enrollment redemption mints a token without creating an org membership, the +recipient would not have appeared in any member list while still reading and +writing that workspace's files. Pass the workspace explicitly; naming `default` +still works. diff --git a/apps/api/src/routes/admin-workspace-required.test.ts b/apps/api/src/routes/admin-workspace-required.test.ts new file mode 100644 index 00000000..2756d1b4 --- /dev/null +++ b/apps/api/src/routes/admin-workspace-required.test.ts @@ -0,0 +1,162 @@ +/** + * The four `/admin` token/enrollment routes used to default an omitted + * `workspace` to the communal `default` workspace. That made the shared tenant + * the silent target of credential issuance: an operator minting a token or an + * enrollment code "for a customer" and forgetting the field handed out + * files:read/files:write on `default` instead — and because enrollment + * redemption mints a token without creating an org membership + * (`redeemEnrollment` in auth-db.ts), the recipient would not appear in any + * member list while still reading and writing that workspace's objects. + * + * Requiring the field turns that silent mis-target into a 400. These tests pin + * the rule per route so the default cannot be reintroduced one handler at a + * time. + */ +import { Hono } from "hono"; +import { beforeAll, describe, expect, it } from "vitest"; +import { SqliteD1, database } from "../../test/helpers/sqlite-d1"; +import { respondError } from "../error-response"; +import { admin } from "./admin"; + +const ADMIN_TOKEN = "test-admin-token"; +const MIGRATIONS = [ + "migrations/20260710120000_auth.sql", + "migrations/20260712230000_token_minting_user.sql", +]; + +const RECORD = { + provider: "r2", + bucket: "shared", + binding: "UPLOADS_DEFAULT", + prefix: "acme/", + publicBaseUrl: "https://storage.uploads.sh", +}; + +beforeAll(() => { + if (typeof crypto.subtle.timingSafeEqual !== "function") { + ( + crypto.subtle as unknown as { timingSafeEqual: (a: Uint8Array, b: Uint8Array) => boolean } + ).timingSafeEqual = (a: Uint8Array, b: Uint8Array) => + a.length === b.length && a.every((byte, i) => byte === b[i]); + } +}); + +function appWith(db: SqliteD1) { + const app = new Hono<{ Bindings: Env }>() + .route("/admin", admin) + .onError((err, c) => respondError(c, err)); + const store = new Map( + Object.entries({ "ws:acme": RECORD, "ws:default": { ...RECORD, prefix: "default/" } }), + ); + const env = { + ADMIN_TOKEN, + REGISTRY: { + get: (async (key: string) => store.get(key) ?? null) as unknown as KVNamespace["get"], + }, + DB: database(db), + } as unknown as Env; + return { app, env }; +} + +function post(path: string, body: unknown) { + return new Request(`https://api.uploads.sh${path}`, { + method: "POST", + headers: { authorization: `Bearer ${ADMIN_TOKEN}`, "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +/** The four routes, each exercised with `workspace` absent. */ +const OMITTED: { name: string; request: () => Request }[] = [ + { + name: "POST /admin/tokens", + request: () => post("/admin/tokens", { label: "ci", scopes: ["files:read"] }), + }, + { + name: "POST /admin/enrollments", + request: () => post("/admin/enrollments", { label: "ci", scopes: ["files:read"] }), + }, + { + name: "GET /admin/tokens", + request: () => + new Request("https://api.uploads.sh/admin/tokens", { + headers: { authorization: `Bearer ${ADMIN_TOKEN}` }, + }), + }, + { + name: "DELETE /admin/tokens", + request: () => + new Request("https://api.uploads.sh/admin/tokens", { + method: "DELETE", + headers: { authorization: `Bearer ${ADMIN_TOKEN}`, "content-type": "application/json" }, + body: JSON.stringify({ label: "ci" }), + }), + }, +]; + +describe("admin routes require an explicit workspace", () => { + for (const route of OMITTED) { + it(`${route.name} rejects an omitted workspace with 400 workspace_required`, async () => { + const { app, env } = appWith(new SqliteD1(MIGRATIONS)); + const res = await app.request(route.request(), {}, env); + expect(res.status).toBe(400); + const body = (await res.json()) as { error?: { code?: string } }; + expect(body.error?.code).toBe("workspace_required"); + }); + + it(`${route.name} rejects a blank workspace with 400 workspace_required`, async () => { + const { app, env } = appWith(new SqliteD1(MIGRATIONS)); + const original = route.request(); + const url = new URL(original.url); + const blanked = + original.method === "GET" + ? new Request(`${url.origin}${url.pathname}?workspace=%20%20`, { + headers: original.headers, + }) + : new Request(original.url, { + method: original.method, + headers: original.headers, + body: JSON.stringify({ + ...(JSON.parse(await original.text()) as Record), + workspace: " ", + }), + }); + const res = await app.request(blanked, {}, env); + expect(res.status).toBe(400); + const body = (await res.json()) as { error?: { code?: string } }; + expect(body.error?.code).toBe("workspace_required"); + }); + } + + it("a malformed workspace stays invalid_workspace, distinct from omission", async () => { + const { app, env } = appWith(new SqliteD1(MIGRATIONS)); + const res = await app.request( + post("/admin/tokens", { workspace: "Not A Slug", label: "ci", scopes: ["files:read"] }), + {}, + env, + ); + expect(res.status).toBe(400); + const body = (await res.json()) as { error?: { code?: string } }; + expect(body.error?.code).toBe("invalid_workspace"); + }); + + it("an explicit workspace still works — the gate is on omission, not on the value", async () => { + const { app, env } = appWith(new SqliteD1(MIGRATIONS)); + const res = await app.request( + post("/admin/tokens", { workspace: "acme", label: "ci", scopes: ["files:read"] }), + {}, + env, + ); + expect(res.status).toBe(201); + }); + + it("naming the communal workspace explicitly is still allowed", async () => { + const { app, env } = appWith(new SqliteD1(MIGRATIONS)); + const res = await app.request( + post("/admin/tokens", { workspace: "default", label: "ci", scopes: ["files:read"] }), + {}, + env, + ); + expect(res.status).toBe(201); + }); +}); diff --git a/apps/api/src/routes/admin.ts b/apps/api/src/routes/admin.ts index caa69d97..54997243 100644 --- a/apps/api/src/routes/admin.ts +++ b/apps/api/src/routes/admin.ts @@ -1,5 +1,4 @@ import { renderEnrollmentInvitationEmail } from "@uploads/email"; -import { COMMUNAL_WORKSPACE } from "@uploads/workspace"; import { AppError, ConflictError, @@ -85,7 +84,17 @@ function labelValue(value: unknown): string | undefined | null { return label.length >= 1 && label.length <= 100 ? label : null; } +/** + * Omission and malformation are reported separately on purpose. These routes + * used to fall back to the communal workspace when `workspace` was absent, so + * the failure mode was a credential silently issued against the shared tenant + * rather than an error. A distinct `workspace_required` makes "you forgot the + * field" legible to an operator instead of reading as a typo'd slug. + */ function requireWorkspaceName(name: string): void { + if (!name) { + throw new ValidationError("workspace is required", { code: "workspace_required" }); + } if (!WS_NAME_RE.test(name)) { throw new ValidationError("invalid workspace", { code: "invalid_workspace" }); } @@ -174,7 +183,8 @@ export const admin = new Hono<{ Bindings: Env }>() return c.json({ created, existing }); }) - // Mint a scoped bearer token for an existing workspace (defaults to the communal one). + // Mint a scoped bearer token for an existing workspace. `workspace` is + // required — see `requireWorkspaceName` for why there is no default. // New credentials live in D1; legacy KV credentials remain readable/revocable. .post("/tokens", async (c) => { const body = await c.req @@ -193,7 +203,7 @@ export const admin = new Hono<{ Bindings: Env }>() expiresInDays?: number; }, ); - const name = body.workspace?.trim() || COMMUNAL_WORKSPACE; + const name = body.workspace?.trim() ?? ""; const label = labelValue(body.label); requireWorkspaceName(name); requireLabel(label); @@ -258,7 +268,7 @@ export const admin = new Hono<{ Bindings: Env }>() email?: string; }, ); - const name = body.workspace?.trim() || COMMUNAL_WORKSPACE; + const name = body.workspace?.trim() ?? ""; const label = labelValue(body.label); requireWorkspaceName(name); requireLabel(label); @@ -344,7 +354,7 @@ export const admin = new Hono<{ Bindings: Env }>() // Lists D1 credentials and legacy KV credentials without exposing secrets. .get("/tokens", async (c) => { - const name = c.req.query("workspace")?.trim() || COMMUNAL_WORKSPACE; + const name = c.req.query("workspace")?.trim() ?? ""; requireWorkspaceName(name); const record = await workspace(c, name); if (!record) { @@ -377,7 +387,7 @@ export const admin = new Hono<{ Bindings: Env }>() const body = await c.req .json<{ workspace?: string; hashPrefix?: string; label?: string }>() .catch(() => ({}) as { workspace?: string; hashPrefix?: string; label?: string }); - const name = body.workspace?.trim() || COMMUNAL_WORKSPACE; + const name = body.workspace?.trim() ?? ""; const hashPrefix = body.hashPrefix?.trim(); const label = body.label?.trim(); requireWorkspaceName(name); diff --git a/apps/api/test/routes-auth.test.ts b/apps/api/test/routes-auth.test.ts index 4c9e0d7a..f84d1a42 100644 --- a/apps/api/test/routes-auth.test.ts +++ b/apps/api/test/routes-auth.test.ts @@ -123,7 +123,7 @@ describe("auth routes", () => { { method: "POST", headers: { Authorization: "Bearer admin-secret", "Content-Type": "application/json" }, - body: JSON.stringify({ label: "x".repeat(101) }), + body: JSON.stringify({ workspace: "default", label: "x".repeat(101) }), }, env(), ); @@ -138,6 +138,7 @@ describe("auth routes", () => { method: "POST", headers: { Authorization: "Bearer admin-secret", "Content-Type": "application/json" }, body: JSON.stringify({ + workspace: "default", label: "remote-mcp-smoke", scopes: ["files:read", "files:write", "files:delete"], }), @@ -157,7 +158,7 @@ describe("auth routes", () => { { method: "POST", headers: { Authorization: "Bearer admin-secret", "Content-Type": "application/json" }, - body: JSON.stringify({ label: "routine-agent" }), + body: JSON.stringify({ workspace: "default", label: "routine-agent" }), }, env(), ); @@ -175,7 +176,7 @@ describe("auth routes", () => { { method: "POST", headers: { Authorization: "Bearer admin-secret", "Content-Type": "application/json" }, - body: JSON.stringify({ email: "adopter@example.com" }), + body: JSON.stringify({ workspace: "default", email: "adopter@example.com" }), }, env({ emailOutbox }), ); @@ -197,7 +198,7 @@ describe("auth routes", () => { { method: "POST", headers: { Authorization: "Bearer admin-secret", "Content-Type": "application/json" }, - body: JSON.stringify({ email: "adopter@example.com" }), + body: JSON.stringify({ workspace: "default", email: "adopter@example.com" }), }, env({ emailOutbox: [], emailThrows: true }), ); @@ -213,7 +214,7 @@ describe("auth routes", () => { { method: "POST", headers: { Authorization: "Bearer admin-secret", "Content-Type": "application/json" }, - body: JSON.stringify({ email: "not-an-email" }), + body: JSON.stringify({ workspace: "default", email: "not-an-email" }), }, env({ emailOutbox: [] }), ); @@ -228,7 +229,7 @@ describe("auth routes", () => { { method: "POST", headers: { Authorization: "Bearer admin-secret", "Content-Type": "application/json" }, - body: JSON.stringify({ email: "Adopter@Example.com" }), + body: JSON.stringify({ workspace: "default", email: "Adopter@Example.com" }), }, env({ emailOutbox: [], inviteAllowed: false, inviteKeys }), ); diff --git a/apps/web/src/pages/console.astro b/apps/web/src/pages/console.astro index 4d70e7e3..1cb3bbd8 100644 --- a/apps/web/src/pages/console.astro +++ b/apps/web/src/pages/console.astro @@ -2,7 +2,6 @@ // Minimal operator console: paste a bearer token, list usage + files via the public API. // No server-side secrets — token stays in the browser. Scaffold toward a fuller files-sdk UI. import { env } from "cloudflare:workers"; -import { COMMUNAL_WORKSPACE } from "@uploads/workspace"; import { resolveConsoleMode } from "../lib/console-mode"; import Brand from "../components/Brand.astro"; import BaseHead from "../components/BaseHead.astro"; @@ -185,7 +184,7 @@ const authOrigin =