Skip to content

Commit 71ccf4f

Browse files
author
Claude
committed
Merge remote-tracking branch 'origin/main' into claude/issue-7307-delete-restricted-user-copy
2 parents 856f11a + b948a41 commit 71ccf4f

25 files changed

Lines changed: 2672 additions & 107 deletions
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
---
2+
"@objectstack/plugin-security": patch
3+
"@objectstack/spec": patch
4+
---
5+
6+
fix(plugin-security): `checkAuthoredRowWrite` answers the declaration, not the caller's read scope (#7281)
7+
8+
`ISecurityService.checkAuthoredRowWrite` asks one question — *does an
9+
app-authored row-level widener admit this row for this write?* — and it resolved
10+
that question by re-reading the row through the **caller's own** execution
11+
context. That `findOne` re-enters the middleware chain, so `plugin-sharing`'s
12+
READ filter applied: on a `private`-OWD object a cross-owner row is invisible to
13+
the caller, the read answered null, and the verdict was `abstain` for a row the
14+
declaration names by predicate.
15+
16+
Measured on the real stack across two objects identical in every respect except
17+
their OWD — same widener text, same principal, same cross-owner row shape:
18+
19+
| OWD | verdict before | verdict after |
20+
|---|---|---|
21+
| `public_read` | `admit` | `admit` |
22+
| `private` | **`abstain`** | **`admit`** |
23+
24+
So the by-id widener surface was live on read-open objects and stood down on
25+
read-closed ones, discriminated by a property the widener's author never
26+
mentions — and `private` is the posture #5493 built that surface for. The
27+
maintainer ruled it a defect (2026-08-10): the verdict is about the row and the
28+
policy, not about what the caller may see. The probe read now resolves under an
29+
elevated, principal-less scope.
30+
31+
**This does not widen anything.** The predicate carries the whole of the
32+
question and travels in the query rather than in the scope: `{id} AND
33+
layer0(tenant wall) AND layer1(app-authored policies)`, both layers still
34+
compiled from the caller's own permission sets and tenant before the read, and
35+
the read is projected to `id` so the probe can only ever learn *that* a row
36+
matches. A row in another tenant, a row no authored policy matches, and a caller
37+
holding no authored policy at all all still answer `abstain` — pinned, including
38+
by mutation: delete the tenant layer from the predicate and the cross-tenant case
39+
goes red. `admit` also remains evidence and never authorization: the by-id write
40+
pre-image gate still resolves the write under the caller's own context and
41+
refuses on its own terms.
42+
43+
One consequence is stated plainly rather than papered over: because that
44+
pre-image gate performs the same caller-scoped read, a `private`-OWD cross-owner
45+
by-id write is **still refused end-to-end** after this change — now by the
46+
row-level gate (`PERMISSION_DENIED`, "…(row-level security)") rather than by the
47+
sharing middleware's `FORBIDDEN`. Whether a write should reach a row the caller
48+
cannot read is a separate contract question about that gate's read scope, and it
49+
is not settled here. Both behaviours are pinned on the real stack.
50+
51+
The `@objectstack/spec` half is documentation only: `ISecurityService`'s contract
52+
listed "the row is unreadable" among the `abstain` cases, which is exactly the
53+
conflation the ruling removed. No signature, shape or vocabulary changes, and the
54+
method stays optional and fail-closed.
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
---
2+
'@objectstack/rest': patch
3+
---
4+
5+
REST: a declared 5xx status now survives on the CRUD data routes
6+
7+
`mapDataError`'s explicit-status passthrough accepted only 4xx, while
8+
`resolveErrorResponse` (the door every metadata/UI/discovery/batch route uses)
9+
accepts 400-599. The same thrown error therefore got two different answers
10+
depending on which route caught it, and on the data routes a producer's
11+
declared 5xx was overwritten — the status re-derived from the message text, or
12+
falling through to `500 INTERNAL_ERROR`.
13+
14+
The passthrough is now 400-599 on both doors, with the same disposition #5437
15+
already ruled for a declared server fault: **keep the status, keep the
16+
machine-readable `code`, drop the prose**. The `code` half reads
17+
`declaresServerFault` from `@objectstack/types`, so an empty or non-string code
18+
is not mistaken for an ADR-0112 declaration and nothing is invented when the
19+
producer named no code.
20+
21+
User-visible effect: an aggregate function a SQL backend cannot compile
22+
(`count_distinct` / `array_agg` / `string_agg`) now answers
23+
`501 NOT_IMPLEMENTED` instead of `500 INTERNAL_ERROR`, and an upstream/
24+
dependency `502` / `503` reaches the caller as itself rather than as a generic
25+
500. The 4xx half is unchanged (wording truncated, `object` retained), no 5xx
26+
message text reaches the client, and the withheld text still reaches the
27+
operator log.
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
---
2+
'@objectstack/service-datasource': patch
3+
---
4+
5+
The `sqlite-wasm` and `mongodb` arms of the shared datasource driver factory now tell you how to install the optional driver package they are missing (#7385)
6+
7+
All three of `sqlite-wasm`, `mongodb` and `turso` are built from OPTIONAL packages, so all three have to answer "the package is not here". After #7314 fixed the libSQL arm, the other two still answered with the fault and nothing else:
8+
9+
```text
10+
sqlite-wasm driver requested but @objectstack/driver-sqlite-wasm is not installed (…).
11+
mongodb driver requested but @objectstack/driver-mongodb is not installed (…).
12+
```
13+
14+
No install command, no statement of what happens next, and not even the name of the datasource that failed — while the `turso` arm beside them stated all three. An admin who added a mongo datasource in Setup and one who added a libSQL datasource hit the same class of problem and got two different qualities of answer, decided by nothing but which driver they picked.
15+
16+
Both arms now answer through a shared builder, keeping the two discipline points #7384 landed under: the message NAMES THE DATASOURCE (several may be declared and only one of them is this engine), and it names exactly one fix with no escape hatch — no `OS_ALLOW_DRIVER_CONNECT_FAILURE` (it would only hide a package that does not exist) and no `OS_DATABASE_URL` / `--database` (they select the HOST's `default` datasource and can do nothing for the one that failed). The underlying import error is still interpolated in full, which is what keeps `isUnbuiltWorkspaceFailure` able to recognise a half-built checkout from these arms and re-route the remedy to `pnpm install && pnpm build`.
17+
18+
The consequence sentence is per-engine rather than copied. Mongo, like libSQL, is a server this process connects to, so a silent fallback would open a local database while the real server stayed untouched. `sqlite-wasm` has no remote to shadow, so it states its own truth instead: stepping down to the in-process memory driver would accept every write and drop it at shutdown, leaving the configured file empty, and stepping down to native `better-sqlite3` would need exactly the native addon a WASM datasource is chosen to avoid.
19+
20+
New exports, mirroring the libSQL pair, so a host that renders its own remedy reads one declaration instead of re-typing a command: `SQLITE_WASM_DRIVER_PACKAGE`, `SQLITE_WASM_DRIVER_INSTALL_COMMAND`, `missingSqliteWasmDriverMessage`, `MONGODB_DRIVER_PACKAGE`, `MONGODB_DRIVER_INSTALL_COMMAND`, `missingMongodbDriverMessage`.
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
feat(objectql): the option marked `default: true` is now the field's default on insert (#7246)
6+
7+
`SelectOption.default` has been authorable and spec-valid since the schema was
8+
written, and nothing on the insert path read it. `ObjectQL.applyFieldDefaults`
9+
resolved `f.defaultValue` — Expression envelopes, the `DEFAULT_VALUE_TOKENS`
10+
family, then static literals — and never looked at `options`. So this, which
11+
reads like a declaration of the initial value:
12+
13+
```ts
14+
status: Field.select({
15+
label: 'Status',
16+
options: [
17+
{ label: 'Draft', value: 'draft', default: true },
18+
{ label: 'Approved', value: 'approved' },
19+
],
20+
}),
21+
```
22+
23+
stored **null** on a create that omitted the field, not `draft`.
24+
25+
The key's one consumer anywhere in the repo was lint's `isNullableField`, which
26+
concluded from it that the column was **always valued** — and that verdict is
27+
build-breaking. So the single place that read the key trusted it, while the
28+
place that would have made it true ignored it: a predicate over such a field
29+
could be silenced by a heuristic resting on a declaration nothing honoured.
30+
31+
**After** (maintainer ruling on #7246, ADR-0049 enforce leg): a field that
32+
declares no `defaultValue` falls back to the option marked `default: true`, on
33+
every driver, resolved by the engine exactly as the token family is.
34+
35+
- **`defaultValue` wins when both are declared** — the more specific
36+
declaration. It names a value for *this* field; the option flag describes the
37+
shared option list. When the two disagree the flag stays inert, as it was
38+
everywhere before.
39+
- **Presence is the engine's own `dv == null` test.** `defaultValue: ''` is a
40+
real default and still wins; the fallback fires only when `defaultValue` is
41+
absent by that test.
42+
- **The fallback resolves in the `defaultValue == null` arm**, downstream of the
43+
token and envelope branches, so an option value is always a plain literal — an
44+
option spelled `current_user` stores those twelve characters rather than the
45+
acting user's id.
46+
- **`multiple: true` assembles an array** of every marked option in declaration
47+
order, because that field stores an Array/JSON; a single-valued field with
48+
several marked options takes the first.
49+
- **No physical column DEFAULT** is emitted for an option-default. The engine is
50+
the one place the two spellings are ranked, the multi-select shape has no
51+
scalar DDL form, and emitting would give new databases a default that older
52+
ones on identical metadata lack with nothing to report the divergence. The
53+
reasoning is recorded on `SqlDriver.applyDeclaredColumnDefault` and pinned by
54+
test.
55+
56+
**Migration.** Metadata declaring an option `default: true` on a field with no
57+
`defaultValue` changes insert behaviour: records that used to be born with a
58+
null in that column are now born with the marked option. That is the behaviour
59+
the declaration always described. In the shipped corpus this covers 30 fields
60+
across the showcase, CRM and todo example apps and the downstream-contract QA
61+
fixture — all of them status/stage/priority selects whose marked option is the
62+
intended initial state. To keep the previous behaviour, drop the `default: true`
63+
flag from the option; to make the value explicit, declare `defaultValue`
64+
alongside it, which now outranks the flag.

examples/app-showcase/test/hook-body-persisted-writes.test.ts

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,17 @@ describe('#7258 — app-showcase sandboxed `body` hooks reach the persisted row'
205205

206206
const stored = await readBack(engine, 'showcase_inquiry', String(created.id));
207207
// The record's own fields are what a body enumerates...
208-
expect(stored.company).toBe('KEYS[email,message,name] hasData=undefined');
208+
//
209+
// `status` is in that list without any hook having run, and is not the
210+
// caller's (#7246): `applyFieldDefaults` runs at the TOP of the insert
211+
// middleware, so engine-resolved defaults are already on the record before
212+
// the first `beforeInsert` body sees it. `showcase_inquiry.status` marks
213+
// `{ value: 'new', default: true }`, and the engine now honours that option
214+
// flag exactly as it has always honoured a `defaultValue`. The priority-10
215+
// ordering this case is really about is unaffected — `source` is absent
216+
// here precisely because it is stamped by `StampInquiryDefaultsHook` at
217+
// priority 50, which still has not run.
218+
expect(stored.company).toBe('KEYS[email,message,name,status] hasData=undefined');
209219
// ...and the write the probe made through that flat view landed in the row,
210220
// which is the second half of `installFlatInput`'s contract.
211221
expect(stored.status).toBe('new');
@@ -223,8 +233,23 @@ describe('#7258 — app-showcase sandboxed `body` hooks reach the persisted row'
223233
ctx as never,
224234
);
225235
const storedInquiry = await readBack(engine, 'showcase_inquiry', String(inquiry.id));
226-
expect(storedInquiry.status == null).toBe(true);
236+
// `source` is the whole vacuity witness for this hook now (#7246).
237+
//
238+
// It used to be that BOTH of the hook's stamps came back null with the hook
239+
// unbound. `status` no longer does, and not because a hook ran: the field
240+
// declares `{ value: 'new', default: true }`, and the ENGINE now applies
241+
// that option default on insert. So `status` can no longer discriminate
242+
// "the hook ran" from "the hook did not" — it reads `'new'` either way, and
243+
// asserting `'new'` here would make this half of the reverse check assert
244+
// the same fact as the forward case, i.e. nothing.
245+
//
246+
// `source` still can: `showcase_inquiry.source` is a plain `Field.text`
247+
// with no options and no `defaultValue`, so `'web'` has exactly one
248+
// producer — `StampInquiryDefaultsHook`. The pin therefore moves onto it
249+
// rather than being softened, and the engine-owned value is asserted
250+
// separately for what it now is.
227251
expect(storedInquiry.source == null).toBe(true);
252+
expect(storedInquiry.status).toBe('new'); // engine option default, NOT the hook
228253

229254
const account: any = await engine.insert(
230255
'showcase_account', { name: 'Initech', status: 'active' }, ctx as never,
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* An option-level `default: true` gets NO physical column DEFAULT (#7246).
5+
*
6+
* The engine now honours the select idiom — `ObjectQL.applyFieldDefaults` falls
7+
* back to the option marked `default: true` when the field declares no
8+
* `defaultValue`. The DDL deliberately does NOT follow, and because that reads
9+
* as an oversight next to case 4 of `applyDeclaredColumnDefault` (an ordinary
10+
* literal IS emitted), it is pinned here rather than left to a comment.
11+
*
12+
* Why not emit, in short — the long form lives on
13+
* `SqlDriver.applyDeclaredColumnDefault`:
14+
*
15+
* - `defaultValue` beats the option flag, and that precedence lives in ONE
16+
* place, the engine. A column DEFAULT is a second resolver.
17+
* - On `multiple: true` the default is an ARRAY; there is no scalar DDL form,
18+
* so emitting needs a carve-out the author cannot see.
19+
* - This method runs for fresh and re-materialized columns only, never a
20+
* retrofit, so emitting would give new databases a DEFAULT that older ones
21+
* on identical metadata lack — and `detectDrift`'s only `default_mismatch`
22+
* producer is the #4560 runtime-token check, so nothing would report it.
23+
*
24+
* The #4560 discipline is NOT the reason: an option's `value` is a plain
25+
* literal, so it could legally be emitted. This is a design decision about
26+
* where a default is resolved, and the last test states the consequence that
27+
* makes it safe — every ObjectStack write path stores the value regardless,
28+
* because the engine, not the database, supplies it.
29+
*/
30+
31+
import { describe, it, expect, afterEach, vi } from 'vitest';
32+
import { SqlDriver } from '../src/index.js';
33+
34+
describe('SqlDriver — an option `default: true` never becomes a column DEFAULT (#7246)', () => {
35+
let knexInstance: any;
36+
37+
const makeDriver = () => {
38+
const d = new SqlDriver({
39+
client: 'better-sqlite3',
40+
connection: { filename: ':memory:' },
41+
useNullAsDefault: true,
42+
});
43+
knexInstance = (d as any).knex;
44+
(d as any).logger = { warn: vi.fn(), info: vi.fn(), error: vi.fn(), debug: vi.fn() };
45+
return d;
46+
};
47+
48+
/** The raw `CREATE TABLE` SQLite stored — the only unambiguous view of a DEFAULT. */
49+
const tableSql = async (table: string): Promise<string> => {
50+
const row = await knexInstance.raw(
51+
"SELECT sql FROM sqlite_master WHERE type = 'table' AND name = ?",
52+
[table],
53+
);
54+
return String(row?.[0]?.sql ?? row?.sql ?? '');
55+
};
56+
57+
afterEach(async () => {
58+
await knexInstance?.destroy();
59+
});
60+
61+
const optionZoo = [
62+
{
63+
name: 'option_zoo',
64+
fields: {
65+
title: { type: 'string' },
66+
// Option-default only — the shape 30 fields in the shipped corpus use.
67+
f_status: {
68+
type: 'select',
69+
options: [
70+
{ label: 'Draft', value: 'draft', default: true },
71+
{ label: 'Active', value: 'active' },
72+
],
73+
},
74+
// BOTH declared: the field-level literal is emitted (case 4), and it is
75+
// also the value the engine resolves — the two agree, which is the
76+
// property that matters when both sides can answer.
77+
f_stage: {
78+
type: 'select',
79+
defaultValue: 'approved',
80+
options: [
81+
{ label: 'Draft', value: 'draft', default: true },
82+
{ label: 'Approved', value: 'approved' },
83+
],
84+
},
85+
},
86+
},
87+
];
88+
89+
it('creates an option-defaulted column with NO database default', async () => {
90+
const driver = makeDriver();
91+
await driver.initObjects(optionZoo as any);
92+
93+
const info = await knexInstance('option_zoo').columnInfo();
94+
expect(info.f_status.defaultValue ?? null).toBeNull();
95+
96+
const sql = await tableSql('option_zoo');
97+
expect(sql).not.toContain("DEFAULT 'draft'");
98+
});
99+
100+
it('REGRESSION: a field-level `defaultValue` on the SAME field is still emitted', async () => {
101+
// The exclusion is scoped to the option flag. Losing case 4 here would be a
102+
// silent behaviour change for every literal default in the platform.
103+
const driver = makeDriver();
104+
await driver.initObjects(optionZoo as any);
105+
const info = await knexInstance('option_zoo').columnInfo();
106+
expect(String(info.f_stage.defaultValue)).toContain('approved');
107+
// ...and never the option the field-level key outranks.
108+
expect(String(info.f_stage.defaultValue)).not.toContain('draft');
109+
});
110+
111+
it('a driver-level insert that omits the field stores NULL — the engine, not the database, defaults it', async () => {
112+
// The consequence of the decision, stated rather than left implicit: this
113+
// is a RAW driver write, below the engine. Through `ObjectQL.insert` the
114+
// same omission stores 'draft' (pinned in
115+
// objectql/src/engine-select-option-default.test.ts), which is every
116+
// ObjectStack write path. Only a writer bypassing the engine sees this
117+
// NULL — and that writer is not reading `options` either.
118+
const driver = makeDriver();
119+
await driver.initObjects(optionZoo as any);
120+
await driver.create('option_zoo', { id: 'o1', title: 't' }, { bypassTenantAudit: true });
121+
const row = await knexInstance('option_zoo').where('id', 'o1').first();
122+
expect(row.f_status).toBeNull();
123+
});
124+
});

0 commit comments

Comments
 (0)