modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 4

fix: stop dropping oauth.onInsufficientScope when extracting the client secret (#2018)

Browse source 
Copilot round 4 (one suppressed finding, and a real bug).

`extractSecretsFromStored` rebuilt the stripped `oauth` object from an
allow-list of the non-secret fields. That list had gone stale: it never named
`onInsufficientScope`, so a server carrying both a client secret and a
non-default SEP-2350 policy silently lost the policy on save, falling back to
`reauthorize`. This predates #2018 — the PR only extended the same list with
`authorizationParams` — but the list is the defect, so fix the class rather
than add a third entry to it.

The stripped entry now subtracts the one secret instead of enumerating what to
keep, so any field added to `oauth` later is preserved by construction.

Regression test verified to fail against the allow-list version:
`expected { clientId: 'cid' } to deeply equal { clientId: 'cid', onInsufficientScope: 'throw' }`.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-15 21:54:16 -04:00
commit d95a6e629c
2 changed files with 28 additions and 17 deletions
Download patch file Download diff file Expand all files Collapse all files

21
clients/web/src/test/core/mcp/serverList.test.ts
View file

@ -925,6 +925,27 @@ describe("extractSecretsFromStored", () => {
expect(stripped.oauth).toEqual({ enterpriseManaged: true });
});
// The stripped entry now subtracts the secret instead of enumerating the
// fields to keep, so a field added to `oauth` later survives by construction.
// `onInsufficientScope` is the case the old allow-list had already lost.
it("preserves oauth.onInsufficientScope when lifting clientSecret to keychain", () => {
const stored: StoredMCPServer = {
type: "streamable-http",
url: "https://x.test",
oauth: {
clientId: "cid",
clientSecret: "shh",
onInsufficientScope: "throw",
},
};
const { stripped, secrets } = extractSecretsFromStored(stored);
expect(secrets).toEqual({ [SECRET_FIELD_OAUTH_CLIENT_SECRET]: "shh" });
expect(stripped.oauth).toEqual({
clientId: "cid",
onInsufficientScope: "throw",
});
});
it("preserves oauth.authorizationParams when lifting clientSecret to keychain", () => {
const stored: StoredMCPServer = {
type: "streamable-http",

24
core/mcp/serverList.ts
View file

@ -642,23 +642,13 @@ export function extractSecretsFromStored(
if (stored.oauth?.clientSecret) {
secrets[SECRET_FIELD_OAUTH_CLIENT_SECRET] = stored.oauth.clientSecret;
const restOauth: {
clientId?: string;
scopes?: string;
authorizationParams?: Record<string, string>;
enterpriseManaged?: boolean;
} = {};
if (stored.oauth.clientId !== undefined)
restOauth.clientId = stored.oauth.clientId;
if (stored.oauth.scopes !== undefined)
restOauth.scopes = stored.oauth.scopes;
// Not a secret — the authorization parameters stay on the stripped entry so
// stripping the client secret doesn't silently discard them. (#2018)
if (stored.oauth.authorizationParams !== undefined)
restOauth.authorizationParams = stored.oauth.authorizationParams;
if (stored.oauth.enterpriseManaged === true) {
restOauth.enterpriseManaged = true;
}
// Keep every OAuth field except the one secret, rather than enumerating the
// non-secret ones. An allow-list here is a standing trap: it silently drops
// any field added to `oauth` afterwards, and it had already done so —
// `onInsufficientScope` was lost whenever a server carried both a client
// secret and a non-default SEP-2350 policy. Subtracting the secret instead
// means a future field is preserved by construction. (#2018)
const { clientSecret: _clientSecret, ...restOauth } = stored.oauth;
if (Object.keys(restOauth).length > 0) {
stripped.oauth = restOauth;
} else {