fix: exempt github_api_helpers.cjs from SEC-004 (false positive) #43731
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| // @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. | ||
|
Contributor
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 💡 DetailsCurrently 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` {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.
Contributor
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 testConsider 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 @copilot please address this.
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] The 💡 Suggested JSDoc additionAdd a /**
* ...
* `@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. |
||
|
|
||
| /** | ||
| * GitHub API helper functions | ||
| * Provides common GitHub API operations with consistent error handling | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-controlledbodycontent will be permanently invisible to SEC-004 with no warning.💡 Details and suggested mitigation
check-safe-outputs-conformance.shlines 145–148 does a whole-filecontinueon match:github_api_helpers.cjsis already imported by 11 other files. OnlycreateDiscussionComment(line ~188) actually carries user-controlledbodycontent — all other exports (logGraphQLError,fetchAllRepoLabels,getFileContent,resolveTopLevelDiscussionCommentId, etc.) are body-free today. But as the module grows, any future function that does accept abodywill never be flagged.Suggested mitigations (pick one):
//@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.