Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
17 changes: 5 additions & 12 deletions src/api/auth-middleware.ts
Original file line number Diff line number Diff line change
@@ -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 ───────────────────────────────────────────
Expand Down Expand Up @@ -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;
Expand Down
6 changes: 2 additions & 4 deletions src/api/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 →
Expand Down
32 changes: 18 additions & 14 deletions src/api/routes/proxy.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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);

Expand Down
17 changes: 17 additions & 0 deletions src/workspace/workspace-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
25 changes: 17 additions & 8 deletions test/integration/api/proxy-route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 9 additions & 5 deletions test/unit/api/workspace-context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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" });
Expand All @@ -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" });

Expand All @@ -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");
}
});

Expand Down