Skip to content

Commit e29c360

Browse files
RhysSullivanRhysSullivan
authored
Honor OAuth resource metadata challenges (#1981)
1 parent cc0fd8f commit e29c360

5 files changed

Lines changed: 346 additions & 171 deletions

File tree

‎e2e/selfhost/mcp-oauth-resource-challenge.test.ts‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import { randomBytes } from "node:crypto";
2+
3+
import { expect } from "@effect/vitest";
4+
import { Effect } from "effect";
5+
import { composePluginApi } from "@executor-js/api/server";
6+
import { connectEmulator } from "@executor-js/emulate";
7+
import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api";
8+
import { IntegrationSlug } from "@executor-js/sdk/shared";
9+
10+
import { createEmulatorInstance } from "../src/emulator-instance";
11+
import { scenario } from "../src/scenario";
12+
import { Api, Browser, Target } from "../src/services";
13+
import { visit } from "../src/surfaces/browser";
14+
15+
const api = composePluginApi([mcpHttpPlugin()] as const);
16+
17+
scenario(
18+
"MCP OAuth · the endpoint challenge selects the resource used by browser authorization",
19+
{ timeout: 180_000 },
20+
Effect.scoped(
21+
Effect.gen(function* () {
22+
const target = yield* Target;
23+
const browser = yield* Browser;
24+
const { client: makeApiClient } = yield* Api;
25+
const identity = yield* target.newIdentity();
26+
const client = yield* makeApiClient(api, identity);
27+
const baseUrl = yield* createEmulatorInstance("mcp", "resource-challenge");
28+
const emulator = yield* Effect.promise(() => connectEmulator({ baseUrl, service: "mcp" }));
29+
const slug = IntegrationSlug.make(`resource_challenge_${randomBytes(4).toString("hex")}`);
30+
31+
// The published emulator advertises root metadata in its Bearer challenge,
32+
// while its path-scoped document describes a different resource (/mcp).
33+
const probe = yield* client.oauth.probe({ payload: { url: `${baseUrl}/mcp` } });
34+
expect(probe.resource).toBe(baseUrl);
35+
36+
yield* client.mcp.addServer({
37+
payload: {
38+
transport: "remote",
39+
name: "Resource challenge MCP",
40+
endpoint: `${baseUrl}/mcp`,
41+
slug,
42+
authenticationTemplate: [{ kind: "oauth2" }],
43+
},
44+
});
45+
yield* Effect.addFinalizer(() =>
46+
client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore),
47+
);
48+
yield* browser.session(identity, async ({ page, step }) => {
49+
await step("Open the protected integration", async () => {
50+
await visit(page, `/integrations/${slug}`);
51+
await page.getByRole("button", { name: "Add connection" }).waitFor();
52+
});
53+
await step("Connect using the resource advertised by the server", async () => {
54+
await page.getByRole("button", { name: "Add connection" }).click();
55+
const popupPromise = page.waitForEvent("popup", { timeout: 30_000 });
56+
await page.getByRole("button", { name: "Connect", exact: true }).click();
57+
const popup = await popupPromise;
58+
await popup.waitForURL((url) => url.pathname.endsWith("/authorize"), { timeout: 60_000 });
59+
expect(new URL(popup.url()).searchParams.get("resource")).toBe(baseUrl);
60+
// The hosted emulator's button omits its required login field.
61+
// Authorize the synthetic account through its HTTP form contract.
62+
const authorization = new URL(popup.url());
63+
const approved = await popup.request.post(`${baseUrl}/authorize/approve`, {
64+
form: { ...Object.fromEntries(authorization.searchParams), login: "admin" },
65+
maxRedirects: 0,
66+
});
67+
expect(approved.status()).toBe(302);
68+
const callback = approved.headers()["location"];
69+
if (!callback) throw new Error("The emulator did not return an OAuth callback");
70+
await popup.goto(callback);
71+
await popup.getByRole("heading", { name: "Connected" }).waitFor({ timeout: 30_000 });
72+
});
73+
});
74+
const clients = yield* client.oauth.listClients();
75+
expect(clients.some((app) => app.resource === baseUrl)).toBe(true);
76+
const ledger = yield* Effect.promise(() => emulator.ledger.list());
77+
expect(ledger.some((entry) => entry.path === "/token" && entry.response.status === 200)).toBe(
78+
true,
79+
);
80+
}),
81+
),
82+
);

‎packages/core/sdk/src/insufficient-scope.ts‎

Lines changed: 2 additions & 166 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
//
2525
// A miss is benign: the failure stays on the existing classification.
2626

27+
import { parseChallenges } from "./www-authenticate";
28+
2729
export type InsufficientScopeDetection = {
2830
/** Scopes the upstream named as required, when it named any (RFC 6750's
2931
* `scope` attribute). Empty when the provider only signalled the class of
@@ -36,172 +38,6 @@ const MAX_DEPTH = 8;
3638
const isRecord = (value: unknown): value is Record<string, unknown> =>
3739
typeof value === "object" && value !== null && !Array.isArray(value);
3840

39-
/** Parser for the whole WWW-Authenticate header per RFC 7235 §2.1: a
40-
* comma-separated #list of challenges, each
41-
* `scheme [ 1*SP ( token68 / #auth-param ) ]`. Implemented as an explicit
42-
* per-challenge state machine so params can never attach across challenge
43-
* boundaries or to a token68 credential:
44-
*
45-
* - "scheme": just read a scheme; accepts a token68 OR a first auth-param
46-
* (space-separated, no comma).
47-
* - "params": accepts further auth-params ONLY after a comma.
48-
* - "token68": accepts nothing; any trailing param is malformed.
49-
*
50-
* Auth-params allow BWS around `=` (RFC 7230). Quoted-strings consume
51-
* quoted-pairs whole and must end at a separator. ANY malformed shape —
52-
* scheme-less params, space-separated param runs, params after token68,
53-
* stray quotes/bytes — returns null and never classifies: a miss is benign,
54-
* a false positive strips a valid recovery path. */
55-
type Challenge = { readonly scheme: string; readonly params: Map<string, string> };
56-
57-
// HTTP `token` alphabet (RFC 7230 §3.2.6) — schemes and auth-param names.
58-
const TOKEN_RE = /[A-Za-z0-9!#$%&'*+.^_`|~-]/;
59-
// token68 alphabet (RFC 7235 §2.1), padding `=` handled separately.
60-
const TOKEN68_RE = /[A-Za-z0-9._~+/-]/;
61-
// Superset used by the word reader; each use site validates against the
62-
// context-specific alphabet after reading.
63-
const WORD_RE = /[A-Za-z0-9!#$%&'*+.^_`|~/-]/;
64-
65-
const isToken = (word: string): boolean => [...word].every((ch) => TOKEN_RE.test(ch));
66-
// Unquoted URL values some providers emit (scheme://host/path?query): URI
67-
// characters per RFC 3986, no whitespace/comma/quotes.
68-
const isUrlish = (word: string): boolean => /^[A-Za-z][A-Za-z0-9+.-]*:\/\/[^\s,"]+$/.test(word);
69-
const isToken68 = (word: string): boolean => [...word].every((ch) => TOKEN68_RE.test(ch));
70-
71-
const parseChallenges = (header: string): readonly Challenge[] | null => {
72-
const len = header.length;
73-
const challenges: Challenge[] = [];
74-
let current: Challenge | null = null;
75-
let state: "boundary" | "scheme" | "token68" | "params" = "boundary";
76-
let sawComma = true; // header start counts as a list boundary
77-
let i = 0;
78-
79-
const readWord = (): string => {
80-
const start = i;
81-
while (i < len && WORD_RE.test(header[i]!)) i += 1;
82-
return header.slice(start, i);
83-
};
84-
// Returns null on an unterminated quote or a quote run into the next token.
85-
const readQuoted = (): string | null => {
86-
let value = "";
87-
i += 1; // opening quote
88-
while (i < len) {
89-
const ch = header[i]!;
90-
if (ch === '"') {
91-
i += 1;
92-
return i >= len || /[\s,]/.test(header[i]!) ? value : null;
93-
}
94-
if (ch === "\\" && i + 1 < len) {
95-
value += header[i + 1];
96-
i += 2;
97-
continue;
98-
}
99-
value += ch;
100-
i += 1;
101-
}
102-
return null; // unterminated
103-
};
104-
105-
while (i < len) {
106-
while (i < len && /\s/.test(header[i]!)) i += 1;
107-
if (i >= len) break;
108-
if (header[i] === ",") {
109-
sawComma = true;
110-
i += 1;
111-
continue;
112-
}
113-
if (!WORD_RE.test(header[i]!)) return null; // stray quote/byte: malformed
114-
const word = readWord();
115-
// Look ahead through BWS for `=` to classify the word.
116-
let j = i;
117-
while (j < len && /[ \t]/.test(header[j]!)) j += 1;
118-
const isPaddingRun = (() => {
119-
// An `=`-run directly on the word (no BWS) that is followed (after
120-
// optional whitespace) by a comma or the end of input is token68
121-
// padding. An `=` followed by a value — even across BWS — is an
122-
// auth-param (RFC 7230 allows BWS around `=`).
123-
if (header[i] !== "=") return false;
124-
let k = i;
125-
while (k < len && header[k] === "=") k += 1;
126-
while (k < len && /[ \t]/.test(header[k]!)) k += 1;
127-
return k >= len || header[k] === ",";
128-
})();
129-
130-
if (isPaddingRun) {
131-
// token68 with padding — only legal directly after a scheme.
132-
if (state !== "scheme" || sawComma) return null;
133-
if (!isToken68(word)) return null;
134-
while (i < len && header[i] === "=") i += 1;
135-
state = "token68";
136-
sawComma = false;
137-
continue;
138-
}
139-
140-
if (header[j] === "=") {
141-
// auth-param: `word BWS = BWS value`.
142-
if (!isToken(word)) return null; // param name must be an HTTP token
143-
if (current === null) return null; // scheme-less param
144-
if (state === "token68") return null; // params after token68
145-
if (state === "scheme" && sawComma) return null; // "Bearer, a=b"
146-
if (state === "params" && !sawComma) return null; // space-separated run
147-
i = j + 1;
148-
while (i < len && /[ \t]/.test(header[i]!)) i += 1;
149-
let value: string;
150-
if (header[i] === '"') {
151-
const quoted = readQuoted();
152-
if (quoted === null) return null;
153-
value = quoted;
154-
} else {
155-
const start = i;
156-
while (i < len && !/[\s,]/.test(header[i]!)) i += 1;
157-
value = header.slice(start, i);
158-
// An unquoted value must be an HTTP token (`realm =,` / `realm=;`
159-
// are malformed) — EXCEPT that real providers emit unquoted URLs for
160-
// resource_metadata (observed live: Stripe), so URL-safe characters
161-
// are tolerated there. The signal params (`error`, `scope`) stay
162-
// token-strict.
163-
if (value.length === 0) return null;
164-
const lowerName = word.toLowerCase();
165-
if (!isToken(value) && !(lowerName === "resource_metadata" && isUrlish(value))) {
166-
return null;
167-
}
168-
}
169-
// Duplicate SIGNAL params (`error`, `scope`) within one challenge mean
170-
// a header playing games — never classify. Other duplicates are
171-
// tolerated first-wins: real providers emit them (observed live:
172-
// Sentry duplicates resource_metadata).
173-
const key = word.toLowerCase();
174-
if (current.params.has(key)) {
175-
if (key === "error" || key === "scope") return null;
176-
} else {
177-
current.params.set(key, value);
178-
}
179-
state = "params";
180-
sawComma = false;
181-
continue;
182-
}
183-
184-
// Bare word: a new challenge's scheme at a list boundary, a token68
185-
// directly after a scheme, malformed anywhere else.
186-
if (sawComma) {
187-
if (!isToken(word)) return null; // a scheme must be an HTTP token
188-
current = { scheme: word.toLowerCase(), params: new Map() };
189-
challenges.push(current);
190-
state = "scheme";
191-
sawComma = false;
192-
continue;
193-
}
194-
if (state === "scheme") {
195-
if (!isToken68(word)) return null;
196-
state = "token68";
197-
continue;
198-
}
199-
return null;
200-
}
201-
202-
return challenges;
203-
};
204-
20541
const detectFromChallenge = (header: string): InsufficientScopeDetection | null => {
20642
const challenges = parseChallenges(header);
20743
if (challenges === null) return null;

‎packages/core/sdk/src/oauth-discovery.ts‎

Lines changed: 71 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import {
2929
createPkceCodeVerifier,
3030
type OAuthEndpointUrlPolicy,
3131
} from "./oauth-helpers";
32+
import { parseChallenges } from "./www-authenticate";
3233

3334
// ---------------------------------------------------------------------------
3435
// Errors
@@ -250,8 +251,8 @@ const executeText = (
250251
// ---------------------------------------------------------------------------
251252
// RFC 9728 — Protected Resource Metadata
252253
//
253-
// Not covered by `oauth4webapi`. Hand-rolled probe: try the path-scoped
254-
// well-known first, then the origin-scoped fallback.
254+
// Follow the protected endpoint's advertised metadata URL before trying
255+
// path-scoped and origin-scoped well-known locations (RFC 9728 section 5).
255256
// ---------------------------------------------------------------------------
256257

257258
const buildResourceMetadataUrls = (resourceUrl: string): string[] => {
@@ -278,6 +279,61 @@ const withResourceQueryParams = (
278279
return parsed.toString();
279280
};
280281

282+
const discoverResourceMetadataChallenge = (
283+
resourceUrl: string,
284+
options: DiscoveryRequestOptions,
285+
): Effect.Effect<string | null, OAuthDiscoveryError> =>
286+
provideHttpClient(
287+
Effect.gen(function* () {
288+
yield* validateEndpointUrl(resourceUrl, "resource", options.endpointUrlPolicy);
289+
let request = HttpClientRequest.get(
290+
withResourceQueryParams(resourceUrl, options.resourceQueryParams),
291+
).pipe(HttpClientRequest.setHeader("accept", "application/json"));
292+
for (const [name, value] of Object.entries(options.resourceHeaders ?? {})) {
293+
request = HttpClientRequest.setHeader(request, name, value);
294+
}
295+
if (options.mcpProtocolVersion) {
296+
request = HttpClientRequest.setHeader(
297+
request,
298+
MCP_PROTOCOL_VERSION_HEADER,
299+
options.mcpProtocolVersion,
300+
);
301+
}
302+
const client = yield* HttpClient.HttpClient;
303+
// Read headers only: an MCP GET can open a long-lived event stream.
304+
const response = yield* HttpClient.withScope(client)
305+
.execute(request)
306+
.pipe(
307+
Effect.timeout(Duration.millis(options.timeoutMs ?? OAUTH2_DEFAULT_TIMEOUT_MS)),
308+
Effect.mapError(
309+
(cause) =>
310+
new OAuthDiscoveryError({
311+
message: "Failed to discover the protected resource authentication challenge",
312+
cause,
313+
}),
314+
),
315+
);
316+
if (response.status !== 401 && response.status !== 403) return null;
317+
const header = response.headers["www-authenticate"];
318+
if (header === undefined) return null;
319+
const challenges = parseChallenges(header);
320+
if (challenges === null) return null;
321+
for (const challenge of challenges) {
322+
if (challenge.scheme !== "bearer") continue;
323+
const metadataUrl = challenge.params.get("resource_metadata");
324+
if (metadataUrl === undefined) continue;
325+
return yield* validateEndpointUrl(
326+
metadataUrl,
327+
"resource_metadata",
328+
options.endpointUrlPolicy,
329+
);
330+
}
331+
return null;
332+
}).pipe(Effect.scoped),
333+
options,
334+
);
335+
336+
/** Discover RFC 9728 metadata, preferring an explicit Bearer challenge URL. */
281337
export const discoverProtectedResourceMetadata = (
282338
resourceUrl: string,
283339
options: DiscoveryRequestOptions = {},
@@ -286,12 +342,22 @@ export const discoverProtectedResourceMetadata = (
286342
OAuthDiscoveryError
287343
> =>
288344
Effect.gen(function* () {
289-
for (const url of buildResourceMetadataUrls(resourceUrl)) {
290-
const requestUrl = withResourceQueryParams(url, options.resourceQueryParams);
345+
const advertisedUrl = yield* discoverResourceMetadataChallenge(resourceUrl, options);
346+
const metadataUrls =
347+
advertisedUrl === null ? buildResourceMetadataUrls(resourceUrl) : [advertisedUrl];
348+
for (const url of metadataUrls) {
349+
// A challenge may name another origin. Never forward resource credentials there.
350+
const sameOrigin = new URL(url).origin === new URL(resourceUrl).origin;
351+
const requestUrl = withResourceQueryParams(
352+
url,
353+
sameOrigin ? options.resourceQueryParams : undefined,
354+
);
291355
let request = HttpClientRequest.get(requestUrl).pipe(
292356
HttpClientRequest.setHeader("accept", "application/json"),
293357
);
294-
for (const [name, value] of Object.entries(options.resourceHeaders ?? {})) {
358+
for (const [name, value] of Object.entries(
359+
sameOrigin ? (options.resourceHeaders ?? {}) : {},
360+
)) {
295361
request = HttpClientRequest.setHeader(request, name, value);
296362
}
297363
if (options.mcpProtocolVersion) {

‎packages/core/sdk/src/www-authenticate.test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import { describe, expect, it } from "@effect/vitest";
2+
3+
import { parseChallenges } from "./www-authenticate";
4+
5+
describe("authentication challenge metadata", () => {
6+
it("keeps a Bearer metadata URL separate from other schemes and quoted text", () => {
7+
const challenges = parseChallenges(
8+
'Basic realm="resource_metadata=wrong", resource_metadata="https://wrong.example", Bearer realm="OAuth", resource_metadata="https://api.example/metadata"',
9+
);
10+
expect(
11+
challenges
12+
?.find((challenge) => challenge.scheme === "bearer")
13+
?.params.get("resource_metadata"),
14+
).toBe("https://api.example/metadata");
15+
});
16+
17+
it("accepts the unquoted URL form emitted by providers", () => {
18+
const challenges = parseChallenges("bearer resource_metadata=https://api.example/metadata");
19+
expect(challenges?.[0]?.params.get("resource_metadata")).toBe("https://api.example/metadata");
20+
});
21+
22+
it("rejects unterminated quoted metadata", () => {
23+
expect(parseChallenges('Bearer resource_metadata="https://api.example/metadata')).toBeNull();
24+
});
25+
});

0 commit comments

Comments
 (0)