fix(security)!: an RLS predicate naming an undeclared column denies in every position - #17115
Conversation
…n every position A predicate naming a column the object does not declare could not narrow, and in a negation-carrying position it did not deny either -- it WIDENED the policy to every row inside the tenant wall (read face) and PERMITTED the write the policy was authored to refuse (write face). Read face: `extractTargetField` is a LEADING `==`/`=`/`in` shape match, so `nope != "x"`, `!(nope == 1)`, `!(nope in [...])` and any arm after the first returned `null`, `if (!targetField) return true` KEPT the policy, `dropped` never incremented and the deny sentinel never armed. Write face: `computeWriteCheckFilter` compiled `check` clauses with no field-existence net at all. The repair is one seam, not two: `RLSCompiler.compileFilter` -- which both the read layer and the ADR-0058 D4 write gate already pass through -- now takes the object's declared-column set and judges every column the policy names on the COMPILED FilterCondition tree. That is positional-agnostic by construction: the pushdown compiler lowers `!` to `$not`, `||` to `$or` and `&&` to `$and`, so a column lands as a plain object key whatever position it was authored in, and there is no spelling of negation left for a shape match to miss. The matcher's include-direction ruling (`noValueSatisfiesNegation`, driver-memory / driver-mongodb) is deliberately UNTOUCHED -- it is correct for an ordinary user query. The defect was that the policy compiler lowered an undeclared column into a filter at all; the matcher now never sees a phantom. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
📓 Docs Drift CheckThis PR changes 2 package(s): 7 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 18 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin e9e4a1ae9ae7312857d1b3bb68149b40cf837bf7 && git checkout e9e4a1ae9ae7312857d1b3bb68149b40cf837bf7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 50b6f17d4140bcb3d161137a58aeaf7f49cc22ba 05f5c96dfeb87853038f89be769734ab46ac7be7 && git checkout -B drift-repro 50b6f17d4140bcb3d161137a58aeaf7f49cc22ba && git merge --no-ff 05f5c96dfeb87853038f89be769734ab46ac7be7
node scripts/docs-audit/affected-docs.mjs --json 50b6f17d4140bcb3d161137a58aeaf7f49cc22ba
|
… and correct the linter's consequence prose The regression suite runs the four negation shapes through the compiler seam and end-to-end through a real ObjectQL + SecurityPlugin on both SQL drivers, on the read face and the write face, each against the two controls that make a reading a reading: a real column must still narrow, and the same phantom column in a positive position must still refuse. Ablated against the pre-fix source: 36 of 50 cells fail, and the 14 that hold are exactly the controls. It also pins the include-direction ruling as UNCHANGED -- the raw matcher still admits 3 of 3 rows for the same filter -- so a later reader can see that what moved is that the policy compiler stopped producing the filter, not what the matcher does with one. The linter's detection is untouched. Its consequence text was stale in one half and misattributed in the other: it described the field miss as having two directions decided by position, and it credited the write leg's fail-closed to a safety net `computeWriteCheckFilter` never had. It now states one direction for both clauses and records the older runtime's fail-open write behaviour explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
Contract review at
|
…s-negated-phantom-column-fail-closed
Contract review at
|
|
Landing provenance — director seat takes the release action under the maintainer's 13:4xZ instruction 「把当前的契约复审全部处理完」 (
Generated by Claude Code |
Adoption record —
|
| check | reading |
|---|---|
| head unmoved | 05f5c96d… — the head the verdict pins; commit status re-read now: success |
| CI | verdict read 39 check-runs on this exact head: 34 success, 5 skipped, 0 red, 0 in progress — Lint & Repo Gates, TypeScript Type Check, all four Type Check legs, Test Core 1-6, Temporal Conformance, Dogfood 1-3, Governed Surface Queue Guard, both claim guards, Part-of guard |
| Clause-② carriers | check-clause2-carriers.mjs --pair 17115 → exit 0, both carriers agree in the fixed spelling |
| governed surface | none — 6 changed files under .changeset/, packages/lint/src/, packages/plugins/plugin-security/src/; ⛔ no content/docs/releases/ edit (the v17 restructure in the merged history is upstream, not a PR edit) |
| mergeability | merge-tree --write-tree origin/main 05f5c96d conflict-free; origin/main has moved further with no file intersection |
Carriers off, with provenance
needs:contract-review removed from both carriers — PR #17115 and card #17042 — on the authority of verdict 5602050708. ⛔ Removed for that reason and no other.
⚠️ F6 is addressed to this seat, and it is right
"The prior verdict named F2 as a follow-up-card candidate; of the 13 issues created since 10:45Z none is it (the seat filed #17128 and #17129 from the dev's
out_of_scope_findings; neither is F2). Owed by thedomain:servicesseat at adoption."
Correct, and the omission is mine: I filed the delivery's own out-of-scope findings and did not carry the reviewer's. Filed now — see below. ⛔ Recorded rather than quietly closed, because "a follow-up card named in a verdict and never filed" is the same silence this lane spends its days on.
Findings carried forward
- F1 — the pin set.
PHANTOM_NEGATIONScovers only the card's four shapes; nothing on the compiler face pins a trailing&&arm or a nested$not-under-$or. Denial there holds by construction of the walker, and the pin is what stops a future walker edit regressing it.⚠️ The reviewer calls it "the one worth a rider before landing"; this seat lands without the rider and files it instead, and states the reason rather than leaving it implicit: the PR closes a live fail-open on both the read and the write face, while F1 protects against a future regression and changes nothing about today's correctness. An hour of exposure is worth more than an hour of pin. ⛔ The card is filed in the same breath so it cannot be lost. - F2 — the schema-lookup miss. When
getObjectFieldNamesanswersnullno guard is passed and a negated phantom still widens. Residual by design (pass-1's stated contract), bounded to boot/unregistered objects, declared openly in the PR body and changeset. Filed per F6. - F3 / F4 / F5 — the linter's "one WARN line" overstatement (a drop is silent when a sibling grants),
RlsFieldGuardnot barrel-exported although it appears in the published signature, and the optionalrls.mdx:176clause. Recorded here; the next seat touching these files should take them.
Release
Ready → auto-merge armed.
Generated by Claude Code
Fixes #17042
needs:contract-reviewand is expected to go green and then park awaiting aCONTRACT_REVIEW_TIERverdict. The body below is written so a reviewer does not have to re-derive anything.The defect
An RLS predicate naming a column the object does not declare, in a negation-carrying position, did not narrow and did not deny — it widened the policy to every row inside the tenant wall (read face) and permitted the write the policy was authored to refuse (write face).
⛔ This is NOT a cross-tenant leak. Tenancy is a separate layer and it holds. What is defeated is the narrowing the policy author wrote inside the wall — an owner-only or private-record policy silently becoming "every row".
Two independent code sites, both on
origin/main:extractTargetField(security-plugin.ts) is a leading-only==/=/inshape match, sonope != "x",!(nope == 1),!(nope in ['a'])and any arm after the first returnednull;if (!targetField) return truekept the policy,dropped++never fired, and the deny sentinel (layer1 == null && dropped > 0) never armed.computeWriteCheckFiltercompiledcheckclauses with no field-existence net at all, andsecurity-plugin.ts:109/:2896evaluate that filter against the post-image (ADR-0058 D4 step 3.6).Deliverable 1 — the previously unmeasured middle column, now closed
Every cell below was run on this branch's parent (
91f65c4ea, unfixed) with the same two controls the card used: a real column must still narrow, and the same phantom column in a positive position must still refuse. ⛔ A refusal with no discriminating control is not a reading — the first version of this probe "refused" every cell because the driver failed to boot, and the controls are what caught it.Read face
matchesFilterCondition)driver-sqldriver-sqlite-wasmnope != "x"{"nope":{"$ne":"x"}}→ 3 / 3INVALID_FILTER/ 400INVALID_FILTER/ 400!(nope == 1){"$not":{"nope":1}}→ 3 / 3INVALID_FILTER/ 400INVALID_FILTER/ 400!(nope in ["a"]){"$not":{"nope":{"$in":["a"]}}}→ 3 / 3INVALID_FILTER/ 400INVALID_FILTER/ 400is_private == false || nope != "x"{"$or":[…]}→ 3 / 3INVALID_FILTER/ 400INVALID_FILTER/ 400is_private == falsenope == false(positive phantom)Write face — ⭐ SOURCE-ESTABLISHED before, now MEASURED
Single-row insert through a real⚠️ The payload must be a single object, never an array — the 3.6 gate is guarded by
ObjectQL+ realSecurityPlugin, policyusing: 'is_private == false'with the shape under test ascheck.!Array.isArray(opCtx.data), so a bulk payload skips it entirely and every cell reads "permitted" for the wrong reason.checkpredicateis_private=falseis_private=truenope != "x"!(nope == 1)!(nope in ["a"])is_private == false || nope != "x"is_private == falsePERMISSION_DENIED/ 403nope == false(positive phantom)PERMISSION_DENIED/ 403PERMISSION_DENIED/ 403Identical on both drivers, which is the expected shape: the
checkis evaluated in-process against the post-image, so the write face is driver-independent. The real-column control discriminating in both directions is what makes the fourPERMITTEDrows a reading.driver-sql — ⭐ now MEASURED, and it refines the card
The card recorded driver-sql as NOT MEASURED, "expected to fail closed by raising
no such column(sql-driver.ts:703)". Measured:no such column— it is driver-sql's ownINVALID_FILTER/ 400 envelope (unresolvableFilterColumnError), whose wording is the ingress door's verbatim.driver-sqlite-wasmanswers identically.checknever reaches SQL.⇒ The read face is driver-dependent (memory/mongodb widen; SQL raises). The write face is not: it is fail-open everywhere. That makes the write face both the worse one and the one no driver choice mitigates.
Deliverable 2 — both faces RED, then green
Prediction was written first (negated shapes widen/permit; real column narrows; positive phantom refuses) and matched the readings above. The same probe, unchanged, on the fixed tree:
driver-sqlsqlite-wasmPERMISSION_DENIED/ 403⭐ The real-column control is the one that catches an over-fix: a change that made every policy deny would pass a naive red→green and break every install. It still narrows to 1 of 3, and it still admits the write it should.
The seam I chose, and the alternative I rejected
Chosen: a positional-agnostic field-existence check inside
RLSCompiler.compileFilter, judged on the COMPILEDFilterConditiontree.compileFilterhas exactly two production callers — the read layer (using) and the ADR-0058 D4 write gate (check) — so one seam closes both faces and they can never again disagree about what an undeclared column means. A policy naming an undeclared column joins the existingdeniedBycollection under a newunknown-fieldreason, so it reuses the fail-closedRLS_DENY_FILTERsentinel and the existingwarnFailClosedDenialobservability rather than growing a second parallel mechanism.Rejected: widening the⚠️ A shape match that must enumerate every spelling of negation is the same "recognises only what it was told about" defect one level over — the next spelling is the next hole. The compiled tree has no spellings left:
extractTargetFieldregex.cel-to-filter.tslowers!to$not,||to$orand&&to$and, and every column lands as a plain object key whatever position it was authored in. The guard therefore stays correct when the pushdown compiler learns a new source form, because a new form still has to lower into this same shape. Widening the regex would also have broken the ADR-0095 delta c carve-out, which depends onextractTargetFieldrecognising only the leading shape.⛔
noValueSatisfiesNegationis untouched.memory-matcher.ts's$ne/$nin/$notContainsruling (#13166, shared withdriver-mongodb) is correct for an ordinary user query, and re-semanticing every filter in the repo to fix one caller is the blast radius this card must not take. The defect was that the policy compiler lowered an undeclared column into a filter at all; after this change the matcher never sees a phantom. The unguarded matcher reading in the table above is identical before and after, which is the evidence.Where the walker deliberately differs from the ingress collectors
Two collectors already answer "which columns does this filter name" —
collectFilterFieldKeys(metadata-protocol) andcollectFilterFieldNames(objectql). This one keeps their combinator rule verbatim and inverts their treatment of an unrecognised$key: they skip it without descending (right for a gate that must not invent 400s on caller input), this one refuses (null⇒ deny). The input here is not caller input — it is this compiler's own output, which emits$and/$or/$notand nothing else at node level, so an unmodelled combinator means the tree grew a shape the guard has not been taught, and leaving the columns beneath it unexamined is precisely the fail-open being closed. The depth backstop refuses on overrun for the same reason.Scope
tenancyDisabled && targetField === 'organization_id'→ skip, not deny) is deliberately not replicated into the compiler pass: replicating it would turn a case that denies today into one that applies no restriction at all, which is a relaxation. It stays at the call site, on the leading shape it was written for, unchanged.getObjectFieldNames→null) passes no guard and behaves exactly as before. A boot-time schema miss must not manufacture denials.Still NOT MEASURED
driver-mongodb, either face. It shares thenoValueSatisfiesNegationruling, so the read face is inferred, not measured. The fix is upstream of every driver, so no driver-specific behaviour is relied on — but the pre-fix reading there remains an inference.sys_permission_setcorpus was surveyed for policies that name an undeclared column today. Such a policy changes behaviour with this PR (it now denies); the linter that detects the authoring mistake is already landed (lint: an RLS predicate naming a non-existent field, or an un-pre-resolvedcurrent_user.*variable, is reported by nothing — both fail CLOSED at runtime #16119 / PR feat(lint): rls-predicate-unknown-field / rls-predicate-unknown-user-variable — the reference half of the RLS predicate gate #17036).Verification — all figures from
05f5c96df(the final commit,origin/mainmerged in)Ablation — the suite can actually fail. Both fix files reverted to the pre-fix base under a shell
trap … EXIT INT TERM; mutation proved on disk before the run (judgeCompiledFieldscount 0 inrls-compiler.ts,17042count 0 insecurity-plugin.ts, and the blob hash differing fromHEAD's), never by an exit code.HEADblob YES for both files, andgit diff HEADempty⭐ The 14 cells that held on the ablated tree are exactly the controls — the real column narrowing, the positive phantom, the untouched matcher ruling, and the no-guard identity. Controls that flip are not controls.
Reverse type verification — proving the new guard type is compiled, not cached: a probe passing
{ declared: ['a','b'] }(an array) whereReadonlySet<string>is required turnstscred withTS2739: Type 'string[]' is missing the following properties from type 'ReadonlySet<string>': has, size. Probe removed,git statusclean.Suites
@objectstack/plugin-securitypnpm test@objectstack/lintpnpm testpnpm typecheckpnpm --filter '...@objectstack/plugin-security' typecheckpackages/restred withTS2307: Cannot find module '@objectstack/service-package'. That was an unbuilt sibling, not this change:service-packagehad nodist/in this worktree and is not in the diff. Re-run on a fully built closure it is clean. Recorded because an unbuilt dependency reads exactly like a broken import.Gates —
node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack, derived from the merged head (no stale-tree warning), then reconciled with--ran:Two returned exit 3 —
PREREQUISITE NOT MET, which is NOT MEASURED and neither a pass nor a finding — and both were then measured rather than reported away:check:dual-build-cjs-loads— four packages outside this diff's build closure had nodist/. Built them; re-run green (104 require entry points across 67 packages).check:type-check-debt— a V8 OOM at a 4 GB heap under shared-box contention. Re-run at 8 GB; green, 5 ledger entries re-measured, none above its recorded number.Lint — the whole-repo scan was run, not narrowed:
eslint . --no-inline-config --format jsonover the population eslint itself resolved — 6420 files — 0 errors, 0 warnings. (The narrowing argument is therefore moot, but for the record the config states its own invariance verbatim ateslint.config.mjs:328: "this repo runs oneeslint.config.mjs, which never enables type-aware linting (noparserOptions.project, no typed@typescript-eslintrules) for ANY file, test or not.") The five linted files of this diff: 0 errors, 0 warnings.Control characters —
grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]'over every file in the diff: no hits.In-flight work on
security-plugin.ts— all 18 open PRs were enumerated and their file lists scanned forplugin-security,rls-compiler,memory-matcherandvalidate-rls-predicate: no open PR touches any of them. #16861 is in the same package but inbootstrap-platform-admin.ts, a different file, and is not among the open PRs' changed files either.security-plugin.tscarries noSINGLE_CLAIM_PATHSfence.Changeset —⚠️
minoron@objectstack/plugin-security,patchon@objectstack/lint, with a**BREAKING**banner.minor-with-a-breaking-banner is not a hedge, it is this repo's own settled convention for a security narrowing during the launch window — the same grading.changeset/insert-check-post-image.mdused for the insert-sidechecknarrowing and.changeset/memory-driver-tenant-scope-refusal.mdfor the driver-memory refusal, andcheck:changeset-no-majorrefusesmajoroutright.check-adr-0087-registrationrecognises the disposition:[BREAKING+bang] not-required (no-migration-prescription).🤖 Generated with Claude Code
https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
Generated by Claude Code