diff --git a/src/api/auth-middleware.ts b/src/api/auth-middleware.ts index b692473f..1587ca74 100644 --- a/src/api/auth-middleware.ts +++ b/src/api/auth-middleware.ts @@ -1,6 +1,6 @@ import type { EventSink } from "../engine/types.ts"; import type { IdentityProvider, UserIdentity } from "../identity/provider.ts"; -import type { WorkspaceStore } from "../workspace/workspace-store.ts"; +import { isWorkspaceMember, type WorkspaceStore } from "../workspace/workspace-store.ts"; import { constantTimeEqual, validateInternalToken } from "./auth-utils.ts"; // ── Auth mode detection ─────────────────────────────────────────── @@ -137,18 +137,11 @@ export async function resolveWorkspace( throw new WorkspaceResolutionError("Invalid workspace ID format.", 400); } - // Validate membership + // Validate membership. Unauthorized access and non-existence are reported identically + // — same message, same status — so callers cannot probe for workspace existence. const workspace = await workspaceStore.get(workspaceId); - if (!workspace) { - throw new WorkspaceResolutionError(`Workspace "${workspaceId}" not found.`, 400); - } - - const isMember = workspace.members.some((m) => m.userId === identity.id); - if (!isMember) { - throw new WorkspaceResolutionError( - `Access denied: not a member of workspace "${workspaceId}".`, - 403, - ); + if (!isWorkspaceMember(workspace, identity.id)) { + throw new WorkspaceResolutionError("Access denied to workspace.", 403); } return workspaceId; diff --git a/src/api/handlers.ts b/src/api/handlers.ts index 532a5466..3517bfee 100644 --- a/src/api/handlers.ts +++ b/src/api/handlers.ts @@ -28,7 +28,7 @@ import { validateToolInput } from "../tools/validate-input.ts"; import { estimateCost } from "../usage/cost.ts"; import { bytesToBase64 } from "../util/base64.ts"; import { PersonalWorkspaceInvariantError } from "../workspace/errors.ts"; -import type { WorkspaceStore } from "../workspace/workspace-store.ts"; +import { isWorkspaceMember, type WorkspaceStore } from "../workspace/workspace-store.ts"; import type { ConversationEventManager } from "./conversation-events.ts"; import type { SseEventManager } from "./events.ts"; import { ChatRequestBody, ToolCallRequestEnvelope } from "./schemas/rest.ts"; @@ -1004,9 +1004,7 @@ export async function handleBootstrap( // 1. Workspaces the user is a member of const allWorkspaces = await runtime.getWorkspaceStore().list(); - const userWorkspaces = allWorkspaces.filter((ws) => - ws.members.some((m) => m.userId === identity.id), - ); + const userWorkspaces = allWorkspaces.filter((ws) => isWorkspaceMember(ws, identity.id)); // Invariant (Phase 1): authenticated users have at least one workspace. // Provisioning runs at the identity boundary (provider.provisionUser → diff --git a/src/api/routes/proxy.ts b/src/api/routes/proxy.ts index bfbccc6d..85ebd695 100644 --- a/src/api/routes/proxy.ts +++ b/src/api/routes/proxy.ts @@ -1,5 +1,6 @@ import { Hono } from "hono"; import { log } from "../../cli/log.ts"; +import { isWorkspaceMember } from "../../workspace/workspace-store.ts"; import { WORKSPACE_ID_RE } from "../auth-middleware.ts"; import { requireAuth } from "../middleware/auth.ts"; import { errorLog } from "../middleware/error-log.ts"; @@ -74,21 +75,24 @@ export function proxyRoutes(ctx: AppContext) { } const ws = await ctx.workspaceStore.get(wsId); - if (!ws) { - log.info(`[proxy] ${method} ${requestPath} → 400 (workspace not found)`); - return apiError(400, "workspace_error", `Workspace "${wsId}" not found.`); - } const identity = c.var.identity; - if (identity) { - const isMember = ws.members.some((m) => m.userId === identity.id); - if (!isMember) { - log.info(`[proxy] ${method} ${requestPath} → 403 (not a member of ${wsId})`); - return apiError( - 403, - "workspace_error", - `Access denied: not a member of workspace "${wsId}".`, - ); - } + + // Authenticated callers: fold "unknown workspace" and "not a member" into + // one generic 403 with no ID echo, so they can't probe for workspace + // existence/membership (issue #17). This mirrors `resolveWorkspace` — the + // proxy reimplements the check here because the wsId is a path param, not + // the X-Workspace-Id header. Server-side logs keep the wsId; logs aren't + // the disclosure surface. + if (identity && !isWorkspaceMember(ws, identity.id)) { + log.info(`[proxy] ${method} ${requestPath} → 403 (access denied to ${wsId})`); + return apiError(403, "workspace_error", "Access denied to workspace."); + } + // `ws` is non-null past here for authenticated callers (membership implies + // existence). It can still be null in dev mode (no identity), where there + // is no disclosure surface — guard before dereferencing below. + if (!ws) { + log.info(`[proxy] ${method} ${requestPath} → 404 (workspace not found)`); + return apiError(404, "workspace_error", "Workspace not found."); } c.set("workspaceId", wsId); diff --git a/src/workspace/workspace-store.ts b/src/workspace/workspace-store.ts index 5f3b7bbc..1614846f 100644 --- a/src/workspace/workspace-store.ts +++ b/src/workspace/workspace-store.ts @@ -140,6 +140,23 @@ export function personalWorkspaceSlugFor(userId: string): string { return personalWorkspaceIdFor(userId).slice(3); } +/** + * True iff `userId` is a member of `ws`. + * + * Accepts a nullish `ws` on purpose: a caller that just did a `.get()` can fold + * "workspace doesn't exist" and "exists but caller isn't a member" into one + * branch — a non-existent workspace is, trivially, one nobody is a member of. + * Folding the two is a deliberate information-disclosure mitigation (issue #17): + * the API must not let an authenticated caller distinguish those states, or the + * status/message becomes an oracle for probing other tenants' workspace ids. + * This is the single definition of "is a member" for every authorization gate — + * keep membership checks routed through it so the two states can't drift apart + * on one surface and stay folded on another. + */ +export function isWorkspaceMember(ws: Workspace | null | undefined, userId: string): boolean { + return ws?.members.some((m) => m.userId === userId) ?? false; +} + // ── WorkspaceStore ───────────────────────────────────────────────── export class WorkspaceStore { diff --git a/test/integration/api/proxy-route.test.ts b/test/integration/api/proxy-route.test.ts index ca202dab..be801516 100644 --- a/test/integration/api/proxy-route.test.ts +++ b/test/integration/api/proxy-route.test.ts @@ -3,8 +3,9 @@ * * Covers: * - Workspace ID validation (path-traversal guard) - * - Workspace existence check - * - Membership enforcement (DevIdentityProvider sets identity = usr_default) + * - Membership enforcement (DevIdentityProvider sets identity = usr_default); + * non-existent and non-member workspaces return the same generic 403 with + * no ID echo, so a caller can't probe for workspace existence (issue #17) * - Per-workspace kill switch (allowHttpProxy = false) * - Bundle / mount existence checks * - Upstream unreachable → 502 @@ -177,20 +178,28 @@ describe("proxy route — workspace ID validation", () => { expect(body.error).toBe("workspace_error"); }); - it("400 when wsId references a workspace that doesn't exist", async () => { - const res = await fetch(`${baseUrl}/v1/ws/ws_does_not_exist/apps/${BUNDLE_NAME}/${MOUNT}/`); - expect(res.status).toBe(400); - const body = (await res.json()) as { error: string }; + it("403 (not 400) when wsId references a non-existent workspace — no existence oracle (#17)", async () => { + const ghostId = "ws_does_not_exist"; + const res = await fetch(`${baseUrl}/v1/ws/${ghostId}/apps/${BUNDLE_NAME}/${MOUNT}/`); + // Same response as the not-a-member case below: an authenticated caller + // can't distinguish "doesn't exist" from "exists but forbidden". + expect(res.status).toBe(403); + const body = (await res.json()) as { error: string; message: string }; expect(body.error).toBe("workspace_error"); + expect(body.message).toBe("Access denied to workspace."); + expect(JSON.stringify(body)).not.toContain(ghostId); }); }); describe("proxy route — auth / membership", () => { - it("403 when authenticated identity is not a member of the workspace", async () => { + it("403 with a generic, ID-free message when identity is not a member (#17)", async () => { const res = await fetch(proxyUrl(NON_MEMBER_WS)); expect(res.status).toBe(403); - const body = (await res.json()) as { error: string }; + const body = (await res.json()) as { error: string; message: string }; expect(body.error).toBe("workspace_error"); + expect(body.message).toBe("Access denied to workspace."); + // Must not echo the workspace id back to a non-member. + expect(JSON.stringify(body)).not.toContain(NON_MEMBER_WS); }); }); diff --git a/test/integration/stage-1-cross-workspace-conversation.test.ts b/test/integration/stage-1-cross-workspace-conversation.test.ts index 0c545682..4508a887 100644 --- a/test/integration/stage-1-cross-workspace-conversation.test.ts +++ b/test/integration/stage-1-cross-workspace-conversation.test.ts @@ -213,7 +213,10 @@ describe("Stage 1 — conversations outlive their workspace context", () => { expect(refusedRes.status).toBe(403); const refusedBody = await refusedRes.json(); expect(refusedBody.error).toBe("workspace_error"); - expect(refusedBody.message).toMatch(/not a member/i); + // Generic, ID-free message: workspace resolution failures don't reveal + // whether the workspace is unknown or merely denies membership (issue #17). + expect(refusedBody.message).toBe("Access denied to workspace."); + expect(refusedBody.message).not.toContain(sharedA); // 6. Continuing the same conversation in a DIFFERENT workspace // Alice is still a member of works. The conversation is diff --git a/test/unit/api/workspace-context.test.ts b/test/unit/api/workspace-context.test.ts index cfe9721e..513eb723 100644 --- a/test/unit/api/workspace-context.test.ts +++ b/test/unit/api/workspace-context.test.ts @@ -125,7 +125,7 @@ describe("resolveWorkspace", () => { expect(createdFor).toHaveLength(0); }); - it("returns 403 when user is not a member of the specified workspace", async () => { + it("returns generic 403 when user is not a member of the specified workspace", async () => { const ws = await workspaceStore.create("Forbidden WS"); // Don't add the identity as a member const identity = makeIdentity({ id: "usr_nonmember" }); @@ -139,11 +139,13 @@ describe("resolveWorkspace", () => { expect(err).toBeInstanceOf(WorkspaceResolutionError); const wsErr = err as WorkspaceResolutionError; expect(wsErr.statusCode).toBe(403); - expect(wsErr.message).toContain("Access denied"); + expect(wsErr.message).toBe("Access denied to workspace."); + // Must not echo the workspace ID (information disclosure). + expect(wsErr.message).not.toContain(ws.id); } }); - it("returns 400 when X-Workspace-Id references a non-existent workspace", async () => { + it("returns generic 403 when X-Workspace-Id references a non-existent workspace", async () => { const identity = makeIdentity({ id: "usr_badref" }); const req = makeRequest({ "x-workspace-id": "ws_doesnotexist" }); @@ -153,8 +155,10 @@ describe("resolveWorkspace", () => { } catch (err) { expect(err).toBeInstanceOf(WorkspaceResolutionError); const wsErr = err as WorkspaceResolutionError; - expect(wsErr.statusCode).toBe(400); - expect(wsErr.message).toContain("not found"); + // Same response as "not a member" so callers cannot probe for existence. + expect(wsErr.statusCode).toBe(403); + expect(wsErr.message).toBe("Access denied to workspace."); + expect(wsErr.message).not.toContain("ws_doesnotexist"); } });