From d41e4e1dd7185ba6f0a0b3b78da4b98072da7578 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 24 Aug 2026 00:59:02 -0400 Subject: [PATCH 01/14] fix(auth): don't answer token_expired from an unknowable local expiry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `checkAuthChallengeSatisfied` answered a `token_expired` challenge with `isAccessTokenUsable`, which treats "we cannot tell" as "still valid": `isJwtExpired` returns false whenever the token is not a JWT or carries no `exp`, and storage never records an absolute expiry (`expires_in` is relative to an issuance instant that is never persisted). So for an opaque access token — Doorkeeper, Keycloak in opaque mode, GitHub-style `gho_…` — the local guess outvoted the resource server's explicit verdict. Callers short-circuited re-authorization and reconnected with the same dead credential, looping with no redirect and no ReAuthBanner. A bare 401 also maps to `token_expired`, so this was not limited to servers sending `error="invalid_token"`. Adds `isAccessTokenProvablyUnexpired` beside `isAccessTokenUsable` in `core/auth/connection-state.ts`: true only for a JWT whose `exp` is still in the future (same 60s skew). `checkAuthChallengeSatisfied` uses it for the `token_expired` branch, so absence of local evidence no longer beats the server, while a provably live JWT keeps its cheap short-circuit. `isAccessTokenUsable` stays optimistic — it is the right default for displaying authorization state, and flipping it would show every opaque-token server as unauthorized. Closes #2051 Signed-off-by: cliffhall --- .../test/core/auth/connection-state.test.ts | 72 +++++++++++++++++++ .../src/test/core/mcp/oauthManager.test.ts | 43 ++++++++++- core/auth/connection-state.ts | 32 ++++++++- core/mcp/oauthManager.ts | 14 ++-- 4 files changed, 152 insertions(+), 9 deletions(-) diff --git a/clients/web/src/test/core/auth/connection-state.test.ts b/clients/web/src/test/core/auth/connection-state.test.ts index cb5f351b..8898b263 100644 --- a/clients/web/src/test/core/auth/connection-state.test.ts +++ b/clients/web/src/test/core/auth/connection-state.test.ts @@ -2,6 +2,8 @@ import { describe, it, expect, vi } from "vitest"; import { buildOAuthConnectionState, hasPersistedOAuthServerState, + isAccessTokenProvablyUnexpired, + isAccessTokenUsable, isServerOAuthConfigured, protocolFromOAuthConfig, } from "@inspector/core/auth/connection-state.js"; @@ -257,3 +259,73 @@ describe("buildOAuthConnectionState", () => { expect(state.ema).not.toHaveProperty("idToken"); }); }); + +/** Minimal unsigned JWT carrying only `exp` (epoch seconds). */ +function jwtWithExp(expSec: number): string { + const payload = btoa(JSON.stringify({ exp: expSec })) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + return `header.${payload}.sig`; +} + +const nowSec = () => Math.floor(Date.now() / 1000); + +describe("isAccessTokenProvablyUnexpired", () => { + it("is true for a JWT whose exp is in the future", () => { + expect( + isAccessTokenProvablyUnexpired({ + access_token: jwtWithExp(nowSec() + 3600), + token_type: "Bearer", + }), + ).toBe(true); + }); + + it("is false for a JWT whose exp has passed", () => { + expect( + isAccessTokenProvablyUnexpired({ + access_token: jwtWithExp(nowSec() - 60), + token_type: "Bearer", + }), + ).toBe(false); + }); + + it("is false inside the 60s expiry skew window", () => { + expect( + isAccessTokenProvablyUnexpired({ + access_token: jwtWithExp(nowSec() + 30), + token_type: "Bearer", + }), + ).toBe(false); + }); + + it("is false for an opaque token, where isAccessTokenUsable is true", () => { + // The #2051 regression: no local expiry evidence must not read as "valid". + const tokens = { access_token: "opaque-tok", token_type: "Bearer" }; + expect(isAccessTokenProvablyUnexpired(tokens)).toBe(false); + expect(isAccessTokenUsable(tokens)).toBe(true); + }); + + it("is false for a JWT carrying no exp claim", () => { + const payload = btoa(JSON.stringify({ sub: "user" })) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + expect( + isAccessTokenProvablyUnexpired({ + access_token: `header.${payload}.sig`, + token_type: "Bearer", + }), + ).toBe(false); + }); + + it("is false when there is no access token", () => { + expect(isAccessTokenProvablyUnexpired(undefined)).toBe(false); + expect( + isAccessTokenProvablyUnexpired({ + access_token: "", + token_type: "Bearer", + }), + ).toBe(false); + }); +}); diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 1fbdf8e9..59ba1bfb 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -27,6 +27,15 @@ const mockedMcpAuth = vi.mocked(mcpAuth); const SERVER_URL = "https://example.com/mcp"; +/** Minimal unsigned JWT carrying only `exp` (epoch seconds). */ +function jwtWithExp(expSec: number): string { + const payload = btoa(JSON.stringify({ exp: expSec })) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + return `header.${payload}.sig`; +} + function createMockParams( overrides?: Partial, ): OAuthManagerParams { @@ -988,10 +997,10 @@ describe("OAuthManager", () => { ).toBe(false); }); - it("returns true for token_expired when a usable access token exists", async () => { + it("returns true for token_expired when a provably unexpired JWT exists", async () => { const params = createMockParams(); storageOf(params).getTokens.mockResolvedValue({ - access_token: "tok", + access_token: jwtWithExp(Math.floor(Date.now() / 1000) + 3600), token_type: "Bearer", }); const manager = new OAuthManager(params); @@ -1001,6 +1010,36 @@ describe("OAuthManager", () => { ).toBe(true); }); + it("returns false for token_expired when the stored JWT has expired", async () => { + const params = createMockParams(); + storageOf(params).getTokens.mockResolvedValue({ + access_token: jwtWithExp(Math.floor(Date.now() / 1000) - 60), + token_type: "Bearer", + }); + const manager = new OAuthManager(params); + + expect( + await manager.checkAuthChallengeSatisfied({ reason: "token_expired" }), + ).toBe(false); + }); + + it("returns false for token_expired when the stored token is opaque", async () => { + // Regression guard for #2051: an opaque token has no local expiry + // evidence, so it must not outvote the resource server's verdict and + // short-circuit re-authorization into replaying a dead credential. + const params = createMockParams(); + storageOf(params).getTokens.mockResolvedValue({ + access_token: "opaque-tok", + token_type: "Bearer", + expires_in: 900, + }); + const manager = new OAuthManager(params); + + expect( + await manager.checkAuthChallengeSatisfied({ reason: "token_expired" }), + ).toBe(false); + }); + it("returns false for invalid_token even when a locally valid token exists", async () => { const params = createMockParams(); storageOf(params).getTokens.mockResolvedValue({ diff --git a/core/auth/connection-state.ts b/core/auth/connection-state.ts index 8d20d7e2..58f8c0df 100644 --- a/core/auth/connection-state.ts +++ b/core/auth/connection-state.ts @@ -5,7 +5,7 @@ import type { import type { EnterpriseManagedAuthIdpConfig } from "../client/types.js"; import { getEmaIdpLoginState, normalizeIdpIssuer } from "./ema/idpSession.js"; import { idpOAuthStorageKey } from "./ema/storage.js"; -import { isJwtExpired } from "./ema/jwt.js"; +import { isJwtExpired, jwtExpiresAtMs } from "./ema/jwt.js"; import type { OAuthStorage } from "./storage.js"; import type { AuthProtocol, @@ -24,12 +24,40 @@ export interface BuildOAuthConnectionStateParams { flowState?: OAuthFlowState; } -/** True when persisted tokens include a non-expired access token (JWT exp when parseable). */ +/** + * True when persisted tokens include a non-expired access token (JWT exp when parseable). + * + * Optimistic: an access token whose expiry cannot be determined locally — an + * opaque token, or a JWT with no `exp` — counts as usable. Storage keeps no + * absolute expiry for those, because `expires_in` is relative to an issuance + * instant that is never recorded. That is the right default for *displaying* + * authorization state (an opaque-token server would otherwise always read as + * unauthorized), and the wrong one once a resource server has actually rejected + * the credential — see {@link isAccessTokenProvablyUnexpired}. + */ export function isAccessTokenUsable(tokens: OAuthTokens | undefined): boolean { if (!tokens?.access_token) return false; return !isJwtExpired(tokens.access_token); } +/** + * Pessimistic counterpart to {@link isAccessTokenUsable}: true only when the + * access token is a JWT carrying an `exp` that is still in the future (same 60s + * skew as {@link isJwtExpired}). + * + * Absence of local expiry evidence returns `false` rather than `true`, so it + * cannot outvote a resource server that has explicitly said the token is + * expired. Use this — not `isAccessTokenUsable` — to decide whether a + * `token_expired` challenge is already satisfied. + */ +export function isAccessTokenProvablyUnexpired( + tokens: OAuthTokens | undefined, +): boolean { + if (!tokens?.access_token) return false; + if (jwtExpiresAtMs(tokens.access_token) === undefined) return false; + return !isJwtExpired(tokens.access_token); +} + function resolveClient( preregistered: OAuthClientInformation | undefined, dynamic: OAuthClientInformation | undefined, diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 59846e2a..732f03d1 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -26,7 +26,7 @@ import { import { buildOAuthConnectionState, hasPersistedOAuthServerState, - isAccessTokenUsable, + isAccessTokenProvablyUnexpired, isServerOAuthConfigured, protocolFromOAuthConfig, } from "../auth/connection-state.js"; @@ -494,9 +494,12 @@ export class OAuthManager { * satisfied without an authorization-server round-trip. * * Returns `true` for `insufficient_scope` when stored + token scope cover the - * SEP-2350 union. For `token_expired`, returns `true` when a usable access - * token is already in storage. `invalid_token` and `unauthorized` always - * return `false` — the resource server explicitly rejected the credential. + * SEP-2350 union. For `token_expired`, returns `true` only when storage holds + * a *provably* unexpired token — a JWT whose `exp` is still in the future. An + * opaque token carries no local expiry evidence, so the resource server's + * verdict stands and re-authorization proceeds. `invalid_token` and + * `unauthorized` always return `false` — the resource server explicitly + * rejected the credential. */ async checkAuthChallengeSatisfied( challenge: AuthChallenge, @@ -514,7 +517,8 @@ export class OAuthManager { if (challenge.reason !== "insufficient_scope") { return ( - challenge.reason === "token_expired" && isAccessTokenUsable(tokens) + challenge.reason === "token_expired" && + isAccessTokenProvablyUnexpired(tokens) ); } From 268bfa108546a646073d1d4879c6ccc91cc2eb29 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 24 Aug 2026 01:02:02 -0400 Subject: [PATCH 02/14] fix(web): state the expected shape when dropping a CSP source (#2064) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `approveCspSources` warned "dropping unsafe CSP source", which states a security verdict the check never makes: `SAFE_CSP_SOURCE` screens each entry for injection safety only, and deliberately does not bound how broad a grant is (`resourceDomains: ["*"]` passes). The four fields are origin lists, so the real cause of a drop is almost always that the entry isn't an origin — a CSP keyword such as `'unsafe-eval'` has no field in the contract to be requested through. Reading it as a safety verdict is what sent #2012 chasing an XSS regression that wasn't one. The message now names the field the entry came from (so an app declaring several lists can tell which one was bad) and states the shape expected, with no claim about the value's safety. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/web/src/utils/sandbox-csp.test.ts | 23 +++++++++++++++++++++ clients/web/src/utils/sandbox-csp.ts | 25 +++++++++++++++++------ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/clients/web/src/utils/sandbox-csp.test.ts b/clients/web/src/utils/sandbox-csp.test.ts index 95b2de94..15042e23 100644 --- a/clients/web/src/utils/sandbox-csp.test.ts +++ b/clients/web/src/utils/sandbox-csp.test.ts @@ -67,6 +67,29 @@ describe("approveCspSources", () => { warn.mockRestore(); }); + it("names the offending field and states the expected shape, without calling the value unsafe", () => { + // #2064: the filter screens for injection safety only, so the message must + // not read as a verdict on the value. These four fields are origin lists, + // so the real cause is almost always "not an origin" — a CSP keyword has + // no field in the contract to be requested through. Naming the key also + // tells an app declaring several lists which one the bad entry was in. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + approveCspSources({ + connectDomains: ["'unsafe-eval'"], + frameDomains: ["