diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 653ab5701..bf804c52f 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 = = { 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 beb9703c4..359620250 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 8620d9c6d..3d436b8b3 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>, + 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),