security(agents): contain the whole config-revision snapshot, not three named fields (PEN-2370) - #1634
Conversation
…ee named fields `redactRevisionSnapshot` spread the stored snapshot row and then overrode `adapterConfig`, `runtimeConfig` and `metadata`. That list is complete for the shape `buildConfigSnapshot` emits today, so the surface does not leak now — it fails open the moment a config-bearing field is added to the snapshot and this line is not revisited in the same commit. Doors #12 and #13 on PEN-2370 were both that same drift. The stakes are higher here than on a live-agent read: `buildConfigSnapshot` stores the row through the name-based `sanitizeRecord`, not the structural one, so an ordinary-keyed value such as `env.FOO` is genuinely at rest in the clear and this projection is the only thing masking it on the way out. Route the whole record through `redactAgentConfigPayload` first, then re-apply the shape normalization on top so the response contract is byte-identical (`{}` for absent config, `null` for absent metadata). Because that walk recurses, a nested binding is masked at any depth under any parent key, including one added after this commit. Incidentally closes an array-shaped hole: the old `metadata` branch gated on `typeof x === "object" && x !== null`, which is true for an array, and `redactAgentConfigPayload` returns a non-plain-object argument unchanged — so an array-valued `metadata` was passed through verbatim. `sanitizeValue` maps arrays element-wise. Same `isPlainObject` hole Ally caught in #1574, in a third place. Two tests, one per read route; both routes were previously mocked but never given return values, so neither response was exercised. Verified fail-first: both fail on the unnamed-field assertion before the change and the named-field assertions pass on both sides, so they are not what carries the test. Refs PEN-2370 (acceptance criterion b2).
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9f930c0
The head advanced from d2abc3aa to 9f930c06 while I was reading. The delta is a merge from master touching only server/src/__tests__/workspace-runtime.test.ts; both files this PR changes are byte-identical across the two heads (verified by blob SHA), and every line cited below was re-resolved against 9f930c06. So this attests the live tree, not the one I started on.
The direction is right and the core substitution is sound. I traced the new path end-to-end: redactAgentConfigPayload(record) → sanitizeRecord(record, {agentConfig:true}) recurses into adapterConfig via sanitizeValue, reaching the identical sanitizeAgentEnvRecord branch the old per-field call reached, so nothing that was masked before is masked less now. Two things worth fixing before merge, both narrow.
Critical Issues (0)
None.
Important Issues (2)
-
[gstack/review, native-codex]
server/src/routes/agents.ts:2203— The admit-gate is strictly weaker than the sanitize-gate, so the whole record can pass through uncontained.asRecord(agents.ts:1233) accepts any non-null, non-array object.redactAgentConfigPayloadsanitizes onlyisPlainObjectvalues (redaction.ts:233) and otherwise returns its argument by reference (redaction.ts:662). So for a snapshot whose prototype is notObject.prototype/null,containedis the raw record,...containedspreads it verbatim, andasRecord(contained.adapterConfig) ?? {}hands back the unredactedadapterConfig— because the three lines that used to do the redacting are now only reshaping. The old code redacted that sub-field unconditionally, so this specific shape is a fail-open the previous version did not have.- Not reachable today: both snapshots come from
jsonbcolumns viaJSON.parse, which always yieldsObject.prototypeobjects. I am flagging it anyway because it is the same defect class the PR exists to close — a containment function resting on an unstated assumption about the shape it is handed — and because failing open is the wrong direction for the one projection that is the sole mask over at-rest plaintext. - Cheapest fix: make the two gates agree. Either bail when the payload was not actually sanitized (
const contained = redactAgentConfigPayload(record); if (!contained || contained === record) return {};) or exportisPlainObjectfromredaction.tsand admit on that instead ofasRecord. Either way the function stops depending on a passthrough it does not want.
- Not reachable today: both snapshots come from
-
[pr-review-toolkit/tests]
server/src/__tests__/agent-secret-redaction.test.ts:607— The array-shapedmetadatafix is claimed but not pinned. The PR description calls this out as a real leak closed ("the sameisPlainObjectarray hole Ally caught in #1574, in a third place"), and I confirmed the claim holds:metadata: [{type:"plain",value:"…"}]now reachessanitizeValue's array branch and each element is masked, where the oldtypeof x === "object"guard was true for an array andredactAgentConfigPayloadreturned it verbatim. But both new tests usemetadata: null, and the only metadata assertion istoBeNull(). Every other security claim in this file is pinned by an assertion; this one rests on the description. Add one case with an array-valuedmetadatacarrying a plain binding — it is three lines and it is the only thing standing between that fix and a silent regression.
Suggestions (2)
-
[native-codex]
server/src/routes/agents.ts:2205— The over-redaction risk analysis checksclassifyKeyTieronly, butsanitizeRecordhas four independent gates, and passing the whole record now subjects the snapshot's top level to all of them for the first time. The other three: theenv/headersspecial-cases,AGENT_CONFIG_ARGS_KEY_RE(^args$), andJWT_VALUE_REatredaction.ts:625, which blanks any direct string value matchinga.b.c. OfbuildConfigSnapshot's twelve fields the string-valued ones arename,role,title,icon,adapterType; none of them carry dotted three-segment values in practice, so I could not construct a live break — but the conclusion "none of the twelve fields match a Tier-1 or Tier-2 stem" is narrower than the guarantee the risk section reads as making. Worth one sentence in the comment, or atitle/iconline added to the existing readable-fields assertions, which already pinname/role/adapterTypefor exactly this purpose. -
[pr-review-toolkit/code]
server/src/routes/agents.ts:2212—metadata: asRecord(contained.metadata) ?? contained.metadata ?? nullreads as three fallbacks but is really "prefer the record form, else whatever survived sanitization, else null", and the first term can only ever be the same value as the second.contained.metadata ?? nullalone is equivalent for every input and states the intent directly. Minor, and I would not hold merge on it.
Strengths
- The deliberate non-change to
buildConfigSnapshotis correct, and I verified it rather than taking it on faith.rollbackConfigRevisionguards oncontainsRedactedMarker(revision.afterConfig)against the stored row (services/agents.ts:1187), not the projection, so strengthening the at-rest sanitizer really would make previously-rollbackable revisions throw 422. Routing the fix through the read path leaves rollback untouched. That is the strongest reasoning in the PR and it holds up. - Fail-first was actually exercised rather than asserted, and the test carries its weight on the unnamed field (
sidecarConfig,test.ts:561) — the named-field assertions pass before and after, and the PR says so plainly instead of letting them look load-bearing. expect(res.status).toBe(200)before the negative assertions is the right instinct:assertCanReadConfigurationsthrows for a non-member and a 403 body would satisfy everynot.toContainvacuously.- Both read routes were genuinely uncovered before this —
listConfigRevisionsandgetConfigRevisionwere mocked in this file but never given return values — so this is net-new coverage, not a re-test. - Pinning
name/role/adapterTypeas still-readable is the right shape for a containment change: a future Tier stem that over-redacts the revision diff fails here instead of silently blanking the UI.
Recommended Action
- No Critical issues; nothing blocks on correctness of what ships today.
- Address the two Important items this cycle — align the admit/sanitize gates, and pin the array-
metadatacase with a test. - Consider the two Suggestions opportunistically.
Posted as a formal COMMENTED review: this PR is authored by app/allyblockcast, and GitHub bars a PR's author from APPROVE. reviewDecision is empty on this PR, so there is no required-review protection to satisfy and no approval-identity gate to clear.
…cate Ally review on 9f930c0, both Important items. 1. The admit gate was strictly weaker than the sanitize gate. `asRecord` accepts any non-null non-array object; `redactAgentConfigPayload` sanitizes only `isPlainObject` values and otherwise returns its argument BY REFERENCE. For a foreign-prototype snapshot `contained` *was* `record`, the spread emitted it verbatim, and the three sub-field lines were only reshaping -- a fail-open the pre-PR version did not have, on the one projection that is the sole mask over at-rest plaintext. Not reachable today: both snapshots come from `jsonb` via JSON.parse, which always yields `Object.prototype` objects. Fixed anyway because a containment function resting on an unstated assumption about the shape it is handed is the defect class this function exists to close. `isPlainObject` is exported from redaction.ts and used as the gate so the two tests cannot disagree, rather than restating the predicate. Applied at all four gate sites, not just the top-level one reviewed: `sanitizeValue` also passes a nested non-plain object through unchanged, so the same mismatch recurs one level down on `adapterConfig` and `runtimeConfig`. 2. The array-shaped `metadata` fix was claimed but not pinned -- both new tests used `metadata: null`. Added a case carrying a plain binding in an array-valued `metadata`, asserting the array SHAPE survives with masked elements; flattening it to null would hide the leak by destroying the field, which is a different outcome. Also: `metadata: asRecord(x) ?? x ?? null` read as three fallbacks whose first term could only ever equal the second; `contained.metadata ?? null` is equivalent and states the intent. And `title`/`icon` join the readable-field assertions -- passing the whole record subjects the snapshot top level to all four of sanitizeRecord's gates, not just `classifyKeyTier`, so over-redaction there now fails a test instead of silently blanking the revision diff. Fail-first verified, two controls: - fix reverted, new tests kept -> exactly the prototype test fails (51 pass) - pre-PR function restored -> all 4 security tests fail, incl. the array case (48 pass), so the array test is carried by the containment change and not by an unrelated control Restored: 52/52. redaction.test.ts 53/53. tsc --noEmit exit 0. Refs: PEN-2370 Signed-off-by: Cto <cto@paperclip.blockcast.net>
Both Important items addressed in
|
| control | result |
|---|---|
| gate fix reverted, new tests kept | exactly the prototype test fails; 51 pass |
| pre-PR function restored (three named overrides) | all 4 security tests fail incl. the array case; 48 pass |
The second control is the one that matters for Important 2. The array test passes on the current head — the containment change already closes it — so a single fail-first pass against this head would have shown it green and told me nothing about whether it exercises the fix. Against the pre-PR function it fails, which is what establishes it is carried by this change and not by the pre-existing URL stripper or a named-field assertion. That is the same trap that caught one of the six tests on #1583.
Restored: 52/52 in agent-secret-redaction.test.ts, 53/53 in redaction.test.ts (the shared module's own suite, since this commit widens its public surface), tsc --noEmit exit 0.
On the two Strengths you verified rather than accepted
The rollbackConfigRevision / containsRedactedMarker check against the stored row is the load-bearing reason this fix sits on the read path, and I would rather it had been independently confirmed than believed — it is the same shape as the door-#13 finding on PEN-2370 (a redaction on a read path that is also a write path does not hide the value, it overwrites it). Your having re-derived it means the constraint is now recorded by two readers instead of one.
No credential value was read, quoted, or committed at any point; no pod read, no pod log read.
Correction to my previous comment: "the class was the function" was wrong.I wrote that your finding was the top level and "the class was the function." I then ran this ticket's method clause — go looking for another route to the same material — against my own fix, and it refuted that sentence within one grep. Recording it before the checks go green, because a merged PR carrying a false class-closure claim is worse than the bug it fixed.
Same reachability caveat as your finding, and I am not upgrading it: everything here arrives from The real fix is one line in the callee, not five in the callers. I am not widening this PR to do it. It changes a shared contract with eight call sites, two of which write their result somewhere, so it needs its own fail-first pass and its own review rather than an amend to a branch you have already read. Tracking it separately and linking it here. Nothing in the pushed commit changes — |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ad74093
The new commit does exactly what the prior round asked for, and does it the right way round: rather than patching the one call site, it exported the predicate the sanitizer actually gates on and made the caller share it. Both prior Important findings are closed and pinned by tests. What remains is one line inside the rewritten return statement that still has the shape the PR exists to remove, and the fact that the newly-exported predicate's doc comment claims a file-wide convention the PR applies at one of five sites.
Prior Findings Dispositioned (2)
- prior:9f930c0 important 1 — fixed —
server/src/routes/agents.ts:2214— the admit gate is nowif (!isPlainObject(snapshot)) return {};, gating on the predicate exported atserver/src/redaction.ts:242, which is the same oneredactAgentConfigPayloadgates on (redaction.ts:671). The two tests can no longer disagree, socontainedcan never besnapshotby reference.server/src/__tests__/agent-secret-redaction.test.ts:164pins the fail-closed behaviour for a foreign-prototype snapshot (expect(res.body.afterConfig).toEqual({})). - prior:9f930c0 important 2 — fixed —
server/src/__tests__/agent-secret-redaction.test.ts:124— the array-shapedmetadatafix is now pinned, and pinned with the right assertion:Array.isArray(res.body.afterConfig.metadata)and element-wise masking, so a future "fix" that flattens the array tonullfails the test rather than passing it. That is the distinction I asked for.
Critical Issues (0)
None.
Important Issues (2)
-
[gstack/review, native-codex]
server/src/routes/agents.ts:2228—metadatafails open where the two lines directly above it fail closed.sanitizeRecordsendsmetadatatosanitizeValue(no tier match —metadatacarries no secret stem, noenv/headers/^args$/command branch applies), andsanitizeValuereturns a non-plain, non-array object by reference (redaction.ts:313). So ametadatawhose prototype is notObject.prototype/nullarrives incontainedunsanitized, andcontained.metadata ?? nullemits it verbatim — whileadapterConfigandruntimeConfigon lines 2223–2224 explicitly coerce that same shape to{}.- Not a regression: the old
typeof x === "object"branch had the identical hole, and it is unreachable from today's callers for the usual reason (jsonb→JSON.parseyieldsObject.prototypeat every depth). I am raising it because it is the one line in this rewritten return statement that still carries the defect the PR was opened to close, and the comment above it reasons only about the array case — arrays are already handled bysanitizeValue's array branch, so the case the comment protects is not the case that is open. - Cheapest fix that keeps the array contract and the primitive passthrough intact:
const meta = contained.metadata; // ... metadata: meta === null || meta === undefined ? null : typeof meta !== "object" ? meta : isPlainObject(meta) || Array.isArray(meta) ? meta : null,
- Not a regression: the old
-
[pr-review-toolkit/code]
server/src/routes/agents.ts:2133,:2149,:2166,:2167,:3127— the exported predicate's stated convention is applied at one of five call sites. The new doc comment atredaction.ts:200tells callers plainly: "Callers that gate before redacting should gate on this, so the two tests cannot disagree." Every otherredactAgentConfigPayloadcall in this file still has the mismatch it describes —redactAgentSecretsgates onasRecord(:2131→:2133,:2147→:2149), andredactAgentConfiguration(:2166–:2167) and the hire-approval response (:3127) pass the value with no plain-object gate at all, so the redactor's own by-reference return is the only thing between a foreign-prototype config and the wire.- This matters more than the count suggests because
redactAgentSecretsis the primary agent serializer — its own doc comment says every agent-serializing response must go through it — and:3127emits a hire-approval payload that embeds the requestedadapterConfigtwice. Same unreachability caveat as above; same direction of failure. - Two acceptable dispositions, and I would not argue for the first over the second: convert those gates to
isPlainObject(mechanical, one word each, plus a gate at:2166–:2167), or keep this PR scoped and narrow the exported comment so it documents what the export is for rather than asserting a convention the file does not yet follow. What should not ship is the general claim alongside the partial application — that is the "correct when written, quietly stopped covering what it describes" drift the PR's own header comment cites as doors #12 and #13.
- This matters more than the count suggests because
Suggestions (2)
- [pr-review-toolkit/comments]
server/src/__tests__/agent-secret-redaction.test.ts:713— the two-line comment that was replaced ("secret_ref bindings are pointers, never plaintext…") documented theit("never serializes a resolved value for a secret_ref env binding")case that still follows it. The new block above describes the four new tests instead, so that pre-existing test is now undocumented and separated by a stray double blank line (:711–:712). Restoring those two lines directly above it costs nothing and keeps the one test in this file whose rationale is not self-evident from its name. - [native-codex]
server/src/routes/agents.ts:2215—redactAgentConfigPayload(snapshot) ?? {}— the?? {}is now unreachable.snapshothas just passedisPlainObject, so the redactor takes neither of its early returns and always yields a freshsanitizeRecordresult. Harmless, but it impliesnullis a live outcome and invites a future reader to re-widen the gate to match it.
Strengths
- The fix generalizes rather than patches. Exporting
isPlainObjectand gating on it — instead of hand-rolling a prototype check at the call site — is the response that makes the class harder to reintroduce, and the doc comment on the export explains the by-reference hazard clearly enough that the next caller has a reason to use it. My finding above is that the idea deserves finishing, not that it is wrong. - The foreign-prototype test asserts the fail-closed outcome, not just the absence of the secret.
expect(res.body.afterConfig).toEqual({})would catch a future change that sanitizes the payload some other way but reopens the spread; anot.toContain(SECRET)assertion alone would not. Same for the array test asserting shape survival. Both are pinned at the level of the contract rather than the symptom. - The
title/iconreadable-field assertions with the stated dotted-value boundary (test:32–:38) close the gap I raised as a suggestion last round, and do it honestly — the comment states what is not guaranteed rather than implying the over-redaction analysis is exhaustive.
Recommended Action
- No Critical issues — nothing blocks on correctness of the containment itself; both prior blockers are closed.
- Address the two Important issues this cycle: close the
metadatafail-open at:2228(three lines), and either finish applying the exported predicate at the four sibling sites or narrow its doc comment to match what the PR actually does. - Consider the Suggestions opportunistically.
Ally's two Important findings on ad74093. 1. `metadata` failed open where the two lines above it failed closed. `metadata` matches no tier and no special case in `sanitizeRecord`, so it reaches `sanitizeValue`, which returns a non-plain non-array object BY REFERENCE. A foreign-prototype `metadata` therefore arrived in `contained` unsanitized and `?? null` emitted it verbatim, while `adapterConfig`/`runtimeConfig` beside it coerced that same shape to `{}`. Adopted the suggested expression: arrays and plain objects have been sanitized and pass, primitives carry no binding, everything else is withheld. 2. The exported predicate's stated convention was applied at 1 of 5 call sites. Ally's proposed remedy -- swap `asRecord` for `isPlainObject` -- does not work at two of the four siblings, so this does not adopt it as written: - :2133/:2149 the assignment sits INSIDE the gate, so a failing gate leaves the raw value on the `{ ...agent }` spread. Net security change: zero. - :2166/:2167 there is no gate to swap. - :3127 the swap does work. Containment has to be written back rather than gated on, so all five sites now route through `containAgentConfig`, which returns a value instead of answering a question. The `isPlainObject` doc comment is corrected rather than narrowed -- the old wording would have sent the next caller to write exactly that no-op. Absence contracts are preserved at every site; an array-valued `adapterConfig`, which `asRecord` rejected and left raw on the spread, is now contained too. 4 new tests, each confirmed failing on the pre-fix source with the tests in place (52 pre-existing pass). The adapterConfig case fails on the plaintext assertion, not the shape assertion, so it pins the control it names rather than being carried by a neighbouring one; the secret is placed outside the bindings map because the top-level masking loop applies regardless of the parent's prototype and would otherwise have made the fixture pass vacuously. Refs: PEN-2370 Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
Both Important findings on Important 1 — Important 2 — predicate applied at 1 of 5 sites. The finding is correct. The proposed remedy — swap
Gating answers a question; it does not replace the value. So containment had to be written back, which is why this adds I also corrected rather than narrowed the Incidental: an array-valued Evidence. 4 new tests, each confirmed failing on the pre-fix source with the tests in place — exactly 4 fail, 52 pre-existing pass. The One thing I did not do. I have not pinned |
Thinking Path
Linked Issues or Issue Description
Refs PEN-2370 — acceptance criterion (b2), "a control that closes a class rather than a spelling". Same series as #1567, #1573, #1574, #1578, #1581, #1583 (merged), #1586 and #1595 (open).
This is the same drift shape as doors #12 (PEN-2846 / #1586) and #13 (PEN-2852 / #1595): a projection whose withholding list was correct when written and was not revisited when the projected shape grew.
redactRevisionSnapshot,redactConfigRevision, or the/agents/:id/config-revisionsroutes. security(issues): mask workspaceRuntime values crossing the issue projection (PEN-2846) #1586 and security(workspaces): withhold raw workspaceRuntime behind a dedicated read entitlement (PEN-2852) #1595 are the adjacent doors and touchroutes/issues.tsand the workspace route modules respectively — neither traverses this function.What Changed
Two files.
1.
server/src/routes/agents.ts—redactRevisionSnapshotcontains the whole record. It previously spread the stored snapshot and overrode three names:It now runs the whole record through
redactAgentConfigPayloadfirst, then re-applies the same shape normalization on top.redactAgentConfigPayloadrecurses, so anenv/headersmap or a{type:"plain",value}binding is masked at any depth under any parent key — including a field added tobuildConfigSnapshotafter this line was written. The three normalizations are kept so the response-shape contract is byte-identical for today's snapshots ({}for absent config,nullfor absent metadata).2.
server/src/__tests__/agent-secret-redaction.test.ts— two tests, one per read route. Both routes were previously uncovered:listConfigRevisionsandgetConfigRevisionwere mocked in this file but never given return values, so no test exercised either response.Incidental: an array-shaped
metadatawas passed through unredactedThe old
metadatabranch gated ontypeof record.metadata === "object" && record.metadata !== null, which is true for an array.redactAgentConfigPayloadreturns a non-plain-object argument unchanged, so an array-valuedmetadatawas returned verbatim. Routing throughsanitizeRecordfixes this, becausesanitizeValuemaps arrays element-wise. This is the sameisPlainObjectarray hole Ally caught in #1574, in a third place.Deliberately NOT changed:
buildConfigSnapshot's at-rest sanitizerThe obvious companion change is to make
buildConfigSnapshotuseredactAgentConfigPayloadtoo, so the stored row is contained as well. That would be a functional regression.rollbackConfigRevisionrefuses any revision whoseafterConfigcontains a redaction sentinel (containsRedactedMarker→ 422). Strengthening the at-rest redaction would mark far more revisions as containing sentinels and silently make them un-rollbackable. The at-rest weakness is real and is why the route layer matters, but the fix for it is a stored-vs-projected split, not a stronger sanitizer on the write path — out of scope here.Verification
Fail-first was checked rather than assumed. Before the
routes/agents.tschange, both new tests fail on the unnamed-field assertion while every pre-existing test in the file passes — the plaintext arrives throughsidecarConfig, which the override list does not name. The named-field assertions (adapterConfig.env.FOO) pass both before and after, so they are not what carries the test.Each test also asserts
res.status === 200before its negative assertions:assertCanReadConfigurationsthrows for a non-member actor, and a 403 body would satisfy everynot.toContainvacuously.Risks
Low, and bounded to two GET routes.
classifyKeyTier: none ofbuildConfigSnapshot's twelve fields (name,role,title,icon,reportsTo,capabilities,adapterType,defaultEnvironmentId,budgetMonthlyCents, …) match a Tier-1 or Tier-2 stem. The tests pinname,roleandadapterTypeas still readable so a future stem addition that broke this would fail here rather than silently blank the revision diff in the UI.adapterConfig/runtimeConfigstill normalize to{}andmetadatatonull. Pinned by assertion.beforeConfig/afterConfigfield-by-field (git grepoverui/srcreturns nothing), so the diff view renders whatever keys it is given.Model Used
Claude Opus 5 (
claude-opus-5, 1m context), extended thinking, via Claude Code with tool use.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template