diff --git a/mcp/CLAUDE.md b/mcp/CLAUDE.md index f958218e2b..2b6a3a1d52 100644 --- a/mcp/CLAUDE.md +++ b/mcp/CLAUDE.md @@ -324,14 +324,22 @@ mcp/ ├── docs/backend-handoff.md # findings for the storyteller-web owner; not our work ├── docs/infra-request.md # one-time batched asks for the backend team (Cloudflare, secrets, Google) ├── fake-upstream/ # separate Worker faking the allowlisted API routes; never imported by src/ -├── scripts/gen-api.ts # fetch api.json → openapi-typescript +├── scripts/gen-api.mjs # fetch api.json → trim to allowlist → test/fixtures/api.json + src/upstream/schema.d.ts ├── src/ │ ├── index.ts # Worker entry: OAuthProvider wrapping the Hono app │ ├── app.ts # Hono routes │ ├── auth/ # provider config, Authenticator impls, consent page (Hono JSX) │ ├── tokens/ # TokenResolver, Principal, GrantStore adapter -│ ├── upstream/ # UpstreamClient, UpstreamCredential, allowlist, generated schema +│ ├── upstream/ +│ │ ├── allowlist.json # the ONLY upstream routes, with `use: auth | read`; also drives the generator +│ │ ├── allowlist.ts # validation + lookups by template path and by concrete pathname +│ │ ├── credential.ts # UpstreamCredential interface; session implementation (swap point 2) +│ │ ├── client.ts # openapi-fetch client; middleware enforces allowlist, use, origin, credential +│ │ └── schema.d.ts # GENERATED from the spec snapshot by `pnpm gen:api`; do not edit │ ├── mcp/ # McpServer factory, tools/, resources, prompts │ └── pages/ # landing, connections -└── test/ # unit/, contract/, oauth/, e2e/, fixtures/ +└── test/ + ├── fixtures/api.json # GENERATED spec snapshot (trimmed); the contract tests' reference + ├── helpers/ # pure check functions so guardrail tests can prove they fire + └── upstream/, oauth/, e2e/, *.test.ts ``` diff --git a/mcp/src/upstream/client.ts b/mcp/src/upstream/client.ts new file mode 100644 index 0000000000..bc696801f8 --- /dev/null +++ b/mcp/src/upstream/client.ts @@ -0,0 +1,97 @@ +import createClient, { type Client, type Middleware } from "openapi-fetch"; + +import { findRoute, findRouteForPathname, isAllowed, type UpstreamUse } from "./allowlist"; +import type { UpstreamCredential } from "./credential"; +import type { paths } from "./schema"; + +/** + * The only way this service talks to the Artcraft API. A typed openapi-fetch client whose + * middleware refuses, before anything leaves the Worker, any request that is + * + * - not on the allowlist for this client's `use` (tools get `read`, sign-in/out gets `auth`), + * - not addressed to the configured upstream origin (per-request `baseUrl` overrides are + * rejected), or + * - whose final URL does not match the allowlisted template (a second, independent check on + * the concrete pathname). + * + * The credential, when present, is attached last so it is never sent on a rejected request. + */ + +const USER_AGENT = "artcraft-mcp/0.0.0"; + +export type UpstreamClient = Client; + +export interface UpstreamClientOptions { + /** Origin of the upstream API, e.g. `https://api.storyteller.ai` (from `Config`). */ + readonly baseUrl: string; + /** Who this client serves; decides which allowlisted routes it may call. */ + readonly use: UpstreamUse; + /** Required for `read` clients (every read route needs a user); optional for `auth`. */ + readonly credential?: UpstreamCredential; + /** Injectable for tests; defaults to the global fetch. */ + readonly fetch?: typeof globalThis.fetch; +} + +export class UpstreamPathNotAllowedError extends Error { + override readonly name = "UpstreamPathNotAllowedError"; + + constructor(method: string, path: string, use: UpstreamUse) { + super(`${method.toUpperCase()} ${path} is not allowlisted for ${use} use`); + } +} + +export class UpstreamOriginError extends Error { + override readonly name = "UpstreamOriginError"; + + constructor(expected: string, actual: string) { + super(`upstream request addressed to ${actual}; only ${expected} is allowed`); + } +} + +export function createUpstreamClient(options: UpstreamClientOptions): UpstreamClient { + if (options.use === "read" && !options.credential) { + throw new Error("a read upstream client requires a credential"); + } + const origin = new URL(options.baseUrl).origin; + + const client = createClient({ + baseUrl: origin, + fetch: options.fetch ?? globalThis.fetch, + headers: { accept: "application/json", "user-agent": USER_AGENT }, + }); + + client.use(allowlistMiddleware(options.use, origin)); + if (options.credential) client.use(credentialMiddleware(options.credential)); + return client; +} + +function allowlistMiddleware(use: UpstreamUse, origin: string): Middleware { + return { + onRequest({ request, schemaPath }) { + const route = findRoute(request.method, schemaPath); + if (!isAllowed(route, use)) { + throw new UpstreamPathNotAllowedError(request.method, schemaPath, use); + } + + const url = new URL(request.url); + if (url.origin !== origin) { + throw new UpstreamOriginError(origin, url.origin); + } + const concreteRoute = findRouteForPathname(request.method, url.pathname); + if (concreteRoute !== route) { + throw new UpstreamPathNotAllowedError(request.method, url.pathname, use); + } + return undefined; + }, + }; +} + +function credentialMiddleware(credential: UpstreamCredential): Middleware { + return { + onRequest({ request }) { + const headers = new Headers(request.headers); + credential.applyTo(headers); + return new Request(request, { headers }); + }, + }; +} diff --git a/mcp/test/upstream/client.test.ts b/mcp/test/upstream/client.test.ts new file mode 100644 index 0000000000..da9fcea986 --- /dev/null +++ b/mcp/test/upstream/client.test.ts @@ -0,0 +1,173 @@ +import { describe, expect, it } from "vitest"; + +import { + createUpstreamClient, + UpstreamOriginError, + UpstreamPathNotAllowedError, +} from "../../src/upstream/client"; +import { createSessionCredential, SESSION_HEADER_NAME } from "../../src/upstream/credential"; + +const BASE_URL = "https://api.example.test"; +const SIGNED_SESSION = "eyJhbGciOiJIUzI1NiJ9.eyJzZXNzaW9uX3Rva2VuIjoic2Vzc2lvbl90ZXN0In0.c2ln"; + +/** A fetch that records every request and answers with a fixed JSON body. */ +function recordingFetch(status = 200, body: unknown = { success: true }) { + const requests: Request[] = []; + const fetchImpl: typeof globalThis.fetch = (input, init) => { + requests.push(new Request(input, init)); + return Promise.resolve( + new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }), + ); + }; + return { requests, fetch: fetchImpl }; +} + +function readClient(fetchImpl: typeof globalThis.fetch) { + return createUpstreamClient({ + baseUrl: BASE_URL, + use: "read", + credential: createSessionCredential(SIGNED_SESSION), + fetch: fetchImpl, + }); +} + +describe("read client", () => { + it("sends an allowlisted request with the session header, JSON accept, and our user agent", async () => { + const { requests, fetch } = recordingFetch(); + const result = await readClient(fetch).GET("/v1/session"); + + expect(result.response.status).toBe(200); + expect(requests).toHaveLength(1); + const sent = requests[0]; + expect(sent?.url).toBe(`${BASE_URL}/v1/session`); + expect(sent?.method).toBe("GET"); + expect(sent?.headers.get(SESSION_HEADER_NAME)).toBe(SIGNED_SESSION); + expect(sent?.headers.get("accept")).toBe("application/json"); + expect(sent?.headers.get("user-agent")).toMatch(/^artcraft-mcp\//); + expect(sent?.headers.get("cookie")).toBeNull(); + expect(sent?.headers.get("authorization")).toBeNull(); + }); + + it("substitutes path parameters and keeps the concrete URL on the template", async () => { + const { requests, fetch } = recordingFetch(); + await readClient(fetch).GET("/v1/jobs/job/{token}", { + params: { path: { token: "jinf_abc123" } }, + }); + expect(requests[0]?.url).toBe(`${BASE_URL}/v1/jobs/job/jinf_abc123`); + }); + + it("serialises array query parameters as repeated keys (what the upstream parses)", async () => { + const { requests, fetch } = recordingFetch(); + await readClient(fetch).GET("/v1/jobs/batch", { + params: { query: { tokens: ["jinf_a", "jinf_b"] } }, + }); + const url = new URL(requests[0]?.url ?? ""); + expect(url.pathname).toBe("/v1/jobs/batch"); + expect(url.searchParams.getAll("tokens")).toEqual(["jinf_a", "jinf_b"]); + }); + + it("passes an upstream 401 through as an error result, not an exception", async () => { + const { fetch } = recordingFetch(401, { success: false, error_code: 401 }); + const result = await readClient(fetch).GET("/v1/credits/namespace/{namespace}", { + params: { path: { namespace: "artcraft" } }, + }); + expect(result.response.status).toBe(401); + expect(result.data).toBeUndefined(); + expect(result.error).toEqual({ success: false, error_code: 401 }); + }); + + it("requires a credential", () => { + expect(() => + createUpstreamClient({ baseUrl: BASE_URL, use: "read", fetch: recordingFetch().fetch }), + ).toThrow(/requires a credential/); + }); +}); + +describe("auth client", () => { + it("may sign in without a credential and sends no session header", async () => { + const { requests, fetch } = recordingFetch(); + const client = createUpstreamClient({ baseUrl: BASE_URL, use: "auth", fetch }); + await client.POST("/v1/login", { + body: { username_or_email: "localdev1", password: "localdev1pass" }, + }); + const sent = requests[0]; + expect(sent?.url).toBe(`${BASE_URL}/v1/login`); + expect(sent?.headers.get(SESSION_HEADER_NAME)).toBeNull(); + expect(sent?.headers.get("content-type")).toMatch(/^application\/json/); + expect(await sent?.json()).toEqual({ + username_or_email: "localdev1", + password: "localdev1pass", + }); + }); + + it("may sign out with a credential", async () => { + const { requests, fetch } = recordingFetch(); + const client = createUpstreamClient({ + baseUrl: BASE_URL, + use: "auth", + credential: createSessionCredential(SIGNED_SESSION), + fetch, + }); + await client.POST("/v1/logout"); + expect(requests[0]?.headers.get(SESSION_HEADER_NAME)).toBe(SIGNED_SESSION); + }); +}); + +describe("the gate fires before anything leaves the Worker", () => { + it("refuses an auth route from a read client, without calling fetch", async () => { + const { requests, fetch } = recordingFetch(); + await expect( + readClient(fetch).POST("/v1/login", { body: { username_or_email: "x", password: "y" } }), + ).rejects.toThrow(UpstreamPathNotAllowedError); + expect(requests).toHaveLength(0); + }); + + it("refuses a read route from an auth client", async () => { + const { requests, fetch } = recordingFetch(); + const client = createUpstreamClient({ baseUrl: BASE_URL, use: "auth", fetch }); + await expect(client.GET("/v1/session")).rejects.toThrow(/not allowlisted for auth use/); + expect(requests).toHaveLength(0); + }); + + it("refuses a path that is not on the allowlist at all", async () => { + const { requests, fetch } = recordingFetch(); + const client = readClient(fetch) as unknown as { GET(path: string): Promise }; + await expect(client.GET("/v1/media_files/list")).rejects.toThrow(UpstreamPathNotAllowedError); + expect(requests).toHaveLength(0); + }); + + it("refuses a per-request baseUrl pointing anywhere else", async () => { + const { requests, fetch } = recordingFetch(); + await expect( + readClient(fetch).GET("/v1/session", { baseUrl: "https://evil.example.test" }), + ).rejects.toThrow(UpstreamOriginError); + expect(requests).toHaveLength(0); + }); + + it("percent-encodes a path parameter so it can never escape its segment", async () => { + const { requests, fetch } = recordingFetch(); + await readClient(fetch).GET("/v1/jobs/job/{token}", { + params: { path: { token: "../../v1/media_files/list" } }, + }); + const pathname = new URL(requests[0]?.url ?? "").pathname; + expect(pathname).toBe("/v1/jobs/job/..%2F..%2Fv1%2Fmedia_files%2Flist"); + expect(pathname).toMatch(/^\/v1\/jobs\/job\/[^/]+$/); + }); + + it("does not include the credential in the rejection message", async () => { + const { fetch } = recordingFetch(); + let rendered = ""; + try { + await readClient(fetch).POST("/v1/login", { + body: { username_or_email: "x", password: "y" }, + }); + } catch (error) { + rendered = error instanceof Error ? `${error.name}: ${error.message}` : "not an Error"; + } + expect(rendered).toMatch(/^UpstreamPathNotAllowedError: /); + expect(rendered).not.toContain(SIGNED_SESSION); + }); +});