CLI: catalog listing, auth flags, and OAuth browser UX (#1782)
* feat(cli): catalog listing, auth flags, and OAuth browser UX Add servers/list and servers/show, --relogin / --stored-auth-only, browser OAuth navigation with OSC 8 links, and extract method handlers under clients/cli/src/handlers. Tracks #1781. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): address #1782 review — relogin clear, stored-auth-only, docs Clear both raw and normalized OAuth keys on --relogin; check the shared store before bailing under --stored-auth-only mid-session; restore the tests README link target; document servers/show redaction limits; keep mcpi groundwork with TODO(#1432) markers. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): gate OAuth browser open and harden #1782 review follow-ups Arm auto-open only for CLI-owned interactive OAuth, honor MCP_AUTO_OPEN_ENABLED, move --relogin clear after validation, skip keychain on servers/list, and cover secret redaction after rehydrate. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): silence disarmed OAuth URLs and round-3 review nits Suppress authorize URL print when navigation is disarmed or --stored-auth-only; document MCP_AUTO_OPEN_ENABLED; redact Cookie/auth headers; keep servers/show unit tests off the OS keychain. Co-authored-by: Cursor <cursoragent@cursor.com> * test(cli): cover withArmedAutoOpen for the ≥90 branch gate CI failed on cliOAuth.ts branch coverage after the auto-open arming helper landed; exercise the armed path through runCliInteractiveOAuth. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): mid-RPC unauthorized recovery and clear round-4 nits Mirror connect's UnauthorizedError path in withCliAuthRecoveryRetry, let MCP_AUTO_OPEN_ENABLED=true force open off-TTY, mock open-url in the in-process runner, reject --relogin on catalog methods, restore the __tests__ README inventory, and narrow OAuth helpers to structural client types. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): reconnect after mid-RPC OAuth and finish round-5 nits Reconnect before retrying after unauthorized recovery, guard non-OAuth configs, decouple forceAutoOpen, move open-url mock to vitest setupFiles, and reject --relogin with stored-auth short-circuits. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): fail fast on non-TTY interactive OAuth and round-6 nits Skip interactive OAuth without a TTY unless MCP_AUTO_OPEN_ENABLED=true; document --stored-auth-only for CI; redact settings.metadata; reject --relogin for stdio; clarify AuthRecoveryRequired reconnect comments. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): admit interactive OAuth when stdin is a TTY (round-7) Gate on stdin||stderr so `2>&1 | tee` still works; fold confirm/auto-open/ TTY into CliOAuthConnectOptions; rename authRequiredFailure; document MCP_AUTO_OPEN_ENABLED=true as non-TTY admit. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): fail fast on step-up without stdin (round-8) Decline [y/N] on readline EOF/close, require a stdin TTY for the default step-up confirmer (force-open admit is not enough), document stderr-only browser auto-open vs stdin||stderr flow admit, and drop the alias wrapper. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): allow piped step-up answers; bound silent pipes (round-9) Drop the hard stdin-TTY gate that broke `echo y | …`. Keep EOF decline and add a short non-TTY timeout so an open pipe that never writes cannot hang. Clarify isTTY as admit-only; extract the readline mock factory. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): keep partial piped step-up answers (round-10) Capture readline `line` before close so `printf y` without a newline is not treated as decline; timeout is auth_required with a distinct message; document the 5s bound and post-confirm callback wait; pin the TTY no-timeout arm. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): tighten step-up timeout messaging and TTY test (round-11) Make the TTY no-timeout assertion use a settled flag (the Promise.race check was vacuous), say "timed out" instead of "declined" on the pipe bound, and document that piped answers need a newline or stdin close. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Bob Dickinson
GitHub
parent
a7b2fdb2ef
commit
33e88eac9f
1 changed files with 4041 additions and 520 deletions
250
clients/cli/__tests__/cli-oauth-navigation.test.ts
Normal file
250
clients/cli/__tests__/cli-oauth-navigation.test.ts
Normal file
|
|
@ -0,0 +1,250 @@
|
|||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
createCliOAuthNavigation,
|
||||
isCliAutoOpenForced,
|
||||
resolveCliAutoOpenEnabled,
|
||||
} from "../src/cli-oauth-navigation.js";
|
||||
import { openUrl } from "../src/open-url.js";
|
||||
|
||||
// Local mock (in addition to suite-wide setupFiles) so this file owns a
|
||||
// `vi.mocked(openUrl)` handle and does not depend on the global mock shape.
|
||||
vi.mock("../src/open-url.js", () => ({
|
||||
openUrl: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
|
||||
const openUrlMock = vi.mocked(openUrl);
|
||||
|
||||
describe("resolveCliAutoOpenEnabled", () => {
|
||||
it("honors MCP_AUTO_OPEN_ENABLED true/false and defaults off under VITEST", () => {
|
||||
expect(
|
||||
resolveCliAutoOpenEnabled({
|
||||
MCP_AUTO_OPEN_ENABLED: "true",
|
||||
VITEST: "true",
|
||||
}),
|
||||
).toBe(true);
|
||||
expect(
|
||||
resolveCliAutoOpenEnabled({
|
||||
MCP_AUTO_OPEN_ENABLED: "false",
|
||||
VITEST: undefined,
|
||||
}),
|
||||
).toBe(false);
|
||||
expect(resolveCliAutoOpenEnabled({ VITEST: "true" })).toBe(false);
|
||||
expect(resolveCliAutoOpenEnabled({})).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("isCliAutoOpenForced", () => {
|
||||
it("is true only for the explicit string true", () => {
|
||||
expect(isCliAutoOpenForced({ MCP_AUTO_OPEN_ENABLED: "true" })).toBe(true);
|
||||
expect(isCliAutoOpenForced({ MCP_AUTO_OPEN_ENABLED: "false" })).toBe(false);
|
||||
expect(isCliAutoOpenForced({})).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("createCliOAuthNavigation", () => {
|
||||
afterEach(() => {
|
||||
openUrlMock.mockClear();
|
||||
openUrlMock.mockResolvedValue(undefined);
|
||||
});
|
||||
|
||||
it("prints OSC 8 link and opens browser when armed on a TTY", async () => {
|
||||
const lines: string[] = [];
|
||||
const openBrowser = vi.fn().mockResolvedValue(undefined);
|
||||
const nav = createCliOAuthNavigation({
|
||||
isTTY: true,
|
||||
// Force ANSI on even when the test runner exports NO_COLOR.
|
||||
noColorEnv: "",
|
||||
write: (line) => lines.push(line),
|
||||
openBrowser,
|
||||
autoOpenControl: { armed: true },
|
||||
autoOpenEnabled: true,
|
||||
});
|
||||
const url = new URL("https://as.example/authorize?x=1");
|
||||
nav.navigateToAuthorization(url);
|
||||
await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce());
|
||||
expect(lines.join("")).toContain("Please navigate to:");
|
||||
expect(lines.join("")).toContain(
|
||||
"\u001b]8;;https://as.example/authorize?x=1\u0007",
|
||||
);
|
||||
expect(openBrowser).toHaveBeenCalledWith(
|
||||
"https://as.example/authorize?x=1",
|
||||
);
|
||||
expect(openUrlMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("is a silent no-op when disarmed (SDK-during-connect default)", async () => {
|
||||
const lines: string[] = [];
|
||||
const openBrowser = vi.fn();
|
||||
const nav = createCliOAuthNavigation({
|
||||
isTTY: true,
|
||||
noColorEnv: "1",
|
||||
write: (line) => lines.push(line),
|
||||
openBrowser,
|
||||
autoOpenEnabled: true,
|
||||
// no autoOpenControl → never armed
|
||||
});
|
||||
nav.navigateToAuthorization(new URL("https://as.example/authorize"));
|
||||
// Navigation is fire-and-forget; give the microtask a turn.
|
||||
await Promise.resolve();
|
||||
expect(lines).toEqual([]);
|
||||
expect(openBrowser).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("is a silent no-op when disableAutoOpen is set (stored-auth-only)", async () => {
|
||||
const lines: string[] = [];
|
||||
const openBrowser = vi.fn();
|
||||
const nav = createCliOAuthNavigation({
|
||||
isTTY: true,
|
||||
noColorEnv: "1",
|
||||
write: (line) => lines.push(line),
|
||||
openBrowser,
|
||||
autoOpenControl: { armed: true },
|
||||
autoOpenEnabled: true,
|
||||
disableAutoOpen: true,
|
||||
});
|
||||
nav.navigateToAuthorization(new URL("https://as.example/authorize"));
|
||||
await Promise.resolve();
|
||||
expect(lines).toEqual([]);
|
||||
expect(openBrowser).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("prints but does not open a browser when autoOpenEnabled is false", async () => {
|
||||
const lines: string[] = [];
|
||||
const openBrowser = vi.fn();
|
||||
const nav = createCliOAuthNavigation({
|
||||
isTTY: true,
|
||||
noColorEnv: "1",
|
||||
write: (line) => lines.push(line),
|
||||
openBrowser,
|
||||
autoOpenControl: { armed: true },
|
||||
autoOpenEnabled: false,
|
||||
});
|
||||
nav.navigateToAuthorization(new URL("https://as.example/authorize"));
|
||||
await vi.waitFor(() => expect(lines.length).toBe(1));
|
||||
expect(lines.join("")).toContain("Please navigate to:");
|
||||
expect(openBrowser).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("prints a plain URL and does not open a browser when not a TTY (unless forced)", async () => {
|
||||
const lines: string[] = [];
|
||||
const openBrowser = vi.fn();
|
||||
const nav = createCliOAuthNavigation({
|
||||
isTTY: false,
|
||||
write: (line) => lines.push(line),
|
||||
openBrowser,
|
||||
autoOpenControl: { armed: true },
|
||||
autoOpenEnabled: true,
|
||||
});
|
||||
nav.navigateToAuthorization(new URL("https://as.example/authorize"));
|
||||
await vi.waitFor(() => expect(lines.length).toBe(1));
|
||||
expect(lines.join("")).toBe(
|
||||
"Please navigate to: https://as.example/authorize\n",
|
||||
);
|
||||
expect(lines.join("")).not.toContain("\u001b]8;;");
|
||||
expect(openBrowser).not.toHaveBeenCalled();
|
||||
expect(openUrlMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("opens on a non-TTY when forceAutoOpen is set (MCP_AUTO_OPEN_ENABLED=true)", async () => {
|
||||
const lines: string[] = [];
|
||||
const openBrowser = vi.fn().mockResolvedValue(undefined);
|
||||
const nav = createCliOAuthNavigation({
|
||||
isTTY: false,
|
||||
noColorEnv: "1",
|
||||
write: (line) => lines.push(line),
|
||||
openBrowser,
|
||||
autoOpenControl: { armed: true },
|
||||
autoOpenEnabled: true,
|
||||
forceAutoOpen: true,
|
||||
});
|
||||
nav.navigateToAuthorization(new URL("https://as.example/authorize"));
|
||||
await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce());
|
||||
expect(lines.join("")).toContain("Please navigate to:");
|
||||
});
|
||||
|
||||
it("infers force-open from MCP_AUTO_OPEN_ENABLED=true when options omit overrides", async () => {
|
||||
const prev = process.env.MCP_AUTO_OPEN_ENABLED;
|
||||
process.env.MCP_AUTO_OPEN_ENABLED = "true";
|
||||
try {
|
||||
const openBrowser = vi.fn().mockResolvedValue(undefined);
|
||||
const nav = createCliOAuthNavigation({
|
||||
isTTY: false,
|
||||
noColorEnv: "1",
|
||||
write: () => {},
|
||||
openBrowser,
|
||||
autoOpenControl: { armed: true },
|
||||
});
|
||||
nav.navigateToAuthorization(new URL("https://as.example/authorize"));
|
||||
await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce());
|
||||
} finally {
|
||||
if (prev === undefined) {
|
||||
delete process.env.MCP_AUTO_OPEN_ENABLED;
|
||||
} else {
|
||||
process.env.MCP_AUTO_OPEN_ENABLED = prev;
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
it("skips OSC 8 when NO_COLOR is set but still opens on a TTY when armed", async () => {
|
||||
const lines: string[] = [];
|
||||
const openBrowser = vi.fn().mockResolvedValue(undefined);
|
||||
const nav = createCliOAuthNavigation({
|
||||
isTTY: true,
|
||||
noColorEnv: "1",
|
||||
write: (line) => lines.push(line),
|
||||
openBrowser,
|
||||
autoOpenControl: { armed: true },
|
||||
autoOpenEnabled: true,
|
||||
});
|
||||
nav.navigateToAuthorization(new URL("https://as.example/a"));
|
||||
await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce());
|
||||
expect(lines.join("")).toBe("Please navigate to: https://as.example/a\n");
|
||||
expect(lines.join("")).not.toContain("\u001b]8;;");
|
||||
});
|
||||
|
||||
it("swallows browser-open failures after printing the URL", async () => {
|
||||
const lines: string[] = [];
|
||||
const openBrowser = vi.fn().mockRejectedValue(new Error("no browser"));
|
||||
const nav = createCliOAuthNavigation({
|
||||
isTTY: true,
|
||||
noColorEnv: "1",
|
||||
write: (line) => lines.push(line),
|
||||
openBrowser,
|
||||
autoOpenControl: { armed: true },
|
||||
autoOpenEnabled: true,
|
||||
});
|
||||
nav.navigateToAuthorization(new URL("https://as.example/a"));
|
||||
await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce());
|
||||
expect(lines.join("")).toContain("Please navigate to:");
|
||||
});
|
||||
|
||||
it("writes to stderr and uses openUrl by default when armed on a TTY", async () => {
|
||||
const writeSpy = vi
|
||||
.spyOn(process.stderr, "write")
|
||||
.mockImplementation(() => true);
|
||||
const ttyDesc = Object.getOwnPropertyDescriptor(process.stderr, "isTTY");
|
||||
Object.defineProperty(process.stderr, "isTTY", {
|
||||
configurable: true,
|
||||
get: () => true,
|
||||
});
|
||||
try {
|
||||
const nav = createCliOAuthNavigation({
|
||||
noColorEnv: "1",
|
||||
autoOpenControl: { armed: true },
|
||||
autoOpenEnabled: true,
|
||||
});
|
||||
nav.navigateToAuthorization(new URL("https://as.example/default"));
|
||||
await vi.waitFor(() => expect(openUrlMock).toHaveBeenCalledOnce());
|
||||
expect(openUrlMock).toHaveBeenCalledWith("https://as.example/default");
|
||||
const written = writeSpy.mock.calls.map((c) => String(c[0])).join("");
|
||||
expect(written).toContain(
|
||||
"Please navigate to: https://as.example/default",
|
||||
);
|
||||
} finally {
|
||||
writeSpy.mockRestore();
|
||||
if (ttyDesc) {
|
||||
Object.defineProperty(process.stderr, "isTTY", ttyDesc);
|
||||
}
|
||||
}
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue