From 4a1089a6dbc493e4180dd9c0d7322f52356de90a Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 20:23:51 -0500 Subject: [PATCH 1/4] fix(cli): keep one log call on one line, and settle --summary before the run A line break in text a settings file or GitHub supplied ended the log line, and what followed opened the next one. A GitHub Actions runner reads a line that starts with two colons as a workflow command, so a label name in a target repository's own settings file could issue one from the admin's run. The action's log and the CLI's log now write CR and LF as %0D and %0A, the spelling annotations already use, so one log call is one line whatever its producer. The CLI redacts before it escapes, so a masked value that spans lines still matches. --summary into a directory that did not exist threw when the summary was appended, after the run's requests had landed, and the run ended "stopped unexpectedly" with exit 1. The path is now settled before the run: its directories are created, as the rendered and snapshot files create theirs, and an append is proved possible without leaving a file behind. A path nothing can append to is refused before any request, naming --summary. The test of the unexpected-stop path used that crash as its vehicle. It now reaches the same path through a client factory that throws. --- docs/start/cli.md | 12 ++-- src/action/io.ts | 11 +++- src/cli/actions.ts | 11 +++- src/cli/io.ts | 58 +++++++++++++++++-- src/cli/program.ts | 32 +++++++++-- src/io.ts | 4 ++ test/cli/actions.test.ts | 74 ++++++++++++++++++++---- test/cli/io.test.ts | 60 ++++++++++++++++++- test/cli/program.test.ts | 121 +++++++++++++++++++++++++++++---------- 9 files changed, 319 insertions(+), 64 deletions(-) diff --git a/docs/start/cli.md b/docs/start/cli.md index 506e1e81e..44cb554ee 100644 --- a/docs/start/cli.md +++ b/docs/start/cli.md @@ -140,12 +140,12 @@ Two inputs have no command-line form: `--private-report artifact` is refused (th Four flags are the command line's own, and `init` has one more, `--force` (replace an existing settings file): -| Flag | Does | -| ------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `--token ` | The token; `GITHUB_TOKEN` when absent | -| `--json` | Print the outputs as one JSON object on stdout; log lines move to stderr | -| `--summary ` | Append the run's markdown summary (the action's step summary) to this file; under GitHub Actions the step summary itself when absent; `init`, `validate`, and `permissions` have none | -| `--verbose` | Show the debug trace on stderr | +| Flag | Does | +| ------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `--token ` | The token; `GITHUB_TOKEN` when absent | +| `--json` | Print the outputs as one JSON object on stdout; log lines move to stderr | +| `--summary ` | Append the run's markdown summary (the action's step summary) to this file, creating its directories; a path that cannot be written is refused before any request; under GitHub Actions the step summary itself when absent; `init`, `validate`, and `permissions` have none | +| `--verbose` | Show the debug trace on stderr | ## Output and exit codes diff --git a/src/action/io.ts b/src/action/io.ts index c985b2e60..a8d473f73 100644 --- a/src/action/io.ts +++ b/src/action/io.ts @@ -43,9 +43,18 @@ function appendSummary(markdown: string): void { appendFileSync(file, `${markdown}\n`); } +/** + * `line` with each line break in the runner's spelling (%0D, %0A), as @actions/core escapes a command's data. The + * runner ends a line at CR or LF and reads a line starting `::` as a workflow command, so text after a raw break + * would issue one. `%` stays as written: nothing decodes a log line. test/cli/actions.test.ts holds the CLI's log to this form. + */ +function oneLine(line: string): string { + return line.replace(/\r/g, "%0D").replace(/\n/g, "%0A"); +} + export const actionsIo: Io = { annotate, - log: (line) => console.log(line), + log: (line) => console.log(oneLine(line)), debug: (line) => core.debug(line), summary: appendSummary, output: setOutput, diff --git a/src/cli/actions.ts b/src/cli/actions.ts index f3cefdcdc..9c0178a44 100644 --- a/src/cli/actions.ts +++ b/src/cli/actions.ts @@ -35,10 +35,17 @@ export function actionsRunner( }; } +/** + * `text` on one line, each line break in the runner's spelling (%0D, %0A). The runner ends a line at CR or LF and + * reads a line starting `::` as a workflow command, so text after a raw break would open a line of its own. + */ +export function oneLine(text: string): string { + return text.replace(/\r/g, "%0D").replace(/\n/g, "%0A"); +} + /** One `::name::message` line with the runner's data escaping (%, CR, LF), as @actions/core issues it. */ export function workflowCommand(name: "add-mask" | AnnotationLevel, message: string): string { - const data = message.replace(/%/g, "%25").replace(/\r/g, "%0D").replace(/\n/g, "%0A"); - return `::${name}::${data}${EOL}`; + return `::${name}::${oneLine(message.replace(/%/g, "%25"))}${EOL}`; } /** One GITHUB_OUTPUT record in the heredoc form the runner reads, with the fresh delimiter @actions/core mints per record. */ diff --git a/src/cli/io.ts b/src/cli/io.ts index 0df63c717..114961d0b 100644 --- a/src/cli/io.ts +++ b/src/cli/io.ts @@ -5,7 +5,17 @@ * beside that redaction, so a gsac step and the action step read alike. */ -import { appendFileSync } from "node:fs"; +import { + appendFileSync, + closeSync, + constants, + existsSync, + lstatSync, + mkdirSync, + openSync, + rmSync, +} from "node:fs"; +import { dirname } from "node:path"; import { Writable } from "node:stream"; import { type ConsolaReporter, @@ -14,6 +24,7 @@ import { LogLevels, type LogType, } from "consola"; +import { err, ok, type Result } from "neverthrow"; import pc from "picocolors"; import { type AnnotationLevel, @@ -23,7 +34,7 @@ import { type OutputName, redactRanges, } from "../index.js"; -import { type ActionsRunner, outputRecord, workflowCommand } from "./actions.js"; +import { type ActionsRunner, oneLine, outputRecord, workflowCommand } from "./actions.js"; export interface CliStreams { readonly stdout: Writable; @@ -103,6 +114,40 @@ export function maskedStreams(streams: CliStreams, runner?: ActionsRunner): Mask }; } +declare const settledSummaryFile: unique symbol; + +/** A `--summary` path proved appendable before the run; settleSummaryFile() alone mints one, so the Io never holds a path nothing tried. */ +export type SummaryFile = string & { readonly [settledSummaryFile]: true }; + +/** + * `path` settled before the run sends a request: its directories are created, as the rendered and snapshot files + * create theirs, and an append is proved possible. The error is the whole refusal, naming `--summary`. + */ +export function settleSummaryFile(path: string): Result { + try { + mkdirSync(dirname(path), { recursive: true }); + if (existsSync(path)) { + closeSync(openSync(path, constants.O_WRONLY | constants.O_APPEND)); + return ok(path as SummaryFile); + } + + // A link to a file not yet written is left to the append, which creates that file: an exclusive create stops at a link. + const pendingLink = lstatSync(path, { throwIfNoEntry: false })?.isSymbolicLink() === true; + if (!pendingLink) { + // Created to prove it can be, then removed: the run has yet to read its inputs, and a settings file named + // here by mistake must still be missing when it is read. + closeSync(openSync(path, "ax")); + rmSync(path); + } + + return ok(path as SummaryFile); + } catch (error) { + return err( + `cannot write the summary to ${path}: ${String(error)}. Check that --summary names a writable path`, + ); + } +} + export interface CliIoOptions { /** The masked boundary every write goes through. */ readonly streams: MaskedStreams; @@ -113,8 +158,8 @@ export interface CliIoOptions { /** Show the debug trace. */ readonly verbose: boolean; - /** The file summary blocks are appended to; none falls back to the runner's step summary, or drops them. */ - readonly summaryFile?: string; + /** The settled `--summary` file summary blocks are appended to; none falls back to the runner's step summary, or drops them. */ + readonly summaryFile?: SummaryFile; /** Color the level labels. */ readonly colors: boolean; @@ -172,7 +217,7 @@ export function cliIo(options: CliIoOptions): CliIo { const consola = createConsola({ level, reporters: [reporter], throttle: 0 }); const logStream = options.json ? streams.stderr : streams.stdout; const { runner } = streams; - const summaryFile = options.summaryFile ?? runner?.summaryFile; + const summaryFile: string | undefined = options.summaryFile ?? runner?.summaryFile; const outputs = new Map(); const annotate: Io["annotate"] = runner === undefined @@ -181,7 +226,8 @@ export function cliIo(options: CliIoOptions): CliIo { return { io: { annotate, - log: (line) => logStream.write(`${line}\n`), + // Redacted before the line breaks are escaped: a masked value that spans lines no longer equals itself once escaped. + log: (line) => logStream.write(`${oneLine(streams.redact(line))}\n`), debug: (line) => consola.debug(line), summary: (markdown) => { if (summaryFile !== undefined) { diff --git a/src/cli/program.ts b/src/cli/program.ts index 083d6c693..801e8b254 100644 --- a/src/cli/program.ts +++ b/src/cli/program.ts @@ -7,6 +7,7 @@ */ import { Command, CommanderError, Option } from "commander"; +import { ok, type Result } from "neverthrow"; import pc from "picocolors"; import { describeProblem, @@ -38,7 +39,15 @@ import { once, tokenValues, } from "./inputs.js"; -import { type CliIo, type CliStreams, cliIo, type MaskedStreams, maskedStreams } from "./io.js"; +import { + type CliIo, + type CliStreams, + cliIo, + type MaskedStreams, + maskedStreams, + type SummaryFile, + settleSummaryFile, +} from "./io.js"; /** Every subcommand, in help order; the package smoke asserts the installed help names each. */ export const CLI_COMMANDS = [ @@ -108,7 +117,9 @@ interface Wiring { readonly bold: (text: string) => string; readonly execute: (cfg: RunConfig, io: Io) => Promise; readonly executeInit: (cfg: InitConfig, io: Io) => Promise; - readonly openIo: (globals: Globals) => CliIo; + + /** The Io a command writes through; `summaryFile` is the settled `--summary` of a command that writes a summary. */ + readonly openIo: (globals: Globals, summaryFile?: SummaryFile) => CliIo; readonly exit: (code: number) => void; } @@ -174,7 +185,18 @@ function addModeCommands(program: Command, wiring: Wiring): void { command.action(async function (this: Command) { const values = this.optsWithGlobals>(); - const { io, flush } = wiring.openIo(values); + const summaryFile: Result = + values.summary === undefined ? ok(undefined) : settleSummaryFile(values.summary); + if (summaryFile.isErr()) { + // No run started, so this ends as a file-only command's failure does: the error line and the failed envelope. + const refused: Rendered = { code: 1, lines: [], json: failedEnvelope(summaryFile.error) }; + wiring.openIo(values).io.annotate("error", summaryFile.error); + present(wiring.streams, refused, values); + wiring.exit(refused.code); + return; + } + + const { io, flush } = wiring.openIo(values, summaryFile.value); const read = argvReader(mode, values); // The envelope carries the fatal problem's text beside the outputs; the line itself is already on stderr. let fatal: string | undefined; @@ -283,12 +305,12 @@ export function buildProgram(options: ProgramOptions): { bold: paint.bold, execute, executeInit, - openIo: (globals) => + openIo: (globals, summaryFile) => cliIo({ streams, json: globals.json === true, verbose: globals.verbose === true, - summaryFile: globals.summary, + summaryFile, colors, }), exit: (code) => { diff --git a/src/io.ts b/src/io.ts index 97be90ad5..edda5595a 100644 --- a/src/io.ts +++ b/src/io.ts @@ -30,6 +30,10 @@ export interface MaskPair { export interface Io extends MaskPair { annotate(level: AnnotationLevel, message: string): void; + /** + * One call is one line. A sink that prints escapes a line break in `line`, so text a settings file or GitHub + * supplied cannot open a line of its own, which a runner reads as a workflow command when it starts `::`. + */ log(line: string): void; /** A trace line, shown only when the run has step debug logging enabled. */ diff --git a/test/cli/actions.test.ts b/test/cli/actions.test.ts index 971b22418..841218e7f 100644 --- a/test/cli/actions.test.ts +++ b/test/cli/actions.test.ts @@ -11,7 +11,7 @@ import { readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { actionsIo } from "../../src/action/io.js"; import { actionsRunner } from "../../src/cli/actions.js"; -import { cliIo, maskedStreams } from "../../src/cli/io.js"; +import { cliIo, maskedStreams, settleSummaryFile } from "../../src/cli/io.js"; import type { Io } from "../../src/index.js"; import { MockApi } from "../mock-api.js"; import { withTempDir } from "../temp-dir.js"; @@ -35,6 +35,7 @@ function drive(io: Io): void { io.annotate("notice", "100% done\r\nnext line"); io.annotate("warning", "skipped teams"); io.annotate("error", "PATCH /repos/o/r failed: 500 boom"); + io.log('labels: created "a\r\nb" at 100%'); io.output("result", "partial"); io.output("repos-result", '{\n "o/r": { "result": "partial" }\n}'); io.summary("## first"); @@ -46,26 +47,35 @@ function sameDelimiters(text: string): string { return text.replace(/ghadelimiter_[0-9a-f-]{36}/g, "ghadelimiter_"); } -/** The action's Io over the real @actions/core, its stdout and runner files captured. */ -function driveAction(dir: string): { stdout: string; output: string; summary: string } { - const output = join(dir, "action-output.txt"); - const summary = join(dir, "action-summary.md"); - // @actions/core refuses to append to a missing GITHUB_OUTPUT; the runner creates it. - writeFileSync(output, ""); - process.env.GITHUB_OUTPUT = output; - process.env.GITHUB_STEP_SUMMARY = summary; +/** What the action's Io puts on stdout over the real @actions/core: its commands, and its log lines as console.log ends them. */ +function actionStdout(calls: (io: Io) => void): string { const chunks: string[] = []; const write = spyOn(process.stdout, "write").mockImplementation((chunk) => { chunks.push(String(chunk)); return true; }); + const log = spyOn(console, "log").mockImplementation((line) => { + chunks.push(`${line}\n`); + }); try { - drive(actionsIo); + calls(actionsIo); } finally { write.mockRestore(); + log.mockRestore(); } + return chunks.join(""); +} + +/** The action's Io over the real @actions/core, its stdout and runner files captured. */ +function driveAction(dir: string): { stdout: string; output: string; summary: string } { + const output = join(dir, "action-output.txt"); + const summary = join(dir, "action-summary.md"); + // @actions/core refuses to append to a missing GITHUB_OUTPUT; the runner creates it. + writeFileSync(output, ""); + process.env.GITHUB_OUTPUT = output; + process.env.GITHUB_STEP_SUMMARY = summary; return { - stdout: chunks.join(""), + stdout: actionStdout(drive), output: readFileSync(output, "utf8"), summary: readFileSync(summary, "utf8"), }; @@ -87,7 +97,8 @@ function driveCli(dir: string, summaryFile?: string) { json: false, verbose: true, colors: false, - summaryFile, + summaryFile: + summaryFile === undefined ? undefined : settleSummaryFile(summaryFile)._unsafeUnwrap(), }); return { ...opened, @@ -111,6 +122,8 @@ describe("the CLI under GitHub Actions", () => { "::notice::100%25 done%0D%0Anext line", "::warning::skipped teams", "::error::PATCH /repos/o/r failed: 500 boom", + // A log line carries the runner's spelling of a line break and keeps its `%`: nothing decodes a log line. + 'labels: created "a%0D%0Ab" at 100%', "", ].join("\n"), ); @@ -139,6 +152,43 @@ describe("the CLI under GitHub Actions", () => { expect(action.summary).toBe("## first\n| a |\n|---|\n"); })); + // The runner ends a line at LF, CRLF, or a lone CR, and reads a line starting `::` as a workflow command, so a + // line break in a name a settings file or GitHub supplied would let that name issue one from the admin's run. + test.each([ + [ + "LF", + "x\n::error file=README.md,line=1::spoofed", + "x%0A::error file=README.md,line=1::spoofed", + ], + [ + "CRLF", + "x\r\n::error file=README.md,line=1::spoofed", + "x%0D%0A::error file=README.md,line=1::spoofed", + ], + [ + "a lone CR", + "x\r::error file=README.md,line=1::spoofed", + "x%0D::error file=README.md,line=1::spoofed", + ], + ])( + "a logged name holding %s stays on its own line on both faces, so no output line starts a workflow command", + (_break, name, escaped) => + withTempDir("gsac-actions-", (dir) => { + const logs = (io: Io): void => { + io.log(`drift: labels["${name}"]: missing`); + io.log("result: drift"); + }; + const cli = driveCli(dir); + logs(cli.io); + for (const stdout of [actionStdout(logs), cli.stdout()]) { + const lines = stdout.split(/\r\n|\r|\n/); + expect(lines).toEqual([`drift: labels["${escaped}"]: missing`, "result: drift", ""]); + // The runner trims a line's leading whitespace before it looks for the command marker. + expect(lines.filter((line) => line.trimStart().startsWith("::"))).toEqual([]); + } + }), + ); + test("--summary takes the summary from the runner's step summary; an unset GITHUB_OUTPUT drops the record", () => withTempDir("gsac-actions-", (dir) => { const named = join(dir, "named.md"); diff --git a/test/cli/io.test.ts b/test/cli/io.test.ts index 3e51838bd..7b6367f92 100644 --- a/test/cli/io.test.ts +++ b/test/cli/io.test.ts @@ -6,9 +6,16 @@ */ import { describe, expect, test } from "bun:test"; -import { readdirSync, readFileSync } from "node:fs"; +import { + lstatSync, + readdirSync, + readFileSync, + readlinkSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { join } from "node:path"; -import { type CliIoOptions, cliIo, maskedStreams } from "../../src/cli/io.js"; +import { type CliIoOptions, cliIo, maskedStreams, settleSummaryFile } from "../../src/cli/io.js"; import { withTempDir } from "../temp-dir.js"; import { memoryStream } from "./streams.js"; @@ -25,6 +32,16 @@ function open(options: Partial> = {}) { return { ...opened, stdout: stdout.text, stderr: stderr.text }; } +/** Each entry of `dir` with what it holds: a file's text, or where a link points. */ +function listing(dir: string): string[] { + return readdirSync(dir).map((name) => { + const path = join(dir, name); + return lstatSync(path).isSymbolicLink() + ? `${name} -> ${readlinkSync(path)}` + : `${name}: ${readFileSync(path, "utf8")}`; + }); +} + /** Every channel written once, the way a run writes them. */ function exercise(io: ReturnType["io"]): void { io.log("labels: 2 in sync"); @@ -69,6 +86,14 @@ describe("the CLI Io", () => { ); }); + test("a masked value that spans lines is redacted before its line breaks are escaped", () => { + // Escaped first, the value in the line would no longer equal the value in the registry, and it would print. + const { io, stdout } = open(); + io.mask("first half\nsecond half"); + io.log("deploy key: first half\nsecond half\ndone"); + expect(stdout()).toBe("deploy key: ***%0Adone\n"); + }); + test("--verbose shows the debug trace on stderr; without it the trace is dropped", () => { const quiet = open(); quiet.io.debug("GET /repos/o/r -> 200"); @@ -87,7 +112,7 @@ describe("the CLI Io", () => { test("summary blocks append to the named file and are dropped without one", () => withTempDir("gsac-io-", async (dir) => { const summary = join(dir, "summary.md"); - const named = open({ summaryFile: summary }); + const named = open({ summaryFile: settleSummaryFile(summary)._unsafeUnwrap() }); named.io.summary("## first"); named.io.summary("## second"); expect(readFileSync(summary, "utf8")).toBe("## first\n## second\n"); @@ -109,6 +134,35 @@ describe("the CLI Io", () => { expect(readFileSync(summary, "utf8")).toBe("## first\n## second\n"); })); + // The check runs before the run reads its inputs. A file it left behind would be read as an input named there by + // mistake, and a link to a file not yet written must still lead the summary there. + test.each<[string, (dir: string) => void, string, string]>([ + ["a path that does not exist", () => {}, "summary.md", ""], + [ + "a file that holds a block", + (dir) => writeFileSync(join(dir, "summary.md"), "## kept\n"), + "summary.md", + "## kept\n", + ], + [ + "a link to a file not yet written", + (dir) => symlinkSync("run.md", join(dir, "summary.md")), + "run.md", + "", + ], + ])( + "settling --summary at %s leaves the directory as it found it, and the block still lands", + (_kind, arrange, lands, kept) => + withTempDir("gsac-io-", (dir) => { + arrange(dir); + const found = listing(dir); + const summaryFile = settleSummaryFile(join(dir, "summary.md"))._unsafeUnwrap(); + expect(listing(dir)).toEqual(found); + open({ summaryFile }).io.summary("## block"); + expect(readFileSync(join(dir, lands), "utf8")).toBe(`${kept}## block\n`); + }), + ); + test("identical consecutive lines are all printed, never folded", () => { // consola folds repeats within a second by default; a drift report with // the same line per target must keep every one. diff --git a/test/cli/program.test.ts b/test/cli/program.test.ts index 9d78a94f3..4b2a0b41a 100644 --- a/test/cli/program.test.ts +++ b/test/cli/program.test.ts @@ -8,7 +8,7 @@ */ import { describe, expect, test } from "bun:test"; -import { writeFileSync } from "node:fs"; +import { existsSync, readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import type { CliHost } from "../../src/cli/commands.js"; import { main } from "../../src/cli/program.js"; @@ -101,36 +101,99 @@ describe("check and apply", () => { test.each([false, true])( "a failure after the parse is reported through the mask boundary, exit 1 (verbose: %p)", - (verbose) => - withTempDir("gsac-cli-", async (dir) => { - // The summary file's directory does not exist, so the write throws inside - // the run; the path carries the token, and the report must not. Without - // --verbose the remedy asks for it; with it the stack is the report. - const summary = join(dir, "missing", `${TOKEN}.md`); - const masked = summary.replaceAll(TOKEN, "***"); - const result = await cli( - ["check", ...target, "--summary", summary, ...(verbose ? ["--verbose"] : [])], - new MockApi({ "GET /repos/o/r": { data: { has_wiki: false, private: false } } }), - ); - expect(result.code).toBe(1); - expect(result.stdout + result.stderr).not.toContain(TOKEN); - const message = `Error: ENOENT: no such file or directory, open '${masked}'`; - if (!verbose) { - expect(result.stderr).toBe( - `error: github-settings-as-code stopped unexpectedly: ${message}. Re-run with --verbose for the stack; if it recurs, file a bug with that output attached\n`, - ); - return; - } - // The stack's frames carry this machine's paths, so the line is pinned around them. - expect(result.stderr).toStartWith( - `error: github-settings-as-code stopped unexpectedly: ${message}\n at `, - ); - expect(result.stderr).toEndWith( - ". The stack above is the report: if it recurs, file a bug with it attached\n", + async (verbose) => { + // The client factory throws inside the run, its message carrying the token, and the report must not. + // Without --verbose the remedy asks for it; with it the stack is the report. + const stdout = memoryStream(); + const stderr = memoryStream(); + const host: CliHost = { + env: {}, + createClient: () => { + throw new Error(`no client for ${TOKEN}`); + }, + }; + const code = await main( + ["node", "gsac", "check", ...target, ...(verbose ? ["--verbose"] : [])], + { host, streams: { stdout: stdout.stream, stderr: stderr.stream }, colors: false }, + ); + expect(code).toBe(1); + expect(stdout.text() + stderr.text()).not.toContain(TOKEN); + const message = "Error: no client for ***"; + if (!verbose) { + expect(stderr.text()).toBe( + `error: github-settings-as-code stopped unexpectedly: ${message}. Re-run with --verbose for the stack; if it recurs, file a bug with that output attached\n`, ); - expect(result.stderr).not.toContain("Re-run with --verbose"); - }), + return; + } + // The stack's frames carry this machine's paths, so the line is pinned around them. + expect(stderr.text()).toStartWith( + `error: github-settings-as-code stopped unexpectedly: ${message}\n at `, + ); + expect(stderr.text()).toEndWith( + ". The stack above is the report: if it recurs, file a bug with it attached\n", + ); + expect(stderr.text()).not.toContain("Re-run with --verbose"); + }, ); + + test("--summary into a directory that does not exist creates it before the run, so the summary lands and the run ends as it went", () => + withTempDir("gsac-cli-", async (dir) => { + // Appended to without its directory, the summary write threw after the PATCH had landed: exit 1 on an applied run. + const summary = join(dir, "no-such-dir", "summary.md"); + const api = new MockApi({ + "GET /repos/o/r": { data: { has_wiki: true, private: false } }, + }).allowMutations("PATCH /repos/o/r"); + const result = await cli(["apply", ...target, "--summary", summary, "--json"], api); + expect({ code: result.code, stdout: JSON.parse(result.stdout) }).toEqual({ + code: 0, + stdout: { result: "applied", "skipped-sections": [], "repos-result": {} }, + }); + expect(api.mutations().map((call) => `${call.method} ${call.path}`)).toEqual([ + "PATCH /repos/o/r", + ]); + expect(readFileSync(summary, "utf8")).toContain( + "| repository | :white_check_mark: applied |", + ); + })); + + test("a --summary path that does not exist stays absent until the summary is written, so a settings file named there by mistake is still refused", () => + withTempDir("gsac-cli-", async (dir) => { + // Left behind by the check, the empty file would read as an empty settings document and the run would end applied. + const missing = join(dir, "missing.yml"); + const api = new MockApi({}); + const result = await cli( + [ + "apply", + "--repository", + "o/r", + "--token", + TOKEN, + "--settings-file", + missing, + "--summary", + missing, + ], + api, + ); + expect(api.calls).toEqual([]); + expect(result.code).toBe(1); + expect(result.stderr).toStartWith(`error: cannot read settings from ${missing}: `); + expect(existsSync(missing)).toBe(false); + })); + + test("a --summary path nothing can append to is refused before any request, naming --summary", () => + withTempDir("gsac-cli-", async (dir) => { + // A directory at the path. Refused at the end of the run instead, the requests would already have landed. + const api = new MockApi({}); + const problem = `cannot write the summary to ${dir}: Error: EISDIR: illegal operation on a directory, open '${dir}'. Check that --summary names a writable path`; + const result = await cli(["apply", ...target, "--summary", dir, "--json"], api); + expect(api.calls).toEqual([]); + expect({ ...result, stdout: JSON.parse(result.stdout) }).toEqual({ + code: 1, + stdout: { result: "failed", problem }, + stderr: `error: ${problem}\n`, + }); + })); }); describe("validate and permissions", () => { From 8cd41014bba2d86572b160487e173b049164e132 Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 21:06:49 -0500 Subject: [PATCH 2/4] fix(action): neutralize the older workflow command form, and hold --summary open from before the run A GitHub Actions runner reads two forms as a workflow command: two colons at the start of a line, after leading whitespace, and the older form that opens with two hash signs and a bracket, which it finds anywhere in a line. Escaping line breaks alone left the second open, so a label name holding it still issued a command with no line break at all. The action's log sink now rewrites that marker as well. For the command line the rewrite sits at the stream boundary, under a runner only, on every chunk that is not a finished command, so the log, the debug trace, the JSON envelope, and any write added later are covered at one point. On a terminal the output is unchanged. The --summary check created and removed a probe file, which needed an exclusive create, which needed an exception for links, and a link into a missing directory still failed after the run's requests had landed. It is replaced by the plain form: the parent directories are created, the path is opened for append before the run, and the summary is appended through that descriptor, which is closed when the run settles. A path the open refuses is refused before any request. The file therefore exists, empty, from the start of the run. --- docs/start/cli.md | 13 +++-- src/action/io.ts | 14 +++-- src/cli/actions.ts | 9 ++- src/cli/io.ts | 69 ++++++++--------------- src/cli/program.ts | 31 +++++----- src/io.ts | 5 +- test/cli/actions.test.ts | 96 +++++++++++++++++++++++++------ test/cli/io.test.ts | 54 ++---------------- test/cli/program.test.ts | 119 ++++++++++++++++++++------------------- 9 files changed, 210 insertions(+), 200 deletions(-) diff --git a/docs/start/cli.md b/docs/start/cli.md index 44cb554ee..cfd2cd4f1 100644 --- a/docs/start/cli.md +++ b/docs/start/cli.md @@ -140,12 +140,12 @@ Two inputs have no command-line form: `--private-report artifact` is refused (th Four flags are the command line's own, and `init` has one more, `--force` (replace an existing settings file): -| Flag | Does | -| ------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `--token ` | The token; `GITHUB_TOKEN` when absent | -| `--json` | Print the outputs as one JSON object on stdout; log lines move to stderr | -| `--summary ` | Append the run's markdown summary (the action's step summary) to this file, creating its directories; a path that cannot be written is refused before any request; under GitHub Actions the step summary itself when absent; `init`, `validate`, and `permissions` have none | -| `--verbose` | Show the debug trace on stderr | +| Flag | Does | +| ------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `--token ` | The token; `GITHUB_TOKEN` when absent | +| `--json` | Print the outputs as one JSON object on stdout; log lines move to stderr | +| `--summary ` | Append the run's markdown summary (the action's step summary) to this file; under GitHub Actions the step summary itself when absent. The file is opened, and its directories created, before the run starts: a path that cannot be opened is refused before any request, and the empty file and its directories stay when the run is then refused for another reason. `init`, `validate`, and `permissions` never open it. | +| `--verbose` | Show the debug trace on stderr | ## Output and exit codes @@ -164,6 +164,7 @@ As a GitHub Actions step (`GITHUB_ACTIONS=true` in the environment, as the runne - **An annotation** is a `::notice::`, `::warning::`, or `::error::` command on stdout instead of the stderr line. - **The outputs** land in the step outputs (`GITHUB_OUTPUT`). - **The summary** lands in the step summary (`GITHUB_STEP_SUMMARY`) unless `--summary` names a file. +- **Every other line**, the `--json` object's text included, has `##[` written `%23%23[`, since the runner reads `##[` anywhere in a line as a workflow command. Those commands share stdout with the `--json` object, so a step reads the outputs from the step outputs, not by parsing stdout; the one-object rule below holds in a terminal. diff --git a/src/action/io.ts b/src/action/io.ts index a8d473f73..f7dacd589 100644 --- a/src/action/io.ts +++ b/src/action/io.ts @@ -44,17 +44,19 @@ function appendSummary(markdown: string): void { } /** - * `line` with each line break in the runner's spelling (%0D, %0A), as @actions/core escapes a command's data. The - * runner ends a line at CR or LF and reads a line starting `::` as a workflow command, so text after a raw break - * would issue one. `%` stays as written: nothing decodes a log line. test/cli/actions.test.ts holds the CLI's log to this form. + * `line` as one log line the runner prints and never reads as a workflow command. test/cli/actions.test.ts holds + * the CLI's log under a runner to the same output. + * + * the runner reads `::` at the start of a line, after leading whitespace -> CR and LF are written %0D and %0A + * the runner reads `##[` anywhere in a line -> it is written %23%23[ */ -function oneLine(line: string): string { - return line.replace(/\r/g, "%0D").replace(/\n/g, "%0A"); +function plainLine(line: string): string { + return line.replace(/\r/g, "%0D").replace(/\n/g, "%0A").replace(/##\[/g, "%23%23["); } export const actionsIo: Io = { annotate, - log: (line) => console.log(oneLine(line)), + log: (line) => console.log(plainLine(line)), debug: (line) => core.debug(line), summary: appendSummary, output: setOutput, diff --git a/src/cli/actions.ts b/src/cli/actions.ts index 9c0178a44..a83d584a0 100644 --- a/src/cli/actions.ts +++ b/src/cli/actions.ts @@ -36,13 +36,18 @@ export function actionsRunner( } /** - * `text` on one line, each line break in the runner's spelling (%0D, %0A). The runner ends a line at CR or LF and - * reads a line starting `::` as a workflow command, so text after a raw break would open a line of its own. + * `text` with each line break in the runner's spelling (%0D, %0A). The runner reads `::` as a workflow command at + * the start of a line, after leading whitespace, so text after a raw break could open one. */ export function oneLine(text: string): string { return text.replace(/\r/g, "%0D").replace(/\n/g, "%0A"); } +/** `text` with `##[` written %23%23[: the runner also reads the older `##[command]` form, and that one anywhere in a line. */ +export function withoutLegacyCommand(text: string): string { + return text.replace(/##\[/g, "%23%23["); +} + /** One `::name::message` line with the runner's data escaping (%, CR, LF), as @actions/core issues it. */ export function workflowCommand(name: "add-mask" | AnnotationLevel, message: string): string { return `::${name}::${oneLine(message.replace(/%/g, "%25"))}${EOL}`; diff --git a/src/cli/io.ts b/src/cli/io.ts index 114961d0b..ad905ecc9 100644 --- a/src/cli/io.ts +++ b/src/cli/io.ts @@ -5,16 +5,7 @@ * beside that redaction, so a gsac step and the action step read alike. */ -import { - appendFileSync, - closeSync, - constants, - existsSync, - lstatSync, - mkdirSync, - openSync, - rmSync, -} from "node:fs"; +import { appendFileSync, mkdirSync, openSync } from "node:fs"; import { dirname } from "node:path"; import { Writable } from "node:stream"; import { @@ -34,7 +25,13 @@ import { type OutputName, redactRanges, } from "../index.js"; -import { type ActionsRunner, oneLine, outputRecord, workflowCommand } from "./actions.js"; +import { + type ActionsRunner, + oneLine, + outputRecord, + withoutLegacyCommand, + workflowCommand, +} from "./actions.js"; export interface CliStreams { readonly stdout: Writable; @@ -61,17 +58,17 @@ class Verbatim { constructor(readonly text: string) {} } -/** A stream that redacts each chunk before handing it to `target`; one queue, so no chunk overtakes another. */ +/** A stream that rewrites each chunk that is not Verbatim before handing it to `target`; one queue, so no chunk overtakes another. */ class RedactingStream extends Writable { constructor( private readonly target: Writable, - private readonly redact: (text: string) => string, + private readonly rewrite: (text: string) => string, ) { super({ objectMode: true }); } override _write(chunk: unknown, _encoding: BufferEncoding, callback: () => void): void { - const text = chunk instanceof Verbatim ? chunk.text : this.redact(String(chunk)); + const text = chunk instanceof Verbatim ? chunk.text : this.rewrite(String(chunk)); // A full target holds the next chunk until it drains, so backpressure reaches the writer. if (this.target.write(text)) { callback(); @@ -94,14 +91,17 @@ export function maskedStreams(streams: CliStreams, runner?: ActionsRunner): Mask runner === undefined ? () => {} : (value) => command(workflowCommand("add-mask", value)), ); const redact = (text: string): string => redactRanges(text, registry.masked()); - const stdout = new RedactingStream(streams.stdout, redact); + // Under a runner a chunk that is no finished command also loses `##[`, so no writer has to remember it. + const rewrite = + runner === undefined ? redact : (text: string): string => withoutLegacyCommand(redact(text)); + const stdout = new RedactingStream(streams.stdout, rewrite); /** A finished command line, queued behind the redacted writes before it. */ function command(line: string): void { stdout.write(new Verbatim(line)); } return { stdout, - stderr: new RedactingStream(streams.stderr, redact), + stderr: new RedactingStream(streams.stderr, rewrite), redact, runner: runner === undefined @@ -114,33 +114,14 @@ export function maskedStreams(streams: CliStreams, runner?: ActionsRunner): Mask }; } -declare const settledSummaryFile: unique symbol; - -/** A `--summary` path proved appendable before the run; settleSummaryFile() alone mints one, so the Io never holds a path nothing tried. */ -export type SummaryFile = string & { readonly [settledSummaryFile]: true }; - /** - * `path` settled before the run sends a request: its directories are created, as the rendered and snapshot files - * create theirs, and an append is proved possible. The error is the whole refusal, naming `--summary`. + * The descriptor of `path` opened for append, its directories created first as the rendered and snapshot files + * create theirs. Taken before the run sends a request, so a path the open refuses is the refusal here, naming `--summary`. */ -export function settleSummaryFile(path: string): Result { +export function openSummaryFile(path: string): Result { try { mkdirSync(dirname(path), { recursive: true }); - if (existsSync(path)) { - closeSync(openSync(path, constants.O_WRONLY | constants.O_APPEND)); - return ok(path as SummaryFile); - } - - // A link to a file not yet written is left to the append, which creates that file: an exclusive create stops at a link. - const pendingLink = lstatSync(path, { throwIfNoEntry: false })?.isSymbolicLink() === true; - if (!pendingLink) { - // Created to prove it can be, then removed: the run has yet to read its inputs, and a settings file named - // here by mistake must still be missing when it is read. - closeSync(openSync(path, "ax")); - rmSync(path); - } - - return ok(path as SummaryFile); + return ok(openSync(path, "a")); } catch (error) { return err( `cannot write the summary to ${path}: ${String(error)}. Check that --summary names a writable path`, @@ -158,8 +139,8 @@ export interface CliIoOptions { /** Show the debug trace. */ readonly verbose: boolean; - /** The settled `--summary` file summary blocks are appended to; none falls back to the runner's step summary, or drops them. */ - readonly summaryFile?: SummaryFile; + /** The descriptor openSummaryFile() took on `--summary`'s file, which summary blocks are appended through; none falls back to the runner's step summary, or drops them. */ + readonly summaryFd?: number; /** Color the level labels. */ readonly colors: boolean; @@ -217,7 +198,7 @@ export function cliIo(options: CliIoOptions): CliIo { const consola = createConsola({ level, reporters: [reporter], throttle: 0 }); const logStream = options.json ? streams.stderr : streams.stdout; const { runner } = streams; - const summaryFile: string | undefined = options.summaryFile ?? runner?.summaryFile; + const summaryTarget = options.summaryFd ?? runner?.summaryFile; const outputs = new Map(); const annotate: Io["annotate"] = runner === undefined @@ -230,8 +211,8 @@ export function cliIo(options: CliIoOptions): CliIo { log: (line) => logStream.write(`${oneLine(streams.redact(line))}\n`), debug: (line) => consola.debug(line), summary: (markdown) => { - if (summaryFile !== undefined) { - appendFileSync(summaryFile, `${streams.redact(markdown)}\n`); + if (summaryTarget !== undefined) { + appendFileSync(summaryTarget, `${streams.redact(markdown)}\n`); } }, output: (name, value) => { diff --git a/src/cli/program.ts b/src/cli/program.ts index 801e8b254..1799bb87e 100644 --- a/src/cli/program.ts +++ b/src/cli/program.ts @@ -6,6 +6,7 @@ * without touching the process. */ +import { closeSync } from "node:fs"; import { Command, CommanderError, Option } from "commander"; import { ok, type Result } from "neverthrow"; import pc from "picocolors"; @@ -45,8 +46,7 @@ import { cliIo, type MaskedStreams, maskedStreams, - type SummaryFile, - settleSummaryFile, + openSummaryFile, } from "./io.js"; /** Every subcommand, in help order; the package smoke asserts the installed help names each. */ @@ -110,16 +110,14 @@ export interface ProgramOptions { readonly executeInit?: (cfg: InitConfig, io: Io) => Promise; } -/** What every subcommand's action shares; `exit` records the code main() returns. */ +/** What every subcommand's action shares; `exit` records the code main() returns, and `openIo` takes the descriptor a mode command holds on its `--summary` file. */ interface Wiring { readonly host: CliHost; readonly streams: MaskedStreams; readonly bold: (text: string) => string; readonly execute: (cfg: RunConfig, io: Io) => Promise; readonly executeInit: (cfg: InitConfig, io: Io) => Promise; - - /** The Io a command writes through; `summaryFile` is the settled `--summary` of a command that writes a summary. */ - readonly openIo: (globals: Globals, summaryFile?: SummaryFile) => CliIo; + readonly openIo: (globals: Globals, summaryFd?: number) => CliIo; readonly exit: (code: number) => void; } @@ -185,22 +183,23 @@ function addModeCommands(program: Command, wiring: Wiring): void { command.action(async function (this: Command) { const values = this.optsWithGlobals>(); - const summaryFile: Result = - values.summary === undefined ? ok(undefined) : settleSummaryFile(values.summary); - if (summaryFile.isErr()) { + const summary: Result = + values.summary === undefined ? ok(undefined) : openSummaryFile(values.summary); + if (summary.isErr()) { // No run started, so this ends as a file-only command's failure does: the error line and the failed envelope. - const refused: Rendered = { code: 1, lines: [], json: failedEnvelope(summaryFile.error) }; - wiring.openIo(values).io.annotate("error", summaryFile.error); + const refused: Rendered = { code: 1, lines: [], json: failedEnvelope(summary.error) }; + wiring.openIo(values).io.annotate("error", summary.error); present(wiring.streams, refused, values); wiring.exit(refused.code); return; } - const { io, flush } = wiring.openIo(values, summaryFile.value); + const summaryFd = summary.value; + const { io, flush } = wiring.openIo(values, summaryFd); const read = argvReader(mode, values); // The envelope carries the fatal problem's text beside the outputs; the line itself is already on stderr. let fatal: string | undefined; - const code = await parseConfig(read, wiring.host.env, CLI_CAPABILITIES).match( + const run = parseConfig(read, wiring.host.env, CLI_CAPABILITIES).match( async (cfg) => { const end = await wiring.execute(cfg, io); fatal = end.fatal === undefined ? undefined : describeProblem(end.fatal); @@ -211,6 +210,8 @@ function addModeCommands(program: Command, wiring: Wiring): void { return failRun(io, problem); }, ); + // The run has written its summary by the time it settles, whichever way it ends. + const code = await (summaryFd === undefined ? run : run.finally(() => closeSync(summaryFd))); wiring.exit(code); flush(fatal); }); @@ -305,12 +306,12 @@ export function buildProgram(options: ProgramOptions): { bold: paint.bold, execute, executeInit, - openIo: (globals, summaryFile) => + openIo: (globals, summaryFd) => cliIo({ streams, json: globals.json === true, verbose: globals.verbose === true, - summaryFile, + summaryFd, colors, }), exit: (code) => { diff --git a/src/io.ts b/src/io.ts index edda5595a..b56edde31 100644 --- a/src/io.ts +++ b/src/io.ts @@ -31,8 +31,9 @@ export interface Io extends MaskPair { annotate(level: AnnotationLevel, message: string): void; /** - * One call is one line. A sink that prints escapes a line break in `line`, so text a settings file or GitHub - * supplied cannot open a line of its own, which a runner reads as a workflow command when it starts `::`. + * One call is one line: a sink that prints escapes a line break in `line`. Where a runner reads the output, `##[` + * is rewritten too, so text a settings file or GitHub supplied cannot issue a workflow command, which the runner + * reads as `::` at the start of a line or `##[` anywhere in one. */ log(line: string): void; diff --git a/test/cli/actions.test.ts b/test/cli/actions.test.ts index 841218e7f..628d01a3a 100644 --- a/test/cli/actions.test.ts +++ b/test/cli/actions.test.ts @@ -7,11 +7,11 @@ */ import { afterEach, describe, expect, spyOn, test } from "bun:test"; -import { readFileSync, writeFileSync } from "node:fs"; +import { closeSync, readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { actionsIo } from "../../src/action/io.js"; import { actionsRunner } from "../../src/cli/actions.js"; -import { cliIo, maskedStreams, settleSummaryFile } from "../../src/cli/io.js"; +import { cliIo, maskedStreams, openSummaryFile } from "../../src/cli/io.js"; import type { Io } from "../../src/index.js"; import { MockApi } from "../mock-api.js"; import { withTempDir } from "../temp-dir.js"; @@ -35,7 +35,7 @@ function drive(io: Io): void { io.annotate("notice", "100% done\r\nnext line"); io.annotate("warning", "skipped teams"); io.annotate("error", "PATCH /repos/o/r failed: 500 boom"); - io.log('labels: created "a\r\nb" at 100%'); + io.log('labels: created "a\r\nb##[error]c" at 100%'); io.output("result", "partial"); io.output("repos-result", '{\n "o/r": { "result": "partial" }\n}'); io.summary("## first"); @@ -82,7 +82,7 @@ function driveAction(dir: string): { stdout: string; output: string; summary: st } /** The CLI's Io under the runner's environment, over memory streams and its own runner files. */ -function driveCli(dir: string, summaryFile?: string) { +function driveCli(dir: string, summaryFd?: number) { const output = join(dir, "cli-output.txt"); const summary = join(dir, "cli-summary.md"); const stdout = memoryStream(); @@ -97,8 +97,7 @@ function driveCli(dir: string, summaryFile?: string) { json: false, verbose: true, colors: false, - summaryFile: - summaryFile === undefined ? undefined : settleSummaryFile(summaryFile)._unsafeUnwrap(), + summaryFd, }); return { ...opened, @@ -122,8 +121,9 @@ describe("the CLI under GitHub Actions", () => { "::notice::100%25 done%0D%0Anext line", "::warning::skipped teams", "::error::PATCH /repos/o/r failed: 500 boom", - // A log line carries the runner's spelling of a line break and keeps its `%`: nothing decodes a log line. - 'labels: created "a%0D%0Ab" at 100%', + // One log line, the same from both faces: its line breaks are escaped at each sink, `##[` at the action's sink and + // at the CLI's stream boundary, and `%` stays, since only a command's data is decoded. + 'labels: created "a%0D%0Ab%23%23[error]c" at 100%', "", ].join("\n"), ); @@ -152,27 +152,29 @@ describe("the CLI under GitHub Actions", () => { expect(action.summary).toBe("## first\n| a |\n|---|\n"); })); - // The runner ends a line at LF, CRLF, or a lone CR, and reads a line starting `::` as a workflow command, so a - // line break in a name a settings file or GitHub supplied would let that name issue one from the admin's run. + // The runner reads two forms as a workflow command: `::` at the start of a line, after leading whitespace, and + // `##[` anywhere in one. A name a settings file or GitHub supplied would issue either from the admin's run. test.each([ [ - "LF", + "a line feed before `::`", "x\n::error file=README.md,line=1::spoofed", "x%0A::error file=README.md,line=1::spoofed", ], [ - "CRLF", + "CR LF before `::`", "x\r\n::error file=README.md,line=1::spoofed", "x%0D%0A::error file=README.md,line=1::spoofed", ], [ - "a lone CR", + "a lone CR before `::`", "x\r::error file=README.md,line=1::spoofed", "x%0D::error file=README.md,line=1::spoofed", ], + ["`##[` and no line break", "x##[error]spoofed", "x%23%23[error]spoofed"], + ["both forms", "x\n::error::a##[error]b", "x%0A::error::a%23%23[error]b"], ])( - "a logged name holding %s stays on its own line on both faces, so no output line starts a workflow command", - (_break, name, escaped) => + "a logged name holding %s is one line of plain text on both faces, so the runner reads no command in it", + (_form, name, escaped) => withTempDir("gsac-actions-", (dir) => { const logs = (io: Io): void => { io.log(`drift: labels["${name}"]: missing`); @@ -183,17 +185,75 @@ describe("the CLI under GitHub Actions", () => { for (const stdout of [actionStdout(logs), cli.stdout()]) { const lines = stdout.split(/\r\n|\r|\n/); expect(lines).toEqual([`drift: labels["${escaped}"]: missing`, "result: drift", ""]); - // The runner trims a line's leading whitespace before it looks for the command marker. - expect(lines.filter((line) => line.trimStart().startsWith("::"))).toEqual([]); + const commands = lines.filter( + (line) => line.trimStart().startsWith("::") || line.includes("##["), + ); + expect(commands).toEqual([]); } }), ); + // The runner reads `##[` as a command anywhere in a line, so under one the stream rewrites it in every chunk that is + // not a finished command, whichever write carried it. A write added later is covered without wrapping it. + test.each<[string, Record, (name: string, problem: string) => string[][]]>([ + [ + "under a runner the log, the debug trace, and the --json envelope lose `##[`, and a finished command keeps its text", + { GITHUB_ACTIONS: "true" }, + (_name, problem) => [ + [ + "::add-mask::ghp_secret", + `::error::${problem}`, + '{"result":"failed","problem":"labels[1].name: \\"x%23%23[error]y\\" names the same label"}', + ], + [ + 'labels: created label "x%23%23[error]y"', + 'debug: POST /repos/o/r/labels -> 201 payload: {"name":"x%23%23[error]y"}', + ], + ], + ], + [ + "on a terminal, where nothing reads commands, the same writes arrive byte for byte", + {}, + (name, problem) => [ + [JSON.stringify({ result: "failed", problem })], + [ + `labels: created label "${name}"`, + `debug: POST /repos/o/r/labels -> 201 payload: {"name":"${name}"}`, + `error: ${problem}`, + ], + ], + ], + ])("%s", (_case, env, expected) => { + const stdout = memoryStream(); + const stderr = memoryStream(); + const { io, flush } = cliIo({ + streams: maskedStreams({ stdout: stdout.stream, stderr: stderr.stream }, actionsRunner(env)), + json: true, + verbose: true, + colors: false, + }); + const name = "x##[error]y"; + const problem = `labels[1].name: "${name}" names the same label`; + io.mask("ghp_secret"); + io.log(`labels: created label "${name}"`); + io.debug(`POST /repos/o/r/labels -> 201 payload: {"name":"${name}"}`); + io.annotate("error", problem); + io.output("result", "failed"); + flush(problem); + const [out, err] = expected(name, problem); + expect([stdout.text().split("\n"), stderr.text().split("\n")]).toEqual([ + [...(out ?? []), ""], + [...(err ?? []), ""], + ]); + }); + test("--summary takes the summary from the runner's step summary; an unset GITHUB_OUTPUT drops the record", () => withTempDir("gsac-actions-", (dir) => { const named = join(dir, "named.md"); - const cli = driveCli(dir, named); + const summaryFd = openSummaryFile(named)._unsafeUnwrap(); + const cli = driveCli(dir, summaryFd); cli.io.summary("## named"); + closeSync(summaryFd); expect(readFileSync(named, "utf8")).toBe("## named\n"); expect(() => cli.summary()).toThrow(/ENOENT/); const stdout = memoryStream(); diff --git a/test/cli/io.test.ts b/test/cli/io.test.ts index 7b6367f92..ef994f52c 100644 --- a/test/cli/io.test.ts +++ b/test/cli/io.test.ts @@ -6,16 +6,9 @@ */ import { describe, expect, test } from "bun:test"; -import { - lstatSync, - readdirSync, - readFileSync, - readlinkSync, - symlinkSync, - writeFileSync, -} from "node:fs"; +import { closeSync, readdirSync, readFileSync } from "node:fs"; import { join } from "node:path"; -import { type CliIoOptions, cliIo, maskedStreams, settleSummaryFile } from "../../src/cli/io.js"; +import { type CliIoOptions, cliIo, maskedStreams, openSummaryFile } from "../../src/cli/io.js"; import { withTempDir } from "../temp-dir.js"; import { memoryStream } from "./streams.js"; @@ -32,16 +25,6 @@ function open(options: Partial> = {}) { return { ...opened, stdout: stdout.text, stderr: stderr.text }; } -/** Each entry of `dir` with what it holds: a file's text, or where a link points. */ -function listing(dir: string): string[] { - return readdirSync(dir).map((name) => { - const path = join(dir, name); - return lstatSync(path).isSymbolicLink() - ? `${name} -> ${readlinkSync(path)}` - : `${name}: ${readFileSync(path, "utf8")}`; - }); -} - /** Every channel written once, the way a run writes them. */ function exercise(io: ReturnType["io"]): void { io.log("labels: 2 in sync"); @@ -112,9 +95,11 @@ describe("the CLI Io", () => { test("summary blocks append to the named file and are dropped without one", () => withTempDir("gsac-io-", async (dir) => { const summary = join(dir, "summary.md"); - const named = open({ summaryFile: settleSummaryFile(summary)._unsafeUnwrap() }); + const summaryFd = openSummaryFile(summary)._unsafeUnwrap(); + const named = open({ summaryFd }); named.io.summary("## first"); named.io.summary("## second"); + closeSync(summaryFd); expect(readFileSync(summary, "utf8")).toBe("## first\n## second\n"); // Without a file the block goes nowhere: no stream carries it, and the working // directory (where a defaulted path would land) stays empty. @@ -134,35 +119,6 @@ describe("the CLI Io", () => { expect(readFileSync(summary, "utf8")).toBe("## first\n## second\n"); })); - // The check runs before the run reads its inputs. A file it left behind would be read as an input named there by - // mistake, and a link to a file not yet written must still lead the summary there. - test.each<[string, (dir: string) => void, string, string]>([ - ["a path that does not exist", () => {}, "summary.md", ""], - [ - "a file that holds a block", - (dir) => writeFileSync(join(dir, "summary.md"), "## kept\n"), - "summary.md", - "## kept\n", - ], - [ - "a link to a file not yet written", - (dir) => symlinkSync("run.md", join(dir, "summary.md")), - "run.md", - "", - ], - ])( - "settling --summary at %s leaves the directory as it found it, and the block still lands", - (_kind, arrange, lands, kept) => - withTempDir("gsac-io-", (dir) => { - arrange(dir); - const found = listing(dir); - const summaryFile = settleSummaryFile(join(dir, "summary.md"))._unsafeUnwrap(); - expect(listing(dir)).toEqual(found); - open({ summaryFile }).io.summary("## block"); - expect(readFileSync(join(dir, lands), "utf8")).toBe(`${kept}## block\n`); - }), - ); - test("identical consecutive lines are all printed, never folded", () => { // consola folds repeats within a second by default; a drift report with // the same line per target must keep every one. diff --git a/test/cli/program.test.ts b/test/cli/program.test.ts index 4b2a0b41a..7d49dc1ad 100644 --- a/test/cli/program.test.ts +++ b/test/cli/program.test.ts @@ -8,7 +8,7 @@ */ import { describe, expect, test } from "bun:test"; -import { existsSync, readFileSync, writeFileSync } from "node:fs"; +import { readFileSync, symlinkSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import type { CliHost } from "../../src/cli/commands.js"; import { main } from "../../src/cli/program.js"; @@ -136,64 +136,67 @@ describe("check and apply", () => { }, ); - test("--summary into a directory that does not exist creates it before the run, so the summary lands and the run ends as it went", () => - withTempDir("gsac-cli-", async (dir) => { - // Appended to without its directory, the summary write threw after the PATCH had landed: exit 1 on an applied run. - const summary = join(dir, "no-such-dir", "summary.md"); - const api = new MockApi({ - "GET /repos/o/r": { data: { has_wiki: true, private: false } }, - }).allowMutations("PATCH /repos/o/r"); - const result = await cli(["apply", ...target, "--summary", summary, "--json"], api); - expect({ code: result.code, stdout: JSON.parse(result.stdout) }).toEqual({ - code: 0, - stdout: { result: "applied", "skipped-sections": [], "repos-result": {} }, - }); - expect(api.mutations().map((call) => `${call.method} ${call.path}`)).toEqual([ - "PATCH /repos/o/r", - ]); - expect(readFileSync(summary, "utf8")).toContain( - "| repository | :white_check_mark: applied |", - ); - })); - - test("a --summary path that does not exist stays absent until the summary is written, so a settings file named there by mistake is still refused", () => - withTempDir("gsac-cli-", async (dir) => { - // Left behind by the check, the empty file would read as an empty settings document and the run would end applied. - const missing = join(dir, "missing.yml"); - const api = new MockApi({}); - const result = await cli( - [ - "apply", - "--repository", - "o/r", - "--token", - TOKEN, - "--settings-file", - missing, - "--summary", - missing, - ], - api, - ); - expect(api.calls).toEqual([]); - expect(result.code).toBe(1); - expect(result.stderr).toStartWith(`error: cannot read settings from ${missing}: `); - expect(existsSync(missing)).toBe(false); - })); + // Opened only when the summary was appended, a path that could not be was found out after the run's requests had landed. + test.each<[string, (dir: string) => string, string]>([ + ["under a directory that does not exist", (dir) => join(dir, "no-such-dir", "summary.md"), ""], + [ + "at a file that already holds a block", + (dir) => { + const file = join(dir, "summary.md"); + writeFileSync(file, "## earlier\n"); + return file; + }, + "## earlier\n", + ], + ])( + "--summary %s is opened before the run, and the run's summary is appended to what the file held", + (_where, arrange, held) => + withTempDir("gsac-cli-", async (dir) => { + const summary = arrange(dir); + const api = new MockApi({ + "GET /repos/o/r": { data: { has_wiki: true, private: false } }, + }).allowMutations("PATCH /repos/o/r"); + const result = await cli(["apply", ...target, "--summary", summary, "--json"], api); + expect({ code: result.code, stdout: JSON.parse(result.stdout) }).toEqual({ + code: 0, + stdout: { result: "applied", "skipped-sections": [], "repos-result": {} }, + }); + expect(api.mutations().map((call) => `${call.method} ${call.path}`)).toEqual([ + "PATCH /repos/o/r", + ]); + const written = readFileSync(summary, "utf8"); + expect(written).toStartWith(`${held}## github-settings-as-code (apply)\n`); + expect(written).toContain("| repository | :white_check_mark: applied |"); + }), + ); - test("a --summary path nothing can append to is refused before any request, naming --summary", () => - withTempDir("gsac-cli-", async (dir) => { - // A directory at the path. Refused at the end of the run instead, the requests would already have landed. - const api = new MockApi({}); - const problem = `cannot write the summary to ${dir}: Error: EISDIR: illegal operation on a directory, open '${dir}'. Check that --summary names a writable path`; - const result = await cli(["apply", ...target, "--summary", dir, "--json"], api); - expect(api.calls).toEqual([]); - expect({ ...result, stdout: JSON.parse(result.stdout) }).toEqual({ - code: 1, - stdout: { result: "failed", problem }, - stderr: `error: ${problem}\n`, - }); - })); + test.each<[string, (dir: string) => string, string]>([ + ["a directory", (dir) => dir, "EISDIR: illegal operation on a directory"], + [ + "a link into a directory that does not exist", + (dir) => { + const link = join(dir, "summary.md"); + symlinkSync(join(dir, "missing", "summary.md"), link); + return link; + }, + "ENOENT: no such file or directory", + ], + ])( + "a --summary path that is %s cannot be opened, so it is refused before any request, naming --summary", + (_what, arrange, reason) => + withTempDir("gsac-cli-", async (dir) => { + const summary = arrange(dir); + const api = new MockApi({}); + const problem = `cannot write the summary to ${summary}: Error: ${reason}, open '${summary}'. Check that --summary names a writable path`; + const result = await cli(["apply", ...target, "--summary", summary, "--json"], api); + expect(api.calls).toEqual([]); + expect({ ...result, stdout: JSON.parse(result.stdout) }).toEqual({ + code: 1, + stdout: { result: "failed", problem }, + stderr: `error: ${problem}\n`, + }); + }), + ); }); describe("validate and permissions", () => { From 1edd78c41e1ac9549774ce0a8179893aadf4c3ad Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 21:49:01 -0500 Subject: [PATCH 3/4] fix(action): close the line-start command form at both faces, and lint direct console calls The guarantee: text from a settings file or from live GitHub state, printed by this action's or this command line's own writers, cannot be read by the Actions runner as a workflow command. The runner reads two colons at the start of a line, after leading whitespace, and the older hash-bracket marker anywhere in a line. Not covered: text a dependency prints itself, a marker split across two writes, and a writer that bypasses the sink or the stream on purpose. Until now two colons at the start of a line were closed only by escaping line breaks in a log call, so a line that itself began with them passed: no producer writes one today, but a new producer, or a non-log write of the command line under a runner, would have. The action's log sink now rewrites a leading pair of colons, and the command line's stream boundary does the same for every line of every chunk that is not a finished command, beside the older marker it already rewrote. Leading whitespace is what the runner trims, the NEL character included. A lint rule now refuses a direct console call anywhere in src except the action's sink, so a new writer cannot bypass the sink unnoticed. The docs say that a line break in a logged value is written in its percent form on a terminal too. The redundant type annotation on the summary open is gone, and the comment beside the descriptor's close states what holds for a refused run as well. --- biome.local.jsonc | 12 +++++++ docs/start/cli.md | 4 ++- src/action/io.ts | 13 +++++--- src/cli/actions.ts | 16 ++++++++-- src/cli/io.ts | 6 ++-- src/cli/program.ts | 6 ++-- src/io.ts | 6 ++-- test/cli/actions.test.ts | 69 +++++++++++++++++++++++++--------------- 8 files changed, 89 insertions(+), 43 deletions(-) diff --git a/biome.local.jsonc b/biome.local.jsonc index aca3e15f9..db1844576 100644 --- a/biome.local.jsonc +++ b/biome.local.jsonc @@ -67,6 +67,18 @@ } } }, + // Every line the action or the CLI prints goes through its sink (src/action/io.ts) or the CLI's streams, where + // text the Actions runner would read as a workflow command is rewritten; a console call elsewhere bypasses both. + { + "includes": ["src/**", "!src/action/io.ts"], + "linter": { + "rules": { + "suspicious": { + "noConsole": "error" + } + } + } + }, { "includes": ["src/**"], "plugins": ["./lint/never-throw.grit"] diff --git a/docs/start/cli.md b/docs/start/cli.md index cfd2cd4f1..ae35dfeb7 100644 --- a/docs/start/cli.md +++ b/docs/start/cli.md @@ -158,13 +158,15 @@ skipped-sections= repos-result={} ``` +A log line is one line in every mode: a line break inside a logged value is written `%0D` or `%0A`. + As a GitHub Actions step (`GITHUB_ACTIONS=true` in the environment, as the runner sets it) the command line reports as the action does, in addition: - **A masked value** is a `::add-mask::` command. - **An annotation** is a `::notice::`, `::warning::`, or `::error::` command on stdout instead of the stderr line. - **The outputs** land in the step outputs (`GITHUB_OUTPUT`). - **The summary** lands in the step summary (`GITHUB_STEP_SUMMARY`) unless `--summary` names a file. -- **Every other line**, the `--json` object's text included, has `##[` written `%23%23[`, since the runner reads `##[` anywhere in a line as a workflow command. +- **Every other line**, the `--json` object's text included, has `##[` written `%23%23[` and a leading `::` written `%3A%3A`, since the runner reads both as workflow commands. Those commands share stdout with the `--json` object, so a step reads the outputs from the step outputs, not by parsing stdout; the one-object rule below holds in a terminal. diff --git a/src/action/io.ts b/src/action/io.ts index f7dacd589..25b43cbe3 100644 --- a/src/action/io.ts +++ b/src/action/io.ts @@ -44,14 +44,19 @@ function appendSummary(markdown: string): void { } /** - * `line` as one log line the runner prints and never reads as a workflow command. test/cli/actions.test.ts holds - * the CLI's log under a runner to the same output. + * `line` as one log line the runner cannot read as a workflow command, so text from a settings file or from live + * GitHub state, printed by this action's own writers, cannot issue one. Not covered: what a dependency prints + * itself, and a writer that bypasses this sink on purpose. test/cli/actions.test.ts holds the CLI to the same output. * - * the runner reads `::` at the start of a line, after leading whitespace -> CR and LF are written %0D and %0A + * the runner reads `::` at the start of a line, after leading whitespace -> CR and LF are written %0D and %0A, a leading `::` %3A%3A * the runner reads `##[` anywhere in a line -> it is written %23%23[ */ function plainLine(line: string): string { - return line.replace(/\r/g, "%0D").replace(/\n/g, "%0A").replace(/##\[/g, "%23%23["); + return line + .replace(/\r/g, "%0D") + .replace(/\n/g, "%0A") + .replace(/^([\s\u0085]*)::/, "$1%3A%3A") + .replace(/##\[/g, "%23%23["); } export const actionsIo: Io = { diff --git a/src/cli/actions.ts b/src/cli/actions.ts index a83d584a0..db4f43373 100644 --- a/src/cli/actions.ts +++ b/src/cli/actions.ts @@ -9,6 +9,9 @@ import { randomUUID } from "node:crypto"; import { EOL } from "node:os"; import type { AnnotationLevel, OutputName } from "../index.js"; +/** `::` where the runner looks for it: at the start of a line, after the whitespace it trims, which JavaScript's `\s` covers but for NEL. */ +const LINE_OPENING_COMMAND = /^([\s\u0085]*)::/gm; + export interface ActionsRunner { /** GITHUB_OUTPUT; the runner always sets it, so an unset one only drops the record, as the action's Io does. */ readonly outputFile: string | undefined; @@ -43,9 +46,16 @@ export function oneLine(text: string): string { return text.replace(/\r/g, "%0D").replace(/\n/g, "%0A"); } -/** `text` with `##[` written %23%23[: the runner also reads the older `##[command]` form, and that one anywhere in a line. */ -export function withoutLegacyCommand(text: string): string { - return text.replace(/##\[/g, "%23%23["); +/** + * `text` with no line the runner reads as a workflow command, so text from a settings file or from live GitHub + * state, printed by any of the CLI's own writers, cannot issue one. Not covered: what a dependency prints itself, + * a marker split across two writes, and a writer that bypasses the stream on purpose. + * + * the runner reads `::` at the start of a line, after leading whitespace -> that `::` is written %3A%3A + * the runner reads `##[` anywhere in a line -> it is written %23%23[ + */ +export function withoutCommands(text: string): string { + return text.replace(LINE_OPENING_COMMAND, "$1%3A%3A").replace(/##\[/g, "%23%23["); } /** One `::name::message` line with the runner's data escaping (%, CR, LF), as @actions/core issues it. */ diff --git a/src/cli/io.ts b/src/cli/io.ts index ad905ecc9..08b9ed08d 100644 --- a/src/cli/io.ts +++ b/src/cli/io.ts @@ -29,7 +29,7 @@ import { type ActionsRunner, oneLine, outputRecord, - withoutLegacyCommand, + withoutCommands, workflowCommand, } from "./actions.js"; @@ -91,9 +91,9 @@ export function maskedStreams(streams: CliStreams, runner?: ActionsRunner): Mask runner === undefined ? () => {} : (value) => command(workflowCommand("add-mask", value)), ); const redact = (text: string): string => redactRanges(text, registry.masked()); - // Under a runner a chunk that is no finished command also loses `##[`, so no writer has to remember it. + // Under a runner a chunk that is no finished command also loses what the runner reads as one, so no writer has to remember it. const rewrite = - runner === undefined ? redact : (text: string): string => withoutLegacyCommand(redact(text)); + runner === undefined ? redact : (text: string): string => withoutCommands(redact(text)); const stdout = new RedactingStream(streams.stdout, rewrite); /** A finished command line, queued behind the redacted writes before it. */ function command(line: string): void { diff --git a/src/cli/program.ts b/src/cli/program.ts index 1799bb87e..4ccbc2897 100644 --- a/src/cli/program.ts +++ b/src/cli/program.ts @@ -8,7 +8,7 @@ import { closeSync } from "node:fs"; import { Command, CommanderError, Option } from "commander"; -import { ok, type Result } from "neverthrow"; +import { ok } from "neverthrow"; import pc from "picocolors"; import { describeProblem, @@ -183,7 +183,7 @@ function addModeCommands(program: Command, wiring: Wiring): void { command.action(async function (this: Command) { const values = this.optsWithGlobals>(); - const summary: Result = + const summary = values.summary === undefined ? ok(undefined) : openSummaryFile(values.summary); if (summary.isErr()) { // No run started, so this ends as a file-only command's failure does: the error line and the failed envelope. @@ -210,7 +210,7 @@ function addModeCommands(program: Command, wiring: Wiring): void { return failRun(io, problem); }, ); - // The run has written its summary by the time it settles, whichever way it ends. + // Nothing appends to the summary once the run has settled, whichever way it ends. const code = await (summaryFd === undefined ? run : run.finally(() => closeSync(summaryFd))); wiring.exit(code); flush(fatal); diff --git a/src/io.ts b/src/io.ts index b56edde31..e3622d6c5 100644 --- a/src/io.ts +++ b/src/io.ts @@ -31,9 +31,9 @@ export interface Io extends MaskPair { annotate(level: AnnotationLevel, message: string): void; /** - * One call is one line: a sink that prints escapes a line break in `line`. Where a runner reads the output, `##[` - * is rewritten too, so text a settings file or GitHub supplied cannot issue a workflow command, which the runner - * reads as `::` at the start of a line or `##[` anywhere in one. + * One call is one line: a sink that prints escapes a line break in `line`. Where a runner reads the output, the + * face also rewrites what the runner reads as a workflow command (`::` at the start of a line, `##[` anywhere in + * one), so text a settings file or GitHub supplied cannot issue one. */ log(line: string): void; diff --git a/test/cli/actions.test.ts b/test/cli/actions.test.ts index 628d01a3a..f0fb32c14 100644 --- a/test/cli/actions.test.ts +++ b/test/cli/actions.test.ts @@ -42,6 +42,16 @@ function drive(io: Io): void { io.summary("| a |\n|---|"); } +/** The runner's two parsers, as its source reads a line: `::` after the whitespace .NET trims (NEL included), or `##[` anywhere. */ +function runnerReadsCommand(line: string): boolean { + return /^[\s\u0085]*::/.test(line) || line.includes("##["); +} + +/** The drift line a list section logs for a declared entry the repository lacks, the name as the settings file gave it. */ +function drift(name: string): string { + return `drift: labels["${name}"]: missing`; +} + /** @actions/core mints one random delimiter per record; the pin compares the records around it. */ function sameDelimiters(text: string): string { return text.replace(/ghadelimiter_[0-9a-f-]{36}/g, "ghadelimiter_"); @@ -153,51 +163,53 @@ describe("the CLI under GitHub Actions", () => { })); // The runner reads two forms as a workflow command: `::` at the start of a line, after leading whitespace, and - // `##[` anywhere in one. A name a settings file or GitHub supplied would issue either from the admin's run. + // `##[` anywhere in one. Text a settings file or GitHub supplied would issue either from the admin's run. test.each([ [ "a line feed before `::`", - "x\n::error file=README.md,line=1::spoofed", - "x%0A::error file=README.md,line=1::spoofed", + drift("x\n::error file=README.md,line=1::spoofed"), + drift("x%0A::error file=README.md,line=1::spoofed"), ], [ "CR LF before `::`", - "x\r\n::error file=README.md,line=1::spoofed", - "x%0D%0A::error file=README.md,line=1::spoofed", + drift("x\r\n::error file=README.md,line=1::spoofed"), + drift("x%0D%0A::error file=README.md,line=1::spoofed"), ], [ "a lone CR before `::`", - "x\r::error file=README.md,line=1::spoofed", - "x%0D::error file=README.md,line=1::spoofed", + drift("x\r::error file=README.md,line=1::spoofed"), + drift("x%0D::error file=README.md,line=1::spoofed"), + ], + ["`##[` and no line break", drift("x##[error]spoofed"), drift("x%23%23[error]spoofed")], + ["both forms", drift("x\n::error::a##[error]b"), drift("x%0A::error::a%23%23[error]b")], + [ + "`::` at its own start, after whitespace", + "\t \u0085::error::spoofed: created", + "\t \u0085%3A%3Aerror::spoofed: created", ], - ["`##[` and no line break", "x##[error]spoofed", "x%23%23[error]spoofed"], - ["both forms", "x\n::error::a##[error]b", "x%0A::error::a%23%23[error]b"], ])( - "a logged name holding %s is one line of plain text on both faces, so the runner reads no command in it", - (_form, name, escaped) => + "a logged line holding %s is one line of plain text on both faces, so the runner reads no command in it", + (_form, line, plain) => withTempDir("gsac-actions-", (dir) => { const logs = (io: Io): void => { - io.log(`drift: labels["${name}"]: missing`); + io.log(line); io.log("result: drift"); }; const cli = driveCli(dir); logs(cli.io); for (const stdout of [actionStdout(logs), cli.stdout()]) { const lines = stdout.split(/\r\n|\r|\n/); - expect(lines).toEqual([`drift: labels["${escaped}"]: missing`, "result: drift", ""]); - const commands = lines.filter( - (line) => line.trimStart().startsWith("::") || line.includes("##["), - ); - expect(commands).toEqual([]); + expect(lines).toEqual([plain, "result: drift", ""]); + expect(lines.filter(runnerReadsCommand)).toEqual([]); } }), ); - // The runner reads `##[` as a command anywhere in a line, so under one the stream rewrites it in every chunk that is - // not a finished command, whichever write carried it. A write added later is covered without wrapping it. + // Under a runner the stream rewrites both forms in every chunk that is not a finished command, whichever write + // carried it, so a write added later is covered without wrapping it. On a terminal nothing reads commands. test.each<[string, Record, (name: string, problem: string) => string[][]]>([ [ - "under a runner the log, the debug trace, and the --json envelope lose `##[`, and a finished command keeps its text", + "under a runner the log, the debug trace, the --json envelope, and a bare stream write lose `##[` and a leading `::`, and a finished command keeps its text", { GITHUB_ACTIONS: "true" }, (_name, problem) => [ [ @@ -207,7 +219,9 @@ describe("the CLI under GitHub Actions", () => { ], [ 'labels: created label "x%23%23[error]y"', + "%3A%3Anotice::from a settings value", 'debug: POST /repos/o/r/labels -> 201 payload: {"name":"x%23%23[error]y"}', + " %3A%3Awarning::from a write added later", ], ], ], @@ -218,7 +232,9 @@ describe("the CLI under GitHub Actions", () => { [JSON.stringify({ result: "failed", problem })], [ `labels: created label "${name}"`, + "::notice::from a settings value", `debug: POST /repos/o/r/labels -> 201 payload: {"name":"${name}"}`, + " ::warning::from a write added later", `error: ${problem}`, ], ], @@ -226,17 +242,18 @@ describe("the CLI under GitHub Actions", () => { ])("%s", (_case, env, expected) => { const stdout = memoryStream(); const stderr = memoryStream(); - const { io, flush } = cliIo({ - streams: maskedStreams({ stdout: stdout.stream, stderr: stderr.stream }, actionsRunner(env)), - json: true, - verbose: true, - colors: false, - }); + const streams = maskedStreams( + { stdout: stdout.stream, stderr: stderr.stream }, + actionsRunner(env), + ); + const { io, flush } = cliIo({ streams, json: true, verbose: true, colors: false }); const name = "x##[error]y"; const problem = `labels[1].name: "${name}" names the same label`; io.mask("ghp_secret"); io.log(`labels: created label "${name}"`); + io.log("::notice::from a settings value"); io.debug(`POST /repos/o/r/labels -> 201 payload: {"name":"${name}"}`); + streams.stderr.write(" ::warning::from a write added later\n"); io.annotate("error", problem); io.output("result", "failed"); flush(problem); From fd24bb7b738e320a1533c82bd85efbd231209725 Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 22:22:37 -0500 Subject: [PATCH 4/4] fix(action): redact before escaping, and match the runner's line starts and white space exactly The last commit rewrote two things the runner does not read, and on the action it escaped a line before the runner could mask it. A registered secret that holds the hash-bracket marker, a line break, or a leading pair of colons was escaped by the action's log sink, so the runner's mask no longer matched it and the secret printed in a reversible form. The sink now redacts the masks the action registered and only then escapes. The command line already redacted first, at its log sink and at its stream; tests now pin that order on both faces. The command line's line-start rule used a multiline anchor, which in JavaScript also matches after U+2028 and U+2029, though the runner's reader ends a line at CR and LF only. A line now starts at the start of a chunk or after CR or LF. The leading white space was JavaScript's class plus NEL, one character wider than what the runner trims. It is now exactly the 25 characters .NET's Char.IsWhiteSpace accepts, written out on each face, and the test checks both against its own list over the whole Basic Multilingual Plane. --- docs/start/cli.md | 2 +- src/action/io.ts | 20 ++++++-- src/cli/actions.ts | 12 +++-- test/cli/actions.test.ts | 106 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 130 insertions(+), 10 deletions(-) diff --git a/docs/start/cli.md b/docs/start/cli.md index ae35dfeb7..08235abb3 100644 --- a/docs/start/cli.md +++ b/docs/start/cli.md @@ -166,7 +166,7 @@ As a GitHub Actions step (`GITHUB_ACTIONS=true` in the environment, as the runne - **An annotation** is a `::notice::`, `::warning::`, or `::error::` command on stdout instead of the stderr line. - **The outputs** land in the step outputs (`GITHUB_OUTPUT`). - **The summary** lands in the step summary (`GITHUB_STEP_SUMMARY`) unless `--summary` names a file. -- **Every other line**, the `--json` object's text included, has `##[` written `%23%23[` and a leading `::` written `%3A%3A`, since the runner reads both as workflow commands. +- **Every other line** has `##[` written `%23%23[`, the `--json` object's text included, and a `::` that opens a line written `%3A%3A`, since the runner reads both as workflow commands. Those commands share stdout with the `--json` object, so a step reads the outputs from the step outputs, not by parsing stdout; the one-object rule below holds in a terminal. diff --git a/src/action/io.ts b/src/action/io.ts index 25b43cbe3..b7a07e99d 100644 --- a/src/action/io.ts +++ b/src/action/io.ts @@ -1,6 +1,6 @@ import { appendFileSync } from "node:fs"; import * as core from "@actions/core"; -import { type Io, maskRegistry, type OutputName } from "../index.js"; +import { type Io, maskRegistry, type OutputName, redactRanges } from "../index.js"; /** Generated into the action.yml `outputs` block (bun run build:action-docs); the satisfies clause locks the keys to OutputName. */ export const OUTPUT_DECLS = { @@ -21,6 +21,17 @@ export const OUTPUT_DECLS = { }, } as const satisfies Record; +/** + * `::` at the start of a line, after the white space the runner's TrimStart() removes: the 25 characters .NET's + * Char.IsWhiteSpace accepts (U+0009..U+000D, U+0020, U+0085, U+00A0, U+1680, U+2000..U+200A, U+2028, U+2029, + * U+202F, U+205F, U+3000). + */ +const LEADING_COMMAND = + /^([\t-\r \u0085\u00a0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000]*)::/; + +/** The masks this action registered with the runner; the log sink redacts them itself before it escapes. */ +const registry = maskRegistry(core.setSecret); + // The static map keeps the namespace access tree-shakeable (biome noDynamicNamespaceImportAccess) and captures the references at module load, so a spy installed after import is not seen; @actions/core owns the workflow-command escaping (%, CR, LF). const annotators = { notice: core.notice, warning: core.warning, error: core.error } as const; @@ -55,15 +66,16 @@ function plainLine(line: string): string { return line .replace(/\r/g, "%0D") .replace(/\n/g, "%0A") - .replace(/^([\s\u0085]*)::/, "$1%3A%3A") + .replace(LEADING_COMMAND, "$1%3A%3A") .replace(/##\[/g, "%23%23["); } export const actionsIo: Io = { annotate, - log: (line) => console.log(plainLine(line)), + // Redacted here, before the escapes: the runner masks a registered value only where it stands as registered. + log: (line) => console.log(plainLine(redactRanges(line, registry.masked()))), debug: (line) => core.debug(line), summary: appendSummary, output: setOutput, - ...maskRegistry(core.setSecret), + ...registry, }; diff --git a/src/cli/actions.ts b/src/cli/actions.ts index db4f43373..734b3b215 100644 --- a/src/cli/actions.ts +++ b/src/cli/actions.ts @@ -9,8 +9,14 @@ import { randomUUID } from "node:crypto"; import { EOL } from "node:os"; import type { AnnotationLevel, OutputName } from "../index.js"; -/** `::` where the runner looks for it: at the start of a line, after the whitespace it trims, which JavaScript's `\s` covers but for NEL. */ -const LINE_OPENING_COMMAND = /^([\s\u0085]*)::/gm; +/** + * `::` where the runner reads it: at the start of a line, after the white space its TrimStart() removes. A line + * starts at the start of a chunk or after CR or LF, the only characters the runner's reader ends one at. The white + * space is the 25 characters .NET's Char.IsWhiteSpace accepts: U+0009..U+000D, U+0020, U+0085, U+00A0, U+1680, + * U+2000..U+200A, U+2028, U+2029, U+202F, U+205F, U+3000. + */ +const LINE_OPENING_COMMAND = + /(^|[\r\n])([\t-\r \u0085\u00a0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000]*)::/g; export interface ActionsRunner { /** GITHUB_OUTPUT; the runner always sets it, so an unset one only drops the record, as the action's Io does. */ @@ -55,7 +61,7 @@ export function oneLine(text: string): string { * the runner reads `##[` anywhere in a line -> it is written %23%23[ */ export function withoutCommands(text: string): string { - return text.replace(LINE_OPENING_COMMAND, "$1%3A%3A").replace(/##\[/g, "%23%23["); + return text.replace(LINE_OPENING_COMMAND, "$1$2%3A%3A").replace(/##\[/g, "%23%23["); } /** One `::name::message` line with the runner's data escaping (%, CR, LF), as @actions/core issues it. */ diff --git a/test/cli/actions.test.ts b/test/cli/actions.test.ts index f0fb32c14..bada79b5a 100644 --- a/test/cli/actions.test.ts +++ b/test/cli/actions.test.ts @@ -42,9 +42,27 @@ function drive(io: Io): void { io.summary("| a |\n|---|"); } -/** The runner's two parsers, as its source reads a line: `::` after the whitespace .NET trims (NEL included), or `##[` anywhere. */ +/** + * From Microsoft's reference for Char.IsWhiteSpace, not from the code under test: the 25 characters the runner's + * TrimStart() removes before it looks for `::`. + * + * U+0009..U+000D, U+0085 -> the control characters the reference names + * U+0020, U+00A0, U+1680, U+2000..U+200A, + * U+202F, U+205F, U+3000 -> the SpaceSeparator category + * U+2028, U+2029 -> the LineSeparator and ParagraphSeparator categories + */ +const DOTNET_WHITE_SPACE: readonly number[] = [ + 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x20, 0x85, 0xa0, 0x1680, 0x2000, 0x2001, 0x2002, 0x2003, 0x2004, + 0x2005, 0x2006, 0x2007, 0x2008, 0x2009, 0x200a, 0x2028, 0x2029, 0x202f, 0x205f, 0x3000, +]; + +/** The runner's two parsers, as its source reads a line: `::` after the white space TrimStart() removes, or `##[` anywhere. */ function runnerReadsCommand(line: string): boolean { - return /^[\s\u0085]*::/.test(line) || line.includes("##["); + let start = 0; + while (DOTNET_WHITE_SPACE.includes(line.charCodeAt(start))) { + start += 1; + } + return line.startsWith("::", start) || line.includes("##["); } /** The drift line a list section logs for a declared entry the repository lacks, the name as the settings file gave it. */ @@ -205,6 +223,80 @@ describe("the CLI under GitHub Actions", () => { }), ); + // The runner masks a registered secret only where its text stands as registered, so an escape that ran first would + // print the secret in a form anyone can reverse. Redaction sees the text as logged; the escapes come after it. + test.each<[string, string | undefined, string, string]>([ + [ + "a registered secret holding `##[` reaches both faces as the redaction and in no other form", + "pw##[tag]123", + "value=pw##[tag]123", + "value=***", + ], + [ + "a registered secret holding a line break reaches both faces as the redaction and in no other form", + "key-line-1\nkey-line-2", + "value=key-line-1\nkey-line-2", + "value=***", + ], + [ + "a registered secret that begins with `::` reaches both faces as the redaction and in no other form", + "::tok-456", + "::tok-456 is the value", + "*** is the value", + ], + [ + "an unregistered value of the same shapes is still escaped on both faces", + undefined, + "::tok-789 holds x##[tag]\nand a break", + "%3A%3Atok-789 holds x%23%23[tag]%0Aand a break", + ], + ])("%s", (_case, secret, line, written) => + withTempDir("gsac-actions-", (dir) => { + const logs = (io: Io): void => { + if (secret !== undefined) { + io.mask(secret); + } + io.log(line); + }; + const cli = driveCli(dir); + logs(cli.io); + // The add-mask command carries the value by design: it is how the runner learns the mask. + const logged = (stdout: string): string[] => + stdout.split("\n").filter((printed) => !printed.startsWith("::add-mask::")); + expect({ action: logged(actionStdout(logs)), cli: logged(cli.stdout()) }).toEqual({ + action: [written, ""], + cli: [written, ""], + }); + }), + ); + + test("both faces take exactly .NET's white-space characters as the leading whitespace before `::`", () => + withTempDir("gsac-actions-", (dir) => { + // CR and LF are left out: a log line escapes them, and the rows above cover that. Surrogate halves are no characters. + const codes = Array.from({ length: 0x10000 }, (_, code) => code).filter( + (code) => code !== 0x0a && code !== 0x0d && (code < 0xd800 || code > 0xdfff), + ); + const logs = (io: Io): void => { + for (const code of codes) { + io.log(`${String.fromCharCode(code)}::x`); + } + }; + const cli = driveCli(dir); + logs(cli.io); + for (const stdout of [actionStdout(logs), cli.stdout()]) { + const lines = stdout.split("\n"); + const trimmed = codes.filter( + (code, at) => lines[at] === `${String.fromCharCode(code)}%3A%3Ax`, + ); + const changed = codes.filter((code, at) => lines[at] !== `${String.fromCharCode(code)}::x`); + expect(trimmed).toEqual( + DOTNET_WHITE_SPACE.filter((code) => code !== 0x0a && code !== 0x0d), + ); + // The one other line that changes is `:::x`, which opens with the command marker itself. + expect(changed.filter((code) => !trimmed.includes(code))).toEqual([0x3a]); + } + })); + // Under a runner the stream rewrites both forms in every chunk that is not a finished command, whichever write // carried it, so a write added later is covered without wrapping it. On a terminal nothing reads commands. test.each<[string, Record, (name: string, problem: string) => string[][]]>([ @@ -214,6 +306,7 @@ describe("the CLI under GitHub Actions", () => { (_name, problem) => [ [ "::add-mask::ghp_secret", + "::add-mask::pw##[tag]123", `::error::${problem}`, '{"result":"failed","problem":"labels[1].name: \\"x%23%23[error]y\\" names the same label"}', ], @@ -221,7 +314,9 @@ describe("the CLI under GitHub Actions", () => { 'labels: created label "x%23%23[error]y"', "%3A%3Anotice::from a settings value", 'debug: POST /repos/o/r/labels -> 201 payload: {"name":"x%23%23[error]y"}', + "debug: Authorization: ***", " %3A%3Awarning::from a write added later", + "safe\u2028::file.yml opens no line, but\r%3A%3Athis does", ], ], ], @@ -234,7 +329,9 @@ describe("the CLI under GitHub Actions", () => { `labels: created label "${name}"`, "::notice::from a settings value", `debug: POST /repos/o/r/labels -> 201 payload: {"name":"${name}"}`, + "debug: Authorization: ***", " ::warning::from a write added later", + "safe\u2028::file.yml opens no line, but\r::this does", `error: ${problem}`, ], ], @@ -250,10 +347,15 @@ describe("the CLI under GitHub Actions", () => { const name = "x##[error]y"; const problem = `labels[1].name: "${name}" names the same label`; io.mask("ghp_secret"); + io.mask("pw##[tag]123"); io.log(`labels: created label "${name}"`); io.log("::notice::from a settings value"); io.debug(`POST /repos/o/r/labels -> 201 payload: {"name":"${name}"}`); + // A write that is no log line, holding a mask the rewrite would break: the stream redacts first. + io.debug("Authorization: pw##[tag]123"); streams.stderr.write(" ::warning::from a write added later\n"); + // The runner's reader ends a line at CR or LF and nowhere else, U+2028 included. + streams.stderr.write("safe\u2028::file.yml opens no line, but\r::this does\n"); io.annotate("error", problem); io.output("result", "failed"); flush(problem);