fix(auth): RFC-encode the Basic credential; make the TUI clear await its work (#2144)
Copilot review round 6. - The Basic credential is now form-urlencoded per RFC 6749 §2.3.1. Round 1 matched the SDK's raw `id:secret` on the theory that a credential should be presented the same way here as at the token endpoint; that does not hold — a client authenticated with `client_secret_post` never exercised the SDK's Basic path, so there is no precedent to match, and the raw form is ambiguous for an id containing `:` and makes `btoa` throw on a non-Latin-1 secret. The fixture decodes each half. - The ctx-less grant read is suppressed on the store's issuer *stamp* rather than on the token's value. The active slot can hold client information without a token, in which case the read falls back to the legacy grant — and a value collision with any other issuer would have dropped that grant unrevoked and unreported. - The TUI announced "OAuth state cleared" on the keypress, so with a bounded revocation request it said so while the work was still in flight and then removed it when the work finished. `onClearOAuth` returns a promise; `AuthTab` shows "Clearing OAuth state…" until it settles, ignores repeats while one is in flight, and skips exactly the revision bump its own clear caused so the confirmation is not immediately reset by it. - `handleClearOAuth` now claims an attempt and retires a superseded one, the way `handleDisconnect` does: the await can keep it suspended for seconds, long enough for the user to switch servers and have server A's outcome — including its disconnect — land on server B. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
e9cd678cd0
commit
56ac83051b
6 changed files with 193 additions and 35 deletions
|
|
@ -100,6 +100,64 @@ describe("AuthTab", () => {
|
|||
);
|
||||
});
|
||||
|
||||
// #2144 — the clear is now a bounded network request, so announcing "cleared"
|
||||
// on the keypress would say it while the work was still in flight.
|
||||
it("shows a pending state until the clear settles, and ignores repeats", async () => {
|
||||
let settle: () => void = () => {};
|
||||
const onClearOAuth = vi.fn(
|
||||
() =>
|
||||
new Promise<void>((resolve) => {
|
||||
settle = resolve;
|
||||
}),
|
||||
);
|
||||
const { lastFrame, stdin } = render(
|
||||
<AuthTab
|
||||
{...baseProps}
|
||||
onClearOAuth={onClearOAuth}
|
||||
inspectorClient={null}
|
||||
oauthStatus="idle"
|
||||
oauthMessage={null}
|
||||
focused
|
||||
/>,
|
||||
);
|
||||
await tick();
|
||||
|
||||
stdin.write("s");
|
||||
await tick();
|
||||
expect(lastFrame() ?? "").toContain("Clearing OAuth state");
|
||||
expect(lastFrame() ?? "").not.toContain("OAuth state cleared");
|
||||
|
||||
// A second press while one is in flight would race the first over the same
|
||||
// store entry, and nothing on screen tells the user the first is running.
|
||||
stdin.write("s");
|
||||
await tick();
|
||||
expect(onClearOAuth).toHaveBeenCalledTimes(1);
|
||||
|
||||
settle();
|
||||
await tick();
|
||||
expect(lastFrame() ?? "").toContain("OAuth state cleared");
|
||||
});
|
||||
|
||||
// A failed revocation still cleared local state, and the failure is reported
|
||||
// through the message line — so the confirmation must not hang forever.
|
||||
it("settles the confirmation even when the clear rejects", async () => {
|
||||
const onClearOAuth = vi.fn(() => Promise.reject(new Error("nope")));
|
||||
const { lastFrame, stdin } = render(
|
||||
<AuthTab
|
||||
{...baseProps}
|
||||
onClearOAuth={onClearOAuth}
|
||||
inspectorClient={null}
|
||||
oauthStatus="idle"
|
||||
oauthMessage={null}
|
||||
focused
|
||||
/>,
|
||||
);
|
||||
await tick();
|
||||
stdin.write("s");
|
||||
await tick();
|
||||
expect(lastFrame() ?? "").toContain("OAuth state cleared");
|
||||
});
|
||||
|
||||
// #2144 — a revocation failure is a *partial* success: the local state really
|
||||
// was cleared, so it is not an `error` status, but the grant may still be
|
||||
// live at the authorization server and the informational tone understates
|
||||
|
|
|
|||
|
|
@ -183,6 +183,8 @@ function App({
|
|||
const [connectError, setConnectError] = useState<string | null>(null);
|
||||
// Monotonic token for in-flight disconnects — see handleDisconnect.
|
||||
const disconnectAttemptRef = useRef(0);
|
||||
/** Retires a superseded "clear OAuth state" the way disconnects are (#2144). */
|
||||
const clearOAuthAttemptRef = useRef(0);
|
||||
// A failed disconnect is deliberately NOT folded into `connectError`. That
|
||||
// one is only rendered by InfoTab when the status is "error", which a
|
||||
// rejected disconnect leaves untouched (the status stays "connected"), so
|
||||
|
|
@ -949,13 +951,27 @@ function App({
|
|||
|
||||
const handleClearOAuth = useCallback(async () => {
|
||||
if (!selectedInspectorClient) return;
|
||||
// RFC 7009 (#2144). Best-effort: the outcome is reported in the status
|
||||
// line, never thrown, so clearing always completes. The per-server
|
||||
// `oauthRevokeOnClear` opt-out is honored here the same way the web client
|
||||
// honors it.
|
||||
// Claim this attempt before awaiting, exactly as `handleDisconnect` does.
|
||||
// The RFC 7009 leg is a bounded network request, so this callback can now
|
||||
// stay suspended for seconds — long enough for the user to switch servers,
|
||||
// at which point publishing this result would put server A's outcome on
|
||||
// server B (and would race a second clear over the same state). (#2144)
|
||||
const attempt = ++clearOAuthAttemptRef.current;
|
||||
const attemptServer = selectedServer;
|
||||
// RFC 7009. Best-effort: the outcome is reported in the status line, never
|
||||
// thrown, so clearing always completes. The per-server `oauthRevokeOnClear`
|
||||
// opt-out is honored here the same way the web client honors it.
|
||||
const revocation = await selectedInspectorClient.clearOAuthTokens({
|
||||
revoke: selectedServerEntry?.settings?.oauthRevokeOnClear !== false,
|
||||
});
|
||||
// The local clear has happened either way — only the *reporting* is
|
||||
// retired, so a superseded attempt leaves no trace on the new selection.
|
||||
if (
|
||||
clearOAuthAttemptRef.current !== attempt ||
|
||||
selectedServerRef.current !== attemptServer
|
||||
) {
|
||||
return;
|
||||
}
|
||||
setOauthStatus("idle");
|
||||
if (revocation.status === "failed") {
|
||||
const warning = `Cleared locally, but revoking the grant at the authorization server failed: ${revocation.detail}. It may still be valid there.`;
|
||||
|
|
@ -971,6 +987,7 @@ function App({
|
|||
setOauthRevision((n) => n + 1);
|
||||
}, [
|
||||
selectedInspectorClient,
|
||||
selectedServer,
|
||||
selectedServerEntry,
|
||||
inspectorStatus,
|
||||
disconnectInspector,
|
||||
|
|
|
|||
|
|
@ -47,7 +47,15 @@ interface AuthTabProps {
|
|||
width: number;
|
||||
height: number;
|
||||
focused?: boolean;
|
||||
onClearOAuth: () => void;
|
||||
/**
|
||||
* Clears (and, unless opted out, revokes) this server's OAuth state.
|
||||
*
|
||||
* Returns a promise so the confirmation below can wait for it. The RFC 7009
|
||||
* revocation leg is a network request with a five-second bound (#2144), so
|
||||
* this is no longer instantaneous, and announcing "cleared" on the keypress
|
||||
* would say it while the work was still in flight.
|
||||
*/
|
||||
onClearOAuth: () => void | Promise<void>;
|
||||
connectionStatus: ConnectionStatus;
|
||||
}
|
||||
|
||||
|
|
@ -82,7 +90,16 @@ export function AuthTab({
|
|||
const [oauthState, setOauthState] = useState<
|
||||
OAuthConnectionState | undefined
|
||||
>(undefined);
|
||||
const [clearedConfirmation, setClearedConfirmation] = useState(false);
|
||||
const [clearState, setClearState] = useState<"idle" | "clearing" | "cleared">(
|
||||
"idle",
|
||||
);
|
||||
/**
|
||||
* Set synchronously when a clear starts, and consumed by the reset effect
|
||||
* below. The clear itself bumps `oauthRevision` on its way out, so without
|
||||
* this the reset would race the confirmation it is meant to outlive — and
|
||||
* which of the two lands first is not something the ordering guarantees.
|
||||
*/
|
||||
const ownClearRef = useRef(false);
|
||||
const [lastClearDisconnected, setLastClearDisconnected] = useState(false);
|
||||
const [stepUpChoiceIndex, setStepUpChoiceIndex] = useState(0);
|
||||
|
||||
|
|
@ -100,7 +117,12 @@ export function AuthTab({
|
|||
}, [refreshOAuthState, oauthRevision, connectionStatus]);
|
||||
|
||||
useEffect(() => {
|
||||
setClearedConfirmation(false);
|
||||
// Skip exactly the revision bump our own clear caused; reset on the next.
|
||||
if (ownClearRef.current) {
|
||||
ownClearRef.current = false;
|
||||
return;
|
||||
}
|
||||
setClearState("idle");
|
||||
setLastClearDisconnected(false);
|
||||
}, [oauthRevision]);
|
||||
|
||||
|
|
@ -163,9 +185,17 @@ export function AuthTab({
|
|||
const h = scrollViewRef.current.getViewportHeight() || 1;
|
||||
scrollViewRef.current.scrollBy(h);
|
||||
} else if (input.toLowerCase() === "s") {
|
||||
// Ignore repeats while one is in flight: the second would race the
|
||||
// first over the same store entry, and the user cannot see that the
|
||||
// first is still running except by this state.
|
||||
if (clearState === "clearing") return;
|
||||
setLastClearDisconnected(isLiveConnection);
|
||||
onClearOAuth();
|
||||
setClearedConfirmation(true);
|
||||
ownClearRef.current = true;
|
||||
setClearState("clearing");
|
||||
const settle = () => setClearState("cleared");
|
||||
// Settled either way: a failed revocation still cleared local state,
|
||||
// and `handleClearOAuth` reports the failure through the message line.
|
||||
void Promise.resolve(onClearOAuth()).then(settle, settle);
|
||||
}
|
||||
},
|
||||
{ isActive: focused },
|
||||
|
|
@ -331,7 +361,10 @@ export function AuthTab({
|
|||
Clear OAuth <Text underline>S</Text>tate
|
||||
{isLiveConnection && " and disconnect"}
|
||||
</SelectableItem>
|
||||
{clearedConfirmation && (
|
||||
{clearState === "clearing" && (
|
||||
<Text color="yellow">Clearing OAuth state…</Text>
|
||||
)}
|
||||
{clearState === "cleared" && (
|
||||
<Text color="green">
|
||||
{lastClearDisconnected
|
||||
? "OAuth state cleared. Disconnected."
|
||||
|
|
|
|||
|
|
@ -191,12 +191,11 @@ describe("buildRevocationRequest", () => {
|
|||
clientInformation: { client_id: "id one", client_secret: "s/ecret" },
|
||||
supportedAuthMethods: ["client_secret_basic"],
|
||||
});
|
||||
// Byte-identical to the SDK's `applyBasicAuth`: the raw `id:secret`, not the
|
||||
// form-urlencoded pair RFC 6749 §2.3.1 asks for. Presenting the credential
|
||||
// differently here than at the token endpoint is what would let an
|
||||
// authorization server accept one request and reject the other.
|
||||
// RFC 6749 §2.3.1 form-urlencodes each half before the colon. That is what
|
||||
// keeps an id containing `:` unambiguous, and what stops `btoa` throwing on
|
||||
// a non-Latin-1 secret.
|
||||
expect(headerOf(init, "Authorization")).toBe(
|
||||
`Basic ${btoa("id one:s/ecret")}`,
|
||||
`Basic ${btoa("id%20one:s%2Fecret")}`,
|
||||
);
|
||||
expect(body(init).has("client_secret")).toBe(false);
|
||||
});
|
||||
|
|
@ -471,6 +470,43 @@ describe("revokeStoredOAuthTokens", () => {
|
|||
).toBe("legacy-r");
|
||||
});
|
||||
|
||||
// The active issuer slot can hold client information without a token, in
|
||||
// which case the ctx-less read falls back to the LEGACY grant — a distinct
|
||||
// grant, even if some other issuer happens to hold the same opaque value.
|
||||
// Suppressing on the value would drop it unrevoked and unreported, which is
|
||||
// why the suppression keys off the store's issuer stamp instead.
|
||||
it("keeps the legacy grant when another issuer holds the same token value", async () => {
|
||||
await storage.saveServerMetadata(
|
||||
SERVER_URL,
|
||||
metadata({ issuer: undefined }),
|
||||
);
|
||||
// A legacy unkeyed grant...
|
||||
await storage.saveTokens(SERVER_URL, {
|
||||
access_token: "shared",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "shared-r",
|
||||
});
|
||||
// ...and an issuer slot that coincidentally holds the same token value.
|
||||
// Saved via the client-information path so it does not clear the legacy
|
||||
// slot, then given tokens through a stubbed exact read.
|
||||
vi.spyOn(storage, "listIssuers").mockResolvedValue([
|
||||
"https://as-a.example.com",
|
||||
]);
|
||||
vi.spyOn(storage, "getIssuerTokens").mockResolvedValue({
|
||||
access_token: "shared",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "shared-r",
|
||||
});
|
||||
const fetchFn = vi.fn<typeof fetch>(
|
||||
async () => new Response(null, { status: 200 }),
|
||||
);
|
||||
|
||||
await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn });
|
||||
|
||||
// Two grants, two requests — the issuer-bound one and the legacy one.
|
||||
expect(fetchFn).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
// A grant bound to an issuer the cached metadata does not describe cannot be
|
||||
// revoked — that endpoint belongs to a different authorization server, and
|
||||
// sending it another AS's token would hand a credential to a server that
|
||||
|
|
|
|||
|
|
@ -140,15 +140,17 @@ export function buildRevocationRequest(params: RevocationRequestParams): {
|
|||
if (client) {
|
||||
const method = selectClientAuthMethod(client, params.supportedAuthMethods);
|
||||
if (method === "client_secret_basic" && client.client_secret) {
|
||||
// Deliberately byte-identical to what the SDK's `applyBasicAuth` sends at
|
||||
// the token endpoint: the raw `id:secret`, *not* the form-urlencoded pair
|
||||
// RFC 6749 §2.3.1 asks for. Matching the RFC here instead would mean this
|
||||
// request and the token request present the same credential differently,
|
||||
// so an authorization server holding a secret with a reserved character
|
||||
// could accept one and reject the other — a failure that would look like
|
||||
// "revocation is broken" rather than like an encoding disagreement.
|
||||
// Whatever the AS accepted to mint these tokens is what ends them.
|
||||
headers.Authorization = `Basic ${base64Encode(`${client.client_id}:${client.client_secret}`)}`;
|
||||
// RFC 6749 §2.3.1: each half is form-urlencoded *before* the colon joins
|
||||
// them. This deliberately differs from the SDK's `applyBasicAuth`, which
|
||||
// base64s the raw pair — matching that was the first instinct, on the
|
||||
// theory that a credential should be presented the same way here as at
|
||||
// the token endpoint. It does not hold up: a client authenticated with
|
||||
// `client_secret_post` never exercised the SDK's Basic path at all, so
|
||||
// there is no precedent to match, and the raw form is ambiguous for a
|
||||
// client id containing `:` and makes `btoa` throw outright on a
|
||||
// non-Latin-1 secret. Encoding is what the server decodes.
|
||||
const credentials = `${encodeURIComponent(client.client_id)}:${encodeURIComponent(client.client_secret)}`;
|
||||
headers.Authorization = `Basic ${base64Encode(credentials)}`;
|
||||
} else if (method === "client_secret_post" && client.client_secret) {
|
||||
body.set("client_id", client.client_id);
|
||||
body.set("client_secret", client.client_secret);
|
||||
|
|
@ -374,11 +376,13 @@ interface CollectedGrants {
|
|||
*
|
||||
* The ctx-less read is included last and is the one exception: it resolves to
|
||||
* the *active* issuer's slot (already collected above) or, on a pre-SEP-2352
|
||||
* entry, to the legacy unkeyed token — which nothing else returns. So it is
|
||||
* suppressed when its token was already taken from any slot, which is exactly
|
||||
* the first case and never the second. It is also the only read here allowed
|
||||
* to fall back; an enumerated issuer is read exactly, so a legacy token is
|
||||
* never mislabelled as belonging to one.
|
||||
* entry, to the legacy unkeyed token — which nothing else returns. It is
|
||||
* suppressed on the issuer *stamp* the store puts on a slot-sourced value
|
||||
* rather than on the token's value, because the active slot may hold client
|
||||
* information without a token: the read then falls back to the legacy grant,
|
||||
* and a value collision with any other issuer would drop a real grant. It is
|
||||
* also the only read here allowed to fall back; an enumerated issuer is read
|
||||
* exactly, so a legacy token is never mislabelled as belonging to one.
|
||||
*/
|
||||
async function collectGrants(
|
||||
storage: OAuthStorage,
|
||||
|
|
@ -387,7 +391,6 @@ async function collectGrants(
|
|||
const grants: StoredGrant[] = [];
|
||||
const failures: TokenRevocationOutcome[] = [];
|
||||
const seenKeys = new Set<string>();
|
||||
const slotTokens = new Set<string>();
|
||||
|
||||
const add = async (issuer?: string): Promise<void> => {
|
||||
const tokens =
|
||||
|
|
@ -398,13 +401,20 @@ async function collectGrants(
|
|||
if (!revocable) return;
|
||||
|
||||
if (issuer === undefined) {
|
||||
// Same grant as the active issuer's slot, already collected.
|
||||
if (slotTokens.has(revocable.token)) return;
|
||||
// `getTokens` stamps the resolved issuer onto a value it took from a
|
||||
// byIssuer slot and leaves an unkeyed one unstamped (see `withIssuer` in
|
||||
// oauth-storage.ts — the stamp is the key it came from). That, not the
|
||||
// token's *value*, is what says this read duplicates a slot already
|
||||
// collected: the active slot can hold client information without a token,
|
||||
// in which case this falls back to the legacy grant, and a coincidental
|
||||
// value collision with some other issuer would otherwise drop it
|
||||
// unrevoked and unreported.
|
||||
const fromSlot = (tokens as { issuer?: string } | undefined)?.issuer;
|
||||
if (fromSlot !== undefined) return;
|
||||
} else {
|
||||
const key = `${issuer}\u0000${revocable.token}`;
|
||||
if (seenKeys.has(key)) return;
|
||||
seenKeys.add(key);
|
||||
slotTokens.add(revocable.token);
|
||||
}
|
||||
|
||||
grants.push({
|
||||
|
|
|
|||
|
|
@ -798,8 +798,12 @@ async function authenticateRevocationClient(
|
|||
).toString("utf8");
|
||||
const separator = decoded.indexOf(":");
|
||||
if (separator === -1) return null;
|
||||
clientId = decoded.slice(0, separator);
|
||||
clientSecret = decoded.slice(separator + 1);
|
||||
// RFC 6749 §2.3.1: each half is form-urlencoded before the colon, so the
|
||||
// server decodes each half after splitting on it. Decoding is what makes a
|
||||
// credential containing a reserved character (`:` in the id, `%` or `/` in
|
||||
// the secret) survive the round trip.
|
||||
clientId = decodeURIComponent(decoded.slice(0, separator));
|
||||
clientSecret = decodeURIComponent(decoded.slice(separator + 1));
|
||||
} else {
|
||||
const bodyId: unknown = req.body?.client_id;
|
||||
const bodySecret: unknown = req.body?.client_secret;
|
||||
|
|
|
|||
Loading…
Reference in a new issue