fix(approvals): stop pushing a free-text predicate onto a masked payload snapshot - #11103
Conversation
…oad snapshot The approval payload snapshot is redacted at serve time, per reader (#10749), but `listRequests` / `countRequests` pushed a free-text disjunct onto the stored column, which the driver evaluates unmasked. The field-level read controls enforced on the way out therefore did not hold on the way in — declared, not enforced. This extends the settled `maskingRule` posture (#8993) to the snapshot column: a caller whose view of the snapshot is masked keeps free-text matching on the four columns of `sys_approval_request` they read whole, and loses only the snapshot disjunct. The masked/unmasked verdict comes from the same authority and the same per-caller call the serve path uses, asked as the caller — never a second, independently derived notion of "redacted", since that drift is the defect one layer down. Where serve declines to narrow (no authority wired, or a wired authority answering "unresolvable"), the disjunct is kept: consistency with serve, not blanket fail-closed. Dropping a disjunct is strictly narrowing — no query is refused, nothing is widened — and four disjuncts always remain, so the `$or` is never empty. Part of #11040
…type.at` `lib` is ES2021 for this package, where `at` does not exist. The package's own `typecheck` excludes `**/*.test.ts`, so the error surfaced only in the `check:type-check-coverage --re-measure` ratchet, as TEST_DEBT 348 -> 349. Fixed in the test rather than by growing the ledger or widening `lib`: the ratchet is shrink-only, and the file needs no ES2022 library surface. Part of #11040
📓 Docs Drift CheckThis PR changes 1 package(s): 3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 5 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 3c1b4cfb6ac005dd857d54e8b0eeec67d573604b && git checkout 3c1b4cfb6ac005dd857d54e8b0eeec67d573604b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin ab47f6974a275586355f06abd83a0975b8637d12 d0d9ef99c1b3e6f652195567d5a7dde3ccb7553d && git checkout -B drift-repro ab47f6974a275586355f06abd83a0975b8637d12 && git merge --no-ff d0d9ef99c1b3e6f652195567d5a7dde3ccb7553d
node scripts/docs-audit/affected-docs.mjs --json ab47f6974a275586355f06abd83a0975b8637d12
|
github-actions
Bot
added
documentation
⛔ merge queue 构建失败 — 先分诊，再决定要不要重排队列构建 32587070810 红了。队列跑的是全量套件（PR 侧 CI 只跑 affected 子集）， 失败的 job（日志抽取，best effort）：
跨 PR 相同签名（24h，按失败测试文件聚合）：
历史信号：
分诊清单：
Generated by Claude Code · merge-queue-triage workflow (#4859) |
2 participants
Fixes #11040
What was wrong
sys_approval_request.payload_jsonholds the submitted record's row. Since #10749 the serve path masks it per reader —redactPayloadscuts each row's snapshot down to the fields that caller may read on that row's subject object — while the full row deliberately stays at rest, so the approval record remains audit evidence of what was actually submitted.listRequests/countRequestspushed a free-text disjunct onto that same column. A filter is evaluated by the driver against the column at rest, before anything is served, so the field-level read controls #10749 enforces on the way out did not hold on the way in: the snapshot column was a free-text predicate for callers who do not read it whole. That isdeclaredwithoutenforced, and the platform has already settled the governing principle for it — undermaskingRule(#8993) a field a caller sees masked is non-filterable, refused loudly, because otherwise equality probes reconstruct the hidden span. This PR extends that settled posture to the snapshot column, reached through a different door.What changed
buildRequestWherenow decides the fifth disjunct per caller. The four columns ofsys_approval_requestitself —process_name,object_name,record_id,submitter_id— are read whole by anyone who can see the row and are untouched; they are always present, so the$oris never empty (an empty$orreads as match-none on some drivers and match-all on others, which would be a silent second defect). The snapshot disjunct is added only when the serve path would hand this caller the whole snapshot.Dropping a disjunct is strictly narrowing: no query is refused, nothing is widened, and ordering and pagination are untouched. Refusing the whole query would have been the louder behaviour change, and is deliberately not what this does.
The invariant that shapes the implementation
The masked/unmasked verdict is read from the same authority and the same per-caller call the serve path uses (
resolveReadableSnapshotFields), asked as the caller, never asSYSTEM_CTX. It is deliberately not a second, independently derived notion of "redacted" — comparing the readable set against the object's schema, say. Two derivations drift, and drift between a serve rule and a filter rule is precisely the defect being closed, reconstituted one layer down. So the only "not masked" answer accepted is the one serve itself acts on:undefined, the seam's documented do-not-narrow branch. When the seam holds a concrete list the mask is in force, whether or not it happens to remove a key from any particular row — a row-dependent question no predicate can answer before rows exist.The predicate-time tension, and how each case is handled
Redaction is decided per row (each row names its own subject object, hence its own readable set), but a filter is built before any row exists. Both cases are handled explicitly:
resolveReadableSnapshotFieldsanswersundefinedfor every object, so serve hands over every snapshot whole. The disjunct is kept — it discloses nothing serve does not already disclose. This is the shape every deployment that has not wired the security plugin runs: search is byte-for-byte unchanged. Checked first, and that order is load-bearing: the same helper also answersundefinedfor a blank object name, and reading that as "not masked" would hand the unfiltered query a disjunct on exactly the grounds that make it unanswerable.filter.objectpresentundefinedkeeps the disjunct, a concrete list drops it.filter.objectConsistency with serve is the rule here rather than blanket fail-closed: hardening the
undefinedcase into a narrowing serve itself does not perform would invent a behaviour with no counterpart on the way out.Accepted consequence, stated plainly: in a deployment with a field-visibility authority wired, the disjunct is dropped for every caller the mask is in force on — including one whose mask happens to be total for a given row. Narrowing that case is strictly safe, and the alternative (deciding "the mask is a no-op for this caller" from the object schema) is the forbidden second derivation above.
The pin
packages/plugins/plugin-approvals/src/approval-free-text-scope.test.ts— 13 tests in four groups, each of which a plausible wrong implementation needs:countRequestsagreeing with the list it paginates, and the security plugin's own fail-closed tier.$orcarries exactly four arms. Without this, dropping free-text search altogether scores green on (1) alone.undefined(approvals: adepartmentapprover never resolves when the business unit hasorganization_id = null(every seeded BU) #3807, explicitly not a denial). Without this, "drop the arm for everyone" scores green on (1) and (2).Ablation, both legs, at
d0d9ef99c. The test imports./approval-service.js— a relative specifier inside the package, so vitest readssrc/directly and neither leg involvesdist/; no rebuild can hide a mutation here.git checkoutfrom the commit, mutation marker confirmed absent, working tree byte-identical toHEAD): 13 passed.The 7 that stay green under the mutation are exactly groups (3) and (4) — behaviour the mutation does not change — which is what makes the red set attributable rather than a blanket failure.
Verification, at
d0d9ef99cpnpm --filter @objectstack/plugin-approvals test— 30 files, 565 tests passed.typecheck— clean (tsc --noEmit, script name echoed, not a zero-match no-op).Derived gate union via
node scripts/pm/dispatch-gates.mjs(no paths passed — the script derives the change set from the merge base itself); 17 families plus the ratchet, each exit code captured before any pipe, all green.One ratchet reddened during the work and was fixed in the code, never in the baseline:
check:type-check-coverage --re-measurereported@objectstack/plugin-approvalsTEST_DEBT348 -> 349. The package's owntypecheckexcludes**/*.test.ts, so the error was invisible there. Cause:Array.prototype.atagainst this package'slib: ES2021. Indexed from the end by hand instead; the count is back to the frozen 348 with zero errors naming the new file, and the gate's own verdict line now reads--re-measure: OK — 33 ledger entr(ies) re-measured, 1908 raw tsc error(s) total, none above its recorded number. The ledger was not touched andlibwas not widened.check:i18nfirst returnedPREREQUISITE NOT MET — the workspace CLI is not built/Nothing was checked. That is not measured, not a pass; it was re-run after building the workspace closure and then reportedOK (9 package(s) — all bundles in sync), namingplugins/plugin-approvals in sync (4 bundle(s)).Release notes
Input is the changeset,
.changeset/approvals-free-text-not-over-masked-snapshot.md, which states this as a deliberate search-semantics change. Nocontent/docs/releases/file is touched.Generated by Claude Code