fix(cli): strip terminal escapes from OAuth error text (GHSA-q9j4-4h4j-mv5m)

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<string, string>) {
+  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");
+  });
+});

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<CallbackResult>;
 }> {
@@ -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",

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

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");
+  });
+});

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;
+}