diff --git a/src/lib/__tests__/require-auth-bearer.test.ts b/src/lib/__tests__/require-auth-bearer.test.ts index 21af979a5..5faecb60d 100644 --- a/src/lib/__tests__/require-auth-bearer.test.ts +++ b/src/lib/__tests__/require-auth-bearer.test.ts @@ -200,12 +200,17 @@ describe("requireAuth — Bearer token path", () => { expect(prisma.user.findUnique).not.toHaveBeenCalled(); }); - // v1.4.25 W10 reconcile (code-review H2): a route that does NOT declare - // a scope is content with any authenticated token. The previous policy - // 403'd every narrow-scope token here and broke `/api/measurements/by- - // external-ids`, `/api/personal-records`, `/api/medications/[id]/glp1`, - // and `/api/dashboard/glp1` — all four were called without a scope. - it("accepts a narrow-scope Bearer token when the route did not declare a scope", async () => { + // Supersedes the v1.4.25 "a route that declares no scope is content with any + // authenticated token" policy. That policy was adopted to unbreak four + // scopeless routes for narrow-token callers, and in doing so it handed every + // narrow token the full authenticated surface — the escalation this replaces. + // The right fix for a route that genuinely wants a narrow token is for that + // route to name the scope, which is a visible diff. + // + // The fail-closed default. Bare `requireAuth()` means "cookie session or + // cookie-equivalent token" — a narrow token is refused, and the audit row + // says why so an operator can name the token and its owner. + it("refuses a narrow-scope Bearer token when the route declared no scope", async () => { vi.mocked(getSession).mockResolvedValue(null); setBearerHeader(`Bearer ${RAW_TOKEN}`); @@ -218,14 +223,40 @@ describe("requireAuth — Bearer token path", () => { } as never); vi.mocked(prisma.user.findUnique).mockResolvedValue(FAKE_USER as never); - const ctx = await requireAuth(); - expect(ctx.user.id).toBe("user-1"); - expect(ctx.session.id).toBe("token-5a"); - // v1.25 — no per-request success audit row on the accept path. - expect(auditLog).not.toHaveBeenCalledWith( - "auth.bearer.success", - expect.anything(), + await expect(requireAuth()).rejects.toMatchObject({ + statusCode: 403, + message: "Insufficient permissions", + } satisfies Partial); + expect(auditLog).toHaveBeenCalledWith( + "auth.bearer.failure", + expect.objectContaining({ + userId: "user-1", + details: expect.objectContaining({ + reason: "undeclared_scope", + tokenId: "token-5a", + }), + }), ); + // The user row is never loaded — the deny happens on the token alone. + expect(prisma.user.findUnique).not.toHaveBeenCalled(); + }); + + it("admits a narrow-scope Bearer token on a route that names its scope", async () => { + vi.mocked(getSession).mockResolvedValue(null); + setBearerHeader(`Bearer ${RAW_TOKEN}`); + + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "token-5a2", + userId: "user-1", + permissions: ["fhir:read"], + revoked: false, + expiresAt: null, + } as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue(FAKE_USER as never); + + const ctx = await requireAuth("fhir:read"); + expect(ctx.user.id).toBe("user-1"); + expect(ctx.session.id).toBe("token-5a2"); }); it("accepts a wildcard Bearer token when the route did not declare a scope", async () => { @@ -273,10 +304,18 @@ describe("requireAuth — Bearer token path", () => { }); }); -describe("requireAuth — H1 MCP health:read audience binding on REST", () => { - // Drive the FULL apiHandler path so the request method is in event context — - // the H1 check reads it to allow safe reads but block writes. - const route = apiHandler(async (req: Request) => { +describe("requireAuth — MCP-audience tokens have no REST reach", () => { + // Before the fail-closed default, an MCP-audience token (`health:read`, or + // `health:read health:write`) was admitted on safe REST methods and refused + // on writes by the `isMcpAudienceToken` guard. It now carries no `*` grant + // and no REST route names `health:read`, so the resolver refuses it on EVERY + // method — reads included. Its audience narrows from "/mcp + REST reads" to + // "/mcp only", which is what RFC 8707 audience binding asked for. + // + // The `isMcpAudienceToken` guard is retained in `api-handler.ts` as defence + // in depth; it is unreachable on this path, which is exactly why these tests + // assert `undeclared_scope` rather than `mcp_audience_write_blocked`. + const route = apiHandler(async (req) => { await requireAuth(); return Response.json({ ok: true, method: req.method }); }); @@ -295,41 +334,36 @@ describe("requireAuth — H1 MCP health:read audience binding on REST", () => { vi.mocked(prisma.user.findUnique).mockResolvedValue(FAKE_USER as never); }); - function mockHealthReadToken(): void { + function mockMcpToken(permissions: string[]): void { vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ id: "mcp-token", userId: "user-1", - permissions: ["health:read"], + permissions, revoked: false, expiresAt: null, } as never); } - it("accepts a health:read token on a GET read route", async () => { - mockHealthReadToken(); - const res = await call("GET"); - expect(res.status).toBe(200); - }); + for (const permissions of [ + ["health:read"], + ["health:read", "health:write"], + ]) { + const label = permissions.join(" "); - it("rejects a health:read token on POST with 403", async () => { - mockHealthReadToken(); - const res = await call("POST"); - expect(res.status).toBe(403); - expect(auditLog).toHaveBeenCalledWith( - "auth.bearer.failure", - expect.objectContaining({ - details: expect.objectContaining({ - reason: "mcp_audience_write_blocked", - }), - }), - ); - }); - - it("rejects a health:read token on DELETE with 403", async () => { - mockHealthReadToken(); - const res = await call("DELETE"); - expect(res.status).toBe(403); - }); + for (const method of ["GET", "POST", "DELETE"]) { + it(`refuses a ${label} token on REST ${method} with 403`, async () => { + mockMcpToken(permissions); + const res = await call(method); + expect(res.status).toBe(403); + expect(auditLog).toHaveBeenCalledWith( + "auth.bearer.failure", + expect.objectContaining({ + details: expect.objectContaining({ reason: "undeclared_scope" }), + }), + ); + }); + } + } it("does NOT restrict a wildcard token on POST (not MCP-audience)", async () => { vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ @@ -342,41 +376,6 @@ describe("requireAuth — H1 MCP health:read audience binding on REST", () => { const res = await call("POST"); expect(res.status).toBe(200); }); - - it("rejects a health:read+write token on POST with 403 (writes are /mcp-only)", async () => { - // A write-scoped MCP token is STILL audience-bound: its `health:write` - // grant admits the confirmed in-process `/mcp` write tools, never a REST - // write/delete. So it gets the same 403 as a read-only MCP token here. - vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ - id: "rw", - userId: "user-1", - permissions: ["health:read", "health:write"], - revoked: false, - expiresAt: null, - } as never); - const res = await call("POST"); - expect(res.status).toBe(403); - expect(auditLog).toHaveBeenCalledWith( - "auth.bearer.failure", - expect.objectContaining({ - details: expect.objectContaining({ - reason: "mcp_audience_write_blocked", - }), - }), - ); - }); - - it("accepts a health:read+write token on a GET read route", async () => { - vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ - id: "rw", - userId: "user-1", - permissions: ["health:read", "health:write"], - revoked: false, - expiresAt: null, - } as never); - const res = await call("GET"); - expect(res.status).toBe(200); - }); }); describe("requireAuth — cookie path remains intact", () => { diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index 3de3383e9..850c0dca3 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -8,7 +8,11 @@ import { emitIfSampled } from "./logging/transports"; import { redactOptional, redactSecrets } from "./logging/redact"; import { getSession } from "./auth/session"; import { auditLog } from "./auth/audit"; -import { resolveBearerToken, BearerAuthError } from "./auth/bearer"; +import { + resolveBearerToken, + BearerAuthError, + type ScopeRequirement, +} from "./auth/bearer"; import { AssistantDisabledError } from "./feature-flags"; import { ConsentRequiredError } from "./ai/consent-guard"; import { SCOPE_HEALTH_READ, SCOPE_HEALTH_WRITE } from "./mcp/oauth/config"; @@ -295,8 +299,12 @@ export type AuthContext = { * 3. Neither → 401. * * @param requiredPermission Optional permission scope. Only enforced for Bearer - * auth — cookie sessions always pass (full user access). When set and missing - * from `ApiToken.permissions`, throws HttpError(403). + * auth — cookie sessions always pass (full user access). Omitting it is a + * positive declaration: the route accepts cookie sessions and cookie- + * equivalent (`["*"]`) tokens ONLY, and refuses a narrow-scope token with + * HttpError(403). Naming a scope additionally admits narrow tokens that list + * it. A token that lists neither `*` nor the named scope throws + * HttpError(403). */ export async function requireAuth( requiredPermission?: string, @@ -346,17 +354,24 @@ export async function requireAuth( * annotation, then maps the result onto the `AuthContext` contract * (`session.id` carries the token id). * - * Authorisation contract (unchanged): a route that declares no - * `requiredPermission` accepts any valid token; one that declares a scope - * accepts wildcard (`["*"]`) tokens and narrow-scope tokens that list it. + * Authorisation contract (fail-closed): a route that declares no + * `requiredPermission` accepts cookie sessions and cookie-equivalent (`["*"]`) + * tokens only — a narrow-scope token is refused 403 with an `undeclared_scope` + * audit row. A route that declares a scope additionally accepts narrow tokens + * that list it. The absence of an argument is a positive statement, not an + * omission, which is what makes the default safe for routes nobody has thought + * about yet. */ async function authenticateBearer( rawToken: string, requiredPermission: string | undefined, ): Promise { + const requirement: ScopeRequirement = requiredPermission + ? { kind: "scope", scope: requiredPermission } + : { kind: "wildcard-only" }; let resolution; try { - resolution = await resolveBearerToken(rawToken, requiredPermission); + resolution = await resolveBearerToken(rawToken, requirement); } catch (err) { if (err instanceof BearerAuthError) { auditLog("auth.bearer.failure", { @@ -393,6 +408,13 @@ async function authenticateBearer( // which always sets the method — an unknown method means we cannot prove a // read, so we deny. This is RFC 8707 audience binding on the credential the // client actually holds, not only during the OAuth exchange. + // + // Since the fail-closed scope default landed, this guard is unreachable on + // the deny path: an MCP-audience token carries no `*`, so `wildcard-only` + // already refused it in `resolveBearerToken` before we get here (an MCP + // token's REST reach is now nil, not "safe methods"). It is kept as defence + // in depth — it still holds the line if a future REST route ever declares + // `health:read` and so admits the token past the resolver. if (isMcpAudienceToken(permissions)) { const method = (getEvent()?.getHttpMethod() ?? "").toUpperCase(); if (!READ_HTTP_METHODS.has(method)) { diff --git a/src/lib/auth/__tests__/bearer.test.ts b/src/lib/auth/__tests__/bearer.test.ts index f3dc89159..734c029e0 100644 --- a/src/lib/auth/__tests__/bearer.test.ts +++ b/src/lib/auth/__tests__/bearer.test.ts @@ -9,6 +9,7 @@ vi.mock("@/lib/db", () => ({ vi.mock("@/lib/auth/hmac", () => ({ hashToken: vi.fn() })); import { resolveBearerToken, BearerAuthError } from "../bearer"; +import type { ScopeRequirement } from "../bearer"; import { prisma } from "@/lib/db"; import { hashToken } from "@/lib/auth/hmac"; @@ -16,6 +17,12 @@ const FAKE_HASH = "deadbeefcafef00d"; const RAW_TOKEN = "hlk_" + "a".repeat(64); const FAKE_USER = { id: "user-1", role: "USER" }; +/** The REST default: cookie sessions and `["*"]` tokens only. */ +const WILDCARD_ONLY: ScopeRequirement = { kind: "wildcard-only" }; +/** The `/mcp` posture: authenticate here, authorise downstream. */ +const ANY_VALID: ScopeRequirement = { kind: "any-valid-token" }; +const scope = (s: string): ScopeRequirement => ({ kind: "scope", scope: s }); + beforeEach(() => { vi.resetAllMocks(); vi.mocked(hashToken).mockReturnValue(FAKE_HASH); @@ -33,7 +40,7 @@ describe("resolveBearerToken", () => { } as never); vi.mocked(prisma.user.findUnique).mockResolvedValue(FAKE_USER as never); - const res = await resolveBearerToken(RAW_TOKEN); + const res = await resolveBearerToken(RAW_TOKEN, WILDCARD_ONLY); expect(hashToken).toHaveBeenCalledWith(RAW_TOKEN); expect(prisma.apiToken.findUnique).toHaveBeenCalledWith( @@ -50,7 +57,9 @@ describe("resolveBearerToken", () => { it("rejects an unknown token (401)", async () => { vi.mocked(prisma.apiToken.findUnique).mockResolvedValue(null as never); - await expect(resolveBearerToken(RAW_TOKEN)).rejects.toMatchObject({ + await expect( + resolveBearerToken(RAW_TOKEN, WILDCARD_ONLY), + ).rejects.toMatchObject({ statusCode: 401, reason: "unknown_token", } satisfies Partial); @@ -65,7 +74,9 @@ describe("resolveBearerToken", () => { revoked: true, expiresAt: null, } as never); - await expect(resolveBearerToken(RAW_TOKEN)).rejects.toMatchObject({ + await expect( + resolveBearerToken(RAW_TOKEN, WILDCARD_ONLY), + ).rejects.toMatchObject({ statusCode: 401, reason: "revoked", }); @@ -79,7 +90,9 @@ describe("resolveBearerToken", () => { revoked: false, expiresAt: new Date(Date.now() - 60_000), } as never); - await expect(resolveBearerToken(RAW_TOKEN)).rejects.toMatchObject({ + await expect( + resolveBearerToken(RAW_TOKEN, WILDCARD_ONLY), + ).rejects.toMatchObject({ statusCode: 401, reason: "expired", }); @@ -94,7 +107,7 @@ describe("resolveBearerToken", () => { expiresAt: null, } as never); await expect( - resolveBearerToken(RAW_TOKEN, "health:write"), + resolveBearerToken(RAW_TOKEN, scope("health:write")), ).rejects.toMatchObject({ statusCode: 403, reason: "insufficient_permissions", @@ -102,7 +115,11 @@ describe("resolveBearerToken", () => { expect(prisma.user.findUnique).not.toHaveBeenCalled(); }); - it("admits any valid token when no permission is required (REQ-SEC-5)", async () => { + // The inversion. A narrow token used to be admitted by every route that + // named no scope — 324 of them — which is how a token minted for medication + // intake reached the full-backup export. `wildcard-only` is now the default + // arm, so the safe outcome is the one a route gets by doing nothing. + it("refuses a narrow-scope token under the wildcard-only default (403)", async () => { vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ id: "token-5", userId: "user-1", @@ -110,8 +127,51 @@ describe("resolveBearerToken", () => { revoked: false, expiresAt: null, } as never); + await expect( + resolveBearerToken(RAW_TOKEN, WILDCARD_ONLY), + ).rejects.toMatchObject({ + statusCode: 403, + reason: "undeclared_scope", + }); + expect(prisma.user.findUnique).not.toHaveBeenCalled(); + }); + + it("admits a narrow-scope token for the scope it lists", async () => { + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "token-5b", + userId: "user-1", + permissions: ["fhir:read"], + revoked: false, + expiresAt: null, + } as never); vi.mocked(prisma.user.findUnique).mockResolvedValue(FAKE_USER as never); - const res = await resolveBearerToken(RAW_TOKEN); + const res = await resolveBearerToken(RAW_TOKEN, scope("fhir:read")); + expect(res.user.id).toBe("user-1"); + }); + + it("admits any valid token under the any-valid-token posture", async () => { + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "token-5c", + userId: "user-1", + permissions: ["health:read"], + revoked: false, + expiresAt: null, + } as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue(FAKE_USER as never); + const res = await resolveBearerToken(RAW_TOKEN, ANY_VALID); + expect(res.user.id).toBe("user-1"); + }); + + it("admits a wildcard token under the wildcard-only default", async () => { + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "token-5d", + userId: "user-1", + permissions: ["*"], + revoked: false, + expiresAt: null, + } as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue(FAKE_USER as never); + const res = await resolveBearerToken(RAW_TOKEN, WILDCARD_ONLY); expect(res.user.id).toBe("user-1"); }); @@ -124,7 +184,7 @@ describe("resolveBearerToken", () => { expiresAt: null, } as never); vi.mocked(prisma.user.findUnique).mockResolvedValue(FAKE_USER as never); - const res = await resolveBearerToken(RAW_TOKEN, "health:write"); + const res = await resolveBearerToken(RAW_TOKEN, scope("health:write")); expect(res.user.id).toBe("user-1"); }); }); diff --git a/src/lib/auth/bearer.ts b/src/lib/auth/bearer.ts index 694f27f14..8abb7997f 100644 --- a/src/lib/auth/bearer.ts +++ b/src/lib/auth/bearer.ts @@ -24,8 +24,29 @@ export type BearerAuthReason = | "revoked" | "expired" | "insufficient_permissions" + | "undeclared_scope" | "user_missing"; +/** + * What a caller demands of a token's scope set. Required on every + * `resolveBearerToken` call — there is deliberately no default, so a new wire + * cannot re-open the fail-open hole by simply omitting the argument. + * + * - `scope` — the route names a grant; a non-wildcard token must list it. + * - `wildcard-only` — the REST default. A route that declares no scope accepts + * cookie sessions and cookie-equivalent (`["*"]`) tokens only. A narrow token + * is refused, so adding a route can never silently widen an existing token's + * reach. + * - `any-valid-token` — authentication only; authorisation is decided by the + * caller's own wire. This is the single deliberate fail-open posture in the + * tree (the `/mcp` transport, which gates on audience + `tokenAllowsWrite` + * downstream) and a structural test freezes it to exactly one call site. + */ +export type ScopeRequirement = + | { kind: "scope"; scope: string } + | { kind: "wildcard-only" } + | { kind: "any-valid-token" }; + /** * Thrown when a token fails validation. Carries the HTTP status the HTTP edge * should surface plus the machine `reason` (and, when known, the owning @@ -59,15 +80,15 @@ export interface BearerResolution { * Resolve a raw `hlk_` token to its user, or throw `BearerAuthError`. * * Lookups are by HMAC hash only — the plaintext token is never compared against - * a stored value. When `requiredPermission` is set, a non-wildcard token must - * list it; an unset `requiredPermission` admits any valid token (matching the - * "route declares no scope ⇒ authentication alone is sufficient" contract). + * a stored value. Authorisation is fail-closed: `requirement` is mandatory and + * a token carrying no `*` grant is admitted only when the caller names a scope + * the token lists, or explicitly opts into `any-valid-token`. * * `lastUsedAt` is refreshed fire-and-forget on success. */ export async function resolveBearerToken( rawToken: string, - requiredPermission?: string, + requirement: ScopeRequirement, ): Promise { const tokenHashValue = hashToken(rawToken); @@ -94,21 +115,34 @@ export async function resolveBearerToken( throw new BearerAuthError(401, "expired", apiToken.userId, apiToken.id); } - // `["*"]` is the wildcard scope. A route that declares no `requiredPermission` - // accepts any valid token; one that declares a scope accepts wildcard tokens - // and narrow-scope tokens that list that scope. - const hasWildcardPermission = apiToken.permissions.includes("*"); - if ( - requiredPermission && - !hasWildcardPermission && - !apiToken.permissions.includes(requiredPermission) - ) { - throw new BearerAuthError( - 403, - "insufficient_permissions", - apiToken.userId, - apiToken.id, - ); + // `["*"]` is the wildcard scope — cookie-equivalent, and the only shape the + // login / passkey / refresh paths mint. It clears every requirement. + // + // A narrow token has to be named. `wildcard-only` (the REST default, reached + // by every route that passes no scope) refuses it outright; `scope` admits it + // only when the token lists exactly that grant. The deny arm is what a route + // gets by doing nothing, so a newly added route cannot widen an existing + // token's reach without a visible diff. + if (!apiToken.permissions.includes("*")) { + if (requirement.kind === "wildcard-only") { + throw new BearerAuthError( + 403, + "undeclared_scope", + apiToken.userId, + apiToken.id, + ); + } + if ( + requirement.kind === "scope" && + !apiToken.permissions.includes(requirement.scope) + ) { + throw new BearerAuthError( + 403, + "insufficient_permissions", + apiToken.userId, + apiToken.id, + ); + } } const user = await prisma.user.findUnique({ diff --git a/src/lib/mcp/__tests__/auth.test.ts b/src/lib/mcp/__tests__/auth.test.ts index c73861031..a1164f24f 100644 --- a/src/lib/mcp/__tests__/auth.test.ts +++ b/src/lib/mcp/__tests__/auth.test.ts @@ -25,7 +25,13 @@ describe("resolveMcpAuthContext", () => { const ctx = await resolveMcpAuthContext(" hlk_abc "); // Trimmed token resolved through the shared validator (no requiredPermission for reads). - expect(resolveBearerToken).toHaveBeenCalledWith("hlk_abc"); + // `any-valid-token` is the one deliberate fail-open posture in the tree: + // `/mcp` authenticates here and authorises downstream (audience binding + + // `tokenAllowsWrite`). Asserted explicitly so a silent switch to another + // posture — in either direction — shows up as a failing test. + expect(resolveBearerToken).toHaveBeenCalledWith("hlk_abc", { + kind: "any-valid-token", + }); expect(ctx.userId).toBe("user-1"); expect(ctx.tokenId).toBe("token-9"); expect(ctx.binding).toBe("user-1:token-9"); diff --git a/src/lib/mcp/auth.ts b/src/lib/mcp/auth.ts index 91eccc9c2..7ed23f852 100644 --- a/src/lib/mcp/auth.ts +++ b/src/lib/mcp/auth.ts @@ -36,8 +36,13 @@ export interface McpAuthContext { /** * Resolve a raw Bearer token into an `McpAuthContext`, or throw on an invalid / - * revoked / expired token. Reads require no special scope (the underlying reads - * are unscoped), so no `requiredPermission` is passed. + * revoked / expired token. + * + * This is the ONE deliberate `any-valid-token` posture in the tree: the `/mcp` + * transport authenticates here and authorises downstream — audience binding + * plus `tokenAllowsWrite` gate the write tools, so a scope check at this line + * would be the wrong layer. A structural test asserts this call site stays the + * only one, because every other wire must fail closed. */ export async function resolveMcpAuthContext( rawToken: string, @@ -47,7 +52,9 @@ export async function resolveMcpAuthContext( throw new Error("Missing MCP Bearer token"); } - const { user, tokenId, permissions } = await resolveBearerToken(trimmed); + const { user, tokenId, permissions } = await resolveBearerToken(trimmed, { + kind: "any-valid-token", + }); return { userId: user.id,