fix(enrichment): harden install-script package rendering#1545
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review — safe to merge
✅ Approved — safe to merge Review summary Nits (5)
Nits — 2 non-blocking
Review context
Contributor next steps
Signal definitions
Review detailsGenerated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative. This PR closes a prompt-injection path in the install-script enrichment path by (1) gating every registry fetch behind an allowlist regex for npm package names and a strict semver regex, so attacker-controlled keys never reach the network; (2) replacing the narrow single-slash percent-encoding with encodeURIComponent so fragment and query delimiters in any surviving name cannot alias a different registry endpoint; and (3) adding promptText in render.ts to strip control characters and escape Markdown metacharacters before the install-script block is spliced into the EXTERNAL REVIEW BRIEF. The validation, encoding, and escaping are each individually correct, and the tests exercise the injection payloads, the URL-encoding expectation for scoped packages, and the escape round-trip directly. Nits (5)
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Motivation
#or backticks.Description
NPM_PACKAGE_REandSEMVER_REand skip changes that do not validate to avoid fetching/recording attacker-controlled keys (file:review-enrichment/src/analyzers/install-scripts.ts).encodeURIComponentwhen fetching the registry packument so fragments and query delimiters cannot alias a different package (file:review-enrichment/src/analyzers/install-scripts.ts).promptTexthelper that escapes control and Markdown characters and apply it to rendered brief fields so findings are safe to include in the EXTERNAL REVIEW BRIEF (file:review-enrichment/src/render.ts).review-enrichment/test/enrichment.test.ts).Testing
npm --prefix review-enrichment run buildand it completed successfully.npm --prefix review-enrichment testand all tests passed (31 passed, 0 failed).npm run test:ci, but the run could not complete due to external/setup issues:actionlintnetwork setup retried and a repository migration-number conflict (duplicate0074) stopped the CI job (automated failure unrelated to these code changes).npm audit --audit-level=moderate, which failed due to the npm audit endpoint returning403 Forbidden, blocking the dependency-audit step (automated failure external to the patch).Codex Task