fix(plugin-security): explain reports a fail-closed RLS denial as denies, not narrows - #13960
fix(plugin-security): explain reports a fail-closed RLS denial as denies, not narrows#13960claude[bot] wants to merge 2 commits into
Conversation
…tinel
`explain` recognised only its own `__deny_all__` sentinel, so a fail-closed
RLS denial — plugin-security's `RLS_DENY_FILTER`, composed when a required
`current_user.*` variable resolves to nothing — was reported with layer verdict
`narrows` and `decision.allowed: true`. That is an affirmatively wrong answer
about a request guaranteed to return zero rows, handed to the operator asking
why a user sees nothing.
Deny recognition is now value-agnostic and routed through ONE named predicate
(`isDenyAll`), so both the record-grained call sites and the object-level `rls`
verdict recognise either sentinel. Two inline literal comparisons answering the
same question is how the two sites drifted apart; explain-engine.ts now holds
no bare occurrence of the sentinel outside its named constant.
Recognition is all that widens. The published payload is NOT rewritten: the
`__deny_all__` collapse stays keyed to `__deny_all__`, so a deployment that
receives `{ id: '__rls_deny__:…' }` in `readFilter` keeps receiving it. The two
sentinels are not merged. Both of those are deployment-facing decisions
recorded on the card as the maintainer's.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs
…adding a second `check:engine-double-contract` reported RETAINED: explain-engine.test.ts pinned 2 findOne doubles where its ledger records 1, because the new #13639 suite declared its own delegator-resolving `ql` beside the identical inline one the D10 partial-mask test already had. The gate's suggested remedy is `--write` so the new double is ratcheted too. Reusing the double the file already pins is cheaper: it leaves the ledger untouched. The inline literal is hoisted to a module-level `DELEGATOR_QL` and both tests share it — behaviour is byte-identical (same getSchema, same `assertEngineFindOnePredicate`-guarded findOne returning `{ id: 'u_boss' }`, same empty find). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs
📓 Docs Drift Check5 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 14 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 7f08f4783e9567bfd82b6316a82827dfa678d16f && git checkout 7f08f4783e9567bfd82b6316a82827dfa678d16f
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 167bcb2e1e33d714d5d334f2823da41e7a310223 a0389e53cde826ace3050e1ea57dd4673e70d7b3 && git checkout -B drift-repro 167bcb2e1e33d714d5d334f2823da41e7a310223 && git merge --no-ff a0389e53cde826ace3050e1ea57dd4673e70d7b3
node scripts/docs-audit/affected-docs.mjs --json 167bcb2e1e33d714d5d334f2823da41e7a310223 |
Closes #13639
Slice 2 of #13639 — the
explainhalf, option A, which the PM ruled on the card(comment 5482449910) after slice 1 (#13942, the logging half) measured that the issue
body's own "Suggested shape" would not fix the symptom it names.
The wrong answer this removes
When applicable RLS policies exist but none can be compiled against the current
execution context — a required
current_user.*variable resolving to nothing, e.g. acaller with no active organization — the compiler fails closed and composes
plugin-security's
RLS_DENY_FILTER({ id: '__rls_deny__:…' }), anidequalityagainst a UUID-shaped string no record can carry. Enforcement was always right: the
caller sees zero rows.
explainrecognised only its own__deny_all__sentinel, so for that whole class itreported:
rlslayer verdictnarrowsdeniesdecision.allowedtruefalsetenant_isolationrecord detailrlsrecord detaildecision.allowed: truefor a request that cannot return a row is not an impreciselabel — it is an affirmatively wrong answer, handed to the operator (or agent) asking
why a user sees nothing. That is the load-bearing line of this PR.
What changed
Deny recognition is value-agnostic and routed through one named predicate. Slice 1
measured why that matters:
isDenyAllhad exactly two call sites, both inside therecord-grained augmentation, while the object-level
rlsverdict compared the samestring literal a second time, inline — which is how the two sites drifted apart in
the first place.
explain-engine.tsnow holds no bare occurrence of the sentineloutside its named constant.
isDenyAll(filter)— "does this deny every row?" — recognises either sentinel, andevery deny-recognition site routes through it.
isDenyAllSentinel(filter)— the narrow question, kept separate because §9's payloadcollapse is keyed to that one value and must not follow the widened recognition.
isRlsDenySentinel(filter)— identity decided on the sentinel's own value, thesame rule
isTenantWallDenialstates (ADR-0123 D2) and for the same reason: producersspread the frozen constant, so a reference check answers
falsefor every real denial.Deliberately NOT done — recorded on the card as the maintainer's
__rls_deny__(option B). The publishedreadFilterkeeps reporting the predicate that was actually composed: a deployment receiving
{ id: '__rls_deny__:…' }today keeps receiving it. The documented__deny_all__collapse still fires for
__deny_all__alone. This boundary is pinned, notcommented — two tests assert it in both directions.
__deny_all__is named in the published schema and docs;__rls_deny__is pinned asa bound SQL parameter by two
service-analyticssuites and dispatched on by value byisTenantWallDenial.packages/spec/**path is touched, so the tier's clause-② path limb does not fire —dispatch-gates.mjsverbatim: "no path-derived mandate: the surface hits none of the3 declared glob(s)". This changes
explain's computation, not its schema.Also in this diff, named rather than quiet
check:engine-double-contractwent red on the first battery: the new suite needs adelegator-resolving
qlfor the composite-readFilterpins, and declaring its owngave the file 2 pinned
findOnedoubles where the ledger records 1. The gate'ssuggested remedy is
--write. Reusing the double the file already pins is cheaper,so the identical inline literal in the pre-existing D10 partial-mask test is hoisted to
a module-level
DELEGATOR_QLshared by both — byte-identical behaviour, ledgeruntouched (
findOne doubles: 291 in 247 test file(s), and the gate's own verdict lineis now
check-engine-double-contract: OK — 727 pinned, 134 in the DEBT ledger, 3 exempt).Verification — all at
a0389e53c, clean treepnpm --filter @objectstack/plugin-security test→Test Files 93 passed (93) / Tests 1750 passed (1750).typecheckexit 0. NOT-MEASURED trap cleared:tsc -p tsconfig.test.json --listFileslists both edited files, so the green typecheck reallycovers the new tests.
pnpm lint(eslint . --no-inline-config, whole repo) exit 0— no narrowing claimed, the repo scan actually ran.
Gates re-derived from the actual diff (
dispatch-gates.mjs --repo objectstack-ai/objectstack, no path argument — it reads the change set itself: 3 pathsvs merge base
00f79c928), both output sections read whole, and the derived family isidentical before and after the refactor. 38 commands, 37 exit 0, including the two
that name a build prerequisite (
check:i18n,check:dual-build-cjs-loads— closurebuilt exactly as
lint.ymldoes) and the ratchetcheck:type-check-debt --re-measure,whose own verdict line reads "29 ledger entr(ies) re-measured in 216.9s, 1531 raw tsc
error(s) total, none above its recorded number". Every exit code captured before any
pipe.
NOT MEASURED (neither green nor red):
check-test-completeness.mjsexit 3 — its owntext says nothing was measured because no
turbo run testlog was passed.Ablation — direction predicted first
Prediction: reverting the recognition restores the old answers, so exactly the pins
asserting the new ones redden — 6, and every control stays green. Repair committed
first so the restore leg had a real reference point. Mutation:
isDenyAllloses the|| isRlsDenySentinel(filter)disjunction.Mutation confirmed on disk before measuring, never by an editor's exit code — HEAD
blob
e9ea653f…equal to the on-disk pre-hash, post-hash717a0bbd…, removed-markercount 1→0, injected-marker count 0→1,
git diff --statone insertion one deletion. Thescript carried
trap restore EXIT INT TERMwith an absoluteREPO_ROOT, and restoreis
git checkout HEAD -- PATH, never the bare form (which restores from the pollutedindex). No rebuild leg is claimed or needed: the suite imports the subject relatively
(
./explain-engine), which cannot resolve through a package exports map todist/, sothe mutated source is what ran — and the reddening corroborates it, since a stale-dist
ablation cannot redden anything.
Result:
Tests 6 failed | 75 passed (81)— exactly the predicted 6, each failing forthe predicted reason:
expected 'narrows' to be 'denies';expected true to be false(×3 — the
allowedpins); and the two record-grained pins failing ondetailonly,expected 'Record\'s organization does not match…'andexpected 'The record does not satisfy the busin…'. That last pair independently reconfirms slice 1's measurement:the
outcome,matchesRecordandeffectassertions sit before the proseassertion in each test and passed under the mutation, so record-grained correctness
really does not move.
Restore proven by state, not by the trap having fired: on-disk blob back to
e9ea653f…equal to the HEAD blob,git diff HEADempty,git status --porcelainempty, marker counts back to 1/0.
Declared controls — green in BOTH directions, therefore controls and NOT ablation
evidence: the payload pin for
__rls_deny__(readFilteris the composed predicateunder both versions — which is the point: it is a regression pin against option B, not a
detector for this repair);
__deny_all__verdict/decision/detail unchanged; the__deny_all__composite collapse; the record-grained__deny_all__prose at both callsites; and every negative (a genuinely narrowing policy, a non-sentinel
idpredicate,no policy at all, and a record the layers genuinely admit).
Generated by Claude Code
Generated by Claude Code