Validate caller-supplied policy positions against the specificity invariant - #1874
Draft
ux-xd wants to merge 1 commit into
Draft
Validate caller-supplied policy positions against the specificity invariant#1874ux-xd wants to merge 1 commit into
ux-xd wants to merge 1 commit into
Conversation
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
policies.create/policies.updateaccepted a caller-suppliedpositionverbatim 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:
resolveToolPolicytakes the FIRST matching rule per owner by position, and the most restrictive action across owners.positionForNewPatternplaces a new rule below every more-specific existing rule — but only when the client omitsposition. When a position IS sent (HTTPPOST/PATCH /policiespasses it through; the only validation on the whole write path isisValidPattern+isToolPolicyAction), it is stored as-is.Because
comparePolicyRowis a plain string compare and every keygenerateKeyBetweenemits sorts above strings like"0"or""("a0"is the smallest emitted key;0x30 < 0x61), any authenticated member can:or PATCH an existing rule's
positionto"0", and hoist a wildcard approve above the org's narrowerblock/require_approvalrules — silently weakening every org guardrail for the whole workspace. There is no role gate on the product plane to stop a member from writingowner: "org"policies; the only defense was the default placement, which an explicit position bypasses.The fix
isValidPositionForPatternenforces 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 realgenerateKeyBetweenoutput 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:
updatenow validates the action the same waycreatedoes; an unknown action previously stored fine and threwMatch.exhaustiveat resolve time, failing execution of every tool the rule matched.Verification
New tests in
policies.test.tsfail on the previous tree (position"0"is stored; the org block rule is shadowed) and pass with the fix: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.