security(workspaces): scrub secrets on the workspace-operation write path (PEN-3205) by allyblockcast[bot] · Pull Request #1853 · Blockcast/paperclip

allyblockcast · 2026-09-14T12:58:01Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work, so every durable byte an agent's command emits is a byte some other principal in the company may later read.

The subsystem here is workspace operations — the recorder that captures stdout/stderr from operator-authored provision / cleanup / teardown / workspace-job commands into stdoutExcerpt, stderrExcerpt and the workspace-operation log store.

The gap: that path applied exactly one transform to captured output, redactCurrentUserText — username censoring driven by the censorUsernameInLogs instance setting. It is not a secret scrub, and redactSensitiveText appeared nowhere in services/workspace-operations.ts or services/workspace-operation-log-store.ts.

The amplifier is what makes this the PEN-2328 / PEN-2370 shape rather than "just git output": buildWorkspaceCommandEnv (services/workspace-runtime.ts:3658-3659) builds those commands' environment as { ...process.env, ...buildAgentGitIdentityEnv(input.agent) } — the server process environment, wholesale. A command running under set -x, or any tool that dumps its environment on failure, wrote that environment into a durable company-readable channel with no secret scrub in the path.

It needs addressing on the write side specifically because narrowing who may read these fields (PEN-3204) does not fix an unscrubbed write, and an unscrubbed write persists in durable storage regardless of how reads are scoped. Both halves are needed and they are independent.

This pull request routes workspace-operation chunks through the same secret scrub the run-log path already uses, once, before the excerpt fork, so the excerpt columns and the log-store body cannot diverge.

The benefit is that the one remaining transcript path with neither a write-time secret scrub nor a read-side projection now has the write-side control, and the misleading comment that claimed otherwise is corrected.

Linked Issues or Issue Description

Refs PEN-3205 — this row (workspace-operation write-path scrub)
Refs PEN-3246 — the backfill decision for already-stored rows (the PEN-3158 analogue), filed rather than silently skipped
Refs PEN-3202 — the ruling this was split out of
Refs PEN-3204 — the read-side half; deliberately not in scope here
Refs PEN-3153, PEN-3139 — the analogous write-path rows on other subsystems

Related PRs found in the dedup search (adjacent paths, not duplicates of this one): #1746 (PEN-3153, resultJson/error persistence), #1736 (PEN-3139, run-log free-text vendor shapes), #1747 (PEN-3152, github MCP egress), #1754 (PEN-3157, server GitHub write boundary), #1741 (PEN-3142, run-transcript read scoping). No open or closed PR targets the workspace-operation write path.

What Changed

Extract the shared sanitizer into services/log-chunk-sanitizer.ts. heartbeat.ts already imports workspace-operations.js, so importing sanitizeRunLogChunkForStorage back from there would close a cycle — which is plausibly why this path grew its own weaker private transform. heartbeat.ts now imports and re-exports both helpers, so existing importers and tests keep their entry point. One definition, not two: a second sanitizer would be a second oracle that can drift, and the weaker one would decide what a reviewer believes is covered.
Sanitize once, before the excerpt fork, so the excerpt columns and the log-store body cannot diverge. Scrubbing one and not the other is the exact half-control this replaces.
Censor command / cwd at write, consistent with the metadata written beside them — a cwd under a censored user's home was masked in the log body and legible in the column next to it. command also gets the secret scrub (an operator provision command can carry a credential as an inline argument). null stays null and "" stays "": the transform applies to the value, it does not reclassify an empty command as an absent one.
Correct the readLog comment that claimed chunks were "sanitized before append-time storage" — that overstated a username-censor-only transform, the same overstatement PEN-3139 corrected on the run-log path, except here there was no secret scrub at all to overstate.
Apply the same username censoring on GET /execution-workspaces/:id/workspace-operations that routes/agents.ts already applied. Both answer with the same historical rows including the excerpts, which publicWorkspaceOperation deliberately does not withhold, so censoring one route and not the other left the same bytes legible one URL over.

Verification

Verified at origin/master @ 2294f311 (fresher than the e34a14b the row was filed against).

npx tsc --noEmit -p server/tsconfig.json → clean
New test file server/src/__tests__/workspace-operation-secret-scrub.test.ts: 5/5 pass
16 related test files (redaction, execution-workspace routes/service, workspace-response withholding, run-log): 282/282 pass
The 14 pre-existing heartbeat-run-log tests pass unchanged through the extracted module, which is the evidence the extraction is behaviour-preserving

The tests fail first. Against the pre-fix path, 3 of 5 fail — the synthetic credential reaches both the excerpt column and the log-store body in the clear:

AssertionError: expected 'PAPERCLIP_SYNTHETIC_TOKEN=fake-pen320…' not to contain 'fake-pen3205-workspace-op-secret'
 ❯ scrubs the same chunk in the durable log-store body
 ❯ scrubs secret-shaped stderr too — the failure path is where env dumps land

Both sinks are asserted deliberately — a test that only read the excerpt would pass against a fix that left the log body unscrubbed. The two pass-through cases are the discriminator between scrubbing and blanking: a fix that simply dropped the output would satisfy every not.toContain and fail those two.

One regression was introduced and fixed during review: importing instanceSettingsService from the ../services/index.js barrel broke execution-workspaces-routes.test.ts, which mocks that barrel with an explicit export list. Importing the module directly (matching the service-layer convention) resolves it.

Risks

Low-to-moderate, and concentrated in the extraction, not the scrub. Moving sanitizeRunLogChunkForStorage out of heartbeat.ts touches a hot path shared with run-log storage. The mitigation is that the 14 pre-existing heartbeat-run-log tests pass unchanged through the extracted module — behaviour-preserving by test, not by assertion.
Over-scrubbing is possible in principle. The shared scrub is regex-driven, so a non-secret string matching a credential shape would be masked in an operation excerpt, making a failure marginally harder to debug. This is the same trade already accepted on the run-log path, and the two pass-through tests pin that ordinary output is not blanked.
This PR does not narrow reads, and does not claim to. The excerpts remain company-readable by standing product decision (CTO Ruling F §4, PEN-3202). Anyone reading this as "workspace operations are now safe to read widely" would be over-reading it — that question is PEN-3204.
Already-stored rows are untouched. Existing unscrubbed excerpts and log bodies remain as written; the backfill decision is PEN-3246. That row notes the log body is a file on disk, not a column, so a SQL-only backfill would clean the excerpts and leave the bodies — reproducing the very half-control this PR closes.
Two CI security flags, both reviewed and expected, neither a merge block (security-review is neutral): the secret-scan hit is the synthetic fixture fake-pen3205-workspace-op-secret in the new test — a deliberately fake literal, no real credential is committed; the sensitive-path hit is routes/execution-workspaces.ts, expected because this PR touches that route by design. The advisory-sync 403 in that job is an environmental permission failure, not a finding.
No migration, no schema change, no API shape change.

Scope — checked and deliberately not changed

publicWorkspaceOperation already masks command/cwd for unentitled viewers. Narrowing the company-readable excerpts is PEN-3204, not this PR.
I swept all four publicWorkspaceOperation(s) call sites, not the two the row named. The other two (execution-workspaces.ts:499, projects.ts:648) return a freshly recorded operation, which is now censored at write, so they need no read-side change.

Corrections to the filed row

Reporting these rather than quietly shipping past them:

The row said routes/execution-workspaces.ts:137 was a bare res.json(operations). At current HEAD it is not — it is res.json(publicWorkspaceOperations(operations, viewer)). That half self-healed before I got here; the residual asymmetry was only the redactCurrentUserValue wrapper, which is what this PR adds.
Line numbers drifted: buildWorkspaceCommandEnv is :3658-3659 (filed as :3482-3483); command/cwd are :143-144 (filed as :138-139). The cited :126, :41 and :256 are unchanged.
The row's comparison table is correct: the run-log path really does scrub, transitively — sanitizeRunLogChunkForStorage → compactRunLogChunk → redactSensitiveText. Worth stating because sanitizeRunLogChunkForStorage does not name the scrub at its own call site.

Occurrence is unmeasured — deliberately

I did not measure whether credential material actually landed here, and I read no peer's operations. The shape was unprotected; the rate is unknown. Do not price this as though it were measured, and do not read "no evidence" as "no occurrence" — nobody has looked.

Model Used

Claude Opus 5 (claude-opus-5), 1M context window, extended thinking enabled, with tool use and code execution. Used to author the change, the tests, and this description.

Checklist

allyblockcast · 2026-09-14T12:58:09Z

🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: PEN-2328
🔗 Paperclip issue: PEN-3139
🔗 Paperclip issue: PEN-3158
🔗 Paperclip issue: PEN-3205
🔗 Paperclip issue: PEN-3153
🔗 Paperclip issue: PEN-3246
🔗 Paperclip issue: PEN-3202
🔗 Paperclip issue: PEN-3204

allyblockcast · 2026-09-14T12:58:09Z

🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: PEN-2328
🔗 Paperclip issue: PEN-3139
🔗 Paperclip issue: PEN-3158
🔗 Paperclip issue: PEN-3205
🔗 Paperclip issue: PEN-3153
🔗 Paperclip issue: PEN-3246
🔗 Paperclip issue: PEN-3202
🔗 Paperclip issue: PEN-3204

allyblockcast · 2026-09-14T13:02:10Z

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

Missing section: ## Thinking Path
Missing section: ## What Changed
Missing section: ## Risks
Missing section: ## Model Used
Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e81d100

The implementation consistently applies the shared sanitizer before both workspace-operation sinks, preserves the existing sanitizer entry points, and masks historical operation-list responses on the sibling route. I found no Critical or Important issues in the current diff.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

Shared write-time sanitization prevents excerpt and log-store divergence.
The regression tests cover stdout, stderr, both persistence sinks, and non-secret pass-through.
The PR clearly separates the write-path fix from the explicitly deferred historical backfill.

Recommended Action

Keep the current implementation.
Complete the pending CI checks before merge.

…path (PEN-3205) `services/workspace-operations.ts` applied exactly one transform to captured command output: `redactCurrentUserText`, which is username censoring driven by `censorUsernameInLogs`. It is not a secret scrub, and `redactSensitiveText` appeared nowhere in that file or in `workspace-operation-log-store.ts`. That single sanitized-only chunk landed in all three durable sinks -- `stdoutExcerpt`, `stderrExcerpt`, and the log-store body. Why this is the PEN-2328 / PEN-2370 shape and not just "git output": `buildWorkspaceCommandEnv` (`services/workspace-runtime.ts:3658-3659`) builds the environment for operator-authored provision / cleanup / teardown / workspace-job commands as `{ ...process.env, ...buildAgentGitIdentityEnv(agent) }` -- the server process environment, wholesale. `recordWorkspaceCommandOperation` then captures that command's stdout and stderr straight into the excerpts and the log body. A command running under `set -x`, or any tool that dumps its environment on failure, wrote the server's environment into a durable company-readable channel with no secret scrub in the path. This is a write-path content defect, independent of PEN-3204: narrowing the read does not fix an unscrubbed write, and scrubbing the write does not by itself justify a standing company-wide read. Both halves are needed. Changes: - Extract `compactRunLogChunk` / `sanitizeRunLogChunkForStorage` out of `heartbeat.ts` into `services/log-chunk-sanitizer.ts`. `heartbeat.ts` already imports `workspace-operations.js`, so importing the sanitizer back from there would close a cycle -- which is why this path grew a weaker private transform in the first place. `heartbeat.ts` re-exports both so existing importers and tests keep their entry point. One definition, not two: a second sanitizer would be a second oracle, and the weaker one decides what a reviewer believes is covered. - Route captured chunks through the shared sanitizer ONCE, before the excerpt fork, so the excerpt columns and the log-store body cannot diverge. Scrubbing one and not the other is the half-control this replaces. - Censor `command` and `cwd` at write, consistent with the `metadata` beside them. A cwd under a censored user's home was masked in the log body and legible in the column next to it. `command` additionally gets the secret scrub -- an operator provision command can carry a credential as an inline argument. Null stays null and "" stays "": the transform applies to the value, it does not reclassify an empty command as an absent one. - Correct the `readLog` comment that claimed chunks were "sanitized before append-time storage". That overstated a username-censor-only transform, the same overstatement PEN-3139 corrected on the run-log path -- except here there was no secret scrub at all to overstate. - Apply the same username censoring on `GET /execution-workspaces/:id/workspace- operations` that the sibling list route in `routes/agents.ts` already applied. Both answer with the same historical rows including the excerpts, which `publicWorkspaceOperation` deliberately does NOT withhold, so censoring one route and not the other left the same bytes legible one URL over. New rows are censored at write now; this still covers rows stored before this commit. Tests fail first against the pre-fix path: the synthetic credential reaches both the excerpt column and the log-store body in the clear (3 of 5 assertions fail). The pass-through cases are the discriminator between scrubbing and blanking -- a fix that simply dropped the output would satisfy every `not.toContain` and fail those two. Scope checked and deliberately NOT changed here: `publicWorkspaceOperation` already masks `command`/`cwd` for unentitled viewers, and the excerpts are company-readable by a standing product decision (CTO Ruling F, PEN-3202). Narrowing that read is PEN-3204, not this row. Occurrence is unmeasured: the shape was unprotected, the rate is unknown, and no peer's operations were read. The backfill decision for already-stored rows is filed as PEN-3246 rather than silently skipped. Refs: PEN-3205, PEN-3246, PEN-3202, PEN-3153, PEN-3139 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

…-3205) The workspace-operations route added in the parent commit reads `censorUsernameInLogs` per request, to apply the same username censoring `routes/agents.ts` already applied on the sibling list route. The PEN-2852 withholding apps mount `{}` as `db`, so the real `instanceSettingsService` reached a `getGeneral()` against a stub and the route answered 500 -- reddening both door-#15 tests. Typecheck was blind to it: the import is a direct module path, and the break is a runtime call through a stubbed `db`. Stub the service the way the other route tests in this suite already do, and default `censorUsernameInLogs` to OFF so the censor is inert underneath the withholding assertions -- those measure `publicWorkspaceOperation`, and a censor running beneath them could mask a sentinel and pass one for the wrong reason. That default would have left the route line itself uncovered, so cover it explicitly with a pair: excerpts are spread through `publicWorkspaceOperation` unmasked by design, so on this route the censor is the only control standing over `stdoutExcerpt`. The setting is the sole discriminator between the two cases -- removing the censor from the route fails the on-case (verified by mutation), and replacing it with blanket blanking fails the readability assertions. The home directory is read from `os.homedir()`, the same value `defaultHomeDirs` derives its module-cached candidates from, so the pair is deterministic on any runner without reaching into that cache. Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9feaa97

The current diff consistently applies the shared sanitizer before both workspace-operation output sinks, preserves the existing heartbeat sanitizer entry points, and adds legacy-route username censoring. I found no Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

Shared write-time sanitization prevents excerpt and log-store divergence.
Tests cover stdout, stderr, both persistence sinks, and non-secret pass-through.
The PR clearly separates the write-path fix from the deferred historical backfill.

Recommended Action

Keep the current implementation.
Merge after the repository's applicable checks remain green.

allyblockcast Bot commented Sep 14, 2026

View reviewed changes

allyblockcast Bot mentioned this pull request Sep 14, 2026

security(workspaces): withhold operator-authored command strings on execution-workspace, project, and issue row responses (PEN-3073, PEN-3252) #1858

Merged

13 tasks

Security Engineer added 2 commits September 14, 2026 16:34

kkroo force-pushed the sec/pen-3205-workspace-op-scrub branch from e81d100 to 9feaa97 Compare September 14, 2026 16:35

allyblockcast Bot commented Sep 14, 2026

View reviewed changes

kkroo added this pull request to the merge queue Sep 14, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 16, 2026

This was referenced Sep 16, 2026

test(recovery): drop the obsolete getLatestIssueRun allowlist entry from the pooled-under-lock ratchet #1893

Merged

fix(ci): key the production-protection alert fingerprint to the violation set (PEN-2863) #1596

Merged

allyblockcast Bot added this pull request to the merge queue Sep 17, 2026

Merged via the queue into master with commit d1706ea Sep 18, 2026
22 checks passed

This was referenced Sep 19, 2026

fix(workspace-operations): project and audit the operation log route (BLO-34631) #1930

Merged

feat(ci): surface merge-queue ejections on the pull request (BLO-28886) #1977

Closed

Navigation Menu

security(workspaces): scrub secrets on the workspace-operation write path (PEN-3205) - #1853

security(workspaces): scrub secrets on the workspace-operation write path (PEN-3205)#1853
allyblockcast[bot] merged 2 commits into
masterfrom
sec/pen-3205-workspace-op-scrub

allyblockcast Bot commented Sep 14, 2026 •

edited

Loading

Uh oh!

allyblockcast Bot commented Sep 14, 2026

Uh oh!

allyblockcast Bot commented Sep 14, 2026

Uh oh!

allyblockcast Bot commented Sep 14, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

Conversation

allyblockcast Bot commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

Linked Issues or Issue Description

What Changed

Verification

Risks

Scope — checked and deliberately not changed

Corrections to the filed row

Occurrence is unmeasured — deliberately

Model Used

Checklist

Uh oh!

allyblockcast Bot commented Sep 14, 2026

Uh oh!

allyblockcast Bot commented Sep 14, 2026

Uh oh!

allyblockcast Bot commented Sep 14, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

Recommended Action

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

allyblockcast Bot commented Sep 14, 2026 •

edited

Loading