Skip to content

Commit 1cd4264

Browse files
os-zhuangclaude
andauthored
test(security): pin server-managed field set + tenant-wall write guard; fix systemFields docs (#3058) (#3168)
#3058 proposed collapsing the three server-managed-field special cases (owner_id / public-form set / organization_id) into one declarative `systemManaged` schema concept. Assessment: the field roster is a closed set of platform columns, the three enforcement semantics are genuinely heterogeneous, and the real risk the issue names (drift between the sites) is closable without a new abstraction. This is the right-sized alternative — it hardens the drift seams and fills test gaps without introducing a speculative declaration layer. - objectql: new conformance test pinning PUBLIC_FORM_SERVER_MANAGED_FIELDS to an exact partition of (fields actually injected by applySystemFields + `id` + the `__search` companion) and (documented defense-in-depth reserved names: tenant_id / is_deleted / deleted_at). A newly-injected system field that is not added to the denylist now fails loudly here instead of leaking through the anonymous public-form surface — the drift vector that first surfaced as #3022. - plugin-security: add the missing package-level write-side unit tests for the step 3.7 organization_id tenant wall (insert forge / update re-point denied, matching-org and absent-value writes pass, isSystem exempt). Previously only the read-side Layer 0 and the multitenant dogfood covered this. - spec: correct the stale systemFields JSDoc/describe on ObjectSchema to match the registry — the organization_id column is provisioned unconditionally (only its index is multi-tenant-gated), `audit` is a wired opt-out, and owner_id provisioning is governed by the `ownership` property, not the `owner` key. No runtime behavior changes: the existing owner/tenant/public-form guards and all 209 security-plugin tests are unchanged. Claude-Session: https://claude.ai/code/session_014343Qv6DFAykuAJc9yjANb Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2049b6a commit 1cd4264

3 files changed

Lines changed: 215 additions & 15 deletions

File tree

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import { PUBLIC_FORM_SERVER_MANAGED_FIELDS } from '@objectstack/spec/security';
5+
import { SystemFieldName } from '@objectstack/spec/system';
6+
import { applySystemFields, SEARCH_COMPANION_FIELD } from './index.js';
7+
8+
// ---------------------------------------------------------------------------
9+
// [#3058] Single-source conformance for the server-managed field set.
10+
//
11+
// `PUBLIC_FORM_SERVER_MANAGED_FIELDS` (@objectstack/spec/security) is the shared
12+
// denylist that BOTH the anonymous public-form enforcement points read — the
13+
// REST form-route allow-list (@objectstack/rest) and the data-layer grant strip
14+
// (@objectstack/plugin-security). It has to name every column the server manages
15+
// on its own, because on the anonymous surface nothing else guards them.
16+
//
17+
// The risk it carries is DRIFT: the actual server-managed columns are assembled
18+
// from three unrelated injection sites (the registry's `applySystemFields`, the
19+
// search-companion, the driver primary key) plus a few defense-in-depth reserved
20+
// names — none of which imports the denylist. When #3022 first shipped this set,
21+
// the fields were hand-copied; a new injected system field added later would
22+
// silently leak through the public-form surface with no test to catch it.
23+
//
24+
// This test pins the denylist to an EXACT partition of two documented groups, so
25+
// adding an injected system field (or a stray denylist entry) fails loudly here
26+
// with a message pointing at what to reconcile — rather than becoming a live
27+
// anonymous-write hole. It deliberately does NOT re-hardcode the 11 names; it
28+
// derives the injected group from the real injection code.
29+
// ---------------------------------------------------------------------------
30+
describe('[#3058] PUBLIC_FORM_SERVER_MANAGED_FIELDS conformance', () => {
31+
// Group A — columns OPEN-CORE actively injects onto a plain author-defined
32+
// business object. Enumerated from the real injection code, not re-listed, so
33+
// a new injected field is caught automatically.
34+
const injectedByRegistry = (): string[] => {
35+
const base: any = { name: 'proj', fields: { title: { type: 'text' } } };
36+
// multiTenant:true exercises the widest injection (organization_id included).
37+
const withSys = applySystemFields(base, { multiTenant: true });
38+
return Object.keys(withSys.fields ?? {}).filter((k) => !(k in base.fields));
39+
};
40+
41+
const activelyInjected = (): Set<string> =>
42+
new Set<string>([
43+
...injectedByRegistry(), // organization_id, created_at, created_by, updated_at, updated_by, owner_id
44+
SEARCH_COMPANION_FIELD, // '__search' — hidden search-normalization companion (search-companion.ts)
45+
SystemFieldName.ID, // 'id' — driver-provisioned primary key
46+
]);
47+
48+
// Group B — names in the denylist that open-core does NOT inject as a schema
49+
// field, kept as defense-in-depth so a forged value can never be admitted on
50+
// the anonymous surface even where the column is contributed elsewhere:
51+
// • tenant_id — legacy/enterprise tenant key (not injected by open-core).
52+
// • is_deleted / deleted_at — soft-delete state, written by the lifecycle/
53+
// trash layer at runtime, never client-suppliable on a public form.
54+
const reservedDefenseInDepth = new Set<string>([
55+
SystemFieldName.TENANT_ID, // 'tenant_id'
56+
'is_deleted',
57+
SystemFieldName.DELETED_AT, // 'deleted_at'
58+
]);
59+
60+
it('registry injection actually produces the fields this test reasons about', () => {
61+
// Guards the test itself: if applySystemFields stops injecting these, the
62+
// conformance assertions below would pass vacuously.
63+
const injected = new Set(injectedByRegistry());
64+
for (const f of ['organization_id', 'created_at', 'created_by', 'updated_at', 'updated_by', 'owner_id']) {
65+
expect(injected.has(f)).toBe(true);
66+
}
67+
});
68+
69+
it('every server-injected system field is on the public-form denylist (no anonymous-write leak)', () => {
70+
// The load-bearing direction: a new injected system field MUST be added to
71+
// PUBLIC_FORM_SERVER_MANAGED_FIELDS, or it becomes client-suppliable on the
72+
// anonymous public-form surface.
73+
for (const field of activelyInjected()) {
74+
expect(
75+
PUBLIC_FORM_SERVER_MANAGED_FIELDS.has(field),
76+
`server-injected field '${field}' is missing from PUBLIC_FORM_SERVER_MANAGED_FIELDS ` +
77+
`(packages/spec/src/security/public-form.ts) — add it or it leaks through the public-form surface`,
78+
).toBe(true);
79+
}
80+
});
81+
82+
it('the denylist is exactly (actively-injected ∪ documented-reserved) — no stray or missing entries', () => {
83+
const accountedFor = new Set<string>([...activelyInjected(), ...reservedDefenseInDepth]);
84+
const denylist = new Set<string>(PUBLIC_FORM_SERVER_MANAGED_FIELDS);
85+
86+
// No denylist entry is unexplained (a name nothing injects and not documented
87+
// as reserved → either dead weight or a mis-categorized new field).
88+
for (const field of denylist) {
89+
expect(
90+
accountedFor.has(field),
91+
`'${field}' is on PUBLIC_FORM_SERVER_MANAGED_FIELDS but is neither injected by open-core ` +
92+
`nor listed as defense-in-depth reserved in this test — classify it`,
93+
).toBe(true);
94+
}
95+
// And every accounted-for name is on the denylist (covered field-by-field for
96+
// the injected group above; this closes the reserved group too).
97+
for (const field of accountedFor) {
98+
expect(denylist.has(field), `'${field}' should be on PUBLIC_FORM_SERVER_MANAGED_FIELDS`).toBe(true);
99+
}
100+
// Exact partition ⇒ identical cardinality (catches a duplicate/renamed entry).
101+
expect(denylist.size).toBe(accountedFor.size);
102+
});
103+
});

packages/plugins/plugin-security/src/security-plugin.test.ts

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,96 @@ describe('SecurityPlugin', () => {
384384
});
385385
});
386386

387+
// -------------------------------------------------------------------------
388+
// [ADR-0095 D1 / #2937 Finding 1] step 3.7 — the Layer 0 tenant wall on the
389+
// WRITE post-image. A SUPPLIED (non-empty) `organization_id` in an insert /
390+
// update payload must satisfy the same Layer 0 filter the read side uses;
391+
// a forged (insert) or re-pointed (update) value that would land the row in
392+
// another tenant is denied fail-closed. Previously only the read-side Layer 0
393+
// and the multitenant dogfood covered this — these are the package-level
394+
// write-side unit tests for the wall.
395+
//
396+
// A plain-CRUD permission set with NO `tenant_isolation` RLS policy is used on
397+
// purpose, so Layer 0 (step 3.7) — not a business RLS `check` (step 3.6) — is
398+
// the sole enforcer under test. `orgScoping: true` flips
399+
// `SecurityPlugin.orgScopingEnabled`, which is what makes Layer 0 active.
400+
// -------------------------------------------------------------------------
401+
describe('organization_id tenant wall on writes (step 3.7)', () => {
402+
const memberSet: PermissionSet = {
403+
name: 'member_default',
404+
label: 'Member',
405+
objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } },
406+
} as any;
407+
408+
const boot = async (findOneImpl?: (query: any) => any) => {
409+
const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
410+
const harness = makeMiddlewareCtx({ permissionSets: [memberSet], orgScoping: true, findOneImpl });
411+
await plugin.init(harness.ctx);
412+
await plugin.start(harness.ctx);
413+
return harness;
414+
};
415+
const memberCtx = (extra: Record<string, unknown> = {}) =>
416+
({ userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], ...extra });
417+
418+
it('insert forging a cross-tenant organization_id is denied', async () => {
419+
const harness = await boot();
420+
const opCtx: any = {
421+
object: 'task', operation: 'insert', data: { name: 'A', organization_id: 'org-2' },
422+
context: memberCtx(),
423+
};
424+
await expect(harness.run(opCtx)).rejects.toThrow(/would place .* in another tenant/);
425+
});
426+
427+
it("insert supplying the caller's own organization_id passes", async () => {
428+
const harness = await boot();
429+
const opCtx: any = {
430+
object: 'task', operation: 'insert', data: { name: 'A', organization_id: 'org-1' },
431+
context: memberCtx(),
432+
};
433+
await harness.run(opCtx); // must not throw
434+
expect(opCtx.data.organization_id).toBe('org-1');
435+
});
436+
437+
it('update re-pointing organization_id to another tenant is denied', async () => {
438+
// Pre-image is visible (in-tenant), so step 2.7 passes and 3.7 is reached.
439+
const harness = await boot(() => ({ id: 't1', organization_id: 'org-1' }));
440+
const opCtx: any = {
441+
object: 'task', operation: 'update', data: { id: 't1', organization_id: 'org-2' },
442+
context: memberCtx(),
443+
};
444+
await expect(harness.run(opCtx)).rejects.toThrow(/would place .* in another tenant/);
445+
});
446+
447+
it("update echoing the caller's own organization_id passes", async () => {
448+
const harness = await boot(() => ({ id: 't1', organization_id: 'org-1' }));
449+
const opCtx: any = {
450+
object: 'task', operation: 'update', data: { id: 't1', name: 'renamed', organization_id: 'org-1' },
451+
context: memberCtx(),
452+
};
453+
await harness.run(opCtx); // must not throw
454+
});
455+
456+
it("a write that doesn't supply organization_id is untouched (auto-stamp's job)", async () => {
457+
const harness = await boot();
458+
const opCtx: any = {
459+
object: 'task', operation: 'insert', data: { name: 'A' },
460+
context: memberCtx(),
461+
};
462+
await harness.run(opCtx); // must not throw — no cross-tenant check on an absent value
463+
// SecurityPlugin never stamps organization_id (that's plugin-org-scoping's job).
464+
expect(opCtx.data.organization_id).toBeUndefined();
465+
});
466+
467+
it('a system-context write bypasses the wall (import / migration / SYSTEM_CTX)', async () => {
468+
const harness = await boot();
469+
const opCtx: any = {
470+
object: 'task', operation: 'insert', data: { name: 'A', organization_id: 'org-2' },
471+
context: memberCtx({ isSystem: true }),
472+
};
473+
await harness.run(opCtx); // isSystem short-circuits the whole middleware — legit cross-org moves unaffected
474+
});
475+
});
476+
387477
it('without org-scoping plugin — strips tenant_isolation RLS so find applies no tenant where', async () => {
388478
const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
389479
const harness = makeMiddlewareCtx({ permissionSets: [tenantPolicySet] });

packages/spec/src/data/object.zod.ts

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -692,32 +692,39 @@ const ObjectSchemaBase = z.object({
692692
*
693693
* The `SchemaRegistry` augments every user object with a small set of
694694
* implicit system fields at registration time so authors don't have to
695-
* declare them per-object (Salesforce-style). Currently injected:
695+
* declare them per-object (Salesforce-style). Currently injected
696+
* (`packages/objectql/src/registry.ts` → `applySystemFields`):
696697
*
697-
* - `organization_id` — `lookup → sys_organization`. Injected only when
698-
* the kernel runs in multi-tenant mode (`OS_MULTI_ORG_ENABLED === 'true'`;
699-
* default is off — single-tenant).
700-
* Required for the default `tenant_isolation` RLS policy and the
701-
* SecurityPlugin's auto-fill on insert to take effect.
698+
* - `organization_id` — `lookup → sys_organization`. The COLUMN is
699+
* provisioned unconditionally (subject to the opt-outs below); only its
700+
* DB index is gated on multi-tenant mode. It stays NULL on single-tenant
701+
* stacks and is auto-stamped on insert by `@objectstack/organizations`
702+
* (the `org-scoping` service) in multi-tenant mode.
703+
* - Audit columns — `created_at`, `created_by`, `updated_at`, `updated_by`
704+
* (`readonly` + `system`). Gated by `audit` below.
705+
* - `owner_id` — `lookup → sys_user`, auto-provisioned on user-authored
706+
* business objects (auto-stamped to the creating user on insert;
707+
* reassignable). Governed by the object-level `ownership` property
708+
* (`'user' | 'org' | 'none'`), NOT by `owner` below.
702709
*
703710
* Author-declared fields with the same name always win over injection
704-
* (no overwrite). Objects with `managedBy` set are skipped entirely —
705-
* better-auth/system/platform tables already declare what they need.
711+
* (no overwrite). Objects with `managedBy` set (and the `sys_*` namespace)
712+
* are skipped for ownership; `managedBy: 'better-auth'` is skipped entirely —
713+
* better-auth's own migrations own that column layout.
706714
*
707715
* Set `systemFields: false` to opt the object out completely. Pass an
708-
* options object to selectively disable individual injections (currently
709-
* only `tenant`, but reserved keys `owner`/`audit` are pre-defined for
710-
* future expansion).
716+
* options object to selectively disable individual injections (`tenant`,
717+
* `audit`).
711718
*
712-
* @default undefined (= injection enabled, gated by kernel mode)
719+
* @default undefined (= injection enabled)
713720
*/
714721
systemFields: z
715722
.union([
716723
z.literal(false),
717724
z.object({
718-
tenant: z.boolean().optional().describe('Inject organization_id (multi-tenant only). Default true.'),
719-
owner: z.boolean().optional().describe('Reserved for future owner_id auto-injection.'),
720-
audit: z.boolean().optional().describe('Reserved for future created_by/updated_by auto-injection.'),
725+
tenant: z.boolean().optional().describe('Inject the organization_id column. Default true (the column is always provisioned; the multi-tenant flag governs only its index).'),
726+
owner: z.boolean().optional().describe('Unwired: owner_id provisioning is governed by the object-level `ownership` property (`user`/`org`/`none`), not this key. Reserved.'),
727+
audit: z.boolean().optional().describe('Inject the audit columns (created_at/created_by/updated_at/updated_by). Default true.'),
721728
}),
722729
])
723730
.optional()

0 commit comments

Comments
 (0)