Skip to content

Commit bdfb2e3

Browse files
cliffhallcliffhall
authored
Merge pull request #5529 from modelcontextprotocol/v2/chore/5506-alerts-hoisted-copy-scope
fix(scripts): scope a hoisted npm copy by the ranges it satisfies
2 parents 1a42b3b + 27122a6 commit bdfb2e3

2 files changed

Lines changed: 118 additions & 41 deletions

File tree

‎scripts/dependabot-alerts.mjs‎

Lines changed: 46 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -271,11 +271,12 @@ export function npmLockEntries(lock, pkg) {
271271

272272
/**
273273
* The manifests that declare `pkg` (any dependency section): the root, and
274-
* each workspace, as the lockfile records them.
274+
* each workspace, as the lockfile records them, with the range each declares.
275275
*
276276
* @param {object} lock parsed `package-lock.json`
277277
* @param {string} pkg
278-
* @returns {string[]} manifest paths, e.g. `package.json`, `src/everything/package.json`
278+
* @returns {Array<{manifest: string, range: string}>} sorted by manifest path,
279+
* e.g. `package.json`, `src/everything/package.json`
279280
*/
280281
export function npmDeclarers(lock, pkg) {
281282
const declarers = [];
@@ -287,12 +288,13 @@ export function npmDeclarers(lock, pkg) {
287288
entry?.optionalDependencies?.[pkg] ??
288289
entry?.peerDependencies?.[pkg];
289290
if (declared) {
290-
declarers.push(
291-
lockPath === "" ? "package.json" : `${lockPath}/package.json`,
292-
);
291+
declarers.push({
292+
manifest: lockPath === "" ? "package.json" : `${lockPath}/package.json`,
293+
range: declared,
294+
});
293295
}
294296
}
295-
return declarers.sort();
297+
return declarers.sort((a, b) => a.manifest.localeCompare(b.manifest));
296298
}
297299

298300
/**
@@ -310,8 +312,9 @@ export function overrideAncestors(lockPath) {
310312

311313
/**
312314
* A parent-scoped `overrides` block for the nested vulnerable copies. npm
313-
* rejects a package-wide override that contradicts a direct dependency of the
314-
* same name (`EOVERRIDE`), so when a manifest declares the package the nested
315+
* rejects a package-wide override that contradicts the ROOT package's own
316+
* direct dependency of the same name (`EOVERRIDE`; a workspace's declaration
317+
* does not trigger it), so when the root declares the package the nested
315318
* copies have to be reached through their parents.
316319
*
317320
* @param {Array<{path: string}>} affected
@@ -651,7 +654,7 @@ export function buildIssueTitle(group) {
651654
* the root declares, or one no manifest declares, names no server.
652655
*
653656
* @param {{manifestPath: string, ecosystem?: string}} group
654-
* @param {{affected?: Array<{path: string, topLevel: boolean}>, declarers?: string[]}} [probe]
657+
* @param {{affected?: Array<{path: string, version?: string, topLevel: boolean}>, declarers?: Array<{manifest: string, range: string}>}} [probe]
655658
* @returns {string[]}
656659
*/
657660
export function issueLabels(group, { affected = [], declarers = [] } = {}) {
@@ -670,41 +673,56 @@ export function issueLabels(group, { affected = [], declarers = [] } = {}) {
670673
return [...SWEEP_LABELS, ...labels];
671674
}
672675

673-
/**
674-
* What an npm fix edits, from WHICH copies are vulnerable: the declared copy
675-
* needs its range raised, a nested or undeclared copy needs an `overrides`
676-
* pin, and both at once need both.
677-
*/
678676
/**
679677
* The manifests whose declared range resolves to this copy. A copy under
680678
* another package's `node_modules` is nobody's declaration. A copy at the top
681679
* of a workspace's own `node_modules` is that workspace's, and only if that
682680
* workspace declares the package: the root declaring it says nothing about a
683-
* copy the workspace could not share. The hoisted root copy serves every
684-
* manifest that declares the package.
681+
* copy the workspace could not share. The hoisted root copy serves each
682+
* manifest whose declared range its version satisfies; a manifest it does not
683+
* satisfy gets a nested copy of its own, so for that manifest the root copy is
684+
* only something other packages pulled in (#5506). A range semver cannot read
685+
* (an alias, a URL) is assumed to resolve to the root copy, as before.
685686
*
686-
* @param {{path: string, topLevel: boolean}} entry
687-
* @param {string[]} declarers every manifest that declares the package
688-
* @returns {string[]}
687+
* @param {{path: string, version?: string, topLevel: boolean}} entry
688+
* @param {Array<{manifest: string, range: string}>} declarers every manifest
689+
* that declares the package, with its declared range
690+
* @returns {string[]} manifest paths
689691
*/
690692
export function npmCopyDeclarers(entry, declarers) {
691693
if (!entry.topLevel) return [];
692694
const { owner } = splitOwner(entry.path);
693-
return owner === ""
694-
? declarers
695-
: declarers.filter((m) => m === `${owner}/package.json`);
695+
if (owner !== "") {
696+
return declarers
697+
.filter((d) => d.manifest === `${owner}/package.json`)
698+
.map((d) => d.manifest);
699+
}
700+
return declarers
701+
.filter(
702+
(d) =>
703+
semver.validRange(d.range) === null ||
704+
semver.valid(entry.version ?? "") === null ||
705+
semver.satisfies(entry.version, d.range),
706+
)
707+
.map((d) => d.manifest);
696708
}
697709

710+
/**
711+
* What an npm fix edits, from WHICH copies are vulnerable: the declared copy
712+
* needs its range raised, a nested or undeclared copy needs an `overrides`
713+
* pin, and both at once need both.
714+
*/
698715
function npmFix(group, { affected, declarers }) {
699716
const copyDeclarers = (entry) => npmCopyDeclarers(entry, declarers);
700717
const raise = [...new Set(affected.flatMap(copyDeclarers))].sort();
701718
const direct = raise.length > 0;
702719
const nested = affected.filter((entry) => copyDeclarers(entry).length === 0);
703720
const transitive = nested.length > 0;
704-
// A package-wide override contradicting ANY declaration of the package is
705-
// refused (`EOVERRIDE`), so the scoped form is needed whenever one exists,
706-
// not only when the declared copy is itself vulnerable.
707-
const scoped = declarers.length > 0;
721+
// A package-wide override contradicting the ROOT package's own declaration
722+
// is refused (`EOVERRIDE`), so the scoped form is needed whenever the root
723+
// declares the package, not only when the root's copy is itself vulnerable.
724+
// A workspace's declaration does not trigger it (verified against npm 11).
725+
const scoped = declarers.some((d) => d.manifest === "package.json");
708726
const unscopable = nested.some((e) => overrideAncestors(e.path).length === 0);
709727
const steps = [];
710728
if (direct) {
@@ -715,7 +733,7 @@ function npmFix(group, { affected, declarers }) {
715733
if (transitive) {
716734
steps.push(
717735
scoped
718-
? `**Add a parent-scoped [\`overrides\`](${DEPENDENCIES_DOC}) entry** in the root \`package.json\` for the nested copies below:\n\n\`\`\`json\n${scopedOverrideExample(nested, group)}\n\`\`\`\n\n${unscopable ? ` A copy at the top of a workspace's own \`node_modules\` names no parent in its path; \`npm explain ${group.package}\` shows which dependency pulls it in, to scope the entry under.\n\n` : ""} A package-wide \`"${group.package}": "${group.fixedIn}"\` would fail with \`EOVERRIDE\`, because a manifest also declares \`${group.package}\` directly. **Not** \`npm audit fix\`, which can resolve an advisory by silently downgrading.`
736+
? `**Add a parent-scoped [\`overrides\`](${DEPENDENCIES_DOC}) entry** in the root \`package.json\` for the nested copies below:\n\n\`\`\`json\n${scopedOverrideExample(nested, group)}\n\`\`\`\n\n${unscopable ? ` A copy at the top of a workspace's own \`node_modules\` names no parent in its path; \`npm explain ${group.package}\` shows which dependency pulls it in, to scope the entry under.\n\n` : ""} A package-wide \`"${group.package}": "${group.fixedIn}"\` would fail with \`EOVERRIDE\`, because the root \`package.json\` also declares \`${group.package}\` directly. **Not** \`npm audit fix\`, which can resolve an advisory by silently downgrading.`
719737
: `**Add an [\`overrides\`](${DEPENDENCIES_DOC}) entry** in the root \`package.json\` pinning \`${group.package}\` to \`>=${group.fixedIn}\`, for the copies below, which no declared range reaches. **Not** \`npm audit fix\`, which can resolve an advisory by silently downgrading.`,
720738
);
721739
}
@@ -737,7 +755,7 @@ function pipFix(group, { declaredSpec }) {
737755

738756
/**
739757
* @param {ReturnType<typeof groupAlerts>[number]} group narrowed to what applies
740-
* @param {{affected: Array<{path: string, version: string, topLevel: boolean}>, declarers?: string[], declaredSpec?: string, ghsas?: string[], securityPrsOff?: boolean}} probe
758+
* @param {{affected: Array<{path: string, version: string, topLevel: boolean}>, declarers?: Array<{manifest: string, range: string}>, declaredSpec?: string, ghsas?: string[], securityPrsOff?: boolean}} probe
741759
* @returns {string}
742760
*/
743761
export function buildIssueBody(

‎scripts/dependabot-alerts.test.mjs‎

Lines changed: 72 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -282,9 +282,11 @@ test("npm lockfile reading is workspace-aware", () => {
282282
[],
283283
);
284284
assert.deepEqual(npmDeclarers(NPM_LOCK, "hono"), [
285-
"src/everything/package.json",
285+
{ manifest: "src/everything/package.json", range: "^4.12.0" },
286+
]);
287+
assert.deepEqual(npmDeclarers(NPM_LOCK, "semver"), [
288+
{ manifest: "package.json", range: "^7" },
286289
]);
287-
assert.deepEqual(npmDeclarers(NPM_LOCK, "semver"), ["package.json"]);
288290
assert.deepEqual(npmDeclarers(NPM_LOCK, "fast-uri"), []);
289291
assert.deepEqual(
290292
overrideAncestors("node_modules/@scope/a/node_modules/b/node_modules/c"),
@@ -480,7 +482,7 @@ test("a vulnerable interval above the target is called out, and every fix is ver
480482
affected: [
481483
{ path: "node_modules/hono", version: "4.12.0", topLevel: true },
482484
],
483-
declarers: ["package.json"],
485+
declarers: [decl("package.json")],
484486
});
485487
assert.match(body, /\*\*Then verify\*\*/);
486488
});
@@ -575,23 +577,23 @@ test("npm labels come from the vulnerable copies, not the root lockfile", () =>
575577
assert.deepEqual(
576578
issueLabels(npm, {
577579
affected: [{ path: "node_modules/hono", topLevel: true }],
578-
declarers: ["src/everything/package.json"],
580+
declarers: [decl("src/everything/package.json")],
579581
}),
580582
[...base, "server-everything"],
581583
);
582584
// A copy in a workspace's own node_modules, declared or not.
583585
assert.deepEqual(
584586
issueLabels(npm, {
585587
affected: [{ path: "src/filesystem/node_modules/diff", topLevel: true }],
586-
declarers: ["package.json"],
588+
declarers: [decl("package.json")],
587589
}),
588590
[...base, "server-filesystem"],
589591
);
590592
// Declared by the root only, or by nobody: no server.
591593
assert.deepEqual(
592594
issueLabels(npm, {
593595
affected: [{ path: "node_modules/semver", topLevel: true }],
594-
declarers: ["package.json"],
596+
declarers: [decl("package.json")],
595597
}),
596598
base,
597599
);
@@ -604,6 +606,11 @@ test("npm labels come from the vulnerable copies, not the root lockfile", () =>
604606
);
605607
});
606608

609+
/** A manifest declaring the package; `*` lets any copy satisfy it. */
610+
function decl(manifest, range = "*") {
611+
return { manifest, range };
612+
}
613+
607614
function npmGroup(pkg, fixedIn, range = "< 99") {
608615
const [group] = groupAlerts([
609616
alert({ name: pkg, range, fixed: fixedIn, ghsa: "GHSA-x" }),
@@ -630,7 +637,7 @@ test("an npm body says which edit clears which copy", () => {
630637
affected: [
631638
{ path: "node_modules/hono", version: "4.12.0", topLevel: true },
632639
],
633-
declarers: ["src/everything/package.json"],
640+
declarers: [decl("src/everything/package.json")],
634641
securityPrsOff: true,
635642
});
636643
assert.match(
@@ -648,7 +655,7 @@ test("an npm body says which edit clears which copy", () => {
648655
topLevel: false,
649656
},
650657
],
651-
declarers: ["package.json"],
658+
declarers: [decl("package.json")],
652659
});
653660
assert.match(both, /Both edits are needed/);
654661
assert.match(both, /1\. \*\*Raise/);
@@ -662,28 +669,80 @@ test("a workspace's own copy is direct only if that workspace declares it", () =
662669
const ws = { path: "src/filesystem/node_modules/diff", topLevel: true };
663670
const hoisted = { path: "node_modules/diff", topLevel: true };
664671
const nested = { path: "node_modules/a/node_modules/diff", topLevel: false };
665-
assert.deepEqual(npmCopyDeclarers(ws, ["package.json"]), []);
672+
assert.deepEqual(npmCopyDeclarers(ws, [decl("package.json")]), []);
666673
assert.deepEqual(
667-
npmCopyDeclarers(ws, ["package.json", "src/filesystem/package.json"]),
674+
npmCopyDeclarers(ws, [
675+
decl("package.json"),
676+
decl("src/filesystem/package.json"),
677+
]),
668678
["src/filesystem/package.json"],
669679
);
670-
assert.deepEqual(npmCopyDeclarers(hoisted, ["package.json"]), [
680+
assert.deepEqual(npmCopyDeclarers(hoisted, [decl("package.json")]), [
671681
"package.json",
672682
]);
673-
assert.deepEqual(npmCopyDeclarers(nested, ["package.json"]), []);
683+
assert.deepEqual(npmCopyDeclarers(nested, [decl("package.json")]), []);
674684

675685
// The root declares `diff`; only the workspace's undeclared copy is
676686
// vulnerable. The fix is the override, scoped, not the root's range.
677687
const body = buildIssueBody(npmGroup("diff", "5.2.2"), {
678688
affected: [{ ...ws, version: "5.0.0" }],
679-
declarers: ["package.json"],
689+
declarers: [decl("package.json")],
680690
});
681691
assert.ok(!body.includes("Raise the declared range"));
682692
assert.match(body, /\*\*Add a parent-scoped/);
683693
assert.match(body, /npm explain diff/);
684694
assert.match(body, /EOVERRIDE/);
685695
});
686696

697+
test("a hoisted copy no declared range resolves to is transitive (#5506)", () => {
698+
// The #5481 shape: src/filesystem declares minimatch ^11.0.4 and has its
699+
// own fixed nested copy; the hoisted root copy is 10.2.5, pulled in only by
700+
// dev tooling. The root copy is not filesystem's, so the fix is a
701+
// package-wide override and no server is in scope.
702+
const lock = {
703+
packages: {
704+
"": { name: "@modelcontextprotocol/servers", devDependencies: {} },
705+
"src/filesystem": {
706+
name: "@modelcontextprotocol/server-filesystem",
707+
dependencies: { minimatch: "^11.0.4" },
708+
},
709+
"src/filesystem/node_modules/minimatch": { version: "11.0.4" },
710+
"node_modules/minimatch": { version: "10.2.5", dev: true },
711+
},
712+
};
713+
const declarers = npmDeclarers(lock, "minimatch");
714+
assert.deepEqual(declarers, [
715+
{ manifest: "src/filesystem/package.json", range: "^11.0.4" },
716+
]);
717+
const hoisted = npmLockEntries(lock, "minimatch").find(
718+
(e) => e.path === "node_modules/minimatch",
719+
);
720+
assert.ok(hoisted);
721+
assert.deepEqual(npmCopyDeclarers(hoisted, declarers), []);
722+
// The same copy is a declarer's when its range does admit it.
723+
assert.deepEqual(
724+
npmCopyDeclarers(hoisted, [decl("src/filesystem/package.json", "^10.0.0")]),
725+
["src/filesystem/package.json"],
726+
);
727+
728+
const affected = [hoisted];
729+
assert.deepEqual(
730+
issueLabels(
731+
{ manifestPath: "package-lock.json", ecosystem: "npm" },
732+
{ affected, declarers },
733+
),
734+
["v2", "chore", "dependencies"],
735+
);
736+
const body = buildIssueBody(npmGroup("minimatch", "10.2.6"), {
737+
affected,
738+
declarers,
739+
});
740+
assert.ok(!body.includes("Raise the declared range"));
741+
assert.match(body, /\*\*Add an \[`overrides`\]/);
742+
// Only the root's own declaration makes a package-wide override fail.
743+
assert.ok(!body.includes("EOVERRIDE"));
744+
});
745+
687746
test("a pip body says whether to raise the bound or refresh the lock", () => {
688747
const [group] = groupAlerts([
689748
alert({

0 commit comments

Comments
 (0)