diff --git a/biome.local.jsonc b/biome.local.jsonc index aca3e15f..db184457 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 506e1e81..08235abb 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; 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 @@ -158,12 +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** 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 c985b2e6..b7a07e99 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; @@ -43,11 +54,28 @@ function appendSummary(markdown: string): void { appendFileSync(file, `${markdown}\n`); } +/** + * `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, 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(LEADING_COMMAND, "$1%3A%3A") + .replace(/##\[/g, "%23%23["); +} + export const actionsIo: Io = { annotate, - log: (line) => console.log(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 f3cefdcd..734b3b21 100644 --- a/src/cli/actions.ts +++ b/src/cli/actions.ts @@ -9,6 +9,15 @@ import { randomUUID } from "node:crypto"; import { EOL } from "node:os"; import type { AnnotationLevel, OutputName } from "../index.js"; +/** + * `::` 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. */ readonly outputFile: string | undefined; @@ -35,10 +44,29 @@ export function actionsRunner( }; } +/** + * `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 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$2%3A%3A").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 { - 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 0df63c71..08b9ed08 100644 --- a/src/cli/io.ts +++ b/src/cli/io.ts @@ -5,7 +5,8 @@ * beside that redaction, so a gsac step and the action step read alike. */ -import { appendFileSync } from "node:fs"; +import { appendFileSync, mkdirSync, openSync } from "node:fs"; +import { dirname } from "node:path"; import { Writable } from "node:stream"; import { type ConsolaReporter, @@ -14,6 +15,7 @@ import { LogLevels, type LogType, } from "consola"; +import { err, ok, type Result } from "neverthrow"; import pc from "picocolors"; import { type AnnotationLevel, @@ -23,7 +25,13 @@ import { type OutputName, redactRanges, } from "../index.js"; -import { type ActionsRunner, outputRecord, workflowCommand } from "./actions.js"; +import { + type ActionsRunner, + oneLine, + outputRecord, + withoutCommands, + workflowCommand, +} from "./actions.js"; export interface CliStreams { readonly stdout: Writable; @@ -50,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(); @@ -83,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 what the runner reads as one, so no writer has to remember it. + const rewrite = + 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 { stdout.write(new Verbatim(line)); } return { stdout, - stderr: new RedactingStream(streams.stderr, redact), + stderr: new RedactingStream(streams.stderr, rewrite), redact, runner: runner === undefined @@ -103,6 +114,21 @@ export function maskedStreams(streams: CliStreams, runner?: ActionsRunner): Mask }; } +/** + * 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 openSummaryFile(path: string): Result { + try { + mkdirSync(dirname(path), { recursive: true }); + return ok(openSync(path, "a")); + } 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 +139,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 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; @@ -172,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 = options.summaryFile ?? runner?.summaryFile; + const summaryTarget = options.summaryFd ?? runner?.summaryFile; const outputs = new Map(); const annotate: Io["annotate"] = runner === undefined @@ -181,11 +207,12 @@ 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) { - 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 083d6c69..4ccbc289 100644 --- a/src/cli/program.ts +++ b/src/cli/program.ts @@ -6,7 +6,9 @@ * without touching the process. */ +import { closeSync } from "node:fs"; import { Command, CommanderError, Option } from "commander"; +import { ok } from "neverthrow"; import pc from "picocolors"; import { describeProblem, @@ -38,7 +40,14 @@ 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, + openSummaryFile, +} from "./io.js"; /** Every subcommand, in help order; the package smoke asserts the installed help names each. */ export const CLI_COMMANDS = [ @@ -101,14 +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; - readonly openIo: (globals: Globals) => CliIo; + readonly openIo: (globals: Globals, summaryFd?: number) => CliIo; readonly exit: (code: number) => void; } @@ -174,11 +183,23 @@ function addModeCommands(program: Command, wiring: Wiring): void { command.action(async function (this: Command) { const values = this.optsWithGlobals>(); - const { io, flush } = wiring.openIo(values); + 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. + 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 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); @@ -189,6 +210,8 @@ function addModeCommands(program: Command, wiring: Wiring): void { return failRun(io, problem); }, ); + // 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); }); @@ -283,12 +306,12 @@ export function buildProgram(options: ProgramOptions): { bold: paint.bold, execute, executeInit, - openIo: (globals) => + openIo: (globals, summaryFd) => cliIo({ streams, json: globals.json === true, verbose: globals.verbose === true, - summaryFile: globals.summary, + summaryFd, colors, }), exit: (code) => { diff --git a/src/io.ts b/src/io.ts index 97be90ad..e3622d6c 100644 --- a/src/io.ts +++ b/src/io.ts @@ -30,6 +30,11 @@ 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`. 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; /** 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 971b2241..bada79b5 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 } 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,44 +35,82 @@ 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##[error]c" at 100%'); io.output("result", "partial"); io.output("repos-result", '{\n "o/r": { "result": "partial" }\n}'); io.summary("## first"); io.summary("| a |\n|---|"); } +/** + * 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 { + 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. */ +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_"); } -/** 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"), }; } /** 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(); @@ -87,7 +125,7 @@ function driveCli(dir: string, summaryFile?: string) { json: false, verbose: true, colors: false, - summaryFile, + summaryFd, }); return { ...opened, @@ -111,6 +149,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", + // 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"), ); @@ -139,11 +180,199 @@ describe("the CLI under GitHub Actions", () => { expect(action.summary).toBe("## first\n| a |\n|---|\n"); })); + // The runner reads two forms as a workflow command: `::` at the start of a line, after leading whitespace, and + // `##[` anywhere in one. Text a settings file or GitHub supplied would issue either from the admin's run. + test.each([ + [ + "a line feed before `::`", + drift("x\n::error file=README.md,line=1::spoofed"), + drift("x%0A::error file=README.md,line=1::spoofed"), + ], + [ + "CR LF before `::`", + 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 `::`", + 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", + ], + ])( + "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(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([plain, "result: drift", ""]); + expect(lines.filter(runnerReadsCommand)).toEqual([]); + } + }), + ); + + // 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[][]]>([ + [ + "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) => [ + [ + "::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"}', + ], + [ + '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", + ], + ], + ], + [ + "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}"`, + "::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}`, + ], + ], + ], + ])("%s", (_case, env, expected) => { + const stdout = memoryStream(); + const stderr = memoryStream(); + 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.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); + 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 3e51838b..ef994f52 100644 --- a/test/cli/io.test.ts +++ b/test/cli/io.test.ts @@ -6,9 +6,9 @@ */ import { describe, expect, test } from "bun:test"; -import { readdirSync, readFileSync } from "node:fs"; +import { closeSync, readdirSync, readFileSync } from "node:fs"; import { join } from "node:path"; -import { type CliIoOptions, cliIo, maskedStreams } 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"; @@ -69,6 +69,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,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: summary }); + 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. diff --git a/test/cli/program.test.ts b/test/cli/program.test.ts index 9d78a94f..7d49dc1a 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 { 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"; @@ -101,34 +101,100 @@ 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"); + }, + ); + + // 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.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`, + }); }), ); });