chore(deps): address Copilot review round 4 on #2243
- 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
GitHub
parent
5c65bd2bfa
commit
1e58d2fe8a
2 changed files with 212 additions and 32 deletions
|
|
@ -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<typeof groupAlerts>[number]} group
|
||||
* @param {Array<{path: string, version: string, hoisted: boolean}>} entries every installed copy
|
||||
* @returns {{group: ReturnType<typeof groupAlerts>[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<typeof groupAlerts>[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
|
||||
|
|
|
|||
|
|
@ -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: [
|
||||
|
|
|
|||
Loading…
Reference in a new issue