Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .changeset/quiet-lions-retire.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
"@buildinternet/uploads": minor
---

`uploads config init` with no flags no longer seeds `UPLOADS_WORKSPACE=default`
into the config file. That entry outranked the workspace encoded in your token,
so it pinned every later `uploads login` to `default` no matter which workspace
the token was actually minted for. It now seeds only `UPLOADS_API_URL`; pass
`--workspace <name>` to set one explicitly.

`uploads login` on an account with no workspace yet now offers a name derived
from your GitHub login as a bracketed default (`… (lowercase, hyphens)
[octocat]:`). Press Enter to accept it, or type anything else to override. When
no name can be derived — no linked GitHub account, or the derived name is
reserved or already taken — the prompt is exactly as before.
1 change: 0 additions & 1 deletion apps/api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
"@uploads/email": "workspace:^",
"@uploads/errors": "workspace:^",
"@uploads/storage": "workspace:^",
"@uploads/workspace": "workspace:^",
"hono": "^4.12.28",
"mediabunny": "^1.50.9"
},
Expand Down
23 changes: 10 additions & 13 deletions apps/api/src/content-hash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
* Design: `.context/479-content-hash-inheritance.md`.
*/

import { isCommunalWorkspace } from "@uploads/workspace";
import { addMissingFileMetadata, getFileMetadata } from "./file-metadata";

/**
Expand All @@ -22,8 +21,8 @@ import { addMissingFileMetadata, getFileMetadata } from "./file-metadata";
*
* Restated rather than imported, and deliberately so. The vocabulary lives in
* `@buildinternet/uploads`, which is **published** and carries no `@uploads/*`
* workspace dependencies — extracting the list to a shared private package (the
* `@uploads/workspace` pattern from #504) would make the published package
* workspace dependencies — extracting the list to a shared private package
* would make the published package
* depend on a private one, and importing the CLI package here would invert the
* dependency and drag CLI deps toward a Worker bundle. This follows the other
* in-repo precedent instead: `packages/billing/src/workspace-cap.ts`, which
Expand Down Expand Up @@ -88,23 +87,21 @@ export async function recordContentHash(
* Derived metadata to inherit for `contentSha256`, or `{}` when there is
* nothing to inherit.
*
* Returns `{}` unconditionally for the communal workspace. `default` is the one
* shared tenant every account belongs to (`packages/workspace`), so
* same-workspace — the boundary issue #479 originally proposed — would let one
* user's `path=/admin/billing` land on an unrelated user's upload of the same
* bytes. Applied in both cloud and self-hosted: inheriting across unrelated
* members of a shared tenant is surprising in either deployment, and a
* trust rule that differs per deployment is one every future cross-tenant
* feature would have to reason about twice.
* Workspace membership is the trust boundary, with no workspace excluded.
* Issue #479 originally carved out `default` because it was the communal
* tenant every account belonged to, where "same workspace" said nothing about
* whether two uploaders were related. That concept is retired (#505): every
* workspace is now an ordinary one whose members are there deliberately. Since
* members can already read each other's files, inheriting metadata from a file
* the caller could simply open discloses nothing new — so the rule is uniform,
* in both cloud and self-hosted.
*/
export async function inheritableMetaForHash(
db: D1Database,
workspace: string,
contentSha256: string,
selfKey: string,
): Promise<Record<string, string>> {
if (isCommunalWorkspace(workspace)) return {};

// Fail-soft, deliberately. This runs *after* the bytes are durably stored, so
// a D1 blip here must cost the convenience (metadata the caller can re-state)
// rather than the upload (bytes the caller would have to re-send). The same
Expand Down
20 changes: 16 additions & 4 deletions apps/api/src/routes/internal-billing.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { COMMUNAL_WORKSPACE } from "@uploads/workspace";
import { Hono } from "hono";
import { beforeAll, describe, expect, it } from "vitest";
import { fakeRegistry } from "../../test/fake-kv";
Expand Down Expand Up @@ -281,12 +280,25 @@ describe("GET /internal/billing/member-cap", () => {
expect(body.message).toBeNull();
});

it("exempts the communal workspace even if a plan is stamped on it", async () => {
// `default` used to be exempt from the member cap as the communal tenant.
// That exemption is gone (#505): it is capped by whatever plan it carries,
// like any other workspace. Its live record carries no plan and no
// `selfServe`, so it still resolves to unlimited on its own — but a plan
// stamped on it now applies, which is the behavior change this pins.
it("applies the cap to `default` when a plan is stamped on it — no exemption", async () => {
const registry = fakeRegistry({
[COMMUNAL_WORKSPACE]: { provider: "r2", bucket: "b", plan: "free", selfServe: true },
default: { provider: "r2", bucket: "b", plan: "free", selfServe: true },
});
const env = { REGISTRY: registry, BILLING_INTERNAL_KEY: SECRET } as unknown as Env;
const res = await get(COMMUNAL_WORKSPACE, { "x-internal-billing-key": SECRET }, env);
const res = await get("default", { "x-internal-billing-key": SECRET }, env);
expect(res.status).toBe(200);
expect((await res.json()) as { cap: number | null }).toMatchObject({ cap: 3 });
});

it("leaves `default` unlimited with no plan stamped — matching its live record", async () => {
const registry = fakeRegistry({ default: { provider: "r2", bucket: "b" } });
const env = { REGISTRY: registry, BILLING_INTERNAL_KEY: SECRET } as unknown as Env;
const res = await get("default", { "x-internal-billing-key": SECRET }, env);
expect(res.status).toBe(200);
expect((await res.json()) as { cap: number | null }).toMatchObject({ cap: null });
});
Expand Down
11 changes: 1 addition & 10 deletions apps/api/src/routes/internal-billing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import {
type PlanId,
} from "@uploads/billing";
import { UnauthorizedError, ValidationError } from "@uploads/errors";
import { isCommunalWorkspace } from "@uploads/workspace";
import { Hono } from "hono";
import type { Context } from "hono";
import { jsonBody } from "./json-body";
Expand Down Expand Up @@ -143,15 +142,7 @@ internalBilling.get("/member-cap", async (c) => {
throw new ValidationError("workspace is required", { code: "invalid_workspace" });
}

// The communal workspace is exempt from the member cap, the same way it's
// already exempt from the other member flows (see slug-policy.ts). Today it
// also resolves to unlimited on its own — it's operator-provisioned, so it
// has neither a `plan` nor `selfServe` — but skipping the lookup means an
// operator stamping a plan on it can't accidentally cap the shared
// workspace.
const record = isCommunalWorkspace(workspace)
? null
: await loadWorkspaceRecord(c.env, workspace);
const record = await loadWorkspaceRecord(c.env, workspace);
const cap = record ? resolveMemberCap(record) : null;

return c.json({
Expand Down
25 changes: 1 addition & 24 deletions apps/api/src/routes/me.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { readFileSync } from "node:fs";
import { DatabaseSync, type SQLInputValue } from "node:sqlite";
import { fileURLToPath, URL as NodeURL } from "node:url";
import { COMMUNAL_WORKSPACE } from "@uploads/workspace";
import { Hono } from "hono";
import { describe, expect, it } from "vitest";
import { respondError } from "../error-response";
Expand Down Expand Up @@ -78,33 +77,12 @@ describe("GET /me/workspaces", () => {
workspace: "acme",
organization: { id: "org1", slug: "acme", name: "Acme Inc" },
role: "owner",
communal: false,
hasPublicUrl: false,
plan: "free",
},
]);
});

it("marks the communal workspace so the account UI doesn't infer it from the slug", async () => {
const env = stubEnv(USER, (path) => {
if (path === "/internal/memberships") {
return Response.json(
ownedMemberships([
[COMMUNAL_WORKSPACE, "owner"],
["acme", "owner"],
]),
);
}
return new Response(null, { status: 404 });
});
const res = await app().request("/me/workspaces", {}, env);
const body = (await res.json()) as { workspaces: { workspace: string; communal: boolean }[] };
expect(body.workspaces.map((ws) => [ws.workspace, ws.communal])).toEqual([
[COMMUNAL_WORKSPACE, true],
["acme", false],
]);
});

it("flags hasPublicUrl for a workspace whose storage record has a publicBaseUrl", async () => {
const env = stubEnv(USER, (path) => {
if (path === "/internal/memberships") {
Expand Down Expand Up @@ -204,7 +182,7 @@ describe("GET /me/workspaces", () => {
expect(await quotaFor(env)).toEqual({ used: 2, cap: 3, allowed: true });
});

it("loads the communal workspace's record like any other — the flag is not a short-circuit", async () => {
it("loads `default`'s record like any other workspace — no name-based short-circuit", async () => {
const env = stubEnv(USER, (path) => {
if (path === "/internal/memberships") {
return Response.json([
Expand All @@ -228,7 +206,6 @@ describe("GET /me/workspaces", () => {
workspace: "default",
organization: { id: "org2", slug: "default", name: "Default" },
role: "member",
communal: true,
hasPublicUrl: false,
plan: "free",
},
Expand Down
15 changes: 1 addition & 14 deletions apps/api/src/routes/me.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import { resolveWorkspaceCreateQuota } from "@uploads/billing";
import { ForbiddenError, NotFoundError, RateLimitedError, ValidationError } from "@uploads/errors";
import { createFilesRouter, signedDownloadUrl } from "@uploads/storage";
import { isCommunalWorkspace } from "@uploads/workspace";
import { Hono, type Context } from "hono";
import { usageWithLimits } from "../budget";
import { throwForInviteError } from "../invite-error";
Expand Down Expand Up @@ -53,20 +52,11 @@ interface MyWorkspace {
workspace: string;
organization: { id: string; slug: string; name: string };
role: string;
/**
* True for the shared tenant every account belongs to. Sent so clients
* don't have to re-derive a privileged-workspace rule from the slug: the
* account UI skips the communal workspace when picking which workspace to
* auto-open, and that is a server-owned fact, not a naming convention the
* browser should be pattern-matching. Free to compute — no extra lookup.
*/
communal: boolean;
}

function myWorkspaceFromMembership(membership: Membership, workspace: string): MyWorkspace {
return {
workspace,
communal: isCommunalWorkspace(workspace),
organization: {
id: membership.organizationId,
slug: membership.organizationSlug,
Expand Down Expand Up @@ -191,9 +181,7 @@ export const me = new Hono<SessionVars>()
// without a per-workspace billing round trip. Both fields are loaded here
// (not in `myWorkspaces`, which every member-gated route calls for
// authorization) so the extra KV read per workspace stays confined to this
// one listing endpoint. `communal` rides along from `myWorkspaces` — it
// costs no lookup, and it's what keeps the account UI from inferring the
// shared-tenant rule from the slug.
// one listing endpoint.
.get("/workspaces", async (c) => {
const userId = c.get("sessionUser")?.id;
if (!userId) throw new NotFoundError("no session user", { code: "workspace_not_found" });
Expand Down Expand Up @@ -262,7 +250,6 @@ export const me = new Hono<SessionVars>()
workspace: ws.workspace,
organization: ws.organization,
role: ws.role,
communal: ws.communal,
hasPublicUrl: Boolean(publicBaseUrl),
publicBaseUrl,
usage,
Expand Down
47 changes: 46 additions & 1 deletion apps/api/src/routes/tokens.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ interface EnvOpts {
db?: D1Database;
/** When set, the WRITE_LIMITER binding reports this success value. */
writeLimitOk?: boolean;
/** Seeds the `ghlogin:<userId>` cache the workspace suggestion reads. */
githubLogin?: string;
}

function stubEnv(opts: EnvOpts = {}): Env {
Expand All @@ -53,6 +55,7 @@ function stubEnv(opts: EnvOpts = {}): Env {
workspaces = { acme: { provider: "r2", bucket: "b" } },
db = captureDb().db,
writeLimitOk,
githubLogin,
} = opts;

const auth = stubAuth((req) => {
Expand Down Expand Up @@ -80,7 +83,18 @@ function stubEnv(opts: EnvOpts = {}): Env {
const WRITE_LIMITER =
writeLimitOk === undefined ? undefined : { limit: async () => ({ success: writeLimitOk }) };

return { AUTH: auth, REGISTRY: registry, DB: db, WRITE_LIMITER } as unknown as Env;
const GITHUB_CACHE = {
get: async (key: string) => (githubLogin && key === `ghlogin:${USER.id}` ? githubLogin : null),
put: async () => {},
};

return {
AUTH: auth,
REGISTRY: registry,
DB: db,
WRITE_LIMITER,
GITHUB_CACHE,
} as unknown as Env;
}

function app() {
Expand Down Expand Up @@ -216,6 +230,37 @@ describe("GET /v1/tokens (workspace listing)", () => {
expect(res.status).toBe(401);
});

// Issue #506: a name to prefill, offered only to an account with nothing to
// pick from. It is a hint — nothing is created and no name is claimed.
it("suggests a workspace derived from the GitHub login when the account has none", async () => {
const env = stubEnv({ memberships: [], workspaces: {}, githubLogin: "Octocat" });
const res = await app().request("/v1/tokens", { headers: { authorization: "Bearer s" } }, env);
expect(await res.json()).toEqual({ workspaces: [], suggestedWorkspace: "octocat" });
});

it("omits the suggestion when the derived name is already taken", async () => {
const env = stubEnv({
memberships: [],
workspaces: { octocat: { provider: "r2", bucket: "b" } },
githubLogin: "Octocat",
});
const res = await app().request("/v1/tokens", { headers: { authorization: "Bearer s" } }, env);
expect(await res.json()).toEqual({ workspaces: [] });
});

it("omits the suggestion when no GitHub login resolves", async () => {
const env = stubEnv({ memberships: [], workspaces: {} });
const res = await app().request("/v1/tokens", { headers: { authorization: "Bearer s" } }, env);
expect(await res.json()).toEqual({ workspaces: [] });
});

it("does not suggest for an account that already has a workspace", async () => {
const env = stubEnv({ githubLogin: "Octocat" });
const res = await app().request("/v1/tokens", { headers: { authorization: "Bearer s" } }, env);
const body = (await res.json()) as { suggestedWorkspace?: string };
expect(body.suggestedWorkspace).toBeUndefined();
});

it("lists memberships whose workspace still exists in KV", async () => {
const env = stubEnv({
memberships: [
Expand Down
16 changes: 15 additions & 1 deletion apps/api/src/routes/tokens.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
type SessionVars,
} from "../session-auth";
import { loadWorkspaceRecord, WS_NAME_RE } from "../workspace";
import { suggestWorkspaceName } from "../workspace-suggestion";

const MAX_BODY_BYTES = 4096;
const MAX_LABEL_LEN = 200;
Expand Down Expand Up @@ -139,7 +140,20 @@ export const tokens = new Hono<SessionVars>()
}),
)
).filter((w): w is { workspace: string; role: string } => w !== null);
return c.json({ workspaces });

// Name to prefill when this account is about to create its first
// workspace (#506). Only computed when there is nothing to pick from —
// a user who already has a workspace is not being asked to name one, so
// the GitHub round-trip would be wasted. Omitted entirely when no clean
// candidate exists; the field being absent means "offer nothing", which
// is the pre-#506 behavior.
const suggestedWorkspace =
workspaces.length === 0 ? await suggestWorkspaceName(c.env, user.id) : null;

return c.json({
workspaces,
...(suggestedWorkspace ? { suggestedWorkspace } : {}),
});
})
.post("/", sessionAuth, requireSessionUser, async (c) => {
const contentLength = Number(c.req.header("Content-Length") ?? 0);
Expand Down
29 changes: 15 additions & 14 deletions apps/api/src/slug-policy.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,26 @@
import { COMMUNAL_WORKSPACE } from "@uploads/workspace";
import { WS_NAME_RE } from "./workspace";
import { SLUG_BLOCKLIST, SLUG_BLOCKLIST_ALLOW } from "./slug-blocklist";

/**
* Names that collide with routes or subdomains.
*
* `COMMUNAL_WORKSPACE` is redundant with the existing-name check today (the
* built-in communal workspace is always registered, so self-serve
* registration would already 409 `workspace_name_taken` without this entry)
* — but it stays reserved deliberately, not just for symmetry. Hard-delete is
* the only path that ever frees a slug (see docs/deletion.md), and the
* communal workspace is uniquely privileged: it's the communal/agent
* workspace exempted from the reaper and referenced by ops tooling, docs, and
* bootstrap scripts by name. If it were ever hard-deleted (accidentally or
* deliberately) and this entry were removed, self-serve registration would
* let any signed-in user reclaim the slug for an unrelated, non-communal
* workspace — a name-squatting/confusion risk this list exists to close for
* the other entries. Keeping it here costs nothing and closes that latent gap.
* `"default"` is redundant with the existing-name check today — it is a
* registered workspace, so self-serve registration would already 409
* `workspace_name_taken` without this entry. It stays reserved anyway.
* Hard-delete is the only path that ever frees a slug (see docs/deletion.md),
* and `default` is the oldest workspace: years of public
* `storage.uploads.sh/default/…` URLs are embedded in merged PRs, issues, and
* chat history. If it were ever hard-deleted and this entry were gone,
* self-serve registration would let any signed-in user claim the slug and
* start serving their own bytes at paths those links still point at. Keeping
* it costs nothing and closes that gap.
*
* This is the only thing left that treats `default` differently, and it is a
* squatting guard on a name that served public traffic — not a claim that the
* workspace itself is special. It isn't; see the retired communal concept.
*/
export const RESERVED_WORKSPACE_NAMES: ReadonlySet<string> = new Set([
COMMUNAL_WORKSPACE,
"default",
"admin",
"api",
"www",
Expand Down
Loading
Loading