🧷 fix: Preserve Document Priority on Section-Scoped Config Writes#13772
Conversation
The patch and tombstone admin-config handlers accept a priority field on the request body, which controls the position of the entire Config document in the precedence cascade. Today, that priority is written unconditionally, even when the caller holds only a section-scoped grant such as manage:configs:memory. On a document containing overrides for other sections, this lets a section-scoped caller silently reorder overrides they have no permission to author. This is a defense-in-depth fix for deployments using section-scoped grants. In a vanilla setup where admins hold broad manage:configs, the path is unreachable because the broad-capability short-circuit lets every priority change through, so the fix is a no-op for those callers. The change makes the contract safe-by-construction for any deployment that narrows the auth model, at no cost to upstream behavior.
There was a problem hiding this comment.
Pull request overview
This PR hardens the admin config field-patch and tombstone handlers so that document-level priority cannot be modified by callers who only have section-scoped config grants (e.g. manage:configs:memory). This prevents section-scoped callers from reordering unrelated overrides in the config precedence cascade.
Changes:
- Computes a single
hasBroadManage(manage:configs) check and uses it to gate whether request-suppliedpriorityis honored. - Strips/ignores
priorityon section-scoped patch/tombstone requests (while preserving existing or default priority) and emits alogger.warnwhen this occurs. - Adds Jest coverage for priority preservation behavior across patch and tombstone flows (including
priority: 0handling and “skip lookup when broad manage supplies priority”).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/api/src/admin/config.ts | Ignores caller-supplied priority unless the caller has broad manage:configs, preserving existing/default priority for section-scoped writes and logging when stripping occurs. |
| packages/api/src/admin/config.handler.spec.ts | Adds regression tests ensuring section-scoped callers cannot change document priority while broad-manage callers can, including priority: 0 cases and lookup-skipping behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
The admin-config field-patch and tombstone handlers accept a
priorityfield on the request body, which controls the position of the entire Config document in the precedence cascade. Today, that priority is written unconditionally, even when the caller holds only a section-scoped grant (e.g.manage:configs:memory). On a document that contains overrides for other sections, this lets a section-scoped caller silently reorder overrides they have no permission to author.This is a defense-in-depth fix that matters for deployments using section-scoped grants (
manage:configs:<section>). In a vanilla setup where admins hold broadmanage:configs, the path is unreachable because the broad-capability short-circuit lets every priority change through; the fix is a no-op for those callers. The change makes the contract safe-by-construction for any deployment that narrows the auth model.Concrete scenario
Tenant has two roles:
content-adminholdingmanage:configs:memoryonlyplatform-adminholding broadmanage:configsplatform-adminposts a config atpriority: 5containing bothmemoryandendpointsoverrides. Later,content-adminpatches amemory.contextfield and includespriority: 999in the body:Pre-PR: 200 OK, the entire document's
priorityjumps to 999, reordering theendpointsoverride in the cascade. Post-PR: 200 OK, document priority stays at 5; the section-scoped caller never touches priority. Alogger.warnis emitted on strip so operators can see the request was partially honored.Change Type
Testing
cd packages/api && npx jest src/admin/config.handler.spec.ts— 71/71 passing (8 new cases covering: section-scoped strip, default-fallback when no existing doc, broad-manage honors request,priority: 0preserved on broad write, existingpriority: 0preserved for section-scoped,findConfigByPrincipalskipped when broad provides priority, bothpatchConfigFieldandtombstoneConfigField)Checklist