From 453476f33adc74bf9bcb633fbc7f17b628eff203 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 07:54:49 +0000 Subject: [PATCH] fix(security): surface swallowed permission-set resolution failures (#2565) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolvePermissionSets swallowed metadata list() and sys_permission_set dbLoader failures silently — fail-closed (unresolvable sets grant nothing), but a transient DB error made custom permission sets vanish with no trace, leaving the resulting 403s undiagnosable. The evaluator now accepts an optional { logger } and emits one warn per failed source, naming the unresolved sets + the error; SecurityPlugin wires its logger into both call sites. Resolution behavior is byte-identical. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014y5kiH3aPLWtRRRGcVrXcT --- .changeset/permset-resolve-observability.md | 13 ++++++ .../src/permission-evaluator.ts | 27 ++++++++--- .../src/security-plugin.test.ts | 45 +++++++++++++++++++ .../plugin-security/src/security-plugin.ts | 2 + 4 files changed, 82 insertions(+), 5 deletions(-) create mode 100644 .changeset/permset-resolve-observability.md diff --git a/.changeset/permset-resolve-observability.md b/.changeset/permset-resolve-observability.md new file mode 100644 index 0000000000..e6acf4d40a --- /dev/null +++ b/.changeset/permset-resolve-observability.md @@ -0,0 +1,13 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(security): surface swallowed permission-set resolution failures (#2565) + +`PermissionEvaluator.resolvePermissionSets` swallowed metadata `list()` and +`sys_permission_set` dbLoader failures silently — fail-closed (unresolvable +sets grant nothing), but a transient DB error made custom permission sets +vanish with no trace, leaving the resulting 403s undiagnosable. The evaluator +now accepts an optional `{ logger }` and emits one `warn` per failed source, +naming the unresolved permission sets and the error. SecurityPlugin wires its +plugin logger into both call sites. Resolution behavior is byte-identical. diff --git a/packages/plugins/plugin-security/src/permission-evaluator.ts b/packages/plugins/plugin-security/src/permission-evaluator.ts index 75f2822250..8e4921168d 100644 --- a/packages/plugins/plugin-security/src/permission-evaluator.ts +++ b/packages/plugins/plugin-security/src/permission-evaluator.ts @@ -266,9 +266,15 @@ export class PermissionEvaluator { /** * Optional async loader for permission set names that aren't found in * metadata or bootstrap. Lets callers query user-defined permission - * sets persisted in `sys_permission_set`. Failures are swallowed. + * sets persisted in `sys_permission_set`. Failures are swallowed + * (fail-closed: unresolvable sets grant nothing) but SURFACED via + * `options.logger` — see #2565: without the warn, a transient DB error + * makes custom permission sets silently vanish and the resulting 403s + * are undiagnosable. */ - dbLoader?: (unresolved: string[]) => Promise + dbLoader?: (unresolved: string[]) => Promise, + /** Optional logger; only `warn` is used. Resolution behavior is unchanged. */ + options: { logger?: { warn?: (msg: string, meta?: Record) => void } } = {}, ): Promise { if (identifiers.length === 0) return []; @@ -283,8 +289,12 @@ export class PermissionEvaluator { ?? metadataService?.list?.('permissions') ?? []; allPermSets = typeof (listed as any)?.then === 'function' ? await listed : listed; - } catch { + } catch (e) { allPermSets = []; + options.logger?.warn?.( + '[security] permission-set metadata list() failed — falling back to bootstrap/db sources (#2565)', + { requested: identifiers, error: (e as Error)?.message }, + ); } if (!Array.isArray(allPermSets)) allPermSets = []; @@ -322,9 +332,16 @@ export class PermissionEvaluator { result.push(ps); } } - } catch { + } catch (e) { // Swallow — the request shouldn't fail just because the DB - // lookup is unavailable. + // lookup is unavailable (fail-closed: the unresolved sets simply + // grant nothing). But surface it: without this warn a transient + // DB error silently drops custom permission sets and the + // resulting 403s point nowhere near the cause (#2565). + options.logger?.warn?.( + '[security] sys_permission_set db lookup failed — unresolved sets grant nothing this request (#2565)', + { unresolved, error: (e as Error)?.message }, + ); } } } diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index c1daa0b670..7896b58a1a 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -1069,6 +1069,51 @@ describe('PermissionEvaluator', () => { expect(result).toEqual([psAdmin]); }); + it('warns (and keeps resolving) when the dbLoader throws — #2565 observability', async () => { + const evaluator = new PermissionEvaluator(); + const psAdmin = { name: 'admin_full_access' }; + const metadata = { list: vi.fn().mockReturnValue([psAdmin]) }; + const warns: Array<{ msg: string; meta?: Record }> = []; + const result = await evaluator.resolvePermissionSets( + ['admin_full_access', 'custom_sales'], + metadata, + [], + async () => { throw new Error('db down'); }, + { logger: { warn: (msg, meta) => warns.push({ msg, meta }) } }, + ); + // behavior unchanged: metadata-resolved set still returned, unresolved grants nothing + expect(result).toEqual([psAdmin]); + // ...but the swallow is surfaced, naming the unresolved sets + expect(warns.length).toBe(1); + expect(warns[0].msg).toContain('db lookup failed'); + expect(warns[0].meta?.unresolved).toEqual(['custom_sales']); + expect(warns[0].meta?.error).toBe('db down'); + }); + + it('warns (and keeps resolving via bootstrap) when metadata list() throws — #2565', async () => { + const evaluator = new PermissionEvaluator(); + const metadata = { list: vi.fn().mockImplementation(() => { throw new Error('index broken'); }) }; + const bootstrap = [{ name: 'member_default', objects: {} } as any]; + const warns: string[] = []; + const result = await evaluator.resolvePermissionSets( + ['member_default'], + metadata, + bootstrap, + undefined, + { logger: { warn: (msg) => warns.push(msg) } }, + ); + expect(result.map((p) => p.name)).toEqual(['member_default']); + expect(warns.some((w) => w.includes('metadata list() failed'))).toBe(true); + }); + + it('stays silent when no logger is provided (back-compat)', async () => { + const evaluator = new PermissionEvaluator(); + const metadata = { list: vi.fn().mockReturnValue([]) }; + await expect( + evaluator.resolvePermissionSets(['x'], metadata, [], async () => { throw new Error('db down'); }), + ).resolves.toEqual([]); + }); + it('matches by both role and explicit permission-set identifiers', async () => { const evaluator = new PermissionEvaluator(); const sets = [ diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index c66d465459..e2e82e5a97 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -929,6 +929,7 @@ export class SecurityPlugin implements Plugin { this.metadata, this.bootstrapPermissionSets, this.dbLoader, + { logger: this.logger }, ); // Post-resolution fallback — closes the fail-open hole where a populated // `roles` array maps to no permission set yet (no sys_role binding), which @@ -943,6 +944,7 @@ export class SecurityPlugin implements Plugin { this.metadata, this.bootstrapPermissionSets, this.dbLoader, + { logger: this.logger }, ); } return permissionSets;