From e2438d616b45655658a438c7c83f2375d4e590af Mon Sep 17 00:00:00 2001
From: Tony <a@cox.sh>
Date: Sat, 29 Aug 2026 22:59:48 -0600
Subject: [PATCH] Validate caller-supplied policy positions against the
 specificity invariant
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

policies.create/update accepted a caller-supplied position verbatim and
stored it as an arbitrary string. Precedence within an owner is match
authority (resolveToolPolicy: first match per owner wins), and every
key positionForNewPattern emits sorts above strings like "0" or "" —
so any authenticated member could create or update an owner:"org"
policy with position "0" and hoist a wildcard approve above the org's
narrower block/require_approval rules, silently weakening every org
guardrail. The specificity-aware default placement only applied when
no position was sent.

Judge an explicit position by the same invariant instead of trusting
it: a rule may never sort above a more-specific rule whose tool set
overlaps its own (equally-specific rules and rules over disjoint
integrations stay freely orderable, so the UI's up/down reorder keeps
working within the invariant). Also validate the action on update,
which create already checked; an unknown action would throw at
resolve time for every tool the rule matches.

The new tests fail on the previous tree (position "0" stored, org
guardrail shadowed) and pin the refusal.
---
 packages/core/sdk/src/executor.ts      | 35 +++++++++-
 packages/core/sdk/src/policies.test.ts | 89 ++++++++++++++++++++++++++
 packages/core/sdk/src/policies.ts      | 58 +++++++++++++++++
 3 files changed, 181 insertions(+), 1 deletion(-)

diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts
index 653ab57016..bf804c52fa 100644
--- a/packages/core/sdk/src/executor.ts
+++ b/packages/core/sdk/src/executor.ts
@@ -136,6 +136,7 @@ import type { FirstPartyOAuthClientConfig } from "./oauth-client";
 import {
   comparePolicyRow,
   isValidPattern,
+  isValidPositionForPattern,
   matchPattern,
   positionForNewPattern,
   resolveEffectivePolicy,
@@ -5403,6 +5404,18 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea
         // rule), not top-of-list: a client that omits position — the UI when
         // its policy list is stale, the API, an agent tool — must not have its
         // broad rule silently shadow an existing narrow one.
+        // An EXPLICIT position is judged by the same invariant rather than
+        // trusted: precedence within an owner is match authority, so a client
+        // key that sorts a broad rule above a narrower one is refused.
+        if (
+          input.position !== undefined &&
+          !isValidPositionForPattern(input.pattern, input.position, existing)
+        ) {
+          return yield* new StorageError({
+            message: `Tool policy position ${input.position} would place ${input.pattern} above a more-specific rule`,
+            cause: undefined,
+          });
+        }
         const position = input.position ?? positionForNewPattern(input.pattern, existing);
         const id = PolicyId.make(
           `pol_${Math.random().toString(36).slice(2)}${Date.now().toString(36)}`,
@@ -5440,9 +5453,29 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea
             cause: undefined,
           });
         }
+        const action = input.action ?? existing.action;
+        if (!isToolPolicyAction(action)) {
+          return yield* new StorageError({
+            message: `Invalid tool policy action: ${String(action)}`,
+            cause: undefined,
+          });
+        }
+        // Same invariant as create, judged against the owner's OTHER rules and
+        // the rule's (possibly updated) pattern: an explicit position may not
+        // hoist a broad rule above a narrower one.
+        const pattern = input.pattern ?? existing.pattern;
+        if (input.position !== undefined) {
+          const ownerRows = yield* core.findMany("tool_policy", { where: byOwner(input.owner) });
+          if (!isValidPositionForPattern(pattern, input.position, ownerRows, input.id)) {
+            return yield* new StorageError({
+              message: `Tool policy position ${input.position} would place ${pattern} above a more-specific rule`,
+              cause: undefined,
+            });
+          }
+        }
         const set: Record<string, unknown> = { updated_at: new Date() };
         if (input.pattern !== undefined) set.pattern = input.pattern;
-        if (input.action !== undefined) set.action = input.action;
+        if (input.action !== undefined) set.action = action;
         if (input.position !== undefined) set.position = input.position;
         yield* core.updateMany("tool_policy", { where, set });
         const updated = yield* core.findFirst("tool_policy", { where });
diff --git a/packages/core/sdk/src/policies.test.ts b/packages/core/sdk/src/policies.test.ts
index beb9703c49..3596202507 100644
--- a/packages/core/sdk/src/policies.test.ts
+++ b/packages/core/sdk/src/policies.test.ts
@@ -482,6 +482,95 @@ describe("executor.policies", () => {
     }),
   );
 
+  it.effect("create refuses an explicit position that shadows a more-specific rule", () =>
+    Effect.gen(function* () {
+      const executor = yield* setupExecutor();
+      // The org's narrow leaf rule commits first, at the top.
+      const leaf = yield* executor.policies.create({
+        owner: "org",
+        pattern: "vercel.org.main.delete",
+        action: "block",
+      });
+      // A broad wildcard approve whose caller-supplied position sorts ABOVE
+      // the leaf rule: first match per owner wins, so this would silently
+      // weaken the org's block into an approve. `"0"` (0x30) sorts before
+      // every key generateKeyBetween emits (`"a0"`…), so it claims the top of
+      // the owner's list. Refused, not stored.
+      const result = yield* Effect.result(
+        executor.policies.create({
+          owner: "org",
+          pattern: "*",
+          action: "approve",
+          position: "0",
+        }),
+      );
+      expect(Result.isFailure(result)).toBe(true);
+      expect(String((result as { failure: { message: string } }).failure.message)).toContain(
+        "more-specific rule",
+      );
+      void leaf;
+    }),
+  );
+
+  it.effect("update refuses a position that hoists a broad rule above a narrower one", () =>
+    Effect.gen(function* () {
+      const executor = yield* setupExecutor();
+      const narrow = yield* executor.policies.create({
+        owner: "org",
+        pattern: "vercel.org.main.delete",
+        action: "block",
+      });
+      const broad = yield* executor.policies.create({
+        owner: "org",
+        pattern: "vercel.*",
+        action: "approve",
+      });
+      // Broad starts below narrow (the default placement). A client-supplied
+      // position above the narrow rule would flip match precedence — refused.
+      const result = yield* Effect.result(
+        executor.policies.update({
+          id: String(broad.id),
+          owner: "org",
+          position: "0",
+        }),
+      );
+      expect(Result.isFailure(result)).toBe(true);
+
+      // Reordering among equally-specific rules is still allowed.
+      const equallySpecific = yield* executor.policies.create({
+        owner: "org",
+        pattern: "github.*",
+        action: "require_approval",
+      });
+      yield* executor.policies.update({
+        id: String(equallySpecific.id),
+        owner: "org",
+        position: narrow.position,
+      });
+      const rules = yield* executor.policies.list();
+      expect(rules.find((r) => r.pattern === "github.*")?.position).toBe(narrow.position);
+    }),
+  );
+
+  it.effect("update rejects an unknown action", () =>
+    Effect.gen(function* () {
+      const executor = yield* setupExecutor();
+      const created = yield* executor.policies.create({
+        owner: "org",
+        pattern: "vercel.*",
+        action: "require_approval",
+      });
+      const result = yield* Effect.result(
+        executor.policies.update({
+          id: String(created.id),
+          owner: "org",
+          action: "deny-everything" as never,
+        }),
+      );
+      expect(Result.isFailure(result)).toBe(true);
+    }),
+  );
+
   it.effect("remove deletes the rule", () =>
     Effect.gen(function* () {
       const executor = yield* setupExecutor();
diff --git a/packages/core/sdk/src/policies.ts b/packages/core/sdk/src/policies.ts
index 8620d9c6d3..3d436b8b3e 100644
--- a/packages/core/sdk/src/policies.ts
+++ b/packages/core/sdk/src/policies.ts
@@ -180,6 +180,64 @@ export const positionForNewPattern = (
   return generateKeyBetween(prev, next);
 };
 
+/**
+ * Whether two patterns can possibly match the same tool id. Conservative:
+ * any shared prefix up to the first differing literal segment counts as
+ * overlap, and a wildcard segment anywhere before a difference does too.
+ * Only used to scope the ordering invariant below — false positives keep the
+ * invariant strict; false negatives would be unsafe, and there are none
+ * (segment-by-segment, a shared literal prefix plus a wildcard or exhaustion
+ * is the only way to a common tool id).
+ */
+const patternsOverlap = (a: string, b: string): boolean => {
+  if (a === "*" || b === "*") return true;
+  const as = a.split(".");
+  const bs = b.split(".");
+  const len = Math.min(as.length, bs.length);
+  for (let i = 0; i < len; i++) {
+    const sa = as[i]!;
+    const sb = bs[i]!;
+    if (sa === "*" || sb === "*") return true;
+    if (sa !== sb) return false;
+  }
+  return true;
+};
+
+/**
+ * Whether a CALLER-SUPPLIED position is safe for `pattern` among the owner's
+ * committed rules. `positionForNewPattern` computes the safest key when the
+ * client omits one; a client that sends one can otherwise place a broad rule
+ * above a narrower rule of the same owner and silently weaken it — first
+ * match per owner wins (`resolveToolPolicy`), so precedence is authority.
+ * The invariant is the same one the default placement enforces: a rule may
+ * never sort ABOVE (lexically before) an existing rule that is MORE specific
+ * than it AND can match a tool this rule also matches. Equally- or
+ * less-specific rules, and rules over disjoint tool sets, remain freely
+ * orderable.
+ *
+ * `excludeId` omits the rule being updated, so a reorder is judged against
+ * the OTHER committed rules.
+ */
+export const isValidPositionForPattern = (
+  pattern: string,
+  position: string,
+  rows: ReadonlyArray<Pick<ToolPolicyRow, "pattern" | "position" | "id">>,
+  excludeId?: string,
+): boolean => {
+  const newScore = patternSpecificity(pattern);
+  for (const row of rows) {
+    if (excludeId !== undefined && row.id === excludeId) continue;
+    if (
+      patternSpecificity(row.pattern) > newScore &&
+      patternsOverlap(row.pattern, pattern) &&
+      position <= row.position
+    ) {
+      return false;
+    }
+  }
+  return true;
+};
+
 const actionRestrictionRank = (action: ToolPolicyAction): number =>
   Match.value(action).pipe(
     Match.when("block", () => 3),