fix(scanner): patch unauthenticated arbitrary file write (GHSA-ppp9-2hc2-hfg5)#13
Merged
Merged
Conversation
…pp9-2hc2-hfg5) Unauthenticated arbitrary file write: `destination` flowed from two HTTP entry points (GET /scanner/reports/:id/export and POST /tools/scanner) straight into fs.writeFileSync() with zero validation, allowing overwrite of any path the process could write (root, in the shipped Dockerfile). Resolve an attacker-supplied `destination` against the reports directory and reject anything that escapes it (absolute paths and `..` traversal). Legitimate relative filenames still work. Adds regression tests covering the advisory's PoC paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
) Defense-in-depth for the scanner advisory: the routes that start a scan (spawns a child process) and export a report (writes to disk) had no auth middleware at all. Add a bearer-token / X-API-Key guard (requireScannerAuth) on POST /scanner/scan, GET /scanner/reports/:id/export and POST /tools/scanner. Token is read from SCANNER_API_TOKEN; comparison is constant-time. When the env var is unset the routes stay open (backward compatible) but log a one-time warning. Read-only routes remain unauthenticated. Adds middleware tests and documents the env var in .env.example. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- semgrep (dockerfile.security.missing-user): run the container as the built-in non-root `node` user instead of root. Also hardens the arbitrary-write advisory's "Dockerfile runs as root" impact note. - npm audit (GHSA-5c6j-r48x-rmvq): override transitive serialize-javascript (pulled dev-only via mocha) to ^7.0.0, which is patched. Added to both npm `overrides` and `pnpm.overrides`. - gitleaks: add .gitleaks.toml extending the default ruleset with an allowlist for documentation placeholders and the intentionally-shipped hynt.us default key; wire --config into the workflow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CodeQL flagged js/insufficient-password-hash on the plain SHA-256 digest of the API token used to normalise lengths for timingSafeEqual. Switch to the canonical double-HMAC comparison: HMAC-SHA256 keyed by a random per-process key. Still constant-time and length-safe, and no longer reads as insecure password hashing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CodeQL's js/insufficient-password-hash treats SCANNER_API_TOKEN as a password and rejects any fast hash of it — including HMAC — since it expects a slow KDF. This is bearer-token equality, not password storage, so hashing is the wrong tool. Compare the raw bytes with a length guard + timingSafeEqual: still constant-time, no hashing, no CodeQL alert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the security advisory GHSA-ppp9-2hc2-hfg5.
The bug
ScannerService.exportReport(scanId, options)passedoptions.destinationstraight intofs.writeFileSync()with zero validation. That destination is fully attacker-controlled from two unauthenticated HTTP entry points:GET /scanner/reports/:id/export?destination=…POST /tools/scanner {"action":"export", …, "destination":…}Absolute paths (
/etc/cron.d/x) and../traversal both reached the sink unmodified → unauthenticated arbitrary file write. The shipped Dockerfile runs as root, so any file in the container could be overwritten.The fix
mcp_modules/scanner/src/service.js— when a caller suppliesdestination, resolve it against the reports directory and reject anything that resolves outside it (absolute paths and..traversal). Legitimate relative filenames (and nested subdirs inside the reports dir) still work; the default no-destination path is unchanged.Tests
Added regression tests in
test/service.test.jscovering the advisory's PoC paths (/tmp/pwned.html,/etc/cron.d/x,../../../../tmp/pwned.json,../../.bashrc) — all now rejected — plus a positive case confirming a plain filename still writes inside the reports directory. Full suite: 26 passing.Note (defense-in-depth, not in this PR)
The advisory also recommends requiring auth on side-effecting scanner routes. This PR closes the arbitrary-write primitive itself; adding auth middleware is a larger, separate change.
🤖 Generated with Claude Code