diff --git a/packages/cli/src/commands/login.test.ts b/packages/cli/src/commands/login.test.ts new file mode 100644 index 00000000..d58c17b1 --- /dev/null +++ b/packages/cli/src/commands/login.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from "vitest"; +import { startCallbackServer } from "./login"; + +const ESC = "\u001B"; + +/** Hit the loopback callback the way the browser would, without following the redirect. */ +function callback(port: number, params: Record) { + const url = new URL(`http://127.0.0.1:${port}/callback`); + for (const [key, value] of Object.entries(params)) { + url.searchParams.set(key, value); + } + return fetch(url, { redirect: "manual" }); +} + +describe("startCallbackServer", () => { + it("resolves with the code and state of a successful callback", async () => { + const { port, waitForCallback } = await startCallbackServer(); + const pending = waitForCallback(); + + const response = await callback(port, { + code: "the-code", + state: "the-state", + }); + expect(response.status).toBe(302); + await expect(pending).resolves.toEqual({ + code: "the-code", + state: "the-state", + }); + }); + + it("rejects with an error description the terminal cannot act on (GHSA-q9j4-4h4j-mv5m)", async () => { + const { port, waitForCallback } = await startCallbackServer(); + // Attached before the request so the rejection is never unhandled. + const failure = waitForCallback().catch((err: unknown) => err); + + const response = await callback(port, { + error: "access_denied", + error_description: "\u001B[2K\r[FAKE] Security alert: run evil.sh", + }); + expect(response.status).toBe(400); + + const error = await failure; + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toBe("[FAKE] Security alert: run evil.sh"); + expect((error as Error).message).not.toContain(ESC); + }); + + it("falls back to its own message when the description is only escape sequences", async () => { + const { port, waitForCallback } = await startCallbackServer(); + const failure = waitForCallback().catch((err: unknown) => err); + + await callback(port, { + error: "access_denied", + error_description: "\u001B[2K\u001B[1A", + }); + + expect(((await failure) as Error).message).toBe("Authorization failed"); + }); +}); diff --git a/packages/cli/src/commands/login.ts b/packages/cli/src/commands/login.ts index cb87eb2b..86086dd2 100644 --- a/packages/cli/src/commands/login.ts +++ b/packages/cli/src/commands/login.ts @@ -10,6 +10,7 @@ import { generatePkce, getAppBaseUrl, } from "../lib/oauth"; +import { sanitizeTerminalText } from "../lib/terminal"; const LOGIN_CLI_SUCCESS_ROUTE = `/auth/cli/success`; @@ -40,7 +41,11 @@ const successColor = (text: string) => color(text, 32, process.stdout.isTTY); const warningColor = (text: string) => color(text, 33, process.stderr.isTTY); const errorColor = (text: string) => color(text, 31, process.stderr.isTTY); -function startCallbackServer(): Promise<{ +/** + * Listen on an ephemeral loopback port for the OAuth redirect, and resolve with + * the authorization code it carries. + */ +export function startCallbackServer(): Promise<{ port: number; waitForCallback: () => Promise; }> { @@ -78,7 +83,12 @@ function startCallbackServer(): Promise<{ const state = url.searchParams.get("state"); if (callbackError) { - const message = callbackErrorDescription ?? callbackError; + // Both values are chosen by the authorization server — whatever + // `ARGOS_APP_BASE_URL` points at — and end up on the user's terminal, + // so they are stripped of anything it would act on rather than display. + const message = + sanitizeTerminalText(callbackErrorDescription ?? callbackError) || + "Authorization failed"; res.writeHead(400, { "Content-Type": "text/html; charset=utf-8", Connection: "close", diff --git a/packages/cli/src/lib/oauth.ts b/packages/cli/src/lib/oauth.ts index 1611bfe9..299932c1 100644 --- a/packages/cli/src/lib/oauth.ts +++ b/packages/cli/src/lib/oauth.ts @@ -1,5 +1,7 @@ import { createHash, randomBytes } from "node:crypto"; +import { sanitizeTerminalText } from "./terminal"; + /** * OAuth 2.1 client configuration and helpers for the `argos login` flow * (Authorization Code + PKCE with a loopback redirect, RFC 8252). @@ -121,8 +123,13 @@ async function postToken( .json() .catch(() => null)) as TokenEndpointResponse | null; if (!response.ok || !data?.access_token) { + // The description is written by the authorization server and printed to the + // terminal by the caller, so it is sanitized here rather than at the print + // site — an `Error` message carrying escape sequences would spoof output + // anywhere it surfaces. const message = - data?.error_description ?? data?.error ?? `HTTP ${response.status}`; + sanitizeTerminalText(data?.error_description ?? data?.error) || + `HTTP ${response.status}`; throw new OAuthTokenError(message, data?.error); } // A response without a refresh token would be persisted as a token set with diff --git a/packages/cli/src/lib/terminal.test.ts b/packages/cli/src/lib/terminal.test.ts new file mode 100644 index 00000000..06a00382 --- /dev/null +++ b/packages/cli/src/lib/terminal.test.ts @@ -0,0 +1,64 @@ +import { describe, expect, it } from "vitest"; +import { sanitizeTerminalText } from "./terminal"; + +const ESC = "\u001B"; + +describe("sanitizeTerminalText", () => { + it("leaves ordinary text alone", () => { + expect(sanitizeTerminalText("Accès refusé — try again.")).toBe( + "Accès refusé — try again.", + ); + }); + + it("strips the line-erasing payload of GHSA-q9j4-4h4j-mv5m", () => { + expect( + sanitizeTerminalText("\u001B[2K\r[FAKE] Security alert: run evil.sh"), + ).toBe("[FAKE] Security alert: run evil.sh"); + }); + + it.each([ + ["a color sequence", "\u001B[31mred\u001B[0m", "red"], + ["a cursor move", "before\u001B[5Aafter", "beforeafter"], + ["an OSC window title", "\u001B]0;pwned\u0007done", "done"], + ["a two-character escape", "reset\u001Bcnow", "resetnow"], + ])("removes %s", (_label, text, expected) => { + expect(sanitizeTerminalText(text)).toBe(expected); + }); + + it("never leaves an escape character behind", () => { + const sanitized = sanitizeTerminalText( + "\u001B[?25l\u001B[1;31mwarning\u001B[0m\u001B[2J", + ); + expect(sanitized).not.toContain(ESC); + }); + + it("turns the characters a terminal acts on into spaces", () => { + expect(sanitizeTerminalText("first\rsecond\nthird\tfourth")).toBe( + "first second third fourth", + ); + }); + + it("drops the directional formatting that reorders what is displayed", () => { + expect(sanitizeTerminalText("run \u202Ehs.live\u2069 now")).toBe( + "run hs.live now", + ); + }); + + it("truncates an overlong description", () => { + expect(sanitizeTerminalText("a".repeat(300))).toBe(`${"a".repeat(200)}…`); + }); + + it.each([ + ["null", null], + ["undefined", undefined], + ["empty", ""], + ["only escapes", "\u001B[2K\u001B[1A"], + ["only whitespace", " \r\n\t "], + ])("returns an empty string when the text is %s", (_label, text) => { + expect(sanitizeTerminalText(text)).toBe(""); + }); + + it("coerces a value the server sent as something other than a string", () => { + expect(sanitizeTerminalText(42)).toBe("42"); + }); +}); diff --git a/packages/cli/src/lib/terminal.ts b/packages/cli/src/lib/terminal.ts new file mode 100644 index 00000000..a9363e04 --- /dev/null +++ b/packages/cli/src/lib/terminal.ts @@ -0,0 +1,67 @@ +/** + * Printing text the CLI did not author — an OAuth error description chosen by + * the authorization server, for instance. + * + * A terminal acts on what it is given: an ANSI escape sequence moves the + * cursor, erases a line or repaints it, and a carriage return overwrites the + * line just printed. Remote text reaching it unfiltered therefore doesn't just + * read badly, it can forge CLI output — a fake "run this command to continue" + * instruction under the CLI's own name (GHSA-q9j4-4h4j-mv5m). + */ + +/** + * Longest remote string we print. A description is a sentence; a wall of text + * is itself a way to scroll the real message off the screen. + */ +const MAX_LENGTH = 200; + +/** + * Escape sequences, matched together with the bytes that belong to them so no + * parameter remnant (`[2K`) is left behind as text: CSI (`ESC [`), OSC + * (`ESC ]`, up to its terminator) and, last, the plain escape sequences — + * optional intermediate bytes and a final one, which covers `ESC c` (reset the + * terminal) as much as `ESC 7`. + */ +const ESCAPE_SEQUENCE_REGEX = + // eslint-disable-next-line no-control-regex + /[\u001B\u009B](?:\[[0-?]*[ -/]*[@-~]|\][^\u0007\u001B]*(?:\u0007|\u001B\\)?|[ -/]*[0-~])/g; + +/** Anything else a terminal acts on rather than displays: C0, DEL and C1. */ +// eslint-disable-next-line no-control-regex +const CONTROL_CHARACTER_REGEX = /[\u0000-\u001F\u007F-\u009F]/g; + +/** + * Explicit directional formatting: the other way to make a line read as + * something other than what it says, by reordering the characters around it + * rather than by driving the terminal. Bidirectional text itself is untouched — + * only the overrides, embeddings, isolates and marks are dropped. + */ +const DIRECTIONAL_FORMATTING_REGEX = + /[\u061C\u200E\u200F\u202A-\u202E\u2066-\u2069]/g; + +/** + * Reduce remote text to characters a terminal can only display, on a single + * line and bounded in length. + * + * Returns an empty string when nothing printable is left, so callers can fall + * back to a message of their own rather than print a blank error. + * + * @param text The remote value. Typed `unknown` because it comes from parsed + * JSON or a URL parameter and is not necessarily a string at runtime. + */ +export function sanitizeTerminalText(text: unknown): string { + if (text === null || text === undefined) { + return ""; + } + const sanitized = String(text) + .replace(ESCAPE_SEQUENCE_REGEX, "") + .replace(DIRECTIONAL_FORMATTING_REGEX, "") + // Replaced rather than dropped: removing the separator in `foo\rbar` would + // splice two words into one. + .replace(CONTROL_CHARACTER_REGEX, " ") + .replace(/\s+/g, " ") + .trim(); + return sanitized.length > MAX_LENGTH + ? `${sanitized.slice(0, MAX_LENGTH)}…` + : sanitized; +}