From 0882ae5071f7364d5a6a5ee1a86755c5cd84e261 Mon Sep 17 00:00:00 2001 From: ScriptSmith Date: Thu, 11 Jun 2026 20:01:08 +1000 Subject: [PATCH 1/2] Review fixes --- README.md | 4 +- src/index.ts | 9 +++- src/oauth/dual-verifier.ts | 3 +- src/oauth/external-verifier.ts | 12 +++-- src/oauth/session-security.ts | 94 +++++++++++++++++++++++++++++++++ src/security.ts | 28 +++++++--- src/tools/edit.ts | 17 ++++++ src/tools/grep.ts | 1 - src/tools/js.ts | 38 +++++++++++-- src/tray/http-controller.ts | 85 ++--------------------------- src/utils.ts | 13 +++++ tests/external-verifier.test.ts | 5 +- tests/js.test.ts | 8 +++ tests/security.test.ts | 81 ++++++++++++++++++++++++++++ 14 files changed, 293 insertions(+), 105 deletions(-) create mode 100644 src/oauth/session-security.ts create mode 100644 tests/security.test.ts diff --git a/README.md b/README.md index c2531b0..12690f0 100644 --- a/README.md +++ b/README.md @@ -83,6 +83,8 @@ External JWKS / OIDC (--auth jwks — verify tokens from an external IdP): --jwks-url Explicit JWKS endpoint (overrides discovery) --oauth-audience Expected token audience (strongly recommended) --jwks-scope-grants Map tools: token scopes to tool access + (fail closed: a token with no tools: + scopes is granted no tools) Process management: --max-processes Max concurrent bash processes per session (default: 20) @@ -222,7 +224,7 @@ platter -t http --auth jwks \ **Discovery.** With `--oauth-issuer`, platter fetches the issuer's `/.well-known/openid-configuration` (falling back to `/.well-known/oauth-authorization-server`) at startup to find the `jwks_uri` and re-advertise the authorization server to RFC 9728-capable MCP clients (at `/.well-known/oauth-protected-resource/mcp`). Pass `--jwks-url` to skip discovery, or if the IdP isn't reachable at startup. If discovery fails but `--jwks-url` is set, token verification still works — only the RFC 9728 auto-advertisement is skipped. -**Authorization (what a token can do).** By default a verified token gets admin-level access, bounded only by the operator's CLI restrictions (`--tools`, `--allow-path`, `--allow-command`, `--sandbox`) — the IdP controls *who* gets in, the CLI flags control *what* they can do. Pass `--jwks-scope-grants` to additionally honor `tools:` scopes in the token (e.g. `tools:read tools:bash`), which further narrow the granted tools (they can only narrow, never widen the operator's ceiling). The `scope` claim and Entra-style `scp` claim are both supported. Tokens without any `tools:*` scope fall back to admin-level. +**Authorization (what a token can do).** By default a verified token gets admin-level access, bounded only by the operator's CLI restrictions (`--tools`, `--allow-path`, `--allow-command`, `--sandbox`) — the IdP controls *who* gets in, the CLI flags control *what* they can do. Pass `--jwks-scope-grants` to additionally honor `tools:` scopes in the token (e.g. `tools:read tools:bash`), which further narrow the granted tools (they can only narrow, never widen the operator's ceiling). The `scope` claim and Entra-style `scp` claim are both supported. In this mode access is **fail closed**: a token carrying no `tools:*` scope is granted no tools at all, so a token must explicitly request the tools it needs. **No static fallback.** Unlike `oauth` mode, `jwks` mode accepts *only* externally-issued JWTs — `--auth-token` is rejected. diff --git a/src/index.ts b/src/index.ts index 5bcafe7..f53a66e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -41,6 +41,8 @@ External JWKS / OIDC (--auth jwks — verify tokens from an external IdP): --jwks-url Explicit JWKS endpoint (overrides discovery) --oauth-audience Expected token audience (strongly recommended) --jwks-scope-grants Map tools: token scopes to tool access + (fail closed: a token with no tools: + scopes is granted no tools) Process management: --max-processes Max concurrent bash processes per session (default: 20) @@ -241,15 +243,18 @@ if (values["allow-path"]?.length) { } if (values["allow-command"]?.length) { - security.allowedCommands = []; + // The CLI patterns form a single group (the global restriction). A per-client + // grant adds further groups at session-build time; see buildSessionSecurity. + const globalGroup: RegExp[] = []; for (const pattern of values["allow-command"]) { try { - security.allowedCommands.push(new RegExp(`^(?:${pattern})$`)); + globalGroup.push(new RegExp(`^(?:${pattern})$`)); } catch (e: any) { console.error(`Error: invalid --allow-command regex "${pattern}": ${e.message}\n`); process.exit(1); } } + security.allowedCommands = [globalGroup]; } const VALID_SANDBOX_FS_MODES = ["memory", "overlay", "readwrite"]; diff --git a/src/oauth/dual-verifier.ts b/src/oauth/dual-verifier.ts index 9f2508d..1e41837 100644 --- a/src/oauth/dual-verifier.ts +++ b/src/oauth/dual-verifier.ts @@ -1,6 +1,7 @@ import { InvalidTokenError } from "@modelcontextprotocol/sdk/server/auth/errors.js"; import type { OAuthTokenVerifier } from "@modelcontextprotocol/sdk/server/auth/provider.js"; import type { AuthInfo } from "@modelcontextprotocol/sdk/server/auth/types.js"; +import { safeStrEqual } from "../utils.js"; import type { PlatterOAuthProvider } from "./provider.js"; /** @@ -24,7 +25,7 @@ export class DualVerifier implements OAuthTokenVerifier { // Legacy static bearer token. const legacy = this.getLegacyToken(); - if (legacy && token === legacy) { + if (legacy && safeStrEqual(token, legacy)) { // No grant attached — per-session security falls back to the global // config unmodified (legacy bearer is an admin-level credential). return { diff --git a/src/oauth/external-verifier.ts b/src/oauth/external-verifier.ts index 3a34057..47f6980 100644 --- a/src/oauth/external-verifier.ts +++ b/src/oauth/external-verifier.ts @@ -129,11 +129,13 @@ function parseScopes(payload: JWTPayload): string[] { } /** - * Build a tools-only grant from `tools:` scopes. Returns `null` when no - * recognized tool scopes are present, which `buildSessionSecurity` treats as - * admin-level (the operator's global CLI restrictions still apply). + * Build a tools-only grant from `tools:` scopes. This is only invoked + * when scope-grants mode is enabled, so it always returns a grant (never null): + * a token with no recognized tool scopes yields an empty `{ tools: [] }` grant, + * which `buildSessionSecurity` intersects to zero tools — fail closed. A token + * must carry `tools:` scopes to be granted any tool. */ -function scopesToGrant(scopes: string[]): ClientGrant | null { +function scopesToGrant(scopes: string[]): ClientGrant { const valid = new Set(ALL_TOOL_NAMES); const tools: ToolName[] = []; for (const s of scopes) { @@ -141,7 +143,7 @@ function scopesToGrant(scopes: string[]): ClientGrant | null { const name = s.slice(TOOL_SCOPE_PREFIX.length); if (valid.has(name)) tools.push(name as ToolName); } - return tools.length > 0 ? { tools } : null; + return { tools }; } function clientIdFromPayload(payload: JWTPayload): string { diff --git a/src/oauth/session-security.ts b/src/oauth/session-security.ts new file mode 100644 index 0000000..a957fc7 --- /dev/null +++ b/src/oauth/session-security.ts @@ -0,0 +1,94 @@ +import { resolve, sep } from "node:path"; +import { ALL_TOOL_NAMES, type SecurityConfig, type ToolName } from "../security.js"; +import type { ClientGrant } from "./provider.js"; + +/** + * Narrow a global security config by a per-client grant. Grants can only + * narrow (never widen), with one exception: if the global config doesn't + * enable the sandbox, a grant CAN turn it on for that client. Commands are + * narrowed by ANDing the grant's patterns with the global ones as separate + * groups (see SecurityConfig.allowedCommands), so a broad grant pattern can't + * escape the global `--allow-command` ceiling. + * + * When `grant` is null (legacy bearer token), the global config is returned + * as-is minus the `onToolsChanged` hook — runtime toggles reach per-session + * tools via `broadcastToolToggle`, not via each session's own hook. + * + * Kept free of any HTTP/express dependency so it can be unit-tested in + * isolation (importing the express-laden HTTP controller into the test runner + * is fragile). + */ +export function buildSessionSecurity(global: SecurityConfig, grant: ClientGrant | null): SecurityConfig { + if (!grant) { + const { onToolsChanged: _drop, ...rest } = global; + return { ...rest }; + } + + const sessionSecurity: SecurityConfig = {}; + + // Tools: intersection of global enabled and grant-requested. + const grantTools = new Set(grant.tools); + const globalTools = global.allowedTools ?? new Set(ALL_TOOL_NAMES); + const intersected = new Set(); + for (const t of grantTools) { + if (globalTools.has(t)) intersected.add(t); + } + sessionSecurity.allowedTools = intersected; + + // Paths: grant paths must be subpaths of at least one global allowed path + // (if the global config has a restriction). Otherwise the grant paths apply + // directly. Grant paths are resolved to absolute form. + if (grant.allowedPaths?.length) { + const grantResolved = grant.allowedPaths.map((p) => resolve(p)); + if (global.allowedPaths?.length) { + const globalPaths = global.allowedPaths; + sessionSecurity.allowedPaths = grantResolved.filter((g) => + globalPaths.some((allowed) => g === allowed || g.startsWith(allowed + sep)), + ); + } else { + sessionSecurity.allowedPaths = grantResolved; + } + } else if (global.allowedPaths) { + sessionSecurity.allowedPaths = global.allowedPaths; + } + + // Commands: enforce the global ceiling AND the grant as a conjunction of + // groups (see SecurityConfig.allowedCommands / validateCommand). The global + // patterns stay as their own group(s); the grant's consent-page patterns are + // compiled into an additional group. A command must satisfy every group, so + // a grant can only narrow — never widen — the global restriction, even if the + // operator types a broad pattern like `.*` on the consent page. + const commandGroups: RegExp[][] = []; + if (global.allowedCommands?.length) { + commandGroups.push(...global.allowedCommands); + } + if (grant.allowedCommands?.length) { + const grantGroup = grant.allowedCommands.flatMap((pat) => { + try { + return [new RegExp(`^(?:${pat})$`)]; + } catch { + return []; + } + }); + if (grantGroup.length) commandGroups.push(grantGroup); + } + if (commandGroups.length) { + sessionSecurity.allowedCommands = commandGroups; + } + + // Sandbox: the global config wins whenever it's on (clients can't weaken). + // Otherwise, if the grant opts in, use the grant's sandbox config. + if (global.sandbox?.enabled) { + sessionSecurity.sandbox = global.sandbox; + } else if (grant.sandbox?.enabled) { + sessionSecurity.sandbox = { + enabled: true, + fsMode: grant.sandbox.fsMode, + allowedUrls: grant.sandbox.allowedUrls, + }; + } else if (global.sandbox) { + sessionSecurity.sandbox = global.sandbox; + } + + return sessionSecurity; +} diff --git a/src/security.ts b/src/security.ts index 6a6f51c..b375fdf 100644 --- a/src/security.ts +++ b/src/security.ts @@ -16,7 +16,14 @@ export interface SandboxConfig { export interface SecurityConfig { allowedTools?: Set; allowedPaths?: string[]; - allowedCommands?: RegExp[]; + /** + * Allowed bash commands, modelled as a conjunction of disjunctions: a list + * of pattern groups where a command must match at least one pattern in + * *every* group. The global `--allow-command` patterns form one group; a + * per-client OAuth grant adds another. Requiring all groups to match is what + * lets a grant only narrow (never widen) the global restriction. + */ + allowedCommands?: RegExp[][]; sandbox?: SandboxConfig; /** * Notified when allowedTools is mutated at runtime via setToolEnabled. @@ -93,14 +100,19 @@ export async function validatePath(absolutePath: string, allowedPaths: string[]) } /** - * Validate that a bash command matches at least one allowed pattern. - * Patterns are fully anchored — the entire command string must match. + * Validate a bash command against a conjunction of pattern groups: the command + * must match at least one pattern in *every* group. Each group is a disjunction + * (any pattern matches); the groups are ANDed together. Patterns are fully + * anchored — the entire command string must match. + * + * A single global group reproduces the old "match any allowed pattern" + * behaviour. A second group from a per-client grant can only narrow access, + * since a command now has to satisfy the global group as well. */ -export function validateCommand(command: string, allowedCommands: RegExp[]): void { - for (const pattern of allowedCommands) { - if (pattern.test(command)) { - return; +export function validateCommand(command: string, commandGroups: RegExp[][]): void { + for (const group of commandGroups) { + if (!group.some((pattern) => pattern.test(command))) { + throw new Error("Command not allowed. Must match one of the allowed command patterns."); } } - throw new Error("Command not allowed. Must match one of the allowed command patterns."); } diff --git a/src/tools/edit.ts b/src/tools/edit.ts index a5a96d2..789d8c0 100644 --- a/src/tools/edit.ts +++ b/src/tools/edit.ts @@ -246,6 +246,23 @@ export async function editTool( } } + // Fuzzy matching maps a position in the normalized text back to the original + // via line:col. Because normalization changes per-line length (trailing- + // whitespace trim, NFKC), that mapping can drift and mis-bound the slice we + // replace. Guard against a silent mis-edit: re-normalize the slice we're + // about to replace and require it to equal the normalized old_text. + if (matchResult.usedFuzzyMatch) { + const matchedSlice = normalizedContent.substring( + matchResult.originalIndex, + matchResult.originalIndex + matchResult.originalMatchLength, + ); + if (normalizeForFuzzyMatch(matchedSlice) !== normalizeForFuzzyMatch(normalizedOldText)) { + throw new Error( + `Could not safely locate the text to replace in ${args.path}. Provide old_text that matches exactly (including whitespace).`, + ); + } + } + newContent = normalizedContent.substring(0, matchResult.originalIndex) + normalizedNewText + diff --git a/src/tools/grep.ts b/src/tools/grep.ts index 41a91d2..4e10ce5 100644 --- a/src/tools/grep.ts +++ b/src/tools/grep.ts @@ -10,7 +10,6 @@ export interface GrepArgs { pattern: string; path?: string; glob?: string; - include?: string; output_mode?: OutputMode; context?: number; before_context?: number; diff --git a/src/tools/js.ts b/src/tools/js.ts index dbf22db..da44378 100644 --- a/src/tools/js.ts +++ b/src/tools/js.ts @@ -32,6 +32,24 @@ function autoReturn(code: string): string { return lines.join("\n"); } +/** + * True for errors that must abort the evalAsync strategy cascade immediately + * instead of falling through to the next wrapping: a timeout (synchronous vm + * timeout or the Promise.race deadline) or an abort. Without this, a runaway + * synchronous loop would be retried under every wrapping, multiplying the + * timeout, and a cancellation would be ignored until the last attempt. + */ +function isFatalEvalError(e: unknown): boolean { + if (!(e instanceof Error)) return false; + const msg = e.message ?? ""; + return ( + (e as { code?: string }).code === "ERR_SCRIPT_EXECUTION_TIMEOUT" || + msg.startsWith("Script execution timed out") || + msg.startsWith("Execution timed out after") || + msg === "Cancelled" + ); +} + export class JsRuntime { private context: vm.Context; private logs: string[] = []; @@ -168,7 +186,13 @@ export class JsRuntime { private async evalAsync(code: string, timeoutMs: number, signal?: AbortSignal): Promise { const run = (wrapped: string): Promise => { - const promise = vm.runInContext(wrapped, this.context) as Promise; + // The `timeout` option only bounds the *synchronous* part of the IIFE + // (everything up to the first `await`), so a busy-loop before any await is + // interrupted here. A synchronous loop *after* an await runs as a + // microtask outside runInContext and still blocks the shared event loop — + // the Promise.race deadline below can't fire while the loop holds it. That + // residual is acceptable: `js` is explicitly not a security sandbox. + const promise = vm.runInContext(wrapped, this.context, { timeout: timeoutMs }) as Promise; const racers: Promise[] = [promise]; racers.push( new Promise((_, reject) => @@ -188,14 +212,16 @@ export class JsRuntime { // Try as single async expression try { return await run(`(async()=>(\n${code}\n))()`); - } catch { + } catch (e) { + if (isFatalEvalError(e)) throw e; // Not a single expression } // Try as block with auto-return on last expression try { return await run(`(async()=>{\n${autoReturn(code)}\n})()`); - } catch { + } catch (e) { + if (isFatalEvalError(e)) throw e; // auto-return failed } @@ -209,12 +235,14 @@ export class JsRuntime { if (tc === code) throw e; try { return await run(`(async()=>(\n${tc.replace(/;\s*$/, "")}\n))()`); - } catch { + } catch (err) { + if (isFatalEvalError(err)) throw err; /* not an expression */ } try { return await run(`(async()=>{\n${autoReturn(tc)}\n})()`); - } catch { + } catch (err) { + if (isFatalEvalError(err)) throw err; /* auto-return failed */ } return await run(`(async()=>{\n${tc}\n})()`); diff --git a/src/tray/http-controller.ts b/src/tray/http-controller.ts index 5cccaf2..9240b57 100644 --- a/src/tray/http-controller.ts +++ b/src/tray/http-controller.ts @@ -2,7 +2,6 @@ import crypto from "node:crypto"; import { readFileSync } from "node:fs"; import type { Server as HttpsServer, Server } from "node:http"; import https from "node:https"; -import { resolve, sep } from "node:path"; import { requireBearerAuth } from "@modelcontextprotocol/sdk/server/auth/middleware/bearerAuth.js"; import { getOAuthProtectedResourceMetadataUrl, @@ -22,10 +21,12 @@ import { type GlobalRestrictions, installConsentRoutes, toolScope } from "../oau import { DualVerifier } from "../oauth/dual-verifier.js"; import { ExternalJwtVerifier } from "../oauth/external-verifier.js"; import type { ClientGrant, PlatterOAuthProvider } from "../oauth/provider.js"; +import { buildSessionSecurity } from "../oauth/session-security.js"; import type { ProcessRegistry } from "../process-registry.js"; import { ALL_TOOL_NAMES, isToolEnabled, type SecurityConfig, type ToolName } from "../security.js"; import { type CreateServerOpts, createServer } from "../server.js"; import type { JsRuntime } from "../tools/js.js"; +import { safeStrEqual } from "../utils.js"; import type { ActivityMonitor } from "./activity-monitor.js"; interface SessionEntry { @@ -237,7 +238,7 @@ export class HttpController { (): GlobalRestrictions => ({ enabledTools: ALL_TOOL_NAMES.filter((t) => isToolEnabled(security, t)), allowedPaths: security.allowedPaths, - allowedCommands: security.allowedCommands?.map((r) => r.source), + allowedCommands: security.allowedCommands?.flat().map((r) => r.source), sandbox: security.sandbox, }), ); @@ -317,12 +318,12 @@ export class HttpController { return; } const auth = req.headers.authorization; - if (!auth) { + if (!auth || typeof auth !== "string") { res.setHeader("WWW-Authenticate", "Bearer"); res.status(401).json({ error: "Unauthorized" }); return; } - if (auth !== `Bearer ${token}`) { + if (!safeStrEqual(auth, `Bearer ${token}`)) { res.setHeader("WWW-Authenticate", 'Bearer error="invalid_token"'); res.status(401).json({ error: "Unauthorized" }); return; @@ -502,82 +503,6 @@ export class HttpController { } } -/** - * Narrow a global security config by a per-client grant. Grants can only - * narrow (never widen), with one exception: if the global config doesn't - * enable the sandbox, a grant CAN turn it on for that client. - * - * When `grant` is null (legacy bearer token), the global config is returned - * as-is minus the `onToolsChanged` hook — runtime toggles reach per-session - * tools via `broadcastToolToggle`, not via each session's own hook. - */ -export function buildSessionSecurity(global: SecurityConfig, grant: ClientGrant | null): SecurityConfig { - if (!grant) { - const { onToolsChanged: _drop, ...rest } = global; - return { ...rest }; - } - - const sessionSecurity: SecurityConfig = {}; - - // Tools: intersection of global enabled and grant-requested. - const grantTools = new Set(grant.tools); - const globalTools = global.allowedTools ?? new Set(ALL_TOOL_NAMES); - const intersected = new Set(); - for (const t of grantTools) { - if (globalTools.has(t)) intersected.add(t); - } - sessionSecurity.allowedTools = intersected; - - // Paths: grant paths must be subpaths of at least one global allowed path - // (if the global config has a restriction). Otherwise the grant paths apply - // directly. Grant paths are resolved to absolute form. - if (grant.allowedPaths?.length) { - const grantResolved = grant.allowedPaths.map((p) => resolve(p)); - if (global.allowedPaths?.length) { - const globalPaths = global.allowedPaths; - sessionSecurity.allowedPaths = grantResolved.filter((g) => - globalPaths.some((allowed) => g === allowed || g.startsWith(allowed + sep)), - ); - } else { - sessionSecurity.allowedPaths = grantResolved; - } - } else if (global.allowedPaths) { - sessionSecurity.allowedPaths = global.allowedPaths; - } - - // Commands: grant regex strings compiled to anchored patterns. Assumed to - // already represent the admin's intent (they typed them on the consent - // page in response to the displayed global restriction). If the grant has - // none, fall back to the global config. - if (grant.allowedCommands?.length) { - sessionSecurity.allowedCommands = grant.allowedCommands.flatMap((pat) => { - try { - return [new RegExp(`^(?:${pat})$`)]; - } catch { - return []; - } - }); - } else if (global.allowedCommands) { - sessionSecurity.allowedCommands = global.allowedCommands; - } - - // Sandbox: the global config wins whenever it's on (clients can't weaken). - // Otherwise, if the grant opts in, use the grant's sandbox config. - if (global.sandbox?.enabled) { - sessionSecurity.sandbox = global.sandbox; - } else if (grant.sandbox?.enabled) { - sessionSecurity.sandbox = { - enabled: true, - fsMode: grant.sandbox.fsMode, - allowedUrls: grant.sandbox.allowedUrls, - }; - } else if (global.sandbox) { - sessionSecurity.sandbox = global.sandbox; - } - - return sessionSecurity; -} - /** * Fetch an external issuer's authorization-server metadata. Tries the OIDC * discovery document first, then the RFC 8414 OAuth document. The well-known diff --git a/src/utils.ts b/src/utils.ts index dccd282..99b5701 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1,6 +1,19 @@ +import { createHash, timingSafeEqual } from "node:crypto"; import { homedir } from "node:os"; import { isAbsolute, resolve } from "node:path"; +/** + * Constant-time string equality for secrets (bearer tokens). Hashing both + * inputs to fixed-length SHA-256 digests first means `timingSafeEqual` never + * throws on a length mismatch and the comparison leaks neither the value nor + * the length of the expected secret. + */ +export function safeStrEqual(a: string, b: string): boolean { + const ha = createHash("sha256").update(a).digest(); + const hb = createHash("sha256").update(b).digest(); + return timingSafeEqual(ha, hb); +} + export function killProcessTree(pid: number): void { try { // Kill entire process group diff --git a/tests/external-verifier.test.ts b/tests/external-verifier.test.ts index f1f4f0a..e4609ac 100644 --- a/tests/external-verifier.test.ts +++ b/tests/external-verifier.test.ts @@ -122,10 +122,11 @@ describe("ExternalJwtVerifier", () => { expect(grant?.tools).toEqual(["read"]); }); - it("falls back to admin (null grant) when no tools: scopes are present", async () => { + it("fails closed (empty grant, no tools) when no tools: scopes are present", async () => { const token = await sign({ audience: AUD, claims: { scope: "openid profile" } }); const info = await makeVerifier({ scopeGrants: true }).verifyAccessToken(token); - expect((info.extra as { grant: unknown }).grant).toBeNull(); + const grant = (info.extra as { grant: { tools: string[] } | null }).grant; + expect(grant).toEqual({ tools: [] }); }); }); diff --git a/tests/js.test.ts b/tests/js.test.ts index e77b94a..68020c0 100644 --- a/tests/js.test.ts +++ b/tests/js.test.ts @@ -93,6 +93,14 @@ describe("JsRuntime", () => { await expect(runtime.evaluate("while(true){}", 1000)).rejects.toThrow("timed out"); }); + it("times out on a sync loop before an await (async path)", async () => { + runtime = new JsRuntime(); + // Contains `await`, so it takes the async evaluation path. The synchronous + // loop runs before the first await, so the vm timeout must interrupt it + // (and abort the wrapping cascade) rather than hang. + await expect(runtime.evaluate("while(true){}\nawait 1;", 1000)).rejects.toThrow("timed out"); + }); + it("context survives after timeout", async () => { runtime = new JsRuntime(); await runtime.evaluate("var before = 123"); diff --git a/tests/security.test.ts b/tests/security.test.ts new file mode 100644 index 0000000..0c6af63 --- /dev/null +++ b/tests/security.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from "bun:test"; +import type { ClientGrant } from "../src/oauth/provider.js"; +import { buildSessionSecurity } from "../src/oauth/session-security.js"; +import { type SecurityConfig, validateCommand } from "../src/security.js"; + +function anchored(...patterns: string[]): RegExp[] { + return patterns.map((p) => new RegExp(`^(?:${p})$`)); +} + +describe("validateCommand (conjunction of groups)", () => { + it("allows when a single global group matches any pattern", () => { + const groups = [anchored("git .*", "ls")]; + expect(() => validateCommand("git status", groups)).not.toThrow(); + expect(() => validateCommand("ls", groups)).not.toThrow(); + }); + + it("rejects when the single group matches nothing", () => { + const groups = [anchored("git .*")]; + expect(() => validateCommand("rm -rf /", groups)).toThrow("Command not allowed"); + }); + + it("requires a match in EVERY group (AND semantics)", () => { + // group 1 = global ceiling (git only); group 2 = grant (anything). + const groups = [anchored("git .*"), anchored(".*")]; + expect(() => validateCommand("git status", groups)).not.toThrow(); + // matches the broad grant group but not the global ceiling → rejected. + expect(() => validateCommand("rm -rf /", groups)).toThrow("Command not allowed"); + }); + + it("treats no groups as unrestricted", () => { + expect(() => validateCommand("anything goes", [])).not.toThrow(); + }); +}); + +describe("buildSessionSecurity command narrowing", () => { + it("a broad grant cannot widen past the global --allow-command ceiling", () => { + const global: SecurityConfig = { allowedCommands: [anchored("git .*")] }; + const grant: ClientGrant = { tools: ["bash"], allowedCommands: [".*"] }; + + const session = buildSessionSecurity(global, grant); + expect(session.allowedCommands).toBeDefined(); + // Two groups now: global ceiling + grant. + expect(session.allowedCommands).toHaveLength(2); + expect(() => validateCommand("git status", session.allowedCommands!)).not.toThrow(); + expect(() => validateCommand("rm -rf /", session.allowedCommands!)).toThrow("Command not allowed"); + }); + + it("a grant narrows further within the global ceiling", () => { + const global: SecurityConfig = { allowedCommands: [anchored("git .*")] }; + const grant: ClientGrant = { tools: ["bash"], allowedCommands: ["git status"] }; + + const session = buildSessionSecurity(global, grant); + expect(() => validateCommand("git status", session.allowedCommands!)).not.toThrow(); + // allowed by the global ceiling but not by the tighter grant → rejected. + expect(() => validateCommand("git push", session.allowedCommands!)).toThrow("Command not allowed"); + }); + + it("a grant can restrict commands when the global config has none", () => { + const global: SecurityConfig = {}; + const grant: ClientGrant = { tools: ["bash"], allowedCommands: ["git .*"] }; + + const session = buildSessionSecurity(global, grant); + expect(() => validateCommand("git status", session.allowedCommands!)).not.toThrow(); + expect(() => validateCommand("rm -rf /", session.allowedCommands!)).toThrow("Command not allowed"); + }); + + it("falls back to the global ceiling when the grant has no commands", () => { + const global: SecurityConfig = { allowedCommands: [anchored("git .*")] }; + const grant: ClientGrant = { tools: ["bash"] }; + + const session = buildSessionSecurity(global, grant); + expect(session.allowedCommands).toHaveLength(1); + expect(() => validateCommand("git status", session.allowedCommands!)).not.toThrow(); + expect(() => validateCommand("rm -rf /", session.allowedCommands!)).toThrow("Command not allowed"); + }); + + it("leaves commands unrestricted when neither global nor grant set any", () => { + const session = buildSessionSecurity({}, { tools: ["bash"] }); + expect(session.allowedCommands).toBeUndefined(); + }); +}); From 0dcc809e7b659387aa2b078649e584bea9f319a5 Mon Sep 17 00:00:00 2001 From: ScriptSmith Date: Thu, 18 Jun 2026 18:23:45 +1000 Subject: [PATCH 2/2] Review fixes --- src/security.ts | 4 ++++ src/tools/js.ts | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/src/security.ts b/src/security.ts index b375fdf..d727aee 100644 --- a/src/security.ts +++ b/src/security.ts @@ -108,6 +108,10 @@ export async function validatePath(absolutePath: string, allowedPaths: string[]) * A single global group reproduces the old "match any allowed pattern" * behaviour. A second group from a per-client grant can only narrow access, * since a command now has to satisfy the global group as well. + * + * An empty `commandGroups` array is unrestricted: with no groups to satisfy, + * every command passes. To enforce any restriction, pass at least one group; + * to block everything, pass a group with no matching patterns. */ export function validateCommand(command: string, commandGroups: RegExp[][]): void { for (const group of commandGroups) { diff --git a/src/tools/js.ts b/src/tools/js.ts index da44378..043a6e7 100644 --- a/src/tools/js.ts +++ b/src/tools/js.ts @@ -43,8 +43,16 @@ function isFatalEvalError(e: unknown): boolean { if (!(e instanceof Error)) return false; const msg = e.message ?? ""; return ( + // The error code is the reliable vm-timeout signal on Node and Bun. The + // message prefix below is a fallback for runtimes that don't set the code. + // If neither matches, the first two evalAsync attempts swallow the timeout + // and re-run the busy loop, but the third catch rethrows non-SyntaxErrors, + // so the worst case is bounded at 3x timeoutMs and the timeout still + // propagates (the sync busy-loop test in tests/js.test.ts checks that). (e as { code?: string }).code === "ERR_SCRIPT_EXECUTION_TIMEOUT" || msg.startsWith("Script execution timed out") || + // These two are platter's own messages: the Promise.race deadline in + // evalAsync and the abort-signal rejection. They are stable. msg.startsWith("Execution timed out after") || msg === "Cancelled" );