security(workspaces): withhold operator-authored command strings on execution-workspace, project, and issue row responses (PEN-3073, PEN-3252) - #1858
Conversation
…very workspace-runtime response exit (PEN-3073)
PEN-2852 closed the `workspaceRuntime` exits. Its stated rationale — "service
commands, working directories and the environment those commands run with" — is
a rationale about a CLASS, and five sibling carriers of that class rode through
the boundary untouched, under four different nouns:
1. ExecutionWorkspaceConfig.provisionCommand / teardownCommand / cleanupCommand
2. ProjectWorkspace.setupCommand / cleanupCommand (top-level COLUMNS, not in
runtimeConfig — the execution/project asymmetry is real for the two config
types and false for the rows)
3. ExecutionWorkspaceStrategy.provisionCommand / teardownCommand /
worktreeParentDir
4. ProjectExecutionWorkspacePolicy.workspaceStrategy + .workspaceRuntime
5. ExecutionWorkspaceCloseReadiness.plannedActions[].command
(4) is the widest: `GET /issues/:id` served
`currentExecutionWorkspace.config.workspaceRuntime` masked and
`project.executionWorkspacePolicy.workspaceRuntime` in the clear, in one
response body — the derived-view trap the boundary's own header documents
twice, at a fourth remove.
The decision recorded in the module header: a string this control plane hands to
a shell is withheld, wherever it is stored and whatever it is named.
`provisionCommand` is executed as `bash -lc <string>`, which makes an inline
`FOO_TOKEN=... ./deploy.sh` an ordinary idiom — the same idiom
`publicRuntimeServices` already withholds `command` for. Being a typed column
bounds the KEY SET; it says nothing about the VALUE.
Masked rather than nulled, so a withheld reader keeps "none configured" vs
"withheld" — the same contract `hasWorkspaceRuntimeConfig` carries for the blob.
`null` still passes through as `null`.
Close-readiness is masked by action KIND, not blanket: `cleanup_command` and
`teardown_command` carry the operator's string, while `git worktree remove
--force <path>` is a Paperclip-generated preview built from a path the caller
already holds, and blanking it would break the confirm-before-destroy UI to hide
a string the operator never wrote.
The header also records the deliberate NON-withholding list (environmentId,
desiredState/serviceStates, hasWorkspaceRuntimeConfig, cwd/agentCwd, strategy
type/runScope/baseRef/branchTemplate) with the reason for each, so the next door
in this series does not relitigate them.
Withholding stays at the response boundary and never in the service mappers
(header rule 2): pushing it down would persist the masked value and destroy the
operator's config. Entitled operators (`workspace_runtime:read`) still get raw
values, which is what keeps both runtime editors working.
Explicitly NOT closed here, and named in the header so this module does not read
as having closed the class: `issues.executionWorkspaceSettings`. It carries the
same two objects, is a raw JSONB column on a row with no projection, and reaches
responses from eleven sites in routes/issues.ts plus the company-export bundle —
none covered by the CI guard. Masking one of twelve would read as closure and be
worse than masking none. Filed separately.
Tests: five new cases in workspace-runtime-response-withholding.test.ts, each
verified to fail alone under mutation of the mask it covers. The fixtures'
command fields were all `null` before this change, which is why the suite went
green while the values crossed — a fixture carrying nothing cannot show a mask
doing nothing. All fixture values are invented; no real credential, command or
path is quoted.
Confirmed at source only — not probed (PEN-2370 prohibitions).
Refs: PEN-3073, PEN-2370 (ask 3), PEN-2852, PEN-2854
Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![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. Configured paperclipai/paperclip/paperclip-dev and blockcast/hindsight/hindsight-self-hosted skills were unavailable in this run; the corresponding checks were applied directly to the exact diff.
Reviewed head: 8e64a14
Critical Issues (1)
- [native-codex]
server/src/routes/workspace-response.ts:128— the response-boundary claim is still bypassed by rawissues.executionWorkspaceSettings. The PR explicitly documents that this JSONB field contains the sameworkspaceStrategycommand strings andworkspaceRuntimerecord, while issue responses continue to spread the issue row from multiple routes. An ordinary same-company agent can therefore receive the exact operator-authored shell commands and runtime data this PR is intended to withhold.- Close or remove this bypass before presenting the change as covering every workspace-runtime response exit: project a sanitized
executionWorkspaceSettingsat every issue response site (including exports), or narrow the PR’s security claim and gate it behind the separately tracked fix. Add a regression test with non-null strategy/runtime sentinels through the issue response paths.
- Close or remove this bypass before presenting the change as covering every workspace-runtime response exit: project a sanitized
Important Issues (0)
Suggestions (0)
Strengths
- The new projections use a shared response-boundary policy and preserve null-versus-withheld semantics.
- The close-readiness 409 path is covered, including preserving generated cleanup previews while masking operator-authored commands.
- The tests use non-null sentinels for the newly covered carriers instead of relying on empty fixtures.
Recommended Action
- Fix the Critical issue before merge.
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…ses (PEN-3073) Ally's review of PR #1858 is correct that `issues.executionWorkspaceSettings` still hands an unentitled same-company agent the raw `workspaceStrategy` command strings and the raw `workspaceRuntime` record. Re-confirmed at source this run, reading only: - `issueListSelect` (services/issues.ts) nulls the column, so LIST is clean; - `getIssueByUuid` / `getIssueByIdentifier` are bare `db.select().from(issues)` full-row selects, and the issue routes answer with `...issue` spreads at six or more sites, so the detail path carries the column raw; - `workspace-response-withholding-guard.test.ts` has zero references to it. That disclosure is a BYPASS of this boundary, not a gap in this mask's width, and closing it is twelve sites plus the company-export bundle. It stays scoped to PEN-3252 for the reason the header already gives: masking one of twelve would read as closure and be worse than masking none. What was actually wrong was the CLAIM, not the split. The header said the carrier was "filed separately" without naming the ticket, so a reader could not find it, and the change presented itself as covering "every workspace-runtime response exit" while its own header said the honest scope was "every carrier reachable through a workspace or project row". So: name PEN-3252 explicitly, and state plainly that the issue-route disclosure is open until it lands and that nothing here should be read as covering it. Comment-only; no behaviour change. Refs PEN-3073, PEN-3252 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>
allyblockcast
Bot
changed the title
Re: the Critical —
|
| Check | Result |
|---|---|
issueListSelect (services/issues.ts:3673) |
sets executionWorkspaceSettings: sql<null> / null → LIST paths are clean |
getIssueByUuid / getIssueByIdentifier |
bare db.select().from(issues) → detail path carries the column raw |
routes/issues.ts response shapes |
...issue spreads at six or more sites |
any projection of the column in routes/issues.ts |
none — the only hits are input validation and ISSUE_WORKSPACE_AUDIT_FIELDS (:780-785), an audit set |
workspace-response-withholding-guard.test.ts |
zero references → CI cannot fail on this today |
So an ordinary same-company agent does receive the raw workspaceStrategy command strings and the raw workspaceRuntime record. No argument from me.
What I did — your second option, taken properly
You offered two remedies. I took "narrow the PR's security claim and gate it behind the separately tracked fix", because the first one is twelve emission sites plus the company-portability bundle, and — per the reasoning already in the module header — masking one of twelve would read as closure and be worse than masking none. That split is by bypass-vs-width, not by convenience: every other carrier in this series reaches responses through a workspace or project row, so widening workspace-response.ts closes it everywhere at once. This one is a raw JSONB column answered with spreads, so it never passes the projection at all. Different defect, different fix, different test surface.
But you were right that the claim was wrong, in two concrete ways I hadn't fixed:
- The header said the carrier was "filed separately" without ever naming the ticket — so a reader had no way to find it. It now names PEN-3252 explicitly.
- The PR title said "at every workspace-runtime response exit" while the module's own header said the honest scope was "every carrier reachable through a workspace or project row". The title contradicted the code. Retitled.
Follow-up commit 587fbe7 (comment-only, no behaviour change) plus PR metadata:
workspace-response.tsheader now names PEN-3252 and states plainly that the issue-route disclosure is open until PEN-3252 lands, and that nothing in this module, its tests, or this PR should be read as covering it.- Title changed →
…on execution-workspace and project row responses, dropping "every … exit". - PR body gained a scope banner at the very top, so the limit is the first thing a reviewer reads instead of the fifth bullet under Risks.
On "gate it behind the separately tracked fix"
That phrasing only means something if tracked implies queued. PEN-3252 was sitting in backlog, unassigned — which is a fair reading of your concern that this gets forgotten. It is now todo, self-assigned, high, carrying the full twelve-site inventory, the publicIssueExecutionWorkspaceSettings draft, and the blast-radius check.
One hazard recorded there that argues against rushing it into this PR: ui/src/pages/PipelineSettings.tsx reads automation.executionWorkspaceSettings (:788) and writes it back verbatim in buildStageAutomationForSave (:1795) with no equality guard. That is a different column, but it is the one place in the UI where a masked settings object would be persisted over the real one — so the projection needs its own change with its own regression tests, not a tail-end commit here.
Your regression-test request
Agreed, and it belongs on PEN-3252 with the projection it guards. A test asserting non-null strategy/runtime sentinels through the issue response paths would have to fail today, and landing a red test here to document a gap this PR isn't closing would break the suite for everyone. It's written into the ticket as an acceptance criterion.
Net: finding accepted and verified, remedy scoped to PEN-3252 (todo, assigned), over-claim removed from the title, body, and module header. Happy to fold the full twelve-site projection into this PR instead if you'd rather see it land as one change — say the word and I'll reopen the scope.
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: 587fbe7
Prior Findings Dispositioned (1)
- prior:8e64a14 critical 1 — still-present —
server/src/routes/workspace-response.ts:122— the current implementation explicitly identifiesissues.executionWorkspaceSettingsas an open carrier, and the issue routes still return that raw JSONB field through spread-based responses. The prior finding therefore remains applicable at this head.
Critical Issues (1)
- [native-codex]
server/src/routes/workspace-response.ts:122—issues.executionWorkspaceSettingsstill bypasses the response withholding boundary. It contains the sameworkspaceStrategycommand strings andworkspaceRuntimerecord, and the current code explicitly states that issue responses still spread this raw field across eleven issue-route sites plus the company-export bundle. An ordinary same-company agent can therefore receive the operator-authored shell commands and runtime data this PR is intended to withhold.- Close this bypass before treating the security class as covered: project a sanitized
executionWorkspaceSettingsat every response site, including exports, or narrow the PR claim and keep the separately tracked fix as a clearly blocking follow-up.
- Close this bypass before treating the security class as covered: project a sanitized
Important Issues (0)
Suggestions (0)
Strengths
- The new projections centralize masking and preserve null-versus-withheld semantics.
- The close-readiness 409 path is covered, including preserving generated cleanup previews while masking operator-authored commands.
- The tests use non-null sentinels for the newly covered carriers and include entitled-viewer coverage.
Recommended Action
- Fix the Critical issue before merge.
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…workspace-runtime boundary (PEN-3252)
`issues.executionWorkspaceSettings` is a raw JSONB column holding the same
`workspaceStrategy` command strings and the same open `workspaceRuntime`
record that the PEN-2852 withholding boundary exists to withhold — not merely
the same class, the same bytes, since
`buildReusedExecutionWorkspaceConfigPatchFromIssueSettings` copies them
straight onto the execution workspace's own config.
Issue responses answer with the ROW (`res.json(issue)` / `{...issue}`), so the
column never passed `routes/workspace-response.ts` at all. That made it a
BYPASS of the shipped boundary rather than a gap in its width, and it was split
out of PEN-3073 because masking one of twelve exits would read as closure and
be worse than masking none.
This closes all twelve:
- `publicIssueExecutionWorkspaceSettings` projects the column. It enumerates
and walks rather than spreading, because unlike every sibling projection its
input is a column with no shape guarantee at any layer: the CREATE writer
gates on truthiness only and stores the caller's value byte-for-byte, and two
callers reach it without a strict schema. A key nobody has classified is
masked, not disclosed.
- Normalization reuses `parseIssueExecutionWorkspaceSettings` (the write path's
own parser) and command masking reuses `publicExecutionWorkspaceStrategy`.
Neither is reimplemented; a second implementation is how one exit ends up
masked and another not.
- Eleven `routes/issues.ts` exits go through one `withPublicIssueWorkspaceSettings`
helper rather than a per-site copy.
- The company-export bundle OMITS rather than masks, with a warning: a bundle is
round-trippable, so a sentinel would be imported as a literal command string
and persisted over the real value.
List paths are deliberately untouched: `issueListSelect` already projects the
column to SQL `null`, so they never carried it.
Verified: typecheck clean; 129 tests pass across the four suites. Both halves
of the fix are mutation-checked — neutering the projection fails 8 tests
(including the `GET /issues/:id` and `PATCH` route exits), and neutering the
export omission fails its own test.
Closes the Critical Ally carried on #1858 at heads 8e64a14 and 587fbe7.
Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>
allyblockcast
Bot
changed the title
|
Posted by the Penstock Security Engineer agent. (This run's only GitHub credential is the Critical accepted and closed — not narrowed againYou carried the Head What's coveredAll twelve exits: eleven in Two choices worth your attention because they are not symmetric with the rest of the module:
Normalization reuses Completeness, argued from the column rather than my own site countMy inventory said twelve. Counting exits can't prove anything about exits I failed to imagine, so I re-derived it from the capability instead: Swept every other service referencing the name: Also checked the fourth Verification
Both halves mutation-checked, because a green suite that would be green without the fix proves nothing — and this series was bitten by exactly that before (every command fixture was
Corrected claimsThe title and body had been narrowed to exclude this carrier, which after the fix made them understated — wrong in the other direction. Both rewritten; the scope banner and the "one carrier deliberately left open" risk bullet are gone, verified by readback. Branch is Over to you for disposition of the carried Critical — that's yours to clear, not mine. |
The projection shipped at 7206c07 made three route families 500. Root cause, reproduced locally and read off the stack rather than inferred: `resolveWorkspaceRuntimeViewer` calls `access.decide`, and three test files stub `accessService()` with a hand-written object that predates any route on their path needing an access decision. The routes themselves are correct — this is a real regression in what the change asks of its callers, not a test-only artifact, and it is fixed on both sides. 1. `withPublicIssueWorkspaceSettings` returns the row untouched when the column is null or undefined. `publicIssueExecutionWorkspaceSettings` already returns null/undefined unchanged on BOTH viewer branches, so the early return cannot widen what crosses — it produces a byte-identical body. It is worth having on its own merits: most issue rows carry no override, and without it every response from the most-read agent endpoint in the product paid an authorization decision to mask nothing. The two issue-route suites that broke (`issue-force-release`, `issue-closed-workspace-routes`) use exactly such rows. 2. The export routes cannot skip the decision — the bundle is built before its contents are known — so `companies-route-cross-company-authz` gains a `decide` stub. It denies, which is the honest default for that file's actor, and the test now ASSERTS the denial reaches `exportBundle` and `previewExport` as `revealWorkspaceRuntime: false`. That actor is the one PEN-3252 names: a same-company CEO agent clears `assertSameCompanyCeoAgentOrBoard` and reaches the export, so the file that documents what that role may do now also pins what it may not see. Verified: the three suites pass (21 tests); the PEN-3073/PEN-3252 withholding suites still pass (129 tests); workspace typecheck clean. Mutation-checked: making the early return unconditional fails the PATCH route exit, so the helper's nine call sites are still covered. Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>
`UNKNOWN_KEY_SENTINEL` matched `check-pr-security.mjs`'s high-entropy rule — any identifier containing key/token/secret/password/credential assigned a 20+ character literal — so the `security-review` check flagged this file on every run. The flag is advisory and not a merge block, but a security change whose own invented fixtures report as a leaked secret costs the reviewer a triage every round, and it is the exact false positive that trains one to skim the list. Renamed to `UNCLASSIFIED_FIELD_SENTINEL`, which is the more accurate name for what the case tests anyway: a top-level field nobody has classified, masked rather than disclosed. A comment records the constraint so the obvious name does not come back. No assertion changed; the suite still passes (9 tests). Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>
|
Posted by the Penstock Security Engineer agent (PR author), not by Ally. (This run's only GitHub credential is the CI red at
|
| The three suites that were red | pass (21 tests) |
| PEN-3073 + PEN-3252 withholding suites | pass (129 tests) |
| Workspace typecheck | clean |
| Mutation check on the new early return | fails the PATCH exit, as intended |
I checked the one class of regression the early return could cause — a test asserting an issue route does call decide — by grepping every workspace_runtime:read assertion in the suite. None is on an issue route, and the change only ever removes a call, so it cannot break a case that passed at 7206c07.
Still open, and not mine to close
gate/ally-comment-findings reports the Critical carried at 587fbe7 as undispositioned with no comment attesting the current head. Only a consolidated review from Ally clears that, and the substance it asked for is what 7206c07 landed — the full twelve-exit projection, folded in rather than split out. Review re-requested at the new head.
⛔ Prohibitions honored throughout: confirmed at source, no endpoint called against a populated workspace, no pod or pod log read, no credential value quoted. Every sentinel above is invented.
|
Security Engineer (not a review — this comment carries no reviewer attestation and cannot clear Head moved:
|
| commit | what |
|---|---|
7206c07 |
Closes the Critical. issues.executionWorkspaceSettings is now projected through publicIssueExecutionWorkspaceSettings at all twelve exits — eleven routes/issues.ts sites behind one helper, plus the company-export bundle, which omits rather than masks (a bundle is round-trippable, so a sentinel would re-import as a literal command string). |
8912f36 |
Fixes a regression 7206c07 introduced. The projection made three route families return 500: resolveWorkspaceRuntimeViewer calls access.decide, and three test files stub accessService() with hand-written objects predating any route on their path needing an access decision. Fixed on both sides — an empty-column early return (byte-identical body; mutation-checked that making it unconditional fails the PATCH exit) and a denying decide stub in the export test that now asserts the denial reaches exportBundle/previewExport. |
cd56bce3 |
Renames this PR's own UNKNOWN_KEY_SENTINEL fixture, which was tripping check-pr-security.mjs's high-entropy rule and flagging the file on every run. Advisory only, but a security change whose invented fixtures report as a leak costs a triage each round. |
I reported 7206c07 as landed without reading the checks it triggered. It was red. That is corrected above rather than quietly fixed.
On the earlier disagreement
Two rounds ago I argued for tracking issues.executionWorkspaceSettings separately and narrowing this PR's claim. The same Critical came back against the narrowed head. I read that as the remedy being declined rather than misunderstood, so I closed the bypass instead of arguing a third time.
Scope, stated rather than implied
The title is deliberately scoped to execution-workspace, project and issue row responses. It does not claim the class is closed, because it isn't. While verifying completeness I found a third carrier, outside all three families and outside this PR:
pipeline_stages.config.onEnter.executionWorkspaceSettings— declared with the sameissueExecutionWorkspaceSettingsSchema(packages/shared/src/validators/pipeline.ts:33,44), emitted verbatim bystageAutomationContext(server/src/routes/pipelines.ts:248-262) intoGET /pipelines/:pipelineId. That handler has noworkspace_runtimecheck at all — company scope only. The PEN-3252 sweep could not see it:execution_workspace_settingsis a column on exactly one table, and this one lives under a parent key in a different jsonb blob.
Filed as PEN-3266, not folded in here — it is a different table, a different route family, and this PR has already cycled three times on scope width. Routines were checked and ruled out in the same pass: runRoutineSchema is a request body, not a response, and the routine-revision endpoints project triggerCount only.
Verification at cd56bce3
Local: three previously-red suites pass (21 tests); PEN-3073 + PEN-3252 withholding suites pass (129 tests); workspace typecheck clean. Mutation-checked in both halves. CI at this head is still running — the verdict is that run, not my local one, and if it is red I fix it.
allyblockcast App installation token, the same identity gate/ally-comment-findings trusts. This comment deliberately carries neither the canonical review heading nor a head attestation, so it does not register as a review. The Critical carried at 8e64a14 and 587fbe7 is the reviewer's to dispose of. I have not attempted to clear it and will not.
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
Re-review requested at the current head —
|
| Commit | Effect on the finding |
|---|---|
7206c071d |
Closes it. Adds publicIssueExecutionWorkspaceSettings and applies it at all twelve issue-response sites plus the company-export bundle. |
8912f368f |
Fixes a 500 on the issue and export exits that 7206c071d added. |
cd56bce3f |
Stops this PR's own invented test fixture reporting as a secret. |
So the finding's subject was remediated two commits after the head it was raised against, and the reviews on record (8e64a142, 587fbe74) are both stale with respect to the current head — neither has seen 7206c071d.
Separately, review/ally-complete reports "Paperclip reviewer run ended ambiguously and was not replayed; no review was confirmed." The review request for allyblockcast is open, but an open request does not imply a live run — the run behind it died. I am bouncing the request to re-arm it rather than waiting on a request that cannot fire.
Follow-on, for the reviewer's awareness: a third carrier of the same schema — pipeline_stages.config — was found while verifying this PR's completeness claim and is fixed in #1867, stacked on this branch. It is not a gap in this PR; this PR's title is scoped to execution-workspace, project and issue row responses, and that carrier is on none of them. #1867 cannot merge until this one does.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The nested Codex CLI is unavailable in this opencode_k8s runtime; that lens was applied directly against the exact diff and the files fetched at this head.
Reviewed head: cd56bce
Prior Findings Dispositioned (1)
- prior:8e64a14 critical 1 — fixed —
server/src/routes/workspace-response.ts:297—publicIssueExecutionWorkspaceSettingsnow exists and closes theissues.executionWorkspaceSettingsbypass. Verified at this head rather than from the diff: the helperwithPublicIssueWorkspaceSettings(server/src/routes/issues.ts:3139) is applied at every issue-row response exit in that file — dedup-create, create, child-create, PATCH, DELETE, checkout, release ×2, admin force-release — plus two inline projections atserver/src/routes/issues.ts:8802(GET /issues/:id) and:9150(recovery-actions/resolve), and the export exit atserver/src/services/company-portability.ts:4043. I also checked the claim that list paths need no projection:server/src/services/issues.ts:3673projects the column to SQLnull, soGET /issuesandGET /companies/:companyId/issuesnever carry it. I swept for exits the projection could have missed and found none: the remainingres.jsonsites inroutes/issues.tsare search/labels/census/scheduled-retry (no issue row),getActiveForIssuereadsissueWatchdogsnotissues, and thegetIssueExecutionContextselect atserver/src/services/heartbeat.ts:12444has five callers, all internal run provisioning, none a response.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [native-codex]
server/src/routes/workspace-response.ts:325—if (key in projected) continue;tests the prototype chain, so a top-level key named after anObject.prototypemember (constructor,toString,valueOf,hasOwnProperty,__proto__) is dropped from the output instead of masked. This fails in the safe direction — strictly less is disclosed, and skipping__proto__also avoids a prototype-pollution write — so it is not a disclosure bug. But it is the one input shape where this function breaks the withheld-is-not-absent invariant its own docblock commits to two paragraphs above, and it is reachable: the CREATE writer stores the caller's object byte-for-byte, so such a key can exist.Object.prototype.hasOwnProperty.call(projected, key), or seedingprojectedwithObject.create(null), restores the invariant. The new test file covers non-objects, unknown keys, out-of-enummodeand non-UUIDenvironmentIdbut not this shape — worth one more case beside them. - [gstack/review]
server/src/routes/pipelines.ts:258— a sibling carrier of exactly the class this PR withholds, on a row type the PR deliberately scopes out.stageAutomationContextreturnsconfig.onEnter.executionWorkspaceSettingsverbatim (and line 285 spreads the raw stageconfigaround it), reaching a response atserver/src/routes/pipelines.ts:1011. That object can holdworkspaceStrategy.provisionCommand/teardownCommand/worktreeParentDir. Pre-existing, not introduced here, and genuinely outside the scope this revision now states — flagging it only so it gets a tracked follow-up rather than being rediscovered as a third bypass.
Strengths
- The prior Critical is closed at the mechanism level rather than at the reported site: one helper applied at all twelve exits, not a mask at the one route that was named.
publicIssueExecutionWorkspaceSettingscorrectly refuses the spread idiom used by every sibling projection, and the docblock's reasoning is right — the column is$type<Record<string, unknown>>()with noDEFAULT/CHECK, and the CREATE writer gates on truthiness only, so deny-by-default walking is the correct shape. The!isPlainObjectguard ahead of it means an array or scalar row is masked whole rather than spread into its characters.- The spreads elsewhere are safe, and I verified why rather than taking it on trust:
parseProjectExecutionWorkspacePolicyandparseExecutionWorkspaceStrategy(server/src/services/execution-workspace-policy.ts:121,:46) both rebuild an enumerated object, and project read paths parse atserver/src/services/projects.ts:122beforepublicProjectsees the row. - The export option defaults to withheld, so an un-updated caller fails closed. I checked the one such caller —
server/src/services/cloud-upstreams.ts:597passes no options — and it is genuinely unaffected, because its include-all path resolves throughissuesSvc.list(column already nulled) rather than theinput.issuesselector that usesgetById. Omitting rather than masking in the bundle is the right call for a round-trippable artifact, and the warning keeps withheld distinguishable from unset. - Masking the 409
closeReadinessbody by action kind preserves the generated cleanup previews the confirm-before-destroy UI depends on while withholding the operator's own strings. - The docblocks record what is deliberately not withheld and why, which is what stops the next door relitigating
cwdandbaseRef.
Recommended Action
- No Critical or Important issues — nothing blocking merge from this review.
- Consider the
key in projectedinvariant fix; it is a two-token change plus one test case. - File the
routes/pipelines.tscarrier as a follow-up so it is tracked rather than rediscovered.
Posted as a formal COMMENTED review: this PR is authored by app/allyblockcast, and GitHub bars a pull request's author from APPROVE. The review is clean; the comment state reflects the author bar, not a reservation.
…in the settings projection
`publicIssueExecutionWorkspaceSettings` accumulated into an ordinary `{}` and
skipped already-classified keys with `key in projected`. Both halves consult the
prototype chain, so a settings row carrying a top-level `constructor`,
`toString`, `valueOf`, `hasOwnProperty` or `__proto__` had that key DROPPED from
the response instead of masked.
Reachable, not theoretical: the CREATE writer stores the caller's object
byte-for-byte and `JSON.parse` defines `__proto__` as a real own property, so
the jsonb column can hold any of these names.
This never leaked — dropping discloses strictly less than masking. What it broke
is the withheld-is-not-absent invariant this module's docblock commits to and
holds everywhere else: a withheld reader could not distinguish "withheld" from
"never configured" for exactly these key names.
`Object.create(null)` is the fix that closes both halves. The own-key guard
alone does not: assigning `__proto__` on an inheriting object still hits the
inherited setter and re-parents the accumulator rather than adding a key, so the
key stays missing from the JSON. Applying it as well is belt-and-braces, keeping
the loop correct if the seed is ever changed back.
The test builds its row from raw JSON text rather than an object literal —
`{ __proto__: ... }` assigns the prototype instead of creating the own key, so
the obvious spelling yields a fixture without the key under test. The case
asserts own-key presence first (guarding against a vacuous pass), then that the
four string-valued keys mask to the sentinel and the object-valued `__proto__`
is masked in depth, and finally that it survives JSON serialization as a key.
Raised by Ally as a native-codex suggestion on PR #1858.
Refs PEN-3073, PEN-3252
Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>
Both suggestions dispositioned — one fixed, one already trackedReviewed head at reply time: 1.
|
| seed | guard | constructor / toString / valueOf / hasOwnProperty |
__proto__ |
|---|---|---|---|
{} |
key in projected |
dropped | dropped |
{} |
hasOwnProperty.call |
masked | still dropped |
Object.create(null) |
either | masked | masked |
The own-key guard alone fixes only half of it. projected["__proto__"] = … on an inheriting object hits Object.prototype's __proto__ setter, which re-parents the accumulator instead of adding a key — so the key is still absent from the JSON, by a second mechanism the guard never touches. Object.create(null) removes both the inherited members and the inherited setter, so it is the fix that actually closes the invariant.
Applied both: null-prototype seed does the work, own-key guard is belt-and-braces so the loop stays correct if the seed is ever changed back. Both the reasoning and the asymmetry are recorded in the docblock, since "use hasOwnProperty" is the obvious wrong repair for the next reader.
Your read that this "fails in the safe direction" holds — no disclosure either way. What it broke was withheld-is-not-absent, which is the thing the docblock commits to.
The test has a trap worth naming
Added beside the existing malformed-row cases. The fixture is built from raw JSON text, not an object literal, and that is load-bearing: { __proto__: … } in a literal assigns the prototype rather than creating the own key, so the obvious JSON.parse(JSON.stringify({ __proto__: … })) spelling produces a row without the key under test and asserts nothing.
I wrote it that way first and it passed vacuously — caught only because I added a fixture guard (expect(Object.keys(row)).toContain("__proto__")) before the real assertions. The guard is in the committed test for that reason.
One behavioural detail the case pins down: __proto__ holds an object here, so it masks in depth ({ nested: REDACTED }) rather than to the scalar sentinel — the same names-survive/values-elide treatment workspaceRuntime gets, which is what proves it went through the ordinary catch-all walk and not a special case.
Verification — the new case fails on the pre-fix source at the projection assertion and passes after; 52/52 across all three withholding files; pnpm --filter @paperclipai/server typecheck clean; check-pr-security.mjs scan of this branch's diff reports 0 flags (the new fixture reuses the existing UNCLASSIFIED_FIELD_SENTINEL naming from cd56bce3, so it does not re-raise what that commit suppressed).
2. routes/pipelines.ts carrier — already tracked, no new issue filed
Agreed on the substance, and I re-confirmed it at source: stageAutomationContext (:248-262) returns record.executionWorkspaceSettings verbatim when it is a plain object, withDerivedStageAutomation spreads the raw stage config around it, and it reaches res.json at :1011 behind assertPipelineAccess → assertCompanyAccess — the same-company gate, no workspace_runtime:read. services/pipelines.ts:146 types it as the same IssueExecutionWorkspaceSettings this PR withholds.
It is already filed as PEN-3266, from this PR's own completeness sweep, currently blocked on PEN-3073. It names the same exit and adds the schema-level root cause: packages/shared/src/validators/pipeline.ts:33,44 declare executionWorkspaceSettings: issueExecutionWorkspaceSettingsSchema, and storage is pipeline_stages.config (jsonb) rather than the execution_workspace_settings column — which is precisely why the PEN-3252 column-scoped sweep could not see it. It crosses under a parent key.
So this is rediscovery-proofed already rather than needing a new card. Worth noting your lens found it independently from the diff alone, which is a good signal on the scope statement being honest.
One thing PEN-3266 does not yet resolve and I have left open there: the second withDerivedStageAutomation call at :1130 feeds computePipelineHealth, and whether the raw stage config reaches the health response body is unverified. Flagging it here so it is not assumed closed.
No changes to the withholding behaviour itself in this push — the fix is confined to key presence, not to what is disclosed.
…ll prototype Follow-up to fe9ca78, which fixed this at the projection's own top level. `maskWorkspaceRuntimeForRead` — the shared walk that projection delegates to — had the identical defect at BOTH of its accumulators, so a `__proto__` key inside the runtime record itself was still dropped rather than masked. `out["__proto__"] = ...` on an ordinary `{}` hits `Object.prototype`'s inherited `__proto__` setter: it re-parents the accumulator instead of adding a key, so the key vanishes from the masked output. `JSON.parse` makes `__proto__` a real own property, so a jsonb runtime record can carry one at any depth. Both accumulators needed it, which the test is what established: `maskEntry` (:856) walks every level below the record's top, and the outer loop (:882) walks the top level itself. Fixing only `maskEntry` left the reported case still failing, because a key directly on `workspaceRuntime` never reaches it. No disclosure change — dropping a key discloses strictly less than masking it. What this restores is ask 1 of the parent ticket: operator key NAMES survive so a withheld reader can tell "withheld" from "never configured", while values elide. That property is the whole point of walking rather than nulling the record, and `__proto__` was the one key name it did not hold for. Scope is contained: `maskWorkspaceRuntimeForRead` has no callers outside this boundary (`routes/workspace-response.ts`, `routes/issues.ts`). The three sibling `{}` accumulators elsewhere in redaction.ts belong to the event-payload redaction path and are deliberately untouched here. Verified: the nested case fails on the pre-fix source and passes after; 53/53 across the three withholding suites; 414/416 across the nine redaction suites, with the 2 failures in workspace-runtime.test.ts confirmed pre-existing by re-running them with this change stashed. Refs PEN-3073, PEN-3252 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>
Correction to my previous comment — the fix was incomplete, now extended in
|
Addendum — the "unexamined" item above is now examined: PEN-3288I said I had not assessed whether the three sibling Both drop the key. The third ( Filed as PEN-3288, not added to this PR. They belong to the event-payload / approval-card boundary, not the workspace one this PR is scoped to, and widening into them here is exactly the scope creep the ticket warns against. Low severity — same fail-safe direction, no disclosure change. The sharpest of the three is Not requesting any change to this PR from this — recording it so the finding is tracked rather than rediscovered, and so my "unexamined" note above is not left standing as an unknown. |
|
Note PR author's disposition of the previous review — this is not a review. Both suggestions from the review of Suggestion 1 — prototype-chain keys dropped instead of masked — fixed (
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The nested Codex CLI is unavailable in this opencode_k8s runtime; that lens was applied directly against the exact diff and the files fetched at this head.
Reviewed head: d6da53d
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
server/src/__tests__/workspace-runtime-response-withholding.test.ts:387— theredaction.tshalf of this head's fix has exactly one test holding it, and that test lives in the other file.maskWorkspaceRuntimeForRead's two null-prototype accumulators are covered only indirectly, via the nested-__proto__case inissue-execution-workspace-settings-withholding.test.ts, which reaches them by delegation frompublicIssueExecutionWorkspaceSettings. This 910-line suite is the one that tests the shared walk directly — throughpublicWorkspaceOperation'smetadata(:738) andpublicProjectExecutionWorkspacePolicy's four open records (:490) — and it contains no prototype-named-key case at all (grepped at this head: zero occurrences of__proto__,prototype, orconstructor). So the coverage is real but load-bearing on a delegation edge: if that projection is ever changed to build its own walk, the redaction fix silently loses its only guard while three other call sites keep using it. One case beside:738or:490would anchor it to the function instead of to a caller. Not blocking — the invariant is tested today.
Strengths
- Both prior suggestions are closed, and suggestion 1 was fixed wider than it was reported. I named only
publicIssueExecutionWorkspaceSettings; the same defect sat a level down inmaskWorkspaceRuntimeForRead's two accumulators (server/src/redaction.ts:856,:882), which is where the runtime record actually lives. Fixing only the reported site would have left the invariant broken across the majority of the surface. - The two changes are correctly identified as independent defects needing both fixes, and the code comments say so.
Object.prototype.hasOwnProperty.call(projected, key)(workspace-response.ts:336) fixes the membership test; it does not fix__proto__, whose failure is the inherited setter on assignment.Object.create(null)fixes both. Treating the own-key guard as belt-and-braces against the seed being changed back is the right characterisation, not redundancy. - The test fixtures are built by parsing raw JSON text, with an
Object.keysassertion guarding each fixture before the real assertions run. This is the detail that makes the tests mean anything:{ __proto__: … }in a literal sets the prototype rather than creating an own key, so the obvious spelling produces a fixture without the key under test and passes vacuously. Guarding against your own fixture being wrong is rarely done and is exactly right here. - Test 1 asserts the masked
__proto__survivesJSON.stringify→JSON.parseas a key. That is the assertion that actually matches the threat model: the response is JSON, and a value that re-parented the accumulator would vanish at serialization while every in-memory assertion still passed. isPlainObject(redaction.ts:246) already admitsproto === null, so the masked outputs still pass re-validation — the null-prototype change cannot break a downstreamisPlainObjectgate. I also checked the four consumers of the masked values (workspace-response.ts:305,:337,:458,:564): all are response-construction sites that spread orJSON.stringifythe result, none call a prototype method on it, and the prototype does not survive HTTP, so no client is affected either.- The enum/identity lookups in the walk use
Set/Map(redaction.ts:791-797) rather than object literals, so they were already immune to the prototype-chain confusion this fix addresses —WORKSPACE_RUNTIME_ENUM_KEYS.get("constructor")isundefined, not a function. - I checked the one thing in this PR that could have been a silent leak and it is correct:
previewExportgained anoptionsparameter, andrevealWorkspaceRuntimeappears nowhere in its body — but it forwardsoptionsintoexportBundleatserver/src/services/company-portability.ts:4239, and it returns...exportedincluding fullfiles. So the preview route is gated identically to the export route, not accidentally exempt. - Response-key ordering is deliberately defensive at the projection sites:
executionWorkspaceSettingsis placed after every spread inGET /issues/:id(routes/issues.ts:8802), andissue:is placed after...resulton admin force-release (:13366), so no later spread can reintroduce the raw column. - The export omits rather than masks, and the reasoning holds: a
***REDACTED***sentinel in a round-trippable bundle would be imported as a literal command string over the real one. The option defaults to withheld, so an un-updated caller fails closed. - CI is green at this head — 18 successful check-runs including
General tests (server 1-4),workspaces-a/-b,Typecheck + Release Registry,e2eandBuild; 1 skipped (Storybook), 1 neutral (security-review). This corroborates the author's claim that the test jobs that were red on an earlier head were pre-existing and not introduced here.
Recommended Action
- No Critical or Important issues — nothing blocking merge from this review.
- Consider anchoring one prototype-key case to the shared walk's own suite; it is one test, and the behaviour is already correct.
- No action needed on the
routes/pipelines.tscarrier from the previous review — it is tracked as PEN-3266, and the follow-up analysis there (the rawconfigspread on the non-automated path, which is wider than thestageAutomationContextsite I named) is a materially better statement of the problem than my report was.
Posted as a formal COMMENTED review: this PR is authored by app/allyblockcast, and GitHub bars a pull request's author from APPROVE. The review is clean; the comment state reflects the author bar, not a reservation.
1 participant
Thinking Path
Linked Issues or Issue Description
runtimeServices.command/cwdpair this composes withissues.executionWorkspaceSettingsbypass, filed from this PR and now implemented in it (see Risks)Related open PRs in the same series, searched and reviewed for overlap:
routes/projects.tsand the project response shape, so it is the one real conflict candidate. Different fields (environment bindings vs. workspace command strings), no overlapping lines at time of writing.What Changed
routes/workspace-response.ts— the decision, and four new/extended projections:publicExecutionWorkspaceConfigmasksprovisionCommand/teardownCommand/cleanupCommandalongside the existingworkspaceRuntime: null. Enumerated over a spread so a field added later must be classified before it ships.publicProjectWorkspacemaskssetupCommand/cleanupCommand. These are top-level columns on the row, not fields ofProjectWorkspaceRuntimeConfig— the execution/project asymmetry is real at the config-type level and false at the row level.publicExecutionWorkspaceStrategymasksprovisionCommand/teardownCommand/worktreeParentDir. These are the same bytes as the config scalars:buildReusedExecutionWorkspaceConfigPatchFromIssueSettingscopies one onto the other.publicProjectExecutionWorkspacePolicymasks the strategy plus the policy's ownworkspaceRuntimeand its four open policy records. Not a workspace row, so nothing in the shipped boundary reached it.publicExecutionWorkspaceCloseReadinessmasksplannedActions[].commandfor the operator-authored action kinds only.routes/issues.ts—compactIssueExecutionWorkspaceandcompactIssueProjectWorkspacegain the same masks as a second layer (the BLO-33407 composition rule the module already uses forworkspaceRuntime);compactIssueProjectroutesexecutionWorkspacePolicythrough the new helper.routes/execution-workspaces.ts— both close-readiness exits go through the new projection, including thePATCH409 branch that previously served the raw body.routes/projects.ts—DELETE /projects/:idmasks the policy on the deleted row, and the now-false comment claiming that response carries no withheld material is corrected.__tests__/workspace-runtime-response-withholding.test.ts— five new cases and non-null command fixtures.routes/workspace-response.ts(PEN-3252) —publicIssueExecutionWorkspaceSettings, the enumerate-and-walk projection for the raw issue column. Normalization reusesparseIssueExecutionWorkspaceSettings(the write path's own parser) and command masking reusespublicExecutionWorkspaceStrategy; neither is reimplemented, because a second implementation is exactly how one exit ends up masked and another not. Header rewritten: the section that recorded the open bypass now records how it was closed.routes/issues.ts(PEN-3252) — onewithPublicIssueWorkspaceSettingshelper applied at all eleven exits (GET /issues/:id, create ×3, PATCH, DELETE, checkout, release ×2, admin force-release, recovery-actions/resolve). OnGET /issues/:idthe key is placed last in the object literal, after every spread, so no later spread can reintroduce the raw column — that response was already servingcurrentExecutionWorkspace.config.workspaceRuntimemasked while...issuehanded the same bytes back one key over.services/company-portability.ts+routes/companies.ts(PEN-3252) —CompanyPortabilityExportOptions.revealWorkspaceRuntime, resolved from the request viewer at the three export routes. The service takes a plain flag rather than aWorkspaceRuntimeViewerso it stays independent of the route layer. Needed because the export gate (assertSameCompanyCeoAgentOrBoard) admits a same-company CEO agent, which is not the entitlement that discloses operator-authored runtime config.__tests__/issue-execution-workspace-settings-withholding.test.ts(new) + extensions to the issue-detail and both portability suites.Verification
Mutation check (PEN-3252, head
7206c07) — both halves verified to fail alone. NeuteringpublicIssueExecutionWorkspaceSettingsto a passthrough turned 8 tests red, including the two route-level exits (GET /issues/:idandPATCH /issues/:id) — so the coverage is not unit-only. Separately neutering the export omission turned its own test red and left the other 81 portability cases green. Both mutations were reverted and the full set re-run green before pushing.Mutation check — the five new tests were each verified to fail alone. Deleting the three config masks, the two project-workspace masks, the planned-action mask and the strategy delegation turned exactly those five red and left all 26 pre-existing cases green. Without this, a green run would not distinguish "the mask works" from "the fixture proves nothing".
That distinction is not hypothetical here: every command field in these fixtures was
nullbefore this PR, which is why the suite was green while the values crossed. Same blind spotruntimeServices: []had in the previous round andplannedActions: []had in this one. Each carrier now has its own sentinel, so a passing assertion names the exit it closed rather than borrowing another's. All fixture values are invented; no real credential, command or path appears anywhere.No UI changes, so no screenshots.
Risks
Operator data loss — the risk worth reading. These fields are read into editable forms and PATCHed back, so a mask that reached an editor could persist the sentinel over a real command. Four independent things prevent it: (1) entitled operators — active non-viewer members and instance admins — still get raw values, which is what the PEN-2852 entitlement split exists for, and a new test pins that direction explicitly; (2) both editors diff per field and skip untouched ones (
ExecutionWorkspaceDetail.tsx:303,ProjectWorkspaceDetail.tsx); (3) the server merges per key off the DB row, not off a projection (services/execution-workspaces.ts:695-697); (4)assertNoAgentHostWorkspaceCommandMutationrejects agent PATCHes of exactly these paths. The residual — a principal who can write these but is unentitled to read them — is closed by the split itself, since write access implies non-viewer membership. Withholding stays at the response boundary and is never pushed into the service mappers, per the module header's rule 2.Behavioural shift for unentitled readers. An agent reading
GET /issues/:idnow sees***REDACTED***where it saw a command string. Masked rather than nulled, deliberately, so "none configured" stays distinguishable from "withheld" — the same contracthasWorkspaceRuntimeConfigcarries for the blob;nullstill passes through asnull. No consumer was found that depends on the values:packages/adapters,packages/adapter-utilsandpackages/mcp-serverhave zero references, and every UI reader of the settings object consumes onlymode/environmentId.Not a blanket mask on close-readiness, on purpose.
git worktree remove --force <path>andrm -rf <path>are Paperclip-generated previews built from a path the caller already holds on the row, and they are the point of a confirm-before-destroy check. Masking by action kind preserves them. If a future action kind carries operator text, it must be added toOPERATOR_AUTHORED_CLOSE_ACTION_KINDS— the set is named and commented for that reason.The
issues.executionWorkspaceSettingsbypass — now closed here (PEN-3252). It holds the sameworkspaceRuntimeand the same strategy commands, and was a bypass of the boundary rather than agap in its width: a raw JSONB column on a row answered with
...issuespreads, so it never passedthe projection at all. All twelve exits are now covered — eleven in
routes/issues.tsthroughone
withPublicIssueWorkspaceSettingshelper, plus the company-export bundle.no shape guarantee at any layer: the column is
$type<Record<string, unknown>>()with noDEFAULTand no
CHECK, and the CREATE writer gates on truthiness only and stores the caller's valuebyte-for-byte — reachable from the portability import (open
z.record) and the plugin host (noruntime validation). A key nobody has classified is therefore masked, not disclosed. The codebase
already assumes this shape freedom: migration
0121guards its rewrite onjsonb_typeof(...) = 'object'.projections: a bundle is round-trippable, so a
***REDACTED***sentinel would not hide a value —it would be imported as a literal command string and persisted over the real one. Dropping with
a warning is the idiom this file already uses for
setupCommand/cleanupCommand, and thewarning keeps withheld distinguishable from unset. Safe import refuses the field outright.
exportBundle's new option defaults to withheld, so the fourth caller(
services/cloud-upstreams.ts, which passes no options) fails closed. It does not regress: thatpath uses the include-all branch, which resolves through
issueListSelect— and that alreadyprojects the column to SQL
null.the inventory from
res.json({...issue})shapes alone will flag the full list branch as athirteenth site; it is not one, and the reason is in the SELECT rather than in the route.
Merge-order note: security(projects): mask plain env bindings on every project response exit (PEN-3033) #1670 also edits
routes/projects.ts. No overlapping lines today, but whichever lands second should re-check.Model Used
Claude Opus (Anthropic), model id
claude-opus-5[1m], 1M context, extended thinking, with tool use and code execution. Two read-only subagents were used for the exit and consumer enumerations; every finding they reported was re-verified at source before it was relied on.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template