From d5f881e9fc184d41014e2a169959570a221dcdb7 Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Wed, 13 May 2026 15:27:13 +0000 Subject: [PATCH] action: trim sensitive env values before GitHub Actions log masking (#698) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * action: trim sensitive env values before GitHub Actions log masking GitHub Actions' log masking is line-based: a secret value containing a newline only registers the first line as a mask, leaving the remainder exposed verbatim in logs. A trailing newline copied from a terminal into a GitHub Actions secret (e.g. ANTHROPIC_API_KEY) was enough to leak "a large part of the key" in run logs (pullfrog/pullfrog#41). normalizeEnv now trims leading/trailing whitespace from any value whose key matches the sensitive name pattern, masks the cleaned value, and warns when whitespace was stripped so the user notices the source. sanitizeSecret is reused for dbSecrets injection in main.ts. The three secret-store PUT/POST routes also trim values defensively, matching the existing name.trim() pattern. Real multi-line secrets are not used in practice — even GITHUB_PRIVATE_KEY PEMs are stored single-line with escaped \n and unescaped at the point of use — so a straight trim() is safe. * action: address review — use core.setSecret for masking, don't zero whitespace-only Pullfrog's review of #698 caught two real issues in the original fix: 1. `console.log(\`::add-mask::\${trimmed}\`)` doesn't escape \r/\n. If a value survives trim with an embedded newline (PEMs, kubeconfigs, JSON), the runner only registers the first line as a mask and the rest leaks. `core.setSecret(trimmed)` routes through @actions/core which percent-encodes \r/\n so the runner V2 parser decodes back to the full value and registers every non-empty line as a separate mask. Removes the load-bearing "no embedded newlines" invariant from the fix. 2. Whitespace-only sensitive values silently became "". Downstream truthy checks would flip from "set" to "missing" with no log. Now sanitizeSecret returns null in that case and callers skip the process.env write, surfacing a clear missing-key error instead. Tests rewritten to assert process.env state directly — no stdout spies. Masking correctness is delegated to @actions/core (trusted dependency). --- main.ts | 11 +++-- utils/normalizeEnv.test.ts | 91 ++++++++++++++++++++++++++++++++++++++ utils/normalizeEnv.ts | 58 ++++++++++++++++++------ 3 files changed, 143 insertions(+), 17 deletions(-) create mode 100644 utils/normalizeEnv.test.ts diff --git a/main.ts b/main.ts index 7d1ab89..3ee0d3c 100644 --- a/main.ts +++ b/main.ts @@ -30,7 +30,7 @@ import { createOctokit, writeGitHubUsageSummaryToFile } from "./utils/github.ts" import { resolveInstructions } from "./utils/instructions.ts"; import { readLearningsFile, seedLearningsFile } from "./utils/learnings.ts"; import { executeLifecycleHook } from "./utils/lifecycle.ts"; -import { normalizeEnv } from "./utils/normalizeEnv.ts"; +import { normalizeEnv, sanitizeSecret } from "./utils/normalizeEnv.ts"; import { aggregateUsage, patchWorkflowRunFields } from "./utils/patchWorkflowRunFields.ts"; import { resolvePayload, resolvePromptInput } from "./utils/payload.ts"; import { isRouterKeylimitExhaustedError } from "./utils/providerErrors.ts"; @@ -559,12 +559,15 @@ export async function main(): Promise { const runContext = await resolveRunContextData({ octokit: initialOctokit, token: jobToken }); timer.checkpoint("runContextData"); - // inject account-level secrets into process.env (YAML secrets take precedence) + // inject account-level secrets into process.env (YAML secrets take precedence). + // sanitizeSecret trims + masks so accidental trailing whitespace doesn't leak + // through GitHub Actions' line-based log masking. whitespace-only values + // return null and skip injection so the user sees a clear missing-key error. if (runContext.dbSecrets) { for (const [key, value] of Object.entries(runContext.dbSecrets)) { if (!process.env[key]) { - process.env[key] = value; - core.setSecret(value); + const sanitized = sanitizeSecret(key, value); + if (sanitized !== null) process.env[key] = sanitized; } } const count = Object.keys(runContext.dbSecrets).length; diff --git a/utils/normalizeEnv.test.ts b/utils/normalizeEnv.test.ts new file mode 100644 index 0000000..0d3c830 --- /dev/null +++ b/utils/normalizeEnv.test.ts @@ -0,0 +1,91 @@ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { normalizeEnv, sanitizeSecret } from "./normalizeEnv.ts"; + +/** + * These tests pin the load-bearing invariants of secret sanitisation: + * - sensitive values are trimmed before downstream code reads them + * - whitespace-only values are NOT silently zeroed (leave env unchanged) + * - case normalisation still happens + * + * Masking (`core.setSecret`) is delegated to `@actions/core` and trusted to + * work as documented — we don't spy on stdout to re-test the toolkit. + */ + +describe("normalizeEnv: process.env state contract", () => { + let originalEnv: NodeJS.ProcessEnv; + + beforeEach(() => { + // normalizeEnv() iterates the entire process.env, so the test must + // control it. snapshot + full wipe + restore is the cleanest isolation. + originalEnv = { ...process.env }; + for (const k of Object.keys(process.env)) delete process.env[k]; + }); + + afterEach(() => { + for (const k of Object.keys(process.env)) delete process.env[k]; + Object.assign(process.env, originalEnv); + }); + + it("trims trailing newline from sensitive env vars", () => { + process.env.ANTHROPIC_API_KEY = "sk-ant-secret-value\n"; + normalizeEnv(); + expect(process.env.ANTHROPIC_API_KEY).toBe("sk-ant-secret-value"); + }); + + it("trims surrounding whitespace including \\r\\n and spaces", () => { + process.env.OPENAI_API_KEY = " sk-openai-value\r\n "; + normalizeEnv(); + expect(process.env.OPENAI_API_KEY).toBe("sk-openai-value"); + }); + + it("leaves clean sensitive values untouched", () => { + process.env.ANTHROPIC_API_KEY = "sk-ant-clean"; + normalizeEnv(); + expect(process.env.ANTHROPIC_API_KEY).toBe("sk-ant-clean"); + }); + + it("ignores non-sensitive env vars", () => { + process.env.NODE_ENV = "production\n"; + normalizeEnv(); + expect(process.env.NODE_ENV).toBe("production\n"); + }); + + it("canonicalises case and trims the value", () => { + process.env.anthropic_api_key = "sk-ant-lowercase\n"; + normalizeEnv(); + expect(process.env.ANTHROPIC_API_KEY).toBe("sk-ant-lowercase"); + expect(process.env.anthropic_api_key).toBeUndefined(); + }); + + it("preserves whitespace-only values rather than silently zeroing them", () => { + // contract: don't mutate when value is whitespace-only. caller sees the + // misconfigured value verbatim and either fails clearly downstream or + // logs a missing-key error. + process.env.ANTHROPIC_API_KEY = " \n "; + normalizeEnv(); + expect(process.env.ANTHROPIC_API_KEY).toBe(" \n "); + }); + + it("preserves embedded newlines (toolkit masks each line)", () => { + // multi-line PEMs aren't used in practice, but if one slipped in via a + // DB secret we don't want to silently mutate it. trim() only touches + // the ends; @actions/core handles per-line masking via the runner. + process.env.ANTHROPIC_API_KEY = "line1\nline2"; + normalizeEnv(); + expect(process.env.ANTHROPIC_API_KEY).toBe("line1\nline2"); + }); +}); + +describe("sanitizeSecret return value", () => { + it("returns the trimmed value for a sensitive secret with trailing newline", () => { + expect(sanitizeSecret("ANTHROPIC_API_KEY", "sk-ant-secret\n")).toBe("sk-ant-secret"); + }); + + it("returns the value unchanged when no trimming is needed", () => { + expect(sanitizeSecret("ANTHROPIC_API_KEY", "sk-ant-clean")).toBe("sk-ant-clean"); + }); + + it("returns null for whitespace-only input so caller can skip injection", () => { + expect(sanitizeSecret("ANTHROPIC_API_KEY", " \n")).toBeNull(); + }); +}); diff --git a/utils/normalizeEnv.ts b/utils/normalizeEnv.ts index 1d3274d..2317528 100644 --- a/utils/normalizeEnv.ts +++ b/utils/normalizeEnv.ts @@ -1,11 +1,40 @@ +import * as core from "@actions/core"; import { log } from "./cli.ts"; import { isSensitiveEnvName } from "./secrets.ts"; -function maskValue(value: string | undefined) { - if (value && typeof value === "string" && value.trim().length > 0) { - // ::add-mask::value tells GitHub Actions to mask this value in logs - console.log(`::add-mask::${value}`); +/** + * Trim surrounding whitespace from a sensitive value and register it as a + * GitHub Actions log mask. Trailing newlines from terminal-copy paste are a + * common footgun: the value travels through GH Actions logs and any tool + * that re-emits parts of it leaks the unmasked tail. Trimming canonicalises + * the value so the mask matches exactly what downstream tools will print. + * + * Masking is delegated to `core.setSecret` (not raw `console.log`) so the + * toolkit percent-encodes `\r`/`\n`; the runner V2 parser decodes them and + * registers the full value plus every non-empty line as separate masks. That + * keeps us safe for embedded-newline values (PEMs, kubeconfigs, JSON blobs) + * even though they aren't currently used. + * + * Returns the trimmed value, or `null` when the input was whitespace-only — + * callers must leave `process.env` untouched in that case so a misconfigured + * value surfaces as a clear "missing key" downstream rather than silently + * mutating to the empty string. + */ +export function sanitizeSecret(key: string, value: string): string | null { + const trimmed = value.trim(); + if (trimmed.length === 0) { + log.warning( + `» ${key} is whitespace-only — leaving env var unchanged. check your secret value.` + ); + return null; } + if (trimmed !== value) { + log.warning( + `» stripped whitespace from ${key} (whitespace in secret values breaks GitHub Actions log masking)` + ); + } + core.setSecret(trimmed); + return trimmed; } /** @@ -15,7 +44,8 @@ function maskValue(value: string | undefined) { * If there are conflicts (same key with different capitalizations but different values), * logs a warning and keeps the uppercase version. * - * Also registers sensitive values as masks in GitHub Actions. + * Also trims and masks sensitive values so accidental trailing whitespace + * doesn't defeat GitHub Actions log masking. */ export function normalizeEnv(): void { const upperKeys = new Map(); @@ -30,14 +60,6 @@ export function normalizeEnv(): void { // process each group for (const [upperKey, keys] of upperKeys) { - // if sensitive, ensure we mask the value (regardless of whether we rename it or not) - if (isSensitiveEnvName(upperKey)) { - // mask all values associated with this key group - for (const key of keys) { - maskValue(process.env[key]); - } - } - if (keys.length === 1) { const key = keys[0]; if (key !== upperKey) { @@ -71,4 +93,14 @@ export function normalizeEnv(): void { // set the uppercase version process.env[upperKey] = preferredValue; } + + // trim + mask sensitive values after case normalisation so each key is + // visited exactly once with its final, canonical value + for (const key of Object.keys(process.env)) { + if (!isSensitiveEnvName(key)) continue; + const value = process.env[key]; + if (typeof value !== "string" || value.length === 0) continue; + const sanitized = sanitizeSecret(key, value); + if (sanitized !== null) process.env[key] = sanitized; + } }