security: drop phantom glob dependency from the production closure, release as 0.1.2 - #157
Merged
Merged
Conversation
domain-router and readme-first-resolver both declared glob ^10.3.0 under dependencies, but neither package imports it, statically or dynamically. Because both are published with private:false, that pulled glob, minimatch and brace-expansion into the production closure with dev=false, exposing consumers running `npm audit --omit=dev` to GHSA-mh99-v99m-4gvg via brace-expansion 2.1.2. Removing the declaration takes all three out of the production closure. Verified: zero lockfile entries for glob, minimatch or brace-expansion carry dev=false afterwards. This does not change the local `npm audit` count. Those packages remain in the report through the dev-side path babel-plugin-istanbul -> test-exclude, untouched here. What changes is what a downstream consumer sees with --omit=dev. Refs: ca2aceff-069c-4568-b0e9-6676cb2f0d0a
…ntom dep Review found that removing the dependency does not reach a single consumer. Both packages sit at 0.1.1, that version is already tagged and published, publishing is tag-driven and hard-fails on a version/tag mismatch, and npm forbids republishing an existing version. The registry therefore still serves 0.1.1 declaring glob ^10.3.0, so the vulnerable closure stayed exactly as reachable as before. Bumps both packages to 0.1.2 with a CHANGELOG entry citing the advisory. The same review also found an identical phantom one line above the removed one: readme-first-resolver declared js-yaml without ever importing it. Its full import set is fs, path, commander, chalk and its own lib, with os and child_process in tests only. Removed there, together with the @types counterpart. domain-router genuinely uses js-yaml and is untouched. Refs: ca2aceff-069c-4568-b0e9-6676cb2f0d0a
…e from security Both entries said the package is "published with private: false", but neither manifest has a private field at all. The statement was semantically right and grep-hostile: someone checking the manifest finds nothing. Now points at what is actually there, an absent private and publishConfig.access public. readme-first-resolver filed both removals under Security, but only glob carried an advisory. js-yaml has none here and was removed as hygiene, so inflating the Security section overstates the exposure. Split into its own Removed entry that says so. Refs: ca2aceff-069c-4568-b0e9-6676cb2f0d0a
LanNguyenSi
added
review:tests-pass
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
domain-routerandreadme-first-resolverboth declaredglob ^10.3.0underdependenciesbut never imported it. Because both are published, that pulledglob,minimatchandbrace-expansion@2.1.2into every consumer'sproduction closure, exposing them to
GHSA-mh99-v99m-4gvg (HIGH,
range
<=5.0.7) onnpm audit --omit=dev.Closes task
ca2aceff.What changed
globremoved from both packages.js-yaml, removed fromreadme-first-resolveronly.domain-routergenuinely uses it and is untouched.published and npm forbids republishing a version. Without the bump this fix
would have reached no consumer at all.
What this does NOT do
The local
npm auditcount stays at 20 high. Those advisories come from thedev-side
babel-plugin-istanbul→test-excludechain, which is untouchedhere and tracked separately. What changes is the production closure, i.e.
what a downstream consumer sees with
--omit=dev. Anyone expecting this PR togreen the Audit check will be surprised; that is expected, not a failure.
Verification
Independently reproduced by the reviewer, not carried over from the
implementer:
flags: 186 packages on master → 153 here, with
glob,minimatchandbrace-expansionabsent and zero flag disagreements in either direction.js-yamlcorrectly remains in the closure fordomain-routeranddebug-playbook-engine.npm audit --omit=dev: 3 high on master → 0 high here. Plainnpm audit:20 high on both, identical advisory set.
version fields changed. No
version/resolved/integritydrift.npm install --package-lock-onlyis a byte-identical no-op.no variable- or template-argument
require/import(), nocreateRequire,module._load,evalornew Function, acrosssrcanddist.Not re-run: the 66 vitest files, because no vitest package's manifest changed
on this branch and other agents were using the machine.
Open action after merge
The goal is met on publish, not on merge. Until
domain-router-v0.1.2andreadme-first-resolver-v0.1.2are pushed andpublish-libs.ymlcompletes,consumers still resolve the vulnerable closure. Verified that 0.1.2 is free on
the registry for both packages and no such tag exists yet.
Follow-ups filed
a5cd84a0— acheck-depsguard so the phantom-dependency class cannotrecur. Two phantoms in one manifest is a pattern, not an accident.
eefeb6a9— 304 of 603 lockfile entries carry neitherresolvednorintegrity, sonpm cicannot verify half the tree. Pre-existing.