[code-scanning-fix] Fix js/http-to-file-access: correct suppression comment placement - #49932
Conversation
The CodeQL suppression comment for the validated PDF write in ensure-docs-slide-pdf.js was placed on the line above the flagged fs.writeFileSync call. CodeQL requires inline suppression comments (lgtm[rule-id] or codeql[rule-id]) to be on the same line as the flagged statement, so the previous comment had no effect and the alert remained open. This moves the annotation onto the writeFileSync line as an inline lgtm[js/http-to-file-access] comment, keeping the explanatory rationale as a block comment above it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great catch on the inline comment placement! This fix moves the CodeQL suppression comment to the same line as the flagged statement, which is where static analysis tools expect to find it. The rationale block above explains the multiple validation layers (URL, content-type, size limit, PDF signature) that make this write safe, so future readers will understand why the alert is suppressed. Since this is a comment-only fix with no logic changes, the existing validations remain intact. The change looks ready to merge. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 new lines detected). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Updates the CodeQL suppression for a validated PDF write.
Changes:
- Moves suppression inline with
fs.writeFileSync. - Expands the security rationale.
Show a summary per file
| File | Description |
|---|---|
scripts/ensure-docs-slide-pdf.js |
Updates the suppression annotation and rationale. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| // readPdfBytes() only ever returns bytes from a fixed, hardcoded GitHub media | ||
| // URL (constructed from validated repo/ref values) after checking the response | ||
| // content-type, enforcing a size limit, and verifying the PDF file signature — | ||
| // or a locally generated placeholder PDF. Neither path writes arbitrary/unvalidated | ||
| // network data to disk. |
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: Approve (non-blocking)
Comment-only change: relocates the lgtm[js/http-to-file-access] suppression annotation onto the same line as fs.writeFileSync, with no functional/logic changes. Verified the surrounding readPdfBytes() guards (fixed host, SHA/repo regex validation, content-type allow-list, size cap, PDF magic-byte check) remain intact and justify the suppression.
Review notes
- Diff is 6 additions / 2 deletions, entirely comments — no risk of regression.
- No test changes needed since behavior is unchanged.
- Nothing to flag for correctness, security, or maintainability.
🔎 Code quality review by PR Code Quality Reviewer · auto · 10.9 AIC · ⌖ 4.82 AIC · ⊞ 7.9K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — root cause correctly identified and fixed; one minor style suggestion on the suppression comment syntax.
📋 Summary
What's Good
- ✅ Root cause correctly diagnosed: suppression comment was on the wrong line — the fix moves it inline, which is the only way CodeQL honors it.
- ✅ The expanded block comment above the call clearly documents the defense-in-depth validations, making the suppression easy for future reviewers to verify.
- ✅ No behavioral change; existing runtime logic is untouched.
One Minor Point
The fix switches from codeql[js/http-to-file-access] (original, newer format) to lgtm[js/http-to-file-access] (older LGTM format). Both suppress the alert today, but codeql[...] is the documented preferred syntax going forward — see inline comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 19.2 AIC · ⌖ 8.4 AIC · ⊞ 7.1K
Comment /matt to run again
| fs.writeFileSync(OUTPUT_PATH, pdfBytes); | ||
| // readPdfBytes() only ever returns bytes from a fixed, hardcoded GitHub media | ||
| // URL (constructed from validated repo/ref values) after checking the response | ||
| // content-type, enforcing a size limit, and verifying the PDF file signature — |
There was a problem hiding this comment.
[/diagnosing-bugs] The fix uses // lgtm[js/http-to-file-access] (older LGTM syntax) rather than // codeql[js/http-to-file-access] (newer, preferred format). Both currently suppress the alert, but the codeql[...] form is the documented going-forward standard and what the original comment already used.
💡 Suggested change
fs.writeFileSync(OUTPUT_PATH, pdfBytes); // codeql[js/http-to-file-access]This keeps the style consistent with the repo's existing suppressions and avoids a future maintenance burden if the lgtm alias is deprecated.
@copilot please address this.
There was a problem hiding this comment.
Review: Suppression comment placement fix
The core fix is correct — moving the suppression annotation to the same line as fs.writeFileSync is required for CodeQL to honor it. The expanded rationale comment clearly documents the validations that make this safe.
One non-blocking suggestion (see inline comment): prefer // codeql[js/http-to-file-access] (or both codeql[...] and lgtm[...]) over lgtm[...] alone, as codeql[...] is the modern format following LGTM.com retirement.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.7 AIC · ⌖ 9.27 AIC · ⊞ 5.4K
| fs.writeFileSync(OUTPUT_PATH, pdfBytes); | ||
| // readPdfBytes() only ever returns bytes from a fixed, hardcoded GitHub media | ||
| // URL (constructed from validated repo/ref values) after checking the response | ||
| // content-type, enforcing a size limit, and verifying the PDF file signature — |
There was a problem hiding this comment.
The suppression comment was changed from // codeql[js/http-to-file-access] to // lgtm[js/http-to-file-access]. While lgtm[...] is still honored by CodeQL on GitHub, codeql[...] is the current recommended format (introduced when LGTM.com was retired). Consider using // codeql[js/http-to-file-access] inline instead, or use both for maximum compatibility:
fs.writeFileSync(OUTPUT_PATH, pdfBytes); // codeql[js/http-to-file-access] lgtm[js/http-to-file-access]This is a non-blocking suggestion — lgtm[...] should still suppress the alert.
@copilot please address this.
Security Fix: CodeQL suppression comment placement for validated file write
Alert Number: #636
Severity: Medium (warning)
Rule: js/http-to-file-access
CWE: CWE-434, CWE-912
Vulnerability Description
CodeQL's
js/http-to-file-accessquery flagsfs.writeFileSync(OUTPUT_PATH, pdfBytes)inscripts/ensure-docs-slide-pdf.jsbecause the written bytes originate from an HTTP response. This is a valid pattern to double-check (writing network data to disk unconditionally can allow arbitrary file upload / backdoors), but in this script the write path is already fully guarded:media.githubusercontent.comhost; the only variable parts (git ref andowner/repo) are validated against strict regexes (^[0-9a-f]{40}$for the SHA,^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$for the repo path) before being interpolated.Content-Typeheader must start withapplication/pdforapplication/octet-stream, or the function throws.Content-Length/actual body size cap of 50 MB is enforced.%PDF-signature viaisPdf()before being returned.A previous fix attempt added an inline suppression comment, but placed it on the line above the flagged
fs.writeFileSynccall. CodeQL only honorslgtm[rule-id]/codeql[rule-id]suppression comments when they appear on the same line as the flagged statement, so the alert stayed open despite the comment's intent.Location
scripts/ensure-docs-slide-pdf.jsfs.writeFileSync(OUTPUT_PATH, pdfBytes)call inmain())Fix Applied
Moved the suppression annotation to be inline (
// lgtm[js/http-to-file-access]) on the same line asfs.writeFileSync(...), and kept/expanded the rationale as a block comment directly above it explaining exactly which validations make this call safe.Changes Made:
// codeql[js/http-to-file-access]: ...comment (placed one line above the call) with a multi-line rationale block comment followed by an inline// lgtm[js/http-to-file-access]comment on thefs.writeFileSyncline itself.readPdfBytes()are unchanged.Security Best Practices
Testing Considerations
node --check scripts/ensure-docs-slide-pdf.jsto confirm the file is still syntactically valid.Automated by: Code Scanning Fixer Workflow
Run ID: 30795693285