diff --git a/actions/setup/js/collect_ndjson_output.test.cjs b/actions/setup/js/collect_ndjson_output.test.cjs index a774b468b83..044e7fb53d7 100644 --- a/actions/setup/js/collect_ndjson_output.test.cjs +++ b/actions/setup/js/collect_ndjson_output.test.cjs @@ -195,6 +195,20 @@ describe("collect_ndjson_output.cjs", () => { const parsedOutput = JSON.parse(outputCall[1]); (expect(parsedOutput.items).toHaveLength(2), expect(parsedOutput.items[0].type).toBe("create_issue"), expect(parsedOutput.items[1].type).toBe("add_comment"), expect(parsedOutput.errors).toHaveLength(0)); }), + it("should preserve Slack mrkdwn links in custom safe-job string inputs", async () => { + const testFile = "/tmp/gh-aw/test-ndjson-output.txt"; + const slackText = "Tracking issue: "; + const ndjsonContent = JSON.stringify({ type: "post_to_slack", text: slackText }); + fs.writeFileSync(testFile, ndjsonContent); + process.env.GH_AW_SAFE_OUTPUTS = testFile; + fs.writeFileSync("/tmp/gh-aw/safeoutputs/config.json", JSON.stringify({ post_to_slack: { inputs: { text: { type: "string", required: true } } } })); + await eval(`(async () => { ${collectScript}; await main(); })()`); + const setOutputCalls = mockCore.setOutput.mock.calls, + outputCall = setOutputCalls.find(call => "output" === call[0]); + expect(outputCall).toBeDefined(); + const parsedOutput = JSON.parse(outputCall[1]); + (expect(parsedOutput.errors).toHaveLength(0), expect(parsedOutput.items).toEqual([{ type: "post_to_slack", text: slackText }])); + }), it("should reject items with unexpected output types", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt", ndjsonContent = '{"type": "create_issue", "title": "Test Issue", "body": "Test body"}\n{"type": "unexpected-type", "data": "some data"}'; diff --git a/actions/setup/js/sanitize_content.test.cjs b/actions/setup/js/sanitize_content.test.cjs index 09fc29a0be1..317d3cd54a5 100644 --- a/actions/setup/js/sanitize_content.test.cjs +++ b/actions/setup/js/sanitize_content.test.cjs @@ -441,9 +441,9 @@ describe("sanitize_content.cjs", () => { }); it("should move title into link text for inline link with angle-bracket URL", () => { - // Note: convertXmlTags runs after neutralizeMarkdownLinkTitles and converts to (url) + // Angle-bracket HTTPS autolinks are preserved so CommonMark/Slack link syntax survives sanitization. const result = sanitizeContent('[click here]( "injected payload")'); - expect(result).toBe("[click here (injected payload)]((https://github.com/path))"); + expect(result).toBe("[click here (injected payload)]()"); }); it("should move multiple link titles into link text in the same content", () => { @@ -581,6 +581,43 @@ describe("sanitize_content.cjs", () => { expect(result).not.toContain("onerror"); }); + it("should preserve HTTPS angle-bracket autolinks", () => { + const input = "Tracking issue: "; + expect(sanitizeContent(input)).toBe(input); + }); + + it("should preserve Slack mrkdwn links on allowed HTTPS domains", () => { + const input = "Tracking issue: "; + expect(sanitizeContent(input)).toBe(input); + }); + + it("should not preserve IPv6 angle-bracket links", () => { + // IPv6 authority is not a simple [\w.-]+ hostname — treat as unknown tag + const result = sanitizeContent(""); + expect(result).not.toContain(" { + // Userinfo (user@host) must not bypass domain filtering + const result = sanitizeContent(""); + expect(result).not.toContain(" { + // Domain is blocked — URL is redacted but label text is preserved so the + // author's visible link text is not silently lost. + const result = sanitizeContent(""); + expect(result).toContain("evil.example.com/redacted"); // domain shows in redacted form + expect(result).toContain("Build failure"); // label is preserved + expect(result).not.toContain(" { + const result = sanitizeContent(""); + expect(result).toContain("evil.example.com/redacted"); // domain shows in redacted form + expect(result).not.toContain(" { const result = sanitizeContent("alert('xss')]]>"); expect(result).toBe("(![CDATA[(script)alert('xss')(/script)]])"); diff --git a/actions/setup/js/sanitize_content_core.cjs b/actions/setup/js/sanitize_content_core.cjs index 48a5ff77cb1..f3effeb5796 100644 --- a/actions/setup/js/sanitize_content_core.cjs +++ b/actions/setup/js/sanitize_content_core.cjs @@ -13,6 +13,32 @@ const SAFE_OUTPUTS_URLS_ENV = "GH_AW_SAFE_OUTPUTS_URLS"; const SAFE_OUTPUTS_URLS_ALLOWED_ONLY = "allowed-only"; const SAFE_OUTPUTS_URLS_ALLOWED_OR_CODE_REGION = "allowed-or-code-region"; +/** + * Matches angle-bracket HTTPS autolinks in CommonMark and Slack mrkdwn form: + * + * + * + * Capture groups: + * 1: hostname with optional port ([\w.-]+(?::\d+)?) + * 2: optional path (\/[^\s<>|]*) + * 3: optional Slack label ([^<>]*) + * + * The path intentionally stops at "|" so that the Slack label is captured + * separately. A bare "|" inside a URL path or query string (valid per + * RFC 3986 but uncommon — browsers percent-encode it as %7C) is therefore + * treated as the Slack label separator. This is a known limitation; use + * %7C in URLs where a literal "|" must be preserved verbatim. + * + * The strict hostname pattern ([\w.-]+) deliberately excludes IPv6 literals + * and userinfo forms so that those fall through to tag-conversion instead of + * being preserved as autolinks. + * + * Defined at module level (not inside sanitizeUrlDomains) so the RegExp object + * is allocated once and reused across calls. String.prototype.replace resets + * lastIndex automatically, so the /g flag is safe here. + */ +const angleBracketHttpsAutolinkRegex = /|]*)?(?:\|([^<>]*))?>/g; + /** * Module-level set to collect redacted URL domains across sanitization calls. * @type {string[]} @@ -318,7 +344,26 @@ function sanitizeUrlDomains(s, allowed) { } } - // First pass: handle explicit https:// URLs + // Pass 1: handle angle-bracket autolinks and Slack mrkdwn links as a unit so + // later generic URL matching does not consume the trailing ">" or "|label". + // + // Capture groups from angleBracketHttpsAutolinkRegex: + // 1: hostnameWithPort + // 2: optional path (stops before "|") + // 3: optional Slack label (everything after "|" up to ">", may contain spaces) + // + // If the domain is disallowed the label text is preserved alongside the + // redacted URL so that the author's visible link text is not silently lost. + s = s.replace(angleBracketHttpsAutolinkRegex, (match, hostnameWithPort, path = "", label = "") => { + const url = `https://${hostnameWithPort}${path}`; + const filtered = applyDomainFilter(url, hostnameWithPort); + if (filtered === url) return match; // allowed — preserve original angle-bracket form + // Disallowed — redact URL but preserve label text if present so the author's + // visible link text is not silently lost. + return label ? `${filtered}|${label}` : filtered; + }); + + // Pass 2: handle remaining explicit https:// URLs s = s.replace(httpsUrlRegex, (match, hostnameWithPort) => applyDomainFilter(match, hostnameWithPort)); // Second pass: handle protocol-relative URLs (//hostname/path). @@ -722,6 +767,32 @@ function convertXmlTags(s) { "ul", ]; + /** + * Slack/CommonMark autolinks use angle brackets but are not HTML tags. + * Preserve HTTPS forms here so downstream URL filtering can inspect them + * without convertXmlTags rewriting them to parentheses first. + * + * Supported forms: + * + * + * + * @param {string} tagContent + * @returns {boolean} + */ + function isHttpsAngleBracketAutolink(tagContent) { + // Only match simple hostname forms that sanitizeUrlDomains can parse and + // filter. The hostname must consist of word characters, dots, and hyphens + // ([\w.-]+), which excludes: + // - IPv6 literals e.g. https://[2001:db8::1]/ + // - Userinfo forms e.g. https://user@evil.example/ + // Those forms fall through to the generic tag-conversion path and are + // wrapped in parentheses instead of being preserved as autolinks. + // + // (\/[^\s<>|]*)? — optional path (mirrors angleBracketHttpsAutolinkRegex) + // (?:\|[^<>]*)? — optional Slack label, may contain spaces + return /^https:\/\/[\w.-]+(?::\d+)?(\/[^\s<>|]*)?(?:\|[^<>]*)?$/.test(tagContent); + } + // First, process CDATA sections specially - convert tags inside them and the CDATA markers s = s.replace(//g, (match, content) => { // Convert tags inside CDATA content @@ -767,6 +838,9 @@ function convertXmlTags(s) { // Convert self-closing tags: or to (tag/) or (tag /) // But preserve allowed safe tags (with dangerous attributes stripped) return s.replace(/<(\/?[A-Za-z!][^>]*?)>/g, (match, tagContent) => { + if (isHttpsAngleBracketAutolink(tagContent)) { + return match; + } // Extract tag name from the content (handle closing tags and attributes) const tagNameMatch = tagContent.match(/^\/?\s*([A-Za-z][A-Za-z0-9]*)/); if (tagNameMatch) { diff --git a/pkg/workflow/sanitize_output_fuzz_test.go b/pkg/workflow/sanitize_output_fuzz_test.go index 372e0b458bc..8ca11710ac1 100644 --- a/pkg/workflow/sanitize_output_fuzz_test.go +++ b/pkg/workflow/sanitize_output_fuzz_test.go @@ -125,7 +125,17 @@ func FuzzSanitizeOutput(f *testing.F) { f.Add("https://evil.com/