Repository navigation
Commit 2f2fa11
Fixes #20964
Clause-②: yes (widening)
The approval payload snapshot is redacted at serve time, keyed on the
reading caller (#10749). It narrowed by the security service's read
projection, `getReadableFields`, alone. That projection counts a field
whose `maskingRule` applies to the reader as readable, because the data
plane serves the column with its value replaced. So the snapshot kept
the field and served it as it was captured at submission.
The redaction now also reads the security contract's query-side answer,
`getQueryableFields` (landed for #20935 as `83480c6a`), and serves only
the fields in both answers. A field served masked to this reader is
dropped, with its derived label. There is no second derivation of the
masking rule in `plugin-approvals`: who a rule applies to is the
contract's answer, asked as the reader.
## Measured first (H1), by class
The measurement used a real boot: `bootStack` with the real
`SecurityPlugin`, `ObjectQL` and SQL driver, REST, automation, the
record-change trigger and the approvals plugin. It had one synthetic
object with one capability-gated masked field, routed to a position held
by two approvers. The evidence is private to the dispatch.
| Read, same record | Reader the rule applies to: before | after |
Reader who lifts the rule (control): before | after |
|---|---|---|---|---|
| Approvals inbox, list | stored | absent | stored | stored |
| Approvals inbox, item | stored | absent | stored | stored |
| Generic data door on the request object | stored | absent | stored |
stored |
| Data plane read of the subject record (reference) | masked | masked |
stored | stored |
The "after" column is the dogfood pin below, green at this PR's head.
## The contract member read (H2)
`ISecurityService.getQueryableFields`, in
`packages/spec/src/contracts/security-service.ts`.
- **By its declaration:** it is a subset of `getReadableFields`, and
"the difference between the two is exactly the fields this caller sees
masked".
- **By `plugin-security`'s implementation:** the read projection keeps a
field that is served masked, using the read path's own partial-mask set.
The query-side answer reads the one query-guard derivation, which folds
every masked-for-this-caller field in as non-queryable. Both start from
the same field map (the evaluator, the `requiredPermissions` fold and
the delegator intersection). Their difference is therefore exactly the
read path's masked set.
So the member names the masked set by complement, and the redaction
reads it. Nothing in `packages/spec` or `plugin-security` changes.
## Drop, not mask (H3), and why
The data plane answers this reader with the masked value. This PR drops
the field instead, and that is a deliberate divergence in shape:
- The contract publishes **which** fields are masked for a reader, not
the masked **value**. Only `plugin-security`'s field masker produces the
masked value. Reproducing it here would be the second copy of the
masking rule that the triage forbids. Publishing it would be a new
contract member, which belongs to the contract lanes and is outside this
claim.
- Dropping is the fail-closed side of the data plane's answer. It
discloses strictly less than the masked value would (no kept characters,
no length). It is also the shape this seam already serves for a field
the reader may not read at all, so a drawer sees one "not for you"
shape.
## Fail closed when the answer cannot be had
The contract obliges a consumer that cannot get the query-side answer
not to read its absence as "nothing is masked". When the source has no
such member, answers `undefined` or throws, the redaction serves no
snapshot field for that object and logs it. An unresolvable read
projection still passes the snapshot through whole, as before (#3807).
With the security plugin wired, the member is always present and answers
a list whenever the read projection does.
## Changes
- `packages/plugins/plugin-approvals/src/payload-redaction.ts`:
`FieldVisibilitySource` gains the optional `getQueryableFields`.
`resolveReadableSnapshotFields` answers the read projection intersected
with it, or fails closed as above. Both doors call this one function,
and so does the free-text predicate check (#11040), whose behaviour is
unchanged.
- `packages/plugins/plugin-approvals/src/approvals-plugin.ts`: the
service door's bridge to the `security` service forwards the member as
well. The generic data door was already handed the service itself.
- `.changeset/20964-approval-snapshot-masked-field.md`: `patch`. It
includes a note for a host that builds `ApprovalService` with its own
field-visibility source. Measured: no such producer exists in the tree,
and the plugin bridge is the only one.
- **Surface note:** the claim's file list names `payload-redaction.ts`
and "`payload-redaction-middleware.ts` only where the wiring needs it".
The service door's wiring lives in `approvals-plugin.ts`, not in the
middleware file. The middleware file needed no change. The unit pin and
the dogfood pin both read the plugin wiring, and ablation B shows it is
load-bearing.
## Pins (committed red before the fix)
-
`packages/plugins/plugin-approvals/src/approval-payload-masked-field.test.ts`,
13 cases. It uses a source double that answers both contract members per
reader. Both doors drop the masked-for-this-caller field and keep the
business fields. The derived label map is built and does not carry the
dropped field. The unmasking reader is served the stored value (the
control). The stored column keeps the whole row. Three fail-closed cases
cover a missing, `undefined` and throwing answer, and the unresolvable
read projection still passes through. The plugin's bridge forwards the
answer.
- Measured with the pins commit's (`a961b3310a`) source in the tree: 9
red and 4 green. The green cases are the two controls, the audit case
and the unresolvable pass-through, which hold both before and after.
-
`packages/qa/dogfood/test/approval-snapshot-masked-field.dogfood.test.ts`,
on a real boot as measured above, beside the inbox's existing route pin.
It asserts by class: the reference (the data plane masks the field for
this reader), the three approval reads (field absent for the reader the
rule applies to, business field present), and the control (stored value
on every read). Red at the pins commit and green after.
- `approval-payload-redaction.test.ts` (the existing #10749 pins): its
source double gains the member the real service now has. Fixture triage:
add the missing declaration. No field there carries a masking rule, so
the answer equals the read projection, and every assertion is unchanged.
## Ablation, predicted before running, at `15f97c152e`
Both suites resolve `plugin-approvals` from **source**: the unit suite
imports it relatively, and the dogfood isolated project aliases it to
`src`. So no `dist` leg applies. Each mutation went through
`scripts/ablation-replace.mjs` (anchor hit 1 to 0, blob moved, marker
counted on disk). Each was restored to a blob equal to HEAD with an
empty `git diff HEAD`, and the tree was clean afterwards.
- **A, the redaction ignores the query-side answer**
(`payload-redaction.ts`). Predicted and observed: 9 red and 4 green in
the new unit file. The green cases are the two controls, the audit case
and the unresolvable pass-through. The 16 existing redaction cases
stayed green, and the dogfood pin went red.
- **B, the plugin bridge stops forwarding the member**
(`approvals-plugin.ts`). Predicted and observed: only the bridge case is
red (1 red and 28 green across both unit files). The dogfood pin went
red: the service door fails closed and serves an empty snapshot to both
readers.
## Verification
Verification ran at `15f97c152e`. Line 2 is the measured `Clause-②`
(H4): no export is added, and the one type addition is an optional
member on an injected source. Any value of that member can only remove
fields from what is served, never add one, and no input is refused. The
other results (the derived gate set, the lint narrowing and the `--ran`
reconciliation) are in the dev report on #20964, because this body is
written once.
- `@objectstack/plugin-approvals`: `test` 52 files and 804 passed.
`typecheck` exit 0, including the test layer (no new debt).
- The dogfood pin and the existing inbox override pin: 2 files and 2
passed.
## Acceptance notes
- Main moved by four commits after this branch was cut (formula, the
explain engine in `plugin-security`, analytics and PM tooling). None
touches this surface, so the branch is not merged with main.
- The masked value is not reproduced, so an approver who may see a field
masked on the data plane sees no value for it in the approval drawer.
Whether a contract member that serves the masked value is wanted is left
to the seat.
## Patch rounds (the seat's append from the dev's report on #20964; the
dev writes a body only once)
### Patch round 1
Head `e6c9b9d56b` (was `15f97c152e`): two commits, no merge of `main`.
It applies the remedy in contract review `5922625982` (FAIL on the
semver grade only) and changes nothing else.
**What changed**
- `.changeset/20964-approval-snapshot-masked-field.md`:
- `@objectstack/plugin-approvals` goes from `patch` to `minor`.
- `Clause-②: no` becomes `Clause-②: yes (widening)`, here and on this
body's line 2. The seat edited line 2.
- One sentence is added. It names the one public-surface addition: an
optional `getQueryableFields(object, context)` member on the
field-visibility source that `ApprovalServiceOptions.fieldVisibility`
and `ApprovalService.attachFieldVisibility` accept.
- Every other byte is unchanged, including the FROM → TO note to a
self-composing host.
-
`packages/plugins/plugin-approvals/src/approval-free-text-scope.test.ts`:
the file's visibility double gains the query-side member, in the same
shape the first round gave the redaction test's double. No assertion
changed.
- Not touched: the fix, the service-door bridge, the unit pins and the
dogfood pin.
**Verification at `e6c9b9d56b`** (every run under the shared verify
lock)
- `@objectstack/plugin-approvals` tests: 52 files and 804 tests passed.
`typecheck` exit 0. The test layer holds exactly at its ledger, and the
edited file has 0 errors.
- The fail-closed branch in the free-text file, measured once with a
recording logger:
- before (the double at `15f97c152e`): 11 warns over 13 passing tests;
- after: 0 warns over the same 13.
- Changeset gates: `check-changeset-no-major` exit 0,
`check-adr-0087-registration` exit 0 (one non-breaking changeset),
`check-changeset-fixed` exit 0.
- The level axis, driven offline with a synthetic pull-request payload
that declares `Clause-②: yes (widening)`: exit 0 on `HEAD`, and exit 1
on `15f97c152e` with the required-minor refusal.
- `dispatch-gates --commands` over the 7-path branch diff: 67 derived,
67 run, every one exit 0. `--ran`: 0 not measured.
- `main` is 9 commits past the branch point. None of them touches this
diff's paths, so `main` was not merged.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
1 parent b84b240 commit 2f2fa11
7 files changed
Lines changed: 649 additions & 4 deletions
File tree
- .changeset
- packages
- plugins/plugin-approvals/src
- qa/dogfood/test
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
Lines changed: 11 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
147 | 147 | | |
148 | 148 | | |
149 | 149 | | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
150 | 161 | | |
151 | 162 | | |
152 | 163 | | |
| |||
Lines changed: 290 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
Lines changed: 11 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
147 | 147 | | |
148 | 148 | | |
149 | 149 | | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
150 | 161 | | |
151 | 162 | | |
152 | 163 | | |
| |||
Lines changed: 12 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
246 | 246 | | |
247 | 247 | | |
248 | 248 | | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
249 | 261 | | |
250 | 262 | | |
251 | 263 | | |
| |||
0 commit comments