fix(auth): cover the CLI refresh path and tighten the well-known match
Two review findings. The stored-token refresh path (`refreshStoredAuthToken`) calls SDK discovery directly rather than through `InspectorClient.effectiveAuthFetch`, so a state file holding a refresh token and client information but no `serverMetadata` still hit the upstream OIDC-schema failure and could not refresh against the very servers this change is for. Its default `discover` now wraps the global fetch with the compat shim; a caller-supplied `fetchFn` is left alone. Covered by a test that injects no `discover` — injecting one would bypass the default under test — against a real server that 404s the RFC 8414 path and serves plain RFC 8414 metadata at the appended OIDC path. `oidcDiscoveryCandidates` matched the well-known path by bare prefix, so it also claimed `/.well-known/oauth-authorization-server-backup` and would have replaced that path's failed response with a document fetched from a derived URL. It now requires the exact path or a `/` boundary, which is precisely the set `buildDiscoveryUrls` can emit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
7ca753bc4c
commit
1db1b9be1f
4 changed files with 116 additions and 1 deletions
|
|
@ -873,3 +873,86 @@ describe("--wait-for-auth", () => {
|
||||||
expect(result.stderr).toContain("positive number of seconds");
|
expect(result.stderr).toContain("positive number of seconds");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The stored-token refresh path calls SDK discovery directly rather than
|
||||||
|
* through `InspectorClient.effectiveAuthFetch`, so it carries its own copy of
|
||||||
|
* the #2172 compatibility wrapper. This test injects no `discover`, because
|
||||||
|
* the wrapper lives in the *default* — injecting one would bypass exactly what
|
||||||
|
* is under test.
|
||||||
|
*/
|
||||||
|
describe("refreshStoredAuthToken discovery compatibility (#2172)", () => {
|
||||||
|
const REFRESHED = {
|
||||||
|
access_token: "refreshed-access-token",
|
||||||
|
token_type: "Bearer",
|
||||||
|
refresh_token: "rotated-refresh-token",
|
||||||
|
expires_in: 3600,
|
||||||
|
};
|
||||||
|
|
||||||
|
it("refreshes against an AS publishing RFC 8414 metadata at the OIDC path", async () => {
|
||||||
|
let base = "";
|
||||||
|
let rfc8414Probes = 0;
|
||||||
|
const server: Server = createServer((req, res) => {
|
||||||
|
const path = req.url ?? "";
|
||||||
|
if (path.startsWith("/.well-known/oauth-authorization-server")) {
|
||||||
|
rfc8414Probes += 1;
|
||||||
|
res.writeHead(404).end();
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
// Plain OAuth 2.0: no jwks_uri, no subject_types_supported, no
|
||||||
|
// id_token_signing_alg_values_supported — the shape the SDK rejects when
|
||||||
|
// it finds it under this filename.
|
||||||
|
if (path === "/mcp/.well-known/openid-configuration") {
|
||||||
|
res.writeHead(200, { "content-type": "application/json" });
|
||||||
|
res.end(
|
||||||
|
JSON.stringify({
|
||||||
|
issuer: `${base}/mcp`,
|
||||||
|
authorization_endpoint: `${base}/oauth/authorize`,
|
||||||
|
token_endpoint: `${base}/oauth/token`,
|
||||||
|
response_types_supported: ["code"],
|
||||||
|
grant_types_supported: ["authorization_code", "refresh_token"],
|
||||||
|
token_endpoint_auth_methods_supported: ["client_secret_post"],
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
if (path === "/oauth/token") {
|
||||||
|
res.writeHead(200, { "content-type": "application/json" });
|
||||||
|
res.end(JSON.stringify(REFRESHED));
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
res.writeHead(404).end();
|
||||||
|
});
|
||||||
|
await new Promise<void>((resolve) => {
|
||||||
|
server.listen(0, "127.0.0.1", resolve);
|
||||||
|
});
|
||||||
|
const addr = server.address();
|
||||||
|
base =
|
||||||
|
typeof addr === "object" && addr ? `http://127.0.0.1:${addr.port}` : "";
|
||||||
|
const serverUrl = `${base}/mcp`;
|
||||||
|
const fixture = writeOAuthFixture({
|
||||||
|
[serverUrl]: {
|
||||||
|
tokens: { refresh_token: "old-refresh", token_type: "Bearer" },
|
||||||
|
clientInformation: { client_id: "cid", client_secret: "sec" },
|
||||||
|
},
|
||||||
|
});
|
||||||
|
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||||
|
try {
|
||||||
|
await expect(refreshStoredAuthToken(serverUrl, fixture)).resolves.toBe(
|
||||||
|
"refreshed-access-token",
|
||||||
|
);
|
||||||
|
// The RFC 8414 location really was tried and really did 404, so the
|
||||||
|
// document could only have come from the OIDC path.
|
||||||
|
expect(rfc8414Probes).toBeGreaterThan(0);
|
||||||
|
expect(warn).toHaveBeenCalledWith(
|
||||||
|
expect.stringContaining("/mcp/.well-known/openid-configuration"),
|
||||||
|
);
|
||||||
|
} finally {
|
||||||
|
warn.mockRestore();
|
||||||
|
rmSync(dirname(fixture), { recursive: true, force: true });
|
||||||
|
await new Promise<void>((resolve) => {
|
||||||
|
server.close(() => resolve());
|
||||||
|
});
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -52,6 +52,7 @@ import {
|
||||||
getAuthorizationServerUrl,
|
getAuthorizationServerUrl,
|
||||||
getAuthorizationServerUrlCandidates,
|
getAuthorizationServerUrlCandidates,
|
||||||
} from "@inspector/core/auth/discovery.js";
|
} from "@inspector/core/auth/discovery.js";
|
||||||
|
import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js";
|
||||||
import { writeStoreFile } from "@inspector/core/storage/store-io.js";
|
import { writeStoreFile } from "@inspector/core/storage/store-io.js";
|
||||||
import {
|
import {
|
||||||
refreshAuthorization,
|
refreshAuthorization,
|
||||||
|
|
@ -329,7 +330,21 @@ export async function refreshStoredAuthToken(
|
||||||
deps: RefreshStoredAuthDeps = {},
|
deps: RefreshStoredAuthDeps = {},
|
||||||
): Promise<string> {
|
): Promise<string> {
|
||||||
const refresh = deps.refresh ?? refreshAuthorization;
|
const refresh = deps.refresh ?? refreshAuthorization;
|
||||||
const discover = deps.discover ?? discoverAuthorizationServerMetadata;
|
// #2172: this path calls SDK discovery directly rather than through
|
||||||
|
// `InspectorClient.effectiveAuthFetch`, so it needs the same compatibility
|
||||||
|
// wrapper — otherwise a stored refresh token with no persisted
|
||||||
|
// `serverMetadata` still cannot refresh against an authorization server that
|
||||||
|
// publishes RFC 8414 metadata at the OIDC well-known path (Copilot).
|
||||||
|
const compatFetch = withRfc8414OidcCompat(fetch);
|
||||||
|
const discover: typeof discoverAuthorizationServerMetadata =
|
||||||
|
deps.discover ??
|
||||||
|
((authorizationServerUrl, options) =>
|
||||||
|
discoverAuthorizationServerMetadata(authorizationServerUrl, {
|
||||||
|
...options,
|
||||||
|
// A caller-supplied fetch is left alone — it is theirs to compose. The
|
||||||
|
// walker below passes no options, so in practice this wraps the global.
|
||||||
|
fetchFn: options?.fetchFn ?? compatFetch,
|
||||||
|
}));
|
||||||
|
|
||||||
const snapshot = await readOAuthSnapshot(statePath);
|
const snapshot = await readOAuthSnapshot(statePath);
|
||||||
const servers = snapshot.servers as StoredServers;
|
const servers = snapshot.servers as StoredServers;
|
||||||
|
|
|
||||||
|
|
@ -75,6 +75,16 @@ describe("oidcDiscoveryCandidates", () => {
|
||||||
it("returns nothing for an unparseable URL", () => {
|
it("returns nothing for an unparseable URL", () => {
|
||||||
expect(oidcDiscoveryCandidates("not a url")).toEqual([]);
|
expect(oidcDiscoveryCandidates("not a url")).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("does not claim a path that merely shares the well-known prefix", () => {
|
||||||
|
// A bare prefix match would treat this as a discovery request and replace
|
||||||
|
// its failed response with a document from a derived URL (Copilot).
|
||||||
|
expect(
|
||||||
|
oidcDiscoveryCandidates(
|
||||||
|
"https://as.example.com/.well-known/oauth-authorization-server-backup",
|
||||||
|
),
|
||||||
|
).toEqual([]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("isRfc8414OnlyMetadata", () => {
|
describe("isRfc8414OnlyMetadata", () => {
|
||||||
|
|
|
||||||
|
|
@ -115,6 +115,13 @@ export function oidcDiscoveryCandidates(rfc8414Url: string): string[] {
|
||||||
}
|
}
|
||||||
if (!url.pathname.startsWith(RFC8414_WELL_KNOWN)) return [];
|
if (!url.pathname.startsWith(RFC8414_WELL_KNOWN)) return [];
|
||||||
const path = url.pathname.slice(RFC8414_WELL_KNOWN.length);
|
const path = url.pathname.slice(RFC8414_WELL_KNOWN.length);
|
||||||
|
// A bare prefix match would also claim `/.well-known/oauth-authorization-server-backup`
|
||||||
|
// — a path this wrapper has no business rewriting, and one whose failed
|
||||||
|
// response it would replace with a document fetched from somewhere else
|
||||||
|
// entirely (Copilot). The SDK appends the authorization server's pathname,
|
||||||
|
// which always begins with `/`, so requiring the exact path or a `/`
|
||||||
|
// boundary is precisely the set it can emit.
|
||||||
|
if (path !== "" && !path.startsWith("/")) return [];
|
||||||
// The authorization server had no path, so the SDK emitted a single OIDC
|
// The authorization server had no path, so the SDK emitted a single OIDC
|
||||||
// candidate at the origin.
|
// candidate at the origin.
|
||||||
if (path === "") return [new URL(OIDC_WELL_KNOWN, url.origin).href];
|
if (path === "") return [new URL(OIDC_WELL_KNOWN, url.origin).href];
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue