diff --git a/docs/PI_EXTENSION.md b/docs/PI_EXTENSION.md index 9b29c0682..7991f2edc 100644 --- a/docs/PI_EXTENSION.md +++ b/docs/PI_EXTENSION.md @@ -48,6 +48,14 @@ Equivalent CLI: - `yaraRulesDir`: optional directory of extra YARA rules. - `verbose`: optional detailed progress. +Inputs inside the session's working directory run without a prompt. Remote targets +and external paths require confirmation; redirected aliases show their resolved +destination too. Print and JSON sessions reject these requests because they cannot +show a dialog. Use TUI or RPC mode, move the skill into the working directory, or +run the CLI directly. Local targets retain the CLI's refusal of symlinked paths. +Missing rule directories fail before scanning, and YARA `include` directives are +disabled: put self-contained rule files in the selected directory. + ## LLM-backed analysis Static scan is default. To use semantic LLM analysis, configure provider credentials in your shell before launching Pi, then call the tool with `noLlm=false` and a provider. diff --git a/extensions/skillspector.ts b/extensions/skillspector.ts index 99a7acd40..6d081cf19 100644 --- a/extensions/skillspector.ts +++ b/extensions/skillspector.ts @@ -1,4 +1,4 @@ -import type { ExtensionAPI } from "@earendil-works/pi-coding-agent"; +import type { ExtensionAPI, ExtensionContext } from "@earendil-works/pi-coding-agent"; import { StringEnum } from "@earendil-works/pi-ai"; import { Type, type Static } from "typebox"; import { chmodSync, constants, copyFileSync, existsSync, lstatSync, mkdtempSync, realpathSync, renameSync, rmSync } from "node:fs"; @@ -7,7 +7,7 @@ import { basename, dirname, isAbsolute, join, relative, resolve, sep } from "nod import { fileURLToPath } from "node:url"; const scanSchema = Type.Object({ - target: Type.String({ description: "Path, URL, zip, Git repo, or SKILL.md to scan." }), + target: Type.String({ description: "Path, URL, zip, Git repo, or SKILL.md to scan. External paths and remote targets require user confirmation." }), format: Type.Optional( StringEnum(["terminal", "json", "markdown", "sarif"] as const, { description: "SkillSpector output format. Defaults to terminal.", @@ -21,22 +21,12 @@ const scanSchema = Type.Object({ }), ), model: Type.Optional(Type.String({ description: "Optional model override." })), - yaraRulesDir: Type.Optional(Type.String({ description: "Optional extra YARA rules directory." })), + yaraRulesDir: Type.Optional(Type.String({ description: "Optional extra YARA rules directory. External paths require user confirmation." })), verbose: Type.Optional(Type.Boolean({ description: "Show detailed progress." })), }); type SkillSpectorScanParams = Static; -function isLikelyUrl(value: string): boolean { - return /^[a-z][a-z0-9+.-]*:\/\//i.test(value) || /^[\w.-]+\/[\w.-]+(?:\.git)?(?:@.+)?$/i.test(value); -} - -function resolveMaybePath(ctxCwd: string, value?: string): string | undefined { - if (!value) return undefined; - if (isLikelyUrl(value)) return value; - return isAbsolute(value) ? value : resolve(ctxCwd, value); -} - function redactSecrets(value: string): string { return value .replace(/(sk-ant-[A-Za-z0-9_-]{12,})/g, "[REDACTED_ANTHROPIC_KEY]") @@ -71,6 +61,85 @@ function isWithin(root: string, path: string): boolean { return rel !== ".." && !rel.startsWith(`..${sep}`) && !isAbsolute(rel); } +async function approveScanInputs( + params: SkillSpectorScanParams, + ctx: ExtensionContext, + signal?: AbortSignal, +): Promise { + signal?.throwIfAborted(); + const workspace = realpathSync(ctx.cwd); + const prepared = { ...params }; + const localPaths: Array<{ field: "target" | "yaraRulesDir"; input: string; resolved?: string }> = []; + const requests: string[] = []; + const readRequest = (field: string, path: string) => + `Read external ${field === "target" ? "scan target" : "YARA rules"}: ${JSON.stringify(path)}`; + async function approve(requests: string[]): Promise { + if (!requests.length) return; + signal?.throwIfAborted(); + if (!ctx.hasUI) throw new Error("External scan inputs require user confirmation in an interactive or RPC session."); + const approved = await ctx.ui.confirm( + "Allow SkillSpector external access?", + `${requests.join("\n")}\n\nScanned content and matching rule text can appear in the agent conversation.`, + { signal }, + ); + signal?.throwIfAborted(); + if (!approved) throw new Error("SkillSpector external access was not approved."); + } + for (const field of ["target", "yaraRulesDir"] as const) { + const value = params[field]?.trim(); + if (field === "yaraRulesDir" && !value) { + prepared[field] = undefined; + continue; + } + if (!value) throw new Error("A scan target is required."); + // Match the CLI's remote forms. A local owner/repo path is not a URL. + const remote = !isAbsolute(value) && (value.startsWith("https://") || (value.startsWith("git@") && value.endsWith(".git"))); + if (remote) { + if (field !== "target") throw new Error("YARA rules must be a local directory."); + prepared[field] = value; + requests.push(`Fetch remote scan target: ${JSON.stringify(value)}`); + } else { + const input = resolve(ctx.cwd, value); + // Ask before resolving external paths, which can probe the host or access + // a Windows network share even when the file is never opened. + if (!isWithin(resolve(ctx.cwd), input)) { + localPaths.push({ field, input }); + requests.push(readRequest(field, input)); + } else { + let resolved: string; + try { + resolved = realpathSync(input); + } catch { + throw new Error(`Could not resolve ${field === "target" ? "scan target" : "YARA rules directory"}. Check that it exists and is accessible.`); + } + localPaths.push({ field, input, resolved }); + if (!isWithin(workspace, resolved)) requests.push(readRequest(field, resolved)); + } + } + } + await approve(requests); + const aliasRequests: string[] = []; + for (const path of localPaths) { + try { + path.resolved ??= realpathSync(path.input); + } catch { + throw new Error(`Could not resolve ${path.field === "target" ? "scan target" : "YARA rules directory"}. Check that it exists and is accessible.`); + } + if (!isWithin(resolve(ctx.cwd), path.input) && !isWithin(workspace, path.resolved) && path.resolved !== path.input) { + aliasRequests.push(readRequest(path.field, path.resolved)); + } + // Keep the original target path so the CLI can enforce its no-symlink + // input policy. YARA directories are canonicalised by the CLI too. + prepared[path.field] = path.field === "target" ? path.input : path.resolved; + } + await approve(aliasRequests); + signal?.throwIfAborted(); + if (realpathSync(ctx.cwd) !== workspace || localPaths.some(({ input, resolved }) => realpathSync(input) !== resolved)) { + throw new Error("Scan input path changed while awaiting confirmation."); + } + return prepared; +} + function reportOutputPath(cwd: string, value?: string): string | undefined { if (!value) return undefined; const output = resolve(cwd, value); @@ -102,8 +171,8 @@ function publishReport(source: string, destination: string): void { } } -function buildScanArgs(params: SkillSpectorScanParams, cwd: string, output?: string): string[] { - const args = ["scan", resolveMaybePath(cwd, params.target) ?? params.target]; +function buildScanArgs(params: SkillSpectorScanParams, output?: string): string[] { + const args = ["scan", params.target]; args.push("--format", params.format ?? "terminal"); const noLlm = params.noLlm ?? true; @@ -111,8 +180,7 @@ function buildScanArgs(params: SkillSpectorScanParams, cwd: string, output?: str if (output) args.push("--output", output); - const yaraRulesDir = resolveMaybePath(cwd, params.yaraRulesDir); - if (yaraRulesDir) args.push("--yara-rules-dir", yaraRulesDir); + if (params.yaraRulesDir) args.push("--yara-rules-dir", params.yaraRulesDir); if (params.verbose) args.push("--verbose"); return args; @@ -132,10 +200,11 @@ export default function (pi: ExtensionAPI) { async execute(_toolCallId, params, signal, onUpdate, ctx) { const bin = findSkillSpectorBin(); const outputPath = reportOutputPath(ctx.cwd, params.output); + const prepared = await approveScanInputs(params, ctx, signal); const reportDir = outputPath ? mkdtempSync(join(tmpdir(), "skillspector-report-")) : undefined; const reportPath = reportDir ? join(reportDir, "report") : undefined; try { - const args = buildScanArgs(params, ctx.cwd, reportPath); + const args = buildScanArgs(prepared, reportPath); const env: Record = {}; if (params.provider) env.SKILLSPECTOR_PROVIDER = params.provider; diff --git a/src/skillspector/nodes/analyzers/static_yara.py b/src/skillspector/nodes/analyzers/static_yara.py index 9879cd3c0..c9b2d9d40 100644 --- a/src/skillspector/nodes/analyzers/static_yara.py +++ b/src/skillspector/nodes/analyzers/static_yara.py @@ -533,7 +533,7 @@ def _compile_rules( """ _enforce_rule_load_deadline() try: - compiled = yara.compile(sources=sources) + compiled = yara.compile(sources=sources, includes=False) _enforce_rule_load_deadline() return compiled, 0 except yara.SyntaxError: @@ -545,7 +545,7 @@ def _compile_rules( for ns, source in sources.items(): _enforce_rule_load_deadline() try: - yara.compile(source=source) + yara.compile(source=source, includes=False) good[ns] = source except (yara.SyntaxError, yara.Error) as exc: skipped += 1 @@ -559,7 +559,7 @@ def _compile_rules( ) _enforce_rule_load_deadline() - compiled = yara.compile(sources=good) if good else None + compiled = yara.compile(sources=good, includes=False) if good else None _enforce_rule_load_deadline() return compiled, skipped diff --git a/tests/nodes/analyzers/test_static_yara.py b/tests/nodes/analyzers/test_static_yara.py index cebfa1daf..8038f8e74 100644 --- a/tests/nodes/analyzers/test_static_yara.py +++ b/tests/nodes/analyzers/test_static_yara.py @@ -2409,6 +2409,37 @@ def test_build_namespace_map_skips_malformed_encoded_rules(self, tmp_path): assert "invalid" not in ns_map assert skipped == 1 + @pytest.mark.parametrize("relative", [False, True]) + def test_external_includes_are_rejected_without_dropping_valid_rules( + self, tmp_path, monkeypatch, relative + ): + rules_dir = tmp_path / "rules" + rules_dir.mkdir() + outside = tmp_path / "private.yar" + outside.write_text('rule external_private_rule { strings: $a = "Local" condition: $a }') + include = "../private.yar" if relative else str(outside) + (rules_dir / "include.yar").write_text(f'include "{include}"') + (rules_dir / "good.yar").write_text( + 'rule approved_local_rule { strings: $a = "Local" condition: $a }' + ) + monkeypatch.chdir(rules_dir) + monkeypatch.setattr(static_yara, "_rule_cache", None) + monkeypatch.setattr(static_yara, "_BUILTIN_RULES_DIR", tmp_path / "empty_builtin") + result = static_yara.node( + { + "components": ["SKILL.md"], + "file_cache": {"SKILL.md": "Local sample skill."}, + "yara_rules_dir": str(rules_dir), + } + ) + assert any("approved_local_rule" in f.message for f in result["findings"]) + assert not any("external_private_rule" in f.message for f in result["findings"]) + assert result["analyzer_status_events"][0]["status"] != "completed" + assert any( + event.get("reason_code") == LedgerReason.READ_ERROR + for event in result["inspection_ledger"] + ) + def test_malformed_rule_is_reported_not_silently_dropped(self, tmp_path, monkeypatch): """A custom rule that can't compile must not report a clean, SAFE scan (#554). diff --git a/tests/unit/test_pi_extension.mjs b/tests/unit/test_pi_extension.mjs index 5f349c62e..e6c88ca2b 100644 --- a/tests/unit/test_pi_extension.mjs +++ b/tests/unit/test_pi_extension.mjs @@ -26,7 +26,7 @@ registerHooks({ }, }); -async function setup(t, exec) { +async function setup(t, exec, confirm = async () => false) { const root = realpathSync(mkdtempSync(join(tmpdir(), "skillspector-pi-test-"))); const workspace = join(root, "workspace"); const install = join(root, "install"); @@ -34,6 +34,7 @@ async function setup(t, exec) { const source = process.env.SKILLSPECTOR_EXTENSION_SOURCE ?? new URL("../../extensions/skillspector.ts", import.meta.url); mkdirSync(workspace); + writeFileSync(join(workspace, "SKILL.md"), "---\nname: synthetic-skill\ndescription: Local permission test\n---\nA benign skill."); mkdirSync(join(install, "extensions"), { recursive: true }); mkdirSync(dirname(bin), { recursive: true }); writeFileSync(bin, "unused mocked executable"); @@ -49,6 +50,7 @@ async function setup(t, exec) { const { default: register } = await import(pathToFileURL(join(install, "extensions/skillspector.ts"))); let tool; const calls = []; + const prompts = []; register({ registerTool(registered) { tool = registered; }, async exec(command, args, options) { @@ -61,8 +63,22 @@ async function setup(t, exec) { }, }); return { - root, workspace, bin, calls, - scan: (params = {}) => tool.execute("scan", { target: "./SKILL.md", ...params }, undefined, undefined, { cwd: workspace }), + root, workspace, bin, calls, prompts, + scan: (params = {}, context = {}, signal) => tool.execute("scan", { target: "./SKILL.md", ...params }, signal, undefined, { + cwd: workspace, + hasUI: true, + ui: { async confirm(title, message, options) { + prompts.push({ title, message }); + return new Promise((resolve, reject) => { + const signal = options?.signal; + const abort = () => reject(signal.reason); + if (signal?.aborted) return abort(); + signal?.addEventListener("abort", abort, { once: true }); + Promise.resolve(confirm({ root, workspace })).then(resolve, reject).finally(() => signal?.removeEventListener("abort", abort)); + }); + } }, + ...context, + }), }; } @@ -70,7 +86,7 @@ test("uses installed absolute executable and preserves scan arguments without ou const ctx = await setup(t); await ctx.scan({ provider: "anthropic", model: "synthetic-model", verbose: true }); assert.equal(ctx.calls[0].command, ctx.bin); - assert.deepEqual(ctx.calls[0].args, ["scan", "./SKILL.md", "--format", "terminal", "--no-llm", "--verbose"]); + assert.deepEqual(ctx.calls[0].args, ["scan", join(ctx.workspace, "SKILL.md"), "--format", "terminal", "--no-llm", "--verbose"]); assert.deepEqual(ctx.calls[0].options.env, { SKILLSPECTOR_PROVIDER: "anthropic", SKILLSPECTOR_MODEL: "synthetic-model" }); assert.equal(ctx.calls[0].options.cwd, ctx.workspace); }); @@ -132,7 +148,7 @@ for (const [noLlm, timeout] of [[undefined, 120000], [true, 120000], [false, 630 } test("uses an absolute operator override and preserves URL targets", async (t) => { - const ctx = await setup(t); + const ctx = await setup(t, undefined, async () => true); process.env.SKILLSPECTOR_BIN = join(ctx.root, "custom-cli"); writeFileSync(process.env.SKILLSPECTOR_BIN, "unused"); await ctx.scan({ target: "https://example.test/skill", noLlm: false, format: "json" }); @@ -140,6 +156,105 @@ test("uses an absolute operator override and preserves URL targets", async (t) = assert.deepEqual(ctx.calls[0].args, ["scan", "https://example.test/skill", "--format", "json"]); }); +test("keeps nested local paths local and scans workspace inputs without a prompt", async (t) => { + const ctx = await setup(t); + mkdirSync(join(ctx.workspace, "owner/repo"), { recursive: true }); + mkdirSync(join(ctx.workspace, "rules")); + for (const target of ["owner/repo", join(ctx.workspace, "SKILL.md"), " ./SKILL.md "]) { + await ctx.scan({ target, yaraRulesDir: "rules" }, { hasUI: false }); + assert.equal(ctx.calls.at(-1).args[1], realpathSync(resolve(ctx.workspace, target.trim()))); + assert.equal(ctx.calls.at(-1).args.at(-1), join(ctx.workspace, "rules")); + } + assert.equal(ctx.prompts.length, 0); +}); + +test("requires approval before exposing external targets or YARA rules to the CLI", async (t) => { + const ctx = await setup(t); + const secret = join(ctx.root, "private.md"); + const rules = join(ctx.root, "rules"); + writeFileSync(secret, "synthetic private content"); + mkdirSync(rules); + symlinkSync(secret, join(ctx.workspace, "linked.md")); + symlinkSync(rules, join(ctx.workspace, "rules")); + for (const params of [ + { target: secret }, + { target: "../private.md" }, + { target: "linked.md" }, + { yaraRulesDir: rules }, + { yaraRulesDir: "../rules" }, + { yaraRulesDir: "rules" }, + { target: "https://example.test/skill.zip" }, + { target: "git@github.com:owner/repo.git" }, + ]) { + await assert.rejects(ctx.scan(params), /not approved/); + await assert.rejects(ctx.scan(params, { hasUI: false }), /require user confirmation/); + } + assert.equal(ctx.calls.length, 0); + assert.equal(ctx.prompts.length, 8); + assert.equal(readFileSync(secret, "utf8"), "synthetic private content"); + assert.ok(ctx.prompts[2].message.includes(JSON.stringify(secret))); + assert.ok(ctx.prompts[5].message.includes(JSON.stringify(rules))); +}); + +test("approves canonical external scope and preserves the CLI target symlink policy", async (t) => { + const ctx = await setup(t, undefined, async () => true); + writeFileSync(join(ctx.root, "private.md"), "synthetic private content"); + mkdirSync(join(ctx.root, "rules")); + symlinkSync(ctx.root, join(ctx.workspace, "external")); + await ctx.scan({ target: "external/private.md", yaraRulesDir: "external/rules" }); + assert.equal(ctx.prompts.length, 1); + assert.match(ctx.prompts[0].message, /Read external scan target:/); + assert.match(ctx.prompts[0].message, /Read external YARA rules:/); + assert.equal(ctx.calls[0].args[1], join(ctx.workspace, "external/private.md")); + assert.equal(ctx.calls[0].args.at(-1), join(ctx.root, "rules")); +}); + +test("treats local git@ paths as canonical local reads", async (t) => { + const ctx = await setup(t, undefined, async () => true); + mkdirSync(join(ctx.workspace, "git@notes")); + writeFileSync(join(ctx.workspace, "git@notes", "local.md"), "local notes"); + writeFileSync(join(ctx.root, "private.md"), "synthetic private content"); + await ctx.scan({ target: "git@notes/local.md" }, { hasUI: false }); + assert.equal(ctx.calls[0].args[1], join(ctx.workspace, "git@notes", "local.md")); + assert.equal(ctx.prompts.length, 0); + await ctx.scan({ target: "git@notes/../../private.md" }); + assert.equal(ctx.calls[1].args[1], join(ctx.root, "private.md")); + assert.equal(ctx.prompts.length, 1); + assert.match(ctx.prompts[0].message, /Read external scan target:/); + assert.ok(ctx.prompts[0].message.includes(JSON.stringify(join(ctx.root, "private.md")))); + assert.doesNotMatch(ctx.prompts[0].message, /Fetch remote/); +}); + +test("does not launch until approval arrives or after a canceled dialog", async (t) => { + const ctx = await setup(t, undefined, () => new Promise(() => {})); + const controller = new AbortController(); + const scan = ctx.scan({ target: "https://example.test/skill.zip" }, {}, controller.signal); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(ctx.calls.length, 0); + controller.abort(); + await assert.rejects(scan, /abort/i); + assert.equal(ctx.calls.length, 0); +}); + +test("rejects input aliases retargeted while awaiting approval", async (t) => { + const ctx = await setup(t, undefined, async ({ workspace, root }) => { + renameSync(join(workspace, "SKILL.md"), join(workspace, "original.md")); + symlinkSync(join(root, "private.md"), join(workspace, "SKILL.md")); + return true; + }); + writeFileSync(join(ctx.root, "private.md"), "synthetic private content"); + mkdirSync(join(ctx.root, "rules")); + await assert.rejects(ctx.scan({ yaraRulesDir: "../rules" }), /input path changed/); + assert.equal(ctx.calls.length, 0); +}); + +test("rejects remote YARA directories instead of treating them as scan targets", async (t) => { + const ctx = await setup(t, undefined, async () => true); + await assert.rejects(ctx.scan({ yaraRulesDir: "https://example.test/rules" }), /local directory/); + assert.equal(ctx.calls.length, 0); + assert.equal(ctx.prompts.length, 0); +}); + test("never resolves a workspace executable through PATH or a relative override", async (t) => { const ctx = await setup(t); rmSync(ctx.bin); @@ -253,7 +368,7 @@ test("preserves generated reports when the scanner returns exit 1 or 2", async ( if (process.platform !== "win32") assert.equal(statSync(destination).mode & 0o777, 0o600); assert.equal(existsSync(dirname(ctx.calls.at(-1).output)), false); } - assert.deepEqual(readdirSync(ctx.workspace), ["existing.json", "new.json"]); + assert.deepEqual(readdirSync(ctx.workspace), ["SKILL.md", "existing.json", "new.json"]); }); } }); @@ -272,7 +387,46 @@ test("cleans staged reports after operational failure, cancellation, and missing await assert.rejects(ctx.scan({ output: "report.txt" })); assert.equal(readFileSync(join(ctx.workspace, "report.txt"), "utf8"), "preserve"); assert.equal(existsSync(dirname(ctx.calls[0].output)), false); - assert.deepEqual(readdirSync(ctx.workspace), ["report.txt"]); + assert.deepEqual(readdirSync(ctx.workspace), ["SKILL.md", "report.txt"]); }); } }); + + +test("rejects external headless paths uniformly before checking their existence", async (t) => { + const ctx = await setup(t); + writeFileSync(join(ctx.root, "existing.md"), "private"); + for (const target of ["../existing.md", "../missing.md"]) { + await assert.rejects(ctx.scan({ target }, { hasUI: false }), /require user confirmation/); + } + assert.equal(ctx.calls.length, 0); +}); + +test("rejects invalid output and pre-canceled calls before showing a dialog", async (t) => { + const ctx = await setup(t); + await assert.rejects(ctx.scan({ target: "https://example.test/skill", output: "../report.json" }), /within the current workspace/); + const controller = new AbortController(); + controller.abort(); + await assert.rejects(ctx.scan({ target: "https://example.test/skill" }, {}, controller.signal), /abort/i); + assert.equal(ctx.prompts.length, 0); + assert.equal(ctx.calls.length, 0); +}); + +test("rejects a workspace swapped while a remote target awaits approval", async (t) => { + const ctx = await setup(t, undefined, async ({ root, workspace }) => { + renameSync(workspace, join(root, "original-workspace")); + symlinkSync(root, workspace); + return true; + }); + await assert.rejects(ctx.scan({ target: "https://example.test/skill" }), /input path changed/); + assert.equal(ctx.calls.length, 0); +}); + +test("gives a clear error for missing workspace targets or rule directories", async (t) => { + const ctx = await setup(t); + for (const params of [{ target: "missing.md" }, { yaraRulesDir: "missing-rules" }]) { + await assert.rejects(ctx.scan(params), /Could not resolve/); + } + assert.equal(ctx.calls.length, 0); + assert.equal(ctx.prompts.length, 0); +});