From 89bd8d6707b1b21331aad6bba1224657911d8cff Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:11:10 +0800 Subject: [PATCH] fix(service-settings): user-keyed loadRows includes tenant/global rows on the engine branch (#11228) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine branch built `where.user_id = userId`, which excludes every upper-scope row (user_id NULL) — while the in-memory branch's predicate includes them. Two consumers search the ONE result set a user-keyed load returns, so on every engine-bound deployment: 1. resolveKey's user→tenant→global cascade fell straight through to the manifest default whenever the user had no personal row, silently ignoring persisted tenant/global values; and 2. the Phase-2 upper-scope lock check found no locked tenant/global row, so a lock that should refuse user-scope writes never fired — a policy bypass, not just a stale read. The suite stayed green because the in-memory double answered correctly (the #4434 class — a double looser than the engine — in a WHERE clause): the shared fake matcher did bare field equality, so it would also have silently matched nothing against the new $or. Both test harnesses now implement $or and THROW on any other combinator, and the new suite pins the fallback, the other-user exclusion, an engine/memory differential, and the lock refusal on the engine path. Co-Authored-By: Claude Fable 5 --- .changeset/settings-loadrows-upper-scope.md | 5 + .../src/settings-getmany.test.ts | 22 ++- .../src/settings-loadrows-scope.test.ts | 127 ++++++++++++++++++ .../service-settings/src/settings-service.ts | 9 +- 4 files changed, 156 insertions(+), 7 deletions(-) create mode 100644 .changeset/settings-loadrows-upper-scope.md create mode 100644 packages/services/service-settings/src/settings-loadrows-scope.test.ts diff --git a/.changeset/settings-loadrows-upper-scope.md b/.changeset/settings-loadrows-upper-scope.md new file mode 100644 index 0000000000..f98df1819e --- /dev/null +++ b/.changeset/settings-loadrows-upper-scope.md @@ -0,0 +1,5 @@ +--- +"@objectstack/service-settings": patch +--- + +`loadRows` user-keyed engine loads now include tenant/global rows (`$or` over `user_id`/upper scopes), mirroring the in-memory branch. Fixes the user→tenant→global read cascade dying at the user level and upper-scope locks never firing on user-scope writes, on engine-bound deployments (#11228). diff --git a/packages/services/service-settings/src/settings-getmany.test.ts b/packages/services/service-settings/src/settings-getmany.test.ts index c0d1368f70..b8e2d321a1 100644 --- a/packages/services/service-settings/src/settings-getmany.test.ts +++ b/packages/services/service-settings/src/settings-getmany.test.ts @@ -21,13 +21,23 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { SettingsService } from './settings-service.js'; -function makeEngine(rows: Array>) { - const find = vi.fn(async (_obj: string, opts: any) => { - const w = opts?.where ?? {}; - return rows.filter((r) => - Object.entries(w).every(([k, v]) => (r as any)[k] === v), - ); +// WHERE-matcher gate: implement exactly the combinators the service emits and +// THROW on the rest — a bare field-equality read of `$or` would silently match +// nothing, which is how a fake matcher lies (#11228 hid behind exactly that). +function matches(row: Record, where: Record): boolean { + return Object.entries(where).every(([k, v]) => { + if (k === '$or') { + return (v as Array>).some((b) => matches(row, b)); + } + if (k.startsWith('$')) throw new Error(`fake matcher: unimplemented combinator ${k}`); + return (row as any)[k] === v; }); +} + +function makeEngine(rows: Array>) { + const find = vi.fn(async (_obj: string, opts: any) => + rows.filter((r) => matches(r, opts?.where ?? {})), + ); return { find, insert: vi.fn(), update: vi.fn(), delete: vi.fn(), count: vi.fn(), diff --git a/packages/services/service-settings/src/settings-loadrows-scope.test.ts b/packages/services/service-settings/src/settings-loadrows-scope.test.ts new file mode 100644 index 0000000000..1358c931c5 --- /dev/null +++ b/packages/services/service-settings/src/settings-loadrows-scope.test.ts @@ -0,0 +1,127 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11228] `loadRows` — a user-keyed load must include tenant/global rows. + * + * The engine branch used to build `where.user_id = userId`, which excludes + * every upper-scope row (they carry `user_id NULL`), while the in-memory + * branch's predicate includes them. Two consumers search the ONE result set a + * user-keyed load returns, so on every engine-bound deployment — i.e. every + * real one — + * + * 1. `resolveKey`'s user→tenant→global cascade fell straight through to the + * manifest default whenever the user had no personal row, silently + * ignoring persisted tenant/global values; and + * 2. the Phase-2 upper-scope lock check found no locked tenant/global row, + * so a lock that should refuse user-scope writes never fired. + * + * The suite stayed green because the in-memory double answered correctly — + * the #4434 class (a double looser than the engine), in a WHERE clause. + * Pinned here: both consumers against the ENGINE branch, plus a differential + * check that the engine and memory branches resolve identically. + * + * Upper-scope rows for a user-declared key are seeded store-level in these + * fixtures: the public write path always lands at the key's DECLARED scope, + * while the cascade and the lock check are explicitly written to honor rows + * at any upper scope, however they were provisioned. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { SettingsService } from './settings-service.js'; +import { SettingsLockedError } from './settings-service.types.js'; + +// Same deliberately-strict matcher as settings-getmany.test.ts: `$or` is +// implemented, every other combinator throws (a silent field-name read of +// `$or` is exactly how this defect hid). +function matches(row: Record, where: Record): boolean { + return Object.entries(where).every(([k, v]) => { + if (k === '$or') { + return (v as Array>).some((b) => matches(row, b)); + } + if (k.startsWith('$')) throw new Error(`fake matcher: unimplemented combinator ${k}`); + return (row as any)[k] === v; + }); +} + +function makeEngine(rows: Array>) { + const find = vi.fn(async (_obj: string, opts: any) => + rows.filter((r) => matches(r, opts?.where ?? {})), + ); + return { + find, + insert: vi.fn(async (_obj: string, data: Record) => data), + update: vi.fn(), delete: vi.fn(), count: vi.fn(), + }; +} + +const MANIFEST = { + namespace: 'localization', + label: 'Localization', + specifiers: [ + { key: 'timezone', type: 'string', scope: 'user', default: 'UTC' }, + ], +} as any; + +// The defect fixture: a persisted GLOBAL value for a user-scope key, and a +// user who has no personal row. Values are stored plain, as the +// persist path writes them. +const GLOBAL_ROW = { + namespace: 'localization', key: 'timezone', scope: 'global', + value: 'America/New_York', user_id: null, + value_enc: null, encrypted: false, locked: false, locked_reason: null, + updated_at: '2026-08-01T00:00:00.000Z', updated_by: null, +}; +const U2_ROW = { + namespace: 'localization', key: 'timezone', scope: 'user', + value: 'Asia/Tokyo', user_id: 'u2', + value_enc: null, encrypted: false, locked: false, locked_reason: null, + updated_at: '2026-08-01T00:00:00.000Z', updated_by: null, +}; + +function makeEngineService(rows: Array>) { + const svc = new SettingsService(); + svc.registerManifest(MANIFEST); + svc.bindEngine(makeEngine(rows) as any); + return svc; +} + +function makeMemoryService(rows: Array>) { + const svc = new SettingsService(); + svc.registerManifest(MANIFEST); + (svc as any).memory.push(...rows.map((r) => ({ ...r }))); + return svc; +} + +describe('[#11228] loadRows user-keyed load includes upper-scope rows', () => { + it('engine branch: a user with no personal row falls back to the persisted global value, not the default', async () => { + const svc = makeEngineService([GLOBAL_ROW]); + const got = await svc.get('localization', 'timezone', { userId: 'u1' }); + expect(got.value).toBe('America/New_York'); + expect(got.source).toBe('global'); + }); + + it("engine branch: another user's personal row is still excluded", async () => { + const svc = makeEngineService([U2_ROW]); + const got = await svc.get('localization', 'timezone', { userId: 'u1' }); + expect(got.value).toBe('UTC'); + expect(got.source).toBe('default'); + }); + + it('engine and memory branches resolve the same fixture identically (differential)', async () => { + const rows = [GLOBAL_ROW, U2_ROW]; + const engineSvc = makeEngineService(rows); + const memorySvc = makeMemoryService(rows); + for (const ctx of [{ userId: 'u1' }, { userId: 'u2' }, {}]) { + const a = await engineSvc.get('localization', 'timezone', ctx); + const b = await memorySvc.get('localization', 'timezone', ctx); + expect({ value: a.value, source: a.source }).toEqual({ value: b.value, source: b.source }); + } + }); + + it('engine branch: a locked global row refuses a user-scope write (Phase-2 lock reaches the engine path)', async () => { + const svc = makeEngineService([{ ...GLOBAL_ROW, locked: true }]); + await expect( + svc.set('localization', 'timezone', 'Asia/Tokyo', { userId: 'u1' }), + ).rejects.toThrow(SettingsLockedError); + }); +}); diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index df5550e076..cf4a18920d 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -2146,7 +2146,14 @@ export class SettingsService { private async loadRows(namespace: string, userId: string | null): Promise { if (this.engine) { const where: Record = { namespace }; - if (userId !== null) where.user_id = userId; + // A user-keyed load must still see tenant/global rows (user_id NULL): + // resolveKey's user→tenant→global cascade and the Phase-2 upper-scope + // lock check both search THIS one result set, so a bare user_id + // equality starves them of every upper-scope row on engine-bound + // deployments while the in-memory branch below includes them (#11228). + if (userId !== null) { + where.$or = [{ user_id: userId }, { scope: 'tenant' }, { scope: 'global' }]; + } // Settings rows include platform-wide (`global` scope, tenant_id=null) // entries; bypass the tenant-scoping audit warning so loads work // uniformly across global/tenant/user without log noise. Per-tenant