Review fixes

README.md

@@ -83,6 +83,8 @@ External JWKS / OIDC (--auth jwks — verify tokens from an external IdP):
       --jwks-url <url>           Explicit JWKS endpoint (overrides discovery)
       --oauth-audience <aud>     Expected token audience (strongly recommended)
       --jwks-scope-grants        Map tools:<name> token scopes to tool access
+                                 (fail closed: a token with no tools:<name>
+                                 scopes is granted no tools)
 
 Process management:
       --max-processes <number>   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:<name>` 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:<name>` 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.
 

src/index.ts

@@ -41,6 +41,8 @@ External JWKS / OIDC (--auth jwks — verify tokens from an external IdP):
       --jwks-url <url>           Explicit JWKS endpoint (overrides discovery)
       --oauth-audience <aud>     Expected token audience (strongly recommended)
       --jwks-scope-grants        Map tools:<name> token scopes to tool access
+                                 (fail closed: a token with no tools:<name>
+                                 scopes is granted no tools)
 
 Process management:
       --max-processes <number>   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"];

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 {

src/oauth/external-verifier.ts

@@ -129,19 +129,21 @@ function parseScopes(payload: JWTPayload): string[] {
 }
 
 /**
- * Build a tools-only grant from `tools:<name>` 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:<name>` 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:<name>` scopes to be granted any tool.
  */
-function scopesToGrant(scopes: string[]): ClientGrant | null {
+function scopesToGrant(scopes: string[]): ClientGrant {
   const valid = new Set<string>(ALL_TOOL_NAMES);
   const tools: ToolName[] = [];
   for (const s of scopes) {
     if (!s.startsWith(TOOL_SCOPE_PREFIX)) continue;
     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 {

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<ToolName>(grant.tools);
+  const globalTools = global.allowedTools ?? new Set<ToolName>(ALL_TOOL_NAMES);
+  const intersected = new Set<ToolName>();
+  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;
+}

src/security.ts

@@ -16,7 +16,14 @@ export interface SandboxConfig {
 export interface SecurityConfig {
   allowedTools?: Set<ToolName>;
   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,23 @@ 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.
+ *
+ * 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, 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.");
 }

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 +

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;

src/tools/js.ts

@@ -32,6 +32,32 @@ 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 (
+    // 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"
+  );
+}
+
 export class JsRuntime {
   private context: vm.Context;
   private logs: string[] = [];
@@ -168,7 +194,13 @@ export class JsRuntime {
 
   private async evalAsync(code: string, timeoutMs: number, signal?: AbortSignal): Promise<unknown> {
     const run = (wrapped: string): Promise<unknown> => {
-      const promise = vm.runInContext(wrapped, this.context) as Promise<unknown>;
+      // 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<unknown>;
       const racers: Promise<unknown>[] = [promise];
       racers.push(
         new Promise((_, reject) =>
@@ -188,14 +220,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 +243,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})()`);