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/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..5dbb773 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"; @@ -51,14 +51,31 @@ 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")); 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")); + +// 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) => { @@ -92,22 +109,42 @@ 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, 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], { + // 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: { - PATH: isolate ? nodeDir : `${binDir}:${nodeDir}`, + PATH: isolate ? nodeDir : `${binDir}:${noDate ? noDateDir : nodeDir}`, HOME: sandbox, CURSOR_PLUGIN_ROOT: pluginRoot, ...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 +248,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 +299,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 +338,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 @@ -297,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"}`