security(issues): mask plugin-authored defaultsJson on the issue project projection (PEN-3114)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agents read each other's issues through GET /issues/:id (paperclipGetIssue), a response whose gate assertIssueReadAllowed admits any same-company agent via allow_company_agent
• That handler builds its project payload with compactIssueProject() — a projection, which enumerates named fields instead of spreading the row, and whose env: null line proves the author treated this response as a deliberate withholding boundary
• One field crossed that boundary verbatim: managedByPlugin. Its defaultsJson is an open Record<string, unknown> over a jsonb column, and its settings leaf is copied straight out of a plugin manifest by buildManagedProjectDefaults
• So the leaf furthest from a Paperclip operator's hands — authored by a plugin author, in the block a manifest would naturally use for integration config — was the one emitted unmasked, one line below an explicitly withheld field
• This pull request masks the values under defaultsJson while keeping every key name, by delegating to the existing maskWorkspaceRuntimeForRead walk rather than writing a second one
• The benefit is that door fix(plugin-secrets): resolve secret refs via owning company instead of failing closed #15 of the PEN-2370 series closes through the same walk as doors fix(ui): drop stale expand-control assertions from SidebarAgents test (v513 fallout) #12/feat(heartbeat): enrich agent.run lifecycle event payload #14, so the next finding against that walk fixes this call site too instead of leaving a divergent copy behind

Linked Issues or Issue Description

Tracked as PEN-3114 (door #15 of the PEN-2370 series) in Paperclip, not as a GitHub issue, so per template path (B) the problem is described in full below and in the commit body.

The problem. compactIssueProject() in server/src/routes/issues.ts emitted managedByPlugin: project.managedByPlugin ?? null verbatim. The chain, traced link by link on master:

step	source
PluginManagedProjectDeclaration.settings?: Record<string, unknown> — "Optional plugin-specific defaults retained for reset/reconcile UI"	packages/shared/src/types/plugin.ts:288-289
buildManagedProjectDefaults(declaration) sets settings: declaration.settings ?? {}	server/src/services/projects.ts
persisted to pluginManagedResources.defaultsJson (jsonb, so the runtime value is arbitrary regardless of the TS type)	packages/db/src/schema/plugin_managed_resources.ts:19
hydrated verbatim onto ProjectManagedByPlugin.defaultsJson (non-optional Record<string, unknown>)	packages/shared/src/types/project.ts:83
emitted verbatim by compactIssueProject	server/src/routes/issues.ts

Severity is Low, deliberately: the type is open and the value crossed unmasked, but no in-tree shipped plugin populates settings today, so this is latent-by-type rather than demonstrated-in-tree. It is fixed anyway because the shape is the class PEN-2370 exists to close. One in-tree corroboration that the chain is live end to end: server/src/__tests__/plugin-orchestration-apis.test.ts declares a manifest with settings: { surface, upgraded } and asserts it lands in defaultsJson. That is a test manifest, not a shipped plugin, so it confirms the mechanism without raising the rating.

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 — introduced maskWorkspaceRuntimeForRead, the walk this PR delegates to.
• Refs #1581 — extracted withholdAgentConfigKeys; evaluated and rejected here (see below).
• Refs #1670 (PEN-3033, door fix(plugin-tool-dispatcher): pass pluginDbId through registerPluginTools — fixes tools/execute 502 #17 — open) — also touches server/src/routes/issues.ts, but at the mentionedProjects call site ~900 lines away and for plain project bindings only. No overlapping hunk.
• Refs #1574, Refs #1583 — the array-shaped and JSON-string bypasses that make "do not copy the walk" a hard rule here.
• No open PR touches server/src/__tests__/issues-goal-context-routes.test.ts, so this does not collide with the sibling test-hardening work tracked as PEN-3130.

What Changed

• server/src/routes/issues.ts — new compactIssueManagedByPlugin() helper, used by compactIssueProject:
  • defaultsJson is passed through maskWorkspaceRuntimeForRead. Values are masked to ***REDACTED***, every key name survives, non-objects are masked outright, and the walk is depth-capped fail-closed — which matters because the column is jsonb.
  • managedByPlugin is now enumerated rather than spread, so a field added to ProjectManagedByPlugin later must be considered here instead of crossing silently. All nine current fields of the type are covered; none is dropped.
  • A block comment records why this walk and the two rejected alternatives, so the next reader does not re-derive it.
• server/src/__tests__/issues-goal-context-routes.test.ts — one test asserting the mask end to end through GET /issues/:id.

Why maskWorkspaceRuntimeForRead and not something else — judged before delegating, as the ticket asked:

• Its contract is exactly PEN-2370 ask 1 / criterion b2: mask every value, keep every key name.
• Its commands/services/jobs identity carve-out is inert here — it only applies to entries sitting directly inside an array under one of those three names, and the platform writes projectKey/displayName/description/status/color/settings. Verified by reading the walk, not assumed. The residual if a manifest ever wrote a top-level services array is bounded: only id/name/label/title strings survive, which that walk already discloses in the strictly more sensitive workspace-runtime position.
• Rejected: withholdAgentConfigKeys (security(skills): withhold agent config from skill test-run snapshots (PEN-2839) #1581) does not fit — it is keyed on the literal names adapterConfig/runtimeConfig, and it blanks its target to {}, erasing the key names ask 1 requires be kept.
• No fresh walk was written. Copying one is the documented transmission mechanism for fix(approvals): gate hire_agent embedded agent config on agent_config:read #1574 and security(logger): contain credential-bearing config in 4xx/5xx request-body logs (PEN-2843) #1583.

Why masking is safe, not a compatibility break. defaultsJson's own doc comment says it is retained for the reset/reconcile UI, so this was checked rather than assumed, and it resolves safe for two independent reasons: (1) that path is write-only with respect to this response — it recomputes the defaults from the manifest declaration and writes them back, never reading this projection; (2) no UI reads defaultsJson at all — swept the complete tree (1,245 tracked ui/ files), and the only ui/ hit is a test fixture (ProjectDetail.test.tsx:151, defaultsJson: {}). ProjectDetail.tsx reads pluginDisplayName, pluginKey and resourceKey, all of which survive untouched, and reads them from GET /projects/:id — a route this PR does not touch.

Verification

All run locally on this branch, rebased onto master c3d1c11b7, in a fresh pnpm install --offline — deliberately not a sibling worktree's node_modules, and the first-party links were confirmed to resolve into this worktree (node_modules/@paperclipai/shared points at pen3114-run2/packages/shared) so the suite loaded the tree under test.

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

$ npx tsc --noEmit          # in server/
  exit 0, no output

Mutation check — the test fails on unfixed code. A passing test proves nothing on its own, and "test passes on unfixed code" is a repeat failure mode in this series, so the single line was reverted to managedByPlugin: project.managedByPlugin ?? null and the test re-run:

× masks plugin-authored defaultsJson on the project from GET /issues/:id
AssertionError: expected '{"id":"11111111-...' not to contain 'invented-plugin-defaults-fixture-value'

The failure body is the finding itself — the explicitly withheld field and the plugin-authored credential in the same object:

"env": null,
"managedByPlugin": { "defaultsJson": { "settings": {
  "INTEGRATION_API_TOKEN": "invented-plugin-defaults-fixture-value", "region": "us-east-1" } } }

The fix was then restored and the tree confirmed byte-identical to the commit (git status clean).

Test-quality notes, both deliberate:

• The credential leaf is asserted by value (.toBe("***REDACTED***")), not by key-set membership. A regression passing settings through verbatim keeps the key set byte-identical, so an Object.keys(...) assertion alone would stay green against it — that exact weakness in the door fix(ui): drop stale expand-control assertions from SidebarAgents test (v513 fallout) #12/#12b tests was tracked as PEN-3130, and its lesson is applied here rather than its diff. (Since this PR was opened, master's f46251a24 replaced those key-set assertions with toBeNull() as part of PEN-2852 — strictly stronger, and it cannot pass on a verbatim passthrough — so PEN-3130's original target no longer exists. The principle still governs this PR's own new test.)
• The fixture secret is a third distinct invented value. Reusing either workspace-runtime constant would let an already-merged mask satisfy the not.toContain, and the test would pass against the unfixed projection. No real endpoint was called to produce it, and no plugin-managed project was read to reproduce the finding — it was confirmed from source, because reading it is the exposure.
• region is asserted masked too, documenting that this walk is mask-by-default rather than a credential-name denylist — the property that makes it cover a manifest key nobody enumerated in advance.

Rebase update — 2026-09-15

master moved under this PR. PEN-2852 / BLO-33407 landed in this same file and the same test file, and on 2026-09-14T18:27:04Z github-merge-queue[bot] ejected this branch from the queue as CONFLICTING. Rebased c6a1a55a0 → c3d1c11b7 and re-verified from scratch; MERGEABLE is restored.

The conflict was not textual only, so two judgements were made rather than auto-resolved:

1. Composition — why defaultsJson is masked unconditionally while the two runtime exits beside it are now entitlement-gated. PEN-2852 changed the workspace-runtime exits here to viewer.revealRuntimeConfig ? raw : mask(...). This call site deliberately does not follow that shape, because workspace_runtime:read is scoped to workspace runtime config and defaultsJson is plugin-manifest material with a different audience — gating it on that flag would disclose plugin defaults to every holder of an unrelated entitlement, widening the grant while looking like a narrowing. There is also no entitled consumer to serve (swept above: no UI reads the field), so the gate's true-branch would be empty. If a consumer ever needs raw defaultsJson, it should arrive with its own entitlement. Rationale is recorded at the call site, not just here.

2. Test placement is load-bearing. The auto-merge put the new test inside PEN-2852's describe, whose local beforeEach denies workspace_runtime:read. It was moved out to a sibling block. Under a denied viewer a future entitlement-gated implementation would also return nothing, so the test would stay green without ever measuring the entitled path — the "test passes on unfixed code" shape this series keeps hitting. At file scope the caller is maximally entitled, which is the hardest case for an unconditional mask to satisfy.

Re-verification after the rebase (not carried over from the previous head):

$ pnpm --filter @paperclipai/shared build   # stale dist predated PEN-2852
$ npx tsc --noEmit -p server/tsconfig.json
  exit 0, no output

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

The mutation check was re-run at the new head, not assumed to still hold: restoring managedByPlugin: project.managedByPlugin ?? null fails the test on the secret-value assertion (expected '{"id":"11111111-…' not to contain 'invented-plugin-defaults-fixture-value'), and the fix was then restored.

One observation that is not mine and is not fixed here: server/src/services/plugin-host-services.ts:1343 fails tsc against a stale packages/plugins/sdk build (params.ifMatch). It reproduces on pristine origin/master in a file this PR does not touch, and clears after rebuilding that package — a stale-artifact symptom, reported rather than silently worked around.

Risks

Low. One projection in one handler; no schema, migration, or write path is touched, and no consumer of the masked field exists (swept above).

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

1. Behavioural change for any agent reading project.managedByPlugin.defaultsJson off GET /issues/:id: keys and structure are unchanged, values are now ***REDACTED***. Intended, and that response is the exposure surface — but it is a real change to a response body.

2. This fix inherits maskWorkspaceRuntimeForRead's behaviour by design. That is the point — a finding against the walk fixes this call site too — but it also means the open PRs editing server/src/redaction.ts (fix(redaction): mask five vendor credential shapes on the run-log free-text path (PEN-3139) #1736, feat(security): scope run-transcript reads to own-run + chain + operators + grant (PEN-3142) #1741, fix(security): secret-scrub resultJson and error before persistence (PEN-3153) #1746, fix(redaction): stop a composite gh token leaking 2 of 3 segments into transcripts (BLO-29553) #1683) change this call site's masking. None of them conflicts textually; this PR does not modify redaction.ts.

3. A sibling on this same response is still not covered by this PR — but it is narrower than this PR originally claimed, and the original claim is retracted. Running PEN-2370's method clause against my own fix, the first version of this description said mentionedProjects is emitted entirely unprojected. That is now false. PEN-2852 (BLO-33407) landed mentionedProjects: publicProjects(mentionedProjects, runtimeViewer) on master while this PR sat in the merge queue, so the workspace-runtime half of that sibling finding is closed and the raw metadata/runtimeConfig no longer cross there. Corrected here rather than left standing.

   What remains uncovered is the narrower half: publicProjects gates workspace runtime material, not the plugin binding, so mentionedProjects[].managedByPlugin.defaultsJson still crosses verbatim — the same leaf this PR masks on the projected path, one key over. That is tracked as its own door (PEN-3210) rather than widened into here, because it needs its own severity and its own call site. Deliberately scoped out, not overlooked.

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. The change, the test, the mutation check and this description were produced in that harness; the source claims above were each read from the tree at the stated commit rather than recalled.

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
• If this change affects the UI, I have included before/after screenshots — n/a, no UI consumer exists (swept; only a test fixture references the field)
• I have updated relevant documentation to reflect my changes — the rationale is documented at the call site, which is where the next reader of this projection 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-3114
🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: PEN-3033
🔗 Paperclip issue: PEN-2846
🔗 Paperclip issue: PEN-3130

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [native-codex] server/src/routes/issues.ts:7565 — Keep the projection helper focused; the explanatory block is substantially larger than the implementation, so consider moving the historical rationale to the PR description or a focused redaction-module comment if maintainability becomes an issue.

Strengths

• The projection now enumerates the managed-plugin fields instead of spreading the object, preventing future fields from crossing this response boundary silently.
• defaultsJson delegates to the existing fail-closed masking walk, preserving keys and shape while masking values, including arbitrary plugin settings.
• The regression test uses a distinct fixture secret and verifies the actual response body, including the unfixed pass-through behavior.

Recommended Action

1. Consider the Suggestion opportunistically.
2. Safe to merge from the reviewed code perspective.

[github-actions]

@ally head cb53e44 has been awaiting review for 12.2h with no review on either surface (pulls/1768/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head cb53e44.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The change is correct, minimal, and complete for its stated scope; every load-bearing claim in the PR body was re-verified against the tree at this head rather than taken on trust.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [gstack/review] server/src/routes/workspace-response.ts:253 — the same plugin-authored defaultsJson this PR masks still crosses verbatim on the project endpoints, so the credential in the threat model stays reachable by a same-company caller through a sibling route. publicProject is a spread (...project at :259) that re-projects only workspaces/primaryWorkspace; managedByPlugin is not enumerated, and the if (viewer.revealRuntimeConfig) return project early return at :257 is scoped to runtime config, so the non-entitled branch spreads it raw too. Reached from routes/projects.ts:150 (GET /projects/:id), :287, and :142 via publicProjects (GET /companies/:companyId/projects); the value is the unmasked column, hydrated at services/projects.ts:317 and attached at :379.
  • This is not a defect in this diff and should not hold the PR — the PEN-2370 series is explicitly door-by-door, and the PR body correctly scopes GET /projects/:id as "a route this PR does not touch." Raising it because that sentence cites the route as where the UI reads pluginDisplayName from, without noting it also discloses the raw defaultsJson being masked here.
  • I checked whether it is already tracked: q=defaultsJson returns nothing, and q=publicProject returns only BLO-33568 (PEN-2852 residual — cleanupCommand / plannedActions[].command, a different group of fields) and its parent BLO-33407. So this residual appears untracked. Recommend filing it as the next door rather than widening this PR.

Strengths

• The enumeration is genuinely exhaustive: all nine fields of ProjectManagedByPlugin (packages/shared/src/types/project.ts:82-92) are covered, none dropped, so a field added later has to be considered here rather than crossing silently.
• Delegating to maskWorkspaceRuntimeForRead instead of copying a walk is the right call, and the contract matches what is claimed — null/undefined pass through at top level and at any depth (redaction.ts:840, :843), so the color: null assertion is correct; non-objects mask outright (:846, :869); depth is capped fail-closed (:844). That last property is what makes this safe over a jsonb column.
• The "identity carve-out is inert here" claim is accurate, and I verified it end to end rather than accepting it: the carve-out only activates for a top-level commands/services/jobs array (redaction.ts:873; identityScope is false on every recursive call at :864), and buildManagedProjectDefaults (services/projects.ts:534-543) fixes the top-level keys to projectKey/displayName/description/status/color/settings. Plugin-controlled data lands under settings, i.e. depth 1, where the carve-out cannot reach it.
• The test asserts the credential leaf by value rather than by key presence, which is the assertion that actually fails against a verbatim pass-through — and the region case pins mask-by-default rather than a key-name denylist. The distinct fixture secret and the documented mutation check close the "test passes on unfixed code" failure mode this series has hit before.
• Test placement as a sibling of the entitlement-denying block is correct, and the comment explains why: it exercises the maximally entitled caller, which is the hardest case for an unconditional mask. The ...(await mockProjectService.getById()) pattern matches existing usage in the file (:326, :826).

Recommended Action

1. No Critical or Important issues — safe to merge from the reviewed code perspective.
2. File the publicProject sibling exposure as the next door in the series; it appears untracked today.

[kkroo]

Re-enqueued once after the 22:19Z ejection (kkroo session, not the author). The merge-group failure was productivity-review-service.test.ts > recovers a stale reservation behind a full window of failing ones (BLO-33477): Test timed out in 60000ms, with 8 test-Postgres CONNECTION_ENDED errors in the same job. That is the test #1911 caps, and it is unrelated to this PR's change. If the queue ejects this a second time I will not re-enqueue; it should wait for #1911.

[kkroo]

Merge-queue status note (kkroo sweep, not the author): this PR has now been ejected three times at head cb53e445 — merge_conflict on 09-14, then failed_checks on 09-17 22:19Z and 09-18 17:12Z. The last ejection's merge-group run 35364126297 failed only in General tests (workspaces-b):

FAIL @paperclipai/adapter-utils src/server-utils.test.ts > runChildProcess > still emits kill_signal when the grace SIGKILL reaches a surviving descendant
AssertionError: expected [ 'spawn_attempted', 'spawned', …(4) ] to include 'kill_signal'

That test is unrelated to this PR's change (issue-projection masking) and passed in neighbouring merge-group runs (e.g. #1846's), so it reads as a timing flake under the current runner-starvation (every ARC pool at its ceiling, unschedulable runner pods, shards at 1.5 to 2x baseline). I am not re-enqueueing: the sweep's rule is one re-enqueue per PR after a flake ejection, and the 09-17 23:12Z enqueue already spent it.

Ally's 0/0 at cb53e445 (09-15) still stands and the head's own verify is green. Suggested next step for the author: rebase onto current master (it is 4 days behind, which also raises the odds of a real semantic conflict on the next attempt) and re-enqueue when the cluster is quieter, or harden that grace-SIGKILL test's timing if it keeps flaking.

🤖 Generated with Claude Code

[allyblockcast]

This PR is clean at its current head but still has an outstanding code-owner review request (allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it.