From 9d03a99295c5a2dff7b9814ca2eeae0456ce87d2 Mon Sep 17 00:00:00 2001 From: shmuelb Date: Wed, 2 Sep 2026 13:24:43 +0300 Subject: [PATCH 1/2] MLAI-1310 - Keep the governed hook one simple command so Cursor delivers the payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skill governance has never functioned on Cursor in any published version. It entered main already broken, in 4a92bc3 (#81): hooks.json carried no governance hooks before that commit, and every commit since has had the defect. Both governed surfaces allowed every skill without ever contacting the governance service, silently and at exit 0. On macOS and Linux Cursor does not write the event to the hook process's stdin. It base64s the JSON into the command string and pipes it in from a pipeline the spawned shell builds itself, while closing the child's own stdin: workbench.desktop.main.js R = `printf %s '${b64}' | base64 -d | ${command}` extensionHostProcess.js stdio: [ pipeStdin ? "pipe" : "ignore", "pipe", "pipe" ] Our command began `_JFAG_NOW=$(date +%s 2>/dev/null); …`, and that top-level `;` terminates Cursor's pipeline: base64 -d piped into a bare assignment that reads nothing, and npx ran as a separate command inheriting the shell's stdin — /dev/null. agent-guard read 0 bytes, could not classify the event, and rendered its no-opinion allow, which is indistinguishable from "this prompt was not a skill invocation". Computing the deadline inside a command substitution scopes the `;` and keeps the hook one simple command, so it stays the tail of Cursor's pipeline. The intent of the defensive clock read is preserved, not reverted: verified under /bin/sh, /bin/zsh and /bin/bash that a governed skill blocks and that the deadline still degrades to EMPTY when date(1) cannot be read. The validator could not catch this because it delivered the payload the way Claude Code does — on the shell's stdin — so all 34 checks passed against a hook that delivered nothing. runHook now reproduces Cursor's wrapper exactly, with fd 0 as /dev/null, which makes the existing stdin assertions load-bearing. Two checks are added: a static one rejecting any top-level `;`, `&&` or `||` in a governed command, and a behavioural one asserting the payload survives under every shell Cursor might pick. Against the previous hooks.json the suite now fails 8 ways. Verified against the published plugin restored byte-for-byte, swapping only these two command lines: before the fix, governed runs reached the governance service zero times; after it, a request arrives within the hook's own window. --- plugins/jfrog/hooks/hooks.json | 4 +- scripts/validate-skill-governance.mjs | 115 ++++++++++++++++++++++---- 2 files changed, 102 insertions(+), 17 deletions(-) diff --git a/plugins/jfrog/hooks/hooks.json b/plugins/jfrog/hooks/hooks.json index f8d6a18..01c90f1 100644 --- a/plugins/jfrog/hooks/hooks.json +++ b/plugins/jfrog/hooks/hooks.json @@ -13,14 +13,14 @@ ], "beforeSubmitPrompt": [ { - "command": "_JFAG_NOW=$(date +%s 2>/dev/null); npm_config_fetch_retries=0 npm_config_fetch_timeout=10000 JF_AGENT_GUARD_ENFORCE_DEADLINE=\"${_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 cursor", + "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 cursor", "timeout": 30, "failClosed": false } ], "preToolUse": [ { - "command": "_JFAG_NOW=$(date +%s 2>/dev/null); npm_config_fetch_retries=0 npm_config_fetch_timeout=10000 JF_AGENT_GUARD_ENFORCE_DEADLINE=\"${_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 cursor", + "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 cursor", "matcher": "Read", "timeout": 30, "failClosed": false diff --git a/scripts/validate-skill-governance.mjs b/scripts/validate-skill-governance.mjs index 7be6a15..7577bde 100644 --- a/scripts/validate-skill-governance.mjs +++ b/scripts/validate-skill-governance.mjs @@ -29,7 +29,7 @@ // on Cursor 3.4.20) — the direct analogue of CLAUDE_PLUGIN_ROOT. import { spawnSync } from "node:child_process"; -import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import process from "node:process"; @@ -59,6 +59,12 @@ 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")); +// `base64` for the same reason: Cursor delivers the event as `printf %s '' | base64 -d | +// ` (see runHook), so without it the harness would hand agent-guard an empty pipe and +// every stdin assertion would fail for a reason that has nothing to do with the hook. +const base64Bin = ["/usr/bin/base64", "/bin/base64"].find((p) => existsSync(p)); +if (!base64Bin) throw new Error("base64 not found; cannot reproduce Cursor's payload delivery"); +symlinkSync(base64Bin, path.join(nodeDir, "base64")); const failures = []; const check = async (label, fn) => { @@ -92,12 +98,32 @@ process.stdin.on("end", () => { return record; } -// Run a hook command the way Cursor does: the string from hooks.json handed to a shell, the event -// JSON on stdin, and CURSOR_PLUGIN_ROOT set as Cursor sets it. `isolate` drops the stub from PATH, -// which is how "npx is not installed at all" is reproduced. -function runHook(command, payload, { isolate = false, extraEnv = {} } = {}) { - const result = spawnSync(SH, ["-c", command], { - input: Buffer.from(payload), +// Deliver the payload EXACTLY as Cursor does. How the event ARRIVES is the part most likely to +// break, and it is not the way Claude Code does it: on macOS and Linux Cursor never writes the +// event to the hook process's stdin. It base64s the JSON into the command string, pipes it in +// from a pipeline the spawned shell builds itself, and closes the child's own stdin: +// +// workbench.desktop.main.js R = `printf %s '${b64}' | base64 -d | ${command}` +// extensionHostProcess.js stdio: [ pipeStdin ? "pipe" : "ignore", "pipe", "pipe" ] +// // hooks never pass pipeStdin, so fd 0 is /dev/null +// +// Two consequences, and this function exists to make both testable: +// +// * our command runs as the TAIL OF A PIPELINE, so a top-level `;`, `&&` or `||` inside it +// severs the payload; agent-guard then reads /dev/null and renders its no-opinion allow. +// That is MLAI-1310 — every skill allowed, silently, exit 0. +// * handing the payload to the shell's own stdin instead, as this helper used to, exercises a +// delivery path Cursor never uses. All 34 checks below passed that way against a hook that +// delivered nothing at all. +// +// `isolate` drops the stub from PATH, which is how "npx is not installed at all" is reproduced. +function runHook(command, payload, { isolate = false, extraEnv = {}, shell = SH } = {}) { + const b64 = Buffer.from(payload).toString("base64"); + const wrapped = `printf %s '${b64}' | base64 -d | ${command}`; + const result = spawnSync(shell, ["-c", wrapped], { + // fd 0 is /dev/null, exactly as Cursor leaves it. A hook that only works because the + // harness fed it stdin does not work in Cursor. + stdio: ["ignore", "pipe", "pipe"], encoding: "buffer", timeout: 30_000, env: { @@ -107,7 +133,7 @@ function runHook(command, payload, { isolate = false, extraEnv = {} } = {}) { ...extraEnv, }, }); - if (result.error) throw new Error(`could not run the hook via ${SH}: ${result.error.message}`); + if (result.error) throw new Error(`could not run the hook via ${shell}: ${result.error.message}`); return { code: result.status, stdout: result.stdout ? result.stdout.toString() : "", @@ -211,10 +237,17 @@ for (const step of GOVERNED_STEPS) { const h = entriesFor(step)[0]; assert(/_JFAG_NOW=\$\(date \+%s 2>\/dev\/null\);/.test(h.command), `${step} must read the clock defensively, tolerating an absent date(1)`); - assert(h.command.includes('JF_AGENT_GUARD_ENFORCE_DEADLINE="${_JFAG_NOW:+$((_JFAG_NOW + 25))}"'), - `${step} must compute an absolute deadline at invocation time, 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`); + // The whole computation lives INSIDE a command substitution. That is not cosmetic: the + // clock read needs a ';' to separate it from the expansion, and at top level that ';' would + // sever the payload pipeline Cursor wraps around this command (MLAI-1310). Scoping it here + // keeps the hook one simple command while preserving the degrade-to-empty behaviour. + assert(h.command.includes( + 'JF_AGENT_GUARD_ENFORCE_DEADLINE="$(_JFAG_NOW=$(date +%s 2>/dev/null); ' + + 'echo ${_JFAG_NOW:+$((_JFAG_NOW + 25))})"'), + `${step} 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), `${step} must not fall back to an inherited value: an absolute instant inherited from an ` + `earlier process pins every later invocation to the past`); @@ -255,9 +288,38 @@ check("every hook command is valid POSIX sh", () => { } }); -// --------------------------------------------------------------------------- -// Behavioural: execute the real hooks.json command string. -// --------------------------------------------------------------------------- +// 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; +}; + +// MLAI-1310, the regression this file failed to catch. Cursor appends our command to a pipeline +// it builds — `printf %s '' | base64 -d | ` — so a governed command must be ONE +// simple command. A top-level `;`, `&&` or `||` ends that pipeline, and agent-guard then reads +// the shell's stdin, which Cursor set to /dev/null: 0 bytes, no classifiable event, and a +// no-opinion ALLOW at exit 0 that is indistinguishable from "this prompt was not a skill". +// +// Asserted statically as well as behaviourally below, because this names the property and fails +// with the offending text instead of a mystery allow. +check("no governed command has a top-level ';', '&&' or '||' (it is the tail of Cursor's pipeline)", () => { + for (const step of GOVERNED_STEPS) { + const top = topLevelOf(commandFor(step)); + for (const op of [";", "&&", "||"]) { + assert(!top.includes(op), + `${step}: a top-level "${op}" severs the payload pipeline Cursor builds, so agent-guard ` + + `reads /dev/null and silently allows (MLAI-1310). Keep it inside $( ).\n` + + ` top-level text: ${top.trim()}`); + } + } +}); // A payload shaped like the surface actually sends, so a check cannot pass by feeding preToolUse's // event to the prompt hook. @@ -265,6 +327,29 @@ const payloadFor = (step) => step === "preToolUse" ? `{"hook_event_name":"preToolUse","tool_name":"Read","tool_input":{"file_path":"/a/b/SKILL.md"}}` : `{"hook_event_name":"beforeSubmitPrompt","prompt":"/demo-skill"}`; +// Cursor spawns `process.env.SHELL || "/bin/sh"` with -c, so the command must survive whichever +// shell the user happens to have. Shells absent from the runner are skipped rather than failed. +check("the payload survives Cursor's pipeline under every shell Cursor may pick", () => { + const shells = ["/bin/sh", "/bin/bash", "/bin/zsh"].filter((s) => existsSync(s)); + assert(shells.length > 0, "no shell found to test with"); + for (const step of GOVERNED_STEPS) { + for (const shell of shells) { + const record = stubNpx({ stdout: "{}" }); + const payload = payloadFor(step); + const r = runHook(commandFor(step), payload, { shell }); + assert(r.code === 0, `${step} under ${shell}: exit=${r.code} stderr=${r.stderr}`); + const seen = JSON.parse(readFileSync(record, "utf8")); + assert(seen.stdin === payload, + `${step} under ${shell}: agent-guard received ${seen.stdin.length} bytes, expected ` + + `${payload.length}. The payload is not reaching it.`); + } + } +}); + +// --------------------------------------------------------------------------- +// Behavioural: execute the real hooks.json command string. +// --------------------------------------------------------------------------- + // Run every behavioural check against BOTH governed surfaces, not just preToolUse. The // byte-identical check above already makes divergence loud, but it only holds while it runs first; // looping here means a future hook that stops being identical is still exercised on its own terms From 46027fd028818671c9ec86c49dd9a995622387c9 Mon Sep 17 00:00:00 2001 From: shmuelb Date: Thu, 3 Sep 2026 14:49:59 +0300 Subject: [PATCH 2/2] MLAI-1310 - Bump the plugin manifests, and execute the deadline degrade path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback. The manifests stay at 0.6.3, which is already tagged, so a [patch] merge would fail the release job's existing-tag guard before publishing anything. Both carriers move to 0.6.4 together; they were already in sync, so this is one bump, not a mismatch. The sandbox comment claimed a PATH without date(1) "would silently yield $(( + 25)) = 25 - an epoch in 1970". That describes the pre-c4cb1d1 inline form. The command this repo has shipped since #81 carries the ${_JFAG_NOW:+…} guard and degrades to an EMPTY deadline instead, which agent-guard ignores in favour of its own budget. The comment has been wrong since it was written; it sits in the block this change edits. The degrade itself was asserted only in text: every behavioural run kept date(1) on PATH, isolate mode included, so the guard was never executed. That is the same class of gap this PR exists to close - the suite asserted the payload was forwarded while the hook forwarded nothing - so the last text-only assertion is closed here. A second sandbox PATH without date(1) drives the real command and asserts the deadline is EMPTY and the payload still arrives. The check is load-bearing, not decorative: swapping the command back to the inline $(($(date +%s) + 25)) form fails it on both surfaces, because that form yields 25 rather than empty, which would floor agent-guard's budget at 500ms and block every skill. 41 checks pass; validate-template and the 23 unit tests are unaffected. --- .cursor-plugin/marketplace.json | 2 +- plugins/jfrog/.cursor-plugin/plugin.json | 2 +- scripts/validate-skill-governance.mjs | 43 ++++++++++++++++++++---- 3 files changed, 39 insertions(+), 8 deletions(-) diff --git a/.cursor-plugin/marketplace.json b/.cursor-plugin/marketplace.json index d38e6b9..14a65c1 100644 --- a/.cursor-plugin/marketplace.json +++ b/.cursor-plugin/marketplace.json @@ -6,7 +6,7 @@ }, "metadata": { "description": "JFrog Platform plugins for Cursor", - "version": "0.6.3", + "version": "0.6.4", "pluginRoot": "plugins" }, "plugins": [ diff --git a/plugins/jfrog/.cursor-plugin/plugin.json b/plugins/jfrog/.cursor-plugin/plugin.json index b2ea641..2be8256 100644 --- a/plugins/jfrog/.cursor-plugin/plugin.json +++ b/plugins/jfrog/.cursor-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "jfrog", "displayName": "JFrog Platform", - "version": "0.6.3", + "version": "0.6.4", "description": "JFrog Platform integration with MCP, security skills, Agent Package Resolution, supply-chain best practices, and JFrog Agent Guard governance for adding, removing, and listing MCP servers.", "author": { "name": "JFrog", diff --git a/scripts/validate-skill-governance.mjs b/scripts/validate-skill-governance.mjs index 7577bde..5dbb773 100644 --- a/scripts/validate-skill-governance.mjs +++ b/scripts/validate-skill-governance.mjs @@ -51,10 +51,13 @@ mkdirSync(binDir, { recursive: true }); // unreachable. Using node's own directory would defeat the "npx is missing" check, because the // real npx sits right beside node — that check would then reach the network instead of exercising // the 127 path. -// `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 it beside node (not by adding /bin to PATH) preserves the isolate mode, -// where npx must stay unreachable. +// `date` lives here too, so the DEFAULT mode exercises the real deadline computation rather +// than its degraded form. With `date` absent the hook passes an EMPTY deadline — measured, and +// the point of the `${_JFAG_NOW:+…}` guard — which agent-guard ignores in favour of its own +// budget. That is the safe direction, but it is not the path most checks mean to test, so the +// two are separated: `noDateDir` below drops `date` for the one check that asserts the degrade. +// Keeping it beside node (not by adding /bin to PATH) preserves the isolate mode, where npx +// must stay unreachable. const nodeDir = path.join(sandbox, "node-only"); mkdirSync(nodeDir, { recursive: true }); symlinkSync(process.execPath, path.join(nodeDir, "node")); @@ -66,6 +69,14 @@ const base64Bin = ["/usr/bin/base64", "/bin/base64"].find((p) => existsSync(p)); if (!base64Bin) throw new Error("base64 not found; cannot reproduce Cursor's payload delivery"); symlinkSync(base64Bin, path.join(nodeDir, "base64")); +// The same directory WITHOUT `date`, for the one check that asserts the degrade path. Built as +// its own directory rather than by unlinking `date` between runs, so the checks stay order- +// independent. +const noDateDir = path.join(sandbox, "node-only-nodate"); +mkdirSync(noDateDir, { recursive: true }); +symlinkSync(process.execPath, path.join(noDateDir, "node")); +symlinkSync(base64Bin, path.join(noDateDir, "base64")); + const failures = []; const check = async (label, fn) => { try { await fn(); console.log(` ok ${label}`); } @@ -117,7 +128,7 @@ process.stdin.on("end", () => { // delivered nothing at all. // // `isolate` drops the stub from PATH, which is how "npx is not installed at all" is reproduced. -function runHook(command, payload, { isolate = false, extraEnv = {}, shell = SH } = {}) { +function runHook(command, payload, { isolate = false, noDate = false, extraEnv = {}, shell = SH } = {}) { const b64 = Buffer.from(payload).toString("base64"); const wrapped = `printf %s '${b64}' | base64 -d | ${command}`; const result = spawnSync(shell, ["-c", wrapped], { @@ -127,7 +138,7 @@ function runHook(command, payload, { isolate = false, extraEnv = {}, shell = SH encoding: "buffer", timeout: 30_000, env: { - PATH: isolate ? nodeDir : `${binDir}:${nodeDir}`, + PATH: isolate ? nodeDir : `${binDir}:${noDate ? noDateDir : nodeDir}`, HOME: sandbox, CURSOR_PLUGIN_ROOT: pluginRoot, ...extraEnv, @@ -382,6 +393,26 @@ for (const step of GOVERNED_STEPS) { `the deadline must be recomputed, not inherited; got ${seen.deadline}`); }); + // The degrade branch, EXECUTED rather than asserted in text. The static check above proves the + // command contains the `${_JFAG_NOW:+…}` guard; only running it with no `date` on PATH proves + // the guard does what the guard is for. That distinction is the whole reason this PR exists: + // the suite asserted the payload was forwarded, textually, while the hook forwarded nothing. + // + // 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))` yields + // as `$(( + 25))` = 25, an instant in 1970) floors that budget at 500ms and blocks every skill. + await check(`${step}: with no date(1) on PATH, the deadline degrades to EMPTY and the payload still arrives`, async () => { + const record = stubNpx({ stdout: "{}" }); + const payload = payloadFor(step); + const r = runHook(commandFor(step), 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)}`); + assert(seen.stdin === payload, + `losing date(1) must not cost the payload: agent-guard received ${seen.stdin.length} bytes, expected ${payload.length}`); + }); + await check(`${step}: forwards a deny verdict's stdout verbatim and exits 0 (the JSON decides)`, async () => { const deny = step === "preToolUse" ? `{"permission":"deny","user_message":"blocked"}`