diff --git a/.github/workflows/dependabot-alerts.yml b/.github/workflows/dependabot-alerts.yml index 72bd99d3..4e1f3f38 100644 --- a/.github/workflows/dependabot-alerts.yml +++ b/.github/workflows/dependabot-alerts.yml @@ -12,8 +12,11 @@ # security PRs off there is no mergeable-against-`main` artifact and no window to # race, so the merge guard #2060 needed has no analogue here. What replaces it is # a precondition inside the script: `automated-security-fixes` is a repo SETTING -# and can be switched back on from the UI without a commit, so the sweep asserts -# it is still disabled and fails loudly if it is not. +# and can be switched back on from the UI without a commit, so the sweep reads it +# back and fails loudly on an explicit `enabled: true`. That read needs a +# permission `GITHUB_TOKEN` cannot hold, so with the default token it reports +# UNVERIFIED instead — see the token notes below; it is a conditional guard, not +# an invariant. # # ⚠️ GitHub computes the dependency graph, and therefore every alert, from the # DEFAULT branch (`main`), while we ship from `v2/main`. Two consequences: @@ -24,9 +27,10 @@ # waiting on a milestone merge to close, so it is skipped silently. # * A vulnerable dependency introduced on `v2/main` and not yet merged to # `main` produces NO alert at all. No approach that consumes GitHub's alerts -# avoids that. The release-time `npm audit fix` from #2231 is a second signal -# that partially covers it; a scheduled `npm audit --audit-level=high` over -# `v2/main`'s lockfiles would close it fully and is a separable follow-up. +# avoids that. The release-time `npm audit --audit-level=high` report from +# #2231 is a second signal that partially covers it — at release time only; +# running that same report over `v2/main`'s lockfiles on a schedule would +# close it fully and is a separable follow-up. # # `vulnerability-alerts: read` is the one non-default permission, and # `GITHUB_TOKEN` supports it — no PAT is needed to read the alerts themselves. diff --git a/AGENTS.md b/AGENTS.md index a5502051..ba08444f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -111,8 +111,8 @@ Four things about this that are not obvious from the code: - **Dependabot *alerts* stay on.** Alerts and security-update PRs are independent settings; only the PRs are off. Turning alerts off would blind the sweep that replaced them. - **The security half is a schedule, not an event handler**, because there is no `dependabot_alert` workflow trigger — it is a webhook event only. -- **Alerts are computed from the default branch (`main`), and we ship from `v2/main`.** So the sweep re-checks each alert's vulnerable range against `v2/main`'s own lockfile before filing, and skips one that is already fixed there. The converse is a real blind spot with no fix on this path: a vulnerable dependency introduced on `v2/main` and not yet merged to `main` produces **no alert at all**. The release-time `npm audit fix` (#2231) is the partial second signal. -- **`automated-security-fixes` can be re-enabled from the UI without a commit**, so nothing in the repo would record it. The sweep asserts it is still disabled and **fails loudly** if it is not; a red run of that workflow means the setting was flipped, not that the script broke. +- **Alerts are computed from the default branch (`main`), and we ship from `v2/main`.** So the sweep re-checks each alert's vulnerable range against `v2/main`'s own lockfile before filing, and skips one that is already fixed there. The converse is a real blind spot with no fix on this path: a vulnerable dependency introduced on `v2/main` and not yet merged to `main` produces **no alert at all**. The release-time `npm audit --audit-level=high` report (#2231) is the partial second signal — and only at release time. +- **`automated-security-fixes` can be re-enabled from the UI without a commit**, so nothing in the repo would record it. The sweep reads it back and **fails loudly on an explicit `enabled: true`**. ⚠️ It is a *conditional* guard, not an invariant: the endpoint needs `administration: read`, which `GITHUB_TOKEN` cannot be granted (`permissions:` has no such key), so under the default token the sweep logs **UNVERIFIED** and carries on rather than going red every day for an unrelated reason. Only a token carrying that scope makes it a real assertion. An issue filed by either sweep is an ordinary board item — `v2` + `chore` + `dependabot`, the current milestone, and a card on #28. A security issue lands at **Todo / High**: arriving through this pipeline *is* the approval, and `High` is a standing override of the [priority rubric](.claude/skills/issue-triage/SKILL.md), which would otherwise score a routine bump Medium. Board placement needs an org-project PAT that `GITHUB_TOKEN` cannot have, so it is **best-effort** — without the secret the issue is still created labeled and milestoned, and the next triage sweep boards it. diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index d56f5f65..8ca81e32 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -20,11 +20,12 @@ // alerts are not minute-sensitive. // 2. `GITHUB_TOKEN` can read alerts with `vulnerability-alerts: read`, so no // PAT is needed for the sweep itself. Two side steps DO need one, and both -// are best-effort rather than preconditions: writing the board card (an org -// project is outside `GITHUB_TOKEN`'s reach — an unboarded-but-milestoned -// issue is swept into Todo by the next `/issue-triage` pass), and reading -// back the `automated-security-fixes` setting (`administration: read`, -// which `permissions:` cannot grant at all). +// degrade rather than block: writing the board card (an org project is +// outside `GITHUB_TOKEN`'s reach — an unboarded-but-milestoned issue is +// swept into Todo by the next `/issue-triage` pass), and reading back the +// `automated-security-fixes` setting (`administration: read`, which +// `permissions:` cannot grant at all — so that guard reports UNVERIFIED +// rather than failing when the token cannot see it). // 3. Alerts are per-ADVISORY but a fix is per-BUMP. Today's seven open alerts // are three `overrides` entries, so grouping by // `(package, manifest_path, first_patched_version)` is what keeps this from @@ -36,7 +37,8 @@ // own lockfile before anything is filed. The blind spot that leaves is stated // plainly in the workflow header: a vulnerable dependency introduced on // `v2/main` and not yet merged to `main` produces no alert at all, and no -// approach that consumes GitHub's alerts can see it. +// approach that consumes GitHub's alerts can see it. The release-time +// `npm audit --audit-level=high` report (#2231) is the partial second signal. // // Idempotency key is the marker comment at the top of each issue body, which // names the package, the manifest and every GHSA the issue covers. A second run @@ -44,12 +46,12 @@ // has an open issue lands as a comment on it and rewrites the marker, rather // than filing a second issue. // -// The pure halves — `toSemverRange`, `lockfileVersions`, `isDirectDependency`, -// `groupAlerts`, `buildMarker`, `parseMarker`, `mergeGhsas`, `buildIssueTitle`, -// `buildIssueBody` and `buildNewAdvisoryComment` — are covered by -// `dependabot-alerts.test.mjs`. `main()` is the CLI entry point, exercised -// against the real repo only via `workflow_dispatch` in CI, per the same split -// `dependency-refresh.mjs` and `verify-skills.mjs` already use. +// Everything here is covered by `dependabot-alerts.test.mjs`: the pure halves +// directly, and `main()` through an injected spawn function, the same way +// `dependency-refresh.mjs` does it. `workflow_dispatch` is a production +// trigger, not a test, so the orchestration that handles API failures, lockfile +// filtering, issue idempotency and partial board writes is exercised here +// rather than left to a real run (Copilot). import { spawnSync } from "node:child_process"; import { readFileSync } from "node:fs"; @@ -78,23 +80,32 @@ export const BOARD_PRIORITY = "High"; export const TARGET_BRANCH = "v2/main"; const MARKER_RE = - /^/; + /^/; + +/** Marker on the comment that announces newly-seen advisories, keyed by GHSA. */ +const COMMENT_MARKER_RE = /^/; /** * The issue body's first line: the idempotency key. * - * @param {{package: string, manifestPath: string, ghsas: string[]}} group + * It carries `fixedIn` as well as the package and manifest because that triple + * IS the grouping key — two advisories on one package needing different patched + * versions are different bumps and get different issues. Keyed on the pair + * alone, a second bump would match the first issue and merge its GHSAs and its + * target version into the wrong one (Copilot). + * + * @param {{package: string, manifestPath: string, fixedIn: string, ghsas: string[]}} group * @returns {string} */ -export function buildMarker({ package: pkg, manifestPath, ghsas }) { - return ``; +export function buildMarker({ package: pkg, manifestPath, fixedIn, ghsas }) { + return ``; } /** * Read a marker back off an issue body. * * @param {string | undefined} body - * @returns {{package: string, manifestPath: string, ghsas: string[]} | null} + * @returns {{package: string, manifestPath: string, fixedIn: string, ghsas: string[]} | null} */ export function parseMarker(body) { const match = MARKER_RE.exec(body ?? ""); @@ -102,10 +113,23 @@ export function parseMarker(body) { return { package: match[1], manifestPath: match[2], - ghsas: match[3].split(",").filter(Boolean), + fixedIn: match[3], + ghsas: match[4].split(",").filter(Boolean), }; } +/** + * The GHSAs a previously-posted "new advisories" comment already announced. + * + * @param {string | undefined} body + * @returns {string[] | null} `null` when the comment carries no marker + */ +export function parseCommentMarker(body) { + const match = COMMENT_MARKER_RE.exec(body ?? ""); + if (!match) return null; + return match[1].split(",").filter(Boolean); +} + /** * Which of `group`'s advisories the existing issue does not already name. * @@ -284,7 +308,7 @@ export function buildIssueBody(group, { installed, direct, ghsas }) { .join("\n"); const fix = direct - ? `\`${group.package}\` is a **direct** dependency of \`${group.manifestPath.replace(/package-lock\.json$/, "package.json")}\` — bump its declared range to \`>=${group.fixedIn}\`.` + ? `\`${group.package}\` is a **direct** dependency of \`${group.manifestPath.replace(/package-lock\.json$/, "package.json")}\` — raise its declared range so it can no longer resolve below \`${group.fixedIn}\`, keeping the operator the manifest already uses. Widening it to a bare \`>=\` would drop the compatibility bound with it (Copilot).` : `\`${group.package}\` is **transitive**, so the fix is an [\`overrides\`](${PLACEMENT_DOC}) entry pinning it to \`${group.fixedIn}\` — **not** \`npm audit fix\`, which "resolves" an advisory with no upward escape by silently downgrading.`; return [ @@ -315,6 +339,16 @@ export function buildIssueBody(group, { installed, direct, ghsas }) { ].join("\n"); } +/** + * The marker that makes a "new advisories" comment idempotent on its own. + * + * @param {string[]} added + * @returns {string} + */ +export function buildCommentMarker(added) { + return ``; +} + /** * The comment a NEW advisory for an already-open issue gets, instead of a * second issue. @@ -332,6 +366,7 @@ export function buildNewAdvisoryComment(group, added) { ) .join("\n"); return [ + buildCommentMarker(added), `${added.length} new Dependabot ${added.length === 1 ? "advisory" : "advisories"} for \`${group.package}\`, cleared by the same bump to \`${group.fixedIn}\`. The issue body's marker now covers ${added.length === 1 ? "it" : "them"} too.`, "", "| GHSA | Severity | Summary |", @@ -341,24 +376,43 @@ export function buildNewAdvisoryComment(group, added) { } // --------------------------------------------------------------------------- -// Impure half: everything below shells out to `gh` or `git`. +// Impure half: everything below shells out to `gh`. Each takes its spawn +// function as a parameter, defaulted to `spawnSync`, so `main()` is testable +// with an injected fake rather than left to `workflow_dispatch` — the same +// shape `dependency-refresh.mjs` uses. // --------------------------------------------------------------------------- -function gh(args, { token } = {}) { +function gh(spawn, args, { token } = {}) { const env = token ? { ...process.env, GH_TOKEN: token } : process.env; - const result = spawnSync("gh", args, { encoding: "utf8", env }); + const result = spawn("gh", args, { encoding: "utf8", env }); if (result.error) throw result.error; return result; } -function ghJson(args) { - const result = gh(args); +function ghJson(spawn, args) { + const result = gh(spawn, args); if (result.status !== 0) { throw new Error(`gh ${args[0]} failed: ${(result.stderr ?? "").trim()}`); } return JSON.parse(result.stdout || "null"); } +/** + * Is this failed lookup the "the token may not read this" 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. + * + * @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); +} + /** * Detect whether Dependabot's security-update PRs have been switched back on. * @@ -370,22 +424,29 @@ function ghJson(args) { * ⚠️ **The endpoint needs `administration: read`, which `GITHUB_TOKEN` cannot * be granted** — `permissions:` has no such key. So with the default token the * call 403s, and treating that as failure would make every scheduled run red - * for a reason unrelated to the alerts. It is therefore reported and skipped: - * only an explicit `enabled: true` throws. Give `PROJECT_TOKEN` the extra - * `administration: read` scope and the guard becomes a real assertion; without - * it the sweep still does its job, it just cannot see that setting. + * for a reason unrelated to the alerts. An authorization-shaped failure is + * therefore reported and skipped; every other failure throws, and an explicit + * `enabled: true` throws. Give `PROJECT_TOKEN` the extra `administration: read` + * scope and the guard becomes a real assertion; without it the sweep still does + * its job, it just cannot see that setting. + * + * @returns {boolean} whether the setting was actually read */ -function checkSecurityPrsStillDisabled(repo) { - const result = gh(["api", `repos/${repo}/automated-security-fixes`], { +function checkSecurityPrsStillDisabled(repo, spawn) { + const result = gh(spawn, ["api", `repos/${repo}/automated-security-fixes`], { token: process.env.PROJECT_TOKEN, }); if (result.status !== 0) { + const stderr = (result.stderr ?? "").trim(); + if (!isPermissionDenied(stderr)) { + throw new Error(`automated-security-fixes lookup failed: ${stderr}`); + } console.log( - "dependabot-alerts: cannot read automated-security-fixes " + - `(${(result.stderr ?? "").trim()}) — the token lacks \`administration: read\`, ` + - "so whether Dependabot security PRs are still off is UNVERIFIED this run", + `dependabot-alerts: cannot read automated-security-fixes (${stderr}) — ` + + "the token lacks `administration: read`, so whether Dependabot security " + + "PRs are still off is UNVERIFIED this run", ); - return; + return false; } const state = JSON.parse(result.stdout || "{}"); if (state.enabled === true) { @@ -396,14 +457,25 @@ function checkSecurityPrsStillDisabled(repo) { `DELETE /repos/${repo}/automated-security-fixes) and re-run.`, ); } + return true; } -function openAlerts(repo) { - return ghJson([ +/** + * Every open Dependabot alert. + * + * ⚠️ `--slurp` is load-bearing. Without it `gh api --paginate` concatenates one + * top-level JSON array PER PAGE, which `JSON.parse` rejects outright the moment + * open alerts exceed the 100-per-page limit (Copilot). With it the pages arrive + * as an array of arrays, flattened here. + */ +function openAlerts(repo, spawn) { + const pages = ghJson(spawn, [ "api", "--paginate", + "--slurp", `repos/${repo}/dependabot/alerts?state=open&per_page=100`, ]); + return (pages ?? []).flat(); } /** @@ -419,25 +491,43 @@ function readManifest(manifestPath) { } } -function openDependabotIssues(repo) { - return ghJson([ - "issue", - "list", - "--repo", - repo, - "--state", - "open", - "--label", - "dependabot", - "--json", - "number,body", - "--limit", - "100", - ]); +function openDependabotIssues(repo, spawn) { + return ( + ghJson(spawn, [ + "issue", + "list", + "--repo", + repo, + "--state", + "open", + "--label", + "dependabot", + "--json", + "number,body", + "--limit", + "100", + ]) ?? [] + ); } -function currentMilestone(repo) { - const result = gh([ +/** The GHSA sets already announced by comments on an issue. */ +function announcedAdvisories(repo, number, spawn) { + const issue = ghJson(spawn, [ + "issue", + "view", + String(number), + "--repo", + repo, + "--json", + "comments", + ]); + return (issue?.comments ?? []) + .map((comment) => parseCommentMarker(comment.body)) + .filter(Boolean); +} + +function currentMilestone(repo, spawn) { + const result = gh(spawn, [ "api", `repos/${repo}/milestones`, "--jq", @@ -456,8 +546,9 @@ function currentMilestone(repo) { * looking them up each run is what keeps an unrelated board edit from turning * into a silently mis-set field here. */ -function optionId(fieldName, optionName, token) { +function optionId(fieldName, optionName, token, spawn) { const result = gh( + spawn, [ "project", "field-list", @@ -487,24 +578,36 @@ function optionId(fieldName, optionName, token) { /** * Put the issue on board #28 at Todo / High. * - * Best-effort by design: an org project is outside `GITHUB_TOKEN`'s reach, so - * this needs a PAT the workflow may not have. A failure here is logged and the - * run continues — the issue is already labeled and milestoned, which is enough - * for the next `/issue-triage` sweep to board it (its documented exception - * moves an unboarded-but-milestoned issue straight into Todo). + * Failing to add the card at all is benign — the issue is already labeled and + * milestoned, which is enough for the next `/issue-triage` sweep to board it + * (its documented exception moves an unboarded-but-milestoned issue straight + * into Todo). That is why an org-project PAT is an optimization here rather + * than a prerequisite. + * + * ⚠️ **Failing PART WAY through is not benign**, and the two cases must not be + * reported the same way (Copilot). Once `item-add` succeeds the issue IS + * boarded, so no later triage sweep will look at it — a failed field edit + * leaves a card sitting on the board with no Status or no Priority, in exactly + * the state nothing else will fix. So a partial placement is returned to the + * caller, which finishes every remaining group and then fails the run. * * Todo rather than Incoming: arriving through this pipeline IS the approval. + * + * @returns {string | null} a description of a PARTIAL placement, else `null` */ -function addToBoard(issueUrl) { +function addToBoard(issueUrl, spawn) { const token = process.env.PROJECT_TOKEN; if (!token) { console.log( "dependabot-alerts: PROJECT_TOKEN unset — issue left unboarded for the next triage sweep", ); - return; + return null; } + + let itemId; try { const added = gh( + spawn, [ "project", "item-add", @@ -518,16 +621,24 @@ function addToBoard(issueUrl) { ], { token }, ); - if (added.status !== 0) { - throw new Error((added.stderr ?? "").trim()); - } - const itemId = JSON.parse(added.stdout).id; - // Each item-edit sets exactly one field, so Status and Priority are two calls. - for (const [fieldId, fieldName, optionName] of [ - [STATUS_FIELD_ID, "Status", BOARD_STATUS], - [PRIORITY_FIELD_ID, "Priority", BOARD_PRIORITY], - ]) { + if (added.status !== 0) throw new Error((added.stderr ?? "").trim()); + itemId = JSON.parse(added.stdout).id; + } catch (error) { + // Nothing was added, so the issue is simply unboarded — recoverable. + console.log( + `dependabot-alerts: board add failed (${error.message}) — issue is labeled and milestoned, next triage sweep will board it`, + ); + return null; + } + + // Each item-edit sets exactly one field, so Status and Priority are two calls. + for (const [fieldId, fieldName, optionName] of [ + [STATUS_FIELD_ID, "Status", BOARD_STATUS], + [PRIORITY_FIELD_ID, "Priority", BOARD_PRIORITY], + ]) { + try { const edit = gh( + spawn, [ "project", "item-edit", @@ -538,24 +649,27 @@ function addToBoard(issueUrl) { "--field-id", fieldId, "--single-select-option-id", - optionId(fieldName, optionName, token), + optionId(fieldName, optionName, token, spawn), ], { token }, ); if (edit.status !== 0) throw new Error((edit.stderr ?? "").trim()); + } catch (error) { + return `${issueUrl} is on board #28 but its ${fieldName} was not set (${error.message}) — no triage sweep will fix this, set it by hand`; } - console.log( - `dependabot-alerts: boarded ${issueUrl} at ${BOARD_STATUS}/${BOARD_PRIORITY}`, - ); - } catch (error) { - console.log( - `dependabot-alerts: board write failed (${error.message}) — issue is labeled and milestoned, next triage sweep will board it`, - ); } + + console.log( + `dependabot-alerts: boarded ${issueUrl} at ${BOARD_STATUS}/${BOARD_PRIORITY}`, + ); + return null; } -function createIssue(repo, group, body) { - const milestone = currentMilestone(repo); +/** + * @returns {{url: string, milestone: string | null}} + */ +function createIssue(repo, group, body, spawn) { + const milestone = currentMilestone(repo, spawn); const args = [ "issue", "create", @@ -573,37 +687,34 @@ function createIssue(repo, group, body) { body, ]; if (milestone) args.push("--milestone", milestone); - const result = gh(args); + const result = gh(spawn, args); if (result.status !== 0) { throw new Error(`gh issue create failed: ${(result.stderr ?? "").trim()}`); } const url = result.stdout.trim(); - if (!milestone) { - console.log( - "dependabot-alerts: no open milestone — issue filed unmilestoned", - ); - } console.log(`dependabot-alerts: filed ${url}`); - return url; + return { url, milestone }; } -export function main(repo = process.env.GITHUB_REPOSITORY) { +export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { if (!repo) throw new Error("repo not specified (GITHUB_REPOSITORY unset)"); - checkSecurityPrsStillDisabled(repo); + checkSecurityPrsStillDisabled(repo, spawn); - const groups = groupAlerts(openAlerts(repo)); + const groups = groupAlerts(openAlerts(repo, spawn)); if (groups.length === 0) { console.log("dependabot-alerts: no open alerts — no-op"); return; } - const existingIssues = openDependabotIssues(repo).map((issue) => ({ + const existingIssues = openDependabotIssues(repo, spawn).map((issue) => ({ ...issue, marker: parseMarker(issue.body), })); const manifests = new Map(); + const boardProblems = []; + for (const group of groups) { if (!manifests.has(group.manifestPath)) { manifests.set(group.manifestPath, readManifest(group.manifestPath)); @@ -630,19 +741,34 @@ export function main(repo = process.env.GITHUB_REPOSITORY) { } const direct = isDirectDependency(lock, group.package); + // Matched on the full grouping key, `fixedIn` included: a second bump of + // the same package is a different issue, not an update to this one. const existing = existingIssues.find( (i) => i.marker?.package === group.package && - i.marker?.manifestPath === group.manifestPath, + i.marker?.manifestPath === group.manifestPath && + i.marker?.fixedIn === group.fixedIn, ); if (!existing) { - const url = createIssue( + const { url, milestone } = createIssue( repo, group, buildIssueBody(group, { installed: affected, direct }), + spawn, ); - addToBoard(url); + // `Incoming` <=> no milestone, everything past it <=> milestoned. With no + // open milestone to assign there is nothing to put the card past Incoming + // WITH, so boarding it at Todo would assert an approval the invariant + // reads off the milestone (Copilot). Leave it for triage instead. + if (!milestone) { + console.log( + "dependabot-alerts: no open milestone — issue filed unmilestoned and unboarded, next triage sweep places it", + ); + continue; + } + const problem = addToBoard(url, spawn); + if (problem) boardProblems.push(problem); continue; } @@ -654,7 +780,31 @@ export function main(repo = process.env.GITHUB_REPOSITORY) { continue; } - const edit = gh([ + // ⚠️ Comment FIRST, then rewrite the marker. The marker is the idempotency + // key, so editing it first and failing on the comment would make the next + // run take the no-op branch above and skip the comment permanently + // (Copilot). In this order the worst case is a repeat, and the comment's + // own marker rules that out too. + const alreadyAnnounced = announcedAdvisories(repo, existing.number, spawn); + const marker = buildCommentMarker(added); + if (!alreadyAnnounced.some((set) => buildCommentMarker(set) === marker)) { + const comment = gh(spawn, [ + "issue", + "comment", + String(existing.number), + "--repo", + repo, + "--body", + buildNewAdvisoryComment(group, added), + ]); + if (comment.status !== 0) { + throw new Error( + `gh issue comment failed: ${(comment.stderr ?? "").trim()}`, + ); + } + } + + const edit = gh(spawn, [ "issue", "edit", String(existing.number), @@ -666,24 +816,18 @@ export function main(repo = process.env.GITHUB_REPOSITORY) { if (edit.status !== 0) { throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`); } - const comment = gh([ - "issue", - "comment", - String(existing.number), - "--repo", - repo, - "--body", - buildNewAdvisoryComment(group, added), - ]); - if (comment.status !== 0) { - throw new Error( - `gh issue comment failed: ${(comment.stderr ?? "").trim()}`, - ); - } console.log( `dependabot-alerts: added ${added.join(", ")} to #${existing.number}`, ); } + + // Every group is processed before this throws: a half-placed card is worth + // failing the run over, but not at the cost of the issues still unfiled. + if (boardProblems.length > 0) { + throw new Error( + `dependabot-alerts: incomplete board placement —\n ${boardProblems.join("\n ")}`, + ); + } } if (import.meta.url === `file://${process.argv[1]}`) { diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 1bc20371..a37faf6a 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -5,15 +5,22 @@ import { test } from "node:test"; import assert from "node:assert/strict"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { tmpdir } from "node:os"; import { + buildCommentMarker, buildIssueBody, buildIssueTitle, buildMarker, buildNewAdvisoryComment, groupAlerts, isDirectDependency, + isPermissionDenied, lockfileVersions, + main, mergeGhsas, + parseCommentMarker, parseMarker, toSemverRange, } from "./dependabot-alerts.mjs"; @@ -165,26 +172,57 @@ test("buildMarker and parseMarker round-trip, sorting the GHSA list", () => { const marker = buildMarker({ package: "fast-uri", manifestPath: "package-lock.json", + fixedIn: "3.1.6", ghsas: ["GHSA-b", "GHSA-a"], }); assert.equal( marker, - "", + "", ); assert.deepEqual(parseMarker(`${marker}\nbody text`), { package: "fast-uri", manifestPath: "package-lock.json", + fixedIn: "3.1.6", ghsas: ["GHSA-a", "GHSA-b"], }); }); +test("the marker carries fixedIn, so two bumps of one package stay distinct", () => { + const [a] = groupAlerts([alert({ ghsa: "GHSA-a", fixed: "3.1.6" })]); + const [b] = groupAlerts([alert({ ghsa: "GHSA-b", fixed: "4.0.0" })]); + assert.notEqual(buildMarker(a), buildMarker(b)); + assert.equal(parseMarker(buildMarker(a)).fixedIn, "3.1.6"); + assert.equal(parseMarker(buildMarker(b)).fixedIn, "4.0.0"); +}); + +test("buildCommentMarker and parseCommentMarker round-trip", () => { + const marker = buildCommentMarker(["GHSA-b", "GHSA-a"]); + assert.equal(marker, ""); + assert.deepEqual(parseCommentMarker(`${marker}\ntext`), ["GHSA-a", "GHSA-b"]); + assert.equal(parseCommentMarker("an ordinary comment"), null); +}); + +test("isPermissionDenied separates an authorization failure from a real one", () => { + assert.equal( + isPermissionDenied("gh: HTTP 403: Resource not accessible"), + true, + ); + assert.equal(isPermissionDenied("gh: HTTP 401: Bad credentials"), true); + assert.equal(isPermissionDenied("gh: HTTP 404: Not Found"), true); + 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", () => { assert.equal(parseMarker(undefined), null); assert.equal(parseMarker("just an issue someone wrote"), null); // The marker is the FIRST line or it is not the idempotency key. assert.equal( parseMarker( - "preamble\n", + "preamble\n", ), null, ); @@ -233,14 +271,16 @@ test("buildIssueBody leads with the marker and asks for an overrides pin when tr assert.match(body, /GHSA-a/); assert.match(body, /CVE-2026-1/); assert.match(body, /`overrides`/); - assert.doesNotMatch(body, /bump its declared range/); + assert.doesNotMatch(body, /raise its declared range/); }); -test("buildIssueBody asks for a plain range bump when the dependency is direct", () => { +test("buildIssueBody asks a direct dependency's range to be raised, not widened", () => { const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); const body = buildIssueBody(group, { installed: ["3.1.5"], direct: true }); assert.match(body, /\*\*direct\*\* dependency of `package\.json`/); - assert.match(body, /bump its declared range to `>=3\.1\.6`/); + assert.match(body, /can no longer resolve below `3\.1\.6`/); + // Prescribing `>=3.1.6` would throw away the manifest's compatibility bound. + assert.doesNotMatch(body, /range to `>=/); }); test("buildIssueBody honors an overridden GHSA list when rewriting an issue", () => { @@ -263,3 +303,393 @@ test("buildNewAdvisoryComment lists only the newly-seen advisories", () => { assert.match(comment, /GHSA-b/); assert.doesNotMatch(comment, /GHSA-a/); }); + +// --------------------------------------------------------------------------- +// main() orchestration, driven through the injected spawn function. +// +// `workflow_dispatch` is a production trigger, not a test — and everything that +// can go wrong here goes wrong in production only: a paginated alert feed, a +// manifest that has moved on, a half-written board card, a comment posted twice +// (Copilot). Manifests are read from the working tree, so each test runs in a +// temp directory it populates itself. +// --------------------------------------------------------------------------- + +/** + * A `spawnSync` stand-in. `gh` responses are matched on the argument list, in + * the order the handlers are declared, and every call is recorded. + */ +function fakeSpawn({ + securityFixes = { enabled: false, paused: false }, + securityFixesStatus = 0, + securityFixesStderr = "", + alertPages = [[]], + issues = [], + comments = [], + milestone = "v2.6.0", + boardEditStatus = 0, +} = {}) { + const calls = []; + const ok = (stdout = "") => ({ status: 0, stdout, stderr: "" }); + + const spawn = (cmd, args, opts) => { + calls.push({ cmd, args, opts }); + const joined = args.join(" "); + + if (joined.includes("automated-security-fixes")) { + return securityFixesStatus === 0 + ? ok(JSON.stringify(securityFixes)) + : { + status: securityFixesStatus, + stdout: "", + stderr: securityFixesStderr, + }; + } + if (joined.includes("dependabot/alerts")) { + // `--slurp` yields one array PER PAGE; main() must flatten them. + return ok(JSON.stringify(alertPages)); + } + if (joined.includes("milestones")) return ok(milestone); + if (args[0] === "issue" && args[1] === "list") + return ok(JSON.stringify(issues)); + if (args[0] === "issue" && args[1] === "view") + return ok(JSON.stringify({ comments })); + if (args[0] === "issue" && args[1] === "create") + return ok("https://github.com/o/r/issues/77"); + if (args[0] === "project" && args[1] === "item-add") + return ok(JSON.stringify({ id: "PVTI_fake" })); + if (args[0] === "project" && args[1] === "field-list") + return ok( + JSON.stringify({ + fields: [ + { name: "Status", options: [{ name: "Todo", id: "todo-id" }] }, + { name: "Priority", options: [{ name: "High", id: "high-id" }] }, + ], + }), + ); + if (args[0] === "project" && args[1] === "item-edit") + return boardEditStatus === 0 + ? ok() + : { status: boardEditStatus, stdout: "", stderr: "field edit blew up" }; + return ok(); + }; + spawn.calls = calls; + return spawn; +} + +const ghCall = (spawn, verb) => + spawn.calls.find( + (c) => c.cmd === "gh" && c.args[0] === "issue" && c.args[1] === verb, + ); +const ghCalls = (spawn, verb) => + spawn.calls.filter( + (c) => c.cmd === "gh" && c.args[0] === "issue" && c.args[1] === verb, + ); + +function captureLog(run) { + const lines = []; + const original = console.log; + console.log = (...a) => lines.push(a.join(" ")); + try { + run(); + } finally { + console.log = original; + } + return lines; +} + +/** Run `body` in a temp cwd populated with `files` (path -> JSON value). */ +function inTempRepo(files, body) { + const dir = mkdtempSync(join(tmpdir(), "dependabot-alerts-")); + const cwd = process.cwd(); + try { + for (const [path, value] of Object.entries(files)) { + const full = join(dir, path); + mkdirSync(dirname(full), { recursive: true }); + writeFileSync(full, JSON.stringify(value)); + } + process.chdir(dir); + return body(); + } finally { + process.chdir(cwd); + rmSync(dir, { recursive: true, force: true }); + } +} + +/** A lockfile holding one transitive copy of `pkg` at `version`. */ +const lockWith = (pkg, version) => ({ + lockfileVersion: 3, + packages: { "": { dependencies: {} }, [`node_modules/${pkg}`]: { version } }, +}); + +/** Without a PAT the board is never touched, which most tests want. */ +function withoutProjectToken(body) { + const saved = process.env.PROJECT_TOKEN; + delete process.env.PROJECT_TOKEN; + try { + return body(); + } finally { + if (saved !== undefined) process.env.PROJECT_TOKEN = saved; + } +} + +function withProjectToken(body) { + const saved = process.env.PROJECT_TOKEN; + process.env.PROJECT_TOKEN = "pat"; + try { + return body(); + } finally { + if (saved === undefined) delete process.env.PROJECT_TOKEN; + else process.env.PROJECT_TOKEN = saved; + } +} + +test("main files one issue per bump, flattening a paginated alert feed", () => { + // Two pages, as `--slurp` returns them: a single JSON.parse of concatenated + // pages would have thrown before this ever reached grouping. + const spawn = fakeSpawn({ + alertPages: [ + [ + alert({ ghsa: "GHSA-a", range: ">= 3.1.3, < 3.1.6" }), + alert({ ghsa: "GHSA-b" }), + ], + [ + alert({ + ghsa: "GHSA-c", + pkg: "browserslist", + manifest: "clients/tui/package-lock.json", + fixed: "4.28.7", + range: "<= 4.28.6", + }), + ], + ], + }); + + const log = inTempRepo( + { + "package-lock.json": lockWith("fast-uri", "3.1.5"), + "clients/tui/package-lock.json": lockWith("browserslist", "4.28.2"), + }, + () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + + const created = ghCalls(spawn, "create"); + assert.equal(created.length, 2, "3 advisories, 2 bumps, 2 issues"); + const titles = created.map((c) => c.args[c.args.indexOf("--title") + 1]); + assert.ok(titles.some((t) => t.includes("`fast-uri` to `3.1.6`"))); + assert.ok(titles.some((t) => t.includes("`browserslist` to `4.28.7`"))); + // The fast-uri issue names both of its advisories. + const fastUri = created.find((c) => + c.args[c.args.indexOf("--title") + 1].includes("fast-uri"), + ); + const body = fastUri.args[fastUri.args.indexOf("--body") + 1]; + assert.deepEqual(parseMarker(body).ghsas, ["GHSA-a", "GHSA-b"]); + assert.deepEqual(created[0].args.slice(-2), ["--milestone", "v2.6.0"]); + assert.ok(log.some((l) => l.includes("filed"))); +}); + +test("main skips an alert already out of range on the checked-out branch", () => { + const spawn = fakeSpawn({ alertPages: [[alert({ ghsa: "GHSA-a" })]] }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.6") }, + () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.equal(ghCall(spawn, "create"), undefined); + assert.ok(log.some((l) => l.includes("already out of range"))); +}); + +test("main skips an alert whose manifest is absent from the checkout", () => { + const spawn = fakeSpawn({ + alertPages: [ + [alert({ ghsa: "GHSA-a", manifest: "gone/package-lock.json" })], + ], + }); + const log = inTempRepo({}, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.equal(ghCall(spawn, "create"), undefined); + assert.ok(log.some((l) => l.includes("absent on v2/main"))); +}); + +test("main is a complete no-op on a second run", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + issues: [ + { + number: 41, + body: buildIssueBody(group, { installed: ["3.1.5"], direct: false }), + }, + ], + }); + 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.equal(ghCall(spawn, "comment"), undefined); + assert.equal(ghCall(spawn, "edit"), undefined); + assert.ok(log.some((l) => l.includes("#41 already covers"))); +}); + +test("main will not update an issue whose bump differs, even for the same package", () => { + // The marker's `fixed=` is what keeps a 4.0.0 bump off the 3.1.6 issue. + const [old] = groupAlerts([alert({ ghsa: "GHSA-a", fixed: "3.1.6" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-b", fixed: "4.0.0", range: "< 4.0.0" })]], + issues: [ + { + number: 41, + body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + }, + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.ok(ghCall(spawn, "create"), "a different bump gets its own issue"); + assert.equal(ghCall(spawn, "edit"), undefined); +}); + +test("main comments a new advisory BEFORE rewriting the marker", () => { + const [old] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" }), alert({ ghsa: "GHSA-b" })]], + issues: [ + { + number: 41, + body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + }, + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + + const order = spawn.calls + .filter( + (c) => c.args[0] === "issue" && ["comment", "edit"].includes(c.args[1]), + ) + .map((c) => c.args[1]); + // Marker-first would let a failed comment be skipped forever by the no-op branch. + assert.deepEqual(order, ["comment", "edit"]); + + const comment = ghCall(spawn, "comment"); + const text = comment.args[comment.args.indexOf("--body") + 1]; + assert.deepEqual(parseCommentMarker(text), ["GHSA-b"]); + assert.ok(!text.includes("GHSA-a"), "only the newly-seen advisory"); + + const edit = ghCall(spawn, "edit"); + assert.deepEqual( + parseMarker(edit.args[edit.args.indexOf("--body") + 1]).ghsas, + ["GHSA-a", "GHSA-b"], + ); +}); + +test("main does not repeat a comment it already posted", () => { + const [old] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" }), alert({ ghsa: "GHSA-b" })]], + issues: [ + { + number: 41, + body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + }, + ], + comments: [{ body: `${buildCommentMarker(["GHSA-b"])}\nsaid already` }], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.equal(ghCall(spawn, "comment"), undefined); + // The marker still gets brought up to date. + assert.ok(ghCall(spawn, "edit")); +}); + +test("main fails loudly when Dependabot security PRs are back on", () => { + const spawn = fakeSpawn({ securityFixes: { enabled: true, paused: false } }); + assert.throws( + () => captureLog(() => main("o/r", spawn)), + /security-update PRs are ENABLED again/, + ); + assert.equal(spawn.calls.length, 1, "nothing else runs"); +}); + +test("main continues, reporting UNVERIFIED, when the token cannot read the setting", () => { + const spawn = fakeSpawn({ + securityFixesStatus: 1, + securityFixesStderr: "gh: HTTP 403: Resource not accessible by integration", + alertPages: [[]], + }); + const log = captureLog(() => main("o/r", spawn)); + assert.ok(log.some((l) => l.includes("UNVERIFIED"))); + assert.ok(log.some((l) => l.includes("no open alerts"))); +}); + +test("main throws when the setting lookup fails for a non-permission reason", () => { + const spawn = fakeSpawn({ + securityFixesStatus: 1, + securityFixesStderr: "gh: HTTP 502: Bad Gateway", + }); + // Swallowing this would exit green having skipped the sweep's own precondition. + assert.throws( + () => captureLog(() => main("o/r", spawn)), + /automated-security-fixes lookup failed.*502/s, + ); +}); + +test("main leaves an unmilestoned issue off the board for triage", () => { + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + milestone: "", + }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.5") }, + () => withProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.ok(ghCall(spawn, "create")); + // `Incoming` <=> no milestone: boarding it at Todo would assert an approval + // the invariant reads off the milestone. + assert.equal( + spawn.calls.find( + (c) => c.args[0] === "project" && c.args[1] === "item-add", + ), + undefined, + ); + assert.ok(log.some((l) => l.includes("unmilestoned and unboarded"))); +}); + +test("main fails the run when a card is added but its fields are not set", () => { + const spawn = fakeSpawn({ + alertPages: [ + [ + alert({ ghsa: "GHSA-a" }), + alert({ + ghsa: "GHSA-b", + pkg: "qs", + fixed: "6.16.0", + range: "< 6.16.0", + }), + ], + ], + boardEditStatus: 1, + }); + assert.throws( + () => + inTempRepo( + { + "package-lock.json": { + lockfileVersion: 3, + packages: { + "": { dependencies: {} }, + "node_modules/fast-uri": { version: "3.1.5" }, + "node_modules/qs": { version: "6.15.3" }, + }, + }, + }, + () => withProjectToken(() => captureLog(() => main("o/r", spawn))), + ), + /incomplete board placement/, + ); + // A half-placed card is worth failing over — but not before both issues exist. + assert.equal(ghCalls(spawn, "create").length, 2); +}); diff --git a/scripts/dependency-refresh.mjs b/scripts/dependency-refresh.mjs index 4ce6799e..348e4236 100644 --- a/scripts/dependency-refresh.mjs +++ b/scripts/dependency-refresh.mjs @@ -191,7 +191,7 @@ export function buildIssueBody(installs, actions = []) { return [ ISSUE_MARKER, - "Routine dependency refresh — `npm outdated` plus a workflow `uses:` check, run against `v2/main` on a monthly schedule. This sweep replaces Dependabot's version-update PRs (#2229, #2235); Dependabot security updates are a separate mechanism and remain enabled.", + "Routine dependency refresh — `npm outdated` plus a workflow `uses:` check, run against `v2/main` on a monthly schedule. This sweep replaces Dependabot's version-update PRs (#2229, #2235); its security-update PRs are off too (#2233), and the alerts they used to act on are swept into their own issues daily.", "", "This is a tracking issue, not a diff: pick what's worth bumping (`wanted` is the safe default; `latest` may cross a major and needs its own judgment call, especially for anything root-declared per [Dependency placement](https://github.com/modelcontextprotocol/inspector/blob/v2/main/AGENTS.md#dependency-placement)) and open a normal PR against `v2/main`.", "",