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 =