Skip to content

Commit f313fbc

Browse files
fix(plugin-audit): a fired milestone activity row is withheld from a reader not served its watched fields (#22814)
Fixes #22786 Clause-②: no A narrowing of what a reader is served; no accepted input changes and no public type moves (triage `6106418840`, claim `6106790619`). ## What changed `packages/plugins/plugin-audit/src/activity-field-redaction.ts`. The withheld-row pre-scan from #21388 now makes ONE judgement per row, `isWithheldRow`. A row is withheld when either arm holds: - **The update arm** (`isWithheldOnlyUpdate`, unchanged): an update whose every recorded change the reader is not served. - **The milestone arm** (`isWithheldMilestone`, new, beside it): a fired milestone whose declared watched fields are not all served. The watched fields are the list the CRUD mirror stamps in `metadata.text_sources.kind` (PR #22783), read through the existing `kindProvenance`. Both arms share one pre-scan, one per-read served answer, and one WHERE on `find` / `findOne` / `count` / `aggregate`. A pre-scan failure denies the read, as before. There is no second judge. - **One answer for both halves.** The row is withheld exactly when the column redaction would drop the row's `metadata.kind`. Both judge a declared value through one helper, `sourcesServed`. A declaration with no readable list is judged by one per-read unknown-provenance answer, `restrictedPerRead`. That answer was lifted out of `redactActivityRows`, so the middleware hands the same answer to the pre-scan and to the redaction. `computeWithheldUpdateFilter` called without that answer judges such a row closed. - **Route 2 is not ridden along.** Route 2 adds the watched field to the summary's sources. Once the row is withheld from that reader, route 2 changes nothing at the served read, so it would only restate the same answer in a second place. - **Out of the diff:** no `packages/spec`, no new `type` value, no new column, and no export from the package entry. ## The PM's mechanism assumptions, measured 1. **Holds.** On `680a86b4c`, `isWithheldOnlyUpdate` is at `:271` and the pre-scan calls it at `:321` with the reader's served set. 2. **Holds for every row the mirror writes from #22783 on.** `matchMilestone` returns every field a declared milestone watches, and the writer stamps that list beside `kind: 'milestone'`. So the list is the right input and is reused as is. - Rows written before #22783 carry no `metadata.kind`, so nothing stored on them says a milestone fired. They are judged exactly as before this change: key-by-key narrowing plus the update arm. They age out with the object's retention, which is the precedent the module header already sets for the mirror's earlier shape. - The `'unknown'` precedent applies only to a row that carries the kind but no readable list. The mirror never writes such a row. It is judged as the text precedent judges one: served only to a reader not restricted on the parent. - Whether earlier rows need more than this is raised with the dispatching seat. It is not decided here. 3. **Holds.** A withheld row reaches the reader through no column at all. This is pinned on the list read scoped by `object_name` + `record_id`, on the by-id read, and on `count`, in the same harness #22783's tests use. ## Pins (`activity-milestone-kind.integration.test.ts`) These run on a real engine, a real SQLite driver, the real CRUD mirror and the real `AuditPlugin` middleware chain, with the security service as the one stand-in: - A reader withheld a watched field gets neither milestone row: not on the list read and not by id. - CONTROL: a reader served every field gets each milestone row with `metadata.kind`, both by id and on the list. - CONTROL: a reader withheld only a field no milestone watches still gets the milestone rows with the kind. This is the existing test, unchanged. - CONTROL: a row no milestone fired is served to the withheld reader deep-equal to the row the control is served. That reader is served exactly the non-milestone rows. - `count` agrees with the rows served, for every reader. - Fail closed: when the pre-scan's system read fails, a non-system reader's list read is empty and its count is 0. The system read is unaffected, and the read recovers once the pre-scan works again. - The deny is a WHERE that matches nothing, as #21388's rule already does. It is not a refusal envelope, so there is no `code` or `status` to assert. - Unit pins cover `isWithheldMilestone`, `isWithheldRow` and the milestone arm of `computeWithheldUpdateFilter`, including the unknown-provenance and no-answer cases. Two existing pins from #22783 asserted that the withheld reader was served the milestone row without its kind. They now assert the row is withheld, which is the behaviour this card changes. The key-for-key pin drops that reader's row. ## Verification Every reading below was taken at `aa9b481c1`, the head this PR opens at. That head has `origin/main` `179f7bf6c` merged in: three docs-only commits, none touching a file this diff touches. Each command ran through `scripts/pm/os-verify-lock.sh`, with the exit code captured before any pipe. - **Build:** `pnpm exec turbo run build` over `@objectstack/plugin-audit...` and the `check:i18n` closure exited 0. - **Tests:** `pnpm --filter @objectstack/plugin-audit exec vitest run --maxWorkers=2` passed 46 files and 738 tests. - **Typecheck:** `pnpm --filter @objectstack/plugin-audit typecheck` exited 0. That covers the source, the scripts and the test layer (`check:test-typecheck: OK`, 0 debt entries). - **Dogfood:** `pnpm --filter @objectstack/dogfood exec vitest run --maxWorkers=2 test/activity-field-values.dogfood.test.ts` passed 10 of 10. It boots for real, with `SecurityPlugin` and REST. It is not owed, because no public surface moved. It was run as a control that a milestone row reaches readers who are served its watched field. - **Gates:** `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 65 commands at this head. All 65 ran and each exited 0, including `check:i18n` and `check:dual-build-cjs-loads` with their prerequisites built. `--ran` reconciliation: 65 derived, 65 run, 0 NOT-MEASURED, 0 UNRUN. - **Lint (a narrowed run, declared with its three proofs):** - **Population:** eslint ran on the diff's two TypeScript files with `--no-inline-config --format json`. The diff's third file, the changeset `.md`, is answered by eslint itself with "File ignored because no matching configuration was supplied". - **Count:** the JSON output counts 2 files linted, 0 errors and 0 warnings. - **Invariance:** `eslint.config.mjs` enables no type-aware linting. `--print-config` on the source file resolves no `parserOptions.project` and no `projectService`. The only files the config reads are two baseline JSONs that this diff does not touch. So no untouched file's verdict can move. - **Ablations (one-off, not left as tests):** run from the committed state `76f2e23b9` through `scripts/ablation-replace.mjs`. In each run the anchor hit exactly once, and the blob was shown to change on disk and then shown restored to the HEAD blob with `git diff HEAD` empty. The subject resolves through `src/` (this package's own tests import it relatively), so no `dist/` preflight applies. 1. **Milestone arm dropped from `isWithheldRow`:** 7 red and 15 green in the milestone file. The red ones are the four withheld-reader served-read pins (list, by-id, served set, count) and the arm's three unit pins. Every control stayed green. 2. **Pre-scan deny removed from the middleware's catch:** exactly one red, the fail-closed pin (1 of 22). CI was not awaited. ## Acceptance notes - `computeWithheldUpdateFilter` keeps its name and its log text ("withheld-update pre-scan"). A fired milestone is itself an update row, so the name still holds, and renaming it would only churn the call sites. - PR #22783's kind-dropping arm in `redactActivityRows` is kept. Through the middleware it no longer sees a withheld milestone row, but it still judges direct callers. It now shares the one answer with the row rule. --- _Generated by [Claude Code](https://claude.ai/code/session_01CBAfsWMSfM3EToQGVStEcp)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f9c7588 commit f313fbc

3 files changed

Lines changed: 300 additions & 49 deletions

File tree

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
---
2+
'@objectstack/plugin-audit': patch
3+
---
4+
5+
A `sys_activity` row that an `activityMilestones` entry fired is withheld from a reader who is not served every field a declared milestone watches
6+
7+
Clause-②: no
8+
9+
A fired milestone's row speaks about its watched fields: its `type` and its summary come from the
10+
milestone declaration, beside `metadata.kind`. Narrowing the row column by column left those two
11+
columns speaking to a reader not served a watched field.
12+
13+
The row is now withheld from that reader as a row, on every read (`find`, `findOne`, `count` and
14+
`aggregate`), by the same pre-scan and the same served-field answer that withhold an update whose
15+
every recorded change the reader is not served. A list's total, its pages and a by-id read all
16+
agree with the rows served.
17+
18+
- **A reader served every watched field** is served the row unchanged, with `metadata.kind`.
19+
- **A reader withheld only fields no milestone watches** is served it too.
20+
- **A row no milestone fired**, and a row an app writes itself, are unchanged.
21+
- **If the pre-scan fails**, the read is denied rather than served unjudged.
22+
23+
Rows written before the CRUD mirror declared the watched fields carry no `metadata.kind`, so nothing
24+
on them says a milestone fired; they are judged as before. Nothing you author changes: no key,
25+
option, export or type is added or removed.

‎packages/plugins/plugin-audit/src/activity-field-redaction.ts‎

Lines changed: 130 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,9 @@
5656
* one of them is served the row without the kind. A reader served them keeps
5757
* it: it is a classification, and every key of it is one the reader may
5858
* read anyway. A kind on a row someone else wrote is the author's, served as
59-
* written, like its text.
59+
* written, like its text. [#22786] On a read through the middleware such a
60+
* milestone row does not reach that reader at all (see below); dropping the
61+
* kind is the column half of the same answer.
6062
* - The declaration itself is bookkeeping and is never served to a reader: it
6163
* names fields, and which field names a reader may see is the metadata
6264
* plane's question, not this one's.
@@ -106,6 +108,28 @@
106108
* judged, so the read is answered from the judged rows alone
107109
* (`{ id: { $in: KEPT } }`), and a warn says so.
108110
*
111+
* ## A fired milestone is withheld as a row from a reader not served its watched fields (#22786)
112+
*
113+
* A fired milestone's row speaks about its watched fields in columns the
114+
* key-by-key narrowing does not reach: its `type` is the declaration's, and
115+
* its summary is the declaration's sentence. Both say that a watched field
116+
* entered its declared value. So the same pre-scan, by the same answer,
117+
* withholds that row as a ROW from a reader not served every field a declared
118+
* milestone watches — the list the mirror declares the row's `metadata.kind`
119+
* from ({@link ActivityTextSources.kind}). One judgement per row
120+
* (`isWithheldRow`), one pre-scan, one WHERE:
121+
*
122+
* - The row is withheld exactly when the redaction would drop its kind. A
123+
* reader served every watched field is served the row with its kind; a
124+
* reader withheld only fields no milestone watches is served it too.
125+
* - A declaration with no readable list is of unknown provenance and is judged
126+
* as the text precedent judges one: served only to a reader not restricted
127+
* on the parent.
128+
* - A `kind` on a row the mirror did not write is the author's: not judged.
129+
* - Rows written before the mirror declared the list carry no kind, so nothing
130+
* on them says a milestone fired; they are judged by the rules above them,
131+
* and age out with the object's retention, like the earlier shape's text.
132+
*
109133
* ## Fail closed
110134
*
111135
* An unexpected failure strips every value-bearing column from the rows of the
@@ -251,11 +275,55 @@ const REDACTION_SEAM = 'activity field redaction';
251275
/** One read's served-unmasked answer, per parent object. */
252276
type ServedFor = (object: string) => Promise<string[] | undefined>;
253277

278+
/**
279+
* One read's answer to "is this reader served fewer fields of `object` than the
280+
* system is?", per parent object: how a value of UNKNOWN provenance is judged.
281+
* No full answer to compare against reads as restricted (closed).
282+
*/
283+
type RestrictedFor = (object: string, served: ReadonlySet<string>) => Promise<boolean>;
284+
285+
/** {@link RestrictedFor} for ONE read: one system read per parent object, never one per row. */
286+
function restrictedPerRead(security: ActivityFieldVisibilitySource | undefined): RestrictedFor {
287+
const restricted = new Map<string, Promise<boolean>>();
288+
return (object, served) => {
289+
let p = restricted.get(object);
290+
if (!p) {
291+
p = (async () => {
292+
let all: string[] | undefined;
293+
try {
294+
all = await security!.getReadableFields(object, SYSTEM_CTX as never);
295+
} catch {
296+
all = undefined;
297+
}
298+
// No full answer to compare against: unknown provenance stays closed.
299+
if (!Array.isArray(all)) return true;
300+
return all.some((f) => !served.has(String(f)));
301+
})();
302+
restricted.set(object, p);
303+
}
304+
return p;
305+
};
306+
}
307+
308+
/**
309+
* Whether a value composed from `sources` is served to a reader served
310+
* `served`: every source served, or, for `'unknown'` provenance, a reader not
311+
* restricted on the parent. The ONE answer both the row rule and the column
312+
* redaction judge a declared value by.
313+
*/
314+
async function sourcesServed(
315+
sources: string[] | 'unknown',
316+
served: ReadonlySet<string>,
317+
restricted: () => Promise<boolean>,
318+
): Promise<boolean> {
319+
return sources === 'unknown' ? !(await restricted()) : sources.every((f) => served.has(f));
320+
}
321+
254322
/** The engine slice the withheld-update rule's pre-scan needs. */
255323
export type ActivityRedactionEngine = ActivityMiddlewareEngine & Pick<CommentAccessEngine, 'find'>;
256324

257325
/** The columns the withheld-update pre-scan reads: the row, its parent object
258-
* and its stored change. */
326+
* and its stored change (with, [#22786], a milestone's kind and its declaration). */
259327
const WITHHELD_SCAN_COLUMNS = ['id', 'object_name', 'metadata'] as const;
260328

261329
/** No real row matches it: the withheld-update rule's fail-closed answer. */
@@ -280,18 +348,62 @@ export function isWithheldOnlyUpdate(metadata: Record<string, unknown> | null, s
280348
}
281349

282350
/**
283-
* [#21388] The WHERE that withholds, from one read, every update row whose
284-
* stored change is withheld whole from the reader `servedFor` answers for:
351+
* [#22786] Whether an activity row is a fired milestone, as the CRUD mirror
352+
* declared it, whose watched fields are not all served to this reader. The
353+
* watched fields are the list the mirror declares the row's
354+
* `metadata.kind` from ({@link kindProvenance}), judged by the same answer
355+
* the redaction serves that kind by, so the row is withheld exactly when the
356+
* redaction would drop its kind. False for a row with no milestone kind and
357+
* for a kind on a row the mirror did not write (the author's, served as
358+
* written). `restricted` answers for a declaration with no readable list.
359+
* Exported for direct testing.
360+
*/
361+
export async function isWithheldMilestone(
362+
metadata: Record<string, unknown> | null,
363+
served: ReadonlySet<string>,
364+
restricted: () => Promise<boolean>,
365+
): Promise<boolean> {
366+
if (!metadata || metadata[ACTIVITY_KIND_KEY] !== ACTIVITY_MILESTONE_KIND) return false;
367+
const watched = kindProvenance(metadata);
368+
if (watched === null) return false;
369+
return !(await sourcesServed(watched, served, restricted));
370+
}
371+
372+
/**
373+
* [#21388, #22786] The ONE withheld-row judgement the pre-scan makes: a row
374+
* whose very presence speaks about a field this reader is not served, which
375+
* narrowing its columns cannot take back. Either an update whose every
376+
* recorded change is withheld ({@link isWithheldOnlyUpdate}), or a fired
377+
* milestone whose watched fields are not all served
378+
* ({@link isWithheldMilestone}). Exported for direct testing.
379+
*/
380+
export async function isWithheldRow(
381+
metadata: Record<string, unknown> | null,
382+
served: ReadonlySet<string>,
383+
restricted: () => Promise<boolean>,
384+
): Promise<boolean> {
385+
return isWithheldOnlyUpdate(metadata, served) || (await isWithheldMilestone(metadata, served, restricted));
386+
}
387+
388+
/** No full answer is the closed answer: see {@link RestrictedFor}. */
389+
const RESTRICTED_CLOSED: RestrictedFor = async () => true;
390+
391+
/**
392+
* [#21388] The WHERE that withholds, from one read, every row
393+
* {@link isWithheldRow} withholds from the reader `servedFor` answers for:
285394
* `null` when the read withholds nothing, `{ id: { $nin: WITHHELD } }` when the
286395
* pre-scan saw every row the read can touch, and `{ id: { $in: KEPT } }` when
287396
* it reached its bound (fail closed: an unjudged row is never served).
397+
* [#22786] `restrictedFor` is the read's answer for a declaration of unknown
398+
* provenance; without one, such a row is judged restricted (closed).
288399
* Exported for direct testing.
289400
*/
290401
export async function computeWithheldUpdateFilter(
291402
engine: Pick<CommentAccessEngine, 'find'>,
292403
ast: Record<string, unknown>,
293404
servedFor: ServedFor,
294405
logger: ActivityRedactionLogger,
406+
restrictedFor: RestrictedFor = RESTRICTED_CLOSED,
295407
): Promise<unknown | null> {
296408
// The rows the read would touch, under SYSTEM context (the reader may not be
297409
// served their change; that is what is being decided). The caller's own
@@ -318,7 +430,9 @@ export async function computeWithheldUpdateFilter(
318430
servedSets.set(object, answer === undefined ? undefined : new Set(answer.map(String)));
319431
}
320432
const served = servedSets.get(object);
321-
isWithheld = served !== undefined && isWithheldOnlyUpdate(parseMetadata(row.metadata), served);
433+
isWithheld =
434+
served !== undefined &&
435+
(await isWithheldRow(parseMetadata(row.metadata), served, () => restrictedFor(object, served)));
322436
}
323437
const id = row.id;
324438
const usable = (typeof id === 'string' || typeof id === 'number') && String(id) !== '';
@@ -355,31 +469,15 @@ export async function redactActivityRows(
355469
/** The read's own answer, when the caller already holds one: the middleware
356470
* shares it with the withheld-update rule, so both judge by ONE answer. */
357471
served?: ServedFor,
472+
/** [#22786] Likewise the read's answer for unknown provenance. */
473+
restricted?: RestrictedFor,
358474
): Promise<void> {
359475
const list = (Array.isArray(rows) ? rows : rows ? [rows] : []) as unknown[];
360476
if (list.length === 0) return;
361477
// One answer per parent object per read, never one per row.
362478
const servedFor = served ?? servedFieldsPerRead(security, context, logger, REDACTION_SEAM);
363-
const restricted = new Map<string, Promise<boolean>>();
364479
/** Is this reader served fewer fields of `object` than the system is? */
365-
const restrictedOn = (object: string, servedSet: Set<string>) => {
366-
let p = restricted.get(object);
367-
if (!p) {
368-
p = (async () => {
369-
let all: string[] | undefined;
370-
try {
371-
all = await security!.getReadableFields(object, SYSTEM_CTX as never);
372-
} catch {
373-
all = undefined;
374-
}
375-
// No full answer to compare against: unknown provenance stays closed.
376-
if (!Array.isArray(all)) return true;
377-
return all.some((f) => !servedSet.has(String(f)));
378-
})();
379-
restricted.set(object, p);
380-
}
381-
return p;
382-
};
480+
const restrictedOn = restricted ?? restrictedPerRead(security);
383481

384482
for (const item of list) {
385483
if (!isRecord(item)) continue;
@@ -400,8 +498,8 @@ export async function redactActivityRows(
400498
const provenance = textProvenance(metadata);
401499
const kindSources = kindProvenance(metadata);
402500
/** Whether a value composed from `sources` is served to this reader. */
403-
const servedWhole = async (sources: string[] | 'unknown') =>
404-
sources === 'unknown' ? !(await restrictedOn(object, servedSet)) : sources.every((f) => servedSet.has(f));
501+
const servedWhole = (sources: string[] | 'unknown') =>
502+
sourcesServed(sources, servedSet, () => restrictedOn(object, servedSet));
405503

406504
if (metadata) {
407505
let changed = false;
@@ -441,8 +539,9 @@ const JUDGED_BY = ['object_name', 'metadata'] as const;
441539
* Inert on an engine without the middleware seam; `AuditPlugin` says so.
442540
*
443541
* [#21388] Before the read runs, the same middleware withholds every update row
444-
* whose stored change is withheld whole from this reader, as a WHERE on all
445-
* four reads (`computeWithheldUpdateFilter`). `AuditPlugin` registers it after
542+
* whose stored change is withheld whole from this reader, and [#22786] every
543+
* fired milestone row whose watched fields it is not all served, as a WHERE on
544+
* all four reads (`computeWithheldUpdateFilter`). `AuditPlugin` registers it after
446545
* the read gate, so the gate's parent filter is already in the WHERE its
447546
* pre-scan reads.
448547
*/
@@ -458,9 +557,10 @@ export function installActivityFieldRedaction(
458557
const security = getSecurity();
459558
// One answer per read, shared by the row rule and the redaction below.
460559
const servedFor = servedFieldsPerRead(security, ctx.context, logger, REDACTION_SEAM);
560+
const restrictedFor = restrictedPerRead(security);
461561
if (security && ctx.ast && PARENT_GATE_READ_OPS.has(ctx.operation)) {
462562
try {
463-
const filter = await computeWithheldUpdateFilter(engine, ctx.ast, servedFor, logger);
563+
const filter = await computeWithheldUpdateFilter(engine, ctx.ast, servedFor, logger, restrictedFor);
464564
if (filter) andIntoWhere(ctx, filter);
465565
} catch (err) {
466566
// A pre-scan failure must never fall open into the timing leak.
@@ -476,7 +576,7 @@ export function installActivityFieldRedaction(
476576
await next();
477577
const list = (Array.isArray(ctx.result) ? ctx.result : ctx.result ? [ctx.result] : []) as unknown[];
478578
try {
479-
await redactActivityRows(list, security, ctx.context, logger, servedFor);
579+
await redactActivityRows(list, security, ctx.context, logger, servedFor, restrictedFor);
480580
} catch (err) {
481581
// A redaction failure must never fall open into a leak.
482582
logger.warn(

0 commit comments

Comments
 (0)