From 639273c4715bd488eea490882327d97ab9a71455 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Sun, 13 Sep 2026 09:57:45 +0200 Subject: [PATCH] fix(cli): strip terminal escapes from OAuth error text (GHSA-q9j4-4h4j-mv5m) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `argos login` printed the `error_description` of a failed OAuth callback verbatim. The authorization server is whatever `ARGOS_APP_BASE_URL` points at, so a hostile one could embed ANSI escape sequences and carriage returns that erase and repaint terminal lines, forging CLI output — a fake "run this command to continue" instruction under the CLI's own name. Sanitize remote text before it becomes an error message: drop escape sequences (CSI, OSC and the plain ones) along with the bytes that belong to them, turn the remaining control characters into spaces, drop the directional formatting that reorders what is displayed, collapse the result onto one line and bound its length. Applied to the loopback callback's `error`/`error_description` and to the token endpoint's error payload, which reaches the same terminal through `OAuthTokenError`; when nothing printable is left, the CLI falls back to a message of its own. Add unit tests for the sanitizer and a regression test driving the advisory's payload through the real callback server. Co-Authored-By: Claude Opus 5 --- packages/cli/src/commands/login.test.ts | 59 ++++++++++++++++++++++ packages/cli/src/commands/login.ts | 14 +++++- packages/cli/src/lib/oauth.ts | 9 +++- packages/cli/src/lib/terminal.test.ts | 64 +++++++++++++++++++++++ packages/cli/src/lib/terminal.ts | 67 +++++++++++++++++++++++++ 5 files changed, 210 insertions(+), 3 deletions(-) create mode 100644 packages/cli/src/commands/login.test.ts create mode 100644 packages/cli/src/lib/terminal.test.ts create mode 100644 packages/cli/src/lib/terminal.ts 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; +}