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

chore(scripts): scan only executable positions, and match on the expression prefix (#2146)

Browse source 
Copilot, second review — both findings were real:

- A `${{ … }}` body can itself contain braces, so the `[^}]*\}\}` arm added in
  the previous round stopped at the first `}` and missed
  `smoke:web:${{ format('{0}', matrix.browser) }}` — the very case it was
  added for. It now keys off the `${{` prefix, which nesting cannot defeat.

- Scanning every non-comment line made ordinary metadata a finding:
  `name: Explain why smoke:web:firefox stays local` executes nothing and was
  reported as an engine invocation, contradicting the header's "what it
  forbids, and nothing more". The scan now walks executable positions only —
  `run:` scalars (inline and block) and `env:`/`with:` values — via an
  exported `extractExecutableRegions`, hand-rolled because `test:scripts` is
  dependency-free by design. The narrowing is a stated limit, not a hidden
  one: an execution vector outside those three positions is not seen.

Verified by injecting a nested-brace SMOKE_BROWSER expression into the real
main.yml — the suite fails, and goes green again when removed.

Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-27 00:35:47 -04:00
commit 398d074ad9
2 changed files with 156 additions and 19 deletions
Download patch file Download diff file Expand all files Collapse all files

103
scripts/lib/workflow-gate.mjs
View file

@ -48,11 +48,21 @@
* Those belong in CI and are there today — `smoke:tui` self-skips under
* `process.env.CI` on its own, so it needs no guarding.
*
* COMMENTS ARE NOT SCANNED. A line whose first non-whitespace character is `#`
* is skipped, whether it is a YAML comment or a shell comment inside a `run:`
* block. A comment cannot invoke anything, and the workflow's value is largely
* in its prose — a guard that made the two tiers undocumentable in the file
* that implements one of them would be traded for the wrong thing.
* ONLY EXECUTABLE POSITIONS ARE SCANNED — `run:` scalars (inline and block),
* and the values of an `env:` or `with:` mapping. Everything else in a workflow
* is metadata that runs nothing, so scanning it produces findings that are
* simply false: `name: Explain why smoke:web:firefox stays local` executes
* nothing, and failing the suite on it would contradict the "what it forbids,
* and nothing more" contract above (Copilot). Comments are skipped for the same
* reason and one more: the workflow's value is largely in its prose, and a
* guard that made the two tiers undocumentable in the file implementing one of
* them would be traded for the wrong thing.
*
* That is a real limit, stated rather than hidden: an execution vector outside
* those three positions — a custom action that reads a script name from
* somewhere else, say — is not seen. The alternative, scanning everything, was
* measured against this repo's own workflow and produces false positives on
* ordinary step names, which is the faster way to get a guard deleted.
*/
import { SUPPORTED_BROWSERS, DEFAULT_BROWSER } from "./headless-browser.mjs";
@ -81,10 +91,24 @@ export const VIOLATION = {
// The name may be spelled out (`local:gate`) or built from an expression
// (`local:${{ matrix.task }}`) — both are `local:` scripts, and neither may be
// invoked from a workflow.
const LOCAL_SCRIPT_RE = /\blocal:(?:[a-z0-9][a-z0-9:-]*|\$\{\{[^}]*\}\})/gi;
//
// The expression arm keys off the `${{` PREFIX and then runs to the end of the
// line, rather than trying to find the matching `}}`. An expression body can
// itself contain braces — `${{ format('{0}', matrix.browser) }}` — so a
// `[^}]*\}\}` body stops at the first `}` and misses exactly the case worth
// catching (Copilot). Matching the prefix cannot be defeated by nesting, and
// since these are display strings the greedy tail costs nothing.
const EXPRESSION = String.raw`\$\{\{[^\n]*`;
const LOCAL_SCRIPT_RE = new RegExp(
String.raw`\blocal:(?:[a-z0-9][a-z0-9:-]*|${EXPRESSION})`,
"gi",
);
// Same reasoning one level down: `smoke:web:` followed by an expression could
// resolve to any engine, so it is read as a forbidden one.
const DYNAMIC_ENGINE_RE = /\bsmoke:web:\$\{\{[^}]*\}\}/gi;
const DYNAMIC_ENGINE_RE = new RegExp(
String.raw`\bsmoke:web:${EXPRESSION}`,
"gi",
);
// The value may be quoted (`SMOKE_BROWSER: "firefox"`), assigned
// (`SMOKE_BROWSER=firefox`), or an expression — capture whatever follows.
const BROWSER_ENV_RE = /\bSMOKE_BROWSER\s*[:=]\s*(\S+)/gi;
@ -93,6 +117,61 @@ function isComment(line) {
return line.trimStart().startsWith("#");
}
function indentOf(line) {
return line.length - line.trimStart().length;
}
/** `run:` with an inline command, optionally as the first key of a `- ` item. */
const RUN_INLINE_RE = /^\s*(?:-\s+)?run:\s*(.*)$/;
/** A key that opens a block whose nested lines are executable: `run: |`, `env:`, `with:`. */
const BLOCK_OPEN_RE = /^\s*(?:-\s+)?(run|env|with):\s*(?:[|>][-+]?\d*)?\s*$/;
/**
* The executable scalars of a workflow, with the line each came from.
*
* Deliberately hand-rolled rather than YAML-parsed: the guard runs in
* `test:scripts`, which is `node --test` with no dependencies by design, and
* the three shapes it needs are simple enough to recognize by indentation.
*
* @param {string} text
* @returns {Array<{line: number, kind: string, text: string}>}
*/
export function extractExecutableRegions(text) {
const regions = [];
let blockKind = null;
let blockIndent = 0;
text.split(/\r?\n/).forEach((line, index) => {
if (isComment(line)) return;
const lineNumber = index + 1;
if (blockKind !== null) {
// A blank line neither ends a block scalar nor carries anything.
if (line.trim() === "") return;
if (indentOf(line) > blockIndent) {
regions.push({ line: lineNumber, kind: blockKind, text: line });
return;
}
// Dedented out of the block — fall through and re-read this line as a key.
blockKind = null;
}
const block = BLOCK_OPEN_RE.exec(line);
if (block) {
blockKind = block[1];
blockIndent = indentOf(line);
return;
}
const run = RUN_INLINE_RE.exec(line);
if (run && run[1].trim() !== "") {
regions.push({ line: lineNumber, kind: "run", text: run[1] });
}
});
return regions;
}
function stripQuotes(value) {
const trimmed = value.trim().replace(/[,;]+$/, "");
const quoted = /^(["'])(.*)\1$/.exec(trimmed);
@ -111,12 +190,10 @@ function stripQuotes(value) {
*/
export function findWorkflowViolations(text, file = "<workflow>") {
const findings = [];
const lines = text.split(/\r?\n/);
lines.forEach((line, index) => {
if (isComment(line)) return;
const lineNumber = index + 1;
for (const { line: lineNumber, text: line } of extractExecutableRegions(
text,
)) {
for (const match of line.matchAll(LOCAL_SCRIPT_RE)) {
findings.push({
file,
@ -174,7 +251,7 @@ export function findWorkflowViolations(text, file = "<workflow>") {
`checked statically. Leave it unset; the default is \`${DEFAULT_BROWSER}\`.`,
});
}
});
}
return findings;
}

66
scripts/lib/workflow-gate.test.mjs
View file

@ -19,6 +19,7 @@ import { describe, it } from "node:test";
import {
FORBIDDEN_ENGINE_SCRIPTS,
VIOLATION,
extractExecutableRegions,
findWorkflowViolations,
formatWorkflowViolations,
} from "./workflow-gate.mjs";
@ -72,7 +73,7 @@ describe("findWorkflowViolations", () => {
},
{
name: "flags a quoted SMOKE_BROWSER value",
text: ' SMOKE_BROWSER: "webkit"\n',
text: ' env:\n SMOKE_BROWSER: "webkit"\n',
rules: [VIOLATION.BROWSER_ENV],
},
{
@ -82,12 +83,12 @@ describe("findWorkflowViolations", () => {
},
{
name: "flags an expression, which cannot be checked statically",
text: " SMOKE_BROWSER: ${{ matrix.browser }}\n",
text: " env:\n SMOKE_BROWSER: ${{ matrix.browser }}\n",
rules: [VIOLATION.BROWSER_ENV],
},
{
name: "allows SMOKE_BROWSER set explicitly to chromium",
text: " SMOKE_BROWSER: chromium\n",
text: " env:\n SMOKE_BROWSER: chromium\n",
rules: [],
},
{
@ -125,6 +126,41 @@ describe("findWorkflowViolations", () => {
].join("\n"),
rules: [],
},
{
// Copilot, second review: an expression body can itself contain braces,
// so a brace-balanced regex stops at the first `}` and misses the one
// case worth catching. The rule keys off the `${{` prefix instead.
name: "flags an expression whose own body contains braces",
text: " - run: npm run smoke:web:${{ format('{0}', matrix.browser) }}\n",
rules: [VIOLATION.ENGINE_SCRIPT],
},
{
name: "flags a command inside a block scalar, not just an inline run",
text: " - run: |\n npm run local:gate\n",
rules: [VIOLATION.LOCAL_SCRIPT],
},
{
name: "flags a command passed as an action input",
text: " with:\n args: npm run local:gate\n",
rules: [VIOLATION.LOCAL_SCRIPT],
},
{
// Copilot, second review: metadata executes nothing, so a finding there
// is simply false — and the fastest way to get a guard deleted.
name: "does not read a step name as an invocation",
text: " - name: Explain why smoke:web:firefox stays local\n",
rules: [],
},
{
name: "does not read an `if:` condition as one either",
text: " if: github.event_name == 'push'\n",
rules: [],
},
{
name: "does not read a bare mapping line outside env/with/run",
text: " SMOKE_BROWSER-ish-doc: firefox\n",
rules: [],
},
{
name: "does not read a comment as an invocation",
text: " # smoke:web:firefox runs in npm run local:gate, never here\n",
@ -169,6 +205,30 @@ describe("findWorkflowViolations", () => {
});
});
describe("extractExecutableRegions", () => {
it("ends a block when the next line dedents, and re-reads that line", () => {
// The table cases cannot isolate this: a block that never closed would
// still produce the right findings for them, and would then read the whole
// rest of the file as executable.
const regions = extractExecutableRegions(
[
" env:",
" SMOKE_BROWSER: chromium",
" - name: smoke:web:firefox is local-only",
" - run: npm run smoke",
"",
].join("\n"),
);
assert.deepEqual(
regions.map((r) => [r.kind, r.text.trim()]),
[
["env", "SMOKE_BROWSER: chromium"],
["run", "npm run smoke"],
],
);
});
});
describe(".github/workflows", () => {
const files = readdirSync(workflowDir).filter((f) => /\.ya?ml$/.test(f));