chore(deps): address Copilot review round 6 on #2243
Both were suppressed "previously missed" findings, and both were real. - isPermissionDenied matched on status alone, so a rate limit (also a 403) and a bad token (401) were waved through as "missing scope" — contradicting the comment right above it, which said those must stop the sweep. It now excludes rate-limit wording and drops 401. - The no-op path asked "were advisories ADDED?" when the question is "did the issue CHANGE?". An issue filed for A+B whose branch moved so only B applies has nothing added, yet its table, severity, affected copies and remediation are all stale. The rendered title and body are now compared against the issue, and a comment stays reserved for genuinely new advisories. - The title counts the advisories that APPLY, matching the body, so it tracks shrinking exposure as well as growth. The marker's GHSA list stays monotonic — its job is to remember what has been announced. 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
a17eace0bb
commit
ff339dc7c3
2 changed files with 145 additions and 31 deletions
|
|
@ -375,13 +375,16 @@ export function narrowToApplicable(group, entries) {
|
|||
}
|
||||
|
||||
/**
|
||||
* @param {ReturnType<typeof groupAlerts>[number]} group
|
||||
* @param {number} [count] advisories the issue covers, when that is more than
|
||||
* this run saw — an issue grown by a later advisory keeps one title.
|
||||
* The title counts the advisories that APPLY, which is what the body shows —
|
||||
* so it tracks an issue that grows a new advisory and one whose exposure
|
||||
* shrinks alike. The marker's GHSA list is a different thing: it is monotonic,
|
||||
* because its job is to remember what has already been announced.
|
||||
*
|
||||
* @param {ReturnType<typeof groupAlerts>[number]} group narrowed to what applies
|
||||
* @returns {string}
|
||||
*/
|
||||
export function buildIssueTitle(group, count = group.advisories.length) {
|
||||
const n = count;
|
||||
export function buildIssueTitle(group) {
|
||||
const n = group.advisories.length;
|
||||
return `chore(deps): bump \`${group.package}\` to \`${group.fixedIn}\` in \`${group.manifestPath}\` (${n} ${n === 1 ? "advisory" : "advisories"})`;
|
||||
}
|
||||
|
||||
|
|
@ -560,19 +563,27 @@ function ghJson(spawn, args) {
|
|||
}
|
||||
|
||||
/**
|
||||
* Is this failed lookup the "the token may not read this" answer, rather than a
|
||||
* real API failure?
|
||||
* Is this failed lookup the "this token may not read that" answer, rather than
|
||||
* a real API failure?
|
||||
*
|
||||
* The distinction is what keeps the security-PR guard honest: a bad token, a
|
||||
* rate limit or a transient 5xx must NOT be waved through as "unverified", or
|
||||
* the sweep exits green having silently skipped its own precondition
|
||||
* (Copilot). Only an authorization-shaped status is tolerated.
|
||||
* the sweep exits green having silently skipped its own precondition.
|
||||
*
|
||||
* ⚠️ Status alone is not enough, which is what the first version got wrong
|
||||
* (Copilot). GitHub answers BOTH "you lack `administration: read`" and "you
|
||||
* have exhausted your quota" with **403**, and the second is a real failure —
|
||||
* so the rate-limit wording is excluded explicitly. **401** is a bad or expired
|
||||
* token, never a scope question, and is a real failure too. **404** stays
|
||||
* tolerated because GitHub hides resources a token cannot see behind one rather
|
||||
* than admitting they exist.
|
||||
*
|
||||
* @param {string} stderr stderr from a non-zero `gh api` call
|
||||
* @returns {boolean}
|
||||
*/
|
||||
export function isPermissionDenied(stderr) {
|
||||
return /HTTP (401|403|404)\b/.test(stderr);
|
||||
if (/rate limit/i.test(stderr)) return false;
|
||||
return /HTTP (403|404)\b/.test(stderr);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -672,7 +683,11 @@ function openDependabotIssues(repo, spawn) {
|
|||
return (pages ?? [])
|
||||
.flat()
|
||||
.filter((issue) => !issue.pull_request)
|
||||
.map((issue) => ({ number: issue.number, body: issue.body }));
|
||||
.map((issue) => ({
|
||||
number: issue.number,
|
||||
title: issue.title,
|
||||
body: issue.body,
|
||||
}));
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -945,9 +960,26 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) {
|
|||
}
|
||||
|
||||
const { merged, added } = mergeGhsas(existing.marker.ghsas, group.ghsas);
|
||||
if (added.length === 0) {
|
||||
const title = buildIssueTitle(group);
|
||||
const body = buildIssueBody(group, {
|
||||
affected,
|
||||
declared,
|
||||
ghsas: merged,
|
||||
});
|
||||
|
||||
// ⚠️ "Nothing NEW" is not the same as "nothing CHANGED" (Copilot). An issue
|
||||
// filed for A+B whose branch has since moved so only B applies has no added
|
||||
// GHSAs, yet its table, severity, affected copies and remediation are all
|
||||
// stale. So the no-op is decided by comparing the rendered issue, not by
|
||||
// counting additions — while a COMMENT stays reserved for advisories that
|
||||
// are genuinely new.
|
||||
if (
|
||||
added.length === 0 &&
|
||||
existing.title === title &&
|
||||
existing.body === body
|
||||
) {
|
||||
console.log(
|
||||
`dependabot-alerts: #${existing.number} already covers ${group.package} — no-op`,
|
||||
`dependabot-alerts: #${existing.number} is up to date for ${group.package} — no-op`,
|
||||
);
|
||||
continue;
|
||||
}
|
||||
|
|
@ -976,8 +1008,6 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) {
|
|||
}
|
||||
}
|
||||
|
||||
// The title carries the advisory count, so it goes stale the moment the
|
||||
// issue covers one more than it was filed with (Copilot).
|
||||
const edit = gh(spawn, [
|
||||
"issue",
|
||||
"edit",
|
||||
|
|
@ -985,15 +1015,17 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) {
|
|||
"--repo",
|
||||
repo,
|
||||
"--title",
|
||||
buildIssueTitle(group, merged.length),
|
||||
title,
|
||||
"--body",
|
||||
buildIssueBody(group, { affected, declared, ghsas: merged }),
|
||||
body,
|
||||
]);
|
||||
if (edit.status !== 0) {
|
||||
throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`);
|
||||
}
|
||||
console.log(
|
||||
`dependabot-alerts: added ${added.join(", ")} to #${existing.number}`,
|
||||
added.length > 0
|
||||
? `dependabot-alerts: added ${added.join(", ")} to #${existing.number}`
|
||||
: `dependabot-alerts: refreshed #${existing.number} for ${group.package}`,
|
||||
);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -212,18 +212,35 @@ test("buildCommentMarker and parseCommentMarker round-trip", () => {
|
|||
assert.equal(parseCommentMarker("an ordinary comment"), null);
|
||||
});
|
||||
|
||||
test("isPermissionDenied separates an authorization failure from a real one", () => {
|
||||
test("isPermissionDenied tolerates a scope refusal", () => {
|
||||
// The two ways GitHub says "this token may not read that": an explicit 403,
|
||||
// and a 404 hiding a resource the token cannot see.
|
||||
assert.equal(
|
||||
isPermissionDenied("gh: HTTP 403: Resource not accessible"),
|
||||
isPermissionDenied("gh: HTTP 403: Resource not accessible by integration"),
|
||||
true,
|
||||
);
|
||||
assert.equal(isPermissionDenied("gh: HTTP 401: Bad credentials"), true);
|
||||
assert.equal(isPermissionDenied("gh: HTTP 404: Not Found"), true);
|
||||
});
|
||||
|
||||
test("isPermissionDenied treats a bad token or a rate limit as a real failure", () => {
|
||||
// ⚠️ A rate limit is also a 403, so status alone cannot decide this — waving
|
||||
// it through would exit green having skipped the sweep's own precondition.
|
||||
assert.equal(
|
||||
isPermissionDenied("gh: API rate limit exceeded (HTTP 403)"),
|
||||
false,
|
||||
);
|
||||
assert.equal(
|
||||
isPermissionDenied(
|
||||
"gh: HTTP 403: You have exceeded a secondary rate limit",
|
||||
),
|
||||
false,
|
||||
);
|
||||
// 401 is a bad or expired token, never a scope question.
|
||||
assert.equal(isPermissionDenied("gh: HTTP 401: Bad credentials"), false);
|
||||
assert.equal(
|
||||
isPermissionDenied("gh: HTTP 500: Internal Server Error"),
|
||||
false,
|
||||
);
|
||||
assert.equal(isPermissionDenied("gh: API rate limit exceeded"), false);
|
||||
});
|
||||
|
||||
test("parseMarker returns null for an unmarked or absent body", () => {
|
||||
|
|
@ -637,6 +654,16 @@ function inTempRepo(files, body) {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The probe `main()` derives from `lockWith`: one hoisted, undeclared copy.
|
||||
* A fixture issue built from this renders byte-identically to what `main()`
|
||||
* would produce, which is what makes the no-op path assertable.
|
||||
*/
|
||||
const asInstalled = (version = "3.1.5") => ({
|
||||
affected: [{ path: "node_modules/fast-uri", version, hoisted: true }],
|
||||
declared: false,
|
||||
});
|
||||
|
||||
/** A lockfile holding one transitive copy of `pkg` at `version`. */
|
||||
const lockWith = (pkg, version) => ({
|
||||
lockfileVersion: 3,
|
||||
|
|
@ -739,7 +766,8 @@ test("main is a complete no-op on a second run", () => {
|
|||
issues: [
|
||||
{
|
||||
number: 41,
|
||||
body: buildIssueBody(group, nested()),
|
||||
title: buildIssueTitle(group),
|
||||
body: buildIssueBody(group, asInstalled()),
|
||||
},
|
||||
],
|
||||
});
|
||||
|
|
@ -750,7 +778,7 @@ test("main is a complete no-op on a second run", () => {
|
|||
assert.equal(ghCall(spawn, "create"), undefined);
|
||||
assert.equal(ghCall(spawn, "comment"), undefined);
|
||||
assert.equal(ghCall(spawn, "edit"), undefined);
|
||||
assert.ok(log.some((l) => l.includes("#41 already covers")));
|
||||
assert.ok(log.some((l) => l.includes("#41 is up to date")));
|
||||
});
|
||||
|
||||
test("main will not update an issue whose bump differs, even for the same package", () => {
|
||||
|
|
@ -761,7 +789,8 @@ test("main will not update an issue whose bump differs, even for the same packag
|
|||
issues: [
|
||||
{
|
||||
number: 41,
|
||||
body: buildIssueBody(old, nested()),
|
||||
title: buildIssueTitle(old),
|
||||
body: buildIssueBody(old, asInstalled()),
|
||||
},
|
||||
],
|
||||
});
|
||||
|
|
@ -779,7 +808,8 @@ test("main comments a new advisory BEFORE rewriting the marker", () => {
|
|||
issues: [
|
||||
{
|
||||
number: 41,
|
||||
body: buildIssueBody(old, nested()),
|
||||
title: buildIssueTitle(old),
|
||||
body: buildIssueBody(old, asInstalled()),
|
||||
},
|
||||
],
|
||||
});
|
||||
|
|
@ -814,7 +844,8 @@ test("main does not repeat a comment it already posted", () => {
|
|||
issues: [
|
||||
{
|
||||
number: 41,
|
||||
body: buildIssueBody(old, nested()),
|
||||
title: buildIssueTitle(old),
|
||||
body: buildIssueBody(old, asInstalled()),
|
||||
},
|
||||
],
|
||||
comments: [{ body: `${buildCommentMarker(["GHSA-b"])}\nsaid already` }],
|
||||
|
|
@ -843,7 +874,8 @@ test("main announces only the advisories no comment has claimed yet", () => {
|
|||
issues: [
|
||||
{
|
||||
number: 41,
|
||||
body: buildIssueBody(old, nested()),
|
||||
title: buildIssueTitle(old),
|
||||
body: buildIssueBody(old, asInstalled()),
|
||||
},
|
||||
],
|
||||
comments: [{ body: `${buildCommentMarker(["GHSA-b"])}\nannounced b` }],
|
||||
|
|
@ -866,7 +898,8 @@ test("main refreshes the title when an issue grows another advisory", () => {
|
|||
issues: [
|
||||
{
|
||||
number: 41,
|
||||
body: buildIssueBody(old, nested()),
|
||||
title: buildIssueTitle(old),
|
||||
body: buildIssueBody(old, asInstalled()),
|
||||
},
|
||||
],
|
||||
});
|
||||
|
|
@ -934,6 +967,49 @@ test("main leaves an unmilestoned issue off the board for triage", () => {
|
|||
assert.ok(log.some((l) => l.includes("unmilestoned and unboarded")));
|
||||
});
|
||||
|
||||
test("main refreshes an issue whose exposure shrank, without commenting", () => {
|
||||
// Filed when both advisories applied; `v2/main` has since moved to 3.1.0,
|
||||
// which is out of range of the narrow one. Nothing is NEW, so `added` is
|
||||
// empty — but the body still claims an advisory that no longer applies.
|
||||
const [filed] = groupAlerts([
|
||||
alert({ ghsa: "GHSA-narrow", range: ">= 3.1.3, < 3.1.6" }),
|
||||
alert({ ghsa: "GHSA-wide", range: ">= 3.0.0, < 3.1.6" }),
|
||||
]);
|
||||
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" }),
|
||||
],
|
||||
],
|
||||
issues: [
|
||||
{
|
||||
number: 41,
|
||||
title: buildIssueTitle(filed),
|
||||
body: buildIssueBody(filed, asInstalled()),
|
||||
},
|
||||
],
|
||||
});
|
||||
const log = inTempRepo(
|
||||
{ "package-lock.json": lockWith("fast-uri", "3.1.0") },
|
||||
() => withoutProjectToken(() => captureLog(() => main("o/r", spawn))),
|
||||
);
|
||||
|
||||
// A comment is for genuinely new advisories, and there are none.
|
||||
assert.equal(ghCall(spawn, "comment"), undefined);
|
||||
|
||||
const edit = ghCall(spawn, "edit");
|
||||
assert.ok(edit, "the stale body is rewritten");
|
||||
const body = edit.args[edit.args.indexOf("--body") + 1];
|
||||
assert.ok(!body.includes("| [GHSA-narrow]"), "no longer in the table");
|
||||
assert.match(body, /`3\.1\.0`/, "the affected version is refreshed");
|
||||
assert.match(edit.args[edit.args.indexOf("--title") + 1], /\(1 advisory\)$/);
|
||||
// The marker stays monotonic: it records what has been announced, so the
|
||||
// dropped advisory cannot be re-announced later.
|
||||
assert.deepEqual(parseMarker(body).ghsas, ["GHSA-narrow", "GHSA-wide"]);
|
||||
assert.ok(log.some((l) => l.includes("refreshed #41")));
|
||||
});
|
||||
|
||||
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.
|
||||
|
|
@ -942,7 +1018,13 @@ test("main reads every page of open dependabot issues", () => {
|
|||
alertPages: [[alert({ ghsa: "GHSA-a" })]],
|
||||
issues: [
|
||||
[{ number: 1, body: "an unrelated dependabot issue" }],
|
||||
[{ number: 41, body: buildIssueBody(group, nested()) }],
|
||||
[
|
||||
{
|
||||
number: 41,
|
||||
title: buildIssueTitle(group),
|
||||
body: buildIssueBody(group, asInstalled()),
|
||||
},
|
||||
],
|
||||
],
|
||||
});
|
||||
const log = inTempRepo(
|
||||
|
|
@ -950,7 +1032,7 @@ test("main reads every page of open dependabot issues", () => {
|
|||
() => withoutProjectToken(() => captureLog(() => main("o/r", spawn))),
|
||||
);
|
||||
assert.equal(ghCall(spawn, "create"), undefined);
|
||||
assert.ok(log.some((l) => l.includes("#41 already covers")));
|
||||
assert.ok(log.some((l) => l.includes("#41 is up to date")));
|
||||
});
|
||||
|
||||
test("main drops a pull request returned by the issues endpoint", () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue