test(security): pin command by value at both workspaceRuntime door guards (PEN-3130)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agents read each other's issues and workspaces through GET /issues/:id and /heartbeat-context, whose gate assertIssueReadAllowed admits any same-company agent
• The PEN-2370 series closes the doors where operator-authored configuration crossed that boundary in the clear; doors fix(ui): drop stale expand-control assertions from SidebarAgents test (v513 fallout) #12 (security(issues): mask workspaceRuntime values crossing the issue projection (PEN-2846) #1586) and #12b (security(issues): mask workspaceRuntime on the project-workspace projection (PEN-2846 door #12b) #1714) mask workspaceRuntime on those two projections
• Those doors are guarded by tests — and a guard that does not fail on the regression it exists to catch is not a guard
• Both door tests assert three fixture keys but pin only two by value: command is constrained solely by its presence in the Object.keys(...) set
• So a regression that re-attached the operator-authored command line after masking keeps the key set byte-identical, keeps the token redacted, and keeps both suites green
• This pull request pins command by value at both door sites, with a two-column mutation proof identifying exactly which regression class the new lines uniquely catch
• The benefit is that the guard protecting a High finding stops being satisfiable by a change that re-opens the door

Linked Issues or Issue Description

Tracked as PEN-3130 in Paperclip, not as a GitHub issue, so per template path (B) the problem is described in full here and in the commit body.

Raised by Ally as Suggestion 2 on #1714 (door #12b), then verified independently at source before filing — a finding that lives only in a reviewer's Suggestion dies when the PR merges, which is the failure mode that produced PEN-2854.

Severity Low, deliberately: this is a coverage gap in a guard, not a live exposure. command is masked today. But the guard it weakens is the one protecting a High finding, so it should not evaporate.

Related PRs, searched before opening:

• Refs #1586 (PEN-2846, door fix(ui): drop stale expand-control assertions from SidebarAgents test (v513 fallout) #12 — merged) and Refs #1714 (door #12b — merged 2026-09-09) — introduced the two guards this PR hardens.
• Refs #1768 (PEN-3114, door fix(plugin-secrets): resolve secret refs via owning company instead of failing closed #15 — open) — also edits server/src/__tests__/issues-goal-context-routes.test.ts, but appends a new it(...) block at a different location. No overlapping hunk; whichever lands second rebases cleanly.

What Changed

Two assertions, one at each door site, plus the rationale for the next reader. No production code is touched.

expect(runtime.services[0].command).toBe("***REDACTED***");

• server/src/__tests__/issues-goal-context-routes.test.ts — door fix(ui): drop stale expand-control assertions from SidebarAgents test (v513 fallout) #12 (execution workspace, both routes) and door #12b (project workspaces, both the array and the primary alias).

Why this is not a behaviour change. command is masked today, verified by reading the walk rather than assuming:

step	source
WORKSPACE_RUNTIME_IDENTITY_KEYS = new Set(["id", "name", "label", "title"]) — command is not a member	server/src/redaction.ts:792
WORKSPACE_RUNTIME_ENUM_KEYS covers kind and lifecycle only	:794
so in maskEntry, command takes neither the identity branch (:855) nor the enum branch (:859)	:855-863
and falls to out[key] = maskEntry(child, depth + 1, false); a string is not an object or array, so it returns REDACTED_EVENT_VALUE	:864, :846, :184

Why the existing assertions could not see the regression. Object.keys(...) compares key names; nothing in those lines constrained the value. And the sibling expect(JSON.stringify(res.body)).not.toContain(SECRET) guard cannot see it either, because both fixtures carry their secret in the token field alongside a benign command:

door	fixture
#12	{ name: "api", command: "pnpm dev", GRAFANA_API_TOKEN: SECRET }
#12b	{ name: "web", command: "pnpm dev", DEPLOY_TOKEN: SECRET }

Key set unchanged, token still redacted, not.toContain still satisfied. That is the actual hole.

command is not an incidental leaf — it is a shell command line, the likeliest place for an inline token, a --token= flag or a credential-bearing URL, and the same field PEN-2854 (door #14) exists to mask on the sibling runtimeServices path.

Verification

Run on this branch at origin/master e34a14b09, in a worktree whose first-party links were confirmed to resolve into this tree (node_modules/@paperclipai/shared → pen3130-pin/packages/shared) so the suite loaded the code under test.

$ npx vitest run src/__tests__/issues-goal-context-routes.test.ts
  Test Files  1 passed (1)
       Tests  15 passed (15)

$ npx tsc --noEmit          # in server/
  exit 0

Mutation proof — recorded per test, not as a suite total

A passing test proves nothing on its own. Two mutations were run, and they do not carry the same weight:

(a) command added to WORKSPACE_RUNTIME_IDENTITY_KEYS — the "the control tools address entries by command too" edit.

× masks configured workspaceRuntime values on GET …/heartbeat-context
× masks configured workspaceRuntime values on GET /api/issues/…
× masks configured workspaceRuntime values on project workspaces from GET /issues/:id
    AssertionError: expected 'pnpm dev' to be '***REDACTED***'
  Tests  3 failed | 12 passed (15)

# and, independently, already on master:
× redaction.test.ts  "masks a service entry's free-text command and cwd"
  Tests  1 failed | 62 passed (63)

This mutation was already caught corpus-wide, so it is not what justifies this PR. redaction.test.ts:992-996 puts the secret inside the command line and pins command by value at the walk level. The ticket's original claim that this mutation keeps "both suites green" was wrong, and is corrected here rather than quietly dropped.

(b) A projection-layer re-attach at server/src/routes/issues.ts:7503 and :7622 — a plausible "keep the command legible in the UI" edit restoring command after maskWorkspaceRuntimeForRead returns. This is the class the new lines uniquely catch:

	door tests	redaction.test.ts
with these lines	3 failed | 12 passed	63 passed
without these lines	15 passed	63 passed

Without these two assertions the entire relevant corpus is green while the operator-authored command line crosses the boundary verbatim. redaction.test.ts cannot see it because it calls the walk directly and never goes through the route; the door tests' own not.toContain cannot see it because the fixture secret is in the token, not the command.

After the proof both mutation targets were reverted and confirmed byte-identical to master (git diff --exit-code server/src/routes/issues.ts server/src/redaction.ts), leaving the test file as the only change.

redaction.test.ts is deliberately not touched — :996 already pins command by value in that same describe, so a line there would be redundant.

Risks

Very low. Test-only; no production code, schema, migration, route or response body changes. The new assertions pin behaviour that already holds on master.

Two things a reviewer should weigh rather than take on trust:

1. These lines constrain future design. If masking command is ever deliberately relaxed — e.g. a reviewed decision to show a sanitised command line in the UI — these assertions will fail and must be updated as part of that decision. That is the intent: the change becomes a decision instead of a silent regression.
2. Column (b)'s mutation is my construction, not an observed bug. No such re-attach exists on master today. The claim is that it is a plausible edit at a real call site and that nothing in the tree would catch it — the table above is the evidence for the second half, which is the half that matters.

Model Used

Claude Opus 5 (claude-opus-5[1m]), 1M-context configuration, extended thinking enabled, run as an autonomous Paperclip agent (Security Engineer) with tool use — filesystem, git/gh, and Paperclip MCP. Every source claim above was read from the tree at e34a14b09 rather than recalled, and every test result was produced by the run quoted.

No credential value was read, quoted, or committed. No populated workspace was read through paperclipGetIssue / paperclipGetHeartbeatContext / paperclipGetIssueWorkspaceRuntime, per the PEN-2370 parent's prohibition — reading that response is the exposure. All fixture values are pre-existing and invented.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable — this PR is the test change
• If this change affects the UI, I have included before/after screenshots — n/a, test-only
• I have updated relevant documentation to reflect my changes — the rationale is at the assertion site, which is where the next reader will be
• I have considered and documented any risks above
• All Paperclip CI gates are green — not yet observed; this PR has just been opened
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet reviewed
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: PEN-3130

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The new assertions pin the sensitive value, not just object shape, at both workspaceRuntime read boundaries.
• The fixtures keep the secret in a sibling token field, which preserves the exact regression class this PR is intended to catch.
• The change is test-only and aligns with the existing redaction contract for free-text workspace runtime fields.

Recommended Action

1. No Ally blockers.
2. Because this PR is authored by app/allyblockcast, this is a formal App COMMENTED review rather than an App approval.

[allyblockcast]

⚠️ This PR is now moot and conflicting — recommend closing it unmerged rather than rebasing

Posting evidence rather than acting: this is my own PR, but "my own PR is moot" is not a call I should make alone. Tracked as PEN-3130.

1. Superseded. f46251a24 ("test(issues): pin PEN-2846's exits to PEN-2852's stronger null, not the old mask", 2026-09-13T09:30:03Z) deleted the assertions this PR strengthens and replaced them with toBeNull():

expect(res.body.currentExecutionWorkspace.config.workspaceRuntime).toBeNull();
expect(emitted.runtimeConfig.workspaceRuntime).toBeNull();

master now has zero ***REDACTED*** assertions in this file. toBeNull() strictly dominates what this PR adds: the threat here was a projection edit re-attaching command while keeping the key set byte-identical — under PEN-2852 an unentitled reader gets no object at all, so there is no key set to hide in.

2. Conflicting. GitHub reports mergeable: UNKNOWN (queued, position 12), so measured locally:

git merge-tree --write-tree origin/master pr1822
  -> CONFLICT (content): server/src/__tests__/issues-goal-context-routes.test.ts

It will be ejected when it reaches the front of the queue, as #1768 was on 2026-09-14T18:27:04Z.

3. Rebasing it would make things worse, not better. The 11 added lines are two copies of expect(runtime.services[0].command).toBe("***REDACTED***"), and on master runtime is null. Resolving the conflict either re-introduces the weaker pre-PEN-2852 mask expectations — a security-test regression against f46251a24 — or adds assertions that throw TypeError and redden master. There is no resolution of this diff that improves the current file.

Recommendation: close unmerged. The finding it was opened for is closed, by a better control than the one proposed here. No action needed on master.

cc @kkroo — this replaces the earlier merge-press ask on PEN-3130; pressing it now would merge a conflicting, regressive diff.

[allyblockcast]

Mutation proof: master already catches the regression this PR was written to catch — the diff is redundant, not just conflicting

Follow-up to my 09-15T02:44Z comment. That one argued this PR was moot because PEN-2852 nulls workspaceRuntime for an unentitled reader. That argument was correct but incomplete: it only covered the two door sites, and it never checked the layer that survives underneath them. I have now checked, and run the experiment.

The surviving layer is already pinned by value

maskWorkspaceRuntimeForRead is still live on master (server/src/redaction.ts:839), retained as defense-in-depth — routes/issues.ts:7630/:7778 re-read viewer.revealRuntimeConfig so the mask still fires if the PEN-2852 gate is ever dropped or bypassed.

That layer already carries the exact assertion this PR adds, at server/src/__tests__/redaction.test.ts:996:

expect(masked.services[0].command).toBe(REDACTED_EVENT_VALUE);
expect(masked.services[0].cwd).toBe(REDACTED_EVENT_VALUE);

The experiment

Applied the precise mutation named in PEN-3130's "Concrete regression this misses" — command added to the identity key list — against origin/master @ 04598e24a:

-const WORKSPACE_RUNTIME_IDENTITY_KEYS = new Set(["id", "name", "label", "title"]);
+const WORKSPACE_RUNTIME_IDENTITY_KEYS = new Set(["id", "name", "label", "title", "command"]);

run	result
origin/master @ 04598e24a, unmutated	PASS (1 passed, 62 skipped)
same, with command added	FAIL — redaction.test.ts:995

AssertionError: expected '{"services":[{"name":"db","command":"...' not to contain 'operator-authored-value-no-list-could...'
Received: {"services":[{"name":"db",
           "command":"psql [paperclip-egress-scrub redacted: credentialed-uri]",
           "cwd":"***REDACTED***"}]}

Mutation reverted; tree clean at 04598e24a.

What that settles

PEN-3130's filed premise was "a regression that passed command through verbatim ... keeps both suites green." On master today that is false. The regression reddens redaction.test.ts immediately, and it does so on the mask layer — so it holds whether or not the PEN-2852 gate is intact. Combined with the null gate at the two door sites, the finding is closed twice over, by two independent controls, neither of which is this PR.

Recommendation unchanged, now better evidenced: close unmerged

• Redundant — proven above.
• Conflicting — re-measured this run: git merge-tree --write-tree origin/master 40f33758c gives CONFLICT (content) in issues-goal-context-routes.test.ts.
• Regressive if rebased — its 11 lines assert runtime.services[0].command, and on master runtime is null. Resolving the conflict mechanically either restores the weaker mask-era expectations that f46251a24 deliberately replaced, or adds assertions that throw TypeError.

FYI it was ejected from the merge queue at 2026-09-15T08:42:08Z by github-merge-queue[bot], consistent with the conflict. Please do not re-queue it — my earlier merge-press card was withdrawn as moot and that withdrawal still stands.

I am not closing it myself: it is outward-facing, and "my own PR is moot" is not a call I should be the only one to make. Routed as a decision card on PEN-3130.

— Security Engineer

[kkroo]

Closing as superseded. I attempted the rebase onto master 3da987c03 and stopped rather than forcing it — the conflict here is not a textual one, it is a behavioural contradiction, and resolving it in this branch's favour would ship failing tests.

Why the assertions are now unreachable. Both hunks assert a masked-but-present shape:

expect(runtime.services[0].command).toBe("***REDACTED***");

Master asserts the opposite at those same two doors — expect(...workspaceRuntime).toBeNull(). That is PEN-2852/BLO-33407, and it is in the production code, not just the tests: routes/issues.ts:7707 and :7874 both note that publicProjectWorkspace / publicExecutionWorkspace have already set workspaceRuntime: null for an unentitled viewer, "so masking it would be a no-op… for an unentitled reader, nothing crosses at all." With the blob null, runtime.services[0] throws.

The invariant you wanted is already enforced, and more widely than this PR proposed. publicRuntimeServices (workspace-response.ts:424-429) still masks command/cwd for unentitled viewers, so the concern was real — but server/src/__tests__/workspace-runtime-response-withholding.test.ts already pins it by value:

• :619 expect(res.body.runtimeServices[0].command).toBe(REDACTED_EVENT_VALUE)
• :682 expect(service.command).toBe(REDACTED_EVENT_VALUE)
• plus provisionCommand, teardownCommand, cleanupCommand, setupCommand at :479-510

That file contains zero Object.keys-only assertions — the exact weaker form this PR was written to strengthen. So the gap is closed, in the file that owns the boundary, against the projection where the mask still has live effect.

Nothing is lost by closing. If you want the door-level coverage specifically, the place to add it is a test against publicRuntimeServices on the entitled path (where revealRuntimeConfig short-circuits and values cross by design) — not against currentExecutionWorkspace.config.workspaceRuntime, which no longer carries services for the reader class these tests exercise.

🤖 Generated with Claude Code