From abbfa6691ef5bf379a9d86c85ae95d82d01a58ff Mon Sep 17 00:00:00 2001 From: pelikhan Date: Thu, 15 Oct 2026 21:39:08 -0700 Subject: [PATCH 1/4] Fix backend ownership and stack-safe artifact redaction Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...tch-agentic-workflows-backend-ownership.md | 5 ++ .../patch-large-record-mask-redaction.md | 5 ++ actions/setup/js/add_mask_redaction.cjs | 83 ++++++++++++++++--- actions/setup/js/add_mask_redaction.test.cjs | 35 ++++++++ actions/setup/js/redact_secrets.test.cjs | 44 ++++++++++ actions/setup/js/start_mcp_gateway.cjs | 33 ++++++++ actions/setup/js/start_mcp_gateway.test.cjs | 56 +++++++++++++ docs/src/content/docs/reference/imports.md | 2 + docs/src/content/docs/reference/tools.md | 2 + 9 files changed, 254 insertions(+), 11 deletions(-) create mode 100644 .changeset/patch-agentic-workflows-backend-ownership.md create mode 100644 .changeset/patch-large-record-mask-redaction.md diff --git a/.changeset/patch-agentic-workflows-backend-ownership.md b/.changeset/patch-agentic-workflows-backend-ownership.md new file mode 100644 index 00000000000..ffefdb0930c --- /dev/null +++ b/.changeset/patch-agentic-workflows-backend-ownership.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Run the built-in agentic-workflows MCP backend as the runner so private downloaded logs remain readable by artifact secret redaction, without loosening file permissions or bypassing fail-closed cleanup. diff --git a/.changeset/patch-large-record-mask-redaction.md b/.changeset/patch-large-record-mask-redaction.md new file mode 100644 index 00000000000..0ab130ef290 --- /dev/null +++ b/.changeset/patch-large-record-mask-redaction.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Avoid regular-expression stack exhaustion when redacting runtime masks from multi-megabyte JSON and mixed-log artifacts, while preserving fail-closed cleanup for genuine redaction failures. diff --git a/actions/setup/js/add_mask_redaction.cjs b/actions/setup/js/add_mask_redaction.cjs index 6c2cde4ba7c..66677f9d40e 100644 --- a/actions/setup/js/add_mask_redaction.cjs +++ b/actions/setup/js/add_mask_redaction.cjs @@ -138,6 +138,62 @@ function applyAddMaskRedaction(text, maskedValues) { return redactMaskedValues(withoutCommands, maskedValues); } +/** + * Visit quoted JSON tokens without a repeated regex alternation, which can + * exhaust the RegExp stack on multi-megabyte log records. + * @param {string} text + * @param {(token: string) => string} transformString + * @param {(text: string) => string} [transformText] + * @returns {string} + */ +function transformJSONStrings(text, transformString, transformText = value => value) { + const parts = []; + let cursor = 0; + while (cursor < text.length) { + const start = text.indexOf('"', cursor); + if (start === -1) { + parts.push(transformText(text.slice(cursor))); + break; + } + let end = start + 1; + let invalidString = false; + while (end < text.length && text[end] !== '"') { + if (text.charCodeAt(end) < 0x20) { + invalidString = true; + break; + } + if (text[end] === "\\") { + end++; + const escaped = text[end]; + if (escaped === "u") { + if (!/^[0-9a-fA-F]{4}$/.test(text.slice(end + 1, end + 5))) { + invalidString = true; + break; + } + end += 4; + } else if (!escaped || !'"\\/bfnrt'.includes(escaped)) { + invalidString = true; + break; + } + } + end++; + } + if (invalidString) { + parts.push(transformText(text.slice(cursor, start + 1))); + cursor = start + 1; + continue; + } + if (end >= text.length) { + parts.push(transformText(text.slice(cursor))); + break; + } + parts.push(transformText(text.slice(cursor, start))); + parts.push(transformString(text.slice(start, end + 1))); + cursor = end + 1; + } + return parts.join(""); +} + /** * Sanitize artifact sources while runtime masks are still available in memory. * Decode JSON strings first so escaping cannot hide a registered value, redact @@ -160,11 +216,15 @@ function redactArtifactMaskedValues(content, maskedValues) { let value = JSON.parse(text); // Skip quoted strings and match original numeric tokens at every depth so // rounding or exponent notation cannot hide a mask. - const numericRedacted = text.replace(/"(?:[^"\\]|\\.)*"|-?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?/g, token => { - if (token.startsWith('"')) return token; - const redacted = redactMaskedValues(token, maskedValues); - return redacted === token ? token : JSON.stringify(redacted); - }); + const numericRedacted = transformJSONStrings( + text, + token => token, + plain => + plain.replace(/-?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?/g, token => { + const redacted = redactMaskedValues(token, maskedValues); + return redacted === token ? token : JSON.stringify(redacted); + }) + ); if (numericRedacted !== text) value = JSON.parse(numericRedacted); const redacted = JSON.stringify(redactValue(value)); return redacted === JSON.stringify(value) ? numericRedacted : text.match(/^\s*/)[0] + redacted + text.match(/\s*$/)[0]; @@ -181,14 +241,15 @@ function redactArtifactMaskedValues(content, maskedValues) { const jsonLine = redactJson(line); if (jsonLine !== undefined) return jsonLine; // Mixed or malformed logs can still contain recoverable JSON strings. - const decoded = line.replace(/"(?:[^"\\]|\\.)*"/g, encoded => { + const decoded = transformJSONStrings(line, encoded => { + let value; try { - const value = JSON.parse(encoded); - const redacted = redactValue(value); - return redacted === value ? encoded : JSON.stringify(redacted); - } catch { - return encoded; + value = JSON.parse(encoded); + } catch (error) { + throw new Error("Unable to decode JSON log string during secret redaction", { cause: error }); } + const redacted = redactValue(value); + return redacted === value ? encoded : JSON.stringify(redacted); }); return redactMaskedValues(decoded, maskedValues); }) diff --git a/actions/setup/js/add_mask_redaction.test.cjs b/actions/setup/js/add_mask_redaction.test.cjs index 81355292823..9e788028c27 100644 --- a/actions/setup/js/add_mask_redaction.test.cjs +++ b/actions/setup/js/add_mask_redaction.test.cjs @@ -134,6 +134,41 @@ describe("add_mask_redaction", () => { }); describe("redactArtifactMaskedValues", () => { + it.each(["json", "mixed"])("redacts multi-megabyte %s records without exhausting the regex stack", shape => { + const mask = 'synthetic-"quoted"\\mask'; + const value = "x".repeat(10 * 1024 * 1024) + mask; + const record = JSON.stringify({ text: value, number: shape === "mixed" ? 42 : 12345678 }); + const prefix = shape === "mixed" ? "tool output: " : ""; + const redacted = redactArtifactMaskedValues(prefix + record, [mask, "12345678"]); + expect(redacted.includes(mask)).toBe(false); + expect(redacted.includes(JSON.stringify(mask).slice(1, -1))).toBe(false); + const parsed = JSON.parse(redacted.slice(prefix.length)); + expect(parsed.text.length).toBe(10 * 1024 * 1024 + 3); + expect(parsed.text.endsWith("***")).toBe(true); + expect(parsed.number).toBe(shape === "mixed" ? 42 : "***"); + }); + + it("handles many escaped quotes and both odd and even backslash runs", () => { + const mask = 'synthetic-"quoted"\\mask'; + const value = '\\"'.repeat(1024 * 1024) + mask + "\\\\"; + const record = JSON.stringify({ text: value, safe: "12345678" }); + const redacted = redactArtifactMaskedValues(record, [mask]); + const parsed = JSON.parse(redacted); + expect(parsed.text.length).toBe(2 * 1024 * 1024 + 5); + expect(parsed.text.endsWith("***\\\\")).toBe(true); + expect(parsed.safe).toBe("12345678"); + }); + + it.each(["\n", "\r", "\u2028", "\u2029"])("recovers encoded masks after an invalid escaped line terminator", terminator => { + const mask = 'synthetic-"quoted"\\mask'; + const prefix = '"invalid\\' + terminator; + expect(redactArtifactMaskedValues(prefix + JSON.stringify(mask), [mask])).toBe(prefix + '"***"'); + }); + + it.each(['"bad\\q ', '"bad\\u12gg ', '"bad\\u12 ', '"bad\u0001 ', '"bad\t ', '"bad\\v '])("recovers Unicode-encoded masks after invalid JSON string prefixes (%j)", prefix => { + expect(redactArtifactMaskedValues(prefix + '"\\u006fpaque"', ["opaque"])).toBe(prefix + '"***"'); + }); + it("redacts decoded JSON strings and keys without mutating syntax or primitives", () => { const secret = 'quote"\\percent%0A'; const value = { [secret]: [secret, 0, false, null], nested: { output: "first\nsecond" } }; diff --git a/actions/setup/js/redact_secrets.test.cjs b/actions/setup/js/redact_secrets.test.cjs index ad0adf21f58..fd6b44c081f 100644 --- a/actions/setup/js/redact_secrets.test.cjs +++ b/actions/setup/js/redact_secrets.test.cjs @@ -127,6 +127,50 @@ describe("redact_secrets.cjs", () => { } }), describe("file handling", () => { + it("retains and sanitizes a private multi-megabyte artifact instead of failing closed on regex stack exhaustion", () => { + const { processFile } = require("./redact_secrets.cjs"); + const file = path.join(tempDir, "large-record.jsonl"); + const mask = 'synthetic-"quoted"\\mask'; + fs.writeFileSync(file, JSON.stringify({ text: "x".repeat(10 * 1024 * 1024) + mask }), { mode: 0o600 }); + expect(processFile(file, [], [mask])).toBe(1); + expect(fs.existsSync(file)).toBe(true); + const parsed = JSON.parse(fs.readFileSync(file, "utf8")); + expect(parsed.text.endsWith("***")).toBe(true); + expect(parsed.text.length).toBe(10 * 1024 * 1024 + 3); + expect(fs.statSync(file).mode & 0o777).toBe(0o600); + expect(mockCore.setFailed).not.toHaveBeenCalled(); + }); + + it.each(['"bad\\q ', '"bad\\u12gg ', '"bad\\u12 ', '"bad\u0001 ', '"bad\t ', '"bad\\v '])("sanitizes recoverable encoded masks after invalid JSON string prefixes (%j)", prefix => { + const { processFile } = require("./redact_secrets.cjs"); + const file = path.join(tempDir, "malformed.log"); + fs.writeFileSync(file, prefix + '"\\u006fpaque"', { mode: 0o600 }); + expect(processFile(file, [], ["opaque"])).toBe(1); + expect(fs.readFileSync(file, "utf8")).toBe(prefix + '"***"'); + expect(fs.statSync(file).mode & 0o777).toBe(0o600); + expect(mockCore.setFailed).not.toHaveBeenCalled(); + }); + + it("removes the artifact if decoding a validated mixed-log token unexpectedly fails", () => { + const { processFile } = require("./redact_secrets.cjs"); + const file = path.join(tempDir, "mixed.log"); + fs.writeFileSync(file, 'output: "\\u006fpaque"'); + const parse = JSON.parse; + const parseSpy = vi.spyOn(JSON, "parse").mockImplementation(text => { + if (text === '"\\u006fpaque"') throw new Error("opaque runtime-mask"); + return parse(text); + }); + try { + expect(processFile(file, [], ["opaque"])).toBe(0); + expect(fs.existsSync(file)).toBe(false); + expect(mockCore.warning).toHaveBeenCalledWith("Secret redaction failed (stage=redact, code=unknown)"); + expect(mockCore.warning.mock.calls.flat().join("\n")).not.toMatch(/opaque|runtime-mask/); + expect(mockCore.setFailed).toHaveBeenCalled(); + } finally { + parseSpy.mockRestore(); + } + }); + it.each(["EACCES", "EPERM", "ENOSPC"])("reports only safe read diagnostics for secret-bearing %s errors", code => { const { processFile } = require("./redact_secrets.cjs"); const file = path.join(tempDir, "secret-value.log"); diff --git a/actions/setup/js/start_mcp_gateway.cjs b/actions/setup/js/start_mcp_gateway.cjs index 35805800e3c..fc26ae735b5 100644 --- a/actions/setup/js/start_mcp_gateway.cjs +++ b/actions/setup/js/start_mcp_gateway.cjs @@ -933,6 +933,7 @@ async function main() { core.setFailed("ERROR: Configuration is missing required 'gateway' section"); return; } + applyAgenticWorkflowsRunnerIdentity(configObj); applyGatewayRuntimeTimeouts(configObj, process.env); mcpConfig = JSON.stringify(configObj); if (!("port" in gw) || gw.port == null) { @@ -1480,7 +1481,39 @@ if (require.main === module) { }); } +/** + * Backend containers do not inherit the gateway container's Docker user. + * Keep private downloaded logs runner-owned so artifact redaction can read them. + * @param {Record} config + * @param {number | undefined} [uid] + * @param {number | undefined} [gid] + */ +function applyAgenticWorkflowsRunnerIdentity(config, uid = process.getuid?.(), gid = process.getgid?.()) { + const servers = config.mcpServers; + if (!servers || typeof servers !== "object" || !("agenticworkflows" in servers)) return; + const server = servers.agenticworkflows; + if (!server || typeof server !== "object" || !("container" in server) || typeof server.container !== "string" || !server.container) return; + const args = "args" in server ? server.args : []; + if (!Array.isArray(args) || !args.every(arg => typeof arg === "string")) { + throw new Error("Invalid agenticworkflows container arguments"); + } + if (args.some(arg => arg === "--user" || arg === "-u" || arg.startsWith("--user=") || (arg.startsWith("-u") && !arg.startsWith("--")))) return; + if (!Number.isSafeInteger(uid) || !Number.isSafeInteger(gid) || uid === undefined || gid === undefined || uid < 0 || gid < 0) { + throw new Error("Runner identity is required for the agenticworkflows container"); + } + const env = "env" in server ? server.env : {}; + if (!env || typeof env !== "object" || Array.isArray(env)) { + throw new Error("Invalid agenticworkflows container environment"); + } + Object.assign(server, { + args: [...args, "--user", `${uid}:${gid}`], + // /tmp is private to this backend container; /root is not writable as the runner. + env: { HOME: "/tmp", ...env }, + }); +} + module.exports = { + applyAgenticWorkflowsRunnerIdentity, applyOTLPIgnoreIfMissing, clearGatewayStartupMarker, createGatewayStderrPath, diff --git a/actions/setup/js/start_mcp_gateway.test.cjs b/actions/setup/js/start_mcp_gateway.test.cjs index 3b03f4a03ff..4f5995987b1 100644 --- a/actions/setup/js/start_mcp_gateway.test.cjs +++ b/actions/setup/js/start_mcp_gateway.test.cjs @@ -4,6 +4,7 @@ import path from "path"; import http from "http"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { + applyAgenticWorkflowsRunnerIdentity, applyOTLPIgnoreIfMissing, clearGatewayStartupMarker, createGatewayStderrPath, @@ -35,6 +36,61 @@ import { } from "./start_mcp_gateway.cjs"; import { writeMcpConfiguration } from "./mcp_configuration.cjs"; +describe("agenticworkflows backend ownership", () => { + it.each(["localhost/gh-aw:dev", "alpine:3.24"])("uses the runner identity without widening private artifact modes (%s)", container => { + const env = { GITHUB_TOKEN: "${GITHUB_TOKEN}", GITHUB_ACTOR: "${GITHUB_ACTOR}" }; + const backend = { container, args: ["--network", "host", "-w", "/workspace"], env }; + const other = { container: "example/server", args: [] }; + const config = { mcpServers: { agenticworkflows: backend, other } }; + applyAgenticWorkflowsRunnerIdentity(config, 1001, 123); + expect(backend.args).toEqual(["--network", "host", "-w", "/workspace", "--user", "1001:123"]); + expect(backend.env).toEqual({ HOME: "/tmp", ...env }); + expect(other).toEqual({ container: "example/server", args: [] }); + applyAgenticWorkflowsRunnerIdentity(config, 1001, 123); + expect(backend.args.filter(arg => arg === "--user")).toHaveLength(1); + }); + + it.each([["--user", "42:43"], ["--user=42:43"], ["-u", "42:43"], ["-u42:43"]])("preserves an explicitly configured container user: %j", (...args) => { + const backend = { container: "example/server", args, env: { HOME: "/custom" } }; + const original = structuredClone(backend); + applyAgenticWorkflowsRunnerIdentity({ mcpServers: { agenticworkflows: backend } }, 1001, 123); + expect(backend).toEqual(original); + }); + + it("preserves a configured HOME and supports default container arguments", () => { + const backend = { container: "alpine:3.24", env: { HOME: "/custom" } }; + applyAgenticWorkflowsRunnerIdentity({ mcpServers: { agenticworkflows: backend } }, 1001, 123); + expect(backend).toEqual({ container: "alpine:3.24", args: ["--user", "1001:123"], env: { HOME: "/custom" } }); + }); + + it.each([{}, { mcpServers: {} }, { mcpServers: { agenticworkflows: { url: "https://example.com/mcp" } } }])("leaves absent and HTTP backends unchanged", config => { + const original = structuredClone(config); + applyAgenticWorkflowsRunnerIdentity(config, 1001, 123); + expect(config).toEqual(original); + }); + + it.each([ + [-1, 123], + [1001, -1], + [NaN, 123], + [1001, Infinity], + [1.5, 123], + ])("rejects invalid runner identities (%s, %s)", (uid, gid) => { + const backend = { container: "alpine:3.24" }; + expect(() => applyAgenticWorkflowsRunnerIdentity({ mcpServers: { agenticworkflows: backend } }, uid, gid)).toThrow("Runner identity is required"); + expect(backend).toEqual({ container: "alpine:3.24" }); + }); + + it.each([{ args: "invalid" }, { args: [42] }, { env: [] }, { env: null }])("rejects malformed backend configuration without publishing it", fields => { + expect(() => applyAgenticWorkflowsRunnerIdentity({ mcpServers: { agenticworkflows: { container: "alpine:3.24", ...fields } } }, 1001, 123)).toThrow(/Invalid agenticworkflows container/); + }); + + it("applies identity before serializing the configuration sent to the gateway", () => { + const source = fs.readFileSync(new URL("./start_mcp_gateway.cjs", import.meta.url), "utf8"); + expect(source.indexOf("applyAgenticWorkflowsRunnerIdentity(configObj)")).toBeLessThan(source.indexOf("mcpConfig = JSON.stringify(configObj)")); + }); +}); + describe("start_mcp_gateway logging", () => { it("records only a sanitized configuration snapshot before launching the gateway", () => { const source = fs.readFileSync(new URL("./start_mcp_gateway.cjs", import.meta.url), "utf8"); diff --git a/docs/src/content/docs/reference/imports.md b/docs/src/content/docs/reference/imports.md index 5da2f90e468..796d6bb3fa3 100644 --- a/docs/src/content/docs/reference/imports.md +++ b/docs/src/content/docs/reference/imports.md @@ -388,6 +388,8 @@ Steps from imports run **before** steps defined in the main workflow. Imported p Built-in redaction scans supported text files under `/tmp/gh-aw` and `${RUNNER_TEMP}/gh-aw`. Files without secret matches are left unchanged unless `secret-masking.steps` configures custom redactors, which require writable files. When redaction or custom-hook preparation requires a write, a readable non-writable file, such as a container-owned log, is atomically replaced with a runner-owned copy with owner-only read/write permissions. If the parent directory is also read-only, such as a Go module cache directory, redaction instead makes the file owner-readable and writable in place without changing directory permissions. This requires permission to change the file's mode; it does not bypass file locks or inaccessible directories. +Runtime-mask redaction handles multi-megabyte JSON strings and mixed log records without relying on recursive regular-expression matching. This does not change GitHub's step-summary size limit. + Custom hooks must tolerate absent optional files and files that disappear between discovery and reading (`ENOENT`). Earlier cleanup or redaction can remove files, including symbolic links and artifacts that could not be sanitized. Do not create empty placeholders for missing logs or outputs. Custom hooks run during the initial workspace redaction, before graders execute. The later grader-output redaction pass sanitizes newly generated results but does not rerun custom hooks, so it does not prepare unchanged files for them. diff --git a/docs/src/content/docs/reference/tools.md b/docs/src/content/docs/reference/tools.md index 32b42dc102f..3efbf1d2f41 100644 --- a/docs/src/content/docs/reference/tools.md +++ b/docs/src/content/docs/reference/tools.md @@ -200,6 +200,8 @@ tools: See [GH-AW as an MCP Server](/gh-aw/reference/gh-aw-as-mcp-server/) for available operations. +The built-in container runs with the runner's UID and GID unless a container user is explicitly configured. This keeps downloaded logs runner-owned without widening their owner-only permissions, so the post-agent secret-redaction step can read them. Its default `HOME` is the backend container's ephemeral `/tmp`, not `/root`. Redaction failures still remove unsanitized sources and fail the step. + Workflow-writing tools (`add`, `update`, `fix`, and `compile`, including dry-run compilation) are disabled by default. Trusted workflows can opt in at server startup: ```aw wrap From 8812361d74e8ce34c66c36021467d58f97f49a6a Mon Sep 17 00:00:00 2001 From: pelikhan Date: Thu, 15 Oct 2026 21:57:54 -0700 Subject: [PATCH 2/4] Expand JSON string redaction scanner boundary tests Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- actions/setup/js/add_mask_redaction.test.cjs | 102 +++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/actions/setup/js/add_mask_redaction.test.cjs b/actions/setup/js/add_mask_redaction.test.cjs index 9e788028c27..d8fbda9cfc4 100644 --- a/actions/setup/js/add_mask_redaction.test.cjs +++ b/actions/setup/js/add_mask_redaction.test.cjs @@ -134,6 +134,108 @@ describe("add_mask_redaction", () => { }); describe("redactArtifactMaskedValues", () => { + describe("JSON string scanner boundaries", () => { + const encodedMask = '"\\u006fpaque"'; + const validStrings = [ + "", + "ordinary ASCII", + 'quote"then opaque', + "backslash\\then opaque", + "slash/then opaque", + "backspace\bthen opaque", + "formfeed\fthen opaque", + "newline\nthen opaque", + "return\rthen opaque", + "tab\tthen opaque", + "\u0000opaque\u001f", + "caf\u00e9 opaque \u2603", + "\ud83d\ude80opaque", + "\ud800opaque\udfff", + "\u2028opaque\u2029", + '""opaque""', + ]; + + it.each(validStrings.map(value => [JSON.stringify(value), value]))("matches decoded-string masking for valid token %s", (encoded, value) => { + const expected = JSON.stringify(value.replaceAll("opaque", "***")); + expect(redactArtifactMaskedValues(encoded, ["opaque"])).toBe(expected); + expect(redactArtifactMaskedValues(`before ${encoded} after`, ["opaque"])).toBe(`before ${expected} after`); + }); + + it.each(Array.from({ length: 17 }, (_, count) => count))("recognizes closing quotes after %i pairs of escaped backslashes", count => { + const safe = JSON.stringify("\\".repeat(count)); + const escapedQuote = JSON.stringify("\\".repeat(count) + '"opaque'); + const content = `prefix ${safe} ${encodedMask} ${escapedQuote} suffix`; + const expected = `prefix ${safe} "***" ${JSON.stringify("\\".repeat(count) + '"***')} suffix`; + expect(redactArtifactMaskedValues(content, ["opaque"])).toBe(expected); + }); + + it.each(["\\u006fpaque", "op\\u0061que", "\\u006F\\u0070\\u0061\\u0071\\u0075\\u0065"])("decodes Unicode escapes at every mask position (%s)", encoded => { + expect(redactArtifactMaskedValues(`log "${encoded}"`, ["opaque"])).toBe('log "***"'); + }); + + it.each(Array.from({ length: 32 }, (_, code) => code))("recovers after unescaped control character U+%i", code => { + const prefix = '"invalid' + String.fromCharCode(code) + " "; + expect(redactArtifactMaskedValues(prefix + encodedMask, ["opaque"])).toBe(prefix + '"***"'); + }); + + it.each(["a", "c", "e", "v", "x", "0", "1", "'", "U", " ", "\t", "\r", "\n", "\u2028", "\u2029"])("recovers after illegal escaped character %j", escaped => { + const prefix = '"invalid\\' + escaped + " "; + expect(redactArtifactMaskedValues(prefix + encodedMask, ["opaque"])).toBe(prefix + '"***"'); + }); + + it.each(["", "0", "00", "000", "gggg", "000g", "00g0", "0g00", "g000", "+000", "-000", " 000"])("recovers after malformed Unicode escape %j", digits => { + const prefix = '"invalid\\u' + digits + " "; + expect(redactArtifactMaskedValues(prefix + encodedMask, ["opaque"])).toBe(prefix + '"***"'); + }); + + it.each(["", "plain", '"', '"unterminated', '"ends\\', '"ends\\u', '"ends\\u123', '"bad\\q', '""', '"" ""', '"safe"tail', '"safe" "unterminated'])("preserves unmatched or truncated input byte-for-byte (%j)", content => { + expect(redactArtifactMaskedValues(content, ["absent"])).toBe(content); + }); + + it.each(['"unterminated opaque', '"ends opaque\\', '"bad\\q opaque', 'opaque "safe" opaque', '"safe" opaque "unterminated'])("still applies raw masks outside recoverable tokens (%j)", content => { + expect(redactArtifactMaskedValues(content, ["opaque"])).toBe(content.replaceAll("opaque", "***")); + }); + + it("handles adjacent strings, empty strings, invalid prefixes, and trailing text together", () => { + const content = `start ""${encodedMask}""${encodedMask} "bad\\q ${encodedMask} trailing`; + const redactedToken = JSON.stringify("***"); + expect(redactArtifactMaskedValues(content, ["opaque"])).toBe(`start ""${redactedToken}""${redactedToken} "bad\\q ${redactedToken} trailing`); + }); + + it("keeps literal escaped Unicode distinct from an actual encoded mask", () => { + const literal = JSON.stringify("\\u006fpaque"); + expect(redactArtifactMaskedValues(`log ${literal} ${encodedMask}`, ["opaque"])).toBe(`log ${literal} "***"`); + }); + + it("keeps numeric-looking string tokens separate from original numeric tokens", () => { + const content = ' \t{"escaped":"\\u0031e400","quoted":"9007199254740993","ids":[1e400,9007199254740993,-0,1e+09]}\r\n'; + const redacted = redactArtifactMaskedValues(content, ["1e400", "9007199254740993"]); + expect(JSON.parse(redacted)).toEqual({ escaped: "***", quoted: "***", ids: ["***", "***", 0, 1000000000] }); + expect(redacted.startsWith(" \t")).toBe(true); + expect(redacted.endsWith("\r\n")).toBe(true); + }); + + it("matches a deterministic decoded-string oracle across generated escape combinations", () => { + const alphabet = ["a", '"', "\\", "/", "\b", "\f", "\n", "\r", "\t", "\u0000", "\u001f", "\u2028", "\ud800", "\ud83d\ude80"]; + for (let index = 0; index < 256; index++) { + const left = alphabet[index % alphabet.length]; + const right = alphabet[Math.floor(index / alphabet.length) % alphabet.length]; + const value = left + right + "opaque" + right + left; + const encoded = JSON.stringify(value); + const expected = JSON.stringify(value.replaceAll("opaque", "***")); + expect(redactArtifactMaskedValues(`record ${encoded} end`, ["opaque"]), `generated record ${index}`).toBe(`record ${expected} end`); + const untouched = `record ${encoded} end`; + expect(redactArtifactMaskedValues(untouched, ["absent"]), `unchanged record ${index}`).toBe(untouched); + } + }); + + it("redacts thousands of adjacent tokens without losing delimiters or unchanged bytes", () => { + const safe = '"safe\\\\\\"value"'; + const record = `prefix ${safe}:${encodedMask}:`; + expect(redactArtifactMaskedValues(record.repeat(4096) + "tail", ["opaque"])).toBe(`prefix ${safe}:"***":`.repeat(4096) + "tail"); + }); + }); + it.each(["json", "mixed"])("redacts multi-megabyte %s records without exhausting the regex stack", shape => { const mask = 'synthetic-"quoted"\\mask'; const value = "x".repeat(10 * 1024 * 1024) + mask; From b681ac5f07cb0e8f3a6648fc3904c69a250681f7 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Thu, 15 Oct 2026 22:05:50 -0700 Subject: [PATCH 3/4] Close malformed-log mask gaps and address scanner review Preserve Docker named-user overrides and add direct scanner tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- actions/setup/js/add_mask_redaction.cjs | 14 +++- actions/setup/js/add_mask_redaction.test.cjs | 75 +++++++++++++++++++- actions/setup/js/redact_secrets.test.cjs | 12 ++++ actions/setup/js/start_mcp_gateway.cjs | 1 + actions/setup/js/start_mcp_gateway.test.cjs | 21 +++++- docs/src/content/docs/reference/imports.md | 2 +- 6 files changed, 120 insertions(+), 5 deletions(-) diff --git a/actions/setup/js/add_mask_redaction.cjs b/actions/setup/js/add_mask_redaction.cjs index 66677f9d40e..5b524ab0c3a 100644 --- a/actions/setup/js/add_mask_redaction.cjs +++ b/actions/setup/js/add_mask_redaction.cjs @@ -141,6 +141,7 @@ function applyAddMaskRedaction(text, maskedValues) { /** * Visit quoted JSON tokens without a repeated regex alternation, which can * exhaust the RegExp stack on multi-megabyte log records. + * Malformed prefixes are preserved as text; their quote pairing is ambiguous. * @param {string} text * @param {(token: string) => string} transformString * @param {(text: string) => string} [transformText] @@ -251,7 +252,17 @@ function redactArtifactMaskedValues(content, maskedValues) { const redacted = redactValue(value); return redacted === value ? encoded : JSON.stringify(redacted); }); - return redactMaskedValues(decoded, maskedValues); + const redacted = redactMaskedValues(decoded, maskedValues); + // Malformed quotes have no unique pairing. Decode remaining escapes once + // conservatively so an alternate pairing cannot hide an encoded mask. + const escapes = { '"': '"', "\\": "\\", "/": "/", b: "\b", f: "\f", n: "\n", r: "\r", t: "\t" }; + const unescaped = redacted.replace(/\\(["\\/bfnrt]|u[0-9a-fA-F]{4})/g, (_, escape) => { + if (escape[0] === "u") return String.fromCharCode(parseInt(escape.slice(1), 16)); + return escapes[escape]; + }); + const conservative = applyAddMaskRedaction(unescaped, maskedValues); + // Only re-encode an ambiguous record when the extra decoding finds a mask. + return conservative === unescaped ? redacted : JSON.stringify(conservative); }) .join("\n"); } @@ -264,5 +275,6 @@ module.exports = { isAddMaskCommandLine, redactArtifactMaskedValues, redactMaskedValues, + transformJSONStrings, unescapeWorkflowCommandValue, }; diff --git a/actions/setup/js/add_mask_redaction.test.cjs b/actions/setup/js/add_mask_redaction.test.cjs index d8fbda9cfc4..c7b4f0edc94 100644 --- a/actions/setup/js/add_mask_redaction.test.cjs +++ b/actions/setup/js/add_mask_redaction.test.cjs @@ -3,9 +3,57 @@ import { describe, it, expect, vi } from "vitest"; import { createRequire } from "module"; const require = createRequire(import.meta.url); -const { applyAddMaskRedaction, collectAddMaskedValues, isAddMaskCommandLine, redactArtifactMaskedValues, redactMaskedValues, unescapeWorkflowCommandValue } = require("./add_mask_redaction.cjs"); +const { applyAddMaskRedaction, collectAddMaskedValues, isAddMaskCommandLine, redactArtifactMaskedValues, redactMaskedValues, transformJSONStrings, unescapeWorkflowCommandValue } = require("./add_mask_redaction.cjs"); describe("add_mask_redaction", () => { + describe("transformJSONStrings", () => { + it("passes complete quoted tokens and plain spans to separate callbacks", () => { + const strings = vi.fn(token => `[${token}]`); + const plain = vi.fn(text => `{${text}}`); + expect(transformJSONStrings('head "one" between "two" tail', strings, plain)).toBe('{head }["one"]{ between }["two"]{ tail}'); + expect(strings.mock.calls.map(([token]) => token)).toEqual(['"one"', '"two"']); + expect(plain.mock.calls.map(([text]) => text)).toEqual(["head ", " between ", " tail"]); + }); + + it("visits adjacent and empty tokens without separators or trailing text", () => { + const strings = vi.fn(token => token); + const content = '"""one""two"""'; + expect(transformJSONStrings(content, strings)).toBe(content); + expect(strings.mock.calls.map(([token]) => token)).toEqual(['""', '"one"', '"two"', '""']); + }); + + it.each(['"unterminated', '"ends\\', '"ends\\u', '"ends\\u123', 'plain "unterminated', '"safe" "ends\\'])("preserves truncated input and never transforms incomplete strings (%j)", content => { + const strings = vi.fn(token => token); + const plain = vi.fn(text => text); + expect(transformJSONStrings(content, strings, plain)).toBe(content); + expect(strings.mock.calls.map(([token]) => token)).toEqual(content.startsWith('"safe"') ? ['"safe"'] : []); + expect(plain.mock.calls.map(([text]) => text).join("")).toBe(content.startsWith('"safe"') ? content.slice(6) : content); + }); + + it.each(['"bad\t ', '"bad\\q ', '"bad\\u12gg '])("recovers valid tokens after invalid unterminated prefixes without losing bytes (%j)", prefix => { + const strings = vi.fn(token => token); + const plain = vi.fn(text => text); + expect(transformJSONStrings(prefix + '"\\u006fpaque" tail', strings, plain)).toBe(prefix + '"\\u006fpaque" tail'); + expect(strings.mock.calls.map(([token]) => token)).toEqual(['"\\u006fpaque"']); + expect(plain.mock.calls.map(([text]) => text).join("")).toBe(prefix + " tail"); + }); + + it("preserves bytes even when malformed terminated input makes quote pairing ambiguous", () => { + const strings = vi.fn(token => token); + const content = '"bad\t" "\\u006fpaque"'; + expect(transformJSONStrings(content, strings)).toBe(content); + expect(strings.mock.calls.map(([token]) => token)).toEqual(['" "']); + }); + + it.each(["string", "plain"])("propagates %s callback failures without adding source text", callback => { + const error = new Error("synthetic callback failure"); + const fail = () => { + throw error; + }; + expect(() => transformJSONStrings('plain "value"', callback === "string" ? fail : token => token, callback === "plain" ? fail : text => text)).toThrow(error); + }); + }); + describe("isAddMaskCommandLine", () => { it("detects add-mask command lines", () => { expect(isAddMaskCommandLine("::add-mask::secret")).toBe(true); @@ -188,6 +236,31 @@ describe("add_mask_redaction", () => { expect(redactArtifactMaskedValues(prefix + encodedMask, ["opaque"])).toBe(prefix + '"***"'); }); + it.each(['"bad\t"', '"bad\u0001"', '"bad\\q"', '"bad\\u12gg"'])("conservatively redacts alternate quote pairings after terminated malformed tokens (%j)", prefix => { + const content = `${prefix} ${encodedMask}`; + const redacted = redactArtifactMaskedValues(content, ["opaque"]); + expect(JSON.parse(redacted)).toBe(`${prefix} "***"`); + expect(redacted).not.toContain("\\u006fpaque"); + expect(redactArtifactMaskedValues(content, ["absent"])).toBe(content); + }); + + it("conservatively redacts escapes before invalid controls and in unterminated strings", () => { + for (const content of ['"\\u006fpaque\t"', '"\\u006fpaque', '"\\u006fpaque\\']) { + const redacted = redactArtifactMaskedValues(content, ["opaque"]); + expect(JSON.parse(redacted)).toBe(content.replace("\\u006fpaque", "***")); + } + }); + + it("redacts encoded masks crossing quote boundaries in an alternate pairing", () => { + const content = '"bad\t" "foo\\"bar"'; + expect(JSON.parse(redactArtifactMaskedValues(content, ['foo"bar']))).toBe('"bad\t" "***"'); + }); + + it("does not recursively decode literal Unicode escapes after malformed prefixes", () => { + const content = '"bad\t" ' + JSON.stringify("\\u006fpaque"); + expect(redactArtifactMaskedValues(content, ["opaque"])).toBe(content); + }); + it.each(["", "plain", '"', '"unterminated', '"ends\\', '"ends\\u', '"ends\\u123', '"bad\\q', '""', '"" ""', '"safe"tail', '"safe" "unterminated'])("preserves unmatched or truncated input byte-for-byte (%j)", content => { expect(redactArtifactMaskedValues(content, ["absent"])).toBe(content); }); diff --git a/actions/setup/js/redact_secrets.test.cjs b/actions/setup/js/redact_secrets.test.cjs index fd6b44c081f..947927a2165 100644 --- a/actions/setup/js/redact_secrets.test.cjs +++ b/actions/setup/js/redact_secrets.test.cjs @@ -151,6 +151,18 @@ describe("redact_secrets.cjs", () => { expect(mockCore.setFailed).not.toHaveBeenCalled(); }); + it.each(['"bad\t"', '"bad\\q"', '"bad\\u12gg"'])("conservatively sanitizes alternate quote pairings after malformed terminated tokens (%j)", prefix => { + const { processFile } = require("./redact_secrets.cjs"); + const file = path.join(tempDir, "ambiguous.log"); + fs.writeFileSync(file, prefix + ' "\\u006fpaque"', { mode: 0o600 }); + expect(processFile(file, [], ["opaque"])).toBe(1); + const redacted = fs.readFileSync(file, "utf8"); + expect(JSON.parse(redacted)).toBe(prefix + ' "***"'); + expect(redacted).not.toContain("\\u006fpaque"); + expect(fs.statSync(file).mode & 0o777).toBe(0o600); + expect(mockCore.setFailed).not.toHaveBeenCalled(); + }); + it("removes the artifact if decoding a validated mixed-log token unexpectedly fails", () => { const { processFile } = require("./redact_secrets.cjs"); const file = path.join(tempDir, "mixed.log"); diff --git a/actions/setup/js/start_mcp_gateway.cjs b/actions/setup/js/start_mcp_gateway.cjs index fc26ae735b5..d9fea7dc3e6 100644 --- a/actions/setup/js/start_mcp_gateway.cjs +++ b/actions/setup/js/start_mcp_gateway.cjs @@ -1497,6 +1497,7 @@ function applyAgenticWorkflowsRunnerIdentity(config, uid = process.getuid?.(), g if (!Array.isArray(args) || !args.every(arg => typeof arg === "string")) { throw new Error("Invalid agenticworkflows container arguments"); } + // Docker parses any suffix after -u as a username or UID, including -uroot. if (args.some(arg => arg === "--user" || arg === "-u" || arg.startsWith("--user=") || (arg.startsWith("-u") && !arg.startsWith("--")))) return; if (!Number.isSafeInteger(uid) || !Number.isSafeInteger(gid) || uid === undefined || gid === undefined || uid < 0 || gid < 0) { throw new Error("Runner identity is required for the agenticworkflows container"); diff --git a/actions/setup/js/start_mcp_gateway.test.cjs b/actions/setup/js/start_mcp_gateway.test.cjs index 4f5995987b1..40b0c8fd3a4 100644 --- a/actions/setup/js/start_mcp_gateway.test.cjs +++ b/actions/setup/js/start_mcp_gateway.test.cjs @@ -50,13 +50,30 @@ describe("agenticworkflows backend ownership", () => { expect(backend.args.filter(arg => arg === "--user")).toHaveLength(1); }); - it.each([["--user", "42:43"], ["--user=42:43"], ["-u", "42:43"], ["-u42:43"]])("preserves an explicitly configured container user: %j", (...args) => { - const backend = { container: "example/server", args, env: { HOME: "/custom" } }; + it.each([["--user", "42:43"], ["--user=42:43"], ["-u", "42:43"], ["-u42:43"], ["--user", "app:staff"], ["--user=app:staff"], ["-u", "app:staff"], ["-uroot"], ["-uapp:staff"], ["-u=app:staff"]])( + "preserves an explicitly configured container user: %j", + (...args) => { + const backend = { container: "example/server", args, env: { HOME: "/custom" } }; + const original = structuredClone(backend); + applyAgenticWorkflowsRunnerIdentity({ mcpServers: { agenticworkflows: backend } }, 1001, 123); + applyAgenticWorkflowsRunnerIdentity({ mcpServers: { agenticworkflows: backend } }, 1001, 123); + expect(backend).toEqual(original); + } + ); + + it.each(["-update", "-unshare"])("preserves %s as Docker's attached -u username, not an unrelated flag", arg => { + const backend = { container: "example/server", args: [arg], env: { HOME: "/custom" } }; const original = structuredClone(backend); applyAgenticWorkflowsRunnerIdentity({ mcpServers: { agenticworkflows: backend } }, 1001, 123); expect(backend).toEqual(original); }); + it("does not confuse --userns with an explicit container user", () => { + const backend = { container: "example/server", args: ["--userns=host"], env: { HOME: "/custom" } }; + applyAgenticWorkflowsRunnerIdentity({ mcpServers: { agenticworkflows: backend } }, 1001, 123); + expect(backend).toEqual({ container: "example/server", args: ["--userns=host", "--user", "1001:123"], env: { HOME: "/custom" } }); + }); + it("preserves a configured HOME and supports default container arguments", () => { const backend = { container: "alpine:3.24", env: { HOME: "/custom" } }; applyAgenticWorkflowsRunnerIdentity({ mcpServers: { agenticworkflows: backend } }, 1001, 123); diff --git a/docs/src/content/docs/reference/imports.md b/docs/src/content/docs/reference/imports.md index 796d6bb3fa3..4f5588d0ac8 100644 --- a/docs/src/content/docs/reference/imports.md +++ b/docs/src/content/docs/reference/imports.md @@ -388,7 +388,7 @@ Steps from imports run **before** steps defined in the main workflow. Imported p Built-in redaction scans supported text files under `/tmp/gh-aw` and `${RUNNER_TEMP}/gh-aw`. Files without secret matches are left unchanged unless `secret-masking.steps` configures custom redactors, which require writable files. When redaction or custom-hook preparation requires a write, a readable non-writable file, such as a container-owned log, is atomically replaced with a runner-owned copy with owner-only read/write permissions. If the parent directory is also read-only, such as a Go module cache directory, redaction instead makes the file owner-readable and writable in place without changing directory permissions. This requires permission to change the file's mode; it does not bypass file locks or inaccessible directories. -Runtime-mask redaction handles multi-megabyte JSON strings and mixed log records without relying on recursive regular-expression matching. This does not change GitHub's step-summary size limit. +Runtime-mask redaction handles multi-megabyte JSON strings and mixed log records without relying on recursive regular-expression matching. If malformed quotes hide an encoded mask, a conservative escape-decoding pass redacts it and represents that record as a JSON string; records without additional mask matches retain their original bytes. This does not change GitHub's step-summary size limit. Custom hooks must tolerate absent optional files and files that disappear between discovery and reading (`ENOENT`). Earlier cleanup or redaction can remove files, including symbolic links and artifacts that could not be sanitized. Do not create empty placeholders for missing logs or outputs. From 50b4f393115a377f7d8d7ec4aa54adc4d7a4e3b7 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Thu, 15 Oct 2026 22:11:37 -0700 Subject: [PATCH 4/4] Split secret parsing changes into an independent PR Keep this PR scoped to backend file ownership and runner identity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../patch-large-record-mask-redaction.md | 5 - actions/setup/js/add_mask_redaction.cjs | 97 +------- actions/setup/js/add_mask_redaction.test.cjs | 212 +----------------- actions/setup/js/redact_secrets.test.cjs | 56 ----- docs/src/content/docs/reference/imports.md | 2 - 5 files changed, 13 insertions(+), 359 deletions(-) delete mode 100644 .changeset/patch-large-record-mask-redaction.md diff --git a/.changeset/patch-large-record-mask-redaction.md b/.changeset/patch-large-record-mask-redaction.md deleted file mode 100644 index 0ab130ef290..00000000000 --- a/.changeset/patch-large-record-mask-redaction.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"gh-aw": patch ---- - -Avoid regular-expression stack exhaustion when redacting runtime masks from multi-megabyte JSON and mixed-log artifacts, while preserving fail-closed cleanup for genuine redaction failures. diff --git a/actions/setup/js/add_mask_redaction.cjs b/actions/setup/js/add_mask_redaction.cjs index 5b524ab0c3a..6c2cde4ba7c 100644 --- a/actions/setup/js/add_mask_redaction.cjs +++ b/actions/setup/js/add_mask_redaction.cjs @@ -138,63 +138,6 @@ function applyAddMaskRedaction(text, maskedValues) { return redactMaskedValues(withoutCommands, maskedValues); } -/** - * Visit quoted JSON tokens without a repeated regex alternation, which can - * exhaust the RegExp stack on multi-megabyte log records. - * Malformed prefixes are preserved as text; their quote pairing is ambiguous. - * @param {string} text - * @param {(token: string) => string} transformString - * @param {(text: string) => string} [transformText] - * @returns {string} - */ -function transformJSONStrings(text, transformString, transformText = value => value) { - const parts = []; - let cursor = 0; - while (cursor < text.length) { - const start = text.indexOf('"', cursor); - if (start === -1) { - parts.push(transformText(text.slice(cursor))); - break; - } - let end = start + 1; - let invalidString = false; - while (end < text.length && text[end] !== '"') { - if (text.charCodeAt(end) < 0x20) { - invalidString = true; - break; - } - if (text[end] === "\\") { - end++; - const escaped = text[end]; - if (escaped === "u") { - if (!/^[0-9a-fA-F]{4}$/.test(text.slice(end + 1, end + 5))) { - invalidString = true; - break; - } - end += 4; - } else if (!escaped || !'"\\/bfnrt'.includes(escaped)) { - invalidString = true; - break; - } - } - end++; - } - if (invalidString) { - parts.push(transformText(text.slice(cursor, start + 1))); - cursor = start + 1; - continue; - } - if (end >= text.length) { - parts.push(transformText(text.slice(cursor))); - break; - } - parts.push(transformText(text.slice(cursor, start))); - parts.push(transformString(text.slice(start, end + 1))); - cursor = end + 1; - } - return parts.join(""); -} - /** * Sanitize artifact sources while runtime masks are still available in memory. * Decode JSON strings first so escaping cannot hide a registered value, redact @@ -217,15 +160,11 @@ function redactArtifactMaskedValues(content, maskedValues) { let value = JSON.parse(text); // Skip quoted strings and match original numeric tokens at every depth so // rounding or exponent notation cannot hide a mask. - const numericRedacted = transformJSONStrings( - text, - token => token, - plain => - plain.replace(/-?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?/g, token => { - const redacted = redactMaskedValues(token, maskedValues); - return redacted === token ? token : JSON.stringify(redacted); - }) - ); + const numericRedacted = text.replace(/"(?:[^"\\]|\\.)*"|-?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?/g, token => { + if (token.startsWith('"')) return token; + const redacted = redactMaskedValues(token, maskedValues); + return redacted === token ? token : JSON.stringify(redacted); + }); if (numericRedacted !== text) value = JSON.parse(numericRedacted); const redacted = JSON.stringify(redactValue(value)); return redacted === JSON.stringify(value) ? numericRedacted : text.match(/^\s*/)[0] + redacted + text.match(/\s*$/)[0]; @@ -242,27 +181,16 @@ function redactArtifactMaskedValues(content, maskedValues) { const jsonLine = redactJson(line); if (jsonLine !== undefined) return jsonLine; // Mixed or malformed logs can still contain recoverable JSON strings. - const decoded = transformJSONStrings(line, encoded => { - let value; + const decoded = line.replace(/"(?:[^"\\]|\\.)*"/g, encoded => { try { - value = JSON.parse(encoded); - } catch (error) { - throw new Error("Unable to decode JSON log string during secret redaction", { cause: error }); + const value = JSON.parse(encoded); + const redacted = redactValue(value); + return redacted === value ? encoded : JSON.stringify(redacted); + } catch { + return encoded; } - const redacted = redactValue(value); - return redacted === value ? encoded : JSON.stringify(redacted); - }); - const redacted = redactMaskedValues(decoded, maskedValues); - // Malformed quotes have no unique pairing. Decode remaining escapes once - // conservatively so an alternate pairing cannot hide an encoded mask. - const escapes = { '"': '"', "\\": "\\", "/": "/", b: "\b", f: "\f", n: "\n", r: "\r", t: "\t" }; - const unescaped = redacted.replace(/\\(["\\/bfnrt]|u[0-9a-fA-F]{4})/g, (_, escape) => { - if (escape[0] === "u") return String.fromCharCode(parseInt(escape.slice(1), 16)); - return escapes[escape]; }); - const conservative = applyAddMaskRedaction(unescaped, maskedValues); - // Only re-encode an ambiguous record when the extra decoding finds a mask. - return conservative === unescaped ? redacted : JSON.stringify(conservative); + return redactMaskedValues(decoded, maskedValues); }) .join("\n"); } @@ -275,6 +203,5 @@ module.exports = { isAddMaskCommandLine, redactArtifactMaskedValues, redactMaskedValues, - transformJSONStrings, unescapeWorkflowCommandValue, }; diff --git a/actions/setup/js/add_mask_redaction.test.cjs b/actions/setup/js/add_mask_redaction.test.cjs index c7b4f0edc94..81355292823 100644 --- a/actions/setup/js/add_mask_redaction.test.cjs +++ b/actions/setup/js/add_mask_redaction.test.cjs @@ -3,57 +3,9 @@ import { describe, it, expect, vi } from "vitest"; import { createRequire } from "module"; const require = createRequire(import.meta.url); -const { applyAddMaskRedaction, collectAddMaskedValues, isAddMaskCommandLine, redactArtifactMaskedValues, redactMaskedValues, transformJSONStrings, unescapeWorkflowCommandValue } = require("./add_mask_redaction.cjs"); +const { applyAddMaskRedaction, collectAddMaskedValues, isAddMaskCommandLine, redactArtifactMaskedValues, redactMaskedValues, unescapeWorkflowCommandValue } = require("./add_mask_redaction.cjs"); describe("add_mask_redaction", () => { - describe("transformJSONStrings", () => { - it("passes complete quoted tokens and plain spans to separate callbacks", () => { - const strings = vi.fn(token => `[${token}]`); - const plain = vi.fn(text => `{${text}}`); - expect(transformJSONStrings('head "one" between "two" tail', strings, plain)).toBe('{head }["one"]{ between }["two"]{ tail}'); - expect(strings.mock.calls.map(([token]) => token)).toEqual(['"one"', '"two"']); - expect(plain.mock.calls.map(([text]) => text)).toEqual(["head ", " between ", " tail"]); - }); - - it("visits adjacent and empty tokens without separators or trailing text", () => { - const strings = vi.fn(token => token); - const content = '"""one""two"""'; - expect(transformJSONStrings(content, strings)).toBe(content); - expect(strings.mock.calls.map(([token]) => token)).toEqual(['""', '"one"', '"two"', '""']); - }); - - it.each(['"unterminated', '"ends\\', '"ends\\u', '"ends\\u123', 'plain "unterminated', '"safe" "ends\\'])("preserves truncated input and never transforms incomplete strings (%j)", content => { - const strings = vi.fn(token => token); - const plain = vi.fn(text => text); - expect(transformJSONStrings(content, strings, plain)).toBe(content); - expect(strings.mock.calls.map(([token]) => token)).toEqual(content.startsWith('"safe"') ? ['"safe"'] : []); - expect(plain.mock.calls.map(([text]) => text).join("")).toBe(content.startsWith('"safe"') ? content.slice(6) : content); - }); - - it.each(['"bad\t ', '"bad\\q ', '"bad\\u12gg '])("recovers valid tokens after invalid unterminated prefixes without losing bytes (%j)", prefix => { - const strings = vi.fn(token => token); - const plain = vi.fn(text => text); - expect(transformJSONStrings(prefix + '"\\u006fpaque" tail', strings, plain)).toBe(prefix + '"\\u006fpaque" tail'); - expect(strings.mock.calls.map(([token]) => token)).toEqual(['"\\u006fpaque"']); - expect(plain.mock.calls.map(([text]) => text).join("")).toBe(prefix + " tail"); - }); - - it("preserves bytes even when malformed terminated input makes quote pairing ambiguous", () => { - const strings = vi.fn(token => token); - const content = '"bad\t" "\\u006fpaque"'; - expect(transformJSONStrings(content, strings)).toBe(content); - expect(strings.mock.calls.map(([token]) => token)).toEqual(['" "']); - }); - - it.each(["string", "plain"])("propagates %s callback failures without adding source text", callback => { - const error = new Error("synthetic callback failure"); - const fail = () => { - throw error; - }; - expect(() => transformJSONStrings('plain "value"', callback === "string" ? fail : token => token, callback === "plain" ? fail : text => text)).toThrow(error); - }); - }); - describe("isAddMaskCommandLine", () => { it("detects add-mask command lines", () => { expect(isAddMaskCommandLine("::add-mask::secret")).toBe(true); @@ -182,168 +134,6 @@ describe("add_mask_redaction", () => { }); describe("redactArtifactMaskedValues", () => { - describe("JSON string scanner boundaries", () => { - const encodedMask = '"\\u006fpaque"'; - const validStrings = [ - "", - "ordinary ASCII", - 'quote"then opaque', - "backslash\\then opaque", - "slash/then opaque", - "backspace\bthen opaque", - "formfeed\fthen opaque", - "newline\nthen opaque", - "return\rthen opaque", - "tab\tthen opaque", - "\u0000opaque\u001f", - "caf\u00e9 opaque \u2603", - "\ud83d\ude80opaque", - "\ud800opaque\udfff", - "\u2028opaque\u2029", - '""opaque""', - ]; - - it.each(validStrings.map(value => [JSON.stringify(value), value]))("matches decoded-string masking for valid token %s", (encoded, value) => { - const expected = JSON.stringify(value.replaceAll("opaque", "***")); - expect(redactArtifactMaskedValues(encoded, ["opaque"])).toBe(expected); - expect(redactArtifactMaskedValues(`before ${encoded} after`, ["opaque"])).toBe(`before ${expected} after`); - }); - - it.each(Array.from({ length: 17 }, (_, count) => count))("recognizes closing quotes after %i pairs of escaped backslashes", count => { - const safe = JSON.stringify("\\".repeat(count)); - const escapedQuote = JSON.stringify("\\".repeat(count) + '"opaque'); - const content = `prefix ${safe} ${encodedMask} ${escapedQuote} suffix`; - const expected = `prefix ${safe} "***" ${JSON.stringify("\\".repeat(count) + '"***')} suffix`; - expect(redactArtifactMaskedValues(content, ["opaque"])).toBe(expected); - }); - - it.each(["\\u006fpaque", "op\\u0061que", "\\u006F\\u0070\\u0061\\u0071\\u0075\\u0065"])("decodes Unicode escapes at every mask position (%s)", encoded => { - expect(redactArtifactMaskedValues(`log "${encoded}"`, ["opaque"])).toBe('log "***"'); - }); - - it.each(Array.from({ length: 32 }, (_, code) => code))("recovers after unescaped control character U+%i", code => { - const prefix = '"invalid' + String.fromCharCode(code) + " "; - expect(redactArtifactMaskedValues(prefix + encodedMask, ["opaque"])).toBe(prefix + '"***"'); - }); - - it.each(["a", "c", "e", "v", "x", "0", "1", "'", "U", " ", "\t", "\r", "\n", "\u2028", "\u2029"])("recovers after illegal escaped character %j", escaped => { - const prefix = '"invalid\\' + escaped + " "; - expect(redactArtifactMaskedValues(prefix + encodedMask, ["opaque"])).toBe(prefix + '"***"'); - }); - - it.each(["", "0", "00", "000", "gggg", "000g", "00g0", "0g00", "g000", "+000", "-000", " 000"])("recovers after malformed Unicode escape %j", digits => { - const prefix = '"invalid\\u' + digits + " "; - expect(redactArtifactMaskedValues(prefix + encodedMask, ["opaque"])).toBe(prefix + '"***"'); - }); - - it.each(['"bad\t"', '"bad\u0001"', '"bad\\q"', '"bad\\u12gg"'])("conservatively redacts alternate quote pairings after terminated malformed tokens (%j)", prefix => { - const content = `${prefix} ${encodedMask}`; - const redacted = redactArtifactMaskedValues(content, ["opaque"]); - expect(JSON.parse(redacted)).toBe(`${prefix} "***"`); - expect(redacted).not.toContain("\\u006fpaque"); - expect(redactArtifactMaskedValues(content, ["absent"])).toBe(content); - }); - - it("conservatively redacts escapes before invalid controls and in unterminated strings", () => { - for (const content of ['"\\u006fpaque\t"', '"\\u006fpaque', '"\\u006fpaque\\']) { - const redacted = redactArtifactMaskedValues(content, ["opaque"]); - expect(JSON.parse(redacted)).toBe(content.replace("\\u006fpaque", "***")); - } - }); - - it("redacts encoded masks crossing quote boundaries in an alternate pairing", () => { - const content = '"bad\t" "foo\\"bar"'; - expect(JSON.parse(redactArtifactMaskedValues(content, ['foo"bar']))).toBe('"bad\t" "***"'); - }); - - it("does not recursively decode literal Unicode escapes after malformed prefixes", () => { - const content = '"bad\t" ' + JSON.stringify("\\u006fpaque"); - expect(redactArtifactMaskedValues(content, ["opaque"])).toBe(content); - }); - - it.each(["", "plain", '"', '"unterminated', '"ends\\', '"ends\\u', '"ends\\u123', '"bad\\q', '""', '"" ""', '"safe"tail', '"safe" "unterminated'])("preserves unmatched or truncated input byte-for-byte (%j)", content => { - expect(redactArtifactMaskedValues(content, ["absent"])).toBe(content); - }); - - it.each(['"unterminated opaque', '"ends opaque\\', '"bad\\q opaque', 'opaque "safe" opaque', '"safe" opaque "unterminated'])("still applies raw masks outside recoverable tokens (%j)", content => { - expect(redactArtifactMaskedValues(content, ["opaque"])).toBe(content.replaceAll("opaque", "***")); - }); - - it("handles adjacent strings, empty strings, invalid prefixes, and trailing text together", () => { - const content = `start ""${encodedMask}""${encodedMask} "bad\\q ${encodedMask} trailing`; - const redactedToken = JSON.stringify("***"); - expect(redactArtifactMaskedValues(content, ["opaque"])).toBe(`start ""${redactedToken}""${redactedToken} "bad\\q ${redactedToken} trailing`); - }); - - it("keeps literal escaped Unicode distinct from an actual encoded mask", () => { - const literal = JSON.stringify("\\u006fpaque"); - expect(redactArtifactMaskedValues(`log ${literal} ${encodedMask}`, ["opaque"])).toBe(`log ${literal} "***"`); - }); - - it("keeps numeric-looking string tokens separate from original numeric tokens", () => { - const content = ' \t{"escaped":"\\u0031e400","quoted":"9007199254740993","ids":[1e400,9007199254740993,-0,1e+09]}\r\n'; - const redacted = redactArtifactMaskedValues(content, ["1e400", "9007199254740993"]); - expect(JSON.parse(redacted)).toEqual({ escaped: "***", quoted: "***", ids: ["***", "***", 0, 1000000000] }); - expect(redacted.startsWith(" \t")).toBe(true); - expect(redacted.endsWith("\r\n")).toBe(true); - }); - - it("matches a deterministic decoded-string oracle across generated escape combinations", () => { - const alphabet = ["a", '"', "\\", "/", "\b", "\f", "\n", "\r", "\t", "\u0000", "\u001f", "\u2028", "\ud800", "\ud83d\ude80"]; - for (let index = 0; index < 256; index++) { - const left = alphabet[index % alphabet.length]; - const right = alphabet[Math.floor(index / alphabet.length) % alphabet.length]; - const value = left + right + "opaque" + right + left; - const encoded = JSON.stringify(value); - const expected = JSON.stringify(value.replaceAll("opaque", "***")); - expect(redactArtifactMaskedValues(`record ${encoded} end`, ["opaque"]), `generated record ${index}`).toBe(`record ${expected} end`); - const untouched = `record ${encoded} end`; - expect(redactArtifactMaskedValues(untouched, ["absent"]), `unchanged record ${index}`).toBe(untouched); - } - }); - - it("redacts thousands of adjacent tokens without losing delimiters or unchanged bytes", () => { - const safe = '"safe\\\\\\"value"'; - const record = `prefix ${safe}:${encodedMask}:`; - expect(redactArtifactMaskedValues(record.repeat(4096) + "tail", ["opaque"])).toBe(`prefix ${safe}:"***":`.repeat(4096) + "tail"); - }); - }); - - it.each(["json", "mixed"])("redacts multi-megabyte %s records without exhausting the regex stack", shape => { - const mask = 'synthetic-"quoted"\\mask'; - const value = "x".repeat(10 * 1024 * 1024) + mask; - const record = JSON.stringify({ text: value, number: shape === "mixed" ? 42 : 12345678 }); - const prefix = shape === "mixed" ? "tool output: " : ""; - const redacted = redactArtifactMaskedValues(prefix + record, [mask, "12345678"]); - expect(redacted.includes(mask)).toBe(false); - expect(redacted.includes(JSON.stringify(mask).slice(1, -1))).toBe(false); - const parsed = JSON.parse(redacted.slice(prefix.length)); - expect(parsed.text.length).toBe(10 * 1024 * 1024 + 3); - expect(parsed.text.endsWith("***")).toBe(true); - expect(parsed.number).toBe(shape === "mixed" ? 42 : "***"); - }); - - it("handles many escaped quotes and both odd and even backslash runs", () => { - const mask = 'synthetic-"quoted"\\mask'; - const value = '\\"'.repeat(1024 * 1024) + mask + "\\\\"; - const record = JSON.stringify({ text: value, safe: "12345678" }); - const redacted = redactArtifactMaskedValues(record, [mask]); - const parsed = JSON.parse(redacted); - expect(parsed.text.length).toBe(2 * 1024 * 1024 + 5); - expect(parsed.text.endsWith("***\\\\")).toBe(true); - expect(parsed.safe).toBe("12345678"); - }); - - it.each(["\n", "\r", "\u2028", "\u2029"])("recovers encoded masks after an invalid escaped line terminator", terminator => { - const mask = 'synthetic-"quoted"\\mask'; - const prefix = '"invalid\\' + terminator; - expect(redactArtifactMaskedValues(prefix + JSON.stringify(mask), [mask])).toBe(prefix + '"***"'); - }); - - it.each(['"bad\\q ', '"bad\\u12gg ', '"bad\\u12 ', '"bad\u0001 ', '"bad\t ', '"bad\\v '])("recovers Unicode-encoded masks after invalid JSON string prefixes (%j)", prefix => { - expect(redactArtifactMaskedValues(prefix + '"\\u006fpaque"', ["opaque"])).toBe(prefix + '"***"'); - }); - it("redacts decoded JSON strings and keys without mutating syntax or primitives", () => { const secret = 'quote"\\percent%0A'; const value = { [secret]: [secret, 0, false, null], nested: { output: "first\nsecond" } }; diff --git a/actions/setup/js/redact_secrets.test.cjs b/actions/setup/js/redact_secrets.test.cjs index 947927a2165..ad0adf21f58 100644 --- a/actions/setup/js/redact_secrets.test.cjs +++ b/actions/setup/js/redact_secrets.test.cjs @@ -127,62 +127,6 @@ describe("redact_secrets.cjs", () => { } }), describe("file handling", () => { - it("retains and sanitizes a private multi-megabyte artifact instead of failing closed on regex stack exhaustion", () => { - const { processFile } = require("./redact_secrets.cjs"); - const file = path.join(tempDir, "large-record.jsonl"); - const mask = 'synthetic-"quoted"\\mask'; - fs.writeFileSync(file, JSON.stringify({ text: "x".repeat(10 * 1024 * 1024) + mask }), { mode: 0o600 }); - expect(processFile(file, [], [mask])).toBe(1); - expect(fs.existsSync(file)).toBe(true); - const parsed = JSON.parse(fs.readFileSync(file, "utf8")); - expect(parsed.text.endsWith("***")).toBe(true); - expect(parsed.text.length).toBe(10 * 1024 * 1024 + 3); - expect(fs.statSync(file).mode & 0o777).toBe(0o600); - expect(mockCore.setFailed).not.toHaveBeenCalled(); - }); - - it.each(['"bad\\q ', '"bad\\u12gg ', '"bad\\u12 ', '"bad\u0001 ', '"bad\t ', '"bad\\v '])("sanitizes recoverable encoded masks after invalid JSON string prefixes (%j)", prefix => { - const { processFile } = require("./redact_secrets.cjs"); - const file = path.join(tempDir, "malformed.log"); - fs.writeFileSync(file, prefix + '"\\u006fpaque"', { mode: 0o600 }); - expect(processFile(file, [], ["opaque"])).toBe(1); - expect(fs.readFileSync(file, "utf8")).toBe(prefix + '"***"'); - expect(fs.statSync(file).mode & 0o777).toBe(0o600); - expect(mockCore.setFailed).not.toHaveBeenCalled(); - }); - - it.each(['"bad\t"', '"bad\\q"', '"bad\\u12gg"'])("conservatively sanitizes alternate quote pairings after malformed terminated tokens (%j)", prefix => { - const { processFile } = require("./redact_secrets.cjs"); - const file = path.join(tempDir, "ambiguous.log"); - fs.writeFileSync(file, prefix + ' "\\u006fpaque"', { mode: 0o600 }); - expect(processFile(file, [], ["opaque"])).toBe(1); - const redacted = fs.readFileSync(file, "utf8"); - expect(JSON.parse(redacted)).toBe(prefix + ' "***"'); - expect(redacted).not.toContain("\\u006fpaque"); - expect(fs.statSync(file).mode & 0o777).toBe(0o600); - expect(mockCore.setFailed).not.toHaveBeenCalled(); - }); - - it("removes the artifact if decoding a validated mixed-log token unexpectedly fails", () => { - const { processFile } = require("./redact_secrets.cjs"); - const file = path.join(tempDir, "mixed.log"); - fs.writeFileSync(file, 'output: "\\u006fpaque"'); - const parse = JSON.parse; - const parseSpy = vi.spyOn(JSON, "parse").mockImplementation(text => { - if (text === '"\\u006fpaque"') throw new Error("opaque runtime-mask"); - return parse(text); - }); - try { - expect(processFile(file, [], ["opaque"])).toBe(0); - expect(fs.existsSync(file)).toBe(false); - expect(mockCore.warning).toHaveBeenCalledWith("Secret redaction failed (stage=redact, code=unknown)"); - expect(mockCore.warning.mock.calls.flat().join("\n")).not.toMatch(/opaque|runtime-mask/); - expect(mockCore.setFailed).toHaveBeenCalled(); - } finally { - parseSpy.mockRestore(); - } - }); - it.each(["EACCES", "EPERM", "ENOSPC"])("reports only safe read diagnostics for secret-bearing %s errors", code => { const { processFile } = require("./redact_secrets.cjs"); const file = path.join(tempDir, "secret-value.log"); diff --git a/docs/src/content/docs/reference/imports.md b/docs/src/content/docs/reference/imports.md index 4f5588d0ac8..5da2f90e468 100644 --- a/docs/src/content/docs/reference/imports.md +++ b/docs/src/content/docs/reference/imports.md @@ -388,8 +388,6 @@ Steps from imports run **before** steps defined in the main workflow. Imported p Built-in redaction scans supported text files under `/tmp/gh-aw` and `${RUNNER_TEMP}/gh-aw`. Files without secret matches are left unchanged unless `secret-masking.steps` configures custom redactors, which require writable files. When redaction or custom-hook preparation requires a write, a readable non-writable file, such as a container-owned log, is atomically replaced with a runner-owned copy with owner-only read/write permissions. If the parent directory is also read-only, such as a Go module cache directory, redaction instead makes the file owner-readable and writable in place without changing directory permissions. This requires permission to change the file's mode; it does not bypass file locks or inaccessible directories. -Runtime-mask redaction handles multi-megabyte JSON strings and mixed log records without relying on recursive regular-expression matching. If malformed quotes hide an encoded mask, a conservative escape-decoding pass redacts it and represents that record as a JSON string; records without additional mask matches retain their original bytes. This does not change GitHub's step-summary size limit. - Custom hooks must tolerate absent optional files and files that disappear between discovery and reading (`ENOENT`). Earlier cleanup or redaction can remove files, including symbolic links and artifacts that could not be sanitized. Do not create empty placeholders for missing logs or outputs. Custom hooks run during the initial workspace redaction, before graders execute. The later grader-output redaction pass sanitizes newly generated results but does not rerun custom hooks, so it does not prepare unchanged files for them.