fix(auth): address review — proxy CIMD discovery fetch, validate metadata path
Two findings from review: - `ensureCimdClientRegistration` ran protected-resource discovery on the global fetch while its AS-metadata leg used the configured one. On web that is `createRemoteFetch`, which proxies through the backend to sidestep CORS — so the discovery this PR routes to a challenge-advertised URL would fail in the browser, be swallowed by the catch, and leave CIMD probing the wrong authorization server. Forward `params.fetchFn`, and assert in the test that nothing reaches the global. - `oauth.resourceMetadataPath` accepted `//other-host/doc`. Resolved against the request base that re-points the origin in the advertised `resource_metadata` while Express still registers the route locally, so the server would advertise a document it does not serve. `/\\host` does the same, since the URL parser folds a backslash into a slash for special schemes — a leading-slash check misses both. Validate origin-relativity with a shared predicate, used by the JSON-config validator and again at server setup for a programmatically built ServerConfig. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
f8a46726b7
commit
2a0290df26
2 changed files with 107 additions and 31 deletions
|
|
@ -105,24 +105,21 @@ describe("ensureCimdClientRegistration", () => {
|
|||
const metadataUrl = new URL(
|
||||
"http://127.0.0.1:9999/custom/protected-resource",
|
||||
);
|
||||
// `ensureCimdClientRegistration` runs protected-resource discovery through
|
||||
// the SDK's default fetch, so the advertised URL is observable only on the
|
||||
// global — the AS-metadata leg is the one that takes `fetchFn`.
|
||||
const globalFetch = vi.fn(async (input: RequestInfo | URL) =>
|
||||
String(input) === metadataUrl.href
|
||||
? new Response(
|
||||
JSON.stringify({
|
||||
resource: SERVER_URL,
|
||||
authorization_servers: ["http://127.0.0.1:9999"],
|
||||
}),
|
||||
)
|
||||
: new Response(null, { status: 404 }),
|
||||
);
|
||||
vi.stubGlobal("fetch", globalFetch);
|
||||
|
||||
const fetchFn = vi.fn(
|
||||
async () =>
|
||||
new Response(
|
||||
// Both discovery legs must run through the configured fetch — on web that
|
||||
// is the backend proxy, so a leg that fell back to the global would fail
|
||||
// on CORS and be swallowed.
|
||||
const fetchFn = vi.fn(async (input: RequestInfo | URL) => {
|
||||
const url = String(input);
|
||||
if (url === metadataUrl.href) {
|
||||
return new Response(
|
||||
JSON.stringify({
|
||||
resource: SERVER_URL,
|
||||
authorization_servers: ["http://127.0.0.1:9999"],
|
||||
}),
|
||||
);
|
||||
}
|
||||
if (url.includes("/.well-known/oauth-authorization-server")) {
|
||||
return new Response(
|
||||
JSON.stringify({
|
||||
issuer: "http://127.0.0.1:9999",
|
||||
authorization_endpoint: "http://127.0.0.1:9999/oauth/authorize",
|
||||
|
|
@ -130,8 +127,14 @@ describe("ensureCimdClientRegistration", () => {
|
|||
response_types_supported: ["code"],
|
||||
client_id_metadata_document_supported: true,
|
||||
}),
|
||||
),
|
||||
);
|
||||
);
|
||||
}
|
||||
return new Response(null, { status: 404 });
|
||||
});
|
||||
// Nothing may reach the network directly; a call here means a leg fell
|
||||
// back to the global fetch.
|
||||
const globalFetch = vi.fn();
|
||||
vi.stubGlobal("fetch", globalFetch);
|
||||
|
||||
await ensureCimdClientRegistration({
|
||||
serverUrl: SERVER_URL,
|
||||
|
|
@ -140,10 +143,14 @@ describe("ensureCimdClientRegistration", () => {
|
|||
resourceMetadataUrl: metadataUrl,
|
||||
});
|
||||
|
||||
expect(globalFetch).toHaveBeenCalled();
|
||||
for (const [input] of globalFetch.mock.calls) {
|
||||
expect(String(input)).toBe(metadataUrl.href);
|
||||
}
|
||||
expect(globalFetch).not.toHaveBeenCalled();
|
||||
const requested = fetchFn.mock.calls.map(([input]) => String(input));
|
||||
expect(requested).toContain(metadataUrl.href);
|
||||
expect(
|
||||
requested.filter((url) =>
|
||||
url.includes("/.well-known/oauth-protected-resource"),
|
||||
),
|
||||
).toEqual([]);
|
||||
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||
SERVER_URL,
|
||||
{ client_id: METADATA_URL },
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ import {
|
|||
TestServerHttp,
|
||||
getDefaultServerConfig,
|
||||
createOAuthTestServerConfig,
|
||||
isOriginRelativePath,
|
||||
loadConfig,
|
||||
resolveConfig,
|
||||
waitForOAuthWellKnown,
|
||||
|
|
@ -103,4 +104,20 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => {
|
|||
resourceMetadataPath: METADATA_PATH,
|
||||
});
|
||||
});
|
||||
|
||||
it.each([
|
||||
["/custom/protected-resource", true],
|
||||
["/a", true],
|
||||
// Both re-point the origin when resolved against the request base while
|
||||
// Express still registers the route locally, so the server would advertise
|
||||
// a document it does not serve (Copilot).
|
||||
["//other-host/doc", false],
|
||||
["/\\other-host/doc", false],
|
||||
["https://other-host/doc", false],
|
||||
["custom/protected-resource", false],
|
||||
["/has a space", false],
|
||||
["", false],
|
||||
])("origin-relative path check: %j -> %s", (value, expected) => {
|
||||
expect(isOriginRelativePath(value)).toBe(expected);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue