Skip to content

Add exact-pinned-intermediate regression fixture (Discussion #528 #2)#661

Open
Ayush7614 wants to merge 2 commits into
OWASP:mainfrom
Ayush7614:ayushfixes
Open

Add exact-pinned-intermediate regression fixture (Discussion #528 #2)#661
Ayush7614 wants to merge 2 commits into
OWASP:mainfrom
Ayush7614:ayushfixes

Conversation

@Ayush7614

@Ayush7614 Ayush7614 commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Adds examples/exact-pinned-intermediate/ for Discussion #528 fixture ** Fixture 2**.

Summary

  • Intermediate parent (body-parser) exact-pins qs@6.14.2 in the lockfile — not a semver range like ~6.14.0
  • CVE Lite must not suggest npm update qs (within-range lockfile refresh)
  • Correct fix: parent upgrade on express

Chain

express@4.22.1body-parser@1.20.4qs@6.14.2

  • Lockfile declares "qs": "6.14.2" (exact) on body-parser — only that version satisfies the pin
  • npm overrides isolate the deep path and pin vulnerable versions (same technique as deep-chain-no-fix)

Verified scan output

npm run build
node dist/index.js examples/exact-pinned-intermediate --verbose
Parsed 69 packages from package-lock.json
Found 1 package (1 CVE) with known OSV matches
1 medium finding: qs@6.14.2 (transitive via express → body-parser → qs)
Fix command: npm install express@4.22.2
Does NOT suggest: npm update qs

What changed

  • examples/exact-pinned-intermediate/package.json + package-lock.json
  • examples/readme.md — fixture table + scan command
  • tests/fixture-scan.test.ts — asserts parent upgrade, not npm update qs

Contrast with related fixtures

Fixture Parent declares Expected fix
wrong-parent / pnpm-within-range Range that covers the fix npm update / pnpm update on vulnerable pkg
deep-chain-no-fix Range that does not cover the fix Parent upgrade
exact-pinned-intermediate Exact pin (no range) Parent upgrade — within-range refresh must not apply

Test plan

  • node dist/index.js examples/exact-pinned-intermediate --verbosenpm install express@4.22.2, not npm update qs
  • Fixture documented in examples/readme.md
  • CI (npm test)

Closes Discussion #528 item fixture 2 (exact-pinned-intermediate).

cc: @sonukapoor

…OWASP#2)

Intermediate parent exact-pins qs@6.14.2 — expects npm install express@4.22.2, not npm update qs.

@sonukapoor sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- Restore multiple-versions-same-pkg test block (Discussion OWASP#528 / PR OWASP#633)
- Restore readme entries dropped in rebase (payload, presenton, twenty)
- Drop hardcoded recommendedParentUpgrade.reason assertion input
@Ayush7614

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @sonukapoor — addressed in 3cb5f78:

  1. Restored multiple-versions-same-pkg test block — accidental removal during the fixture branch work; left assigned to @coder-Yash886 / PR Fixture(#11)/multiple versions same pkg #633 unchanged.
  2. Restored readme.md regressions — re-added the twenty snapshot section, payload / presenton local-only rows, and the corrected strapi entry (rebase accident).
  3. Dropped hardcoded reason — removed the manually authored recommendedParentUpgrade.reason from the exact-pinned-intermediate mock; the test still asserts the fix command and target kinds.

Ready for another look.

@sonukapoor sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants