Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/policy/posthog-command-wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down
9 changes: 6 additions & 3 deletions tests/mcp-wrapper.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)]);
Expand All @@ -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(
Expand Down Expand Up @@ -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();
Expand Down
6 changes: 6 additions & 0 deletions tests/posthog-command-wrapper.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand All @@ -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 {}",
Expand Down
8 changes: 8 additions & 0 deletions tests/risk-classifier.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", {}, {
Expand Down