From 61d6f074b8fb95a0cd77c1749d9c19618399d4d1 Mon Sep 17 00:00:00 2001 From: nadav-dvir Date: Thu, 15 Oct 2026 13:43:31 +0300 Subject: [PATCH 1/3] [patch] Run skill-governance hooks through a node launcher for Windows VS Code runs hook strings with PowerShell on Windows, which cannot parse the POSIX env prefix and $(date) in the old one-liner, so skills ran unchecked. - plugin/scripts/enforce-skill.mjs: computes the enforcement deadline, sets the npm fetch bounds, and spawns `npx @jfrog/agent-guard --enforce-skill --client vscode` (npx.cmd through cmd.exe on Windows). Exit 2 and stdout pass through; a spawn failure fails open. - hooks.json: UserPromptSubmit and PreToolUse (--prefer-offline) start the launcher. - validate-skill-governance.mjs: asserts the launcher wiring and the Windows spawn plan, and executes the real hook strings against a stub npx. Mirrors the same launcher in jfrog/claude-plugin. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- marketplace.json | 2 +- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/hooks.json | 4 +- plugin/scripts/enforce-skill.mjs | 127 ++++++++++++++++++++++ scripts/validate-skill-governance.mjs | 146 +++++++++++++++++--------- 6 files changed, 228 insertions(+), 55 deletions(-) create mode 100644 plugin/scripts/enforce-skill.mjs diff --git a/README.md b/README.md index 9423ba0..375ecb2 100644 --- a/README.md +++ b/README.md @@ -268,7 +268,7 @@ When a policy block carries a waiver scope, the block message shows the command - Set `JFROG_PLATFORM_URL` and `JFROG_ACCESS_TOKEN` (or configure the JFrog CLI — see [Authentication](#authentication)) and `JF_PROJECT` (the JFrog project the skill runs in). For an entitled account with credentials but no project, skills are **blocked** with a message telling you what to set; with no credentials at all they are **allowed**, per the table above. - **Node.js (≥ 20) with `npx` on your `PATH`** — the hook resolves the Agent Guard through `npx`. Without it, governed actions are allowed unchecked. -- **macOS and Linux only.** These hook commands are POSIX shell, and VS Code starts hooks with `cmd.exe` on Windows, where the command fails before the Agent Guard is reached — so skills run **unchecked** on Windows rather than being blocked. Windows support is tracked separately. +- **Windows, macOS, and Linux.** The hook command is `node` plus `scripts/enforce-skill.mjs`. VS Code runs that string with PowerShell on a normal Windows session, which cannot parse a POSIX one-liner, so the launcher computes the deadline and starts `npx` itself (`npx.cmd` through `cmd.exe` on Windows). Agent Guard still decides allow or deny. If `node` or `npx` cannot start, the skill is allowed. - **Unlike the MCP alignment hook, this one does not pin the Agent Guard version.** It always resolves the latest published release, so a governance fix reaches you without waiting for a plugin release. `JFROG_AGENT_GUARD_REPO` still redirects the registry. - **Coverage limits.** `github.copilot.chat.skillTool.enabled` is **off by default**, and with it off Copilot registers no `skill` tool — it instructs the model to read the matching `SKILL.md` with its `read_file` tool instead. That call reaches the `PreToolUse` hook and the Agent Guard governs it, so a default install is covered through `read_file` rather than through the skill tool. What is *not* covered: skills contributed by other extensions (`contributes.chatSkills`) are not searched, and neither are folders added through `chat.agentSkillsLocations`. A skill's content can also be reached without a governed surface — through a subagent, a terminal command, or a text search — and hooks are a VS Code Preview feature that can be disabled organization-side with no signal. An absence of blocks never by itself means a skill was allowed. - **Cost per call.** VS Code parses `matcher` for Claude Code compatibility but ignores its value, so the `PreToolUse` hook runs on *every* tool call, and `UserPromptSubmit` on every prompt. The prompt hook revalidates the Agent Guard against the registry; the tool hook reads that from cache, which is what keeps the per-call cost down. diff --git a/marketplace.json b/marketplace.json index 1749923..f841112 100644 --- a/marketplace.json +++ b/marketplace.json @@ -9,7 +9,7 @@ { "name": "jfrog", "description": "JFrog Platform integration with MCP, security skills, and supply-chain best practices", - "version": "1.1.3", + "version": "1.1.4", "license": "Apache-2.0", "source": "plugin", "categories": [ diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 181b541..57979d1 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "jfrog", "description": "JFrog Platform integration with MCP, security skills, and supply-chain best practices", - "version": "1.1.3", + "version": "1.1.4", "license": "Apache-2.0", "author": { "name": "JFrog", diff --git a/plugin/hooks/hooks.json b/plugin/hooks/hooks.json index 211b985..15f5abc 100644 --- a/plugin/hooks/hooks.json +++ b/plugin/hooks/hooks.json @@ -23,7 +23,7 @@ "hooks": [ { "type": "command", - "command": "npm_config_fetch_retries=0 npm_config_fetch_timeout=10000 JF_AGENT_GUARD_ENFORCE_DEADLINE=\"$(_JFAG_NOW=$(date +%s 2>/dev/null); echo ${_JFAG_NOW:+$((_JFAG_NOW + 25))})\" npx --yes --registry \"${JFROG_AGENT_GUARD_REPO:-https://releases.jfrog.io/artifactory/api/npm/coding-agents-npm/}\" @jfrog/agent-guard --enforce-skill --client vscode", + "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/enforce-skill.mjs\"", "timeout": 30, "statusMessage": "Checking this skill against your JFrog governance policy…" } @@ -35,7 +35,7 @@ "hooks": [ { "type": "command", - "command": "npm_config_fetch_retries=0 npm_config_fetch_timeout=10000 JF_AGENT_GUARD_ENFORCE_DEADLINE=\"$(_JFAG_NOW=$(date +%s 2>/dev/null); echo ${_JFAG_NOW:+$((_JFAG_NOW + 25))})\" npx --yes --prefer-offline --registry \"${JFROG_AGENT_GUARD_REPO:-https://releases.jfrog.io/artifactory/api/npm/coding-agents-npm/}\" @jfrog/agent-guard --enforce-skill --client vscode", + "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/enforce-skill.mjs\" --prefer-offline", "timeout": 30, "statusMessage": "Checking this skill against your JFrog governance policy…" } diff --git a/plugin/scripts/enforce-skill.mjs b/plugin/scripts/enforce-skill.mjs new file mode 100644 index 0000000..8bf3fd0 --- /dev/null +++ b/plugin/scripts/enforce-skill.mjs @@ -0,0 +1,127 @@ +#!/usr/bin/env node + +// Starts @jfrog/agent-guard --enforce-skill. The hook string in hooks.json is only +// `node` plus this file: VS Code runs that string with PowerShell on Windows, which +// cannot parse a POSIX env prefix or $(date). This process computes the deadline, +// then spawns npx. It does not read the verdict. Exit 2 and stdout pass through. +// A spawn failure exits 1 so the host fails open. + +import { spawn } from "node:child_process"; +import process from "node:process"; + +import { isMainEntry } from "../modules/core/entry.mjs"; + +export const RELEASES_REGISTRY = "https://releases.jfrog.io/artifactory/api/npm/coding-agents-npm/"; +const DEADLINE_SLACK_SEC = 25; + +/** + * @param {string} arg + * @returns {string} + */ +export function quoteWindowsArg(arg) { + const value = String(arg ?? ""); + if (/[\r\n]/.test(value)) { + throw new Error("Windows spawn arg must not contain CR/LF"); + } + const escaped = value.replace(/%/g, "%%").replace(/"/g, '""'); + return `"${escaped}"`; +} + +/** + * @param {NodeJS.ProcessEnv} env + * @returns {string} + */ +export function registryFrom(env) { + const raw = env.JFROG_AGENT_GUARD_REPO; + if (typeof raw === "string" && raw.trim() !== "") return raw; + return RELEASES_REGISTRY; +} + +/** + * Spawn plan for `npx --enforce-skill`. On Windows the command is npx.cmd under + * cmd.exe, not ComSpec, so a PowerShell ComSpec cannot parse the argument list. + * JFROG_AGENT_GUARD_VERSION is ignored: governance must stay on the latest release. + * + * @param {{ + * preferOffline?: boolean, + * env?: NodeJS.ProcessEnv, + * nowSec?: number, + * platform?: NodeJS.Platform, + * }} [opts] + */ +export function resolveEnforceSpawn(opts = {}) { + const env = opts.env ?? process.env; + const platform = opts.platform ?? process.platform; + const childEnv = { ...env }; + childEnv.npm_config_fetch_retries = "0"; + childEnv.npm_config_fetch_timeout = "10000"; + if (Number.isFinite(opts.nowSec)) { + childEnv.JF_AGENT_GUARD_ENFORCE_DEADLINE = String(Math.trunc(opts.nowSec) + DEADLINE_SLACK_SEC); + } else { + delete childEnv.JF_AGENT_GUARD_ENFORCE_DEADLINE; + } + + const args = ["--yes"]; + if (opts.preferOffline === true) args.push("--prefer-offline"); + args.push( + "--registry", + registryFrom(env), + "@jfrog/agent-guard", + "--enforce-skill", + "--client", + "vscode", + ); + + const windows = platform === "win32"; + return { + command: windows ? "npx.cmd" : "npx", + args: windows ? args.map(quoteWindowsArg) : args, + shell: windows ? "cmd.exe" : false, + env: childEnv, + }; +} + +function main() { + /** @type {ReturnType} */ + let plan; + try { + plan = resolveEnforceSpawn({ + preferOffline: process.argv.slice(2).includes("--prefer-offline"), + env: process.env, + nowSec: Math.floor(Date.now() / 1000), + platform: process.platform, + }); + } catch { + process.exit(1); + } + + /** @type {ReturnType} */ + let child; + try { + child = spawn(plan.command, plan.args, { + env: plan.env, + shell: plan.shell, + windowsHide: true, + stdio: ["pipe", "inherit", "inherit"], + }); + } catch { + process.exit(1); + } + + child.on("error", () => { + process.exit(1); + }); + // A fast exit (including a policy exit 2) can EPIPE the stdin pipe. That must + // not replace the child's status. + if (!child.stdin) process.exit(1); + child.stdin.on("error", () => {}); + process.stdin.pipe(child.stdin); + child.on("exit", (code, signal) => { + if (signal || code === null) process.exit(1); + process.exit(code); + }); +} + +if (isMainEntry(import.meta.url)) { + main(); +} diff --git a/scripts/validate-skill-governance.mjs b/scripts/validate-skill-governance.mjs index f9e69e8..40951ac 100644 --- a/scripts/validate-skill-governance.mjs +++ b/scripts/validate-skill-governance.mjs @@ -3,9 +3,12 @@ // Licensed under the Apache License, Version 2.0 // https://www.apache.org/licenses/LICENSE-2.0 -// Tests the skill-governance hook wiring. hooks.json invokes agent-guard DIRECTLY via npx — no -// plugin-side governance code at all — so this validator asserts the wiring's shape and then -// EXECUTES the real command string out of hooks.json against a stub agent-guard. +// Tests the skill-governance hook wiring. hooks.json starts a node launcher, which spawns +// agent-guard via npx. The launcher is not a policy engine: it only builds the argv, the +// deadline, and a spawn that Windows can run. VS Code executes the hook string with +// PowerShell when ComSpec is cmd.exe, so the string itself cannot be a POSIX one-liner. +// This validator asserts that shape, the Windows spawn plan, and then EXECUTES the real +// command string out of hooks.json against a stub agent-guard. // // Four properties carry the whole design: // @@ -29,13 +32,14 @@ import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, w import { tmpdir } from "node:os"; import path from "node:path"; import process from "node:process"; -import { fileURLToPath } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const pluginRoot = path.join(repoRoot, "plugin"); const hooks = JSON.parse(readFileSync(path.join(pluginRoot, "hooks", "hooks.json"), "utf8")); const RELEASES_REGISTRY = "https://releases.jfrog.io/artifactory/api/npm/coding-agents-npm/"; +const LAUNCHER_COMMAND = 'node "${CLAUDE_PLUGIN_ROOT}/scripts/enforce-skill.mjs"'; const GOVERNED_EVENTS = ["UserPromptSubmit", "PreToolUse"]; // An absolute sh: the PATH below is deliberately minimal, so `sh` by name would not resolve. const SH = "/bin/sh"; @@ -82,7 +86,7 @@ const fs = require("node:fs"); let input = ""; process.stdin.on("data", (d) => (input += d)); process.stdin.on("end", () => { - fs.writeFileSync(${JSON.stringify(record)}, JSON.stringify({ argv: process.argv.slice(2), stdin: input, deadline: process.env.JF_AGENT_GUARD_ENFORCE_DEADLINE ?? "" })); + fs.writeFileSync(${JSON.stringify(record)}, JSON.stringify({ argv: process.argv.slice(2), stdin: input, deadline: process.env.JF_AGENT_GUARD_ENFORCE_DEADLINE ?? "", retries: process.env.npm_config_fetch_retries ?? "", fetchTimeout: process.env.npm_config_fetch_timeout ?? "" })); if (${JSON.stringify(stdout)}) process.stdout.write(${JSON.stringify(stdout)}); process.exit(${exitCode}); }); @@ -175,20 +179,25 @@ check("one governed hook refreshes the npx cache, the other reads it", () => { }); for (const event of GOVERNED_EVENTS) { - check(`${event} invokes agent-guard through npx only, with no plugin script in the path`, () => { + check(`${event} starts the node launcher, with no POSIX syntax in the hook string`, () => { const entries = hooksFor(event); assert(entries.length === 1, `expected exactly one ${event} hook, got ${entries.length}`); const h = entries[0]; assert(h.type === "command", `${event} must be a command hook, got ${h.type}`); - assert(/(^|\s)npx\s/.test(h.command), `${event} must invoke npx directly, got: ${h.command}`); + const expected = event === "PreToolUse" + ? `${LAUNCHER_COMMAND} --prefer-offline` + : LAUNCHER_COMMAND; + assert(h.command === expected, + `${event} must be the node launcher. A POSIX env prefix or $(...) never starts under ` + + `PowerShell, which is what VS Code uses on Windows. Got: ${h.command}`); + const beyondPluginRoot = h.command.replaceAll("${CLAUDE_PLUGIN_ROOT}", ""); + assert(!beyondPluginRoot.includes("$"), + `${event} command must not use shell substitution other than the plugin-root token ` + + `VS Code expands before the shell runs`); assert(!h.command.includes("command -v"), `${event} must not have a PATH fast path: a hijackable agent-guard earlier on PATH would win`); assert(!/block-skill|governance-client|skill-path|request-waiver|--waiver-helper/.test(h.command), `${event} must not route through a plugin governance script; agent-guard owns the waiver flow`); - assert(h.command.includes("--enforce-skill") && h.command.includes("--client vscode"), - `${event} must pass --enforce-skill --client vscode`); - assert(h.command.includes(RELEASES_REGISTRY), - `${event} must default to the releases registry, so the artifact is the published one`); }); check(`${event} carries no matcher, which VS Code would ignore anyway`, () => { @@ -203,45 +212,20 @@ for (const event of GOVERNED_EVENTS) { `block refuses every skill on a machine that simply cannot run the guard`); }); - check(`${event} lets npx cold-start and bounds its fetch`, () => { + check(`${event} leaves time for an npx cold start`, () => { const h = hooksFor(event)[0]; assert((h.timeout ?? 0) >= 30, `${event} timeout ${h.timeout} is too short for an npx cold start`); - const retries = /npm_config_fetch_retries=(\d+)/.exec(h.command); - const fetchTimeout = /npm_config_fetch_timeout=(\d+)/.exec(h.command); - assert(retries && Number(retries[1]) === 0, - `${event} must set npm_config_fetch_retries=0: npm's default of 2 backs off 10s then 60s`); - // The VALUE, not just its presence: npm_config_fetch_timeout=300000 is npm's own default, so - // asserting presence alone would let an edit back to the default pass unnoticed. - assert(fetchTimeout && Number(fetchTimeout[1]) <= 10_000, - `${event} must set npm_config_fetch_timeout <= 10000 (npm's default is 300000ms), got ${fetchTimeout?.[1]}`); }); - check(`${event} computes the deadline fresh, with no inheritable fallback`, () => { - const h = hooksFor(event)[0]; - assert(/_JFAG_NOW=\$\(date \+%s 2>\/dev\/null\);/.test(h.command), - `${event} must read the clock defensively, tolerating an absent date(1)`); - // Computed INSIDE a command substitution, so the `;` the clock read needs is scoped and the - // hook stays one simple command. See the top-level-operator check below for why. - assert(h.command.includes( - 'JF_AGENT_GUARD_ENFORCE_DEADLINE="$(_JFAG_NOW=$(date +%s 2>/dev/null); ' + - 'echo ${_JFAG_NOW:+$((_JFAG_NOW + 25))})"'), - `${event} must compute an absolute deadline at invocation time INSIDE a command ` + - `substitution, and pass EMPTY when the clock could not be read: agent-guard ignores an ` + - `empty deadline and falls back to its own budget, whereas a garbage epoch floors the ` + - `budget at 500ms and blocks every skill`); - assert(!/JF_AGENT_GUARD_ENFORCE_DEADLINE:[-=]/.test(h.command), - `${event} must not fall back to an inherited value: an absolute instant inherited from an ` + - `earlier process pins every later invocation to the past`); - }); - - check(`${event} cannot pin the agent-guard version`, () => { + check(`${event} cannot pin the agent-guard version from the hook string`, () => { const h = hooksFor(event)[0]; // Deliberately unlike the MCP-align hook, which pins on purpose. Governance must pick up a - // shipped GA fix without waiting for a plugin release. + // shipped GA fix without waiting for a plugin release. The package spec itself lives in the + // launcher; the hook string must not grow a pin of its own. assert(!h.command.includes("JFROG_AGENT_GUARD_VERSION"), `${event} must resolve latest, so a shipped GA fix reaches users without a plugin release`); - assert(/@jfrog\/agent-guard(\s|$)/.test(h.command), - `${event} must name the package unpinned: ${h.command}`); + assert(!/@jfrog\/agent-guard@/.test(h.command), + `${event} hook string must not pin the package: ${h.command}`); }); } @@ -295,6 +279,61 @@ check("no governed command has a top-level ';', '&&' or '||'", () => { } }); +await check("the launcher spawns npx.cmd through cmd.exe on Windows and does not pin a version", async () => { + const launcherUrl = pathToFileURL(path.join(pluginRoot, "scripts", "enforce-skill.mjs")).href; + const mod = await import(launcherUrl); + assert(typeof mod.resolveEnforceSpawn === "function", + "enforce-skill.mjs must export resolveEnforceSpawn"); + const nowSec = 1_700_000_000; + const common = { + env: { + JF_AGENT_GUARD_ENFORCE_DEADLINE: "1", + JFROG_AGENT_GUARD_VERSION: "0.0.0-master.1.gabc", + PATH: "C:\\Windows\\System32", + }, + nowSec, + }; + const win = mod.resolveEnforceSpawn({ ...common, preferOffline: true, platform: "win32" }); + assert(win.command === "npx.cmd", `Windows must spawn npx.cmd, got ${win.command}`); + assert(win.shell === "cmd.exe", + `Windows must pin cmd.exe so ComSpec cannot select PowerShell, got ${String(win.shell)}`); + assert(win.args.includes('"--prefer-offline"'), `missing offline flag: ${win.args}`); + assert(win.args.includes(`"${RELEASES_REGISTRY}"`), `registry must be quoted for cmd.exe: ${win.args}`); + assert(win.args.includes('"@jfrog/agent-guard"'), `package spec must stay unpinned: ${win.args}`); + assert(!win.args.some((arg) => arg.includes("@jfrog/agent-guard@")), + `JFROG_AGENT_GUARD_VERSION must not pin the spec: ${win.args}`); + assert(win.env.JF_AGENT_GUARD_ENFORCE_DEADLINE === String(nowSec + 25), + `deadline must replace an inherited value; got ${win.env.JF_AGENT_GUARD_ENFORCE_DEADLINE}`); + assert(win.env.npm_config_fetch_retries === "0", + "npm_config_fetch_retries=0 is required: npm's default of 2 backs off 10s then 60s"); + assert(win.env.npm_config_fetch_timeout === "10000", + `npm_config_fetch_timeout must be 10000 (npm's default is 300000), got ${win.env.npm_config_fetch_timeout}`); + const quoted = mod.resolveEnforceSpawn({ + preferOffline: false, + env: { JFROG_AGENT_GUARD_REPO: "https://example.invalid/npm/?a=1&b=2" }, + nowSec, + platform: "win32", + }); + assert(quoted.args.includes('"https://example.invalid/npm/?a=1&b=2"'), + `a registry & must stay inside one cmd.exe argument: ${quoted.args}`); + assert(!quoted.args.includes('"--prefer-offline"'), + `UserPromptSubmit must refresh the cache: ${quoted.args}`); + const unix = mod.resolveEnforceSpawn({ ...common, preferOffline: false, platform: "linux" }); + assert(unix.command === "npx" && unix.shell === false, + `Unix must spawn npx with no shell, got ${unix.command} shell=${String(unix.shell)}`); + assert(unix.args[unix.args.indexOf("--registry") + 1] === RELEASES_REGISTRY, + `default registry drifted: ${unix.args}`); + assert(!unix.args.includes("--prefer-offline"), `offline flag leaked: ${unix.args}`); + const stale = mod.resolveEnforceSpawn({ + preferOffline: false, + env: { JF_AGENT_GUARD_ENFORCE_DEADLINE: "1" }, + nowSec: Number.NaN, + platform: "linux", + }); + assert(stale.env.JF_AGENT_GUARD_ENFORCE_DEADLINE === undefined, + `an unusable clock must drop an inherited deadline, got ${stale.env.JF_AGENT_GUARD_ENFORCE_DEADLINE}`); +}); + // --------------------------------------------------------------------------- // Behavioural: execute the real hooks.json command string. // --------------------------------------------------------------------------- @@ -320,6 +359,13 @@ for (const event of GOVERNED_EVENTS) { assert(seen.argv[seen.argv.indexOf("--client") + 1] === "vscode", `bad --client: ${seen.argv}`); assert(seen.argv[seen.argv.indexOf("--registry") + 1] === RELEASES_REGISTRY, `must default to the releases registry: ${seen.argv}`); + if (event === "PreToolUse") { + assert(seen.argv.includes("--prefer-offline"), `PreToolUse must read the npx cache: ${seen.argv}`); + } else { + assert(!seen.argv.includes("--prefer-offline"), `UserPromptSubmit must refresh the registry: ${seen.argv}`); + } + assert(seen.retries === "0" && seen.fetchTimeout === "10000", + `fetch must be bounded: retries=${seen.retries} timeout=${seen.fetchTimeout}`); }); await check(`${event}: hands agent-guard a deadline in the future, computed at invocation`, async () => { @@ -335,19 +381,19 @@ for (const event of GOVERNED_EVENTS) { `the deadline must be recomputed, not inherited; got ${seen.deadline}`); }); - // The static check above proves the command CONTAINS the `${_JFAG_NOW:+…}` guard; only running - // it with no date(1) on PATH proves the guard does what the guard is for. EMPTY is the required - // outcome, not merely "some value": agent-guard ignores an empty deadline and falls back to its - // own budget, whereas a garbage epoch (what `$(($(date +%s) + 25))` degrades to — `$(( + 25))` - // = 25, an instant in 1970) floors that budget at 500ms and blocks every skill. - await check(`${event}: with no date(1) on PATH, the deadline degrades to EMPTY and the payload still arrives`, async () => { + // The deadline used to be $(date +%s) inside the hook string. That binary is absent from a + // minimal Windows PATH, and PowerShell cannot parse the substitution anyway. The launcher + // reads the clock in process, so a missing date(1) must not empty the deadline or drop stdin. + await check(`${event}: with no date(1) on PATH, the deadline is still in the future and the payload arrives`, async () => { const record = stubNpx({ stdout: `{"continue":true}` }); const payload = payloadFor(event); + const before = Math.floor(Date.now() / 1000); const r = runHook(commandFor(event), payload, { noDate: true }); assert(r.code === 0, `exit=${r.code} stderr=${r.stderr}`); const seen = JSON.parse(readFileSync(record, "utf8")); - assert(seen.deadline === "", - `an unreadable clock must yield an EMPTY deadline, not a stale or garbage one; got ${JSON.stringify(seen.deadline)}`); + const deadline = Number(seen.deadline); + assert(Number.isFinite(deadline) && deadline > before, + `a missing date(1) must not block the clock; got ${JSON.stringify(seen.deadline)}`); assert(seen.stdin === payload, `losing date(1) must not cost the payload: agent-guard received ${seen.stdin.length} bytes, expected ${payload.length}`); }); From 538bd091bbb9e600c962e1e5f90af6b44222ca5f Mon Sep 17 00:00:00 2001 From: nadav-dvir Date: Mon, 26 Oct 2026 15:00:23 +0200 Subject: [PATCH 2/3] [patch] Fail open with exit 0 in the skill-governance launcher VS Code's PreToolUse denies the tool call on any non-zero exit other than 2, so the launcher's exit 1 on a spawn error, signal or npx failure blocked every tool call instead of failing open. The launcher now buffers the child's stdout and passes through only exit 0 or 2 with that output; every other outcome prints {} and exits 0. On Windows it runs npm's npx-cli.js with node and no shell, falling back to npx.cmd under cmd.exe only when npm is not beside node.exe. The fallback refuses arguments cmd.exe cannot carry (%, ", CR/LF) instead of the %% escaping, which cmd /c does not undo. The registry comes from the shared resolveAgentGuardNpmRegistry. Spawn-plan and exit-code coverage moves to plugin/scripts/enforce-skill.test.mjs with a fake spawn. The validator now asserts exit 0 and {} for fail-open, and drops the date(1), POSIX-sh and top-level ';' checks that no longer test anything. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- plugin/scripts/enforce-skill.mjs | 200 +++++++++++++------- plugin/scripts/enforce-skill.test.mjs | 251 ++++++++++++++++++++++++++ scripts/validate-skill-governance.mjs | 172 +++--------------- 4 files changed, 415 insertions(+), 210 deletions(-) create mode 100644 plugin/scripts/enforce-skill.test.mjs diff --git a/README.md b/README.md index 375ecb2..1e94445 100644 --- a/README.md +++ b/README.md @@ -268,7 +268,7 @@ When a policy block carries a waiver scope, the block message shows the command - Set `JFROG_PLATFORM_URL` and `JFROG_ACCESS_TOKEN` (or configure the JFrog CLI — see [Authentication](#authentication)) and `JF_PROJECT` (the JFrog project the skill runs in). For an entitled account with credentials but no project, skills are **blocked** with a message telling you what to set; with no credentials at all they are **allowed**, per the table above. - **Node.js (≥ 20) with `npx` on your `PATH`** — the hook resolves the Agent Guard through `npx`. Without it, governed actions are allowed unchecked. -- **Windows, macOS, and Linux.** The hook command is `node` plus `scripts/enforce-skill.mjs`. VS Code runs that string with PowerShell on a normal Windows session, which cannot parse a POSIX one-liner, so the launcher computes the deadline and starts `npx` itself (`npx.cmd` through `cmd.exe` on Windows). Agent Guard still decides allow or deny. If `node` or `npx` cannot start, the skill is allowed. +- **Windows, macOS, and Linux.** The hook command is `node` plus `scripts/enforce-skill.mjs`. VS Code runs that string with PowerShell on a normal Windows session, which cannot parse a POSIX one-liner, so the launcher computes the deadline and starts `npx` itself (on Windows, npm's `npx-cli.js` run by `node` with no shell, falling back to `npx.cmd` through `cmd.exe`). Agent Guard still decides allow or deny. If `npx` cannot start or Agent Guard fails, the hook returns `{}` with exit 0 and the skill is allowed. - **Unlike the MCP alignment hook, this one does not pin the Agent Guard version.** It always resolves the latest published release, so a governance fix reaches you without waiting for a plugin release. `JFROG_AGENT_GUARD_REPO` still redirects the registry. - **Coverage limits.** `github.copilot.chat.skillTool.enabled` is **off by default**, and with it off Copilot registers no `skill` tool — it instructs the model to read the matching `SKILL.md` with its `read_file` tool instead. That call reaches the `PreToolUse` hook and the Agent Guard governs it, so a default install is covered through `read_file` rather than through the skill tool. What is *not* covered: skills contributed by other extensions (`contributes.chatSkills`) are not searched, and neither are folders added through `chat.agentSkillsLocations`. A skill's content can also be reached without a governed surface — through a subagent, a terminal command, or a text search — and hooks are a VS Code Preview feature that can be disabled organization-side with no signal. An absence of blocks never by itself means a skill was allowed. - **Cost per call.** VS Code parses `matcher` for Claude Code compatibility but ignores its value, so the `PreToolUse` hook runs on *every* tool call, and `UserPromptSubmit` on every prompt. The prompt hook revalidates the Agent Guard against the registry; the tool hook reads that from cache, which is what keeps the per-call cost down. diff --git a/plugin/scripts/enforce-skill.mjs b/plugin/scripts/enforce-skill.mjs index 8bf3fd0..1f7185e 100644 --- a/plugin/scripts/enforce-skill.mjs +++ b/plugin/scripts/enforce-skill.mjs @@ -3,43 +3,60 @@ // Starts @jfrog/agent-guard --enforce-skill. The hook string in hooks.json is only // `node` plus this file: VS Code runs that string with PowerShell on Windows, which // cannot parse a POSIX env prefix or $(date). This process computes the deadline, -// then spawns npx. It does not read the verdict. Exit 2 and stdout pass through. -// A spawn failure exits 1 so the host fails open. +// then spawns npx. It does not read the verdict. +// +// Exit codes: VS Code's PreToolUse fails closed on ANY non-zero exit, and exit 2 is +// the only deliberate block. agent-guard exits 0 or 2, so both pass through with its +// stdout. Every other outcome (spawn failure, signal, any other exit code) prints `{}` +// and exits 0, so a machine that cannot run the guard is not blocked by it. import { spawn } from "node:child_process"; +import { existsSync } from "node:fs"; +import path from "node:path"; import process from "node:process"; import { isMainEntry } from "../modules/core/entry.mjs"; +import { + AGENT_GUARD_PACKAGE, + resolveAgentGuardNpmRegistry, +} from "../modules/core/rewrite-mcp-json.mjs"; -export const RELEASES_REGISTRY = "https://releases.jfrog.io/artifactory/api/npm/coding-agents-npm/"; const DEADLINE_SLACK_SEC = 25; +export const FAIL_OPEN_OUTPUT = "{}"; /** - * @param {string} arg - * @returns {string} + * npm's npx entry point, installed next to node.exe by the Windows Node installer. + * Running it with process.execPath needs no shell, unlike npx.cmd. + * + * @param {string} execPath + * @param {(p: string) => boolean} [exists] + * @returns {string | undefined} */ -export function quoteWindowsArg(arg) { - const value = String(arg ?? ""); - if (/[\r\n]/.test(value)) { - throw new Error("Windows spawn arg must not contain CR/LF"); - } - const escaped = value.replace(/%/g, "%%").replace(/"/g, '""'); - return `"${escaped}"`; +export function bundledNpxCli(execPath, exists = existsSync) { + const cli = path.win32.join(path.win32.dirname(execPath), "node_modules", "npm", "bin", "npx-cli.js"); + return exists(cli) ? cli : undefined; } /** - * @param {NodeJS.ProcessEnv} env + * Quote one argument for the npx.cmd fallback under `cmd.exe /d /s /c`. That command + * line has no escape for `%` (`%%` stays literal, `%VAR%` expands) and none for `"` + * inside quotes, so an argument holding either is refused rather than mangled. + * + * @param {string} arg * @returns {string} */ -export function registryFrom(env) { - const raw = env.JFROG_AGENT_GUARD_REPO; - if (typeof raw === "string" && raw.trim() !== "") return raw; - return RELEASES_REGISTRY; +export function quoteCmdArg(arg) { + const value = String(arg ?? ""); + if (/[\r\n%"]/.test(value)) { + throw new Error(`cmd.exe cannot carry this argument intact: ${JSON.stringify(value)}`); + } + return `"${value}"`; } /** - * Spawn plan for `npx --enforce-skill`. On Windows the command is npx.cmd under - * cmd.exe, not ComSpec, so a PowerShell ComSpec cannot parse the argument list. + * Spawn plan for `npx --enforce-skill`. POSIX runs npx with no shell. Windows runs + * npm's npx-cli.js with node itself, and falls back to npx.cmd under cmd.exe (not + * ComSpec, which may be PowerShell) only when npm is not installed beside node.exe. * JFROG_AGENT_GUARD_VERSION is ignored: governance must stay on the latest release. * * @param {{ @@ -47,11 +64,15 @@ export function registryFrom(env) { * env?: NodeJS.ProcessEnv, * nowSec?: number, * platform?: NodeJS.Platform, + * execPath?: string, + * exists?: (p: string) => boolean, * }} [opts] + * @returns {{ command: string, args: string[], shell: string | false, env: NodeJS.ProcessEnv }} */ export function resolveEnforceSpawn(opts = {}) { const env = opts.env ?? process.env; const platform = opts.platform ?? process.platform; + const execPath = opts.execPath ?? process.execPath; const childEnv = { ...env }; childEnv.npm_config_fetch_retries = "0"; childEnv.npm_config_fetch_timeout = "10000"; @@ -65,63 +86,112 @@ export function resolveEnforceSpawn(opts = {}) { if (opts.preferOffline === true) args.push("--prefer-offline"); args.push( "--registry", - registryFrom(env), - "@jfrog/agent-guard", + resolveAgentGuardNpmRegistry(env), + AGENT_GUARD_PACKAGE, "--enforce-skill", "--client", "vscode", ); - const windows = platform === "win32"; - return { - command: windows ? "npx.cmd" : "npx", - args: windows ? args.map(quoteWindowsArg) : args, - shell: windows ? "cmd.exe" : false, - env: childEnv, - }; + if (platform !== "win32") { + return { command: "npx", args, shell: false, env: childEnv }; + } + const cli = bundledNpxCli(execPath, opts.exists); + if (cli) { + return { command: execPath, args: [cli, ...args], shell: false, env: childEnv }; + } + return { command: "npx.cmd", args: args.map(quoteCmdArg), shell: "cmd.exe", env: childEnv }; } -function main() { - /** @type {ReturnType} */ - let plan; - try { - plan = resolveEnforceSpawn({ - preferOffline: process.argv.slice(2).includes("--prefer-offline"), - env: process.env, - nowSec: Math.floor(Date.now() / 1000), - platform: process.platform, - }); - } catch { - process.exit(1); - } +/** + * Runs agent-guard with the hook payload on its stdin and resolves the exit code for + * VS Code: the child's 0 or 2 with its stdout, otherwise `{}` and 0. + * + * @param {{ + * argv?: string[], + * env?: NodeJS.ProcessEnv, + * platform?: NodeJS.Platform, + * execPath?: string, + * exists?: (p: string) => boolean, + * nowSec?: number, + * spawnFn?: typeof spawn, + * stdin?: NodeJS.ReadableStream, + * stdout?: NodeJS.WritableStream, + * }} [opts] + * @returns {Promise<0 | 2>} + */ +export function runEnforce(opts = {}) { + const stdin = opts.stdin ?? process.stdin; + const stdout = opts.stdout ?? process.stdout; + const spawnFn = opts.spawnFn ?? spawn; - /** @type {ReturnType} */ - let child; - try { - child = spawn(plan.command, plan.args, { - env: plan.env, - shell: plan.shell, - windowsHide: true, - stdio: ["pipe", "inherit", "inherit"], - }); - } catch { - process.exit(1); - } + return new Promise((resolve) => { + let settled = false; + /** @param {0 | 2} code @param {string | Buffer} output */ + const finish = (code, output) => { + if (settled) return; + settled = true; + if (output.length === 0) { + resolve(code); + return; + } + stdout.write(output, () => resolve(code)); + }; + const failOpen = () => finish(0, FAIL_OPEN_OUTPUT); - child.on("error", () => { - process.exit(1); - }); - // A fast exit (including a policy exit 2) can EPIPE the stdin pipe. That must - // not replace the child's status. - if (!child.stdin) process.exit(1); - child.stdin.on("error", () => {}); - process.stdin.pipe(child.stdin); - child.on("exit", (code, signal) => { - if (signal || code === null) process.exit(1); - process.exit(code); + /** @type {ReturnType} */ + let plan; + try { + plan = resolveEnforceSpawn({ + preferOffline: (opts.argv ?? process.argv.slice(2)).includes("--prefer-offline"), + env: opts.env ?? process.env, + nowSec: opts.nowSec ?? Math.floor(Date.now() / 1000), + platform: opts.platform, + execPath: opts.execPath, + exists: opts.exists, + }); + } catch { + failOpen(); + return; + } + + /** @type {ReturnType} */ + let child; + try { + child = spawnFn(plan.command, plan.args, { + env: plan.env, + shell: plan.shell, + windowsHide: true, + stdio: ["pipe", "pipe", "inherit"], + }); + } catch { + failOpen(); + return; + } + + child.on("error", failOpen); + if (!child.stdin || !child.stdout) { + child.kill?.(); + failOpen(); + return; + } + /** @type {Buffer[]} */ + const chunks = []; + child.stdout.on("data", (chunk) => chunks.push(chunk)); + // A fast exit (including a policy exit 2) can EPIPE the stdin pipe. That must + // not replace the child's status. + child.stdin.on("error", () => {}); + stdin.pipe(child.stdin); + child.on("close", (code, signal) => { + if (!signal && (code === 0 || code === 2)) finish(code, Buffer.concat(chunks)); + else failOpen(); + }); }); } if (isMainEntry(import.meta.url)) { - main(); + runEnforce().then( + (code) => process.exit(code), + () => process.stdout.write(FAIL_OPEN_OUTPUT, () => process.exit(0)), + ); } diff --git a/plugin/scripts/enforce-skill.test.mjs b/plugin/scripts/enforce-skill.test.mjs new file mode 100644 index 0000000..87b4d08 --- /dev/null +++ b/plugin/scripts/enforce-skill.test.mjs @@ -0,0 +1,251 @@ +import assert from "node:assert/strict"; +import { EventEmitter } from "node:events"; +import { PassThrough, Readable, Writable } from "node:stream"; +import test from "node:test"; + +import { DEFAULT_AGENT_GUARD_NPM_REGISTRY } from "../modules/core/rewrite-mcp-json.mjs"; +import { + FAIL_OPEN_OUTPUT, + bundledNpxCli, + quoteCmdArg, + resolveEnforceSpawn, + runEnforce, +} from "./enforce-skill.mjs"; + +const NOW = 1_700_000_000; +const WIN_NODE = "C:\\Program Files\\nodejs\\node.exe"; +const WIN_NPX_CLI = "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npx-cli.js"; +const PAYLOAD = `{"hook_event_name":"PreToolUse","tool_name":"skill"}`; + +const winPlan = (opts = {}) => + resolveEnforceSpawn({ platform: "win32", execPath: WIN_NODE, nowSec: NOW, env: {}, ...opts }); + +// --- spawn plan ----------------------------------------------------------------------------- + +test("POSIX spawns npx with no shell and the expected argv", () => { + const plan = resolveEnforceSpawn({ platform: "linux", nowSec: NOW, env: {} }); + assert.equal(plan.command, "npx"); + assert.equal(plan.shell, false); + assert.deepEqual(plan.args, [ + "--yes", "--registry", DEFAULT_AGENT_GUARD_NPM_REGISTRY, + "@jfrog/agent-guard", "--enforce-skill", "--client", "vscode", + ]); +}); + +test("--prefer-offline is passed only when asked for", () => { + const offline = resolveEnforceSpawn({ platform: "linux", nowSec: NOW, env: {}, preferOffline: true }); + const online = resolveEnforceSpawn({ platform: "linux", nowSec: NOW, env: {} }); + assert.ok(offline.args.includes("--prefer-offline")); + assert.ok(!online.args.includes("--prefer-offline")); +}); + +test("npm fetches are bounded: no retries, 10s timeout", () => { + const { env } = resolveEnforceSpawn({ platform: "linux", nowSec: NOW, env: {} }); + assert.equal(env.npm_config_fetch_retries, "0"); + assert.equal(env.npm_config_fetch_timeout, "10000"); +}); + +test("the deadline is recomputed and replaces an inherited value", () => { + const { env } = resolveEnforceSpawn({ + platform: "linux", nowSec: NOW, env: { JF_AGENT_GUARD_ENFORCE_DEADLINE: "1" }, + }); + assert.equal(env.JF_AGENT_GUARD_ENFORCE_DEADLINE, String(NOW + 25)); +}); + +test("an unusable clock drops an inherited deadline", () => { + const { env } = resolveEnforceSpawn({ + platform: "linux", nowSec: Number.NaN, env: { JF_AGENT_GUARD_ENFORCE_DEADLINE: "1" }, + }); + assert.equal(env.JF_AGENT_GUARD_ENFORCE_DEADLINE, undefined); +}); + +test("JFROG_AGENT_GUARD_VERSION cannot pin the package", () => { + const plan = resolveEnforceSpawn({ + platform: "linux", nowSec: NOW, env: { JFROG_AGENT_GUARD_VERSION: "0.0.0-master.1.gabc" }, + }); + assert.ok(plan.args.includes("@jfrog/agent-guard")); + assert.ok(!plan.args.some((a) => a.startsWith("@jfrog/agent-guard@"))); +}); + +test("JFROG_AGENT_GUARD_REPO redirects the registry, trimmed like the MCP-align hook", () => { + const plan = resolveEnforceSpawn({ + platform: "linux", nowSec: NOW, env: { JFROG_AGENT_GUARD_REPO: " https://example.invalid/npm/ " }, + }); + assert.equal(plan.args[plan.args.indexOf("--registry") + 1], "https://example.invalid/npm/"); +}); + +test("a blank JFROG_AGENT_GUARD_REPO falls back to the releases registry", () => { + const plan = resolveEnforceSpawn({ platform: "linux", nowSec: NOW, env: { JFROG_AGENT_GUARD_REPO: " " } }); + assert.equal(plan.args[plan.args.indexOf("--registry") + 1], DEFAULT_AGENT_GUARD_NPM_REGISTRY); +}); + +test("bundledNpxCli looks beside node.exe", () => { + assert.equal(bundledNpxCli(WIN_NODE, (p) => p === WIN_NPX_CLI), WIN_NPX_CLI); + assert.equal(bundledNpxCli(WIN_NODE, () => false), undefined); +}); + +test("Windows runs npx-cli.js with node itself, no shell", () => { + const plan = winPlan({ exists: (p) => p === WIN_NPX_CLI }); + assert.equal(plan.command, WIN_NODE); + assert.equal(plan.shell, false); + assert.equal(plan.args[0], WIN_NPX_CLI); + assert.ok(plan.args.includes("--enforce-skill")); +}); + +test("Windows without a shell passes a registry with & and % unquoted", () => { + const registry = "https://example.invalid/npm/?a=1&b=%41"; + const plan = winPlan({ exists: () => true, env: { JFROG_AGENT_GUARD_REPO: registry } }); + assert.equal(plan.args[plan.args.indexOf("--registry") + 1], registry); +}); + +test("Windows falls back to npx.cmd under a pinned cmd.exe when npm is not beside node.exe", () => { + const plan = winPlan({ exists: () => false, preferOffline: true }); + assert.equal(plan.command, "npx.cmd"); + assert.equal(plan.shell, "cmd.exe"); + assert.ok(plan.args.includes('"--prefer-offline"')); + assert.ok(plan.args.includes(`"${DEFAULT_AGENT_GUARD_NPM_REGISTRY}"`)); +}); + +test("the cmd.exe fallback keeps & inside one quoted argument", () => { + const plan = winPlan({ exists: () => false, env: { JFROG_AGENT_GUARD_REPO: "https://example.invalid/?a=1&b=2" } }); + assert.ok(plan.args.includes('"https://example.invalid/?a=1&b=2"')); +}); + +test("the cmd.exe fallback refuses an argument it cannot carry intact", () => { + for (const registry of ["https://x/%PATH%/", 'https://x/"/', "https://x/\r\n/"]) { + assert.throws(() => winPlan({ exists: () => false, env: { JFROG_AGENT_GUARD_REPO: registry } }), + `should refuse ${JSON.stringify(registry)}`); + } +}); + +test("quoteCmdArg wraps a plain argument in quotes", () => { + assert.equal(quoteCmdArg("--enforce-skill"), '"--enforce-skill"'); + assert.throws(() => quoteCmdArg("50%")); + assert.throws(() => quoteCmdArg('a"b')); + assert.throws(() => quoteCmdArg("a\nb")); +}); + +// --- runEnforce: exit-code and stdout contract ---------------------------------------------- + +/** + * A fake child process. `script(child)` drives it after the launcher has wired its streams. + * @param {(child: EventEmitter & { stdin: Writable, stdout: PassThrough }) => void} script + * @param {{ stdin?: Writable, throwOnSpawn?: boolean }} [opts] + */ +function fakeSpawn(script, opts = {}) { + const calls = []; + const spawnFn = (command, args, options) => { + calls.push({ command, args, options }); + if (opts.throwOnSpawn) throw new Error("spawn EINVAL"); + const child = Object.assign(new EventEmitter(), { + stdin: opts.stdin ?? new PassThrough(), + stdout: new PassThrough(), + kill: () => true, + }); + setImmediate(() => script(child)); + return child; + }; + return { spawnFn, calls }; +} + +function captureStdout() { + let text = ""; + const stream = new Writable({ + write(chunk, _enc, cb) { text += chunk.toString(); cb(); }, + }); + return { stream, text: () => text }; +} + +/** Emit stdout, then close with the given status, the way a real child does. */ +const exitWith = (code, { stdout = "", signal = null } = {}) => (child) => { + if (stdout) child.stdout.write(stdout); + child.stdout.end(); + setImmediate(() => child.emit("close", code, signal)); +}; + +const run = (spawnFn, extra = {}) => { + const out = captureStdout(); + const done = runEnforce({ + argv: [], env: {}, platform: "linux", nowSec: NOW, + spawnFn, stdin: Readable.from([PAYLOAD]), stdout: out.stream, ...extra, + }); + return done.then((code) => ({ code, stdout: out.text() })); +}; + +test("exit 0 passes the child's stdout through verbatim", async () => { + const verdict = `{"continue":true}`; + const r = await run(fakeSpawn(exitWith(0, { stdout: verdict })).spawnFn); + assert.deepEqual(r, { code: 0, stdout: verdict }); +}); + +test("exit 0 with no stdout stays silent", async () => { + const r = await run(fakeSpawn(exitWith(0)).spawnFn); + assert.deepEqual(r, { code: 0, stdout: "" }); +}); + +test("exit 2 passes through with the child's stdout", async () => { + const deny = `{"hookSpecificOutput":{"permissionDecision":"deny"}}`; + const r = await run(fakeSpawn(exitWith(2, { stdout: deny })).spawnFn); + assert.deepEqual(r, { code: 2, stdout: deny }); +}); + +test("any other exit code fails open with {} and discards partial output", async () => { + for (const code of [1, 127, 3]) { + const r = await run(fakeSpawn(exitWith(code, { stdout: `{"hookSpecificOutput":{"perm` })).spawnFn); + assert.deepEqual(r, { code: 0, stdout: FAIL_OPEN_OUTPUT }, `exit ${code}`); + } +}); + +test("a child killed by a signal fails open", async () => { + const r = await run(fakeSpawn(exitWith(null, { signal: "SIGKILL" })).spawnFn); + assert.deepEqual(r, { code: 0, stdout: FAIL_OPEN_OUTPUT }); +}); + +test("a spawn that throws fails open", async () => { + const r = await run(fakeSpawn(() => {}, { throwOnSpawn: true }).spawnFn); + assert.deepEqual(r, { code: 0, stdout: FAIL_OPEN_OUTPUT }); +}); + +test("a spawn error event (npx missing) fails open, even if close follows", async () => { + const r = await run(fakeSpawn((child) => { + child.emit("error", Object.assign(new Error("spawn npx ENOENT"), { code: "ENOENT" })); + child.emit("close", -2, null); + }).spawnFn); + assert.deepEqual(r, { code: 0, stdout: FAIL_OPEN_OUTPUT }); +}); + +test("a spawn plan that cannot be built fails open without spawning", async () => { + const { spawnFn, calls } = fakeSpawn(exitWith(2)); + const r = await run(spawnFn, { + platform: "win32", execPath: WIN_NODE, exists: () => false, + env: { JFROG_AGENT_GUARD_REPO: "https://x/%PATH%/" }, + }); + assert.deepEqual(r, { code: 0, stdout: FAIL_OPEN_OUTPUT }); + assert.equal(calls.length, 0); +}); + +test("an EPIPE on the child's stdin does not replace its exit 2", async () => { + const brokenStdin = new Writable({ + write(_chunk, _enc, cb) { cb(Object.assign(new Error("write EPIPE"), { code: "EPIPE" })); }, + }); + const r = await run(fakeSpawn(exitWith(2, { stdout: "blocked" }), { stdin: brokenStdin }).spawnFn); + assert.deepEqual(r, { code: 2, stdout: "blocked" }); +}); + +test("the hook payload reaches the child's stdin verbatim", async () => { + let received = ""; + const r = await run(fakeSpawn((child) => { + child.stdin.on("data", (d) => (received += d)); + child.stdin.on("end", () => exitWith(0, { stdout: "{}" })(child)); + }).spawnFn); + assert.equal(received, PAYLOAD); + assert.equal(r.code, 0); +}); + +test("the child gets piped stdout and inherited stderr", async () => { + const { spawnFn, calls } = fakeSpawn(exitWith(0)); + await run(spawnFn, { argv: ["--prefer-offline"] }); + assert.deepEqual(calls[0].options.stdio, ["pipe", "pipe", "inherit"]); + assert.equal(calls[0].options.shell, false); + assert.ok(calls[0].args.includes("--prefer-offline")); +}); diff --git a/scripts/validate-skill-governance.mjs b/scripts/validate-skill-governance.mjs index 40951ac..4032916 100644 --- a/scripts/validate-skill-governance.mjs +++ b/scripts/validate-skill-governance.mjs @@ -7,8 +7,9 @@ // agent-guard via npx. The launcher is not a policy engine: it only builds the argv, the // deadline, and a spawn that Windows can run. VS Code executes the hook string with // PowerShell when ComSpec is cmd.exe, so the string itself cannot be a POSIX one-liner. -// This validator asserts that shape, the Windows spawn plan, and then EXECUTES the real -// command string out of hooks.json against a stub agent-guard. +// This validator asserts that shape, then EXECUTES the real command string out of +// hooks.json against a stub agent-guard. The launcher's spawn plan and exit-code mapping +// are unit-tested in plugin/scripts/enforce-skill.test.mjs. // // Four properties carry the whole design: // @@ -18,7 +19,8 @@ // // 2. Infrastructure failure fails OPEN. npx missing, a failed install, an unreachable registry // or no configured JFrog server must let the skill through: a machine that cannot run the -// guard is not governed by it. A real policy denial still blocks, in the JSON payload. +// guard is not governed by it. PreToolUse denies on ANY non-zero exit but 2, so failing open +// means exit 0 with `{}`. A real policy denial still blocks, in the JSON payload or exit 2. // // 3. Nothing may pin the agent-guard version — unlike the MCP-align hook, which pins on purpose. // Governance must pick up a shipped GA fix without a plugin release. @@ -32,7 +34,7 @@ import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, w import { tmpdir } from "node:os"; import path from "node:path"; import process from "node:process"; -import { fileURLToPath, pathToFileURL } from "node:url"; +import { fileURLToPath } from "node:url"; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const pluginRoot = path.join(repoRoot, "plugin"); @@ -47,20 +49,11 @@ const SH = "/bin/sh"; const sandbox = mkdtempSync(path.join(tmpdir(), "vscode-gov-")); const binDir = path.join(sandbox, "bin"); mkdirSync(binDir, { recursive: true }); -// A directory holding ONLY node, so the stub's shebang resolves while the real npx stays -// unreachable. `date` lives here too: the hook computes its deadline with $(date +%s), and a PATH -// without it would silently yield "$(( + 25))" = 25 — an epoch in 1970 — rather than exercising -// the real computation. Keeping both here (not by adding /bin to PATH) preserves the isolate mode, -// where npx must stay unreachable. +// A directory holding ONLY node, so the launcher and the stub's shebang resolve while the real +// npx stays unreachable in isolate mode. const nodeDir = path.join(sandbox, "node-only"); mkdirSync(nodeDir, { recursive: true }); symlinkSync(process.execPath, path.join(nodeDir, "node")); -symlinkSync("/bin/date", path.join(nodeDir, "date")); -// The same directory WITHOUT `date`, for the one check that asserts the degrade path. Separated -// from nodeDir so every other check exercises the real clock rather than its degraded form. -const noDateDir = path.join(sandbox, "node-only-nodate"); -mkdirSync(noDateDir, { recursive: true }); -symlinkSync(process.execPath, path.join(noDateDir, "node")); const failures = []; const check = async (label, fn) => { @@ -72,9 +65,11 @@ const assert = (cond, msg) => { if (!cond) throw new Error(msg); }; const groupsFor = (event) => hooks?.hooks?.[event] ?? []; const hooksFor = (event) => groupsFor(event).flatMap((g) => g.hooks ?? []); const commandFor = (event) => hooksFor(event)[0]?.command ?? ""; -// A blocking answer is a deny in the JSON, or agent-guard's own exit 2. Anything else lets the -// skill through — which is what "fails open" has to mean here. -const blocks = (r) => r.code === 2 || /"permissionDecision"\s*:\s*"deny"|"continue"\s*:\s*false/.test(r.stdout); +// A blocking answer is a deny in the JSON, or an exit code VS Code treats as one: exit 2 on +// either surface, and any non-zero exit on PreToolUse, which fails closed on a hook error. +const blocks = (event, r) => + (event === "PreToolUse" ? r.code !== 0 : r.code === 2) || + /"permissionDecision"\s*:\s*"deny"|"continue"\s*:\s*false/.test(r.stdout); // A stub npx that records the argv it was handed and the stdin it received, then replays a canned // result. Installed as `npx` so the hook command finds it first on PATH. @@ -107,13 +102,13 @@ process.stdin.on("end", () => { // a regression ship — a top-level `;` in the command severed Cursor's pipeline and every skill // was silently allowed, while a stdin-based harness passed all 34 checks. If a third delivery shape // ever appears, model it here rather than reusing this one. -function runHook(command, payload, { isolate = false, noDate = false, extraEnv = {} } = {}) { +function runHook(command, payload, { isolate = false, extraEnv = {} } = {}) { const result = spawnSync(SH, ["-c", command], { input: Buffer.from(payload), encoding: "buffer", timeout: 30_000, env: { - PATH: isolate ? nodeDir : `${binDir}:${noDate ? noDateDir : nodeDir}`, + PATH: isolate ? nodeDir : `${binDir}:${nodeDir}`, HOME: sandbox, CLAUDE_PLUGIN_ROOT: pluginRoot, ...extraEnv, @@ -236,104 +231,6 @@ check("the two governed hooks differ ONLY in the cache flag", () => { ` UserPromptSubmit: ${commandFor("UserPromptSubmit")}\n PreToolUse: ${commandFor("PreToolUse")}`); }); -check("every hook command is valid POSIX sh", () => { - for (const event of Object.keys(hooks.hooks ?? {})) { - for (const h of hooksFor(event)) { - const r = spawnSync(SH, ["-n", "-c", h.command], { encoding: "utf8" }); - assert(r.status === 0, `${event} command is not valid sh: ${r.stderr.trim()}`); - } - } -}); - -// Strip every $(…) / $((…)) group, leaving only the command's TOP-LEVEL text. A ';' inside a -// substitution is scoped and harmless; one outside it is not. -const topLevelOf = (s) => { - let out = "", depth = 0; - for (let i = 0; i < s.length; i++) { - if (s.startsWith("$(", i)) { depth++; i++; continue; } - if (depth && s[i] === "(") { depth++; continue; } - if (depth && s[i] === ")") { depth--; continue; } - if (!depth) out += s[i]; - } - return out; -}; - -// Asserted here even though VS Code delivers the payload on the child's real stdin, where a -// top-level `;` is harmless. Two reasons it is still a requirement: -// -// * These command strings are kept deliberately identical across the Cursor, Claude Code and -// VS Code plugins, and on Cursor a top-level `;` severs the pipeline Cursor wraps around the -// command, silently allowing every skill. A string copied from here to there must -// not carry the defect with it. -// * "Harmless on today's client" is not a property to depend on. A one-simple-command hook works -// under every delivery model; one that relies on inheriting the shell's stdin does not. -check("no governed command has a top-level ';', '&&' or '||'", () => { - for (const event of GOVERNED_EVENTS) { - const top = topLevelOf(hooksFor(event)[0].command); - for (const op of [";", "&&", "||"]) { - assert(!top.includes(op), - `${event}: a top-level "${op}" makes the hook depend on inheriting the shell's stdin. ` + - `On Cursor that silently allows every skill. Keep it inside $( ).\n` + - ` top-level text: ${top.trim()}`); - } - } -}); - -await check("the launcher spawns npx.cmd through cmd.exe on Windows and does not pin a version", async () => { - const launcherUrl = pathToFileURL(path.join(pluginRoot, "scripts", "enforce-skill.mjs")).href; - const mod = await import(launcherUrl); - assert(typeof mod.resolveEnforceSpawn === "function", - "enforce-skill.mjs must export resolveEnforceSpawn"); - const nowSec = 1_700_000_000; - const common = { - env: { - JF_AGENT_GUARD_ENFORCE_DEADLINE: "1", - JFROG_AGENT_GUARD_VERSION: "0.0.0-master.1.gabc", - PATH: "C:\\Windows\\System32", - }, - nowSec, - }; - const win = mod.resolveEnforceSpawn({ ...common, preferOffline: true, platform: "win32" }); - assert(win.command === "npx.cmd", `Windows must spawn npx.cmd, got ${win.command}`); - assert(win.shell === "cmd.exe", - `Windows must pin cmd.exe so ComSpec cannot select PowerShell, got ${String(win.shell)}`); - assert(win.args.includes('"--prefer-offline"'), `missing offline flag: ${win.args}`); - assert(win.args.includes(`"${RELEASES_REGISTRY}"`), `registry must be quoted for cmd.exe: ${win.args}`); - assert(win.args.includes('"@jfrog/agent-guard"'), `package spec must stay unpinned: ${win.args}`); - assert(!win.args.some((arg) => arg.includes("@jfrog/agent-guard@")), - `JFROG_AGENT_GUARD_VERSION must not pin the spec: ${win.args}`); - assert(win.env.JF_AGENT_GUARD_ENFORCE_DEADLINE === String(nowSec + 25), - `deadline must replace an inherited value; got ${win.env.JF_AGENT_GUARD_ENFORCE_DEADLINE}`); - assert(win.env.npm_config_fetch_retries === "0", - "npm_config_fetch_retries=0 is required: npm's default of 2 backs off 10s then 60s"); - assert(win.env.npm_config_fetch_timeout === "10000", - `npm_config_fetch_timeout must be 10000 (npm's default is 300000), got ${win.env.npm_config_fetch_timeout}`); - const quoted = mod.resolveEnforceSpawn({ - preferOffline: false, - env: { JFROG_AGENT_GUARD_REPO: "https://example.invalid/npm/?a=1&b=2" }, - nowSec, - platform: "win32", - }); - assert(quoted.args.includes('"https://example.invalid/npm/?a=1&b=2"'), - `a registry & must stay inside one cmd.exe argument: ${quoted.args}`); - assert(!quoted.args.includes('"--prefer-offline"'), - `UserPromptSubmit must refresh the cache: ${quoted.args}`); - const unix = mod.resolveEnforceSpawn({ ...common, preferOffline: false, platform: "linux" }); - assert(unix.command === "npx" && unix.shell === false, - `Unix must spawn npx with no shell, got ${unix.command} shell=${String(unix.shell)}`); - assert(unix.args[unix.args.indexOf("--registry") + 1] === RELEASES_REGISTRY, - `default registry drifted: ${unix.args}`); - assert(!unix.args.includes("--prefer-offline"), `offline flag leaked: ${unix.args}`); - const stale = mod.resolveEnforceSpawn({ - preferOffline: false, - env: { JF_AGENT_GUARD_ENFORCE_DEADLINE: "1" }, - nowSec: Number.NaN, - platform: "linux", - }); - assert(stale.env.JF_AGENT_GUARD_ENFORCE_DEADLINE === undefined, - `an unusable clock must drop an inherited deadline, got ${stale.env.JF_AGENT_GUARD_ENFORCE_DEADLINE}`); -}); - // --------------------------------------------------------------------------- // Behavioural: execute the real hooks.json command string. // --------------------------------------------------------------------------- @@ -381,23 +278,6 @@ for (const event of GOVERNED_EVENTS) { `the deadline must be recomputed, not inherited; got ${seen.deadline}`); }); - // The deadline used to be $(date +%s) inside the hook string. That binary is absent from a - // minimal Windows PATH, and PowerShell cannot parse the substitution anyway. The launcher - // reads the clock in process, so a missing date(1) must not empty the deadline or drop stdin. - await check(`${event}: with no date(1) on PATH, the deadline is still in the future and the payload arrives`, async () => { - const record = stubNpx({ stdout: `{"continue":true}` }); - const payload = payloadFor(event); - const before = Math.floor(Date.now() / 1000); - const r = runHook(commandFor(event), payload, { noDate: true }); - assert(r.code === 0, `exit=${r.code} stderr=${r.stderr}`); - const seen = JSON.parse(readFileSync(record, "utf8")); - const deadline = Number(seen.deadline); - assert(Number.isFinite(deadline) && deadline > before, - `a missing date(1) must not block the clock; got ${JSON.stringify(seen.deadline)}`); - assert(seen.stdin === payload, - `losing date(1) must not cost the payload: agent-guard received ${seen.stdin.length} bytes, expected ${payload.length}`); - }); - await check(`${event}: forwards a deny verbatim and exits 0 (the JSON decides)`, async () => { const deny = event === "PreToolUse" ? `{"systemMessage":"blocked","hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny","permissionDecisionReason":"policy"}}` @@ -406,7 +286,7 @@ for (const event of GOVERNED_EVENTS) { const r = runHook(commandFor(event), payloadFor(event)); assert(r.code === 0, `a rendered verdict must exit 0 and let the JSON decide, got ${r.code}`); assert(r.stdout === deny, `stdout altered: ${r.stdout}`); - assert(blocks(r), "a deny payload must read as a block"); + assert(blocks(event, r), "a deny payload must read as a block"); }); await check(`${event}: an allow is forwarded as-is (explicit JSON, never silence)`, async () => { @@ -414,7 +294,7 @@ for (const event of GOVERNED_EVENTS) { const r = runHook(commandFor(event), payloadFor(event)); assert(r.code === 0 && r.stdout === `{"continue":true}`, `an allow must be explicit JSON: exit=${r.code} stdout=${r.stdout}`); - assert(!blocks(r), "continue:true must not read as a block"); + assert(!blocks(event, r), "continue:true must not read as a block"); // permissionDecision:"allow" would APPROVE the call, bypassing VS Code's own permission // prompt and overruling every other hook. assert(!r.stdout.includes("permissionDecision"), "an allow must carry no permission decision"); @@ -425,28 +305,32 @@ for (const event of GOVERNED_EVENTS) { const r = runHook(commandFor(event), payloadFor(event)); assert(r.stdout === "not json at all", `the hook must forward bytes verbatim and never rewrite a verdict: ${r.stdout}`); - assert(!blocks(r), "unparseable output is not a deny"); + assert(!blocks(event, r), "unparseable output is not a deny"); }); await check(`${event}: empty stdout with exit 0 stays empty`, async () => { stubNpx({ stdout: "", exitCode: 0 }); const r = runHook(commandFor(event), payloadFor(event)); assert(r.stdout === "", `the hook must not invent a verdict: ${r.stdout}`); - assert(!blocks(r), "silence is not a deny"); + assert(!blocks(event, r), "silence is not a deny"); }); - await check(`${event}: an agent-guard failure fails OPEN, not closed`, async () => { - stubNpx({ stdout: "", exitCode: 1 }); + // PreToolUse denies the tool call on any non-zero exit but 2, so failing open must be exactly + // exit 0 with `{}`. Partial output from the failed run is dropped, never spliced into `{}`. + await check(`${event}: an agent-guard failure fails OPEN with exit 0 and {}`, async () => { + stubNpx({ stdout: `{"hookSpecificOutput":{"perm`, exitCode: 1 }); const r = runHook(commandFor(event), payloadFor(event)); - assert(!blocks(r), + assert(r.code === 0 && r.stdout === "{}", `an internal failure must let the skill through, got exit=${r.code} stdout=${r.stdout}`); + assert(!blocks(event, r), "a failed run must not read as a block"); }); // With npx absent the guard never runs at all. That machine is not governed, so it must not be // punished. - await check(`${event}: npx missing entirely fails OPEN`, async () => { + await check(`${event}: npx missing entirely fails OPEN with exit 0 and {}`, async () => { const r = runHook(commandFor(event), payloadFor(event), { isolate: true }); - assert(!blocks(r), `a missing npx must fail open, got exit=${r.code} stdout=${r.stdout}`); + assert(r.code === 0 && r.stdout === "{}", + `a missing npx must fail open, got exit=${r.code} stdout=${r.stdout}`); }); await check(`${event}: agent-guard's own exit 2 still blocks`, async () => { From ad4feab381378ec18ad4c0942b68d498beacd1c3 Mon Sep 17 00:00:00 2001 From: nadav-dvir Date: Mon, 26 Oct 2026 15:57:12 +0200 Subject: [PATCH 3/3] [patch] Say why on stderr when skill governance fails open Every fail-open path in the launcher now writes one stderr line naming the cause and stating that the skill was not governed, so a silent allow is diagnosable. The README no longer claims a missing node fails open: the hook command fails before the launcher starts, and PreToolUse denies any exit other than 0. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- plugin/scripts/enforce-skill.mjs | 36 ++++++++++++++++++------- plugin/scripts/enforce-skill.test.mjs | 39 +++++++++++++++++++-------- scripts/validate-skill-governance.mjs | 4 +++ 4 files changed, 59 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 1e94445..734a909 100644 --- a/README.md +++ b/README.md @@ -267,7 +267,7 @@ When a policy block carries a waiver scope, the block message shows the command > unconfigured or unentitled user, so no setup is needed to opt out of the feature. - Set `JFROG_PLATFORM_URL` and `JFROG_ACCESS_TOKEN` (or configure the JFrog CLI — see [Authentication](#authentication)) and `JF_PROJECT` (the JFrog project the skill runs in). For an entitled account with credentials but no project, skills are **blocked** with a message telling you what to set; with no credentials at all they are **allowed**, per the table above. -- **Node.js (≥ 20) with `npx` on your `PATH`** — the hook resolves the Agent Guard through `npx`. Without it, governed actions are allowed unchecked. +- **Node.js (≥ 20) with `npx` on your `PATH`** — the hook resolves the Agent Guard through `npx`. If `npx` is missing or the Agent Guard fails, governed actions are allowed unchecked, and the hook writes the cause to stderr. That fail-open does **not** cover `node` itself: if `node` is missing or too old to run the launcher, the hook command fails before it starts, and VS Code denies every tool call (`PreToolUse` treats any exit other than 0 as a deny). - **Windows, macOS, and Linux.** The hook command is `node` plus `scripts/enforce-skill.mjs`. VS Code runs that string with PowerShell on a normal Windows session, which cannot parse a POSIX one-liner, so the launcher computes the deadline and starts `npx` itself (on Windows, npm's `npx-cli.js` run by `node` with no shell, falling back to `npx.cmd` through `cmd.exe`). Agent Guard still decides allow or deny. If `npx` cannot start or Agent Guard fails, the hook returns `{}` with exit 0 and the skill is allowed. - **Unlike the MCP alignment hook, this one does not pin the Agent Guard version.** It always resolves the latest published release, so a governance fix reaches you without waiting for a plugin release. `JFROG_AGENT_GUARD_REPO` still redirects the registry. - **Coverage limits.** `github.copilot.chat.skillTool.enabled` is **off by default**, and with it off Copilot registers no `skill` tool — it instructs the model to read the matching `SKILL.md` with its `read_file` tool instead. That call reaches the `PreToolUse` hook and the Agent Guard governs it, so a default install is covered through `read_file` rather than through the skill tool. What is *not* covered: skills contributed by other extensions (`contributes.chatSkills`) are not searched, and neither are folders added through `chat.agentSkillsLocations`. A skill's content can also be reached without a governed surface — through a subagent, a terminal command, or a text search — and hooks are a VS Code Preview feature that can be disabled organization-side with no signal. An absence of blocks never by itself means a skill was allowed. diff --git a/plugin/scripts/enforce-skill.mjs b/plugin/scripts/enforce-skill.mjs index 1f7185e..fa8ef45 100644 --- a/plugin/scripts/enforce-skill.mjs +++ b/plugin/scripts/enforce-skill.mjs @@ -8,7 +8,8 @@ // Exit codes: VS Code's PreToolUse fails closed on ANY non-zero exit, and exit 2 is // the only deliberate block. agent-guard exits 0 or 2, so both pass through with its // stdout. Every other outcome (spawn failure, signal, any other exit code) prints `{}` -// and exits 0, so a machine that cannot run the guard is not blocked by it. +// and exits 0, so a machine that cannot run the guard is not blocked by it. Each of +// those also writes one line to stderr naming the cause. import { spawn } from "node:child_process"; import { existsSync } from "node:fs"; @@ -24,6 +25,11 @@ import { const DEADLINE_SLACK_SEC = 25; export const FAIL_OPEN_OUTPUT = "{}"; +/** @param {string} cause */ +export function failOpenMessage(cause) { + return `[jfrog-agent-guard] skill governance did not run (${cause}); this skill was not governed.\n`; +} + /** * npm's npx entry point, installed next to node.exe by the Windows Node installer. * Running it with process.execPath needs no shell, unlike npx.cmd. @@ -117,12 +123,14 @@ export function resolveEnforceSpawn(opts = {}) { * spawnFn?: typeof spawn, * stdin?: NodeJS.ReadableStream, * stdout?: NodeJS.WritableStream, + * stderr?: NodeJS.WritableStream, * }} [opts] * @returns {Promise<0 | 2>} */ export function runEnforce(opts = {}) { const stdin = opts.stdin ?? process.stdin; const stdout = opts.stdout ?? process.stdout; + const stderr = opts.stderr ?? process.stderr; const spawnFn = opts.spawnFn ?? spawn; return new Promise((resolve) => { @@ -137,7 +145,12 @@ export function runEnforce(opts = {}) { } stdout.write(output, () => resolve(code)); }; - const failOpen = () => finish(0, FAIL_OPEN_OUTPUT); + /** @param {string} cause */ + const failOpen = (cause) => { + if (settled) return; + stderr.write(failOpenMessage(cause)); + finish(0, FAIL_OPEN_OUTPUT); + }; /** @type {ReturnType} */ let plan; @@ -150,8 +163,8 @@ export function runEnforce(opts = {}) { execPath: opts.execPath, exists: opts.exists, }); - } catch { - failOpen(); + } catch (error) { + failOpen(`could not build the npx command: ${error?.message ?? error}`); return; } @@ -164,15 +177,15 @@ export function runEnforce(opts = {}) { windowsHide: true, stdio: ["pipe", "pipe", "inherit"], }); - } catch { - failOpen(); + } catch (error) { + failOpen(`could not start ${plan.command}: ${error?.message ?? error}`); return; } - child.on("error", failOpen); + child.on("error", (error) => failOpen(`could not start ${plan.command}: ${error?.message ?? error}`)); if (!child.stdin || !child.stdout) { child.kill?.(); - failOpen(); + failOpen(`${plan.command} has no stdio pipes`); return; } /** @type {Buffer[]} */ @@ -184,7 +197,7 @@ export function runEnforce(opts = {}) { stdin.pipe(child.stdin); child.on("close", (code, signal) => { if (!signal && (code === 0 || code === 2)) finish(code, Buffer.concat(chunks)); - else failOpen(); + else failOpen(signal ? `npx was killed by ${signal}` : `npx exited with code ${code}`); }); }); } @@ -192,6 +205,9 @@ export function runEnforce(opts = {}) { if (isMainEntry(import.meta.url)) { runEnforce().then( (code) => process.exit(code), - () => process.stdout.write(FAIL_OPEN_OUTPUT, () => process.exit(0)), + (error) => { + process.stderr.write(failOpenMessage(`launcher error: ${error?.message ?? error}`)); + process.stdout.write(FAIL_OPEN_OUTPUT, () => process.exit(0)); + }, ); } diff --git a/plugin/scripts/enforce-skill.test.mjs b/plugin/scripts/enforce-skill.test.mjs index 87b4d08..ac80143 100644 --- a/plugin/scripts/enforce-skill.test.mjs +++ b/plugin/scripts/enforce-skill.test.mjs @@ -7,6 +7,7 @@ import { DEFAULT_AGENT_GUARD_NPM_REGISTRY } from "../modules/core/rewrite-mcp-js import { FAIL_OPEN_OUTPUT, bundledNpxCli, + failOpenMessage, quoteCmdArg, resolveEnforceSpawn, runEnforce, @@ -165,45 +166,55 @@ const exitWith = (code, { stdout = "", signal = null } = {}) => (child) => { const run = (spawnFn, extra = {}) => { const out = captureStdout(); + const err = captureStdout(); const done = runEnforce({ argv: [], env: {}, platform: "linux", nowSec: NOW, - spawnFn, stdin: Readable.from([PAYLOAD]), stdout: out.stream, ...extra, + spawnFn, stdin: Readable.from([PAYLOAD]), stdout: out.stream, stderr: err.stream, ...extra, }); - return done.then((code) => ({ code, stdout: out.text() })); + return done.then((code) => ({ code, stdout: out.text(), stderr: err.text() })); +}; + +/** A fail-open result: exit 0, `{}` on stdout, and exactly one stderr line naming the cause. */ +const assertFailOpen = (r, cause) => { + assert.equal(r.code, 0); + assert.equal(r.stdout, FAIL_OPEN_OUTPUT); + assert.equal(r.stderr.split("\n").filter(Boolean).length, 1, `one stderr line: ${r.stderr}`); + assert.match(r.stderr, /this skill was not governed/); + assert.match(r.stderr, cause); }; test("exit 0 passes the child's stdout through verbatim", async () => { const verdict = `{"continue":true}`; const r = await run(fakeSpawn(exitWith(0, { stdout: verdict })).spawnFn); - assert.deepEqual(r, { code: 0, stdout: verdict }); + assert.deepEqual(r, { code: 0, stdout: verdict, stderr: "" }); }); test("exit 0 with no stdout stays silent", async () => { const r = await run(fakeSpawn(exitWith(0)).spawnFn); - assert.deepEqual(r, { code: 0, stdout: "" }); + assert.deepEqual(r, { code: 0, stdout: "", stderr: "" }); }); test("exit 2 passes through with the child's stdout", async () => { const deny = `{"hookSpecificOutput":{"permissionDecision":"deny"}}`; const r = await run(fakeSpawn(exitWith(2, { stdout: deny })).spawnFn); - assert.deepEqual(r, { code: 2, stdout: deny }); + assert.deepEqual(r, { code: 2, stdout: deny, stderr: "" }); }); test("any other exit code fails open with {} and discards partial output", async () => { for (const code of [1, 127, 3]) { const r = await run(fakeSpawn(exitWith(code, { stdout: `{"hookSpecificOutput":{"perm` })).spawnFn); - assert.deepEqual(r, { code: 0, stdout: FAIL_OPEN_OUTPUT }, `exit ${code}`); + assertFailOpen(r, new RegExp(`npx exited with code ${code}`)); } }); test("a child killed by a signal fails open", async () => { const r = await run(fakeSpawn(exitWith(null, { signal: "SIGKILL" })).spawnFn); - assert.deepEqual(r, { code: 0, stdout: FAIL_OPEN_OUTPUT }); + assertFailOpen(r, /killed by SIGKILL/); }); test("a spawn that throws fails open", async () => { const r = await run(fakeSpawn(() => {}, { throwOnSpawn: true }).spawnFn); - assert.deepEqual(r, { code: 0, stdout: FAIL_OPEN_OUTPUT }); + assertFailOpen(r, /could not start npx: spawn EINVAL/); }); test("a spawn error event (npx missing) fails open, even if close follows", async () => { @@ -211,7 +222,7 @@ test("a spawn error event (npx missing) fails open, even if close follows", asyn child.emit("error", Object.assign(new Error("spawn npx ENOENT"), { code: "ENOENT" })); child.emit("close", -2, null); }).spawnFn); - assert.deepEqual(r, { code: 0, stdout: FAIL_OPEN_OUTPUT }); + assertFailOpen(r, /could not start npx: spawn npx ENOENT/); }); test("a spawn plan that cannot be built fails open without spawning", async () => { @@ -220,7 +231,7 @@ test("a spawn plan that cannot be built fails open without spawning", async () = platform: "win32", execPath: WIN_NODE, exists: () => false, env: { JFROG_AGENT_GUARD_REPO: "https://x/%PATH%/" }, }); - assert.deepEqual(r, { code: 0, stdout: FAIL_OPEN_OUTPUT }); + assertFailOpen(r, /could not build the npx command: cmd\.exe cannot carry/); assert.equal(calls.length, 0); }); @@ -229,7 +240,7 @@ test("an EPIPE on the child's stdin does not replace its exit 2", async () => { write(_chunk, _enc, cb) { cb(Object.assign(new Error("write EPIPE"), { code: "EPIPE" })); }, }); const r = await run(fakeSpawn(exitWith(2, { stdout: "blocked" }), { stdin: brokenStdin }).spawnFn); - assert.deepEqual(r, { code: 2, stdout: "blocked" }); + assert.deepEqual(r, { code: 2, stdout: "blocked", stderr: "" }); }); test("the hook payload reaches the child's stdin verbatim", async () => { @@ -249,3 +260,9 @@ test("the child gets piped stdout and inherited stderr", async () => { assert.equal(calls[0].options.shell, false); assert.ok(calls[0].args.includes("--prefer-offline")); }); + +test("failOpenMessage is one line naming the cause", () => { + const line = failOpenMessage("npx exited with code 1"); + assert.ok(line.endsWith("\n") && line.indexOf("\n") === line.length - 1); + assert.match(line, /npx exited with code 1/); +}); diff --git a/scripts/validate-skill-governance.mjs b/scripts/validate-skill-governance.mjs index 4032916..9923098 100644 --- a/scripts/validate-skill-governance.mjs +++ b/scripts/validate-skill-governance.mjs @@ -323,6 +323,8 @@ for (const event of GOVERNED_EVENTS) { assert(r.code === 0 && r.stdout === "{}", `an internal failure must let the skill through, got exit=${r.code} stdout=${r.stdout}`); assert(!blocks(event, r), "a failed run must not read as a block"); + assert(/npx exited with code 1\); this skill was not governed/.test(r.stderr), + `a fail-open must say why on stderr: ${r.stderr}`); }); // With npx absent the guard never runs at all. That machine is not governed, so it must not be @@ -331,6 +333,8 @@ for (const event of GOVERNED_EVENTS) { const r = runHook(commandFor(event), payloadFor(event), { isolate: true }); assert(r.code === 0 && r.stdout === "{}", `a missing npx must fail open, got exit=${r.code} stdout=${r.stdout}`); + assert(/could not start npx: .*ENOENT.*this skill was not governed/.test(r.stderr), + `a fail-open must say why on stderr: ${r.stderr}`); }); await check(`${event}: agent-guard's own exit 2 still blocks`, async () => {