From 1e58d2fe8a3bcff2d2044339cfe846b570168d04 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 01:01:02 -0400 Subject: [PATCH] chore(deps): address Copilot review round 4 on #2243 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Narrow each group to the advisories the installed copies are actually in range of. Two advisories can share a package, manifest and patched version while having different vulnerable ranges, so a group-level "does any match?" left the marker, title, severity and table all claiming an advisory that does not apply on this branch. - Escape every free-form Markdown cell, the vulnerable range included. A semver range may contain `||`, which is also the column separator. - Paginate the open-issue lookup instead of capping it at 100. The marker lookup is what makes the sweep idempotent, so a truncated list would file a duplicate for every issue it could not see — at the same scale the alert fetch is built to handle. Pull requests, which the issues endpoint also returns, are dropped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- scripts/dependabot-alerts.mjs | 114 ++++++++++++++++++------- scripts/dependabot-alerts.test.mjs | 130 ++++++++++++++++++++++++++++- 2 files changed, 212 insertions(+), 32 deletions(-) diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index c1650602..1cb67411 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -308,6 +308,51 @@ export function groupAlerts(alerts) { ); } +/** + * Narrow a group to the advisories that actually apply to what is installed. + * + * ⚠️ Grouping is by `(package, manifest, first_patched_version)`, and two + * advisories sharing that triple can still have DIFFERENT vulnerable ranges — + * `>= 3.1.3, < 3.1.6` and `>= 3.0.0, < 3.1.6` both patch at 3.1.6, and an + * installed `3.1.0` matches only the second. Validating at the group level + * ("does ANY advisory match?") keeps both, and the issue's marker, title, + * severity, advisory table and later comments then all claim an advisory that + * does not apply on this branch (Copilot). + * + * @param {ReturnType[number]} group + * @param {Array<{path: string, version: string, hoisted: boolean}>} entries every installed copy + * @returns {{group: ReturnType[number], affected: Array<{path: string, version: string, hoisted: boolean}>} | null} + * `null` when nothing installed is in range of any of the group's advisories + */ +export function narrowToApplicable(group, entries) { + const applies = (advisory, entry) => + semver.satisfies(entry.version, toSemverRange(advisory.range)); + + const advisories = group.advisories.filter((a) => + entries.some((e) => applies(a, e)), + ); + if (advisories.length === 0) return null; + + const affected = entries.filter((e) => advisories.some((a) => applies(a, e))); + const severity = advisories.reduce( + (worst, a) => + (SEVERITY_RANK[a.severity] ?? 0) > (SEVERITY_RANK[worst] ?? 0) + ? a.severity + : worst, + advisories[0].severity, + ); + + return { + group: { + ...group, + advisories, + ghsas: advisories.map((a) => a.ghsa), + severity, + }, + affected, + }; +} + /** * @param {ReturnType[number]} group * @param {number} [count] advisories the issue covers, when that is more than @@ -319,6 +364,9 @@ export function buildIssueTitle(group, count = group.advisories.length) { return `chore(deps): bump \`${group.package}\` to \`${group.fixedIn}\` in \`${group.manifestPath}\` (${n} ${n === 1 ? "advisory" : "advisories"})`; } +/** Escape a value going into a Markdown table cell. */ +const cell = (value) => String(value).replace(/\|/g, "\\|"); + const PLACEMENT_DOC = "https://github.com/modelcontextprotocol/inspector/blob/v2/main/AGENTS.md#dependency-placement"; @@ -363,10 +411,14 @@ export function remediation(affected, declared) { */ export function buildIssueBody(group, { affected, declared, ghsas }) { const covered = ghsas ?? group.ghsas; + // ⚠️ Every free-form cell is escaped, the RANGE included: a semver range is + // allowed to contain `||`, so a disjoint advisory range like + // `>= 1.0, < 2.0 || >= 3.0, < 3.5` would otherwise inject two extra column + // separators and shear the table apart (Copilot). const rows = group.advisories .map( (a) => - `| [${a.ghsa}](${a.url}) | ${a.cve ?? "—"} | ${a.severity} | ${a.range} | ${a.summary.replace(/\|/g, "\\|")} |`, + `| [${a.ghsa}](${a.url}) | ${cell(a.cve ?? "—")} | ${cell(a.severity)} | ${cell(a.range)} | ${cell(a.summary)} |`, ) .join("\n"); @@ -580,23 +632,26 @@ function readManifest(manifestPath) { } } +/** + * Every open `dependabot`-labeled issue. + * + * Paginated rather than capped: the marker lookup is what makes this sweep + * idempotent, so a truncated list files a duplicate for every issue it could + * not see — at exactly the scale the `--slurp`ed alert fetch is built to handle + * (Copilot). `/issues` also returns pull requests, which carry no marker and + * are dropped. + */ function openDependabotIssues(repo, spawn) { - return ( - ghJson(spawn, [ - "issue", - "list", - "--repo", - repo, - "--state", - "open", - "--label", - "dependabot", - "--json", - "number,body", - "--limit", - "100", - ]) ?? [] - ); + const pages = ghJson(spawn, [ + "api", + "--paginate", + "--slurp", + `repos/${repo}/issues?state=open&labels=dependabot&per_page=100`, + ]); + return (pages ?? []) + .flat() + .filter((issue) => !issue.pull_request) + .map((issue) => ({ number: issue.number, body: issue.body })); } /** @@ -816,31 +871,30 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { const manifests = new Map(); const boardProblems = []; - for (const group of groups) { - if (!manifests.has(group.manifestPath)) { - manifests.set(group.manifestPath, readManifest(group.manifestPath)); + for (const rawGroup of groups) { + if (!manifests.has(rawGroup.manifestPath)) { + manifests.set(rawGroup.manifestPath, readManifest(rawGroup.manifestPath)); } - const lock = manifests.get(group.manifestPath); + const lock = manifests.get(rawGroup.manifestPath); if (lock === null) { console.log( - `dependabot-alerts: ${group.manifestPath} absent on ${TARGET_BRANCH} — skipping ${group.package}`, + `dependabot-alerts: ${rawGroup.manifestPath} absent on ${TARGET_BRANCH} — skipping ${rawGroup.package}`, ); continue; } - const entries = lockfileEntries(lock, group.package); - const affected = entries.filter((entry) => - group.advisories.some((a) => - semver.satisfies(entry.version, toSemverRange(a.range)), - ), - ); - if (affected.length === 0) { + const entries = lockfileEntries(lock, rawGroup.package); + const applicable = narrowToApplicable(rawGroup, entries); + if (applicable === null) { const seen = [...new Set(entries.map((e) => e.version))]; console.log( - `dependabot-alerts: ${group.package}@${seen.join("/") || "(absent)"} is already out of range on ${TARGET_BRANCH} — skipping`, + `dependabot-alerts: ${rawGroup.package}@${seen.join("/") || "(absent)"} is already out of range on ${TARGET_BRANCH} — skipping`, ); continue; } + // From here on `group` carries only the advisories that apply to this + // branch, so the marker, title, severity and table cannot overstate it. + const { group, affected } = applicable; const declared = isDirectDependency(lock, group.package); // Matched on the full grouping key, `fixedIn` included: a second bump of diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index c1926e35..076d4a9d 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -16,6 +16,7 @@ import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { buildCommentMarker, + narrowToApplicable, lockfileEntries, remediation, buildIssueBody, @@ -263,6 +264,76 @@ const hoisted = (version = "3.1.5") => ({ declared: true, }); +test("narrowToApplicable drops advisories the installed version is out of range of", () => { + // Same package, manifest and patched version, so one group — but different + // vulnerable ranges, and 3.1.0 is in range of only one of them. + const [group] = groupAlerts([ + alert({ + ghsa: "GHSA-narrow", + range: ">= 3.1.3, < 3.1.6", + severity: "critical", + }), + alert({ + ghsa: "GHSA-wide", + range: ">= 3.0.0, < 3.1.6", + severity: "medium", + }), + ]); + assert.equal(group.advisories.length, 2); + + const result = narrowToApplicable(group, [ + { path: "node_modules/fast-uri", version: "3.1.0", hoisted: true }, + ]); + assert.deepEqual(result.group.ghsas, ["GHSA-wide"]); + // Severity is re-derived: the critical one does not apply here. + assert.equal(result.group.severity, "medium"); + assert.equal(result.affected.length, 1); + // ...and the marker cannot claim an advisory this branch is not exposed to. + assert.deepEqual(parseMarker(buildMarker(result.group)).ghsas, ["GHSA-wide"]); +}); + +test("narrowToApplicable keeps every advisory that does apply", () => { + const [group] = groupAlerts([ + alert({ ghsa: "GHSA-narrow", range: ">= 3.1.3, < 3.1.6" }), + alert({ ghsa: "GHSA-wide", range: ">= 3.0.0, < 3.1.6" }), + ]); + const result = narrowToApplicable(group, [ + { path: "node_modules/fast-uri", version: "3.1.5", hoisted: true }, + ]); + assert.deepEqual(result.group.ghsas, ["GHSA-narrow", "GHSA-wide"]); +}); + +test("narrowToApplicable returns null when nothing installed is in range", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + assert.equal( + narrowToApplicable(group, [ + { path: "node_modules/fast-uri", version: "3.1.6", hoisted: true }, + ]), + null, + ); + assert.equal(narrowToApplicable(group, []), null); +}); + +test("buildIssueBody escapes a disjoint range so the table survives it", () => { + // `||` is legal in a semver range and is also the Markdown column separator. + const [group] = groupAlerts([ + alert({ ghsa: "GHSA-a", range: ">= 1.0.0, < 2.0.0 || >= 3.0.0, < 3.5.0" }), + ]); + const body = buildIssueBody(group, nested()); + const row = body + .split("\n") + .find((line) => line.includes("GHSA-a") && line.startsWith("|")); + // Count only the pipes Markdown will treat as separators: five columns means + // four inner separators plus the two outer ones. + const separators = row.replace(/\\\|/g, "").split("|").length - 1; + assert.equal( + separators, + 6, + `escaped row should keep its column count: ${row}`, + ); + assert.ok(row.includes(String.raw`\|\|`), "the range's own pipes survive"); +}); + test("buildIssueTitle names the bump and pluralizes the advisory count", () => { const [many] = groupAlerts([ alert({ ghsa: "GHSA-a" }), @@ -460,8 +531,9 @@ function fakeSpawn({ return ok(JSON.stringify(alertPages)); } if (joined.includes("milestones")) return ok(milestone); - if (args[0] === "issue" && args[1] === "list") - return ok(JSON.stringify(issues)); + // `--slurp`, so one array per page — `issues` may be a flat list or pages. + if (joined.includes("/issues?")) + return ok(JSON.stringify(Array.isArray(issues[0]) ? issues : [issues])); if (args[0] === "issue" && args[1] === "view") return ok(JSON.stringify({ comments })); if (args[0] === "issue" && args[1] === "create") @@ -823,6 +895,60 @@ test("main leaves an unmilestoned issue off the board for triage", () => { assert.ok(log.some((l) => l.includes("unmilestoned and unboarded"))); }); +test("main reads every page of open dependabot issues", () => { + // The second page holds the matching marker. Truncating the lookup would + // file a duplicate issue rather than recognising this one. + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + issues: [ + [{ number: 1, body: "an unrelated dependabot issue" }], + [{ number: 41, body: buildIssueBody(group, nested()) }], + ], + }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.5") }, + () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.equal(ghCall(spawn, "create"), undefined); + assert.ok(log.some((l) => l.includes("#41 already covers"))); +}); + +test("main drops a pull request returned by the issues endpoint", () => { + // `/issues` returns PRs too; one carrying no marker must not be mistaken for + // a match, nor crash the lookup. + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + issues: [[{ number: 9, body: "a PR body", pull_request: { url: "..." } }]], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.ok(ghCall(spawn, "create"), "the issue is still filed"); +}); + +test("main files an issue naming only the advisories that apply here", () => { + const spawn = fakeSpawn({ + alertPages: [ + [ + alert({ ghsa: "GHSA-narrow", range: ">= 3.1.3, < 3.1.6" }), + alert({ ghsa: "GHSA-wide", range: ">= 3.0.0, < 3.1.6" }), + ], + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.0") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + const create = ghCall(spawn, "create"); + const body = create.args[create.args.indexOf("--body") + 1]; + assert.deepEqual(parseMarker(body).ghsas, ["GHSA-wide"]); + assert.ok(!body.includes("GHSA-narrow"), "3.1.0 is out of that range"); + assert.match( + create.args[create.args.indexOf("--title") + 1], + /\(1 advisory\)$/, + ); +}); + test("main fails the run when a card is added but its fields are not set", () => { const spawn = fakeSpawn({ alertPages: [