fix: exempt github_api_helpers.cjs from SEC-004 (false positive) - #43731
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43731 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR aims to resolve a SEC-004 safe-outputs conformance false positive by documenting an exemption for a low-level GraphQL helper (github_api_helpers.cjs) where body values are expected to be sanitized by higher-level callers.
Changes:
- Added a
@safe-outputs-exempt SEC-004annotation toactions/setup/js/github_api_helpers.cjsto bypass the conformance heuristic forbodyusage in this helper. - Regenerated numerous workflow
.lock.ymlfiles, primarily altering YAML block chomping / trailing-newline handling in embedded scripts.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/github_api_helpers.cjs | Adds SEC-004 exemption annotation for the helper file to avoid false-positive conformance failures. |
| .github/workflows/workflow-skill-extractor.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/workflow-health-manager.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/weekly-blog-post-writer.lock.yml | Lockfile regeneration / YAML block chomping change (` |
| .github/workflows/unbloat-docs.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/uk-ai-operational-resilience.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/test-quality-sentinel.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/step-name-alignment.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/static-analysis-report.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/stale-repo-identifier.lock.yml | Lockfile regeneration / YAML block chomping change (` |
| .github/workflows/spec-extractor.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/schema-consistency-checker.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/repository-quality-improver.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/release.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/prompt-clustering-analysis.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/pr-description-caveman.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/objective-impact-report.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/mergefest.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/mattpocock-skills-reviewer.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/linter-miner.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/lint-monster.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/issue-arborist.lock.yml | Lockfile regeneration / YAML block chomping change (` |
| .github/workflows/impeccable-skills-reviewer.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/gpclean.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/glossary-maintainer.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/eslint-monster.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/docs-noob-tester.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/designer-drift-audit.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/design-decision-gate.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/dependabot-burner.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/delight.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/dataflow-pr-discussion-dataset.lock.yml | Lockfile regeneration / YAML block chomping change (` |
| .github/workflows/daily-spdd-spec-planner.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/daily-security-observability.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/daily-multi-device-docs-tester.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/daily-community-attribution.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/daily-ambient-context-optimizer.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/copilot-pr-nlp-analysis.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/copilot-centralization-drilldown.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/contribution-check.lock.yml | Lockfile regeneration / YAML block chomping change (` |
| .github/workflows/code-simplifier.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/cli-consistency-checker.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/ci-doctor.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/aw-failure-investigator.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/auto-triage-issues.lock.yml | Lockfile regeneration / YAML block chomping change (` |
| .github/workflows/architecture-guardian.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/agentic-token-trend-audit.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/agentic-token-optimizer.lock.yml | Lockfile regeneration / embedded script formatting change. |
| .github/workflows/agentic-token-audit.lock.yml | Lockfile regeneration / embedded script formatting change. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 49/49 changed files
- Comments generated: 1
- Review effort level: Low
| name: Build Inventory | ||
| run: "set -euo pipefail\nmkdir -p /tmp/gh-aw/agent\n# Run compilation validation and capture output\ngh aw compile --validate > /tmp/gh-aw/agent/compile-validate.txt 2>&1 || true\n# List executable workflow files (exclude shared/ subdirectory)\nls .github/workflows/*.md 2>/dev/null > /tmp/gh-aw/agent/workflow-list.txt || true\necho \"Inventory complete: $(wc -l < /tmp/gh-aw/agent/workflow-list.txt | tr -d ' ') workflows found\"\n" | ||
| run: "set -euo pipefail\nmkdir -p /tmp/gh-aw/agent\n# Run compilation validation and capture output\ngh aw compile --validate > /tmp/gh-aw/agent/compile-validate.txt 2>&1 || true\n# List executable workflow files (exclude shared/ subdirectory)\nls .github/workflows/*.md 2>/dev/null > /tmp/gh-aw/agent/workflow-list.txt || true\necho \"Inventory complete: $(wc -l < /tmp/gh-aw/agent/workflow-list.txt | tr -d ' ') workflows found\"" |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The underlying fix is correct — the current callers do sanitize, SEC-004 is genuinely a false positive, and the conformance checker passes. Two non-blocking concerns about the exemption's forward-looking robustness are noted inline.
### Review themes
The exemption is correct today but operates at file scope, not function scope. The checker does a whole-file continue when it finds @safe-outputs-exempt SEC-004, which means any future body-taking function added to github_api_helpers.cjs will be silently excluded from SEC-004 checks. Given the module already has 11+ importers and is growing, this scope needs to be documented explicitly in the annotation or constrained in the checker.
Caller contract is asserted but not enforced. The annotation names two callers as the sanitization guarantee, but createDiscussionComment is a public export and the contract has no machine-readable enforcement (no JSDoc precondition, no type brand). A future caller bypassing sanitization would not be caught by SEC-004 (because the file is exempted) and would not be caught by types. Adding a JSDoc @param note adjacent to the function is a low-effort improvement.
The core security claim holds. Both named callers call sanitizeContent() before reaching the helper. No double-sanitization hazard exists. The lock file bumps are correct.
🔎 Code quality review by PR Code Quality Reviewer · 81.4 AIC · ⌖ 6.91 AIC · ⊞ 5.4K
Comment /review to run again
| // @ts-check | ||
| /// <reference types="@actions/github-script" /> | ||
|
|
||
| // @safe-outputs-exempt SEC-004: low-level GraphQL helper; body is sanitized by callers (add_comment.cjs, add_reaction_and_edit_comment.cjs) before it reaches this layer. |
There was a problem hiding this comment.
File-level exemption silently blanket-covers all future functions in this module. The checker skips the entire file when it finds @safe-outputs-exempt SEC-004, so any future function added here that accepts user-controlled body content will be permanently invisible to SEC-004 with no warning.
💡 Details and suggested mitigation
check-safe-outputs-conformance.sh lines 145–148 does a whole-file continue on match:
if grep -q "`@safe-outputs-exempt`[[:space:]]\+SEC-004" "$handler"; then
continue # skips the whole file
figithub_api_helpers.cjs is already imported by 11 other files. Only createDiscussionComment (line ~188) actually carries user-controlled body content — all other exports (logGraphQLError, fetchAllRepoLabels, getFileContent, resolveTopLevelDiscussionCommentId, etc.) are body-free today. But as the module grows, any future function that does accept a body will never be flagged.
Suggested mitigations (pick one):
- Update the annotation to explicitly state the whole-file scope and the invariant:
//@safe-outputs-exemptSEC-004: WHOLE-FILE — only createDiscussionComment carries user-controlled body; all other exports are body-free. Update if new body-taking functions are added. - Or update the checker to support a narrower function-adjacent annotation so the exemption scope is bounded.
| // @ts-check | ||
| /// <reference types="@actions/github-script" /> | ||
|
|
||
| // @safe-outputs-exempt SEC-004: low-level GraphQL helper; body is sanitized by callers (add_comment.cjs, add_reaction_and_edit_comment.cjs) before it reaches this layer. |
There was a problem hiding this comment.
Annotation asserts caller sanitization but nothing enforces that contract; stale annotation risk is real. The annotation names two callers as the sanitization guarantee, but there are currently 10+ other importers of this module and createDiscussionComment is a public export — any new caller can bypass sanitization silently.
💡 Details
Currently createDiscussionComment is called only from add_comment.cjs (sanitizes at line 646) and add_reaction_and_edit_comment.cjs (sanitizes at line 245). Those two are correct.
But the function is exported and the module is already imported by 11 other files. Nothing in the code, types, or checker prevents a third caller from passing unsanitized content. As the exemption suppresses the whole file from SEC-004 (see sibling comment), the usual safety net for new callers is also gone.
A low-cost mitigation: add a JSDoc @param note or a branded type so the expectation is machine-visible:
/**
* `@param` {string} body - Comment body. MUST be pre-sanitized by the caller
* using sanitizeContent() before passing here. Do not add sanitization
* inside this function — double-sanitization strips allowed mentions.
*/
async function createDiscussionComment(github, discussionId, body, replyToId) {This doesn't prevent misuse but makes the contract explicit in IDEs and makes it visually obvious when reviewing future callers.
There was a problem hiding this comment.
Review: SEC-004 exemption for github_api_helpers.cjs
Change adds @safe-outputs-exempt SEC-004 to github_api_helpers.cjs with 48 lock-file trailing-newline normalizations.
Correctness: annotation recognized by conformance checker (line 146). Both callers sanitize before passing body to createDiscussionComment. No other callers exist. Lock changes are cosmetic recompile artifacts. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 91.3 AIC · ⌖ 6.27 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix is correct but has two small gaps that could create future maintenance risk.
📋 Key Themes & Highlights
Key Themes
- Missing regression test: No test verifies that
check-safe-outputs-conformance.shcorrectly skips the file after the annotation. A regex change in the checker would silently re-introduce the false positive (or fail to catch a real violation). - Undocumented sanitization contract:
createDiscussionComment's JSDoc doesn't state thatbodymust be pre-sanitized by callers. Future contributors adding a third caller won't see this invariant without reading the conformance checker script.
Positive Highlights
- ✅ Root cause correctly identified: the checker triggers on
"body"references without a co-located sanitize call; helper is validly exempt because sanitization is enforced one layer up. - ✅ Exemption justification is clear and specific (names the two caller files and the reason for avoiding double-sanitization).
- ✅ Follows the established pattern — other files in
actions/setup/js/use the same@safe-outputs-exemptconvention correctly. - ✅ Lock files are correctly auto-generated; no manual edits needed.
Summary
The exemption is accurate and the reasoning is sound. The two comments above are not blocking — they're incremental improvements to make the invariant explicit and to add a safety net. Happy to approve once those are addressed or explicitly deferred.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 76.2 AIC · ⌖ 5.39 AIC · ⊞ 6.7K
Comment /matt to run again
| // @ts-check | ||
| /// <reference types="@actions/github-script" /> | ||
|
|
||
| // @safe-outputs-exempt SEC-004: low-level GraphQL helper; body is sanitized by callers (add_comment.cjs, add_reaction_and_edit_comment.cjs) before it reaches this layer. |
There was a problem hiding this comment.
[/diagnosing-bugs] No regression test guards the exemption: if the checker's exemption regex ever changes (e.g. adding a colon variant), this annotation silently stops matching and the false positive returns — or a real violation could be silently skipped.
💡 Suggested regression test
Consider adding a small shell test that verifies the checker exits clean when run against a stub file containing the exemption annotation and a body field:
# test: sec-004 exemption is honoured
stub=$(mktemp --suffix=.cjs)
printf '%s\n' '// `@safe-outputs-exempt` SEC-004: test' 'const x = { "body": foo };' > "$stub"
grep -q '`@safe-outputs-exempt`[[:space:]]\+SEC-004' "$stub" && echo PASS || echo FAIL
rm "$stub"Without this, a future refactor of the exemption pattern in check-safe-outputs-conformance.sh could re-introduce the false positive invisibly.
@copilot please address this.
| // @ts-check | ||
| /// <reference types="@actions/github-script" /> | ||
|
|
||
| // @safe-outputs-exempt SEC-004: low-level GraphQL helper; body is sanitized by callers (add_comment.cjs, add_reaction_and_edit_comment.cjs) before it reaches this layer. |
There was a problem hiding this comment.
[/diagnosing-bugs] The createDiscussionComment JSDoc does not document the sanitization contract — that callers must pre-sanitize body before passing it in. A future caller could omit sanitization and silently introduce a real SEC-004 violation without the checker catching it (since the file is now exempt).
💡 Suggested JSDoc addition
Add a @remarks or a note to the @param body line:
/**
* ...
* `@param` {string} body - Comment body. MUST be pre-sanitized by the caller
* (e.g. via sanitizeContent()) before being passed here. This file is
* exempt from SEC-004 checks on the assumption that all callers sanitize.
* ...
*/This makes the invariant machine-readable and visible to future contributors without requiring them to trace back through the conformance checker script.
@copilot please address this.
PR Triage
Adds
|
|
🎉 This pull request is included in a new release. Release: |
SEC-004 conformance check was flagging
github_api_helpers.cjsfor containingbodyfields without calling a sanitization function. This is a false positive — the file is a low-level GraphQL API helper whose callers (add_comment.cjs,add_reaction_and_edit_comment.cjs) already invokesanitizeContent()before passingbodyinto it.Change
Added the
@safe-outputs-exemptannotation near the top ofgithub_api_helpers.cjs, which the checker already recognizes (lines 145–148 ofcheck-safe-outputs-conformance.sh):// @safe-outputs-exempt SEC-004: low-level GraphQL helper; body is sanitized by callers (add_comment.cjs, add_reaction_and_edit_comment.cjs) before it reaches this layer.No changes to the conformance checker were needed. Adding sanitization inside the helper itself was intentionally avoided — double-sanitization would risk stripping workflow markers and allowed mentions preserved by the caller layer.