Skip to content

fix(service-analytics): row-scope bridge tells absent from broken security service - #17125

Merged
os-trump merged 2 commits into
mainfrom
claude/issue-16918-row-scope-bridge-three-way
Sep 9, 2026
Merged

fix(service-analytics): row-scope bridge tells absent from broken security service#17125
os-trump merged 2 commits into
mainfrom
claude/issue-16918-row-scope-bridge-three-way

Conversation

@os-trump

@os-trump os-trump commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16918

What changed

packages/services/service-analytics/src/plugin.ts bridges to the security service twice. The object-level bridge (admitObjectReadcanReadObject, landed and contract-reviewed via #16860) resolves an explicit three-way: ABSENT admits, THROWING and METHOD-LESS deny at error. The row-scope bridge (getReadScopegetReadFilter) collapsed all three into one value:

const trySecurity = () => {
  try {
    const svc = ctx.getService<SecurityReadFilter>('security');
    return svc && typeof svc.getReadFilter === 'function' ? svc : undefined;
  } catch { return undefined; }
};
getReadScope = (object, context) => trySecurity()?.getReadFilter(object, context);

undefined is the value ISecurityService.getReadFilter reserves for one meaning only — "this caller has no row restriction on this object". Spending it on a provider that never answered is how an analytics query ends up running with no row-level policy at all. After #16860 one door of this file failed closed on a throwing resolver and its neighbour failed open, and the neighbour is the one carrying row-level policy.

This mirrors the landed shape, one function up:

  • ABSENT → unchanged, byte for byte. A single-tenant deployment shipping no plugin-security has no row-level policy anywhere, /data included; the init log already says so.
  • THROWING resolver / METHOD-LESS service → ctx.logger.error(...) naming the object and which state it was, then a throw. AnalyticsService.resolveReadScopes — fail-closed since ADR-0021 D-C — turns that into "deny the whole query rather than emit SQL with that object unscoped".

That is refusal option (1) from the dispatch brief: refuse the query, the outcome the object-level bridge already produces. It is neutral between the two candidate tenant walls — it answers "should we serve at all", never "what shape is the wall" — so it does not touch triage condition 5 (#16645). ⛔ Option (3) (a louder log over an undefined) is not what landed: a log is not a refusal.

Published-surface delta: none

Clause-② stays no, checked rather than assumed against all three triggers:

trigger verdict
new error code (a row in packages/spec/src/api/error-code-ledger.zod.ts) no — the refusal rides the existing fail-closed seam in resolveReadScopes, which throws its own bare error. Nothing new is declared.
exported symbol / key on a published payload or option type noSecurityReadFilterResolution is a local type inside init(), exactly like the object-level bridge's SecurityAdmissionResolution. AnalyticsServicePluginOptions is untouched.
documented envelope shape no — no envelope is produced or altered here.

git diff --stat is three files: the bridge, one new test file, one changeset.

Prove it can fail — three arms, predicted then measured

Predictions were written before running:

arm before (origin/main 50b6f17d4) after
1 · throwing getService('security') serves {cnt: 24} off an unscoped statement refuses; error-level report naming the throw
2 · registered service, no getReadFilter serves {cnt: 24} off an unscoped statement refuses; error-level report naming the missing method
3 · ⭐ absent service (control) serves {cnt: 24}, unscoped, no error log identical

Results are appended below as they arrive.

Escalation clause — arm 2's reachability

Answered in the report; see the "escalation" section appended below.

Fences

  • Only tightens. The ABSENT arm — the one a relaxation would have to touch — is unchanged and is pinned by the control test.
  • No behaviour change outside the row-scope bridge: the object-level bridge, driver-sql's auto-scope and plugin-security's Layer 0 are untouched.
  • No test skipped, disabled or quarantined. No force-push, rebase or amend.

验收备注

  • securityPresentAtInit keeps its exact truth table (kind === 'usable' ⟺ the old !!trySecurity()), so both init log branches — including the object-level bridge's, which reads the same flag — are unchanged. That the admission branch reads a flag computed by the row-scope bridge (and stays false whenever a host supplies its own getReadScope) is pre-existing and deliberately left alone here.
  • Out-of-scope observations recorded in the report, none filed.

🤖 Generated with Claude Code

https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37


Measurements

Ablation — the three arms, predicted above, then measured

The mutation is git checkout 50b6f17d4 -- packages/services/service-analytics/src/plugin.ts (this branch's only edit to that file is the bridge), restored under trap … EXIT INT TERM with git checkout HEAD -- <ABSPATH>.

The mutation reached disk (counted, not inferred from an editor's exit code):

== markers BEFORE mutation ==
new-refusal marker: 2        # 'row-level read scope could not be resolved'
old-collapse marker: 0       # 'trySecurity()?.getReadFilter(object, context)'
== markers AFTER mutation (proof it reached disk) ==
new-refusal marker: 0 (expect 0)
old-collapse marker: 1 (expect 1)
mutated blob: b5d20d49e0ff39d43ceb86b6bc6947eb3c6073f1

BEFORE — src/__tests__/read-scope-bridge-resolution.test.ts against the pre-fix bridge:

 ❯ src/__tests__/read-scope-bridge-resolution.test.ts (5 tests | 2 failed) 30ms
     × REFUSES when resolving the "security" service THROWS 17ms
     × REFUSES when the registered "security" service exposes no getReadFilter 4ms
 Tests  2 failed | 3 passed (5)

AssertionError: promise resolved "{ rows: [ { cnt: 24 } ], …(1) }" instead of rejecting

Both broken-provider arms served rows. ⭐ The absent-service control passed untouched, as did the positive arm (a usable service, whose predicate appears in the statement that ran) and the both-bridges-live arm.

AFTER: Test Files 1 passed (1) · Tests 5 passed (5).

Restoration, proven by state and not by an exit code:

$ git diff HEAD --stat      → (empty)
$ git status --short        → (empty)
worktree blob: 48a0ef4f44b2a81255d50d15ef8cb00bdc12d87c
HEAD blob:     48a0ef4f44b2a81255d50d15ef8cb00bdc12d87c

The two object-level doubles this change invalidated — reported, not quietly rewritten

The first full-package run came back Tests 2 failed | 2193 passed, both in admission-bridge-resolution.test.ts:

× asks canReadObject when the service has it, and serves an ADMITTED caller
× falls back to explain for a service that predates canReadObject — both verdicts

Their doubles model a working security service as { canReadObject } (and { explain }) with nothing else — which is now, correctly, a service that cannot answer the row half. getReadFilter is a REQUIRED member of ISecurityService and undefined is its documented "no row restriction on this object", so each double gained exactly that (rowScopeOpen). No assertion, no verdict and no object-level behaviour changed; the deny-path doubles are untouched because the object-level gate refuses before the row half is asked. ⛔ Nothing was skipped, disabled or quarantined.

Second run: Test Files 99 passed (99) · Tests 2195 passed (2195), and tsc --noEmit exit 0 — with --listFiles confirming the new test file is inside the typecheck program (the package's tsconfig.json includes src and excludes no test).

Gates

node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack over the real change set (4 paths, merge base 50b6f17d4) derived 59 commands. All 59 were invoked; the --ran reconciliation reports 59 derived famil(ies) accounted for — 59 run, 0 UNRUN.

56 measured green. 3 are NOT MEASURED, and are named rather than counted as passes — each exits 3 PREREQUISITE NOT MET because it reads a whole-workspace dist/, which is a 66-package build and therefore CI's run, not this seat's:

  • pnpm check:dual-build-cjs-loads
  • pnpm check:lean-entry-closure
  • pnpm check:type-check-debt

Repo-wide lint is the union, not a narrowing: pnpm lint (eslint . --no-inline-config) exits 0 with zero findings at cb3c68513.

Also NOT MEASURED, same cause: packages/qa/dogfood's analytics-rls / analytics-label-scope suites fail to resolve @objectstack/verify (no dist). Unaffected by construction — both fixtures boot a real SecurityPlugin, which registers getReadFilter — but declared here rather than claimed.

Escalation clause — arm 2 is NOT reachable in any configuration this repo ships or permits

The card escalates to priority:p1 if a registered security service without getReadFilter is reachable through a version pair the workspace allows or an out-of-repo provider the published ISecurityService admits. Measured on three independent legs:

  1. History. getReadFilter was introduced on the registered literal by 69da71b7d, the same commit that first registered the bare 'security' name at all — at its parent the plugin registers only security.permissions, security.rls, security.fieldMasker, … so getService('security') answered nothing there. Every plugin-security that has ever exposed this service has exposed getReadFilter; every earlier one is the ABSENT arm. Across all 146 commits touching security-plugin.ts since that commit, the count of commits whose registered literal lacks getReadFilter: is 0 (checked commit by commit, --follow agreeing with the path-as-is walk, so no rename hides a gap).
  2. Type. The literal is annotated const securityService: ISecurityService, and getReadFilter is a required member — kept compiler-exhaustive in both directions by REQUIRED_MEMBERS in packages/spec/src/contracts/security-service.test.ts. Dropping it fails plugin-security's own build.
  3. Registrants. plugin-security is the only in-repo registrant of the bare name 'security' (registerService('security' over packages/, apps/, examples/, non-test); the dogfood and verify fixtures pass real SecurityPlugin instances.

The card stays priority:p2. Two boundaries of that answer, stated rather than hedged:

  • The only way to reach arm 2 is an out-of-repo provider that omits a REQUIRED contract member — "not the contract it claims to be", the same class the landed object-level bridge names for explain. ⚠️ Worth a maintainer's eye: ISecurityService's own preamble tells implementations to "omit" a method they cannot honour and consumers to feature-detect, which reads as permission to do exactly that, while the type says the member is required. That tension is not this card's to settle and nothing here depends on it — the state is now refused either way.
  • Arm 1's reachability under default wiring is narrower than the card assumes, and in one respect wider: with both auto-bridges live the object-level bridge (fix(analytics): ask the object-level read grant before serving an inline dataset — one admission verdict on every driver #16860) refuses a throwing resolver first, so the row-scope collapse needs either a host supplying its own admitObjectRead (a documented option) or a fault window that opens between the two resolutions — the resolver is consulted twice per query, at two different instants, and the second call is the row scope. A boot-order fault clearing between them is precisely the shape the card describes.

Generated by Claude Code

…urity service

The plugin bridges to the `security` service twice. The object-level bridge
resolves an explicit three-way — ABSENT admits, THROWING and METHOD-LESS deny
at `error`. The row-scope bridge collapsed all three into `undefined`, which is
the value `ISecurityService.getReadFilter` reserves for "this caller has no row
restriction", so a wired-but-broken security service made analytics queries run
with no row-level policy at all, indistinguishable from a deployment that ships
no security plugin.

Resolve the same three-way here. ABSENT keeps today's behaviour byte for byte
(a legitimate single-tenant configuration, reported at init). THROWING and
METHOD-LESS report at `error` and throw, which the fail-closed
`AnalyticsService.resolveReadScopes` seam already turns into a refusal of the
whole query rather than SQL with the object unscoped — the same outcome the
object-level bridge produces, and neutral between the two candidate tenant
walls. No new error code, no exported symbol, no payload key.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
@github-actions github-actions Bot added the size/m label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-analytics, touching 1 documentable anchor(s).

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

  • content/docs/plugins/packages.mdx (via AnalyticsServicePlugin (symbol, a top-level class))
What this run could not see
  • 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 — 9 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 50b6f17d4140bcb3d161137a58aeaf7f49cc22bapackageMentionDocs.

Which tree this was computed on

This run read content/docs from dfc5f62dc8d5fc1360004be55d754edab11944cd — the merge of head cb3c68513fabcadf96d9e4afd65710c0ba83bd22 into base 50b6f17d4140bcb3d161137a58aeaf7f49cc22ba, 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 dfc5f62dc8d5fc1360004be55d754edab11944cd && git checkout dfc5f62dc8d5fc1360004be55d754edab11944cd
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 50b6f17d4140bcb3d161137a58aeaf7f49cc22ba cb3c68513fabcadf96d9e4afd65710c0ba83bd22 && git checkout -B drift-repro 50b6f17d4140bcb3d161137a58aeaf7f49cc22ba && git merge --no-ff cb3c68513fabcadf96d9e4afd65710c0ba83bd22

node scripts/docs-audit/affected-docs.mjs --json 50b6f17d4140bcb3d161137a58aeaf7f49cc22ba

⚠️ 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 50b6f17d4140bcb3d161137a58aeaf7f49cc22ba → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 9, 2026
… half

Two doubles in `admission-bridge-resolution.test.ts` model a WORKING security
service with `canReadObject` (or the `explain` fallback) and nothing else. With
the row-scope bridge now refusing a registered service that exposes no
`getReadFilter`, those doubles describe a state the platform refuses, and the
two object-level ADMIT assertions could no longer be reached.

`getReadFilter` is a required member of `ISecurityService`, and `undefined` is
its documented "no row restriction on this object", so the doubles gain exactly
that and stay minimal and conforming. No assertion, no verdict and no
object-level behaviour changes; the deny-path doubles are untouched because the
object-level gate refuses before the row half is asked.

Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
Co-authored-by: Claude <noreply@anthropic.com>
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

2 participants