Skip to content

fix(plugin-security): explain reports a fail-closed RLS denial as denies, not narrows - #13960

Draft
claude[bot] wants to merge 2 commits into
mainfrom
claude/issue-13639-explain-deny-recognition
Draft

fix(plugin-security): explain reports a fail-closed RLS denial as denies, not narrows#13960
claude[bot] wants to merge 2 commits into
mainfrom
claude/issue-13639-explain-deny-recognition

Conversation

@claude

@claude claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Closes #13639

Slice 2 of #13639 — the explain half, 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. a
caller with no active organization — the compiler fails closed and composes
plugin-security's RLS_DENY_FILTER ({ id: '__rls_deny__:…' }), an id equality
against a UUID-shaped string no record can carry. Enforcement was always right: the
caller sees zero rows.

explain recognised only its own __deny_all__ sentinel, so for that whole class it
reported:

before after
rls layer verdict narrows denies
decision.allowed true false
tenant_isolation record detail "Record's organization does not match…" "No active organization on the context — the tenant wall denies all rows (fail closed)."
rls record detail "The record does not satisfy the business row-level predicate." "Business RLS composes to DENY ALL for this principal."

decision.allowed: true for a request that cannot return a row is not an imprecise
label — 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: isDenyAll had exactly two call sites, both inside the
record-grained augmentation, while the object-level rls verdict compared the same
string literal a second time, inline — which is how the two sites drifted apart in
the first place. explain-engine.ts now holds no bare occurrence of the sentinel
outside its named constant.

  • isDenyAll(filter) — "does this deny every row?" — recognises either sentinel, and
    every deny-recognition site routes through it.
  • isDenyAllSentinel(filter) — the narrow question, kept separate because §9's payload
    collapse is keyed to that one value and must not follow the widened recognition.
  • isRlsDenySentinel(filter) — identity decided on the sentinel's own value, the
    same rule isTenantWallDenial states (ADR-0123 D2) and for the same reason: producers
    spread the frozen constant, so a reference check answers false for every real denial.

Deliberately NOT done — recorded on the card as the maintainer's

  • No payload replacement for __rls_deny__ (option B). The published readFilter
    keeps 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, not
    commented
    — two tests assert it in both directions.
  • The two sentinels are not merged (option C). That is a spec change:
    __deny_all__ is named in the published schema and docs; __rls_deny__ is pinned as
    a bound SQL parameter by two service-analytics suites and dispatched on by value by
    isTenantWallDenial.
  • No packages/spec/** path is touched, so the tier's clause-② path limb does not fire —
    dispatch-gates.mjs verbatim: "no path-derived mandate: the surface hits none of the
    3 declared glob(s)"
    . This changes explain's computation, not its schema.

Also in this diff, named rather than quiet

check:engine-double-contract went red on the first battery: the new suite needs a
delegator-resolving ql for the composite-readFilter pins, and declaring its own
gave the file 2 pinned findOne doubles where the ledger records 1. The gate's
suggested 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_QL shared by both — byte-identical behaviour, ledger
untouched
(findOne doubles: 291 in 247 test file(s), and the gate's own verdict line
is now check-engine-double-contract: OK — 727 pinned, 134 in the DEBT ledger, 3 exempt).

Verification — all at a0389e53c, clean tree

pnpm --filter @objectstack/plugin-security testTest Files 93 passed (93) / Tests 1750 passed (1750). typecheck exit 0. NOT-MEASURED trap cleared: tsc -p tsconfig.test.json --listFiles lists both edited files, so the green typecheck really
covers 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 paths
vs merge base 00f79c928), both output sections read whole, and the derived family is
identical 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 — closure
built exactly as lint.yml does) and the ratchet check: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.mjs exit 3 — its own
text says nothing was measured because no turbo run test log 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: isDenyAll loses 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-hash 717a0bbd…, removed-marker
count 1→0, injected-marker count 0→1, git diff --stat one insertion one deletion. The
script carried trap restore EXIT INT TERM with an absolute REPO_ROOT, and restore
is git checkout HEAD -- PATH, never the bare form (which restores from the polluted
index). No rebuild leg is claimed or needed: the suite imports the subject relatively
(./explain-engine), which cannot resolve through a package exports map to dist/, so
the 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 for
the predicted reason: expected 'narrows' to be 'denies'; expected true to be false
(×3 — the allowed pins); and the two record-grained pins failing on detail only,
expected 'Record\'s organization does not match…' and expected 'The record does not satisfy the busin…'. That last pair independently reconfirms slice 1's measurement:
the outcome, matchesRecord and effect assertions sit before the prose
assertion 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 HEAD empty, git status --porcelain
empty, marker counts back to 1/0.

Declared controls — green in BOTH directions, therefore controls and NOT ablation
evidence:
the payload pin for __rls_deny__ (readFilter is the composed predicate
under 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 call
sites; and every negative (a genuinely narrowing policy, a non-sentinel id predicate,
no policy at all, and a record the layers genuinely admit).

Generated by Claude Code


Generated by Claude Code

os-steve and others added 2 commits August 31, 2026 18:23
…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
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

5 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot 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
  • 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 — 14 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 167bcb2e1e33d714d5d334f2823da41e7a310223packageMentionDocs.

Which tree this was computed on

This run read content/docs from 7f08f4783e9567bfd82b6316a82827dfa678d16f — the merge of head a0389e53cde826ace3050e1ea57dd4673e70d7b3 into base 167bcb2e1e33d714d5d334f2823da41e7a310223, 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 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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

1 participant