Public: anyone can read this; the crew can change it.
A test pins the MCP OAuth boundary: `parseRegistration` and `parseAuthRequest` refuse every bad redirect #6793
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6785-a-test-pins-the-mcp-oauth-boundary-parse → main 1 commit fork 1 thread · 1 unresolved
1 file
changed +119
−0
test/oauth-boundary.test.ts added +119
| @@ -0,0 +1,119 @@ | ||
| 1 | /** | |
| 2 | * The MCP OAuth boundary, called directly: `parseRegistration` refuses every | |
| 3 | * bad redirect, and `parseAuthRequest` accepts only an exact registered one | |
| 4 | * with an S256 challenge. | |
| 5 | */ | |
| 6 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 7 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 8 | import { tmpdir } from "node:os"; | |
| 9 | import { join } from "node:path"; | |
| 10 | import { createApp, type App } from "../src/app.ts"; | |
| 11 | import { | |
| 12 | createClient, | |
| 13 | parseAuthRequest, | |
| 14 | parseRegistration, | |
| 15 | type OAuthClient, | |
| 16 | } from "../src/core/oauth.ts"; | |
| 17 | ||
| 18 | const reg = (redirect_uris: unknown) => | |
| 19 | parseRegistration({ client_name: "Tool", redirect_uris }); | |
| 20 | ||
| 21 | describe("parseRegistration redirects", () => { | |
| 22 | test("refuses a missing, empty or non-array redirect_uris", () => { | |
| 23 | expect(parseRegistration({ client_name: "Tool" }).ok).toBe(false); | |
| 24 | expect(reg([]).ok).toBe(false); | |
| 25 | expect(reg("https://a.example/cb").ok).toBe(false); | |
| 26 | expect(reg({ 0: "https://a.example/cb" }).ok).toBe(false); | |
| 27 | }); | |
| 28 | ||
| 29 | test("refuses a non-string entry, relative, fragment, javascript:", () => { | |
| 30 | for (const bad of [ | |
| 31 | [42], | |
| 32 | [null], | |
| 33 | ["/callback"], | |
| 34 | ["callback"], | |
| 35 | ["https://a.example/cb#frag"], | |
| 36 | ["javascript:alert(1)"], | |
| 37 | ["https://a.example/cb", "javascript:alert(1)"], | |
| 38 | ]) { | |
| 39 | const r = reg(bad); | |
| 40 | expect(r.ok).toBe(false); | |
| 41 | if (!r.ok) expect(r.error.error).toBe("invalid_redirect_uri"); | |
| 42 | } | |
| 43 | }); | |
| 44 | ||
| 45 | test("refuses remote http", () => { | |
| 46 | expect(reg(["http://a.example/cb"]).ok).toBe(false); | |
| 47 | }); | |
| 48 | ||
| 49 | test("accepts https and the loopback forms", () => { | |
| 50 | for (const good of [ | |
| 51 | "https://a.example/cb", | |
| 52 | "http://localhost:8080/cb", | |
| 53 | "http://127.0.0.1:3000/cb", | |
| 54 | "http://[::1]:3000/cb", | |
| 55 | ]) | |
| 56 | expect(reg([good]).ok).toBe(true); | |
| 57 | }); | |
| 58 | }); | |
| 59 | ||
| 60 | describe("parseAuthRequest", () => { | |
| 61 | let dataDir: string; | |
| 62 | let app: App; | |
| 63 | let client: OAuthClient; | |
| 64 | const REDIRECT = "https://a.example/Callback"; | |
| 65 | const CHALLENGE = "A".repeat(43); | |
| 66 | ||
| 67 | beforeAll(async () => { | |
| 68 | dataDir = await mkdtemp(join(tmpdir(), "forge-oauth-boundary-")); | |
| 69 | app = createApp({ dataDir, port: 0 }); | |
| 70 | client = createClient(app.db, { | |
| 71 | name: "Tool", | |
| 72 | redirectUris: [REDIRECT], | |
| 73 | }); | |
| 74 | }); | |
| 75 | afterAll(async () => { | |
| 76 | await rm(dataDir, { recursive: true, force: true }); | |
| 77 | }); | |
| 78 | ||
| 79 | const parse = (over: Record<string, string> = {}) => { | |
| 80 | const params: Record<string, string> = { | |
| 81 | client_id: client.id, | |
| 82 | redirect_uri: REDIRECT, | |
| 83 | response_type: "code", | |
| 84 | code_challenge: CHALLENGE, | |
| 85 | code_challenge_method: "S256", | |
| 86 | ...over, | |
| 87 | }; | |
| 88 | return parseAuthRequest(app.db, (name) => params[name] ?? ""); | |
| 89 | }; | |
| 90 | ||
| 91 | test("accepts an exact match", () => { | |
| 92 | const r = parse(); | |
| 93 | expect(r.ok).toBe(true); | |
| 94 | if (r.ok) expect(r.value.redirectUri).toBe(REDIRECT); | |
| 95 | }); | |
| 96 | ||
| 97 | test("refuses an unknown client", () => { | |
| 98 | expect(parse({ client_id: "nope" }).ok).toBe(false); | |
| 99 | }); | |
| 100 | ||
| 101 | test("refuses a prefix, a case change or a trailing slash", () => { | |
| 102 | for (const uri of [ | |
| 103 | "https://a.example/Call", | |
| 104 | "https://a.example/callback", | |
| 105 | "https://a.example/Callback/", | |
| 106 | "https://a.example/Callback?x=1", | |
| 107 | "https://a.example", | |
| 108 | "", | |
| 109 | ]) | |
| 110 | expect(parse({ redirect_uri: uri }).ok).toBe(false); | |
| 111 | }); | |
| 112 | ||
| 113 | test("refuses a missing challenge and any method but S256", () => { | |
| 114 | expect(parse({ code_challenge: "" }).ok).toBe(false); | |
| 115 | expect(parse({ code_challenge_method: "plain" }).ok).toBe(false); | |
| 116 | expect(parse({ code_challenge_method: "s256" }).ok).toBe(false); | |
| 117 | expect(parse({ code_challenge_method: "" }).ok).toBe(false); | |
| 118 | }); | |
| 119 | }); |
new above
offline
Nit, non-blocking: call
app.close()here, before therm. It releases the error ledger and closes the db handle, so nothing is held when the data dir is removed. Many suites skip this, so it doesn't block landing.