From 6ee7402adeb79d7ad113cfbf5e94f5c77b887499 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Mon, 15 Jun 2026 00:20:51 -0700 Subject: [PATCH 1/6] Add OAuth observability, config plumbing, and trusted-IP resolver Introduce a shared OAuth observability log-wrapper (src/oauth/observability.ts) and a setter-injected trusted-client-IP resolver (src/oauth/trusted-client-ip.ts) with an assertion that fails fast if the resolver is not wired before serving requests. Extend src/config.ts with oauthConsentHmacKeys (parsed list with rotation support) and update cross-cutting test mocks for the new Config field. --- src/__tests__/analytics-endpoints.test.ts | 6 + src/__tests__/analytics-server.test.ts | 6 + .../atlas-ratification-endpoints.test.ts | 2 + src/__tests__/atlas-search-endpoint.test.ts | 2 + src/__tests__/config.test.ts | 128 ++++++++++++++++++ src/__tests__/oauth-trusted-client-ip.test.ts | 125 +++++++++++++++++ src/config.ts | 57 ++++++++ src/oauth/observability.ts | 86 ++++++++++++ src/oauth/trusted-client-ip.ts | 89 ++++++++++++ 9 files changed, 501 insertions(+) create mode 100644 src/__tests__/oauth-trusted-client-ip.test.ts create mode 100644 src/oauth/observability.ts create mode 100644 src/oauth/trusted-client-ip.ts diff --git a/src/__tests__/analytics-endpoints.test.ts b/src/__tests__/analytics-endpoints.test.ts index 7cd9b4a..52eeb3c 100644 --- a/src/__tests__/analytics-endpoints.test.ts +++ b/src/__tests__/analytics-endpoints.test.ts @@ -45,6 +45,7 @@ vi.mock("../config.js", () => ({ discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -83,6 +84,7 @@ const DEFAULT_TEST_CONFIG = { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -387,6 +389,7 @@ describe("analyticsAuth middleware", () => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -428,6 +431,7 @@ describe("analyticsAuth middleware", () => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -471,6 +475,7 @@ describe("analyticsAuth middleware", () => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -516,6 +521,7 @@ describe("analyticsAuth middleware", () => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", diff --git a/src/__tests__/analytics-server.test.ts b/src/__tests__/analytics-server.test.ts index 91aea23..7bd239e 100644 --- a/src/__tests__/analytics-server.test.ts +++ b/src/__tests__/analytics-server.test.ts @@ -33,6 +33,7 @@ vi.mock("../config.js", () => ({ discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -953,6 +954,7 @@ describe("Analytics server routes (HTTP-level)", () => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -994,6 +996,7 @@ describe("Analytics server routes (HTTP-level)", () => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -1021,6 +1024,7 @@ describe("Analytics server routes (HTTP-level)", () => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -1061,6 +1065,7 @@ describe("Analytics server routes (HTTP-level)", () => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -1115,6 +1120,7 @@ describe("Analytics server routes (HTTP-level)", () => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", diff --git a/src/__tests__/atlas-ratification-endpoints.test.ts b/src/__tests__/atlas-ratification-endpoints.test.ts index f3a2190..619d864 100644 --- a/src/__tests__/atlas-ratification-endpoints.test.ts +++ b/src/__tests__/atlas-ratification-endpoints.test.ts @@ -40,6 +40,7 @@ vi.mock("../config.js", async (importOriginal) => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -73,6 +74,7 @@ const DEFAULT_TEST_CONFIG = { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", diff --git a/src/__tests__/atlas-search-endpoint.test.ts b/src/__tests__/atlas-search-endpoint.test.ts index 7895f05..d65457e 100644 --- a/src/__tests__/atlas-search-endpoint.test.ts +++ b/src/__tests__/atlas-search-endpoint.test.ts @@ -73,6 +73,7 @@ vi.mock("../config.js", async (importOriginal) => { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", @@ -104,6 +105,7 @@ const DEFAULT_TEST_CONFIG = { discordPublicKey: "", notionToken: "", mcpJwtSecret: "x".repeat(32), + oauthConsentHmacKeys: ["a".repeat(64)], p2pTelemetryUrl: undefined, p2pTelemetryDisabled: false, packageVersion: "test", diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index 1233bc7..a7ed946 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -1136,6 +1136,7 @@ describe("config.ts", () => { process.env.GITHUB_TOKEN = "ghp_test"; process.env.GITHUB_WEBHOOK_SECRET = "secret"; process.env.MCP_JWT_SECRET = "x".repeat(64); + process.env.PATHFINDER_CONSENT_HMAC_KEY = "a".repeat(64); mockedExistsSync.mockReturnValue(true); mockedReadFileSync.mockReturnValue(makeYaml()); @@ -1465,6 +1466,133 @@ describe("config.ts", () => { }); }); + // ── oauthConsentHmacKeys ───────────────────────────────────────────────── + + describe("oauthConsentHmacKeys", () => { + beforeEach(() => { + process.env.PATHFINDER_CONFIG = "/tmp/test.yaml"; + process.env.DATABASE_URL = "postgresql://test"; + process.env.OPENAI_API_KEY = "sk-test"; + }); + + it("parses a single PATHFINDER_CONSENT_HMAC_KEY", async () => { + const k = "a".repeat(64); + process.env.PATHFINDER_CONSENT_HMAC_KEY = k; + mockedExistsSync.mockReturnValue(true); + mockedReadFileSync.mockReturnValue(makeYaml()); + + const { getConfig } = await freshImport(); + const cfg = getConfig(); + expect(cfg.oauthConsentHmacKeys).toEqual([k]); + }); + + it("parses comma-separated keys for rotation", async () => { + const k1 = "a".repeat(64); + const k2 = "b".repeat(48); + process.env.PATHFINDER_CONSENT_HMAC_KEY = `${k1},${k2}`; + mockedExistsSync.mockReturnValue(true); + mockedReadFileSync.mockReturnValue(makeYaml()); + + const { getConfig } = await freshImport(); + const cfg = getConfig(); + expect(cfg.oauthConsentHmacKeys).toEqual([k1, k2]); + }); + + it("trims whitespace around comma-separated entries", async () => { + const k1 = "a".repeat(64); + const k2 = "b".repeat(64); + process.env.PATHFINDER_CONSENT_HMAC_KEY = ` ${k1} , ${k2} `; + mockedExistsSync.mockReturnValue(true); + mockedReadFileSync.mockReturnValue(makeYaml()); + + const { getConfig } = await freshImport(); + const cfg = getConfig(); + expect(cfg.oauthConsentHmacKeys).toEqual([k1, k2]); + }); + + it("rejects keys shorter than 32 hex chars", async () => { + process.env.PATHFINDER_CONSENT_HMAC_KEY = "abcdef"; // way too short + mockedExistsSync.mockReturnValue(true); + mockedReadFileSync.mockReturnValue(makeYaml()); + + const { getConfig } = await freshImport(); + expect(() => getConfig()).toThrow( + /PATHFINDER_CONSENT_HMAC_KEY entries must be ≥32 hex chars/, + ); + }); + + it("rejects non-hex characters in keys", async () => { + // 64 chars but contains 'g' (not hex) + process.env.PATHFINDER_CONSENT_HMAC_KEY = "g".repeat(64); + mockedExistsSync.mockReturnValue(true); + mockedReadFileSync.mockReturnValue(makeYaml()); + + const { getConfig } = await freshImport(); + expect(() => getConfig()).toThrow( + /PATHFINDER_CONSENT_HMAC_KEY entries must be ≥32 hex chars/, + ); + }); + + it("rejects when any key in a rotation list is invalid", async () => { + const good = "a".repeat(64); + const bad = "short"; + process.env.PATHFINDER_CONSENT_HMAC_KEY = `${good},${bad}`; + mockedExistsSync.mockReturnValue(true); + mockedReadFileSync.mockReturnValue(makeYaml()); + + const { getConfig } = await freshImport(); + expect(() => getConfig()).toThrow( + /PATHFINDER_CONSENT_HMAC_KEY entries must be ≥32 hex chars/, + ); + }); + + it("throws in production when PATHFINDER_CONSENT_HMAC_KEY is unset", async () => { + process.env.NODE_ENV = "production"; + process.env.MCP_JWT_SECRET = "x".repeat(64); + delete process.env.PATHFINDER_CONSENT_HMAC_KEY; + mockedExistsSync.mockReturnValue(true); + mockedReadFileSync.mockReturnValue(makeYaml()); + + const { getConfig } = await freshImport(); + expect(() => getConfig()).toThrow( + /PATHFINDER_CONSENT_HMAC_KEY is required in production/, + ); + }); + + it("generates an ephemeral key in development with a WARN log", async () => { + process.env.NODE_ENV = "development"; + delete process.env.PATHFINDER_CONSENT_HMAC_KEY; + mockedExistsSync.mockReturnValue(true); + mockedReadFileSync.mockReturnValue(makeYaml()); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const { getConfig } = await freshImport(); + const cfg = getConfig(); + expect(cfg.oauthConsentHmacKeys).toHaveLength(1); + expect(cfg.oauthConsentHmacKeys[0]).toMatch(/^[0-9a-f]{64}$/); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringMatching( + /\[oauth\] PATHFINDER_CONSENT_HMAC_KEY not set — generated an ephemeral consent-nonce key for development\./, + ), + ); + warnSpy.mockRestore(); + }); + + it("treats whitespace-only PATHFINDER_CONSENT_HMAC_KEY as unset (dev fallback)", async () => { + process.env.NODE_ENV = "development"; + process.env.PATHFINDER_CONSENT_HMAC_KEY = " "; + mockedExistsSync.mockReturnValue(true); + mockedReadFileSync.mockReturnValue(makeYaml()); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const { getConfig } = await freshImport(); + const cfg = getConfig(); + expect(cfg.oauthConsentHmacKeys).toHaveLength(1); + expect(cfg.oauthConsentHmacKeys[0]).toMatch(/^[0-9a-f]{64}$/); + warnSpy.mockRestore(); + }); + }); + // ── Config caching ─────────────────────────────────────────────────────── describe("caching", () => { diff --git a/src/__tests__/oauth-trusted-client-ip.test.ts b/src/__tests__/oauth-trusted-client-ip.test.ts new file mode 100644 index 0000000..0cc5aef --- /dev/null +++ b/src/__tests__/oauth-trusted-client-ip.test.ts @@ -0,0 +1,125 @@ +import { describe, it, expect, afterEach, vi } from "vitest"; +import { + oauthClientIp, + setTrustingProxy, + oauthIpResolverInjected, + assertOauthIpResolverInjected, +} from "../oauth/trusted-client-ip.js"; + +// Reset the injected accessor between tests so a stray setter from one case +// can never leak into another. The module-level default is `() => false`, the +// fail-safe pre-bootstrap behavior. +afterEach(() => setTrustingProxy(() => false)); + +function fakeReq( + over: Partial<{ ip: string; xff: string; socket: string }> = {}, +) { + return { + ip: over.ip, + headers: over.xff ? { "x-forwarded-for": over.xff } : {}, + socket: { remoteAddress: over.socket ?? "203.0.113.5" }, + } as unknown as import("express").Request; +} + +describe("oauthClientIp", () => { + it("ignores X-Forwarded-For when trustProxy=false (default)", () => { + setTrustingProxy(() => false); + const req = fakeReq({ + ip: "1.2.3.4", + xff: "1.2.3.4", + socket: "203.0.113.5", + }); + expect(oauthClientIp(req)).toBe("203.0.113.5"); + }); + + it("honors req.ip (which Express derives from XFF) when trustProxy=true", () => { + setTrustingProxy(() => true); + const req = fakeReq({ + ip: "1.2.3.4", + xff: "1.2.3.4", + socket: "203.0.113.5", + }); + expect(oauthClientIp(req)).toBe("1.2.3.4"); + }); + + it("falls back to socket when req.ip is empty and trustProxy=false", () => { + setTrustingProxy(() => false); + const req = fakeReq({ + ip: "", + xff: "1.2.3.4", + socket: "203.0.113.99", + }); + expect(oauthClientIp(req)).toBe("203.0.113.99"); + }); + + it("returns 'unknown' when everything is empty", () => { + setTrustingProxy(() => false); + expect(oauthClientIp({ headers: {}, socket: {} } as never)).toBe( + "unknown", + ); + }); + + it("defaults to fail-safe (trustProxy=false) when setTrustingProxy has not been called", async () => { + // Re-import the module fresh so the accessor is at its initial default. + // We can't actually re-import in-place from this test (ESM cache), but + // we can simulate the pre-bootstrap state by explicitly resetting to + // the fail-safe accessor and asserting XFF is ignored. + setTrustingProxy(() => false); + const req = fakeReq({ + ip: "1.2.3.4", + xff: "1.2.3.4", + socket: "203.0.113.5", + }); + // Spoof attempt via XFF must NOT bypass the socket address. + expect(oauthClientIp(req)).toBe("203.0.113.5"); + }); +}); + +// ────────────────────────────────────────────────────────────────────── +// Injection guard — oauthIpResolverInjected / assertOauthIpResolverInjected +// ────────────────────────────────────────────────────────────────────── +// +// Bootstrap regression guard. The fail-safe `() => false` default is safe +// for tests, but in production we want a deploy that loses the +// `setTrustingProxy(...)` call to crash loudly instead of silently +// degrading. These tests exercise both the diagnostic boolean and the +// throwing assertion via a `vi.resetModules()` fresh import so the +// pre-injection state is observable (the suite above has already injected +// in this process's module cache). + +describe("oauthIpResolverInjected / assertOauthIpResolverInjected", () => { + afterEach(() => { + vi.resetModules(); + }); + + it("oauthIpResolverInjected returns false before setTrustingProxy and true after", async () => { + vi.resetModules(); + const fresh = (await import("../oauth/trusted-client-ip.js")) as { + setTrustingProxy: typeof setTrustingProxy; + oauthIpResolverInjected: typeof oauthIpResolverInjected; + }; + expect(fresh.oauthIpResolverInjected()).toBe(false); + fresh.setTrustingProxy(() => false); + expect(fresh.oauthIpResolverInjected()).toBe(true); + }); + + it("assertOauthIpResolverInjected throws when not injected", async () => { + vi.resetModules(); + const fresh = (await import("../oauth/trusted-client-ip.js")) as { + assertOauthIpResolverInjected: typeof assertOauthIpResolverInjected; + }; + expect(() => fresh.assertOauthIpResolverInjected()).toThrow( + /oauth trusted-IP resolver was not wired/i, + ); + }); + + it("assertOauthIpResolverInjected does NOT throw after setTrustingProxy", async () => { + vi.resetModules(); + const fresh = (await import("../oauth/trusted-client-ip.js")) as { + setTrustingProxy: typeof setTrustingProxy; + assertOauthIpResolverInjected: typeof assertOauthIpResolverInjected; + }; + fresh.setTrustingProxy(() => false); + expect(() => fresh.assertOauthIpResolverInjected()).not.toThrow(); + }); +}); diff --git a/src/config.ts b/src/config.ts index 01c14b6..e3953ad 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,6 +1,7 @@ // Centralized configuration: env-var secrets + YAML server config. import "dotenv/config"; +import { randomBytes } from "node:crypto"; import { readFileSync, existsSync } from "node:fs"; import { resolve, dirname } from "node:path"; import { fileURLToPath } from "node:url"; @@ -53,6 +54,16 @@ export interface Config { discordPublicKey: string; notionToken: string; mcpJwtSecret: string; + /** + * HMAC keys (≥32 hex chars each) used to sign and verify the OAuth + * consent-screen nonce. Parsed from comma-separated env + * PATHFINDER_CONSENT_HMAC_KEY. The first key signs new nonces; all keys + * are accepted on verify so a value can be rotated without invalidating + * in-flight consent screens. Required in production (fail-loud on unset + * or short keys); in non-production an ephemeral 64-hex key is generated + * at startup with a WARN log so dev/test runs work out of the box. + */ + oauthConsentHmacKeys: string[]; /** * URL of the CopilotKit-hosted telemetry-sink Lambda. Hosted-only — when * unset (the default for OSS deployments), the p2p-telemetry client @@ -123,6 +134,50 @@ export function getIndexableSourceNames(): Set { let cachedConfig: Config | null = null; +/** + * Parse PATHFINDER_CONSENT_HMAC_KEY into a list of HMAC keys for the OAuth + * consent-nonce signer/verifier. Mirrors `resolveJwtSecret` policy: + * - present + all entries ≥32 hex chars → use as-is (comma-separated for rotation) + * - present + any entry invalid → throw (fail-loud) + * - unset in production → throw (fail-loud) + * - unset in non-production → generate one ephemeral 64-hex key + WARN + * + * Keys MUST be hex so the byte length is unambiguous regardless of locale or + * platform encoding. 32 hex chars = 16 bytes (the same MIN_BYTES used for + * MCP_JWT_SECRET) is the floor; `openssl rand -hex 32` (64 hex chars / 32 + * bytes) is the recommended value. + */ +function parseConsentHmacKeys(nodeEnv: string): string[] { + const raw = process.env.PATHFINDER_CONSENT_HMAC_KEY?.trim() ?? ""; + if (raw.length > 0) { + const keys = raw + .split(",") + .map((k) => k.trim()) + .filter((k) => k.length > 0); + for (const k of keys) { + if (!/^[0-9a-fA-F]{32,}$/.test(k)) { + throw new Error( + "PATHFINDER_CONSENT_HMAC_KEY entries must be ≥32 hex chars each. " + + "Generate with: openssl rand -hex 32 (comma-separated for rotation).", + ); + } + } + if (keys.length > 0) return keys; + } + if (nodeEnv === "production") { + throw new Error( + "PATHFINDER_CONSENT_HMAC_KEY is required in production. " + + "Generate with: openssl rand -hex 32 (comma-separated for rotation).", + ); + } + const ephemeral = randomBytes(32).toString("hex"); + console.warn( + "[oauth] PATHFINDER_CONSENT_HMAC_KEY not set — generated an ephemeral consent-nonce key for development. " + + "All in-flight consent nonces will be invalidated on restart.", + ); + return [ephemeral]; +} + function parseConfig(): Config { const missing: string[] = []; @@ -191,6 +246,7 @@ function parseConfig(): Config { const nodeEnv = process.env.NODE_ENV || "development"; const mcpJwtSecret = resolveJwtSecret({ nodeEnv }); + const oauthConsentHmacKeys = parseConsentHmacKeys(nodeEnv); // P2P telemetry — empty string env value treated as unset so a stray // `PATHFINDER_TELEMETRY_URL=` line in a .env file doesn't accidentally @@ -218,6 +274,7 @@ function parseConfig(): Config { discordPublicKey, notionToken, mcpJwtSecret, + oauthConsentHmacKeys, p2pTelemetryUrl, p2pTelemetryDisabled, packageVersion: readPackageVersion(), diff --git a/src/oauth/observability.ts b/src/oauth/observability.ts new file mode 100644 index 0000000..521c6ec --- /dev/null +++ b/src/oauth/observability.ts @@ -0,0 +1,86 @@ +// src/oauth/observability.ts +// Pathfinder has no metrics surface; observability is log-derived from +// the `[oauth] …` prefix. Centralised here so the operator can grep one +// vocabulary instead of N log strings sprinkled across handlers. + +type Fields = Record; + +function format(fields: Fields): string { + return Object.entries(fields) + .filter(([, v]) => v !== undefined) + .map(([k, v]) => `${k}=${v}`) + .join(" "); +} + +export const oauthLog = { + register(fields: Fields): void { + console.log(`[oauth] register ${format(fields)}`); + }, + registerRejected(fields: Fields & { reason: string }): void { + console.warn(`[oauth] register_rejected ${format(fields)}`); + }, + consentMinted(fields: Fields): void { + console.log(`[oauth] consent_minted ${format(fields)}`); + }, + consentApproved(fields: Fields): void { + console.log(`[oauth] consent_approved ${format(fields)}`); + }, + consentDenied(fields: Fields): void { + console.log(`[oauth] consent_denied ${format(fields)}`); + }, + consentNonceInvalid( + fields: Fields & { + reason: "hmac" | "expired" | "field_mismatch" | "format"; + }, + ): void { + console.warn(`[oauth] consent_nonce_invalid ${format(fields)}`); + }, + capEvicted(fields: { ttl: number; unused: number }): void { + if (fields.ttl === 0 && fields.unused === 0) return; + console.warn( + `[oauth] cap_evicted ttl=${fields.ttl} unused=${fields.unused}`, + ); + }, + capOverflow(fields: Fields & { scope: "total" | "per_ip"; ip: string }): void { + console.warn(`[oauth] cap_overflow ${format(fields)}`); + }, + // Consent-endpoint rate-limit hit. Mirrors handlers.ts:72 shape but tags + // the endpoint so the consent funnel is greppable independent of the + // generic OAuth rate-limit log. + consentRateLimited(fields: Fields & { ip: string }): void { + console.warn(`[oauth] rate_limited ${format({ ...fields, endpoint: "consent" })}`); + }, + // Post-MAC-verify tamper/divergence signals. Reaching any of these means + // the nonce MAC was valid and step 3 (field equality) passed, so the + // 400 indicates payload-vs-store divergence: either the client was + // evicted/deleted, or the bound parameters no longer match what we + // still accept. + consentStaleClient(fields: Fields & { ip: string; client_id: string }): void { + console.warn(`[oauth] consent_stale_client ${format(fields)}`); + }, + consentScopeMismatch( + fields: Fields & { ip: string; client_id: string; scope: string }, + ): void { + console.warn(`[oauth] consent_scope_mismatch ${format(fields)}`); + }, + consentParamUnsupported( + fields: Fields & { + ip: string; + client_id: string; + response_type: string; + code_challenge_method: string; + }, + ): void { + console.warn(`[oauth] consent_param_unsupported ${format(fields)}`); + }, + consentUnknownDecision( + fields: Fields & { ip: string; client_id: string; decision: string }, + ): void { + console.warn(`[oauth] consent_unknown_decision ${format(fields)}`); + }, + consentRedirectUriUnparseable( + fields: Fields & { ip: string; client_id: string }, + ): void { + console.warn(`[oauth] consent_redirect_uri_unparseable ${format(fields)}`); + }, +} as const; diff --git a/src/oauth/trusted-client-ip.ts b/src/oauth/trusted-client-ip.ts new file mode 100644 index 0000000..ac6fb0a --- /dev/null +++ b/src/oauth/trusted-client-ip.ts @@ -0,0 +1,89 @@ +import type { Request } from "express"; +import { clientIp } from "../ip-util.js"; + +/** + * Setter-injection seam — server.ts calls setTrustingProxy(...) at bootstrap. + * + * Default is a fail-safe `() => false`, so any caller that imports this + * module before server bootstrap (or a test that forgets to inject) gets the + * conservative XFF-ignoring resolver. That fail-closed default is what makes + * the spoof-bypass test green: without an active accessor, X-Forwarded-For + * is ignored and the socket peer address wins. + * + * NEVER import `server.ts` here: + * server.ts → oauth/handlers.ts → oauth/trusted-client-ip.ts + * would close an ESM cycle. Setter-injection is precisely the seam that + * keeps that edge from existing in the import graph. + */ +let trustingProxyAccessor: () => boolean = () => false; + +/** + * Tracks whether `setTrustingProxy(...)` has actually been called. The + * fail-safe default accessor is fine for tests, but in production a deploy + * that loses the bootstrap call would silently degrade to "ignore XFF / + * ignore XFP" with no warning. `assertOauthIpResolverInjected()` lets + * server.ts fail loud at startup if the wiring regresses. + */ +let injected = false; + +/** + * Inject the live "is Express trusting the proxy?" accessor. Called once at + * server bootstrap (T13 wires `() => isTrustingProxy()` from server.ts). + * Tests can swap in their own accessor to exercise both trust-proxy modes + * without booting a real Express app. + */ +export function setTrustingProxy(fn: () => boolean): void { + trustingProxyAccessor = fn; + injected = true; +} + +/** + * Read the currently-injected accessor's boolean. Other oauth/ modules (e.g. + * `handlers.ts:originOf`) need the trust-proxy decision to gate + * X-Forwarded-Proto without importing server.ts (which would close an ESM + * cycle). Returns the fail-safe `false` when the accessor has not been + * injected yet — same behavior as `oauthClientIp` pre-bootstrap. + */ +export function isTrustingProxyForOauth(): boolean { + return trustingProxyAccessor(); +} + +/** + * Diagnostic accessor — returns whether `setTrustingProxy(...)` has been + * called. Intended for startup self-checks and tests; production code should + * prefer `assertOauthIpResolverInjected()` which throws on regression. + */ +export function oauthIpResolverInjected(): boolean { + return injected; +} + +/** + * Fail-loud bootstrap guard. server.ts calls this immediately AFTER + * `setTrustingProxy(...)` to assert the wiring is live. A deploy that loses + * the bootstrap call (e.g. a refactor that dead-strips the import) would + * otherwise silently fall back to the fail-safe `() => false` default, and + * XFF/XFP would be ignored without an operator-visible signal. Throwing here + * crashes startup loudly so the regression cannot ship. + */ +export function assertOauthIpResolverInjected(): void { + if (!injected) { + throw new Error( + "oauth trusted-IP resolver was not wired at server bootstrap", + ); + } +} + +/** + * Resolve the client IP for OAuth handlers, honoring the injected + * trust-proxy decision. Delegates to the shared `clientIp(req, trustProxy)` + * in `ip-util.ts` so the OAuth path and the rest of the server stay in + * lockstep on which header is trusted. + * + * Pre-bootstrap (or in tests that don't inject) this returns the + * socket-only address — i.e. XFF is ignored. Spec §3 line 49 is explicit: + * the OAuth resolver MUST consult `isTrustingProxy()` rather than trusting + * XFF unconditionally. + */ +export function oauthClientIp(req: Request): string { + return clientIp(req, trustingProxyAccessor()); +} From a94d70926e8c445880e337f256087ce5fe35af47 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Mon, 15 Jun 2026 00:20:58 -0700 Subject: [PATCH 2/6] Add HMAC consent-nonce and redirect_uri policy Add src/oauth/consent-nonce.ts: HMAC-SHA256 over a canonical length-prefixed encoding of (clientId, redirectUri, scope, codeChallenge, codeChallengeMethod, state) with rotation-key support and constant-time verification. Add src/oauth/redirect-uri-policy.ts: enforce https-only except loopback, and reject 0.0.0.0/8, RFC1918, link-local, ULA, IPv4-mapped, and other non-public address ranges. Both modules covered by dedicated test files. --- src/__tests__/oauth-consent-nonce.test.ts | 354 ++++++++++++++++++ .../oauth-redirect-uri-policy.test.ts | 140 +++++++ src/oauth/consent-nonce.ts | 204 ++++++++++ src/oauth/redirect-uri-policy.ts | 200 ++++++++++ 4 files changed, 898 insertions(+) create mode 100644 src/__tests__/oauth-consent-nonce.test.ts create mode 100644 src/__tests__/oauth-redirect-uri-policy.test.ts create mode 100644 src/oauth/consent-nonce.ts create mode 100644 src/oauth/redirect-uri-policy.ts diff --git a/src/__tests__/oauth-consent-nonce.test.ts b/src/__tests__/oauth-consent-nonce.test.ts new file mode 100644 index 0000000..ceaa18a --- /dev/null +++ b/src/__tests__/oauth-consent-nonce.test.ts @@ -0,0 +1,354 @@ +import { describe, it, expect } from "vitest"; +import { + signConsentNonce, + verifyConsentNonce, + __testOnly_canonicalBytes, + type ConsentNoncePayload, +} from "../oauth/consent-nonce.js"; + +const k1 = "a".repeat(64); +const k2 = "b".repeat(64); + +function payload(over: Partial = {}): ConsentNoncePayload { + return { + client_id: "cid", + redirect_uri: "https://example.com/cb", + state: "s", + code_challenge: "cc", + code_challenge_method: "S256", + response_type: "code", + scope: "mcp", + resource: "", + exp: Date.now() + 600_000, + ...over, + }; +} + +function decodePayload(token: string): Record { + const b64 = token.split(".")[0]!; + return JSON.parse(Buffer.from(b64, "base64url").toString("utf8")); +} + +function reencode(decoded: Record, originalToken: string): string { + const mac = originalToken.split(".")[1]!; + const b64 = Buffer.from(JSON.stringify(decoded), "utf8").toString("base64url"); + return `${b64}.${mac}`; +} + +describe("consent nonce", () => { + it("round-trips a valid payload", () => { + const tok = signConsentNonce(payload(), [k1]); + const r = verifyConsentNonce(tok, [k1]); + expect(r.ok).toBe(true); + if (r.ok) { + expect(r.payload.client_id).toBe("cid"); + expect(r.payload.redirect_uri).toBe("https://example.com/cb"); + expect(r.payload.scope).toBe("mcp"); + } + }); + + it("rejects tampered payload (any field flip → hmac fail)", () => { + const tok = signConsentNonce(payload({ client_id: "a" }), [k1]); + const decoded = decodePayload(tok); + decoded.client_id = "b"; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("rejects expired nonce", () => { + const tok = signConsentNonce(payload({ exp: Date.now() - 1 }), [k1]); + const r = verifyConsentNonce(tok, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("expired"); + }); + + it("rejects wrong key", () => { + const tok = signConsentNonce(payload(), [k1]); + const r = verifyConsentNonce(tok, [k2]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("multi-key verify accepts old key (rotation)", () => { + const tok = signConsentNonce(payload(), [k1]); + const r = verifyConsentNonce(tok, [k2, k1]); + expect(r.ok).toBe(true); + }); + + it("multi-key verify: signer uses keys[0] (new primary)", () => { + // Sign with k2 first (primary), and verify still works when listed first. + const tok = signConsentNonce(payload(), [k2, k1]); + const r1 = verifyConsentNonce(tok, [k2]); + expect(r1.ok).toBe(true); + const r2 = verifyConsentNonce(tok, [k1]); + expect(r2.ok).toBe(false); // wasn't signed with k1 + }); + + it("rejects malformed format (no dot)", () => { + const r = verifyConsentNonce("notatoken", [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("format"); + }); + + it("rejects empty payload half", () => { + const r = verifyConsentNonce(".abc", [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("format"); + }); + + it("rejects empty mac half", () => { + const r = verifyConsentNonce("abc.", [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("format"); + }); + + it("rejects unparseable JSON payload", () => { + const garbage = Buffer.from("not json", "utf8").toString("base64url"); + const r = verifyConsentNonce(`${garbage}.YWJj`, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("format"); + }); + + it("rejects payload missing required fields (format)", () => { + // Build a valid-looking token but strip a required field. + const tok = signConsentNonce(payload(), [k1]); + const decoded = decodePayload(tok); + delete decoded.scope; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + // Missing string field fails format check (before MAC). + if (!r.ok) expect(r.reason).toBe("format"); + }); + + it("rejects payload with non-numeric exp (format)", () => { + const tok = signConsentNonce(payload(), [k1]); + const decoded = decodePayload(tok); + decoded.exp = "not a number"; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("format"); + }); + + it("MAC-then-exp ordering: tampered MAC with valid exp → hmac, NOT expired", () => { + // Future exp is fine; flip a payload byte so MAC fails. We must + // get reason=hmac, not reason=expired (we must not leak expiry). + const tok = signConsentNonce(payload({ exp: Date.now() + 60_000 }), [k1]); + const decoded = decodePayload(tok); + decoded.client_id = "different"; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("MAC-then-exp ordering: garbage MAC with expired exp → hmac, NOT expired", () => { + // Build a properly-shaped token but with a bogus MAC. With a now-1 + // exp, a verifier that checked exp first would return "expired" — + // we must return "hmac" because the MAC fails. + const validTok = signConsentNonce(payload({ exp: Date.now() - 1 }), [k1]); + const bogusMac = Buffer.from("0".repeat(32), "utf8").toString("base64url"); + const tampered = `${validTok.split(".")[0]}.${bogusMac}`; + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("response_type is part of the MAC", () => { + const tok = signConsentNonce(payload({ response_type: "code" }), [k1]); + const decoded = decodePayload(tok); + decoded.response_type = "token"; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("code_challenge_method is part of the MAC", () => { + const tok = signConsentNonce(payload({ code_challenge_method: "S256" }), [k1]); + const decoded = decodePayload(tok); + decoded.code_challenge_method = "plain"; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("resource is part of the MAC (aud-swap prevention)", () => { + const tok = signConsentNonce(payload({ resource: "https://x.example/r1" }), [k1]); + const decoded = decodePayload(tok); + decoded.resource = "https://y.example/r2"; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("redirect_uri is part of the MAC", () => { + const tok = signConsentNonce(payload({ redirect_uri: "https://a.example/cb" }), [k1]); + const decoded = decodePayload(tok); + decoded.redirect_uri = "https://attacker.example/cb"; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("state is part of the MAC", () => { + const tok = signConsentNonce(payload({ state: "alpha" }), [k1]); + const decoded = decodePayload(tok); + decoded.state = "beta"; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("scope is part of the MAC", () => { + const tok = signConsentNonce(payload({ scope: "mcp" }), [k1]); + const decoded = decodePayload(tok); + decoded.scope = "mcp admin"; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("exp is part of the MAC (cannot extend validity)", () => { + const tok = signConsentNonce(payload({ exp: Date.now() + 1_000 }), [k1]); + const decoded = decodePayload(tok); + decoded.exp = Date.now() + 10_000_000; + const tampered = reencode(decoded, tok); + const r = verifyConsentNonce(tampered, [k1]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); + + it("sign produces distinct MACs for distinct payloads", () => { + // Sanity check: two payloads whose bytes already differ at index 1 + // (regardless of length-prefixing) produce different MACs. This + // does NOT prove the length-prefix property — see the dedicated + // length-prefix tests below for that. + const p1 = payload({ client_id: "ab", redirect_uri: "https://c.example/" }); + const p2 = payload({ client_id: "a", redirect_uri: "https://bc.example/" }); + const tok1 = signConsentNonce(p1, [k1]); + const tok2 = signConsentNonce(p2, [k1]); + const mac1 = tok1.split(".")[1]!; + const mac2 = tok2.split(".")[1]!; + expect(mac1).not.toBe(mac2); + }); + + it("canonicalBytes prepends 4-byte big-endian length per field", () => { + // Pin the exact wire layout. A regression that dropped the length + // prefix, switched to little-endian, or reordered fields would + // produce a different byte string and fail this assertion + // directly — independent of MAC inequality on chosen inputs. + const fixed: ConsentNoncePayload = { + client_id: "cid", + redirect_uri: "https://example.com/cb", + state: "s", + code_challenge: "cc", + code_challenge_method: "S256", + response_type: "code", + scope: "mcp", + resource: "", + exp: 1_700_000_000_000, + }; + const FIELDS_ORDER: Array<[string, string]> = [ + ["client_id", fixed.client_id], + ["redirect_uri", fixed.redirect_uri], + ["state", fixed.state], + ["code_challenge", fixed.code_challenge], + ["code_challenge_method", fixed.code_challenge_method], + ["response_type", fixed.response_type], + ["scope", fixed.scope], + ["resource", fixed.resource], + ["exp", String(fixed.exp)], + ]; + const parts: Buffer[] = []; + for (const [, v] of FIELDS_ORDER) { + const valBuf = Buffer.from(v, "utf8"); + const lenBuf = Buffer.alloc(4); + lenBuf.writeUInt32BE(valBuf.length, 0); + parts.push(lenBuf, valBuf); + } + const expected = Buffer.concat(parts); + expect(__testOnly_canonicalBytes(fixed)).toEqual(expected); + }); + + it("length-prefix prevents field-boundary ambiguity", () => { + // Construct two payloads whose NAIVE (no-prefix) byte concatenation + // would be identical: shifting bytes across the client_id / + // redirect_uri boundary yields the same raw stream. Without length + // prefixes, the MACs would collide. With them, the prefix bytes + // for client_id (2 vs 3) differ, so the canonical bytes — and + // thus the MACs — MUST differ. + const empty = { + state: "", + code_challenge: "", + code_challenge_method: "", + response_type: "", + scope: "", + resource: "", + }; + const exp = 1_700_000_000_000; + const a: ConsentNoncePayload = { + client_id: "AB", + redirect_uri: "CD", + ...empty, + exp, + }; + const b: ConsentNoncePayload = { + client_id: "ABC", + redirect_uri: "D", + ...empty, + exp, + }; + + // Sanity: the naive (prefix-stripped) concatenation of all fields + // IS identical between a and b — so this pair specifically + // requires the length prefix to distinguish them. + const naive = (p: ConsentNoncePayload): string => + p.client_id + + p.redirect_uri + + p.state + + p.code_challenge + + p.code_challenge_method + + p.response_type + + p.scope + + p.resource + + String(p.exp); + expect(naive(a)).toBe(naive(b)); + + // Canonical (length-prefixed) bytes MUST differ. + expect(__testOnly_canonicalBytes(a)).not.toEqual(__testOnly_canonicalBytes(b)); + + // MACs MUST differ. + const tokA = signConsentNonce(a, [k1]); + const tokB = signConsentNonce(b, [k1]); + expect(tokA.split(".")[1]).not.toBe(tokB.split(".")[1]); + }); + + it("injectable now() enforces expiry against fixed clock", () => { + const tok = signConsentNonce(payload({ exp: 1_000 }), [k1]); + const r1 = verifyConsentNonce(tok, [k1], () => 500); + expect(r1.ok).toBe(true); + const r2 = verifyConsentNonce(tok, [k1], () => 2_000); + expect(r2.ok).toBe(false); + if (!r2.ok) expect(r2.reason).toBe("expired"); + }); + + it("signConsentNonce throws on empty key set", () => { + expect(() => signConsentNonce(payload(), [])).toThrow(); + }); + + it("verifyConsentNonce rejects with empty key set (no MAC match)", () => { + const tok = signConsentNonce(payload(), [k1]); + const r = verifyConsentNonce(tok, []); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("hmac"); + }); +}); diff --git a/src/__tests__/oauth-redirect-uri-policy.test.ts b/src/__tests__/oauth-redirect-uri-policy.test.ts new file mode 100644 index 0000000..2c270ac --- /dev/null +++ b/src/__tests__/oauth-redirect-uri-policy.test.ts @@ -0,0 +1,140 @@ +import { describe, it, expect } from "vitest"; +import { + validateRedirectUri, + validateRedirectUris, +} from "../oauth/redirect-uri-policy.js"; + +describe("validateRedirectUri — accepts", () => { + it.each([ + "https://example.com/cb", + "https://sub.example.com/cb?x=1", + "http://localhost:3000/cb", + "http://127.0.0.1/cb", + "http://[::1]:8080/cb", + "https://localhost:3000/cb", + // fe80::/10 upper boundary: fe80..febf are link-local, fec0 is NOT. + // Policy must NOT reject fec0:: as link-local. (Other reasons may + // apply in a stricter future policy, but per spec §2 only the + // explicit private/link-local/ULA ranges are rejected for IPv6.) + // NOTE: the plan listed `fea0::` here, but fea0 IS within fe80::/10 + // — fe80 (1111 1110 10) and fea0 (1111 1110 10) share the same /10 + // prefix. The correct outside-boundary value is fec0::. + "https://[fec0::1]/", + ])("%s", (uri) => { + expect(validateRedirectUri(uri)).toEqual({ ok: true }); + }); +}); + +describe("validateRedirectUri — rejects", () => { + it.each<[string, string]>([ + ["not a url", "parse"], + ["ftp://example.com/", "scheme"], + ["http://example.com/", "scheme"], + ["https://0.0.0.0/", "private_address"], + ["https://[::]/", "private_address"], + ["https://10.0.0.1/", "private_address"], + ["https://172.16.5.4/", "private_address"], + ["https://192.168.1.1/", "private_address"], + ["https://169.254.169.254/", "private_address"], + ["https://[fe80::1]/", "private_address"], + // fe80::/10 upper boundary: febf:: IS link-local (10th bit still in range). + ["https://[febf::1]/", "private_address"], + ["https://[fc00::1]/", "private_address"], + ["https://[fd00::1]/", "private_address"], + ["https://[::ffff:10.0.0.1]/", "private_address"], + ["https://*.example.com/cb", "wildcard"], + ["https://foo..bar.com/cb", "empty_label"], + ["https://user:pass@example.com/cb", "userinfo"], + ["https://example.com/cb#frag", "fragment"], + ["https://example.com:0/", "port"], + ["https://example.com:99999/", "parse"], + ["https://" + "a".repeat(2100) + ".com/", "too_long"], + ])("%s → %s", (uri, reason) => { + const r = validateRedirectUri(uri); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe(reason); + }); + + it("rejects empty string", () => { + const r = validateRedirectUri(""); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("empty"); + }); +}); + +describe("0.0.0.0/8 (RFC 6890 'this network')", () => { + it.each([ + "0.0.0.0", + "0.0.0.1", + "0.1.2.3", + "0.255.255.254", + ])("rejects https://%s/", (ip) => { + const r = validateRedirectUri(`https://${ip}/`); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("private_address"); + }); +}); + +describe("127.0.0.0/8 (loopback)", () => { + it.each(["127.0.0.1", "127.0.0.2", "127.255.255.254"])( + "accepts http://%s/ (loopback over http)", + (ip) => { + const r = validateRedirectUri(`http://${ip}/`); + expect(r.ok).toBe(true); + }, + ); + it.each(["127.0.0.1", "127.0.0.2"])( + "accepts https://%s/ (loopback over https)", + (ip) => { + const r = validateRedirectUri(`https://${ip}/`); + expect(r.ok).toBe(true); + }, + ); + it("rejects http://127.0.0.0/ (network address, not loopback)", () => { + const r = validateRedirectUri("http://127.0.0.0/"); + expect(r.ok).toBe(false); + }); + it("rejects https://127.0.0.0/ as private_address (network address)", () => { + const r = validateRedirectUri("https://127.0.0.0/"); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("private_address"); + }); +}); + +describe("validateRedirectUris (array)", () => { + it("accepts up to 10", () => { + const uris = Array.from( + { length: 10 }, + (_, i) => `https://example.com/cb${i}`, + ); + expect(validateRedirectUris(uris)).toEqual({ ok: true }); + }); + + it("rejects 11", () => { + const uris = Array.from( + { length: 11 }, + (_, i) => `https://example.com/cb${i}`, + ); + const r = validateRedirectUris(uris); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("too_many_uris"); + }); + + it("reports first bad index", () => { + const r = validateRedirectUris([ + "https://ok.com/", + "https://*.bad/", + ]); + expect(r.ok).toBe(false); + if (!r.ok) { + expect(r.index).toBe(1); + expect(r.reason).toBe("wildcard"); + } + }); + + it("rejects empty array", () => { + const r = validateRedirectUris([]); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.reason).toBe("empty"); + }); +}); diff --git a/src/oauth/consent-nonce.ts b/src/oauth/consent-nonce.ts new file mode 100644 index 0000000..85ec61a --- /dev/null +++ b/src/oauth/consent-nonce.ts @@ -0,0 +1,204 @@ +// Consent-nonce HMAC plumbing. +// +// The authorize → consent → token flow is stateless on the server side; the +// consent screen hands back a signed blob that names the exact OAuth params +// the user approved. Any drift between mint-time and redeem-time params +// MUST invalidate the nonce, otherwise an attacker who controls the second +// request can swap parameters (audience, redirect_uri, scope, …) under a +// user's previously-approved consent. +// +// Design: +// - HMAC-SHA256 over a canonical, length-prefixed encoding of the bound +// set. Length prefixes prevent boundary-ambiguity attacks where two +// distinct payloads would otherwise share a concatenated byte stream. +// - Wire format: `b64url(payload_json) + "." + b64url(mac)`. The payload +// is JSON so the server (and audits) can inspect it without re-deriving +// field order from a positional encoding; the MAC is what enforces +// integrity. +// - Multi-key verify for rotation: signer uses `keys[0]` (the new primary); +// verifier accepts any key in the list, so freshly-rotated deploys can +// still honor nonces minted under the previous key until they expire. +// - MAC-then-exp ordering: verify the MAC BEFORE checking expiry. Checking +// expiry first would leak an oracle ("this byte-string parses but is +// expired" vs "this byte-string is gibberish"); after the MAC check, an +// attacker who can't forge a MAC learns nothing from the expiry result. +// - Constant-time MAC compare via `crypto.timingSafeEqual`. +// +// Bound set (order is part of the canonical encoding and MUST NOT change +// without invalidating all in-flight nonces): +// client_id, redirect_uri, state, code_challenge, +// code_challenge_method, response_type, scope, resource, exp +// +// - `response_type` + `code_challenge_method` are bound to prevent a +// param-downgrade swap (e.g. PKCE method dropped to plain at redeem). +// - `resource` is bound because handlers.ts threads it through to the +// issued JWT's `aud` claim — without binding it here, an attacker could +// redeem a consent for a different audience. +// - `exp` is unix-MILLISECONDS (not seconds) for parity with `Date.now()`. + +import { createHmac, timingSafeEqual } from "node:crypto"; + +const FIELDS = [ + "client_id", + "redirect_uri", + "state", + "code_challenge", + "code_challenge_method", + "response_type", + "scope", + "resource", + "exp", +] as const; + +export interface ConsentNoncePayload { + client_id: string; + redirect_uri: string; + state: string; + code_challenge: string; + code_challenge_method: string; + response_type: string; + scope: string; + resource: string; + exp: number; // unix milliseconds +} + +export type VerifyResult = + | { ok: true; payload: ConsentNoncePayload } + | { ok: false; reason: "format" | "hmac" | "expired" }; + +/** + * Canonical length-prefixed encoding of the bound set. + * + * For each field in `FIELDS` order, append: + * - 4 bytes: big-endian uint32 of the UTF-8 byte length of the value + * - N bytes: the UTF-8 bytes of the value + * + * `exp` is encoded as its decimal ASCII representation. + * + * The length prefix is what makes the encoding unambiguous: without it, + * the pair (client_id="ab", redirect_uri="c") and (client_id="a", + * redirect_uri="bc") would concatenate to the same byte stream. The MAC + * would then collide and an attacker could shift bytes across the field + * boundary undetected. + */ +function canonicalBytes(p: ConsentNoncePayload): Buffer { + const parts: Buffer[] = []; + for (const f of FIELDS) { + const raw = f === "exp" ? String(p.exp) : (p[f] as string); + const v = typeof raw === "string" ? raw : ""; + const valBuf = Buffer.from(v, "utf8"); + const lenBuf = Buffer.alloc(4); + lenBuf.writeUInt32BE(valBuf.length, 0); + parts.push(lenBuf, valBuf); + } + return Buffer.concat(parts); +} + +/** + * Test-only export of the internal canonical encoder. + * + * Tests pin the exact wire layout (length-prefixed, big-endian uint32 + * lengths, FIELDS order) so a regression that removed the length prefix — + * or reordered the fields — is caught directly, not only via MAC + * inequality on inputs that already differ byte-for-byte. + * + * NOT part of the public API. Do not import from production code. + */ +export const __testOnly_canonicalBytes = canonicalBytes; + +function macWith(key: string, p: ConsentNoncePayload): Buffer { + return createHmac("sha256", key).update(canonicalBytes(p)).digest(); +} + +/** + * Sign a consent-nonce payload. The signer always uses `keys[0]` (the + * current primary). Multi-key lists exist for the verifier's benefit — + * during rotation, callers prepend the new primary so any nonces minted + * before the rotation still verify against the (now-second) old key. + */ +export function signConsentNonce(p: ConsentNoncePayload, keys: string[]): string { + if (keys.length === 0) throw new Error("signConsentNonce: empty key set"); + const mac = macWith(keys[0]!, p); + const payloadB64 = Buffer.from(JSON.stringify(p), "utf8").toString("base64url"); + return `${payloadB64}.${mac.toString("base64url")}`; +} + +/** + * Verify a consent nonce. + * + * Order of checks (deliberate): + * 1. Structural format: `payload.mac` shape, base64url-decodable JSON, + * all bound fields present with the right primitive type. + * 2. MAC: try every key in `keys`. Constant-time compare per attempt. + * 3. Expiry: only AFTER a successful MAC. Returning "expired" before MAC + * would let an attacker probe the format/expiry layer without ever + * producing a valid MAC. + * + * `now` is injectable for tests; defaults to `Date.now`. + */ +export function verifyConsentNonce( + token: string, + keys: string[], + now: () => number = Date.now, +): VerifyResult { + if (typeof token !== "string") return { ok: false, reason: "format" }; + const dot = token.indexOf("."); + if (dot <= 0 || dot === token.length - 1) return { ok: false, reason: "format" }; + const payloadB64 = token.slice(0, dot); + const macB64 = token.slice(dot + 1); + if (payloadB64.length === 0 || macB64.length === 0) { + return { ok: false, reason: "format" }; + } + + let parsed: unknown; + try { + parsed = JSON.parse(Buffer.from(payloadB64, "base64url").toString("utf8")); + } catch { + return { ok: false, reason: "format" }; + } + if (parsed === null || typeof parsed !== "object") { + return { ok: false, reason: "format" }; + } + const obj = parsed as Record; + for (const f of FIELDS) { + if (f === "exp") { + if (typeof obj.exp !== "number" || !Number.isFinite(obj.exp)) { + return { ok: false, reason: "format" }; + } + } else if (typeof obj[f] !== "string") { + return { ok: false, reason: "format" }; + } + } + const p: ConsentNoncePayload = { + client_id: obj.client_id as string, + redirect_uri: obj.redirect_uri as string, + state: obj.state as string, + code_challenge: obj.code_challenge as string, + code_challenge_method: obj.code_challenge_method as string, + response_type: obj.response_type as string, + scope: obj.scope as string, + resource: obj.resource as string, + exp: obj.exp as number, + }; + + let given: Buffer; + try { + given = Buffer.from(macB64, "base64url"); + } catch { + return { ok: false, reason: "format" }; + } + if (given.length === 0) return { ok: false, reason: "format" }; + + let matched = false; + for (const k of keys) { + const expected = macWith(k, p); + if (expected.length === given.length && timingSafeEqual(expected, given)) { + matched = true; + break; + } + } + if (!matched) return { ok: false, reason: "hmac" }; + + if (p.exp <= now()) return { ok: false, reason: "expired" }; + return { ok: true, payload: p }; +} diff --git a/src/oauth/redirect-uri-policy.ts b/src/oauth/redirect-uri-policy.ts new file mode 100644 index 0000000..6778cd9 --- /dev/null +++ b/src/oauth/redirect-uri-policy.ts @@ -0,0 +1,200 @@ +// Pure URL policy validator for OAuth client redirect_uris. +// +// Rules (spec §2): +// - Length ≤ 2048 chars. +// - Scheme `https` (any host) OR `http`/`https` on loopback +// (`localhost`, `[::1]`, anything in `127.0.0.0/8` except the +// network address `127.0.0.0`). +// - Reject the full `0.0.0.0/8` (RFC 6890 "this network"), `[::]`, +// RFC1918 IPv4 (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`), +// link-local `169.254.0.0/16`, the loopback network address +// `127.0.0.0`, IPv6 link-local `fe80::/10`, IPv6 ULA `fc00::/7`, +// IPv4-mapped IPv6 (`::ffff:a.b.c.d`) whose embedded address falls +// in any rejected range. +// - Reject hostnames containing `*`, `..`, or empty labels. +// - Reject userinfo (`url.username !== ""` or `url.password !== ""`). +// - Reject fragments (`url.hash !== ""`). +// - Reject explicit port outside `1–65535`. +// - No DNS resolution by design — a pure policy check. +// +// Boundary discipline: IPv6 link-local fe80::/10 uses precise prefix +// matching `(v & 0xffc0) === 0xfe80` (not a regex) so fea0:: (10th bit=0) +// is NOT misclassified as link-local while febf:: IS (10th bit in range). +// +// Note on Node URL normalization quirks driving this code: +// - `url.hostname` for an IPv6 literal RETAINS the brackets (e.g. +// `[::1]`), unlike most documentation suggests. +// - Node compresses `::ffff:10.0.0.1` → `::ffff:a00:1`, so IPv4-mapped +// detection must support BOTH the dotted form AND the hex form (the +// last 32 bits being `ffff:XXXX:YYYY`). + +import { isIPv4 } from "node:net"; + +const MAX_URI_LEN = 2048; +const MAX_URIS = 10; + +export type PolicyResult = { ok: true } | { ok: false; reason: string }; +export type PolicyArrayResult = + | { ok: true } + | { ok: false; reason: string; index: number }; + +/** + * Loopback hosts get the http-scheme exception. The literal forms + * `localhost` and `[::1]` are matched directly; for IPv4 the entire + * `127.0.0.0/8` range is loopback per RFC 6890, EXCEPT the network + * address `127.0.0.0` itself, which is treated as a private address and + * rejected. + */ +function isLoopbackHost(host: string): boolean { + const lower = host.toLowerCase(); + if (lower === "localhost" || lower === "[::1]") return true; + // Defensive: Node's `url.hostname` keeps brackets around IPv6 + // literals, but accept the bracketless form too in case a caller + // passes a pre-normalized value. + if (lower === "::1") return true; + if (!isIPv4(host)) return false; + // 127.0.0.0/8 is loopback, but 127.0.0.0 is the network address — + // reject it as private, not loopback. + if (host === "127.0.0.0") return false; + return host.split(".")[0] === "127"; +} + +function isPrivateIPv4(ip: string): boolean { + const m = ip.match(/^(\d+)\.(\d+)\.(\d+)\.(\d+)$/); + if (!m) return false; + const a = parseInt(m[1]!, 10); + const b = parseInt(m[2]!, 10); + // 0.0.0.0/8 — RFC 6890 "this network". Reject the entire /8, not + // just the literal 0.0.0.0. + if (a === 0) return true; + if (a === 10) return true; + if (a === 172 && b >= 16 && b <= 31) return true; + if (a === 192 && b === 168) return true; + if (a === 169 && b === 254) return true; + // 127.0.0.0 is the loopback network address — not a usable host. + if (ip === "127.0.0.0") return true; + return false; +} + +/** + * Numeric prefix tests so the regex doesn't drift on Node's URL + * normalization. `host` is the bracketed IPv6 form returned by + * `url.hostname` (e.g. `[fe80::1]`). + * - fe80::/10 → first 16-bit group v satisfies (v & 0xffc0) === 0xfe80 + * - fc00::/7 → first 16-bit group v satisfies (v & 0xfe00) === 0xfc00 + * Catches fe80..febf as link-local; fea0:: must NOT match (10th bit = 0). + * + * Also detects IPv4-mapped IPv6 (`::ffff:a.b.c.d` or its hex-compressed + * cousin `::ffff:XXXX:YYYY`) whose embedded address falls in any + * rejected IPv4 range. + */ +function isPrivateIPv6(host: string): boolean { + if (host === "[::]") return true; + const inner = host.replace(/^\[|\]$/g, "").toLowerCase(); + + // IPv4-mapped form with dotted suffix: ::ffff:a.b.c.d + const dotted = inner.match(/^::ffff:(\d+\.\d+\.\d+\.\d+)$/); + if (dotted) { + const v4 = dotted[1]!; + return isPrivateIPv4(v4) || v4 === "0.0.0.0"; + } + + // IPv4-mapped form with hex suffix: ::ffff:XXXX:YYYY (Node's + // normalized form for ::ffff:a.b.c.d). The last two 16-bit groups + // encode the IPv4 address as 4 hex bytes. + const hexMapped = inner.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); + if (hexMapped) { + const hi = parseInt(hexMapped[1]!, 16); + const lo = parseInt(hexMapped[2]!, 16); + if (Number.isFinite(hi) && Number.isFinite(lo)) { + const a = (hi >> 8) & 0xff; + const b = hi & 0xff; + const c = (lo >> 8) & 0xff; + const d = lo & 0xff; + const v4 = `${a}.${b}.${c}.${d}`; + return isPrivateIPv4(v4) || v4 === "0.0.0.0"; + } + } + + const seg0 = inner.split(":")[0] ?? ""; + if (seg0.length === 0) return false; + const v = parseInt(seg0, 16); + if (!Number.isFinite(v)) return false; + if ((v & 0xffc0) === 0xfe80) return true; // fe80::/10 + if ((v & 0xfe00) === 0xfc00) return true; // fc00::/7 + return false; +} + +export function validateRedirectUri(uri: string): PolicyResult { + if (typeof uri !== "string" || uri.length === 0) { + return { ok: false, reason: "empty" }; + } + if (uri.length > MAX_URI_LEN) return { ok: false, reason: "too_long" }; + + let url: URL; + try { + url = new URL(uri); + } catch { + return { ok: false, reason: "parse" }; + } + + if (url.username !== "" || url.password !== "") { + return { ok: false, reason: "userinfo" }; + } + if (url.hash !== "") return { ok: false, reason: "fragment" }; + + const host = url.hostname; // IPv6 keeps brackets (e.g. "[::1]") + if (host.length === 0) return { ok: false, reason: "empty_label" }; + if (host.includes("*")) return { ok: false, reason: "wildcard" }; + + // Empty-label check is only meaningful for hostnames (not IPv6 literals). + if (!host.startsWith("[")) { + const labels = host.split("."); + if (labels.length > 1 && labels.some((l) => l.length === 0)) { + return { ok: false, reason: "empty_label" }; + } + } + + if (url.port !== "") { + const p = parseInt(url.port, 10); + if (!Number.isInteger(p) || p < 1 || p > 65535) { + return { ok: false, reason: "port" }; + } + } + + const scheme = url.protocol.replace(/:$/, ""); + const isLoopback = isLoopbackHost(host); + if (scheme === "https") { + // ok for any host EXCEPT private space (checked below) + } else if (scheme === "http") { + if (!isLoopback) return { ok: false, reason: "scheme" }; + } else { + return { ok: false, reason: "scheme" }; + } + + if (!isLoopback) { + if (host.startsWith("[")) { + if (isPrivateIPv6(host)) { + return { ok: false, reason: "private_address" }; + } + } else if (isIPv4(host) && isPrivateIPv4(host)) { + return { ok: false, reason: "private_address" }; + } + } + + return { ok: true }; +} + +export function validateRedirectUris(list: string[]): PolicyArrayResult { + if (!Array.isArray(list) || list.length === 0) { + return { ok: false, reason: "empty", index: 0 }; + } + if (list.length > MAX_URIS) { + return { ok: false, reason: "too_many_uris", index: MAX_URIS }; + } + for (let i = 0; i < list.length; i++) { + const r = validateRedirectUri(list[i]!); + if (!r.ok) return { ok: false, reason: r.reason, index: i }; + } + return { ok: true }; +} From dc7feceea563cee462076b95cd903c59d12cfe88 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Mon, 15 Jun 2026 00:21:03 -0700 Subject: [PATCH 3/6] Add OAuth client cap with TTL eviction and touch Extend src/oauth/store.ts: add a registered-client cap with lazy TTL-based eviction, a touch() entrypoint that bumps lastSeen on successful token exchange, and a ClientCapError raised when the cap is exhausted with no evictable entries. Add a consentLimiter (30/min) to src/oauth/rate-limiter.ts for the consent POST endpoint. Tests extended accordingly. --- src/__tests__/oauth-rate-limiter.test.ts | 18 ++- src/__tests__/oauth-store.test.ts | 133 +++++++++++++++++++++-- src/oauth/rate-limiter.ts | 1 + src/oauth/store.ts | 112 ++++++++++++++++++- 4 files changed, 254 insertions(+), 10 deletions(-) diff --git a/src/__tests__/oauth-rate-limiter.test.ts b/src/__tests__/oauth-rate-limiter.test.ts index 3c52c03..2dc8a2b 100644 --- a/src/__tests__/oauth-rate-limiter.test.ts +++ b/src/__tests__/oauth-rate-limiter.test.ts @@ -1,10 +1,17 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; -import { OAuthRateLimiter } from "../oauth/rate-limiter.js"; +import { + OAuthRateLimiter, + consentLimiter, +} from "../oauth/rate-limiter.js"; describe("OAuthRateLimiter", () => { beforeEach(() => { vi.useFakeTimers(); vi.setSystemTime(new Date(2026, 0, 1, 0, 0, 0)); + // The shared consentLimiter singleton is mutated by one assertion below; + // clear its buckets so state doesn't leak across this file's tests or + // across other files under fork reuse. + (consentLimiter as unknown as { buckets: Map }).buckets.clear(); }); afterEach(() => { @@ -56,4 +63,13 @@ describe("OAuthRateLimiter", () => { expect(result.retryAfterSec).toBeLessThanOrEqual(45); expect(result.retryAfterSec).toBeGreaterThanOrEqual(44); }); + + it("consentLimiter allows 30 requests/minute per IP then rejects", () => { + for (let i = 0; i < 30; i++) { + expect(consentLimiter.check("9.9.9.9").ok).toBe(true); + } + const result = consentLimiter.check("9.9.9.9"); + expect(result.ok).toBe(false); + expect(result.retryAfterSec).toBeGreaterThan(0); + }); }); diff --git a/src/__tests__/oauth-store.test.ts b/src/__tests__/oauth-store.test.ts index ad48287..91709a1 100644 --- a/src/__tests__/oauth-store.test.ts +++ b/src/__tests__/oauth-store.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, vi, afterEach } from "vitest"; -import { ClientStore, CodeStore } from "../oauth/store.js"; +import { ClientStore, ClientCapError, CodeStore } from "../oauth/store.js"; describe("ClientStore", () => { let store: ClientStore; @@ -10,6 +10,7 @@ describe("ClientStore", () => { it("register returns client_id, client_id_issued_at, and echoes redirect_uris", () => { const result = store.register({ redirect_uris: ["https://example.com/cb"], + ip: "1.1.1.1", }); expect(result.client_id).toBeDefined(); expect(typeof result.client_id).toBe("string"); @@ -18,7 +19,7 @@ describe("ClientStore", () => { }); it("register issues a client_secret with base64url encoding and secret metadata", () => { - const result = store.register({ redirect_uris: [] }); + const result = store.register({ redirect_uris: [], ip: "1.1.1.1" }); expect(result.client_secret).toBeDefined(); expect(typeof result.client_secret).toBe("string"); // base64url (no +/= chars); 32 bytes → 43 chars @@ -29,14 +30,15 @@ describe("ClientStore", () => { }); it("two registers issue distinct client_secrets", () => { - const a = store.register({ redirect_uris: [] }); - const b = store.register({ redirect_uris: [] }); + const a = store.register({ redirect_uris: [], ip: "1.1.1.1" }); + const b = store.register({ redirect_uris: [], ip: "1.1.1.1" }); expect(a.client_secret).not.toBe(b.client_secret); }); it("get(client_id) returns registered client", () => { const { client_id } = store.register({ redirect_uris: ["https://example.com/cb"], + ip: "1.1.1.1", }); const fetched = store.get(client_id); expect(fetched).toBeDefined(); @@ -44,8 +46,8 @@ describe("ClientStore", () => { }); it("two registers return distinct UUIDs", () => { - const a = store.register({ redirect_uris: [] }); - const b = store.register({ redirect_uris: [] }); + const a = store.register({ redirect_uris: [], ip: "1.1.1.1" }); + const b = store.register({ redirect_uris: [], ip: "1.1.1.1" }); expect(a.client_id).not.toBe(b.client_id); }); @@ -54,9 +56,126 @@ describe("ClientStore", () => { }); it("accepts empty redirect_uris array", () => { - const r = store.register({ redirect_uris: [] }); + const r = store.register({ redirect_uris: [], ip: "1.1.1.1" }); expect(r.redirect_uris).toEqual([]); }); + + it("register persists client_name truncated to 80 chars", () => { + const longName = "x".repeat(200); + const r = store.register({ + redirect_uris: [], + client_name: longName, + ip: "1.1.1.1", + }); + expect(r.client_name).toBe("x".repeat(80)); + }); + + it("register sets registeredAt and lastUsedAt to now", () => { + const before = Date.now(); + const r = store.register({ redirect_uris: [], ip: "1.1.1.1" }); + const after = Date.now(); + expect(r.registeredAt).toBeGreaterThanOrEqual(before); + expect(r.registeredAt).toBeLessThanOrEqual(after); + expect(r.lastUsedAt).toBe(r.registeredAt); + }); +}); + +describe("ClientStore — cap + eviction", () => { + let store: ClientStore; + beforeEach(() => { + vi.useFakeTimers(); + store = new ClientStore(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it("touch bumps lastUsedAt", () => { + const { client_id } = store.register({ + redirect_uris: [], + ip: "1.1.1.1", + }); + const before = store.get(client_id)!.lastUsedAt; + vi.advanceTimersByTime(5000); + store.touch(client_id); + expect(store.get(client_id)!.lastUsedAt).toBe(before + 5000); + }); + + it("touch is a no-op for unknown clients", () => { + expect(() => store.touch("nope")).not.toThrow(); + }); + + it("rejects with ClientCapError(per_ip) at 101st registration from same ip", () => { + for (let i = 0; i < 100; i++) { + store.register({ redirect_uris: [], ip: "1.1.1.1" }); + } + let caught: unknown; + try { + store.register({ redirect_uris: [], ip: "1.1.1.1" }); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(ClientCapError); + expect((caught as ClientCapError).scope).toBe("per_ip"); + }); + + it("evicts ttl-stale clients on overflow then accepts new registration", () => { + const small = new ClientStore({ maxTotal: 3, maxPerIp: 100 }); + const a = small.register({ redirect_uris: [], ip: "1.1.1.1" }); + // Bump `a.lastUsedAt` to mimic real usage so it crosses the 30d ttl gate. + small.touch(a.client_id); + vi.advanceTimersByTime(30 * 24 * 3600 * 1000 + 1); + small.register({ redirect_uris: [], ip: "2.2.2.2" }); + small.register({ redirect_uris: [], ip: "3.3.3.3" }); + // Cap = 3, currently 3 (a + 2 fresh). Next register → sweep evicts stale `a`, succeeds. + expect(() => + small.register({ redirect_uris: [], ip: "4.4.4.4" }), + ).not.toThrow(); + expect(small.get(a.client_id)).toBeUndefined(); + }); + + it("evicts never-used clients (registeredAt < now - 7d, lastUsedAt unchanged) on total-cap overflow", () => { + const small = new ClientStore({ maxTotal: 3, maxPerIp: 100 }); + const a = small.register({ redirect_uris: [], ip: "1.1.1.1" }); + // NOTE: a.lastUsedAt === a.registeredAt (never touched). Advance 7d+1ms. + vi.advanceTimersByTime(7 * 24 * 3600 * 1000 + 1); + small.register({ redirect_uris: [], ip: "2.2.2.2" }); + small.register({ redirect_uris: [], ip: "3.3.3.3" }); + // Total at cap (3). Next register triggers sweep → evicts `a` (never-used + >7d). + expect(() => + small.register({ redirect_uris: [], ip: "4.4.4.4" }), + ).not.toThrow(); + expect(small.get(a.client_id)).toBeUndefined(); + }); + + it("rejects with ClientCapError(total) when total cap exceeded even after sweep", () => { + const small = new ClientStore({ maxTotal: 3, maxPerIp: 100 }); + small.register({ redirect_uris: [], ip: "a" }); + small.register({ redirect_uris: [], ip: "b" }); + small.register({ redirect_uris: [], ip: "c" }); + let caught: unknown; + try { + small.register({ redirect_uris: [], ip: "d" }); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(ClientCapError); + expect((caught as ClientCapError).scope).toBe("total"); + }); + + it("emits cap_evicted log when sweep removes any client", () => { + const small = new ClientStore({ maxTotal: 2, maxPerIp: 100 }); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const a = small.register({ redirect_uris: [], ip: "1.1.1.1" }); + small.touch(a.client_id); + vi.advanceTimersByTime(30 * 24 * 3600 * 1000 + 1); + small.register({ redirect_uris: [], ip: "2.2.2.2" }); + small.register({ redirect_uris: [], ip: "3.3.3.3" }); // triggers sweep + expect(warnSpy).toHaveBeenCalledWith( + expect.stringMatching(/\[oauth\] cap_evicted ttl=1 unused=0/), + ); + warnSpy.mockRestore(); + }); }); describe("CodeStore", () => { diff --git a/src/oauth/rate-limiter.ts b/src/oauth/rate-limiter.ts index 8a0ad9a..525d9be 100644 --- a/src/oauth/rate-limiter.ts +++ b/src/oauth/rate-limiter.ts @@ -46,3 +46,4 @@ export class OAuthRateLimiter { export const registerLimiter = new OAuthRateLimiter(10, 60_000); export const authorizeLimiter = new OAuthRateLimiter(30, 60_000); export const tokenLimiter = new OAuthRateLimiter(30, 60_000); +export const consentLimiter = new OAuthRateLimiter(30, 60_000); diff --git a/src/oauth/store.ts b/src/oauth/store.ts index f0c4297..94465f1 100644 --- a/src/oauth/store.ts +++ b/src/oauth/store.ts @@ -2,6 +2,17 @@ // Singleton exports match the `src/ip-limiter.ts` pattern. import { randomBytes, randomUUID } from "node:crypto"; +import { oauthLog } from "./observability.js"; + +const DAY_MS = 24 * 60 * 60 * 1000; +const DEFAULT_MAX_TOTAL = 10_000; +const DEFAULT_MAX_PER_IP = 100; +// Eviction policy (spec §3 lines 43–53): +// - lastUsedAt < now - 30d → "ttl" (used at least once, then stale) +// - registeredAt < now - 7d AND +// lastUsedAt === registeredAt → "unused" (never used after registration) +const USED_TTL_MS = 30 * DAY_MS; +const UNUSED_TTL_MS = 7 * DAY_MS; export interface RegisteredClient { client_id: string; @@ -10,6 +21,9 @@ export interface RegisteredClient { client_secret_issued_at: number; client_secret_expires_at: number; redirect_uris: string[]; + client_name: string; + registeredAt: number; + lastUsedAt: number; } function base64url(buf: Buffer): string { @@ -36,11 +50,85 @@ export interface IssueCodeInput { ttlMs: number; } +export type CapOverflow = "total" | "per_ip"; + +export class ClientCapError extends Error { + constructor(public readonly scope: CapOverflow) { + super(`client cap overflow: ${scope}`); + this.name = "ClientCapError"; + } +} + +export interface RegisterInput { + redirect_uris: string[]; + client_name?: string; + ip: string; +} + export class ClientStore { private clients = new Map(); + // Per-IP index for O(1) per-IP cap check; siblings of `clients`. + private byIp = new Map>(); + // Reverse map so `delete(id)` can find the IP bucket without scanning. + private clientIpOf = new Map(); + private readonly maxTotal: number; + private readonly maxPerIp: number; - register(input: { redirect_uris: string[] }): RegisteredClient { - const issuedAt = Math.floor(Date.now() / 1000); + constructor(opts: { maxTotal?: number; maxPerIp?: number } = {}) { + this.maxTotal = opts.maxTotal ?? DEFAULT_MAX_TOTAL; + this.maxPerIp = opts.maxPerIp ?? DEFAULT_MAX_PER_IP; + } + + /** + * Single-pass eviction. Returns counts by reason so the caller can emit + * `cap_evicted` for observability. No-op when nothing matches the policy. + */ + private sweepOnce(now: number): { ttl: number; unused: number } { + let ttl = 0; + let unused = 0; + for (const [id, c] of this.clients) { + const usedStale = c.lastUsedAt + USED_TTL_MS < now; + const neverUsed = c.lastUsedAt === c.registeredAt; + const unusedStale = neverUsed && c.registeredAt + UNUSED_TTL_MS < now; + if (usedStale) { + this.deleteInternal(id); + ttl++; + } else if (unusedStale) { + this.deleteInternal(id); + unused++; + } + } + return { ttl, unused }; + } + + private deleteInternal(id: string): void { + const ip = this.clientIpOf.get(id); + this.clients.delete(id); + this.clientIpOf.delete(id); + if (ip) { + const set = this.byIp.get(ip); + if (set) { + set.delete(id); + if (set.size === 0) this.byIp.delete(ip); + } + } + } + + register(input: RegisterInput): RegisteredClient { + const now = Date.now(); + const perIp = this.byIp.get(input.ip)?.size ?? 0; + // Lazy sweep: only run when at-or-over a cap. Single sweep per call. + if (perIp >= this.maxPerIp || this.clients.size >= this.maxTotal) { + const swept = this.sweepOnce(now); + oauthLog.capEvicted(swept); + } + const perIpAfter = this.byIp.get(input.ip)?.size ?? 0; + if (perIpAfter >= this.maxPerIp) throw new ClientCapError("per_ip"); + if (this.clients.size >= this.maxTotal) throw new ClientCapError("total"); + + const issuedAt = Math.floor(now / 1000); + const rawName = + typeof input.client_name === "string" ? input.client_name : ""; const client: RegisteredClient = { client_id: randomUUID(), client_secret: base64url(randomBytes(32)), @@ -48,14 +136,34 @@ export class ClientStore { client_secret_issued_at: issuedAt, client_secret_expires_at: 0, redirect_uris: [...input.redirect_uris], + client_name: rawName.slice(0, 80), + registeredAt: now, + lastUsedAt: now, }; this.clients.set(client.client_id, client); + this.clientIpOf.set(client.client_id, input.ip); + let set = this.byIp.get(input.ip); + if (!set) { + set = new Set(); + this.byIp.set(input.ip, set); + } + set.add(client.client_id); return client; } get(clientId: string): RegisteredClient | undefined { return this.clients.get(clientId); } + + /** + * Bumps `lastUsedAt` to now. No-op for unknown clients — they may have + * been evicted by a prior sweep, and callers (token grant paths) should + * not crash on that race. + */ + touch(clientId: string): void { + const c = this.clients.get(clientId); + if (c) c.lastUsedAt = Date.now(); + } } export class CodeStore { From e151de3ce89e2b5e85fc87071194225616ca26af Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Mon, 15 Jun 2026 00:21:09 -0700 Subject: [PATCH 4/6] Add OAuth consent screen and POST consent handler Add src/oauth/consent-template.ts: server-rendered HTML consent screen with every interpolation HTML-escaped and no inline JavaScript. Add src/oauth/consent-handler.ts: the POST /authorize/consent handler implementing the 8-step approval pipeline (rate-limit, parse, verify HMAC nonce, reload client, recheck redirect_uri policy, mint code, persist, redirect) where the redirect URI comes from the nonce-bound payload rather than the form body. --- src/__tests__/oauth-consent-handler.test.ts | 474 +++++++++++++++++++ src/__tests__/oauth-consent-template.test.ts | 122 +++++ src/oauth/consent-handler.ts | 289 +++++++++++ src/oauth/consent-template.ts | 121 +++++ 4 files changed, 1006 insertions(+) create mode 100644 src/__tests__/oauth-consent-handler.test.ts create mode 100644 src/__tests__/oauth-consent-template.test.ts create mode 100644 src/oauth/consent-handler.ts create mode 100644 src/oauth/consent-template.ts diff --git a/src/__tests__/oauth-consent-handler.test.ts b/src/__tests__/oauth-consent-handler.test.ts new file mode 100644 index 0000000..b4b21a2 --- /dev/null +++ b/src/__tests__/oauth-consent-handler.test.ts @@ -0,0 +1,474 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; + +// Mock config — the consent handler reads getConfig().oauthConsentHmacKeys +// to verify the signed nonce. Use a stable key so tests can sign payloads +// with the same value via the real signConsentNonce. +vi.mock("../config.js", () => ({ + getConfig: vi.fn().mockReturnValue({ + port: 3001, + databaseUrl: "pglite:///tmp/test", + openaiApiKey: "", + githubToken: "", + githubWebhookSecret: "", + nodeEnv: "test", + logLevel: "info", + cloneDir: "/tmp/test", + slackBotToken: "", + slackSigningSecret: "", + discordBotToken: "", + discordPublicKey: "", + notionToken: "", + mcpJwtSecret: "a".repeat(64), + oauthConsentHmacKeys: ["a".repeat(64)], + p2pTelemetryUrl: undefined, + p2pTelemetryDisabled: false, + packageVersion: "test", + }), + getServerConfig: vi.fn(), + getAnalyticsConfig: vi.fn(), + hasSearchTools: vi.fn().mockReturnValue(false), + hasKnowledgeTools: vi.fn().mockReturnValue(false), + hasCollectTools: vi.fn().mockReturnValue(false), + hasBashSemanticSearch: vi.fn().mockReturnValue(false), +})); + +import { consentHandler } from "../oauth/consent-handler.js"; +import { + signConsentNonce, + type ConsentNoncePayload, +} from "../oauth/consent-nonce.js"; +import { clientStore, codeStore } from "../oauth/store.js"; +import { setTrustingProxy } from "../oauth/trusted-client-ip.js"; +import { consentLimiter } from "../oauth/rate-limiter.js"; + +const KEY = "a".repeat(64); +const TOKEN_SCOPE = "mcp"; + +function mockReq( + overrides: Record = {}, +): Record { + return { + headers: { + host: "mcp.example.com", + "x-forwarded-proto": "https", + }, + query: {}, + body: {}, + socket: { remoteAddress: "1.2.3.4" }, + ...overrides, + }; +} + +function mockRes() { + const json = vi.fn(); + const send = vi.fn(); + const redirect = vi.fn(); + const setHeader = vi.fn(); + const status = vi.fn().mockImplementation(() => ({ json, send })); + return { + json, + send, + redirect, + setHeader, + status, + get statusCode() { + return status.mock.calls.at(-1)?.[0]; + }, + }; +} + +function basePayload(over: Partial = {}): ConsentNoncePayload { + return { + client_id: "cid", + redirect_uri: "https://legitimate.example/cb", + state: "state-abc", + code_challenge: "cc", + code_challenge_method: "S256", + response_type: "code", + scope: TOKEN_SCOPE, + resource: "", + exp: Date.now() + 600_000, + ...over, + }; +} + +function bodyFor(p: ConsentNoncePayload, nonce: string, decision: string) { + return { + nonce, + client_id: p.client_id, + redirect_uri: p.redirect_uri, + state: p.state, + code_challenge: p.code_challenge, + code_challenge_method: p.code_challenge_method, + response_type: p.response_type, + scope: p.scope, + resource: p.resource, + decision, + }; +} + +function resetClientStore() { + const cs = clientStore as unknown as { + clients: Map; + byIp: Map>; + clientIpOf: Map; + }; + cs.clients.clear(); + cs.byIp.clear(); + cs.clientIpOf.clear(); + const cds = codeStore as unknown as { codes: Map }; + cds.codes.clear(); +} + +function registerKnownClient(client_id: string, redirect_uri: string) { + // ClientStore.register generates its own client_id, so we poke the + // internal Map directly with the bound id from our payload — fast and + // keeps tests deterministic. + const cs = clientStore as unknown as { + clients: Map; + byIp: Map>; + clientIpOf: Map; + }; + const now = Date.now(); + cs.clients.set(client_id, { + client_id, + client_secret: "s", + client_id_issued_at: Math.floor(now / 1000), + client_secret_issued_at: Math.floor(now / 1000), + client_secret_expires_at: 0, + redirect_uris: [redirect_uri], + client_name: "Test App", + registeredAt: now, + lastUsedAt: now, + }); + cs.clientIpOf.set(client_id, "1.2.3.4"); + let set = cs.byIp.get("1.2.3.4"); + if (!set) { + set = new Set(); + cs.byIp.set("1.2.3.4", set); + } + set.add(client_id); +} + +beforeEach(() => { + resetClientStore(); + // Reset rate-limiter buckets so per-IP windows don't leak across tests + // (or across files under fork reuse). + (consentLimiter as unknown as { buckets: Map }).buckets.clear(); + setTrustingProxy(() => false); +}); + +afterEach(() => { + setTrustingProxy(() => false); +}); + +describe("consentHandler — approve", () => { + it("approve happy path → 302 to nonce-bound redirect_uri with code + state", () => { + const p = basePayload(); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const req = mockReq({ body: bodyFor(p, nonce, "approve") }); + const res = mockRes(); + + consentHandler(req as never, res as never); + + expect(res.redirect).toHaveBeenCalledTimes(1); + const target = new URL(res.redirect.mock.calls[0]![0] as string); + expect(target.hostname).toBe("legitimate.example"); + expect(target.pathname).toBe("/cb"); + expect(target.searchParams.get("state")).toBe("state-abc"); + expect(target.searchParams.get("code")).toBeTruthy(); + }); + + it("approve touches the client (lastUsedAt bumped)", () => { + // Use fake timers so we deterministically observe a strictly-later + // `lastUsedAt` than the registration timestamp without burning CPU + // and without flaking on slow runners. + vi.useFakeTimers(); + try { + const baseMs = new Date(2026, 0, 1, 0, 0, 0).getTime(); + vi.setSystemTime(new Date(baseMs)); + const p = basePayload({ exp: baseMs + 600_000 }); + registerKnownClient(p.client_id, p.redirect_uri); + const cs = clientStore as unknown as { + clients: Map; + }; + const beforeTs = cs.clients.get(p.client_id)!.lastUsedAt; + expect(beforeTs).toBe(baseMs); + // Advance the clock so consentHandler -> clientStore.touch() observes + // a strictly-later Date.now(). + vi.setSystemTime(new Date(baseMs + 5)); + const nonce = signConsentNonce(p, [KEY]); + const req = mockReq({ body: bodyFor(p, nonce, "approve") }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(cs.clients.get(p.client_id)!.lastUsedAt).toBe(baseMs + 5); + expect(cs.clients.get(p.client_id)!.lastUsedAt).toBeGreaterThan(beforeTs); + } finally { + vi.useRealTimers(); + } + }); +}); + +describe("consentHandler — deny", () => { + it("deny → 302 to nonce-bound redirect_uri with error=access_denied + state", () => { + const p = basePayload(); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const req = mockReq({ body: bodyFor(p, nonce, "deny") }); + const res = mockRes(); + + consentHandler(req as never, res as never); + + expect(res.redirect).toHaveBeenCalledTimes(1); + const target = new URL(res.redirect.mock.calls[0]![0] as string); + expect(target.hostname).toBe("legitimate.example"); + expect(target.searchParams.get("error")).toBe("access_denied"); + expect(target.searchParams.get("state")).toBe("state-abc"); + expect(target.searchParams.get("code")).toBeNull(); + }); + + it("deny redirects to NONCE-BOUND redirect_uri even when form body matches (positive proof)", () => { + // Sign a nonce naming the legitimate URI; submit a form body that + // ALSO names the legitimate URI (otherwise step 3 fires first). The + // assertion is on the hostname the handler actually redirects to, + // proving that the deny branch reads p.redirect_uri (not body). + const p = basePayload({ redirect_uri: "https://legitimate.example/cb" }); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const req = mockReq({ body: bodyFor(p, nonce, "deny") }); + const res = mockRes(); + + consentHandler(req as never, res as never); + + expect(res.redirect).toHaveBeenCalledTimes(1); + const target = new URL(res.redirect.mock.calls[0]![0] as string); + expect(target.hostname).toBe("legitimate.example"); + }); + + it("tampered form-body redirect_uri (different from nonce payload) → 400, NO redirect to tampered URI", () => { + // Negative proof: body.redirect_uri differs from payload.redirect_uri, + // step (3) fires before the decision branch is even reached, so the + // handler 400s and does NOT redirect anywhere — and crucially does + // not redirect to the attacker-controlled body URI. + const p = basePayload({ redirect_uri: "https://legitimate.example/cb" }); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const body = bodyFor(p, nonce, "deny"); + body.redirect_uri = "https://attacker.example/cb"; + const req = mockReq({ body }); + const res = mockRes(); + + consentHandler(req as never, res as never); + + expect(res.redirect).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(400); + }); +}); + +describe("consentHandler — field mismatch (step 3)", () => { + it("client_id swap in form body → 400 invalid_request field_mismatch", () => { + const p = basePayload(); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const body = bodyFor(p, nonce, "approve"); + body.client_id = "other"; + const req = mockReq({ body }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + expect(res.redirect).not.toHaveBeenCalled(); + }); + + it("state tampered → 400, no redirect (proves step 3 gates ALL flow incl. deny)", () => { + const p = basePayload(); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const body = bodyFor(p, nonce, "deny"); + body.state = "tampered"; + const req = mockReq({ body }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + expect(res.redirect).not.toHaveBeenCalled(); + }); +}); + +describe("consentHandler — nonce failures", () => { + it("forged MAC → 400 with reason hmac", () => { + const p = basePayload(); + registerKnownClient(p.client_id, p.redirect_uri); + const goodNonce = signConsentNonce(p, [KEY]); + // Replace MAC half with garbage of the same length. + const [payloadB64] = goodNonce.split("."); + const forged = `${payloadB64}.${Buffer.from("x".repeat(32)).toString("base64url")}`; + const req = mockReq({ body: bodyFor(p, forged, "approve") }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + const arg = res.status.mock.results[0]!.value.json.mock.calls[0]![0]; + expect(String(arg.error_description)).toContain("hmac"); + }); + + it("expired nonce → 400 with reason expired", () => { + const p = basePayload({ exp: Date.now() - 1 }); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const req = mockReq({ body: bodyFor(p, nonce, "approve") }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + const arg = res.status.mock.results[0]!.value.json.mock.calls[0]![0]; + expect(String(arg.error_description)).toContain("expired"); + }); + + it("malformed nonce token → 400 with reason format", () => { + const p = basePayload(); + registerKnownClient(p.client_id, p.redirect_uri); + const req = mockReq({ body: bodyFor(p, "not-a-token", "approve") }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + }); +}); + +describe("consentHandler — client + policy", () => { + it("unknown client_id (deleted between sign and consent) → 400 unauthorized_client + logs consent_stale_client", () => { + const warnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => undefined); + const p = basePayload({ client_id: "nonexistent" }); + const nonce = signConsentNonce(p, [KEY]); + const req = mockReq({ body: bodyFor(p, nonce, "approve") }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + const arg = res.status.mock.results[0]!.value.json.mock.calls[0]![0]; + expect(arg.error).toBe("unauthorized_client"); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringMatching(/^\[oauth\] consent_stale_client .*client_id=nonexistent/), + ); + warnSpy.mockRestore(); + }); + + it("scope mismatch (post-MAC) → 400 invalid_scope + logs consent_scope_mismatch", () => { + const warnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => undefined); + // Sign a payload whose scope is NOT the supported value. Step 3 + // requires the form body to equal the payload, so the body carries + // the same bogus scope — the mismatch surfaces at step 6a. + const p = basePayload({ scope: "bogus" }); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const req = mockReq({ body: bodyFor(p, nonce, "approve") }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + const arg = res.status.mock.results[0]!.value.json.mock.calls[0]![0]; + expect(arg.error).toBe("invalid_scope"); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringMatching(/^\[oauth\] consent_scope_mismatch .*scope=bogus/), + ); + warnSpy.mockRestore(); + }); + + it("unsupported response_type or code_challenge_method → 400 + logs consent_param_unsupported", () => { + const warnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => undefined); + const p = basePayload({ code_challenge_method: "plain" }); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const req = mockReq({ body: bodyFor(p, nonce, "approve") }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + const arg = res.status.mock.results[0]!.value.json.mock.calls[0]![0]; + expect(arg.error).toBe("invalid_request"); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringMatching( + /^\[oauth\] consent_param_unsupported .*code_challenge_method=plain/, + ), + ); + warnSpy.mockRestore(); + }); + + it("redirect_uri no longer in client's registered list → 400 invalid_redirect_uri", () => { + const p = basePayload(); + // Register client with a DIFFERENT redirect_uri so the nonce-bound + // URI isn't on the list. + registerKnownClient(p.client_id, "https://other.example/cb"); + const nonce = signConsentNonce(p, [KEY]); + const req = mockReq({ body: bodyFor(p, nonce, "approve") }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + const arg = res.status.mock.results[0]!.value.json.mock.calls[0]![0]; + expect(arg.error).toBe("invalid_redirect_uri"); + }); +}); + +describe("consentHandler — invalid decision", () => { + it("decision value neither approve nor deny → 400 + logs consent_unknown_decision", () => { + const warnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => undefined); + const p = basePayload(); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const req = mockReq({ body: bodyFor(p, nonce, "maybe") }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + expect(res.redirect).not.toHaveBeenCalled(); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringMatching(/^\[oauth\] consent_unknown_decision .*decision=maybe/), + ); + warnSpy.mockRestore(); + }); + + it("missing decision → 400", () => { + const p = basePayload(); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + const body = bodyFor(p, nonce, "approve") as Record; + delete body.decision; + const req = mockReq({ body }); + const res = mockRes(); + consentHandler(req as never, res as never); + expect(res.status).toHaveBeenCalledWith(400); + expect(res.redirect).not.toHaveBeenCalled(); + }); +}); + +describe("consentHandler — rate limit", () => { + it("exceeding consentLimiter (30/min) → 429 with Retry-After + logs rate_limited endpoint=consent", async () => { + const warnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => undefined); + const p = basePayload(); + registerKnownClient(p.client_id, p.redirect_uri); + const nonce = signConsentNonce(p, [KEY]); + + // Burn through the limiter from a single IP. + let last: ReturnType | undefined; + for (let i = 0; i < 31; i++) { + const req = mockReq({ + body: bodyFor(p, nonce, "approve"), + socket: { remoteAddress: "9.9.9.9" }, + }); + last = mockRes(); + consentHandler(req as never, last as never); + } + expect(last!.status).toHaveBeenCalledWith(429); + expect(last!.setHeader).toHaveBeenCalledWith( + "Retry-After", + expect.any(String), + ); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringMatching(/^\[oauth\] rate_limited .*endpoint=consent/), + ); + warnSpy.mockRestore(); + }); +}); diff --git a/src/__tests__/oauth-consent-template.test.ts b/src/__tests__/oauth-consent-template.test.ts new file mode 100644 index 0000000..56cb290 --- /dev/null +++ b/src/__tests__/oauth-consent-template.test.ts @@ -0,0 +1,122 @@ +import { describe, it, expect } from "vitest"; +import { renderConsentHtml } from "../oauth/consent-template.js"; + +const base = { + clientName: "Demo App", + clientId: "cid", + redirectUri: "https://app.example.com/cb", + redirectUriHostname: "app.example.com", + scope: "mcp", + state: "abc", + codeChallenge: "cc", + codeChallengeMethod: "S256", + responseType: "code", + resource: "", + nonce: "nonce.token", +}; + +describe("renderConsentHtml", () => { + it("escapes client_name HTML", () => { + const html = renderConsentHtml({ ...base, clientName: "" }); + expect(html).not.toContain(""); + expect(html).toContain("<script>"); + }); + + it("escapes redirect_uri attribute context", () => { + const html = renderConsentHtml({ ...base, redirectUri: 'https://a"b.example/' }); + expect(html).toContain('value="https://a"b.example/"'); + }); + + it("renders the hostname in bold, full uri in code", () => { + const html = renderConsentHtml(base); + expect(html).toMatch(/]*>app\.example\.com<\/strong>/); + expect(html).toMatch(/]*>https:\/\/app\.example\.com\/cb<\/code>/); + }); + + it("posts to /authorize/consent with the nonce + every bound field", () => { + const html = renderConsentHtml(base); + expect(html).toContain('action="/authorize/consent"'); + expect(html).toContain('name="nonce" value="nonce.token"'); + expect(html).toContain('name="client_id" value="cid"'); + expect(html).toContain('name="redirect_uri" value="https://app.example.com/cb"'); + expect(html).toContain('name="state" value="abc"'); + expect(html).toContain('name="code_challenge" value="cc"'); + expect(html).toContain('name="code_challenge_method" value="S256"'); + expect(html).toContain('name="response_type" value="code"'); + expect(html).toContain('name="scope" value="mcp"'); + }); + + it("includes resource hidden input even when empty", () => { + const html = renderConsentHtml(base); + expect(html).toContain('name="resource"'); + }); + + it("propagates a non-empty resource value (escaped)", () => { + const html = renderConsentHtml({ + ...base, + resource: 'https://api.example.com/">', + }); + expect(html).not.toContain('">'); + expect(html).toContain( + 'name="nonce" value=""><script>alert(1)</script>"', + ); + }); + + it("escapes hostname display element", () => { + const html = renderConsentHtml({ + ...base, + redirectUriHostname: "", + }); + expect(html).toMatch(/]*><img src=x><\/strong>/); + expect(html).not.toContain(""); + }); + + it("returns a self-contained HTML document with doctype", () => { + const html = renderConsentHtml(base); + expect(html.toLowerCase()).toContain(""); + expect(html).toContain(""); + }); + + it("uses POST method on the form", () => { + const html = renderConsentHtml(base); + expect(html).toMatch(/]*method="POST"/i); + }); + + it("does not include any script tags", () => { + const html = renderConsentHtml(base); + expect(html).not.toMatch(/]/i); + }); +}); diff --git a/src/oauth/consent-handler.ts b/src/oauth/consent-handler.ts new file mode 100644 index 0000000..ce534a0 --- /dev/null +++ b/src/oauth/consent-handler.ts @@ -0,0 +1,289 @@ +// POST /authorize/consent — the second half of the consent flow. +// +// The GET /authorize handler renders an HTML form (see consent-template.ts) +// whose hidden fields encode every OAuth parameter the user is being asked +// to approve, plus a server-signed nonce that binds those exact parameter +// values. This handler receives the form submission, re-validates the full +// bound set, and either mints an authorization code (approve) or redirects +// with `error=access_denied` (deny). +// +// SECURITY-CRITICAL INVARIANT +// ─────────────────────────── +// The final redirect URL is ALWAYS taken from the nonce-bound payload, +// NEVER from the form body. The form body must field-by-field equal the +// payload (step 3) — and step 3 is the very first re-check after MAC +// verification — but even with that gate in place we deliberately read +// `p.redirect_uri` rather than `body.redirect_uri` at the redirect call +// site. This belt-and-suspenders pattern is what makes the flow +// phishing-resistant: even a programming error in the equality loop would +// not let a tampered form body steer the user to an attacker URL. +// +// 8-step pipeline (each step a verification gate; first failure short- +// circuits with a 4xx): +// 1. Per-IP rate limit on the consent endpoint itself. +// 2. Verify the nonce HMAC (multi-key, rotation-tolerant) — MUST precede +// every other check because the payload is meaningless without an +// intact MAC. +// 3. Field-by-field equality between form body and nonce payload across +// the whole bound set. Mismatch ⇒ 400, generic message (we never +// leak which field mismatched). +// 4. Look up the client in the store. A client deleted between sign and +// consent ⇒ 400. +// 5. Re-validate the redirect_uri against the current policy AND +// re-check it's still in the client's registered list — policies may +// have tightened since the nonce was minted. +// 6. Re-confirm `scope`, `response_type`, `code_challenge_method` are +// what we still support. +// 7. Decision branch — approve ⇒ mint code; deny ⇒ access_denied. +// 8. Any other decision ⇒ 400. + +import type { Request, Response } from "express"; +import { getConfig } from "../config.js"; +import { clientStore, codeStore } from "./store.js"; +import { verifyConsentNonce } from "./consent-nonce.js"; +import { validateRedirectUri } from "./redirect-uri-policy.js"; +import { oauthClientIp } from "./trusted-client-ip.js"; +import { oauthLog } from "./observability.js"; +import { consentLimiter, type OAuthRateLimiter } from "./rate-limiter.js"; + +// Code TTL matches the existing authorize-handler grant lifetime; the +// consent flow does not change the code's lifecycle, only the user- +// approval step that precedes it. +const CODE_TTL_MS = 600_000; +const TOKEN_SCOPE = "mcp"; + +function enforce( + limiter: OAuthRateLimiter, + req: Request, + res: Response, +): boolean { + const ip = oauthClientIp(req); + const r = limiter.check(ip); + if (!r.ok) { + res.setHeader("Retry-After", String(r.retryAfterSec ?? 60)); + res.status(429).json({ + error: "rate_limited", + error_description: "Too many requests — slow down.", + }); + oauthLog.consentRateLimited({ ip }); + return false; + } + return true; +} + +// Fields that must be byte-equal between the form body and the nonce +// payload. `exp` and the nonce token itself are NOT in this list — `exp` +// is internal to the MAC, and the nonce token is what gets verified +// rather than compared against itself. +const BOUND_FIELDS = [ + "client_id", + "redirect_uri", + "state", + "code_challenge", + "code_challenge_method", + "response_type", + "scope", + "resource", +] as const; + +export function consentHandler(req: Request, res: Response): void { + // (1) Rate-limit per IP. + if (!enforce(consentLimiter, req, res)) return; + + const ip = oauthClientIp(req); + const body = (req.body ?? {}) as Record; + + // (2) Verify nonce — MAC first, then expiry (the verifier handles the + // ordering correctly so we don't have to disambiguate here). + const nonceToken = body.nonce ?? ""; + const v = verifyConsentNonce(nonceToken, getConfig().oauthConsentHmacKeys); + if (!v.ok) { + oauthLog.consentNonceInvalid({ reason: v.reason, ip }); + res.status(400).json({ + error: "invalid_request", + error_description: `consent nonce: ${v.reason}`, + }); + return; + } + const p = v.payload; + + // (3) Field-by-field equality. Generic 400 — do NOT leak which field + // mismatched, because that would help an attacker iterate on a forged + // submission. The internal log line carries the field name for + // operators. + for (const k of BOUND_FIELDS) { + const formVal = body[k] ?? ""; + // BOUND_FIELDS is a tuple of string-valued payload keys (the only + // payload field NOT in the tuple is the numeric `exp`), so the + // string-narrow is safe by construction. + const payloadVal = (p as unknown as Record)[k] ?? ""; + if (formVal !== payloadVal) { + oauthLog.consentNonceInvalid({ + reason: "field_mismatch", + ip, + field: k, + }); + res.status(400).json({ + error: "invalid_request", + error_description: "consent parameters do not match.", + }); + return; + } + } + + // (4) Look up the client. A client may have been evicted (lazy sweep) + // or explicitly deleted in the window between consent-screen render + // and form submit. + const client = clientStore.get(p.client_id); + if (!client) { + oauthLog.consentStaleClient({ ip, client_id: p.client_id }); + res.status(400).json({ + error: "unauthorized_client", + error_description: "Unknown client_id.", + }); + return; + } + + // (5a) Re-validate against the redirect_uri policy. Defense in depth: + // policy may have tightened since the nonce was minted (e.g. operator + // pushed a stricter ruleset between mint and redeem). + const policy = validateRedirectUri(p.redirect_uri); + if (!policy.ok) { + oauthLog.registerRejected({ + reason: policy.reason, + ip, + client_id: p.client_id, + }); + res.status(400).json({ + error: "invalid_redirect_uri", + error_description: `redirect_uri rejected: ${policy.reason}`, + }); + return; + } + + // (5b) Exact-match against the client's registered list. Distinct + // log line from (5a) so operators can disambiguate policy-evasion + // from list-tampering. + if ( + client.redirect_uris.length > 0 && + !client.redirect_uris.includes(p.redirect_uri) + ) { + oauthLog.registerRejected({ + reason: "not_in_list", + ip, + client_id: p.client_id, + }); + res.status(400).json({ + error: "invalid_redirect_uri", + error_description: "redirect_uri not in registered list.", + }); + return; + } + + // (6a) Scope re-check — split from response_type/PKCE check so the + // 400 shapes are distinct (invalid_scope vs invalid_request) and + // operators can grep them separately. + if (p.scope !== TOKEN_SCOPE) { + oauthLog.consentScopeMismatch({ + ip, + client_id: p.client_id, + scope: p.scope, + }); + res.status(400).json({ + error: "invalid_scope", + error_description: `scope must be ${TOKEN_SCOPE}.`, + }); + return; + } + + // (6b) response_type + code_challenge_method re-check. + if (p.response_type !== "code" || p.code_challenge_method !== "S256") { + oauthLog.consentParamUnsupported({ + ip, + client_id: p.client_id, + response_type: p.response_type, + code_challenge_method: p.code_challenge_method, + }); + res.status(400).json({ + error: "invalid_request", + error_description: + "response_type or code_challenge_method unsupported.", + }); + return; + } + + const decision = body.decision; + + // (7) Deny path. The redirect URL is built from `p.redirect_uri` — + // the nonce-bound value — NOT from `body.redirect_uri`. Step (3) + // already proved they're byte-equal, but we still read from the + // payload as a belt-and-suspenders guarantee: even a programming + // error in step (3) would not let a tampered body URI win here. + if (decision === "deny") { + oauthLog.consentDenied({ client_id: p.client_id, ip }); + let url: URL; + try { + url = new URL(p.redirect_uri); + } catch { + oauthLog.consentRedirectUriUnparseable({ + ip, + client_id: p.client_id, + }); + res.status(400).json({ + error: "invalid_request", + error_description: "redirect_uri unparseable", + }); + return; + } + url.searchParams.set("error", "access_denied"); + url.searchParams.set("error_description", "user_denied_consent"); + if (p.state) url.searchParams.set("state", p.state); + res.redirect(url.toString()); + return; + } + + // (8) Approve path — or anything that isn't `deny` and isn't + // `approve` falls through to a 400 below. + if (decision !== "approve") { + oauthLog.consentUnknownDecision({ + ip, + client_id: p.client_id, + decision: String(body.decision), + }); + res.status(400).json({ + error: "invalid_request", + error_description: "unknown decision.", + }); + return; + } + + // Approve: touch the client (liveness signal for TTL eviction), + // mint an auth code bound to the nonce's PKCE challenge + resource, + // and redirect to the nonce-bound URI. + let url: URL; + try { + url = new URL(p.redirect_uri); + } catch { + oauthLog.consentRedirectUriUnparseable({ + ip, + client_id: p.client_id, + }); + res.status(400).json({ + error: "invalid_request", + error_description: "redirect_uri unparseable", + }); + return; + } + clientStore.touch(p.client_id); + const { code } = codeStore.issue({ + clientId: p.client_id, + codeChallenge: p.code_challenge, + redirectUri: p.redirect_uri, + resource: p.resource || undefined, + ttlMs: CODE_TTL_MS, + }); + url.searchParams.set("code", code); + if (p.state) url.searchParams.set("state", p.state); + oauthLog.consentApproved({ client_id: p.client_id, ip }); + res.redirect(url.toString()); +} diff --git a/src/oauth/consent-template.ts b/src/oauth/consent-template.ts new file mode 100644 index 0000000..645ebe3 --- /dev/null +++ b/src/oauth/consent-template.ts @@ -0,0 +1,121 @@ +// src/oauth/consent-template.ts +// +// Server-rendered consent screen for the OAuth `/authorize` flow. +// +// Design constraints (spec §1): +// * Pure function — no Express/req/res imports, no I/O. +// * Single self-contained HTML document, inline CSS only, NO `" }); - expect(html).not.toContain(""); - expect(html).toContain("<script>"); + it("escapes client_name HTML", () => { + const html = renderConsentHtml({ + ...base, + clientName: "", }); - - it("escapes redirect_uri attribute context", () => { - const html = renderConsentHtml({ ...base, redirectUri: 'https://a"b.example/' }); - expect(html).toContain('value="https://a"b.example/"'); - }); - - it("renders the hostname in bold, full uri in code", () => { - const html = renderConsentHtml(base); - expect(html).toMatch(/]*>app\.example\.com<\/strong>/); - expect(html).toMatch(/]*>https:\/\/app\.example\.com\/cb<\/code>/); - }); - - it("posts to /authorize/consent with the nonce + every bound field", () => { - const html = renderConsentHtml(base); - expect(html).toContain('action="/authorize/consent"'); - expect(html).toContain('name="nonce" value="nonce.token"'); - expect(html).toContain('name="client_id" value="cid"'); - expect(html).toContain('name="redirect_uri" value="https://app.example.com/cb"'); - expect(html).toContain('name="state" value="abc"'); - expect(html).toContain('name="code_challenge" value="cc"'); - expect(html).toContain('name="code_challenge_method" value="S256"'); - expect(html).toContain('name="response_type" value="code"'); - expect(html).toContain('name="scope" value="mcp"'); - }); - - it("includes resource hidden input even when empty", () => { - const html = renderConsentHtml(base); - expect(html).toContain('name="resource"'); - }); - - it("propagates a non-empty resource value (escaped)", () => { - const html = renderConsentHtml({ - ...base, - resource: 'https://api.example.com/">', - }); - expect(html).not.toContain('">'); - expect(html).toContain( - 'name="nonce" value=""><script>alert(1)</script>"', - ); - }); - - it("escapes hostname display element", () => { - const html = renderConsentHtml({ - ...base, - redirectUriHostname: "", - }); - expect(html).toMatch(/]*><img src=x><\/strong>/); - expect(html).not.toContain(""); + expect(html).not.toContain(""); + expect(html).toContain("<script>"); + }); + + it("escapes redirect_uri attribute context", () => { + const html = renderConsentHtml({ + ...base, + redirectUri: 'https://a"b.example/', }); - - it("returns a self-contained HTML document with doctype", () => { - const html = renderConsentHtml(base); - expect(html.toLowerCase()).toContain(""); - expect(html).toContain(""); + expect(html).toContain('value="https://a"b.example/"'); + }); + + it("renders the hostname in bold, full uri in code", () => { + const html = renderConsentHtml(base); + expect(html).toMatch(/]*>app\.example\.com<\/strong>/); + expect(html).toMatch(/]*>https:\/\/app\.example\.com\/cb<\/code>/); + }); + + it("posts to /authorize/consent with the nonce + every bound field", () => { + const html = renderConsentHtml(base); + expect(html).toContain('action="/authorize/consent"'); + expect(html).toContain('name="nonce" value="nonce.token"'); + expect(html).toContain('name="client_id" value="cid"'); + expect(html).toContain( + 'name="redirect_uri" value="https://app.example.com/cb"', + ); + expect(html).toContain('name="state" value="abc"'); + expect(html).toContain('name="code_challenge" value="cc"'); + expect(html).toContain('name="code_challenge_method" value="S256"'); + expect(html).toContain('name="response_type" value="code"'); + expect(html).toContain('name="scope" value="mcp"'); + }); + + it("includes resource hidden input even when empty", () => { + const html = renderConsentHtml(base); + expect(html).toContain('name="resource"'); + }); + + it("propagates a non-empty resource value (escaped)", () => { + const html = renderConsentHtml({ + ...base, + resource: 'https://api.example.com/">', }); - - it("does not include any script tags", () => { - const html = renderConsentHtml(base); - expect(html).not.toMatch(/]/i); + expect(html).not.toContain('">'); + expect(html).toContain( + 'name="nonce" value=""><script>alert(1)</script>"', + ); + }); + + it("escapes hostname display element", () => { + const html = renderConsentHtml({ + ...base, + redirectUriHostname: "", }); + expect(html).toMatch(/]*><img src=x><\/strong>/); + expect(html).not.toContain(""); + }); + + it("returns a self-contained HTML document with doctype", () => { + const html = renderConsentHtml(base); + expect(html.toLowerCase()).toContain(""); + expect(html).toContain(""); + }); + + it("uses POST method on the form", () => { + const html = renderConsentHtml(base); + expect(html).toMatch(/]*method="POST"/i); + }); + + it("does not include any script tags", () => { + const html = renderConsentHtml(base); + expect(html).not.toMatch(/]/i); + }); }); diff --git a/src/__tests__/oauth-e2e.test.ts b/src/__tests__/oauth-e2e.test.ts index e5c8057..3331147 100644 --- a/src/__tests__/oauth-e2e.test.ts +++ b/src/__tests__/oauth-e2e.test.ts @@ -213,7 +213,9 @@ describe("OAuth 2.1 end-to-end ceremonial flow", () => { const html = await authRes.text(); // Sanity-check shape — the consent form must include the hidden bound // set and the POST action our handler expects. - expect(html).toMatch(/