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