You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit 94924e0
Browse filesBrowse the repository at this point in the historyBrowse files
fix(plugin-audit): the parent-record read gates skip an object the caller cannot read without reading it, so no denial is logged for the skip (#23663)
Fixes#23279
Clause-②: no
## What this changes
The `sys_activity` and `sys_audit_log` read gates
(`packages/plugins/plugin-audit/src/parent-record-read-gate.ts`,
`computeParentRecordFilter`) serve a row only when the caller can read
the record it is about. They decide that once per parent object the
query's rows name, by reading that object as the caller
(`resolveReadableParentIds`, #20833 / PR #21069). For every parent
object the caller holds no object-level read on, plugin-security's
middleware refused that read and logged `[Security] Access denied` at
WARN before the gate dropped the object.
The gate now asks the security service's `canReadObject` first, for each
parent object, with the caller's own envelope (minus the
operation-private keys, exactly what the caller-scoped read gets). It
then reads as the caller only the objects that check admits. An object
it refuses gets no readable id, which is what its refused read gave it,
so the emitted WHERE and the rows served do not change. It is just never
read, so nothing is refused and nothing is logged.
- **The check already existed.** `ISecurityService.canReadObject` is
optional on the contract
(`packages/spec/src/contracts/security-service.ts`). plugin-security
registers it on its `security` service. It is the middleware's own
object-level read gate, arm for arm: the principal-less and no-set deny
baselines, the unresolved-posture refusal, the ADR-0066 D3 capability
AND-gate, the `allowRead` CRUD grant and the ADR-0090 D10 delegator
intersection. It answers without logging or throwing, and
`can-read-object-admission.test.ts` pins it against the real middleware.
`searchAll` already asks it the same way. So **plugin-security does not
change**: no new method, and nothing is touched in `security-plugin.ts`.
- **The check can only exclude objects.** An admitted object is still
read as the caller, and that read still decides row scope, sharing, RLS
and every later middleware arm.
- **Fails closed.** A `canReadObject` that throws, or answers anything
other than `true`, refuses that object. Its rows are excluded and the
gate logs one warning naming the object. plugin-security's own
implementation never throws: it catches internally and answers `false`.
- **No check composed.** On a kernel without a security service, or with
one whose contract omits the optional method, every parent object is
read as the caller, as before. The engine's middleware still decides, so
this state can leave the answer only as it is today, never widen it.
This is the contract's "absence is a defined state" reading. It is also
what keeps plugin-audit's existing integration suites, which compose no
security plugin, unchanged.
- **Wiring.** `AuditPlugin` resolves `ctx.getService('security')` on
every read (the security plugin may register after this one) and passes
the resolver to both installers. The installers and
`computeParentRecordFilter` take it as a required argument. None of them
is exported: `src/index.ts`, `package.json` and `src/objects/**` are
unchanged, and `ParentObjectReadAdmission`,
`ParentObjectReadAdmissionSource` and `computeParentRecordFilter` appear
in neither `dist/index.d.ts` nor `dist/index.d.mts` (positive control:
`installCommentReadVisibility` appears twice in each).
- ⛔ **Not changed:** the level of plugin-security's denial log, and the
gate does not catch or mute that log. A caller's own read of a skipped
object is still refused and still logs exactly once. That is pinned
below.
## Measured on a real stack (before and after)
`packages/qa/dogfood/test/parent-read-gate-silent-skip.dogfood.test.ts`
runs a real org-bound `bootStack` with the real `SecurityPlugin` and the
real `AuditPlugin`. The member holds `org_member` and a no-wildcard app
grant, plus an explicit set granting object-level read on `sys_activity`
and `sys_audit_log`. The kernel logger every plugin context shares is
spied on per request.
| member request | denial WARN lines, base build (`c5dd7fce30`) | after
|
|:--|:--|:--|
| `GET /data/sys_activity?top=20&sort=-timestamp` (the console header's
feed) | 8 | 0 |
| `GET /data/sys_activity?limit=1000` | 8 | 0 |
| `GET /data/sys_audit_log?limit=1000` | 8 | 0 |
The 8 lines are 4 objects × 2 engine reads: the list's rows and its
`total` both run the gate. Two objects hit the CRUD arm ("operation
'find' on object … is not permitted for positions [org_member,
everyone]"): `sys_user_permission_set` and `sys_setting_audit`. Two hit
the capability arm ("requires capability [manage_platform_settings]"):
`sys_platform_setting` and `sys_migration`. The HotCRM report counted 16
lines on a larger app; this fixture's stream names fewer platform
objects. The mechanism is the same.
**Same rows.** Each case first reads with the security service's
`canReadObject` withdrawn (deleted from the registered service object,
then restored). That is the contract's optional-method state, where the
gate reads every parent as the caller, as the base did. The case then
reads again with it in place, on the same boot and the same rows, and
asserts the sorted ids are equal. The no-probe read must also log the
denials for exactly the set of refused parents, so the comparison is
armed. At the base build the id equality held and only the zero-denial
assertion was red: 3 failed and 1 passed of 4. After the fix: 4 passed
of 4.
**A real denial still logs once.** The member's direct `GET
/data/sys_user_permission_set` (CRUD arm) and `GET /data/sys_migration`
(capability arm) each answer 403 and log exactly one denial line naming
the object, both before and after.
**Row-level narrowing stays silent.** `cmt_private` is readable at
object level and owner-scoped at row level. The member gets 404 on the
admin's record, which is an armed control. The gate reads that object as
the member and logs nothing while doing it: the zero-denial reads above
include it.
## Pins
| file | what it holds | layer |
|:--|:--|:--|
| `plugin-audit/src/parent-record-read-admission.integration.test.ts`
(new) | a real kernel, the real `AuditPlugin` and CRUD mirror, a real
SQLite driver; the `security` stand-in registers AFTER the audit plugin.
On both `sys_activity` and `sys_audit_log`: a member's list and total do
not read the refused object (zero refusals), serve the same rows and
total as the no-probe read (which refuses it twice), and ask the check
once per parent object per read with the envelope minus `__readScope`.
An admin still reads every vault row | PR CI |
| `plugin-audit/src/activity-read-visibility.test.ts` (+8 cases) | a
refused object is never read and the WHERE equals the one its refused
read gave; an admitted object's own read still decides its ids; the
envelope; every object refused gives the deny-all sentinel with zero
reads; fail-closed on a throw (logged, never widening) and on any answer
that is not `true`; no check composed (absent, null, a throwing
resolver, a service without the method) reads every parent as before;
resolution on every read | PR CI |
| `plugin-audit/src/audit-log-read-visibility.test.ts` (+2 cases) | the
ledger shares the seam: a refused object is not read, the outside-class
rows are kept, and the WHERE is unchanged; a holder of the audit
capability is exempt before the check is asked | PR CI |
| `qa/dogfood/test/parent-read-gate-silent-skip.dogfood.test.ts` (new,
committed red first at `9defa89117`) | the real pair end to end, as
above | full battery |
## Ablations: old behaviour back, red, restore
Each ablation ran through `scripts/ablation-replace.mjs`, from the
committed fix (`9863e10294`). In every leg the anchor went from 1 to 0,
the injected text from 0 to 1, and the blob changed. A trap restored the
file by absolute path. The restore was proven each time: blob
`3cee7dd179` equals HEAD and `git diff HEAD` is empty.
| mutation | what it puts back | result (source-resolved plugin-audit
suites, 45 cases) |
|:--|:--|:--|
| M1: the check is never asked | the old gate | 8 failed: 5 activity
unit, 1 ledger unit, both integration cases |
| M2: a throwing check admits | fail-open | 1 failed: the throw case |
| M3: a truthy answer admits | a loose answer | 1 failed: the not-`true`
case |
| M4: the raw context is sent, with its private keys | a widened
envelope | 3 failed: the envelope unit case and both integration cases |
| (control, unmutated) | | 45 passed |
**Leg B (dogfood, resolves plugin-audit from `dist/`), M1.** After the
mutation the rebuild exited 0, and `ablation-dist-preflight` found the
marker in `dist/index.js` and `dist/index.mjs`. The dogfood pin then
gave 3 failed and 1 passed of 4. After the restore the rebuild exited 0,
the preflight with `--absent` reported the marker gone from all 6 built
files and the tree clean, and the pin gave 4 passed of 4.
## Verification (head `262e8548de`, `origin/main` `6792be6ab` merged in)
- `pnpm --filter @objectstack/plugin-audit test`: 49 files, 785 tests
passed.
- `pnpm --filter @objectstack/plugin-audit typecheck`: exit 0. That
covers `tsc --noEmit`, the scripts program and `check:test-typecheck`
over `tsconfig.test.json`, which includes `src/**/*`, so the new test
files are in it (0 errors).
- `pnpm --filter @objectstack/dogfood typecheck`: exit 0, 0 errors, with
the dogfood closure built at this head.
- Dogfood at this head: the new pin, plus `activity-parent-read-gate`,
`audit-log-parent-read-gate`, `audit-log-audit-capability`,
`activity-withheld-update`, `activity-field-values` and
`audit-log-field-values`: 7 files, 55 tests passed.
- Lint, declared narrowed: `eslint --no-inline-config --format json`
over the 8 changed TypeScript files gives 8 files, 0 errors and 0
warnings. The whole-repo `pnpm lint` runs in CI.
- Gate families from `dispatch-gates --commands` (70, derived at this
head): 69 exit 0. `check:dual-build-cjs-loads` exits 3 with PREREQUISITE
NOT MET, because it needs a whole-repo build. **NOT MEASURED** locally;
CI runs it. `dispatch-gates --ran` with exit codes recorded: 70 derived,
69 run, 1 NOT-MEASURED, 0 UNRUN.
- Changeset: `.changeset/23279-parent-read-gate-silent-skip.md`, a
`patch` on `@objectstack/plugin-audit` only. plugin-security ships
nothing new. The dogfood package is private.
## Acceptance notes
- **Measured against the dispatch's mechanism notes.** The producer is
as described. Each list request logs twice per object, because the rows
and the `total` both run the gate. The `sys_migration` line is the
middleware's ADR-0066 D3 capability arm, not a second producer. The
object-level check did not have to be added: it is the existing
`canReadObject`. One divergence from the suggested route: an *absent*
check reads every parent as the caller, as before, rather than denying
every object. Denying would change the rows on every kernel composed
without a security service, plugin-audit's own integration suites among
them. It is also not what the contract's optional-method rule asks. A
check that *throws* or answers anything but `true` does deny.
- **The same flood is likely at the other parent-derived gates. Read,
not measured.** These call core's `resolveReadableParentIds` with one
caller-scoped read per parent object: the `sys_comment` read gate
(`comment-access-hooks.ts`, `installCommentReadVisibility`),
`service-mailbox`'s `mail-read-visibility.ts`, and `service-storage`'s
attachment read gate (`attachment-access-hooks.ts`). A caller listing
rows whose parents include objects it holds no object-level read on
would log one denial per object per read. The fix would be the same
check before the caller-scoped read. The way to close it for every gate
at once is an optional object-level check on `resolveReadableParentIds`
itself, in `packages/core` (`domain:engine`), which this card leaves
alone. Nobody has picked this up.
- **No change to `content/docs/permissions/system-context.mdx`.** No new
`isSystem` read site is added.
- **Not measured:** a boot with more than one organization; an
on-behalf-of (delegated) caller through the real stack. The check's
delegator arm is plugin-security's, and its parity is pinned there.
---
_Generated by [Claude
Code](https://claude.ai/code/session_017WLmd5in9RXu7AjyHR8fKQ)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
fix(plugin-audit): the `sys_activity` and `sys_audit_log` read gates skip a parent object the caller cannot read without reading it, so the security plugin no longer logs a denial for each skipped object
6
+
7
+
Clause-②: no
8
+
9
+
Both gates serve a row only when the caller can read the record it is about. They decide that once per parent object the query's rows name, by reading that object as the caller. For every object the caller holds no object-level read on, plugin-security refused that read and logged `[Security] Access denied` at WARN: one line per object per read, and again for the list's `total`. The console header's activity feed reads `sys_activity` on every page view, so every non-admin page view logged a line for each platform object the stream names (`sys_migration`, `sys_platform_setting`, `sys_user_permission_set`, and others), and real denials were buried among them.
10
+
11
+
Each gate now asks the security service's `canReadObject` first, with the caller's own context. That is the security middleware's own object-level read check, and it answers without logging. The gate then reads only the objects it admits. An object it refuses contributes no rows, exactly as its refused read did before, so every caller is served the same rows, with the same `total`, as before. A caller's own read of such an object is still refused and still logged, once.
12
+
13
+
The check can only exclude objects. An admitted object is still read as the caller, and that read still decides row scope, sharing and RLS. If `canReadObject` throws, or answers anything other than `true`, that object's rows are excluded and the gate logs a warning naming the object. On a deployment without the security plugin, or with a security service that does not offer `canReadObject`, every parent object is read as the caller, as before.
0 commit comments