fix(plugin-sharing): guard the record-share $in against a nullish record_id before String() coerces it

[os-steve]

Fixes#13551

What was wrong

buildReadFilter and the bulk-write half of buildWriteFilter each turned the caller's sys_record_share rows into the members of a security predicate, { id: { $in: [...] } }, with the same expression:

constgrantedIds: string[]=Array.isArray(grants)
? grants.map((g: any)=>String(g.record_id)).filter(Boolean)
: [];

.filter(Boolean) reads as "drop rows whose record_id is nullish". It cannot: String(null) is 'null', String(undefined) is 'undefined', and both are truthy. The only value the filter could drop was the empty string, so the guard was dead for exactly the case its spelling advertised, and a row with a nullish record_id put the literal string 'null' into the emitted $in.

Direction: a dead guard, not an open bypass

The card's direction analysis holds, and this PR is not a bypass fix. The emitted member is a bogus id matching no row on any backend, and both sites are positive polarity — an OR-ed branch beside the owner match, never negated — so a corrupt row lost its grant rather than widening anyone's scope. It also took an already-corrupt row to reach at all. What was broken is the guard's honesty: a reader, or an audit asking which security paths already handle nullish ids, would have counted these two sites as covered when they provably were not.

The repair

Both sites now call one module-private grantedRecordIds() that tests the raw column value first and coerces after. That order is the whole content of the change. Factoring it into a single helper is deliberate: the expression stood in two places, and repairing one would have left the other advertising a guarantee it does not keep.

The shape matches the guards audited alongside these rather than inventing a third — in this package, sharing-rule-service.ts:915 (if (rid != null && rid !== '') granted.add(String(rid))) and primary-bu-projection.ts:74; across the fence and read-only here, core/src/security/resolve-authz-context.ts, plugin-security's controlled-by-parent masterIds, and objectql's master-detail parent resolution. All of them share one invariant: test before String().

The non-null path is unchanged. Every non-nullish value still stringifies exactly as before — a driver-numeric primary key still becomes its decimal string — and the empty string, the one value the old spelling really did drop, is still dropped by the trailing !== ''. The only behavioural difference is that rows with a nullish record_id contribute no member; when they are the only grants, the filter collapses to the plain owner match instead of OR-ing in a branch that matched nothing.

The map now reads g?.record_id rather than g.record_id, matching the sibling guards: a null element in the row array contributes nothing instead of throwing.

The escalation gate triage asked the implementer to answer first

Triage asked whether any object's record id can equal the string null, which would turn this from a dead guard into a real over-grant. Measured: it is not excluded by construction. Drivers mint an id only when the caller supplies none — sql-driver.ts writes toInsert.id = nanoid(...) only in the toInsert.id === undefined arm, and mongodb-driver.ts guards the same way — so a caller-supplied or imported id is stored verbatim, and no format validation stands between it and the column. Federated objects carry the remote primary key verbatim as well.

Two things that does not change. It still takes an already-corrupt share row and such a record to coincide, so nothing here demonstrates a live over-grant; and this PR closes the path either way, because the emitted set can no longer contain any member derived from a nullish record_id. I did not demonstrate that either row exists. Priority and labels are left untouched — that call is triage's.

The PM's mechanism assumptions, measured

1. Are the two sites genuinely identical? Yes, and the helper is right for both. The expression was byte-identical at both. Upstream they differ only in the query — the read site fetches every access level and also selects access_level, the write site restricts to WRITE_ACCESS_LEVELS and selects only record_id — but both read the same column of the same object, both feed a positive-polarity $or branch, and both collapse the empty case to the owner match. Same nullability, same polarity.
2. Is dropping the row right, rather than stamping a sentinel? Dropping is right, and the call sites had already decided it. Both read if (grantedIds.length === 0) return ownerMatch, so "no grants" and "grants that resolved to nothing" already produce the identical return value at both sites. No consumer can distinguish them, and a sentinel would have to invent a distinction the call sites deliberately do not make.
3. Does any pin assert the current behaviour? No. Nothing in this package's tests names a nullish record_id or the string 'null', and a repo-wide scan for a $in carrying 'null' returned only unrelated table-driven test labels. No pin had to be edited to keep this green.

Verification

Run at 374de16.

• pnpm --filter @objectstack/plugin-sharing test — 30 files, 683 tests passed; pnpm --filter @objectstack/plugin-sharing typecheck clean.
• Five new pins in sharing-service.test.ts, at both construction sites: a nullish record_id contributes no member while the real grant survives; when every grant is nullish the share branch disappears entirely rather than OR-ing in a branch that matches nothing; and the over-denial control — an ordinary grant set still produces exactly its ids, on both filters. A fifth pins the unchanged non-null path: a driver-numeric id still stringifies, an empty string is still dropped. Rows are seeded straight into the fake table because grant() refuses a nullish recordId at the front door, so writing the row directly is the only way to stand up the already-corrupt state the guard exists for.
• Ablation. Restoring the dead guard inside the helper turns 3 of the 5 new pins red and leaves 116 passing: AssertionError: expected [ 'a1', 'null', 'undefined' ] to deeply equal [ 'a1' ]. The mutation was confirmed on disk before measuring (the injected line greps to 1, the removed guard to 0, and the blob hash differs from the HEAD blob), and the restore leg was confirmed by the file's blob hash returning to the HEAD blob exactly. No rebuild leg was needed and none is claimed: the test imports the subject by relative path inside the package, so vitest resolves it from source with no exports or dist hop.
• The two pins that stay green under the ablation are the evidence for the byte-identical claim: the old and new implementations are indistinguishable on every non-nullish input.

Gate families were re-derived from the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, which is also what caught the docs page below — the first derivation, before that page was in the diff, named 20 fewer families. All derived families were run except four, each declared: check:i18n and check:skill-examples both refused with an explicit PREREQUISITE NOT MET (an unbuilt workspace CLI and an unbuilt client-react/dist) and state that nothing was checked, so they are NOT MEASURED here rather than green or red; check:type-check-debt --re-measure and check:dual-build-cjs-loads need the built workspace closure. CI builds the workspace and runs all four. check:test-completeness and check:pm/half-states exited 3, which both scripts define as NOT MEASURED — they need a saved turbo test log and a GitHub token respectively.

Surface beyond the dispatched file list, declared

content/docs/permissions/system-context.mdx — 8 line anchors, all in the plugin-sharing/src/sharing-service.ts rows, shifted by +29.

This is collateral from this PR, not a drive-by. The helper adds a net 29 lines near the top of the file, which moves every elevation-read site below it, and check:system-context-census went red with 16 problems. The shift is provable rather than judged: every reported site sits exactly 29 lines below the anchor that used to name it (654/625, 920/891, 1007/978, 1208/1179, 1286/1257, 1338/1309, 1597/1568), and the eighth anchor — the "guard at" citation in row 34 — lands on the NON_READ_ANCHORS needle's new line, 1311.

The gate's own --fix could not do this repair on my base, and the reason is already known and already fixed: it classified non-read anchors by comparing them to the needle's currently-resolved line, so the stale :1282 citation counted as a read anchor, the per-file tally read 8 anchors against 7 census sites, and a pure shift was rejected as a population change (NOT fixable: ... the POPULATION changed, this is not a shift). That is #13490 exactly, which PR #13575 repaired; it merged into main as 9c120f0 after this branch was cut — so nothing is filed here, and no gate was weakened. The hand-written shift is the same end state that --fix now produces; content/docs/permissions/system-context.mdx has not moved on main since this base, so it conflicts with nothing.

check:system-context-census is green after the shift: 109 elevation read sites in 20 packages across 45 files, all anchored; 145 anchors resolve, 27 declared non-read.

No other file in plugin-sharing was touched, per the fence around the in-flight batch-6 slice.

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-sharing, touching 3 documentable anchor(s).

3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

• content/docs/kernel/index.mdx(via buildReadFilter (symbol))
• content/docs/kernel/runtime-services/sharing-service.mdx(via buildReadFilter (symbol))
• content/docs/permissions/permissions-matrix.mdx(via buildReadFilter (symbol), buildWriteFilter (symbol))

What this run could not see

• the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
• a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 2006f7a49731d11be213da37167c0f41a3abd5df — the merge of head 374de16c863049f7ef841340a845d478b747a7d7 into base 9c120f0308d5aed3c26f53c406054a95a631d156, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 2006f7a49731d11be213da37167c0f41a3abd5df && git checkout 2006f7a49731d11be213da37167c0f41a3abd5df
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9c120f0308d5aed3c26f53c406054a95a631d156 374de16c863049f7ef841340a845d478b747a7d7 && git checkout -B drift-repro 9c120f0308d5aed3c26f53c406054a95a631d156 && git merge --no-ff 374de16c863049f7ef841340a845d478b747a7d7
node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9c120f0308d5aed3c26f53c406054a95a631d156 → pass the list as
args.docs, on the commit named under Which tree this was computed on.