fix: parse dual-document pnpm v9 lockfiles (bootstrap + project sections) - #743
Conversation
|
Thanks @sonukapoor — opened PR implementing your suggested approach from #669. Document selection: on multi-document Fixture + tests: added Closes #669 |
sonukapoor
left a comment
There was a problem hiding this comment.
The try-first / fall-back pattern is the right approach - single-document lockfiles hit zero overhead, and the three-function decomposition is clean and readable. Two things to address before this merges.
peerDependencies note: hasProjectImporterSections only checks dependencies, devDependencies, and optionalDependencies. A lockfile where importers declare only peerDependencies would miss tier-1 and fall to tier-2 - still correct, just worth knowing.
Fixture note: lodash@4.17.20 being intentionally vulnerable is the right call for the e2e scan test, but a short comment in the fixture package.json or readme.md would help a future contributor who might try to "fix" it.
| return YAML.parse(content); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| if (!message.includes("multiple documents")) { |
There was a problem hiding this comment.
String matching on the error message is fragile - the yaml library exposes a stable code property on YAMLParseError that is part of its public TypeScript API since v2.0. The message could be reworded in a future release (yaml v3 is already in next) and this catch would silently stop working.
The safe version:
import { YAMLParseError } from "yaml";
} catch (error) {
if (!(error instanceof YAMLParseError) || error.code !== "MULTIPLE_DOCS") {
throw error;
}
}| const withImporters = documents.find(doc => doc.importers && typeof doc.importers === "object"); | ||
| if (withImporters) return withImporters; | ||
|
|
||
| return documents[documents.length - 1]; |
There was a problem hiding this comment.
This fallback has no test coverage and silently returns the last document if neither tier-1 nor tier-2 matched. In all known pnpm v9 files the project section is last, but if that ordering ever changed the scan would use the wrong document with no error or warning.
Prefer throwing explicitly so we know if this case ever fires in the wild:
throw new Error(
"pnpm-lock.yaml contains multiple YAML documents but none could be identified as the project lockfile. Please open an issue at https://github.com/OWASP/cve-lite-cli/issues"
);…ons) Closes OWASP#669 Select the project lockfile YAML document by looking for importers with project dependency sections (dependencies/devDependencies/optionalDependencies), falling back to any document with importers, then the last document. Adds examples/pnpm-dual-document fixture and regression tests confirming dual-document lockfiles parse and scan without throwing.
Use YAMLParseError MULTIPLE_DOCS instead of message matching, throw when no project lockfile document is found, include peerDependencies in importer detection, document intentionally vulnerable lodash fixture, and add throw test. Co-authored-by: Cursor <cursoragent@cursor.com>
cbff426 to
2879ab0
Compare
Use YAMLParseError MULTIPLE_DOCS instead of message matching, throw when no project lockfile document is found, include peerDependencies in importer detection, document intentionally vulnerable lodash fixture, and add throw test.
2879ab0 to
9158022
Compare
|
Thanks @sonukapoor — all review items addressed in 9158022:
Rebased onto latest |
sonukapoor
left a comment
There was a problem hiding this comment.
Both issues addressed: is in the check, and the fixture has the intentionally-vulnerable comment in both the package.json description and the readme. CI is green. Good to go.
sonukapoor
left a comment
There was a problem hiding this comment.
Both issues addressed: peerDependencies is in the hasProjectImporterSections check, and the fixture has the intentionally-vulnerable comment in both the package.json description and the readme. CI is green. Good to go.
|
Merged - thank you @Ayush7614! |
Summary
Fixes #669 — pnpm v9 lockfiles that contain two YAML documents (
----separated bootstrap + project sections) no longer crash CVE Lite withYAML.parseAllDocuments()errors.Approach
Per @sonukapoor's review on the issue:
YAML.parse()first (single-document lockfiles unchanged)YAML.parseAllDocuments()and select the project lockfile document by finding the document whoseimportersinclude project dependency sections (dependencies/devDependencies/optionalDependencies)importers, then the last documentBootstrap documents in v9 also carry an
importersblock (withpackageManagerDependenciesonly), so the project-section check is more deterministic than document size scoring.Changes
src/parsers/pnpm-lock.ts— sharedparsePnpmLockContent()used byloadFromPnpmLockandbuildPnpmWorkspaceMapexamples/pnpm-dual-document/— minimal dual-document fixture (bootstrap + lodash project lockfile)tests/parsers/pnpm-lock.test.ts— dual-document parsing regression testtests/fixture-scan.test.ts— fixture loads project packages, excludes bootstrappnpmtests/e2e/cve-scanning.test.ts— e2e scan completes with findings on the new fixtureTest plan
npm run buildnpm test -- tests/parsers/pnpm-lock.test.ts tests/fixture-scan.test.ts