SEC-004: Fix content sanitization gaps in exchange_otlp_workload_identity.cjs and report_failed_jobs.cjs - #50806
Conversation
…led_jobs content Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage SummaryCategory: bug (security) | Risk: high | Priority score: 78/100 Score breakdown: impact 40 + urgency 25 + quality 13 Recommended action: Fixes a SEC-004 sanitization gap allowing unsanitized job names/URLs into issue bodies (potential injection vector). Touches security-sensitive path ( Note: related to OpenTelemetry (otlp) per customer triage rules — labeling
|
There was a problem hiding this comment.
Pull request overview
Fixes SEC-004 safe-output conformance gaps for failed-job reporting and OAuth transport.
Changes:
- Documents the OAuth request-body exemption.
- Sanitizes failed-job names and validates/sanitizes URLs.
- Adds focused formatting and injection tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/exchange_otlp_workload_identity.cjs |
Adds the SEC-004 transport exemption. |
actions/setup/js/report_failed_jobs.cjs |
Sanitizes dynamic failed-job content. |
actions/setup/js/report_failed_jobs.test.cjs |
Tests sanitization and URL handling. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report
📊 Metrics (7 tests)
Verdict
References: §31100183012
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — leaving as COMMENT with a few small hardening suggestions.
📋 Key Themes & Highlights
Key Themes
- Negative-only assertions: Two tests check that bad content is absent without verifying what the sanitized output actually is — a broken
sanitizeContentthat returns empty string would still pass. - HTML comment vector: The injection test targets the
@mentionbut not the<!--delimiter itself, which is the real attack surface. - Null safety:
job.nameis not guarded againstnull/undefinedbefore being passed tosanitizeContent.
Positive Highlights
- ✅ Correct scheme-check (
startsWith('https://')) before acceptinghtml_url— simple and effective. - ✅ Good test breadth: covers `(redacted) URI, HTML injection, multi-job, and empty-array cases.
- ✅ Exempt annotation in
exchange_otlp_workload_identity.cjsis well-commented and mirrors the established pattern.
|
|
||
| it("sanitizes markdown/HTML special characters in job name", () => { | ||
| const result = formatFailedJobsList([{ name: "<script>alert(1)</script>", html_url: null }]); | ||
| expect(result).not.toContain("<script>"); |
There was a problem hiding this comment.
[/diagnosing-bugs] Test asserts absence of the raw tag but not what the sanitized output is — if sanitizeContent silently returns an empty string or garbles legitimate names, this test still passes.
💡 Suggested fix
Add a positive assertion alongside the negative ones:
it('sanitizes markdown/HTML special characters in job name', () => {
const result = formatFailedJobsList([{ name: '<script>alert(1)</script>', html_url: null }]);
expect(result).not.toContain('<script>');
expect(result).not.toContain('</script>');
// Verify something meaningful remains after sanitization
expect(result).toContain('alert(1)');
});Without the positive assertion, a regression returning - \`` would still pass the test.
@copilot please address this.
| expect(result).not.toContain("</script>"); | ||
| }); | ||
|
|
||
| it("sanitizes job name containing HTML comment injection", () => { |
There was a problem hiding this comment.
[/tdd] The HTML-comment injection test checks @exploituser is absent, but this conflates mention-neutralization with injection prevention — the real concern is that <!-- itself is stripped. A more precise assertion targets the payload structure.
💡 Suggested fix
it('sanitizes HTML comment injection in job name', () => {
const result = formatFailedJobsList([{ name: '<!-- `@exploituser` injected payload -->', html_url: null }]);
expect(result).not.toContain('<!--');
expect(result).not.toContain('`@exploituser`');
});Testing only the mention leaves <!-- passing through if mention-neutralization is disabled.
@copilot please address this.
| @@ -48,10 +49,12 @@ function isActionsReadPermissionError(error) { | |||
| function formatFailedJobsList(jobs) { | |||
| return jobs | |||
| .map(job => { | |||
There was a problem hiding this comment.
[/diagnosing-bugs] sanitizeContent(job.name) is called without guarding against null/undefined — the GitHub Jobs API marks name as required, but defensive handling is cheap and prevents a hard crash if the contract is ever violated.
💡 Suggested fix
const safeName = sanitizeContent(job.name ?? '');Alternatively, add a test for { name: null, html_url: null } to document the current behaviour.
@copilot please address this.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Request changes — the sanitization fix has a real markdown-breakout gap via unescaped backticks
💡 Themes
- The
exchange_otlp_workload_identity.cjsexemption annotation is consistent with existing@safe-outputs-exempt SEC-004usage across the codebase (e.g.artifact_client.cjs,mcp_cli_bridge.cjs) — legitimate, no issue. report_failed_jobs.cjs's newsanitizeContentusage correctly filters<script>, HTML-comment@mentioninjection, and non-httpsURL schemes, butsanitizeContent/sanitizeContentCoredoes not strip or escape a literal backtick character. Since the sanitized name is interpolated directly inside backtick/markdown-link syntax, ajob.namecontaining a backtick can break out of the intended inline-code span and inject a forged markdown link (verified via direct reproduction) — this defeats the intent of the fix for the exact injection class SEC-004 targets.- The new test suite is a solid start (covers script tags, comment injection,
(redacted) rejection) but has no case for a backtick injob.name`, so it would not catch the above.
| const safeName = sanitizeContent(job.name); | ||
| if (job.html_url && job.html_url.startsWith("https://")) { | ||
| const safeUrl = sanitizeContent(job.html_url); | ||
| return `- [\`${safeName}\`](${safeUrl})`; |
There was a problem hiding this comment.
Critical: sanitizeContent does not escape raw backtick characters, so a job name containing a backtick can break out of the inline-code span and inject arbitrary markdown (including a forged link) into the issue body — defeating the purpose of this fix.
💡 Details
sanitizeContent/sanitizeContentCore neutralizes @mentions, #refs, XML comments, and non-allowed URL schemes/domains, but never strips or escapes a literal character in the input. Since the code interpolatessafeName/safeUrldirectly inside the backtick+link template, ajob.name` such as:
job`](https://github.com/evil/repo)[pwn
escapes the intended backtick span and renders as a working markdown link with attacker-controlled anchor text, as long as the injected URL host is on the allowed-domains list (e.g. github.com) or the link segment is simply text with no URL at all. Confirmed by direct reproduction against the current sanitizeContent implementation:
formatFailedJobsList([{name: "job`](https://github.com/evil/repo)[pwn", html_url: null}])
// => "- `job`](https://github.com/evil/repo)[pwn`"job.name in GitHub Actions job payloads can be influenced by matrix values or workflow-defined job/step names, so this is a plausible untrusted-input path feeding directly into a GitHub issue body — the exact class of injection this PR is meant to close.
Suggested fix: escape backticks in safeName/safeUrl before interpolation (e.g. replace a backtick with an escaped backtick or a safe placeholder), or use a helper that guarantees the output cannot contain an unescaped backtick, in addition to the existing sanitizeContent call.
| it("returns empty string for empty jobs array", () => { | ||
| expect(formatFailedJobsList([])).toBe(""); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The new test suite doesn't include a case for a backtick in job.name, so it wouldn't have caught the markdown-breakout injection this PR is meant to prevent.
💡 Details
Tests cover <script> tags, HTML comment @mention injection, and (redacted) URL rejection, but none exercise a job name containing a literal backtick (e.g. `` job](https://github.com/evil/repo)[pwn ``). That's precisely the character sanitizeContent fails to neutralize (see the companion comment on `report_failed_jobs.cjs`), so this test suite gives false confidence that the injection surface is fully closed.
Suggested addition:
it("escapes/handles backticks in job name to prevent markdown breakout", () => {
const result = formatFailedJobsList([{ name: "job`](https://github.com/evil/repo)[pwn", html_url: null }]);
// Should not produce a working forged markdown link
expect(result).not.toMatch(/\]\(https:\/\/github\.com\/evil\/repo\)/);
});
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: SEC-004 Content Sanitization Fixes
The changes are correct and well-scoped. Both security issues are addressed appropriately.
exchange_otlp_workload_identity.cjs
The @safe-outputs-exempt SEC-004 annotation is the right fix — body here is an OAuth/WIF HTTP POST payload, not a GitHub content field. Mirrors the established pattern in artifact_client.cjs.
report_failed_jobs.cjs
sanitizeContent(job.name)neutralizes HTML tags, XML comments,@mentions, and other injection vectors.startsWith("https://")guard correctly rejects(redacted)(redacted) and other unsafe schemes before embedding in Markdown.- Applying
sanitizeContentto the URL is safe:buildAllowedDomains()includesGITHUB_SERVER_URL, sohtml_urlfrom the GitHub API always passes the domain allowlist.
Test coverage
All key injection scenarios are covered: <script> tags, HTML comment @mention injection, `(redacted) URI, multi-job formatting, and empty input. Assertions are valid given the sanitizer behavior.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 31.3 AIC · ⊞ 5.4K
|
🎉 This pull request is included in a new release. Release: |
Two handlers tripped the SEC-004 Safe Outputs conformance check: one legitimately uses
bodyfor HTTP transport (exempt), the other interpolates API-sourced job names/URLs directly into a GitHub issue body without sanitization.Changes
exchange_otlp_workload_identity.cjs— Add@safe-outputs-exempt SEC-004annotation;bodyhere is an OAuth/WIF HTTP request payload, not a GitHub content field (mirrors the pattern inartifact_client.cjs).report_failed_jobs.cjs— ImportsanitizeContentand apply it informatFailedJobsList():job.namepassed throughsanitizeContent()before backtick interpolationjob.html_urlvalidated againsthttps://scheme (dropsjavascript:and other unsafe schemes) then also sanitizedreport_failed_jobs.test.cjs(new) — Unit tests forformatFailedJobsListcovering plain names, URLs,<script>injection in name, HTML comment@mentioninjection,javascript:URI rejection, multi-job formatting, and empty input.