refactor(auth): address review — accurate toast copy, document state asymmetry, tighten test
- Soften the OAuth-callback-rejected toast: the shape guard is
defense-in-depth, not full CSRF prevention (PKCE remains primary), so the
message no longer overstates ("rejecting to prevent a cross-site request" ->
"did not originate from this session").
- Document the intentional asymmetry between a present-but-malformed `state`
(rejected) and a wholly absent `state` (accepted, matched via
OAUTH_PENDING_SERVER_KEY) — rejecting the null case would mask real provider
error redirects that omit `state`.
- Make the "valid state is not rejected" test assert the specific downstream
"could not be matched" toast (seeding OAUTH_PENDING_SERVER_KEY) instead of an
indirect "some toast fired" check.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
This commit is contained in:
parent
2b84281346
commit
d21da16983
2 changed files with 33 additions and 9 deletions
|
|
@ -404,6 +404,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
|
|||
}));
|
||||
|
||||
import App from "./App";
|
||||
import { OAUTH_PENDING_SERVER_KEY } from "./utils/oauthFlow";
|
||||
import * as McpIndex from "@inspector/core/mcp/index.js";
|
||||
import * as FetchLogModule from "@inspector/core/mcp/state/fetchRequestLogState.js";
|
||||
import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js";
|
||||
|
|
@ -1625,9 +1626,16 @@ describe("App OAuth callback state validation", () => {
|
|||
});
|
||||
|
||||
it("does not reject when the state param parses to a valid authId", async () => {
|
||||
// A well-formed 64-char-hex state is accepted; the handler proceeds past
|
||||
// the rejection guard (it fails later for other reasons in this harness,
|
||||
// but never shows the 'OAuth callback rejected' toast).
|
||||
// A well-formed 64-char-hex state passes the shape guard, so the handler
|
||||
// proceeds to the server-matching step. With no server registered under the
|
||||
// seeded pending id, that step surfaces the "could not be matched" toast —
|
||||
// asserting on that specific downstream toast proves the state was accepted
|
||||
// (never the "OAuth callback rejected" toast) rather than relying on an
|
||||
// indirect "some toast fired" check.
|
||||
window.sessionStorage.setItem(
|
||||
OAUTH_PENDING_SERVER_KEY,
|
||||
"server-that-does-not-exist",
|
||||
);
|
||||
window.history.replaceState(
|
||||
{},
|
||||
"",
|
||||
|
|
@ -1636,7 +1644,13 @@ describe("App OAuth callback state validation", () => {
|
|||
|
||||
renderWithMantine(<App />);
|
||||
|
||||
await waitFor(() => expect(notificationsMock.show).toHaveBeenCalled());
|
||||
await waitFor(() =>
|
||||
expect(notificationsMock.show).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
title: "OAuth callback could not be matched",
|
||||
}),
|
||||
),
|
||||
);
|
||||
expect(notificationsMock.show).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ title: "OAuth callback rejected" }),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -1473,10 +1473,20 @@ function App() {
|
|||
// rebuilt client can restore those `auth` entries. Read it before the
|
||||
// URL is cleared below.
|
||||
//
|
||||
// CSRF hardening: a `state` that is present but does not parse to our
|
||||
// expected 64-char-hex authId shape is a forgery indicator — the value
|
||||
// did not originate from `generateOAuthState`. Reject the callback with a
|
||||
// clear error instead of silently proceeding with an undefined sessionId.
|
||||
// Defense-in-depth: a `state` that is present but does not parse to our
|
||||
// expected 64-char-hex authId shape did not originate from
|
||||
// `generateOAuthState`, so reject the callback instead of silently
|
||||
// proceeding with an undefined sessionId. This is a shape check, not full
|
||||
// state-matching — the primary CSRF protection remains PKCE
|
||||
// (`code_verifier`); this layer catches malformed/forged `state` early.
|
||||
//
|
||||
// Intentional asymmetry: a present-but-malformed `state` (including the
|
||||
// empty string `?state=`) is rejected, but a wholly absent `state`
|
||||
// (`stateParam === null`) is *not* — it falls through with
|
||||
// `sessionId = undefined` and is matched via `OAUTH_PENDING_SERVER_KEY`
|
||||
// instead. Rejecting the null case would turn any provider error redirect
|
||||
// that omits `state` into a misleading "rejected" toast, hiding the real
|
||||
// OAuth error surfaced by the `!params.successful` branch below.
|
||||
const stateParam = new URLSearchParams(window.location.search).get("state");
|
||||
const parsedState = stateParam ? parseOAuthState(stateParam) : null;
|
||||
const stateRejected = stateParam !== null && parsedState === null;
|
||||
|
|
@ -1493,7 +1503,7 @@ function App() {
|
|||
notifications.show({
|
||||
title: "OAuth callback rejected",
|
||||
message:
|
||||
"OAuth callback carried an unrecognized state parameter; rejecting to prevent a cross-site request. Please try connecting again.",
|
||||
"OAuth callback carried an unrecognized state parameter that did not originate from this session. Please try connecting again.",
|
||||
color: "red",
|
||||
});
|
||||
return;
|
||||
|
|
|
|||
Loading…
Reference in a new issue