diff --git a/src/policy/posthog-command-wrapper.ts b/src/policy/posthog-command-wrapper.ts index 330a38e0..a5227d6a 100644 --- a/src/policy/posthog-command-wrapper.ts +++ b/src/policy/posthog-command-wrapper.ts @@ -6,7 +6,8 @@ const MAX_SEARCH_QUERY_LENGTH = 256; const canonicalToolNamePattern = /^[A-Za-z][A-Za-z0-9:_-]{0,127}$/u; const canonicalFieldPathPattern = /^[A-Za-z][A-Za-z0-9_.-]{0,127}$/u; const safeSearchQueryPattern = /^[A-Za-z0-9][A-Za-z0-9 .,:_\-/()]{0,255}$/u; -const unsafeCommandSyntaxPattern = /[;|&`$<>\\]/u; +const unsafeCommandSyntaxPattern = /[;|&`<>\\]/u; +const unsafeDollarSubstitutionPattern = /\$(?:\(|\{)/u; type ParsedPosthogCommand = | { readonly kind: "read-discovery" } @@ -58,7 +59,10 @@ function commandArgument(input: string, verb: string): string | undefined { return argument.length === 0 ? undefined : argument; } -/** Rejects control characters and syntax that could change shell semantics. */ +/** + * Rejects control characters and syntax that could change shell semantics. + * Dollar handling is deliberately deferred to the validated JSON call payload. + */ function hasUnsafeCommandCharacter(command: string): boolean { for (let index = 0; index < command.length; index += 1) { const code = command.charCodeAt(index); @@ -111,10 +115,17 @@ function parseCallCommand(input: string): ParsedPosthogCommand { if (target === undefined || !canonicalToolNamePattern.test(target.value) || target.remaining.length === 0) { return { kind: "invalid" }; } - if (!isJsonObject(target.remaining)) return { kind: "invalid" }; + if (!isJsonObject(target.remaining) || hasUnsafeDollarSubstitution(target.remaining)) { + return { kind: "invalid" }; + } return { kind: "call", toolName: target.value }; } +/** Rejects actual shell command-substitution forms while preserving HogQL `$identifier` data. */ +function hasUnsafeDollarSubstitution(payload: string): boolean { + return unsafeDollarSubstitutionPattern.test(payload); +} + /** Splits whitespace-delimited grammar tokens; quoted shell tokens are not supported. */ function firstToken(input: string): { readonly value: string; readonly remaining: string } | undefined { const trimmed = input.trim(); diff --git a/tests/mcp-wrapper.test.ts b/tests/mcp-wrapper.test.ts index 83a37eb7..4c17f4be 100644 --- a/tests/mcp-wrapper.test.ts +++ b/tests/mcp-wrapper.test.ts @@ -461,7 +461,10 @@ describe("Miftah MCP wrapper", () => { const wrapper = new MiftahServer(config, new ProfileManager(config), upstreams); const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); const client = new Client({ name: "posthog-command-wrapper-client", version: "1.0.0" }); - const readArguments = { command: "info query-trends", context: "scheduled task" }; + const readArguments = { + command: "call query-trends {\"event\":\"$pageview\",\"math\":\"dau\"}", + context: "scheduled task" + }; try { await Promise.all([wrapper.connect(serverTransport), client.connect(clientTransport)]); @@ -476,7 +479,7 @@ describe("Miftah MCP wrapper", () => { enforcement: { status: "allowed" } }); expect(await client.callTool({ name: "exec", arguments: readArguments })).toMatchObject({ - content: [{ type: "text", text: "exec:info query-trends" }] + content: [{ type: "text", text: `exec:${readArguments.command}` }] }); expect( @@ -507,7 +510,7 @@ describe("Miftah MCP wrapper", () => { const audit = await readFile(auditPath, "utf8"); expect(audit).toContain('"riskSource":"trusted-command-adapter"'); - expect(audit).not.toContain("info query-trends"); + expect(audit).not.toContain(readArguments.command); } finally { await client.close(); await wrapper.close(); diff --git a/tests/posthog-command-wrapper.test.ts b/tests/posthog-command-wrapper.test.ts index 4347fe9d..9f6ad8fc 100644 --- a/tests/posthog-command-wrapper.test.ts +++ b/tests/posthog-command-wrapper.test.ts @@ -11,7 +11,9 @@ describe("PostHog command-wrapper classifier", () => { ["schema query-trends results.columns", "read"], ["call query-trends {}", "read"], ["call --json query-trends {\"limit\":10}", "read"], + ["call query-trends {\"event\":\"$pageview\",\"math\":\"dau\"}", "read"], ["call dashboard-create {}", "write"], + ["call dashboard-delete {\"event\":\"$pageview\"}", "destructive"], ["call dashboard-delete {}", "destructive"], ["call --confirm dashboard-delete {}", "destructive"], ["call execute-sql {}", "destructive"], @@ -28,11 +30,15 @@ describe("PostHog command-wrapper classifier", () => { "info --json --json query-trends", "schema query-trends extra one", "search ", + "search $pageview", "search query; call dashboard-delete {}", "call", "call --confirm --confirm query-trends {}", "call --force query-trends {}", + "call $query-trends {}", "call query-trends []", + "call query-trends {\"event\":\"$(whoami)\"}", + "call query-trends {\"event\":\"${HOME}\"}", "call query-trends {\"limit\":1} trailing", "call query-trends {}; tools", "call query-trends {}\ncall dashboard-delete {}", diff --git a/tests/risk-classifier.test.ts b/tests/risk-classifier.test.ts index 403e3f5d..0a0d110a 100644 --- a/tests/risk-classifier.test.ts +++ b/tests/risk-classifier.test.ts @@ -2,6 +2,14 @@ import { describe, expect, it } from "vitest"; import { classifyToolRisk } from "../src/policy/risk-classifier.js"; describe("tool risk classifier", () => { + it("keeps a PostHog HogQL dollar identifier read-only through the trusted adapter", () => { + expect( + classifyToolRisk("exec", {}, { + posthogCommand: { command: "call query-trends {\"event\":\"$pageview\",\"math\":\"dau\"}" } + }) + ).toEqual({ risk: "read", riskSource: "trusted-command-adapter", riskConfidence: "high" }); + }); + it("gives a trusted PostHog command precedence over a static read-only hint", () => { expect( classifyToolRisk("exec", {}, {