From 84886c56fbdc21ee58b28bb91304bc892b54d19c Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Thu, 18 Jun 2026 12:39:44 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=B7=20fix:=20Preserve=20Document=20Pri?= =?UTF-8?q?ority=20on=20Section-Scoped=20Config=20Writes=20(#13772)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/api/src/admin/config.handler.spec.ts | 168 ++++++++++++++++++ packages/api/src/admin/config.ts | 34 +++- 2 files changed, 195 insertions(+), 7 deletions(-) diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index b3f8e9ee3b..7649d10ae9 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -647,6 +647,174 @@ describe('createAdminConfigHandlers', () => { }); }); + describe('patchConfigField: section-scoped priority preservation', () => { + it('ignores request-supplied priority when caller lacks broad manage:configs', async () => { + const { handlers, deps } = createHandlers({ + hasConfigCapability: jest.fn(async (_user, section) => section === 'memory'), + findConfigByPrincipal: jest + .fn() + .mockResolvedValue({ _id: 'c1', priority: 7, overrides: {} }), + }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { + entries: [{ fieldPath: 'memory.context', value: 'updated' }], + priority: 999, + }, + }); + const res = mockRes(); + + await handlers.patchConfigField(req, res); + + expect(res.statusCode).toBe(200); + const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0]; + expect(priorityArg).toBe(7); + }); + + it('falls back to default priority when no existing doc and caller lacks broad manage', async () => { + const { handlers, deps } = createHandlers({ + hasConfigCapability: jest.fn(async (_user, section) => section === 'memory'), + findConfigByPrincipal: jest.fn().mockResolvedValue(null), + }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { + entries: [{ fieldPath: 'memory.context', value: 'updated' }], + priority: 999, + }, + }); + const res = mockRes(); + + await handlers.patchConfigField(req, res); + + expect(res.statusCode).toBe(200); + const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0]; + expect(priorityArg).toBe(10); + }); + + it('honors request-supplied priority when caller holds broad manage:configs', async () => { + const { handlers, deps } = createHandlers({ + hasConfigCapability: jest.fn().mockResolvedValue(true), + }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { + entries: [{ fieldPath: 'memory.context', value: 'updated' }], + priority: 999, + }, + }); + const res = mockRes(); + + await handlers.patchConfigField(req, res); + + expect(res.statusCode).toBe(200); + const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0]; + expect(priorityArg).toBe(999); + expect(deps.findConfigByPrincipal).not.toHaveBeenCalled(); + }); + + it('preserves priority 0 when broad caller supplies it', async () => { + const { handlers, deps } = createHandlers({ + hasConfigCapability: jest.fn().mockResolvedValue(true), + }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { + entries: [{ fieldPath: 'memory.context', value: 'updated' }], + priority: 0, + }, + }); + const res = mockRes(); + + await handlers.patchConfigField(req, res); + + expect(res.statusCode).toBe(200); + const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0]; + expect(priorityArg).toBe(0); + }); + + it('preserves existing priority 0 for section-scoped callers', async () => { + const { handlers, deps } = createHandlers({ + hasConfigCapability: jest.fn(async (_user, section) => section === 'memory'), + findConfigByPrincipal: jest + .fn() + .mockResolvedValue({ _id: 'c1', priority: 0, overrides: {} }), + }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { + entries: [{ fieldPath: 'memory.context', value: 'updated' }], + priority: 999, + }, + }); + const res = mockRes(); + + await handlers.patchConfigField(req, res); + + expect(res.statusCode).toBe(200); + const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0]; + expect(priorityArg).toBe(0); + }); + }); + + describe('tombstoneConfigField: section-scoped priority preservation', () => { + it('ignores request-supplied priority when caller lacks broad manage:configs', async () => { + const { handlers, deps } = createHandlers({ + hasConfigCapability: jest.fn(async (_user, section) => section === 'memory'), + findConfigByPrincipal: jest + .fn() + .mockResolvedValue({ _id: 'c1', priority: 7, overrides: {} }), + }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { fieldPath: 'memory.context', priority: 999 }, + }); + const res = mockRes(); + + await handlers.tombstoneConfigField(req, res); + + expect(res.statusCode).toBe(200); + const [, , , , priorityArg] = deps.tombstoneConfigField.mock.calls[0]; + expect(priorityArg).toBe(7); + }); + + it('falls back to default priority when no existing doc and caller lacks broad manage', async () => { + const { handlers, deps } = createHandlers({ + hasConfigCapability: jest.fn(async (_user, section) => section === 'memory'), + findConfigByPrincipal: jest.fn().mockResolvedValue(null), + }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { fieldPath: 'memory.context', priority: 999 }, + }); + const res = mockRes(); + + await handlers.tombstoneConfigField(req, res); + + expect(res.statusCode).toBe(200); + const [, , , , priorityArg] = deps.tombstoneConfigField.mock.calls[0]; + expect(priorityArg).toBe(10); + }); + + it('honors request-supplied priority when caller holds broad manage:configs', async () => { + const { handlers, deps } = createHandlers({ + hasConfigCapability: jest.fn().mockResolvedValue(true), + }); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { fieldPath: 'memory.context', priority: 999 }, + }); + const res = mockRes(); + + await handlers.tombstoneConfigField(req, res); + + expect(res.statusCode).toBe(200); + const [, , , , priorityArg] = deps.tombstoneConfigField.mock.calls[0]; + expect(priorityArg).toBe(999); + expect(deps.findConfigByPrincipal).not.toHaveBeenCalled(); + }); + }); + describe('upsertConfigOverrides — Bug 2 regression', () => { it('returns 403 for empty overrides when user lacks MANAGE_CONFIGS', async () => { const { handlers } = createHandlers({ diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index 1a79e01384..de0b57b525 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -466,14 +466,16 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { return true; }); + const hasBroadManage = await hasConfigCapability(user, null, 'manage'); + if (validEntries.length === 0) { - if (!(await hasConfigCapability(user, null, 'manage'))) { + if (!hasBroadManage) { return res.status(403).json({ error: 'Insufficient permissions' }); } return res.status(200).json({ message: 'No actionable field entries provided' }); } - if (!(await hasConfigCapability(user, null, 'manage'))) { + if (!hasBroadManage) { const sections = [...new Set(validEntries.map((e) => getTopLevelSection(e.fieldPath)))]; const allowed = await Promise.all( sections.map((s) => hasConfigCapability(user, s as ConfigSection, 'manage')), @@ -496,8 +498,15 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { fields[entry.fieldPath] = entry.value; } + if (priority != null && !hasBroadManage) { + logger.warn( + `[adminConfig] Ignoring caller-supplied priority on section-scoped patch to ${principalType}/${principalId}: only broad manage:configs may modify document priority`, + ); + } + const requestedPriority = hasBroadManage ? priority : undefined; + const existing = - priority == null + requestedPriority == null ? await findConfigByPrincipal(principalType, principalId, { includeInactive: true }) : null; @@ -506,7 +515,7 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { principalId, principalModel(principalType), fields, - priority ?? existing?.priority ?? DEFAULT_PRIORITY, + requestedPriority ?? existing?.priority ?? DEFAULT_PRIORITY, ); invalidateConfigCaches?.(user.tenantId)?.catch((err) => @@ -557,7 +566,11 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { const section = getTopLevelSection(fieldPath); - if (!(await hasConfigCapability(user, section as ConfigSection, 'manage'))) { + const hasBroadManage = await hasConfigCapability(user, null, 'manage'); + if ( + !hasBroadManage && + !(await hasConfigCapability(user, section as ConfigSection, 'manage')) + ) { return res.status(403).json({ error: `Insufficient permissions for config section: ${section}`, }); @@ -570,8 +583,15 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { return res.status(200).json({ message: 'No actionable field path provided' }); } + if (priority != null && !hasBroadManage) { + logger.warn( + `[adminConfig] Ignoring caller-supplied priority on section-scoped tombstone for ${principalType}/${principalId}: only broad manage:configs may modify document priority`, + ); + } + const requestedPriority = hasBroadManage ? priority : undefined; + const existing = - priority == null + requestedPriority == null ? await findConfigByPrincipal(principalType, principalId, { includeInactive: true }) : null; @@ -580,7 +600,7 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { principalId, principalModel(principalType), fieldPath, - priority ?? existing?.priority ?? DEFAULT_PRIORITY, + requestedPriority ?? existing?.priority ?? DEFAULT_PRIORITY, ); invalidateConfigCaches?.(user.tenantId)?.catch((err) =>