action: trim sensitive env values before GitHub Actions log masking (#698)

* 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

@@ -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<MainResult> {
   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;
+        const sanitized = sanitizeSecret(key, value);
-        core.setSecret(value);
+        if (sanitized !== null) process.env[key] = sanitized;
       }
     }
     const count = Object.keys(runContext.dbSecrets).length;

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

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) {
+ * Trim surrounding whitespace from a sensitive value and register it as a
-    // ::add-mask::value tells GitHub Actions to mask this value in logs
+ * GitHub Actions log mask. Trailing newlines from terminal-copy paste are a
-    console.log(`::add-mask::${value}`);
+ * 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<string, string[]>();
@@ -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;
+  }
 }