Add exact-pinned-intermediate regression fixture (Discussion #528 #2)#661
Add exact-pinned-intermediate regression fixture (Discussion #528 #2)#661Ayush7614 wants to merge 3 commits into
Conversation
sonukapoor
left a comment
There was a problem hiding this comment.
The fixture itself is solid - this is exactly the edge case #528 item #2 describes, and the lockfile structure correctly isolates the nested vulnerable qs path. A few things to fix:
Removed test block - the multiple-versions-same-pkg test block was dropped with no explanation. That fixture is assigned to @coder-Yash886 in Discussion #528 and has a sister PR (#633) in flight - removing its test coverage here needs a clear reason. Could you either restore it or explain why it was removed?
readme.md regressions - three entries disappeared: payload, presenton, and the detailed twenty entry. These look like a rebase accident rather than intentional removals. Please restore them.
Hardcoded reason string - the recommendedParentUpgrade.reason in the test is manually authored, so it won't catch regressions if the scanner's actual output changes. Consider either deriving it from a real scanPackages() call or dropping the assertion on that field.
|
Thanks for the review, @sonukapoor — addressed in
Ready for another look. |
sonukapoor
left a comment
There was a problem hiding this comment.
The fixture structure is clean and the requirePackage version parameter improvement in the second commit is a nice touch.
Two things need to be addressed:
There's a TypeScript compile error: recommendedParentUpgrade in the test input is missing the required reason field. Every other test that uses this type includes it (e.g. the transitive-only test has reason: "Upgrade lint-staged..."). The field doesn't need to be meaningful for the assertion - just add reason: "Parent upgrade for exact-pinned qs@6.14.2 via express." or similar.
The test doesn't actually exercise the edge case it's designed to catch. The scenario where exact pinning produces a different outcome than a semver range requires a fix version that falls within ~6.14.0 but outside =6.14.2 - for example 6.14.3. With firstFixedVersion: "6.15.2", both the range fixture and the exact-pinned fixture follow the same code path so the test can't distinguish between them. Please update the fixture and fix version so the test actually catches the regression it claims to cover.
Branch is behind main - please rebase with git fetch origin && git rebase origin/main && git push --force-with-lease.
|
Thanks again, @sonukapoor — both points addressed in
Rebasing onto latest |
…OWASP#2) Intermediate parent exact-pins qs@6.14.2 — expects npm install express@4.22.2, not npm update qs.
…ersion - Add required reason field to recommendedParentUpgrade (TS compile error) - Use firstFixedVersion 6.14.3 (within ~6.14.0 but outside pinned 6.14.2) so the test distinguishes exact-pin parent-upgrade from within-range refresh
|
Rebased onto latest |
|
This branch has fallen behind main. Would you mind rebasing? ( |
sonukapoor
left a comment
There was a problem hiding this comment.
Thanks for putting this together. The lockfile structure looks correct - body-parser@1.20.4 has "qs": "6.14.2" as an exact pin in the lockfile (not ~6.14.2 or ^6.14.2), which is exactly the scenario you're trying to capture. The readme update is clean and consistent. A couple of things to address before this lands:
The test doesn't actually exercise the exact-pin detection. The regression this is meant to guard against (Discussion #528) is that the scanner incorrectly routes to npm update qs when body-parser exact-pins the vulnerable version. But the test hardcodes recommendedParentUpgrade in the overrides, then calls buildSuggestedFixCommandPlan directly. That means the fix-command builder's routing is tested (given a pre-classified finding, produce npm install express@4.22.2), but the scanner's ability to detect the exact pin and produce recommendedParentUpgrade is never called. If the analysis layer produced recommendedNpmTransitiveRemediation.kind = "update-parent-within-range" for this lockfile, this test would still pass. The existing transitive-only test already covers the routing behavior being tested here.
To actually guard the regression, the test needs to run the analysis layer against this fixture - something like calling resolveNpmTransitiveRemediation directly on the loaded scanInput and asserting it produces recommendedParentUpgrade (not update-parent-within-range). Or, if a scanner-level test is out of scope for this PR, the description and PR title should say so - "adds a lockfile fixture for manual testing and documents the exact-pin scenario" rather than "regression fixture."
The package.json is identical to deep-chain-no-fix. Both fixtures have the same overrides block (body-parser["."] = "1.20.4", body-parser.qs = "6.14.2", express.qs = "6.15.2") and the same direct dependency (express: 4.22.1). The difference is in the lockfile only: deep-chain-no-fix has "qs": "~6.14.0" in body-parser's dependency field, while this fixture has "qs": "6.14.2" (exact pin). That's a genuinely different scenario, so both fixtures make sense - but it would help to call out the distinction explicitly in the description field. Something like: "...unlike deep-chain-no-fix which uses a tilde range, this fixture uses a bare version string, meaning no update within the installed parent version can resolve it."
Minor: the confidence: "best-effort" in the test overrides doesn't match what the real scanner produces for this routing path. When the scanner routes through upgrade-parent-to-version, it sets confidence: "exact-direct-child" (scanner.ts:450). buildSuggestedFixCommandPlan doesn't check confidence, so this doesn't break anything, but if the test is meant to represent a realistic finding shape, it should match.
Adds
examples/exact-pinned-intermediate/for Discussion #528 fixture ** Fixture 2**.Summary
body-parser) exact-pinsqs@6.14.2in the lockfile — not a semver range like~6.14.0npm update qs(within-range lockfile refresh)expressChain
express@4.22.1→body-parser@1.20.4→qs@6.14.2"qs": "6.14.2"(exact) onbody-parser— only that version satisfies the pindeep-chain-no-fix)Verified scan output
What changed
examples/exact-pinned-intermediate/package.json+package-lock.jsonexamples/readme.md— fixture table + scan commandtests/fixture-scan.test.ts— asserts parent upgrade, notnpm update qsContrast with related fixtures
wrong-parent/pnpm-within-rangenpm update/pnpm updateon vulnerable pkgdeep-chain-no-fixexact-pinned-intermediateTest plan
node dist/index.js examples/exact-pinned-intermediate --verbose→npm install express@4.22.2, notnpm update qsexamples/readme.mdnpm test)Closes Discussion #528 item fixture 2 (
exact-pinned-intermediate).cc: @sonukapoor