Skip to content

Validate caller-supplied policy positions against the specificity invariant - #1874

Draft
ux-xd wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
ux-xd:fix/policy-position-shadow
Draft

Validate caller-supplied policy positions against the specificity invariant#1874
ux-xd wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
ux-xd:fix/policy-position-shadow

Conversation

@ux-xd

@ux-xd ux-xd commented Aug 30, 2026

Copy link
Copy Markdown

Summary

policies.create / policies.update accepted a caller-supplied position verbatim and stored it as an arbitrary string. This PR judges an explicit position by the same specificity invariant the server-side default placement already enforces, and adds the missing action validation on update.

The issue

Precedence within an owner is match authority: resolveToolPolicy takes the FIRST matching rule per owner by position, and the most restrictive action across owners. positionForNewPattern places a new rule below every more-specific existing rule — but only when the client omits position. When a position IS sent (HTTP POST/PATCH /policies passes it through; the only validation on the whole write path is isValidPattern + isToolPolicyAction), it is stored as-is.

Because comparePolicyRow is a plain string compare and every key generateKeyBetween emits sorts above strings like "0" or "" ("a0" is the smallest emitted key; 0x30 < 0x61), any authenticated member can:

POST /policies  { "owner": "org", "pattern": "*", "action": "approve", "position": "0" }

or PATCH an existing rule's position to "0", and hoist a wildcard approve above the org's narrower block / require_approval rules — silently weakening every org guardrail for the whole workspace. There is no role gate on the product plane to stop a member from writing owner: "org" policies; the only defense was the default placement, which an explicit position bypasses.

The fix

isValidPositionForPattern enforces the invariant on explicit positions for both create and update: a rule may never sort ABOVE a more-specific rule whose tool set overlaps its own. A conservative segment-wise overlap check keeps rules over disjoint integrations freely orderable, so the console's up/down reorder (the only legitimate explicit-position sender, which always sends real generateKeyBetween output over committed neighbors) keeps working within the invariant. Behavior change to flag: moving a broad rule above a narrower overlapping one via the console reorder is now refused by the server — previously it silently weakened the guardrail, which is part of this issue.

Also: update now validates the action the same way create does; an unknown action previously stored fine and threw Match.exhaustive at resolve time, failing execution of every tool the rule matched.

Verification

New tests in policies.test.ts fail on the previous tree (position "0" is stored; the org block rule is shadowed) and pass with the fix:

  • create refuses an explicit position that shadows a more-specific rule
  • update refuses a position that hoists a broad rule above a narrower one; reordering among equally-specific rules still works
  • update rejects an unknown action

vitest run src/policies.test.ts — 38 passed. turbo run typecheck --filter=@executor-js/sdk — green.

Not run: e2e. One behavior note for review: the console reorder can no longer hoist a broad rule above a narrower overlapping rule; today that move silently creates exactly the shadowing this PR exists to prevent, so failing loudly seemed right, but it is a small UX change and easy to relax (e.g. clamp to the nearest legal key instead of refusing) if preferred.

…ariant

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant