From 723c31e2c858c8bab3bfea0420cf4b2d2422183b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Sun, 13 Sep 2026 10:02:47 +0200 Subject: [PATCH 1/3] fix(core): redact secrets from debug output (GHSA-28pg-v3hp-9g7f) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DEBUG=@argos-ci/core` is the documented way to diagnose an upload, and its output is pasted into public issues and written to CI logs. It printed the whole `process.env` snapshot while detecting the CI environment — `GITHUB_TOKEN`, the OIDC request token and any secret a project defines itself — while the upload path printed its parameters and the resolved config, both carrying the `ARGOS_TOKEN` repository token. Redact in the logger rather than at each call site: every argument is copied with its credential-looking properties replaced before it reaches the debug package, so no caller has to remember which of its fields is a secret. The environment snapshot keeps the values of the variables CI detection reads, minus the credentials among them, and reduces every other variable to its name — which is what a "why wasn't my CI detected?" report needs, and leaks nothing the project defined itself. The hand-written token stripping in `deploy` and `uploadMedia` is the logger's job now, so it goes away. Co-Authored-By: Claude Opus 5 --- packages/core/src/ci-environment/index.ts | 3 +- packages/core/src/debug.test.ts | 43 ++++++++ packages/core/src/debug.ts | 20 +++- packages/core/src/deploy.ts | 3 +- packages/core/src/media.ts | 3 +- packages/core/src/redact.test.ts | 116 +++++++++++++++++++++ packages/core/src/redact.ts | 121 ++++++++++++++++++++++ 7 files changed, 303 insertions(+), 6 deletions(-) create mode 100644 packages/core/src/debug.test.ts create mode 100644 packages/core/src/redact.test.ts create mode 100644 packages/core/src/redact.ts diff --git a/packages/core/src/ci-environment/index.ts b/packages/core/src/ci-environment/index.ts index 72e461d2..86b90968 100644 --- a/packages/core/src/ci-environment/index.ts +++ b/packages/core/src/ci-environment/index.ts @@ -8,6 +8,7 @@ import gitlab from "./services/gitlab"; import git from "./services/git"; import type { CiEnvironment, Context } from "./types"; import { debug } from "../debug"; +import { redactEnv } from "../redact"; export type { CiEnvironment }; @@ -73,7 +74,7 @@ export function listAncestorCommits(input: { export async function getCiEnvironment(): Promise { const context = createContext(); - debug("Detecting CI environment", context); + debug("Detecting CI environment", { env: redactEnv(context.env) }); const service = getCiService(context); // Service matched diff --git a/packages/core/src/debug.test.ts b/packages/core/src/debug.test.ts new file mode 100644 index 00000000..2ad7adfb --- /dev/null +++ b/packages/core/src/debug.test.ts @@ -0,0 +1,43 @@ +import createDebug from "debug"; +import { describe, expect, it, vi } from "vitest"; +import { debug } from "./debug"; + +/** Run `log` with the namespace enabled and return what it wrote to stderr. */ +function captureDebugOutput(log: () => void): string { + const write = vi.spyOn(process.stderr, "write").mockReturnValue(true); + createDebug.enable("@argos-ci/core"); + try { + log(); + return write.mock.calls.map(([chunk]) => String(chunk)).join(""); + } finally { + createDebug.disable(); + write.mockRestore(); + } +} + +describe("debug", () => { + it("never prints a token it is handed (GHSA-28pg-v3hp-9g7f)", () => { + const output = captureDebugOutput(() => { + debug("Starting upload with params", { + token: "a".repeat(40), + commit: "0".repeat(40), + }); + }); + + expect(output).toContain("Starting upload with params"); + expect(output).toContain("[redacted]"); + expect(output).not.toContain("a".repeat(40)); + // The rest of the object is still there to debug with. + expect(output).toContain("0".repeat(40)); + }); + + it("writes nothing when the namespace is disabled", () => { + const write = vi.spyOn(process.stderr, "write").mockReturnValue(true); + try { + debug("Starting upload with params", { token: "a".repeat(40) }); + expect(write).not.toHaveBeenCalled(); + } finally { + write.mockRestore(); + } + }); +}); diff --git a/packages/core/src/debug.ts b/packages/core/src/debug.ts index f998a0b6..757d6a41 100644 --- a/packages/core/src/debug.ts +++ b/packages/core/src/debug.ts @@ -1,8 +1,26 @@ import createDebug from "debug"; +import { redactSecrets } from "./redact"; + const KEY = "@argos-ci/core"; -export const debug = createDebug(KEY); +const logger = createDebug(KEY); + +/** + * Log a line under `DEBUG=@argos-ci/core`. + * + * Arguments are redacted on the way in: this output is pasted into public + * issues and written to CI logs, so a credential reaching it is a published + * credential (GHSA-28pg-v3hp-9g7f). Callers pass whole objects — parameters, + * the resolved config, an API response — and must not have to remember which + * of their fields is a secret. + */ +export const debug = (message: unknown, ...args: unknown[]): void => { + if (!logger.enabled) { + return; + } + logger(redactSecrets(message), ...args.map((arg) => redactSecrets(arg))); +}; export const isDebugEnabled = createDebug.enabled(KEY); diff --git a/packages/core/src/deploy.ts b/packages/core/src/deploy.ts index 903575fe..0ea4f84b 100644 --- a/packages/core/src/deploy.ts +++ b/packages/core/src/deploy.ts @@ -58,8 +58,7 @@ export interface DeployParameters { * Deploy a static site (e.g. Storybook) to Argos. */ export async function deploy(params: DeployParameters) { - const { token: _token, ...debugParams } = params; - debug("Starting deploy with params", debugParams); + debug("Starting deploy with params", params); // Read config const config = await getConfigFromOptions(params); diff --git a/packages/core/src/media.ts b/packages/core/src/media.ts index 8c076c54..e1cbb439 100644 --- a/packages/core/src/media.ts +++ b/packages/core/src/media.ts @@ -135,8 +135,7 @@ export interface UploadMediaParameters { export async function uploadMedia( params: UploadMediaParameters, ): Promise { - const { token: _token, ...debugParams } = params; - debug("Starting media upload with params", debugParams); + debug("Starting media upload with params", params); if (params.files.length === 0) { throw new Error("No files to upload"); diff --git a/packages/core/src/redact.test.ts b/packages/core/src/redact.test.ts new file mode 100644 index 00000000..5c61a14f --- /dev/null +++ b/packages/core/src/redact.test.ts @@ -0,0 +1,116 @@ +import { describe, expect, it } from "vitest"; +import { redactEnv, redactSecrets } from "./redact"; + +describe("redactSecrets", () => { + it("redacts a credential wherever it sits", () => { + expect( + redactSecrets({ + token: "a".repeat(40), + apiBaseUrl: "https://api.argos-ci.com", + parallel: { nonce: "1", secret: "shh" }, + requests: [{ authorization: "Bearer aaa" }], + }), + ).toEqual({ + token: "[redacted]", + apiBaseUrl: "https://api.argos-ci.com", + parallel: { nonce: "1", secret: "[redacted]" }, + requests: [{ authorization: "[redacted]" }], + }); + }); + + it.each([ + "token", + "accessToken", + "refresh_token", + "ARGOS_TOKEN", + "apiKey", + "api_key", + "password", + "clientSecret", + "Authorization", + "cookie", + "signature", + ])("redacts the %s property", (key) => { + expect(redactSecrets({ [key]: "value" })).toEqual({ [key]: "[redacted]" }); + }); + + it("keeps an empty value, so an unresolved token stays visible", () => { + expect(redactSecrets({ token: null, project: "argos" })).toEqual({ + token: null, + project: "argos", + }); + }); + + it("leaves the value it was given untouched", () => { + const params = { token: "a".repeat(40) }; + redactSecrets(params); + expect(params.token).toBe("a".repeat(40)); + }); + + it("passes through what it cannot walk", () => { + const error = new Error("boom"); + expect(redactSecrets(error)).toBe(error); + expect(redactSecrets("plain")).toBe("plain"); + expect(redactSecrets(null)).toBeNull(); + expect(redactSecrets(undefined)).toBeUndefined(); + }); + + it("copies a cycle rather than walking it forever", () => { + const node: Record = { token: "a".repeat(40) }; + node.self = node; + + const redacted = redactSecrets(node) as Record; + + expect(redacted.token).toBe("[redacted]"); + expect(redacted.self).toBe(redacted); + }); +}); + +describe("redactEnv", () => { + it("keeps the variables CI detection reads", () => { + expect( + redactEnv({ + CI: "true", + GITHUB_ACTIONS: "true", + GITHUB_REPOSITORY: "argos-ci/argos-javascript", + GITHUB_SHA: "0".repeat(40), + ARGOS_BRANCH: "main", + }), + ).toEqual({ + CI: "true", + GITHUB_ACTIONS: "true", + GITHUB_REPOSITORY: "argos-ci/argos-javascript", + GITHUB_SHA: "0".repeat(40), + ARGOS_BRANCH: "main", + }); + }); + + it("redacts the credentials among them (GHSA-28pg-v3hp-9g7f)", () => { + expect( + redactEnv({ + ARGOS_TOKEN: "a".repeat(40), + GITHUB_TOKEN: "ghp_canary", + ACTIONS_ID_TOKEN_REQUEST_TOKEN: "oidc-request-token", + }), + ).toEqual({ + ARGOS_TOKEN: "[redacted]", + GITHUB_TOKEN: "[redacted]", + ACTIONS_ID_TOKEN_REQUEST_TOKEN: "[redacted]", + }); + }); + + it("reduces a variable it does not know to its name", () => { + expect(redactEnv({ MY_APP_CANARY: "s3cret", HOME: "/home/argos" })).toEqual( + { + MY_APP_CANARY: "[redacted]", + HOME: "[redacted]", + }, + ); + }); + + it("leaves out the variables that are not set", () => { + expect(redactEnv({ CI: "true", ARGOS_BRANCH: undefined })).toEqual({ + CI: "true", + }); + }); +}); diff --git a/packages/core/src/redact.ts b/packages/core/src/redact.ts new file mode 100644 index 00000000..d69819f0 --- /dev/null +++ b/packages/core/src/redact.ts @@ -0,0 +1,121 @@ +/** + * What `DEBUG=@argos-ci/core` is allowed to print. + * + * The documentation tells users to rerun a failing command with that flag and + * share the output, and CI writes it to build logs that are world-readable on + * public repositories. Everything the debug log prints is therefore effectively + * published — which is how the Argos repository token, the `GITHUB_TOKEN` and + * the OIDC request token came to leak (GHSA-28pg-v3hp-9g7f). + */ + +/** Printed in place of a value that must not be published. */ +const REDACTED = "[redacted]"; + +/** + * Property names that hold a credential, wherever they sit. Matched loosely on + * purpose: a false positive costs one line of debug output, a miss costs a + * token. + */ +const SECRET_KEY_REGEX = + /token|secret|password|passwd|credential|api[-_]?key|authorization|cookie|signature/i; + +/** + * Environment variables whose value the debug log keeps: the ones CI detection + * and the configuration actually read. Every other variable — a project's own + * secrets included — is reported by name only, which is all that "why wasn't my + * CI detected?" needs. + */ +const CI_ENV_PREFIXES = [ + "ACTIONS_", + "ARGOS_", + "BITRISE", + "BUILDKITE", + "CIRCLE", + "CI_", + "GITHUB_", + "GITLAB_", + "HEROKU_", + "TRAVIS", +]; + +/** CI variables that are not covered by a prefix. */ +const CI_ENV_NAMES = ["CI", "DISABLE_GITHUB_TOKEN_WARNING"]; + +function isSecretKey(key: string): boolean { + return SECRET_KEY_REGEX.test(key); +} + +function isCiVariable(name: string): boolean { + return ( + CI_ENV_NAMES.includes(name) || + CI_ENV_PREFIXES.some((prefix) => name.startsWith(prefix)) + ); +} + +function isPlainObject(value: object): boolean { + const prototype = Object.getPrototypeOf(value); + return prototype === Object.prototype || prototype === null; +} + +function redactValue(value: unknown, seen: Map): unknown { + if (typeof value !== "object" || value === null) { + return value; + } + + // A shape pointing back at itself is copied once and then shared, so a cycle + // stays a cycle instead of becoming an endless walk. + const copied = seen.get(value); + if (copied !== undefined) { + return copied; + } + + if (Array.isArray(value)) { + const copy: unknown[] = []; + seen.set(value, copy); + for (const item of value) { + copy.push(redactValue(item, seen)); + } + return copy; + } + + if (!isPlainObject(value)) { + return value; + } + + const copy: Record = {}; + seen.set(value, copy); + for (const [key, item] of Object.entries(value)) { + copy[key] = isSecretKey(key) && item ? REDACTED : redactValue(item, seen); + } + return copy; +} + +/** + * Copy `value` with every credential-looking property replaced. + * + * Only plain objects and arrays are walked: anything else — an `Error`, a + * `Buffer`, a class instance — is passed through untouched so the debug output + * keeps rendering it as it always did. An empty value is kept as it is too: a + * `token: null` says the token was never resolved, which is worth seeing. + */ +export function redactSecrets(value: unknown): unknown { + return redactValue(value, new Map()); +} + +/** + * The environment as the debug log may show it: CI variables keep their value, + * every other one is reduced to its name. + */ +export function redactEnv( + env: Record, +): Record { + const redacted: Record = {}; + for (const [name, value] of Object.entries(env)) { + if (value === undefined) { + continue; + } + redacted[name] = + isCiVariable(name) && !isSecretKey(name) ? value : REDACTED; + } + return redacted; +} From dfc9016a214cfdde155e2ed57465e3f986c2ba86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Sun, 13 Sep 2026 11:12:31 +0200 Subject: [PATCH 2/3] fix(core): stop printing the environment in debug output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allowlisting CI variables by prefix still printed whatever a project names `GITHUB_*`, `CI_*` or `ARGOS_*` itself, secrets included. Rather than maintain an exact list of the variables the detectors read, drop the environment from the debug log altogether: when a service matches, the next line already logs the resolved CI environment — commit, branch, repository, pull request — which is what the output is read for. A "no CI service matched" line takes over what the dump was worth when nothing matched. `redactSecrets` stays, covering the other sink: the upload parameters and the resolved config, both carrying `ARGOS_TOKEN`. It now walks property descriptors instead of `Object.entries`, so turning the debug flag on never calls a caller's getter — `util.inspect` renders an accessor as `[Getter]` without invoking it, and a getter that throws no longer takes the upload down with it. Co-Authored-By: Claude Opus 5 --- packages/core/src/ci-environment/index.ts | 4 +- packages/core/src/debug.test.ts | 16 ++++ packages/core/src/redact.test.ts | 81 ++++++++------------- packages/core/src/redact.ts | 89 +++++++---------------- 4 files changed, 76 insertions(+), 114 deletions(-) diff --git a/packages/core/src/ci-environment/index.ts b/packages/core/src/ci-environment/index.ts index 86b90968..fe267b8a 100644 --- a/packages/core/src/ci-environment/index.ts +++ b/packages/core/src/ci-environment/index.ts @@ -8,7 +8,6 @@ import gitlab from "./services/gitlab"; import git from "./services/git"; import type { CiEnvironment, Context } from "./types"; import { debug } from "../debug"; -import { redactEnv } from "../redact"; export type { CiEnvironment }; @@ -74,7 +73,7 @@ export function listAncestorCommits(input: { export async function getCiEnvironment(): Promise { const context = createContext(); - debug("Detecting CI environment", { env: redactEnv(context.env) }); + debug("Detecting CI environment"); const service = getCiService(context); // Service matched @@ -90,5 +89,6 @@ export async function getCiEnvironment(): Promise { return ciEnvironment; } + debug("No CI service matched"); return null; } diff --git a/packages/core/src/debug.test.ts b/packages/core/src/debug.test.ts index 2ad7adfb..5791177e 100644 --- a/packages/core/src/debug.test.ts +++ b/packages/core/src/debug.test.ts @@ -31,6 +31,22 @@ describe("debug", () => { expect(output).toContain("0".repeat(40)); }); + it("does not run a getter of the object it logs", () => { + let called = false; + const output = captureDebugOutput(() => { + debug("Starting upload with params", { + commit: "0".repeat(40), + get token() { + called = true; + throw new Error("boom"); + }, + }); + }); + + expect(called).toBe(false); + expect(output).toContain("[Getter]"); + }); + it("writes nothing when the namespace is disabled", () => { const write = vi.spyOn(process.stderr, "write").mockReturnValue(true); try { diff --git a/packages/core/src/redact.test.ts b/packages/core/src/redact.test.ts index 5c61a14f..c18da106 100644 --- a/packages/core/src/redact.test.ts +++ b/packages/core/src/redact.test.ts @@ -1,5 +1,6 @@ +import { inspect } from "node:util"; import { describe, expect, it } from "vitest"; -import { redactEnv, redactSecrets } from "./redact"; +import { redactSecrets } from "./redact"; describe("redactSecrets", () => { it("redacts a credential wherever it sits", () => { @@ -55,62 +56,42 @@ describe("redactSecrets", () => { expect(redactSecrets(undefined)).toBeUndefined(); }); - it("copies a cycle rather than walking it forever", () => { - const node: Record = { token: "a".repeat(40) }; - node.self = node; + it("never calls a getter it walks", () => { + let called = false; + const params = { + commit: "0".repeat(40), + get token() { + called = true; + return "a".repeat(40); + }, + }; - const redacted = redactSecrets(node) as Record; + const redacted = redactSecrets(params); - expect(redacted.token).toBe("[redacted]"); - expect(redacted.self).toBe(redacted); + expect(called).toBe(false); + // Rendered as `[Getter]`, the way the debug output always showed it. + expect(inspect(redacted)).toContain("[Getter]"); + expect(inspect(redacted)).not.toContain("a".repeat(40)); }); -}); -describe("redactEnv", () => { - it("keeps the variables CI detection reads", () => { - expect( - redactEnv({ - CI: "true", - GITHUB_ACTIONS: "true", - GITHUB_REPOSITORY: "argos-ci/argos-javascript", - GITHUB_SHA: "0".repeat(40), - ARGOS_BRANCH: "main", - }), - ).toEqual({ - CI: "true", - GITHUB_ACTIONS: "true", - GITHUB_REPOSITORY: "argos-ci/argos-javascript", - GITHUB_SHA: "0".repeat(40), - ARGOS_BRANCH: "main", - }); - }); + it("survives a getter that throws", () => { + const params = { + commit: "0".repeat(40), + get metadata(): unknown { + throw new Error("boom"); + }, + }; - it("redacts the credentials among them (GHSA-28pg-v3hp-9g7f)", () => { - expect( - redactEnv({ - ARGOS_TOKEN: "a".repeat(40), - GITHUB_TOKEN: "ghp_canary", - ACTIONS_ID_TOKEN_REQUEST_TOKEN: "oidc-request-token", - }), - ).toEqual({ - ARGOS_TOKEN: "[redacted]", - GITHUB_TOKEN: "[redacted]", - ACTIONS_ID_TOKEN_REQUEST_TOKEN: "[redacted]", - }); + expect(() => redactSecrets(params)).not.toThrow(); }); - it("reduces a variable it does not know to its name", () => { - expect(redactEnv({ MY_APP_CANARY: "s3cret", HOME: "/home/argos" })).toEqual( - { - MY_APP_CANARY: "[redacted]", - HOME: "[redacted]", - }, - ); - }); + it("copies a cycle rather than walking it forever", () => { + const node: Record = { token: "a".repeat(40) }; + node.self = node; - it("leaves out the variables that are not set", () => { - expect(redactEnv({ CI: "true", ARGOS_BRANCH: undefined })).toEqual({ - CI: "true", - }); + const redacted = redactSecrets(node) as Record; + + expect(redacted.token).toBe("[redacted]"); + expect(redacted.self).toBe(redacted); }); }); diff --git a/packages/core/src/redact.ts b/packages/core/src/redact.ts index d69819f0..2b68b26e 100644 --- a/packages/core/src/redact.ts +++ b/packages/core/src/redact.ts @@ -19,39 +19,10 @@ const REDACTED = "[redacted]"; const SECRET_KEY_REGEX = /token|secret|password|passwd|credential|api[-_]?key|authorization|cookie|signature/i; -/** - * Environment variables whose value the debug log keeps: the ones CI detection - * and the configuration actually read. Every other variable — a project's own - * secrets included — is reported by name only, which is all that "why wasn't my - * CI detected?" needs. - */ -const CI_ENV_PREFIXES = [ - "ACTIONS_", - "ARGOS_", - "BITRISE", - "BUILDKITE", - "CIRCLE", - "CI_", - "GITHUB_", - "GITLAB_", - "HEROKU_", - "TRAVIS", -]; - -/** CI variables that are not covered by a prefix. */ -const CI_ENV_NAMES = ["CI", "DISABLE_GITHUB_TOKEN_WARNING"]; - function isSecretKey(key: string): boolean { return SECRET_KEY_REGEX.test(key); } -function isCiVariable(name: string): boolean { - return ( - CI_ENV_NAMES.includes(name) || - CI_ENV_PREFIXES.some((prefix) => name.startsWith(prefix)) - ); -} - function isPlainObject(value: object): boolean { const prototype = Object.getPrototypeOf(value); return prototype === Object.prototype || prototype === null; @@ -69,23 +40,34 @@ function redactValue(value: unknown, seen: Map): unknown { return copied; } - if (Array.isArray(value)) { - const copy: unknown[] = []; - seen.set(value, copy); - for (const item of value) { - copy.push(redactValue(item, seen)); - } - return copy; - } - - if (!isPlainObject(value)) { + const isArray = Array.isArray(value); + if (!isArray && !isPlainObject(value)) { return value; } - const copy: Record = {}; + const copy: unknown[] | Record = isArray ? [] : {}; seen.set(value, copy); - for (const [key, item] of Object.entries(value)) { - copy[key] = isSecretKey(key) && item ? REDACTED : redactValue(item, seen); + + // Walked through its property descriptors rather than by reading it: turning + // the debug flag on must not run a caller's getter, let alone throw inside + // one and take the upload down with it. An accessor is copied as it is and + // stays uncalled — `util.inspect` renders it as `[Getter]`, which is what the + // debug output showed before anything was redacted at all. + for (const [key, descriptor] of Object.entries( + Object.getOwnPropertyDescriptors(value), + )) { + if (!descriptor.enumerable) { + continue; + } + if (!("value" in descriptor)) { + Object.defineProperty(copy, key, descriptor); + continue; + } + const item: unknown = descriptor.value; + Object.defineProperty(copy, key, { + ...descriptor, + value: isSecretKey(key) && item ? REDACTED : redactValue(item, seen), + }); } return copy; } @@ -93,29 +75,12 @@ function redactValue(value: unknown, seen: Map): unknown { /** * Copy `value` with every credential-looking property replaced. * - * Only plain objects and arrays are walked: anything else — an `Error`, a - * `Buffer`, a class instance — is passed through untouched so the debug output + * Only plain objects and arrays are walked, and only through their property + * descriptors, so no getter is ever called. Anything else — an `Error`, a + * `Buffer`, a class instance — is passed through untouched, so the debug output * keeps rendering it as it always did. An empty value is kept as it is too: a * `token: null` says the token was never resolved, which is worth seeing. */ export function redactSecrets(value: unknown): unknown { return redactValue(value, new Map()); } - -/** - * The environment as the debug log may show it: CI variables keep their value, - * every other one is reduced to its name. - */ -export function redactEnv( - env: Record, -): Record { - const redacted: Record = {}; - for (const [name, value] of Object.entries(env)) { - if (value === undefined) { - continue; - } - redacted[name] = - isCiVariable(name) && !isSecretKey(name) ? value : REDACTED; - } - return redacted; -} From 1e8bab24696bc4d6ae47e1bfd6e4805ed39e9b89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Sun, 13 Sep 2026 11:21:14 +0200 Subject: [PATCH 3/3] fix(core): mask the token in debug output instead of redacting everything MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the environment no longer logged, the only credential left reaching the debug output is the Argos token, and it gets there through three flat objects — not enough to justify a logger that walks every argument it is handed. Drop `redact.ts` and strip the token where it is logged, the way `deploy` and `uploadMedia` already did. The token is still worth seeing: "is that the token I think it is?" is a real question when an upload lands on the wrong project. `resolveArgosToken` now logs its first six characters — enough to recognize it, useless to anyone reading a public CI log. That is also the one place that knows which token the command ends up using, whether it came from the parameters or from the environment, so it is logged once there instead of being repeated by every object carrying it. Add a regression test driving the advisory's own PoC through `upload()`: the token never appears in the debug output, the masked prefix does. Co-Authored-By: Claude Opus 5 --- packages/core/src/auth.ts | 7 ++- packages/core/src/debug.test.ts | 59 ------------------- packages/core/src/debug.ts | 34 ++++++----- packages/core/src/deploy.ts | 3 +- packages/core/src/media.ts | 3 +- packages/core/src/redact.test.ts | 97 -------------------------------- packages/core/src/redact.ts | 86 ---------------------------- packages/core/src/upload.test.ts | 29 +++++++++- packages/core/src/upload.ts | 6 +- 9 files changed, 61 insertions(+), 263 deletions(-) delete mode 100644 packages/core/src/debug.test.ts delete mode 100644 packages/core/src/redact.test.ts delete mode 100644 packages/core/src/redact.ts diff --git a/packages/core/src/auth.ts b/packages/core/src/auth.ts index 8e44d590..222ffb80 100644 --- a/packages/core/src/auth.ts +++ b/packages/core/src/auth.ts @@ -7,7 +7,7 @@ import { exchangeGitHubActionsTokenlessToken, } from "./github-actions-tokenless"; import type { Config } from "./config"; -import { debug } from "./debug"; +import { debug, maskToken } from "./debug"; /** * Resolve the Argos authentication token. @@ -15,7 +15,10 @@ import { debug } from "./debug"; */ export async function resolveArgosToken(config: Config): Promise { if (config.token) { - debug("Authenticated with ARGOS_TOKEN."); + // Masked, and logged here only: this is the one place that knows which + // token the command ends up using, whether it came from the parameters or + // from the environment. + debug(`Authenticated with ARGOS_TOKEN (${maskToken(config.token)}).`); return config.token; } diff --git a/packages/core/src/debug.test.ts b/packages/core/src/debug.test.ts deleted file mode 100644 index 5791177e..00000000 --- a/packages/core/src/debug.test.ts +++ /dev/null @@ -1,59 +0,0 @@ -import createDebug from "debug"; -import { describe, expect, it, vi } from "vitest"; -import { debug } from "./debug"; - -/** Run `log` with the namespace enabled and return what it wrote to stderr. */ -function captureDebugOutput(log: () => void): string { - const write = vi.spyOn(process.stderr, "write").mockReturnValue(true); - createDebug.enable("@argos-ci/core"); - try { - log(); - return write.mock.calls.map(([chunk]) => String(chunk)).join(""); - } finally { - createDebug.disable(); - write.mockRestore(); - } -} - -describe("debug", () => { - it("never prints a token it is handed (GHSA-28pg-v3hp-9g7f)", () => { - const output = captureDebugOutput(() => { - debug("Starting upload with params", { - token: "a".repeat(40), - commit: "0".repeat(40), - }); - }); - - expect(output).toContain("Starting upload with params"); - expect(output).toContain("[redacted]"); - expect(output).not.toContain("a".repeat(40)); - // The rest of the object is still there to debug with. - expect(output).toContain("0".repeat(40)); - }); - - it("does not run a getter of the object it logs", () => { - let called = false; - const output = captureDebugOutput(() => { - debug("Starting upload with params", { - commit: "0".repeat(40), - get token() { - called = true; - throw new Error("boom"); - }, - }); - }); - - expect(called).toBe(false); - expect(output).toContain("[Getter]"); - }); - - it("writes nothing when the namespace is disabled", () => { - const write = vi.spyOn(process.stderr, "write").mockReturnValue(true); - try { - debug("Starting upload with params", { token: "a".repeat(40) }); - expect(write).not.toHaveBeenCalled(); - } finally { - write.mockRestore(); - } - }); -}); diff --git a/packages/core/src/debug.ts b/packages/core/src/debug.ts index 757d6a41..5f2e9191 100644 --- a/packages/core/src/debug.ts +++ b/packages/core/src/debug.ts @@ -1,26 +1,32 @@ import createDebug from "debug"; -import { redactSecrets } from "./redact"; - const KEY = "@argos-ci/core"; -const logger = createDebug(KEY); +export const debug = createDebug(KEY); /** - * Log a line under `DEBUG=@argos-ci/core`. + * Leading characters of a token kept in the debug output: enough to tell two + * tokens apart, far too few to use. + */ +const TOKEN_PREVIEW_LENGTH = 6; + +/** + * Show which token is in play without publishing it. * - * Arguments are redacted on the way in: this output is pasted into public - * issues and written to CI logs, so a credential reaching it is a published - * credential (GHSA-28pg-v3hp-9g7f). Callers pass whole objects — parameters, - * the resolved config, an API response — and must not have to remember which - * of their fields is a secret. + * Debug output is the documented way to report an upload problem: it gets + * pasted into public issues and written to CI logs, which are world-readable on + * public repositories, so a token printed in full is a published token + * (GHSA-28pg-v3hp-9g7f). The first few characters answer "is that the token I + * think it is?" and are useless to anyone else. */ -export const debug = (message: unknown, ...args: unknown[]): void => { - if (!logger.enabled) { - return; +export function maskToken( + token: string | null | undefined, +): string | null | undefined { + if (!token) { + return token; } - logger(redactSecrets(message), ...args.map((arg) => redactSecrets(arg))); -}; + return `${token.slice(0, TOKEN_PREVIEW_LENGTH)}…`; +} export const isDebugEnabled = createDebug.enabled(KEY); diff --git a/packages/core/src/deploy.ts b/packages/core/src/deploy.ts index 0ea4f84b..903575fe 100644 --- a/packages/core/src/deploy.ts +++ b/packages/core/src/deploy.ts @@ -58,7 +58,8 @@ export interface DeployParameters { * Deploy a static site (e.g. Storybook) to Argos. */ export async function deploy(params: DeployParameters) { - debug("Starting deploy with params", params); + const { token: _token, ...debugParams } = params; + debug("Starting deploy with params", debugParams); // Read config const config = await getConfigFromOptions(params); diff --git a/packages/core/src/media.ts b/packages/core/src/media.ts index e1cbb439..8c076c54 100644 --- a/packages/core/src/media.ts +++ b/packages/core/src/media.ts @@ -135,7 +135,8 @@ export interface UploadMediaParameters { export async function uploadMedia( params: UploadMediaParameters, ): Promise { - debug("Starting media upload with params", params); + const { token: _token, ...debugParams } = params; + debug("Starting media upload with params", debugParams); if (params.files.length === 0) { throw new Error("No files to upload"); diff --git a/packages/core/src/redact.test.ts b/packages/core/src/redact.test.ts deleted file mode 100644 index c18da106..00000000 --- a/packages/core/src/redact.test.ts +++ /dev/null @@ -1,97 +0,0 @@ -import { inspect } from "node:util"; -import { describe, expect, it } from "vitest"; -import { redactSecrets } from "./redact"; - -describe("redactSecrets", () => { - it("redacts a credential wherever it sits", () => { - expect( - redactSecrets({ - token: "a".repeat(40), - apiBaseUrl: "https://api.argos-ci.com", - parallel: { nonce: "1", secret: "shh" }, - requests: [{ authorization: "Bearer aaa" }], - }), - ).toEqual({ - token: "[redacted]", - apiBaseUrl: "https://api.argos-ci.com", - parallel: { nonce: "1", secret: "[redacted]" }, - requests: [{ authorization: "[redacted]" }], - }); - }); - - it.each([ - "token", - "accessToken", - "refresh_token", - "ARGOS_TOKEN", - "apiKey", - "api_key", - "password", - "clientSecret", - "Authorization", - "cookie", - "signature", - ])("redacts the %s property", (key) => { - expect(redactSecrets({ [key]: "value" })).toEqual({ [key]: "[redacted]" }); - }); - - it("keeps an empty value, so an unresolved token stays visible", () => { - expect(redactSecrets({ token: null, project: "argos" })).toEqual({ - token: null, - project: "argos", - }); - }); - - it("leaves the value it was given untouched", () => { - const params = { token: "a".repeat(40) }; - redactSecrets(params); - expect(params.token).toBe("a".repeat(40)); - }); - - it("passes through what it cannot walk", () => { - const error = new Error("boom"); - expect(redactSecrets(error)).toBe(error); - expect(redactSecrets("plain")).toBe("plain"); - expect(redactSecrets(null)).toBeNull(); - expect(redactSecrets(undefined)).toBeUndefined(); - }); - - it("never calls a getter it walks", () => { - let called = false; - const params = { - commit: "0".repeat(40), - get token() { - called = true; - return "a".repeat(40); - }, - }; - - const redacted = redactSecrets(params); - - expect(called).toBe(false); - // Rendered as `[Getter]`, the way the debug output always showed it. - expect(inspect(redacted)).toContain("[Getter]"); - expect(inspect(redacted)).not.toContain("a".repeat(40)); - }); - - it("survives a getter that throws", () => { - const params = { - commit: "0".repeat(40), - get metadata(): unknown { - throw new Error("boom"); - }, - }; - - expect(() => redactSecrets(params)).not.toThrow(); - }); - - it("copies a cycle rather than walking it forever", () => { - const node: Record = { token: "a".repeat(40) }; - node.self = node; - - const redacted = redactSecrets(node) as Record; - - expect(redacted.token).toBe("[redacted]"); - expect(redacted.self).toBe(redacted); - }); -}); diff --git a/packages/core/src/redact.ts b/packages/core/src/redact.ts deleted file mode 100644 index 2b68b26e..00000000 --- a/packages/core/src/redact.ts +++ /dev/null @@ -1,86 +0,0 @@ -/** - * What `DEBUG=@argos-ci/core` is allowed to print. - * - * The documentation tells users to rerun a failing command with that flag and - * share the output, and CI writes it to build logs that are world-readable on - * public repositories. Everything the debug log prints is therefore effectively - * published — which is how the Argos repository token, the `GITHUB_TOKEN` and - * the OIDC request token came to leak (GHSA-28pg-v3hp-9g7f). - */ - -/** Printed in place of a value that must not be published. */ -const REDACTED = "[redacted]"; - -/** - * Property names that hold a credential, wherever they sit. Matched loosely on - * purpose: a false positive costs one line of debug output, a miss costs a - * token. - */ -const SECRET_KEY_REGEX = - /token|secret|password|passwd|credential|api[-_]?key|authorization|cookie|signature/i; - -function isSecretKey(key: string): boolean { - return SECRET_KEY_REGEX.test(key); -} - -function isPlainObject(value: object): boolean { - const prototype = Object.getPrototypeOf(value); - return prototype === Object.prototype || prototype === null; -} - -function redactValue(value: unknown, seen: Map): unknown { - if (typeof value !== "object" || value === null) { - return value; - } - - // A shape pointing back at itself is copied once and then shared, so a cycle - // stays a cycle instead of becoming an endless walk. - const copied = seen.get(value); - if (copied !== undefined) { - return copied; - } - - const isArray = Array.isArray(value); - if (!isArray && !isPlainObject(value)) { - return value; - } - - const copy: unknown[] | Record = isArray ? [] : {}; - seen.set(value, copy); - - // Walked through its property descriptors rather than by reading it: turning - // the debug flag on must not run a caller's getter, let alone throw inside - // one and take the upload down with it. An accessor is copied as it is and - // stays uncalled — `util.inspect` renders it as `[Getter]`, which is what the - // debug output showed before anything was redacted at all. - for (const [key, descriptor] of Object.entries( - Object.getOwnPropertyDescriptors(value), - )) { - if (!descriptor.enumerable) { - continue; - } - if (!("value" in descriptor)) { - Object.defineProperty(copy, key, descriptor); - continue; - } - const item: unknown = descriptor.value; - Object.defineProperty(copy, key, { - ...descriptor, - value: isSecretKey(key) && item ? REDACTED : redactValue(item, seen), - }); - } - return copy; -} - -/** - * Copy `value` with every credential-looking property replaced. - * - * Only plain objects and arrays are walked, and only through their property - * descriptors, so no getter is ever called. Anything else — an `Error`, a - * `Buffer`, a class instance — is passed through untouched, so the debug output - * keeps rendering it as it always did. An empty value is kept as it is too: a - * `token: null` says the token was never resolved, which is worth seeing. - */ -export function redactSecrets(value: unknown): unknown { - return redactValue(value, new Map()); -} diff --git a/packages/core/src/upload.test.ts b/packages/core/src/upload.test.ts index d68ba32c..724854f4 100644 --- a/packages/core/src/upload.test.ts +++ b/packages/core/src/upload.test.ts @@ -1,4 +1,5 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi } from "vitest"; +import createDebug from "debug"; import { join } from "node:path"; import { upload } from "./upload"; import { server, setupMockServer } from "../mocks/server"; @@ -292,4 +293,30 @@ describe("#upload", () => { expect(receivedMergeQueuePrNumbers).toEqual([12, 34]); })(); }); + + it("never prints the token in debug output (GHSA-28pg-v3hp-9g7f)", async () => { + const token = "92d832e0d22ab113c8979d73a87a11130eaa24a9"; + const write = vi.spyOn(process.stderr, "write").mockReturnValue(true); + createDebug.enable("@argos-ci/core"); + + let output: string; + try { + await upload({ + branch: "main", + apiBaseUrl: "https://api.argos-ci.dev", + root: join(__dirname, "../../../__fixtures__/screenshots"), + commit: "f16f980bd17cccfa93a1ae7766727e67950773d0", + token, + }); + output = write.mock.calls.map(([chunk]) => String(chunk)).join(""); + } finally { + createDebug.disable(); + write.mockRestore(); + } + + expect(output).not.toContain(token); + // Masked once, where the token is resolved, so the user can still tell + // which one was used. + expect(output).toContain("Authenticated with ARGOS_TOKEN (92d832…)"); + }); }); diff --git a/packages/core/src/upload.ts b/packages/core/src/upload.ts index 892ed0cb..5bf9b215 100644 --- a/packages/core/src/upload.ts +++ b/packages/core/src/upload.ts @@ -188,7 +188,8 @@ export async function upload(params: UploadParameters): Promise<{ build: ArgosAPISchema.components["schemas"]["Build"]; screenshots: Screenshot[]; }> { - debug("Starting upload with params", params); + const { token: _paramsToken, ...debugParams } = params; + debug("Starting upload with params", debugParams); // Read config const [config, argosSdk] = await Promise.all([ @@ -213,7 +214,8 @@ export async function upload(params: UploadParameters): Promise<{ (config.previewBaseUrl ? { baseUrl: config.previewBaseUrl } : undefined); const globs = params.files ?? ["**/*.{png,jpg,jpeg}"]; - debug("Using config and files", config, globs); + const { token: _configToken, ...debugConfig } = config; + debug("Using config and files", debugConfig, globs); // Collect snapshots const files = await discoverSnapshots(globs, {