From 01cc79c61b2c669021f33cb0dc6f73dc66921b8d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 4 Jul 2026 16:47:48 -0400 Subject: [PATCH] =?UTF-8?q?test(cli):=20cover=20cliOAuth.ts=20step-up/reco?= =?UTF-8?q?very=20paths=20to=20the=20=E2=89=A590%=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring clients/cli/src/cliOAuth.ts from 75% to ≥90% on all four coverage dimensions (now 100/96.42/100/100) by exercising the previously-untested interactive step-up and auth-recovery paths: - confirmStepUpFromStdin (the default stdin confirmer): drive it via the default `confirmStepUp` parameter of handleCliAuthRecoveryRequired with node:readline/promises mocked — covers the y / "yes" (trimmed, case-insensitive) accept paths and the n decline path (which throws). - connectInspectorWithOAuth's AuthRecoveryRequiredError branch: both the storage-already-satisfies resume and the interactive-recovery sub-branches, plus the isUnauthorizedError branch (with a rejecting disconnect to hit the `.catch(() => {})` guard), the non-OAuth-capable-config rethrow, and the non-OAuth-error rethrow. Part of #1610 — v2/main mid-session-auth code below the per-file coverage gate that v2/main's CI does not run but the Wave-1 rollup (#1550) does. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX --- clients/cli/__tests__/cliOAuth.test.ts | 227 +++++++++++++++++++++++++ 1 file changed, 227 insertions(+) diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index 42178e29..f9d19153 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -3,15 +3,39 @@ import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { MutableRedirectUrlProvider } from "@inspector/core/auth/index.js"; import * as runnerInteractive from "@inspector/core/auth/node/runner-interactive-oauth.js"; import { + connectInspectorWithOAuth, handleCliAuthRecoveryRequired, isStandardOAuthStepUp, runCliInteractiveOAuth, withCliAuthRecoveryRetry, } from "../src/cliOAuth.js"; +import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; + +// `confirmStepUpFromStdin` (the default step-up confirmer) reads a line from +// stdin via node:readline/promises. Mock the module so the default path can be +// exercised deterministically without real TTY input. +const { mockQuestion, mockClose } = vi.hoisted(() => ({ + mockQuestion: vi.fn(), + mockClose: vi.fn(), +})); +vi.mock("node:readline/promises", () => ({ + createInterface: vi.fn(() => ({ + question: mockQuestion, + close: mockClose, + })), +})); + +const CALLBACK_URL_CONFIG = { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", +}; describe("cliOAuth", () => { afterEach(() => { vi.restoreAllMocks(); + mockQuestion.mockReset(); + mockClose.mockReset(); }); describe("isStandardOAuthStepUp", () => { @@ -227,4 +251,207 @@ describe("cliOAuth", () => { expect(result).toBe("ok"); expect(fn).toHaveBeenCalledTimes(2); }); + + describe("confirmStepUpFromStdin (default stdin confirmer)", () => { + const standardStepUpError = () => + new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { + reason: "insufficient_scope", + requiredScopes: ["weather:read"], + }); + + const clientNeedingStepUp = () => ({ + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), + }); + + it("proceeds with OAuth when the user answers y (no confirmStepUp arg)", async () => { + mockQuestion.mockResolvedValue("y"); + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + + // Omitting the confirmStepUp argument exercises the default + // confirmStepUpFromStdin, which reads from the mocked readline interface. + await handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + ); + + expect(mockQuestion).toHaveBeenCalled(); + expect(mockClose).toHaveBeenCalled(); + expect(runSpy).toHaveBeenCalled(); + }); + + it("accepts a whitespace-padded, upper-case 'YES'", async () => { + mockQuestion.mockResolvedValue(" YES "); + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + + await handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + ); + + expect(runSpy).toHaveBeenCalled(); + }); + + it("declines (throws) when the user answers n", async () => { + mockQuestion.mockResolvedValue("n"); + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + + await expect( + handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + ), + ).rejects.toThrow("Step-up authorization declined."); + + expect(mockClose).toHaveBeenCalled(); + expect(runSpy).not.toHaveBeenCalled(); + }); + }); + + describe("connectInspectorWithOAuth recovery branch", () => { + const oauthServerConfig = { + type: "streamable-http", + url: "https://as.example/mcp", + } as MCPServerConfig; + + it("resumes without re-auth when storage already satisfies the challenge", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const connect = vi + .fn() + .mockRejectedValueOnce( + new AuthRecoveryRequiredError( + new URL("https://as.example/authorize"), + { reason: "insufficient_scope", requiredScopes: ["weather:read"] }, + ), + ) + .mockResolvedValueOnce(undefined); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(true), + }; + + await connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ); + + expect(connect).toHaveBeenCalledTimes(2); + expect(runSpy).not.toHaveBeenCalled(); + }); + + it("runs interactive recovery when storage does not satisfy the challenge", async () => { + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + const connect = vi + .fn() + .mockRejectedValueOnce( + new AuthRecoveryRequiredError( + new URL("https://as.example/authorize"), + { reason: "token_expired" }, + ), + ) + .mockResolvedValueOnce(undefined); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), + }; + + await connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ); + + expect(connect).toHaveBeenCalledTimes(2); + expect(runSpy).toHaveBeenCalled(); + }); + + it("runs interactive OAuth on a plain unauthorized error (disconnect failure is swallowed)", async () => { + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + const connect = vi + .fn() + .mockRejectedValueOnce(new Error("Connection failed for server (401)")) + .mockResolvedValueOnce(undefined); + // A rejecting disconnect exercises the `.catch(() => {})` guard. + const client = { + connect, + disconnect: vi.fn().mockRejectedValue(new Error("disconnect failed")), + checkAuthChallengeSatisfied: vi.fn(), + }; + + await connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ); + + expect(client.disconnect).toHaveBeenCalled(); + expect(runSpy).toHaveBeenCalled(); + expect(connect).toHaveBeenCalledTimes(2); + }); + + it("rethrows a non-OAuth error unchanged", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const connect = vi + .fn() + .mockRejectedValue(new Error("some unrelated failure")); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }; + + await expect( + connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ), + ).rejects.toThrow("some unrelated failure"); + expect(runSpy).not.toHaveBeenCalled(); + }); + + it("rethrows when the server config is not OAuth-capable", async () => { + const connect = vi.fn().mockRejectedValue(new Error("nope (401)")); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }; + + await expect( + connectInspectorWithOAuth( + client, + { type: "stdio", command: "x" } as MCPServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ), + ).rejects.toThrow("nope (401)"); + }); + }); });