fix(auth): don't answer token_expired from an unknowable local expiry
`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 <cliff@futurescale.com>
This commit is contained in:
parent
a92a73003b
commit
d41e4e1dd7
4 changed files with 152 additions and 9 deletions
|
|
@ -2,6 +2,8 @@ import { describe, it, expect, vi } from "vitest";
|
||||||
import {
|
import {
|
||||||
buildOAuthConnectionState,
|
buildOAuthConnectionState,
|
||||||
hasPersistedOAuthServerState,
|
hasPersistedOAuthServerState,
|
||||||
|
isAccessTokenProvablyUnexpired,
|
||||||
|
isAccessTokenUsable,
|
||||||
isServerOAuthConfigured,
|
isServerOAuthConfigured,
|
||||||
protocolFromOAuthConfig,
|
protocolFromOAuthConfig,
|
||||||
} from "@inspector/core/auth/connection-state.js";
|
} from "@inspector/core/auth/connection-state.js";
|
||||||
|
|
@ -257,3 +259,73 @@ describe("buildOAuthConnectionState", () => {
|
||||||
expect(state.ema).not.toHaveProperty("idToken");
|
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);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -27,6 +27,15 @@ const mockedMcpAuth = vi.mocked(mcpAuth);
|
||||||
|
|
||||||
const SERVER_URL = "https://example.com/mcp";
|
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(
|
function createMockParams(
|
||||||
overrides?: Partial<OAuthManagerParams>,
|
overrides?: Partial<OAuthManagerParams>,
|
||||||
): OAuthManagerParams {
|
): OAuthManagerParams {
|
||||||
|
|
@ -988,10 +997,10 @@ describe("OAuthManager", () => {
|
||||||
).toBe(false);
|
).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();
|
const params = createMockParams();
|
||||||
storageOf(params).getTokens.mockResolvedValue({
|
storageOf(params).getTokens.mockResolvedValue({
|
||||||
access_token: "tok",
|
access_token: jwtWithExp(Math.floor(Date.now() / 1000) + 3600),
|
||||||
token_type: "Bearer",
|
token_type: "Bearer",
|
||||||
});
|
});
|
||||||
const manager = new OAuthManager(params);
|
const manager = new OAuthManager(params);
|
||||||
|
|
@ -1001,6 +1010,36 @@ describe("OAuthManager", () => {
|
||||||
).toBe(true);
|
).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 () => {
|
it("returns false for invalid_token even when a locally valid token exists", async () => {
|
||||||
const params = createMockParams();
|
const params = createMockParams();
|
||||||
storageOf(params).getTokens.mockResolvedValue({
|
storageOf(params).getTokens.mockResolvedValue({
|
||||||
|
|
|
||||||
|
|
@ -5,7 +5,7 @@ import type {
|
||||||
import type { EnterpriseManagedAuthIdpConfig } from "../client/types.js";
|
import type { EnterpriseManagedAuthIdpConfig } from "../client/types.js";
|
||||||
import { getEmaIdpLoginState, normalizeIdpIssuer } from "./ema/idpSession.js";
|
import { getEmaIdpLoginState, normalizeIdpIssuer } from "./ema/idpSession.js";
|
||||||
import { idpOAuthStorageKey } from "./ema/storage.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 { OAuthStorage } from "./storage.js";
|
||||||
import type {
|
import type {
|
||||||
AuthProtocol,
|
AuthProtocol,
|
||||||
|
|
@ -24,12 +24,40 @@ export interface BuildOAuthConnectionStateParams {
|
||||||
flowState?: OAuthFlowState;
|
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 {
|
export function isAccessTokenUsable(tokens: OAuthTokens | undefined): boolean {
|
||||||
if (!tokens?.access_token) return false;
|
if (!tokens?.access_token) return false;
|
||||||
return !isJwtExpired(tokens.access_token);
|
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(
|
function resolveClient(
|
||||||
preregistered: OAuthClientInformation | undefined,
|
preregistered: OAuthClientInformation | undefined,
|
||||||
dynamic: OAuthClientInformation | undefined,
|
dynamic: OAuthClientInformation | undefined,
|
||||||
|
|
|
||||||
|
|
@ -26,7 +26,7 @@ import {
|
||||||
import {
|
import {
|
||||||
buildOAuthConnectionState,
|
buildOAuthConnectionState,
|
||||||
hasPersistedOAuthServerState,
|
hasPersistedOAuthServerState,
|
||||||
isAccessTokenUsable,
|
isAccessTokenProvablyUnexpired,
|
||||||
isServerOAuthConfigured,
|
isServerOAuthConfigured,
|
||||||
protocolFromOAuthConfig,
|
protocolFromOAuthConfig,
|
||||||
} from "../auth/connection-state.js";
|
} from "../auth/connection-state.js";
|
||||||
|
|
@ -494,9 +494,12 @@ export class OAuthManager {
|
||||||
* satisfied without an authorization-server round-trip.
|
* satisfied without an authorization-server round-trip.
|
||||||
*
|
*
|
||||||
* Returns `true` for `insufficient_scope` when stored + token scope cover the
|
* Returns `true` for `insufficient_scope` when stored + token scope cover the
|
||||||
* SEP-2350 union. For `token_expired`, returns `true` when a usable access
|
* SEP-2350 union. For `token_expired`, returns `true` only when storage holds
|
||||||
* token is already in storage. `invalid_token` and `unauthorized` always
|
* a *provably* unexpired token — a JWT whose `exp` is still in the future. An
|
||||||
* return `false` — the resource server explicitly rejected the credential.
|
* 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(
|
async checkAuthChallengeSatisfied(
|
||||||
challenge: AuthChallenge,
|
challenge: AuthChallenge,
|
||||||
|
|
@ -514,7 +517,8 @@ export class OAuthManager {
|
||||||
|
|
||||||
if (challenge.reason !== "insufficient_scope") {
|
if (challenge.reason !== "insufficient_scope") {
|
||||||
return (
|
return (
|
||||||
challenge.reason === "token_expired" && isAccessTokenUsable(tokens)
|
challenge.reason === "token_expired" &&
|
||||||
|
isAccessTokenProvablyUnexpired(tokens)
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue