Conversation
|
any update on this? |
|
@ubeddulla Thanks for working on this. The implementation is aligned with npm/rfcs#868: the RFC explicitly includes The production change also appears correct: passing the resolved policy through Before merging, we need stronger regression coverage for the behavior this PR is fixing. Current test does not reproduce the bugThe new test in dependencies: { abbrev: '1.1.1' }
It does not prove that:
The test could pass even if pacote ignored the option or if the relevant source dependency path remained vulnerable. Requested regression coveragePlease replace or supplement the {
"name": "prepare-fixture",
"version": "1.0.0",
"scripts": {
"prepare": "node prepare.js"
}
}// prepare.js
require('node:fs').writeFileSync('prepare-ran', '')Please use Node for the side effect rather than shell redirection so that the test works on Windows. Required cases
A local fixture or mocked Git transport is preferable; the test must not contact GitHub or another live network service. Cover the second production call siteThis PR also modifies the That path is used when Arborist builds a complete ideal tree and cracks open a dependency containing bundled dependencies, including package-lock-only operations. The current test only exercises the extraction call in Please add targeted coverage in: At minimum, verify that this extraction receives: ignoreScripts: truefor an unreviewed dependency, and: ignoreScripts: falsefor an explicitly approved dependency or when If practical, use the same observable Acceptance criteriaThe updated tests should fail against the PR's parent commit for the intended reason: the unapproved source dependency executes
We investigated the broader approve-after-install recovery behavior separately and found that Git dependencies may require re-extraction rather than a normal rebuild. That is substantially larger follow-up work and is not being requested as part of this PR. This PR should remain focused on closing the extraction-time policy bypass and adding accurate regression tests for that fix. |
martinrrm
left a comment
There was a problem hiding this comment.
Tests are not correct and we should add more tests
|
Updated the tests. In reify.js the option-spy is replaced with a real non-registry fixture: a directory dependency whose
I used a directory dep rather than git so the fixture stays local and cross-platform, and it exercises DirFetcher#prepareDir, one of the two prepare paths pacote runs. The unreviewed case fails against the parent commit for the intended reason (prepare runs) and passes here. For the second call site I added coverage in build-ideal-tree.js. It spies the crack-open Both the reify and build-ideal-tree suites are green and lint is clean. |
pacote runs a git or directory dep's
preparescript while extracting it, which happens before the node ever reaches the rebuild queues where the allowScripts gate is applied. WithallowScripts: {}a git dependency's prepare script still runs on install, and npm prints its "install scripts blocked because they are not covered by allowScripts" warning naming that same package while the script is running, so the policy reads as enforced when it is not. Both of pacote's fetchers already honorignoreScripts, so deriving it from the same check rebuild uses closes this at the twopacote.extractcall sites (reify, and the bundle-dep crack-open under--package-lock-only); approved packages and--dangerously-allow-all-scriptsstill run prepare exactly as before.