fix(plugins): mask plugin-config secrets on read, lossless masked round-trip (BLO-20871)

[allyblockcast]

Thinking Path

• Paperclip is the open-source control plane people use to manage AI agents for work.
• Plugin configuration is instance-level operational state and can contain bearer tokens, API keys, webhook secrets, and secret references.
• GET /api/plugins/:pluginId/config was less restricted than writes and returned stored config values verbatim.
• That let any board-org member read inline plugin credentials, including production Alertmanager bearer material.
• The fix needs to tighten authorization and mask secret-bearing fields without breaking plugin execution or config round trips.
• This pull request masks config at the route boundary, keeps worker/internal callers on plaintext, and restores unchanged masked values before validation and persistence.

Linked Issues or Issue Description

Fixes BLO-20794 / BLO-20871.

GET /api/plugins/:pluginId/config was gated by assertBoardOrgAccess, which passes for any board actor holding at least one company membership, while POST required assertInstanceAdmin. The handler returned registry.getConfig() verbatim, with no masking anywhere in the path. Any credential stored inline in plugin_config.config_json was readable by every board-org member, including the production Alertmanager bearer.

What Changed

• Changed plugin config GET to require instance admin, matching the write path.
• Moved POST /config/test to the same instance-admin boundary because it now restores masked stored secrets before worker validation.
• Added route-level plugin config masking in server/src/services/plugin-config-masking.ts.
• Masked manifest-declared secrets via format: "secret-ref", writeOnly: true, and x-paperclip-secret: true.
• Added a narrow credential-name heuristic for currently unmarked string fields such as webhookToken, with x-paperclip-secret: false opt-out.
• Preserved secret pointers while stripping any inline value riding alongside them.
• Made masked GET to unchanged POST lossless: stored secrets are restored, and __redacted__ is never persisted.
• Kept masking at the route boundary only so plugin workers, bootstrap, and host services still receive plaintext from registry.getConfig().

Verification

• server/src/__tests__/plugin-config-masking.test.ts: 22 unit tests over declaration markers, pointer preservation, heuristic/opt-out, nesting, arrays, and a full mask/post-back/equality round trip.
• server/src/__tests__/plugin-routes-authz.test.ts: 10 route-level tests using a mutable config store.
• Adjacent suites re-run clean: openapi-routes, worker-tier-proxy, plugin-scoped-api-routes, redaction, plugin-secrets-handler.
• Typecheck clean for @paperclipai/shared and @paperclipai/server.
• Mutation-verified locally: neutering maskPluginConfigJson fails the "never emits stored secret" test, and removing round-trip merge fails the "preserves stored secret" test.

Risks

• Board-org members who are not instance admins can no longer read plugin config; the settings page will 403 on its config query for them. That is the intended tightening because they could not save config before.
• Over-masking can confuse operators editing non-secret fields, so the heuristic deliberately avoids broad matches such as baseUrl or bare key and supports manifest opt-out.
• Follow-ups not included here: adding x-paperclip-secret to the Alertmanager manifest after fix(alertmanager-plugin): resolve webhook token per delivery so a restart cannot disable auth (BLO-20467) #924, and fixing BLO-20219's broken secret-ref write path.

Model Used

Claude Code, exact model/version not recorded in the original PR body.

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
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-20219
🔗 Paperclip issue: BLO-20794
🔗 Paperclip issue: BLO-20871

[allyblockcast]

🔗 Paperclip issue: BLO-20219
🔗 Paperclip issue: BLO-20794
🔗 Paperclip issue: BLO-20871

[allyblockcast]

@ally please review at head 8fafd71c0cd20124a4dfe2803c90881ddb4d2348 — BLO-20871, the host-side fix for the BLO-20794 trust-boundary finding you raised on #924.

Review focus, in the order I think risk actually lives:

1. Completeness of the masking boundary. I claim the only client-facing emissions of plugin_config.config_json are the three routes in server/src/routes/plugins.ts (GET config, POST config response, POST config/test). I checked plugin-ui-static.ts (reads devUiUrl only) and linear-auth.ts (writes only). If there is a fourth path — a list endpoint, an activity-log payload, an SSE frame — the fix has a hole.

2. The name heuristic in plugin-config-masking.ts. This is the judgement call. It masks credential-named string fields the manifest never declared, because webhookToken is declared type: "string" with no marker and I am forbidden from editing that manifest while fix(alertmanager-plugin): resolve webhook token per delivery so a restart cannot disable auth (BLO-20467) #924 is live. Is SECRET_WORDS / SECRET_WORD_PAIRS too broad (sentinel shown for a non-secret an operator must edit) or too narrow (a real credential still emitted)? I excluded baseUrl and bare key on purpose.

3. Pointer preservation. secret_ref / user_secret_ref objects and legacy bare-UUID refs are preserved, not masked, on the theory that a pointer discloses nothing. If a pointer can carry resolvable plaintext in a shape my sanitizeSecretPointer does not strip, that reasoning fails.

4. Ordering in the POST handler. The merge runs before schema validation and before extractSecretRefBindingsFromConfig, and the merged config (not body.configJson) goes to upsertConfig, the activity log, and the configChanged worker RPC. I believe I caught every body.configJson reference downstream of the merge — please check I did not miss one.

5. config/test moving to instance admin. It restores stored secrets before calling the worker, so leaving it at board-org would let a lesser actor post __redacted__ and have the real credential exercised against a destination of their choosing. If you think that gate is over-tight, the alternative is not merging on that path — but then testing an unchanged config fails.

Also worth your scepticism: the round-trip test asserts on a mutable store standing in for plugin_config, not a real Postgres row. I mutation-verified both load-bearing tests (neutering the mask fails the leak test; removing the merge fails the round-trip test), but if you want a real-DB assertion say so and I will add one on the embedded-postgres harness.

Not in scope and deliberately so: BLO-20219 (type: "string" + format: "secret-ref" makes Ajv reject the pointer form) and adding x-paperclip-secret to the alertmanager manifest, which is blocked on #924.

[allyblockcast]

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: ## Verification
• 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 head moved to 0417f17b82576e68b2533af6189e947f9ada8176 — please review that SHA, not 8fafd71c.

The follow-up commit closes a gap I found re-reading my own diff before you got to it. The name heuristic covered credential-shaped scalars but recursed straight past credential-shaped containers, so credentials: { user, pass } and tokens: ["..."] still emitted plaintext — neither pass nor a bare array entry matches a secret word on its own. Suspicion now propagates into the subtree.

That introduces an asymmetry worth your judgement: a declared secret is masked wholesale (the author said so explicitly), while a merely suspected one keeps its structure and has only its string leaves masked. A side effect is that credentials.user — a username, not a secret — now comes back masked. I decided that is the right trade for a container the host is only guessing about, and it is round-trip-safe either way, but tell me if you would rather it were narrower.

Everything in my original review request still stands, in particular item 1 (is the three-route masking boundary actually complete?) and item 2 (heuristic breadth — now with more reach than when I first asked).

[allyblockcast]

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: ## Verification
• 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 (direct in-run analysis; nested CLI skipped per Kubernetes policy).
Reviewed head: 0417f17

Critical Issues (0)

Important Issues (3)

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/plugin-config-masking.ts:198 — Schema-declared secrets can still be emitted from valid array, dynamic-property, and composed schemas. collectSecretBearingPaths only descends through properties; it never follows items, prefixItems, additionalProperties, or patternProperties, and composition traversal does not evaluate a secret marker on the branch node itself. For example, a targets.items.properties.value field marked writeOnly: true, or a property whose oneOf branch is writeOnly, is absent from secret and its plaintext reaches the response. Replace flattened property-only paths with schema-aware traversal alongside the runtime value, including array item and composition-node declarations, and add leak assertions for each supported JSON Schema shape.

• [native-codex] server/src/services/plugin-config-masking.ts:281 — The new credential-container recursion handles direct strings and records but returns nested arrays unchanged. { tokens: [["live-secret"]] } therefore emits the plaintext even though tokens is explicitly classified as credential-bearing. Recursively process array entries at arbitrary depth while carrying the suspect state, and add a nested-array leak test.

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/plugin-config-masking.ts:344 — Masked array secrets are restored by positional index, which can silently move a credential to another target after deletion or reordering. If stored entries are [A(tokenA), B(tokenB)] and the operator removes A from the masked form, B's sentinel is restored from storedArray[0], persisting tokenA under B's endpoint and handing that mismatched pair to the worker. Restore only through stable element identity, or reject masked array sentinels when structure/order changed and require explicit re-entry; add deletion and reorder tests.

Strengths

• The GET and config-test authority tightening closes the original board-member trust-boundary failure.
• The save path restores masks before validation and secret-ref extraction, and masks the persisted-row response.
• Route tests verify storage state and worker RPC payloads rather than checking response shape alone.

Recommended Action

1. Fix the three Important issues before merge and add regression coverage for array item schemas, credential arrays, and array deletion/reordering.
2. This PR is authored by app/allyblockcast; the exact reviewed head must be reopened under an independent author before the allyblockcast App can provide the gate-authorizing approval. The shared merge-token user is not a substitute for that App review.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (4)

• [pr-review-toolkit/gstack/native-codex] server/src/services/plugin-config-masking.ts:198 — Schema-declared secrets inside array items are not discovered, and runtime paths include numeric indexes that cannot match property-only paths. For example, accounts[].value with writeOnly: true returns value in plaintext because items is never traversed and value does not trigger the name heuristic.
  • Traverse items/prefixItems with schema-aware wildcard paths (or mask by walking schema and value together), then add unit and route tests for declared secrets in arrays of objects and primitive arrays.
• [gstack] server/src/services/plugin-config-masking.ts:265 — Every UUID string at every declared-secret path is preserved as a legacy pointer, including ordinary fields marked only writeOnly: true or x-paperclip-secret: true. A UUID-shaped password, token, or API key is therefore emitted verbatim.
  • Preserve bare UUIDs only when the corresponding schema node specifically declares format: "secret-ref"; mask UUID values for the other secret declarations and add regression coverage.
• [pr-review-toolkit/gstack/native-codex] server/src/services/plugin-config-masking.ts:340 — Mask restoration correlates array entries only by index. If stored targets are [A(secretA), B(secretB)] and an operator deletes A or moves B first, B's sentinel restores secretA; the wrong credential is then persisted and sent to the worker.
  • Restore masked array entries using stable item identity, or reject masked array updates when unchanged identity/order cannot be proven. Cover insertion, deletion, and reordering.
• [pr-review-toolkit/native-codex] server/src/routes/plugins.ts:2773 — /config/test sends restored plaintext to the worker and returns warnings/errors verbatim. A validator that includes config.webhookToken in a diagnostic re-emits the stored credential through the API response.
  • Sanitize worker diagnostics at this boundary against the restored secret values, and add a route test where validateConfig reflects a secret in both warning and error output.

Strengths

• The instance-admin gate is applied consistently to read, write, and test routes.
• Save responses are masked, and the merge occurs before schema validation, secret-ref extraction, persistence, and worker notification.
• The focused mutable-store tests meaningfully exercise the ordinary unchanged masked round-trip.

Recommended Action

1. Fix the four Important issues before merge.
2. Add the missing array mutation, array-schema, UUID credential, and reflected-diagnostic regressions.
3. This PR is authored by app/allyblockcast, so the Ally App cannot review its own PR. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; do not substitute the shared merge-token user for that App evidence.

[allyblockcast]

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: ## Verification
• 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 (direct in-run analysis; nested CLI skipped per Kubernetes policy).
Reviewed head: 5b5de18

Prior Findings Dispositioned (4)

• prior:0417f17 important 1 — still-present — server/src/services/plugin-config-masking.ts:198 — The exact-head walker still follows only properties and composition branches, not items, prefixItems, additionalProperties, or patternProperties; the runtime array branch at line 281 also stops at nested arrays.
• prior:0417f17 important 2 — still-present — server/src/services/plugin-config-masking.ts:265 — Every UUID string at any declared-secret path is still preserved, even when the declaration is only writeOnly or x-paperclip-secret rather than format: "secret-ref".
• prior:0417f17 important 3 — still-present — server/src/services/plugin-config-masking.ts:340 — Masked array values are still restored from stored values by positional index, with no identity or structure check.
• prior:0417f17 important 4 — still-present — server/src/routes/plugins.ts:2773 — The worker still receives restored plaintext and its warnings/errors are returned verbatim at lines 2781-2790.

Critical Issues (0)

Important Issues (4)

• [prior:0417f17 important 1; pr-review-toolkit + gstack/review + native-codex] server/src/services/plugin-config-masking.ts:198 — Schema-declared secrets can still escape masking in valid array and dynamic-property schemas. The collector never traverses items, prefixItems, additionalProperties, or patternProperties, and a secret marker on a composition branch node itself is not evaluated. Separately, the runtime array branch at line 281 returns nested arrays unchanged, so a credential-shaped container such as tokens: [["live-secret"]] also leaks.
  • Walk the runtime value together with its applicable schema, including composition nodes, dynamic properties, and arbitrarily nested arrays. Add response-level leak tests for each supported shape.
• [prior:0417f17 important 2; gstack/review + native-codex] server/src/services/plugin-config-masking.ts:265 — UUID-shaped plaintext credentials bypass masking. The code preserves any UUID at every declared-secret path as a legacy binding, so a UUID API key in a writeOnly or x-paperclip-secret field is returned verbatim.
  • Preserve bare UUIDs only when the effective schema declaration is specifically format: "secret-ref"; mask UUID values for other secret declarations and add regressions for both markers.
• [prior:0417f17 important 3; pr-review-toolkit + gstack/review + native-codex] server/src/services/plugin-config-masking.ts:340 — Positional array restoration can silently attach the wrong credential to an entry. Deleting, inserting, or reordering masked objects restores secrets from the old numeric index, then persists and sends the mismatched endpoint/credential pair to the worker.
  • Restore only through stable item identity, or reject masked array updates when unchanged identity and order cannot be proven. Cover insertion, deletion, and reorder cases.
• [prior:0417f17 important 4; pr-review-toolkit + gstack/review] server/src/routes/plugins.ts:2773 — /config/test can reflect a restored secret through worker diagnostics. validateConfig receives plaintext, while its arbitrary warnings/errors are joined and returned to the client without redaction.
  • Treat worker diagnostics as untrusted output: redact the restored secret values before responding, or expose bounded host-defined diagnostics. Add a route test where both a warning and an error echo the credential.

Strengths

• The read, write, and test routes now consistently require instance-admin authority.
• Mask restoration occurs before validation, secret-ref extraction, persistence, and worker notification.
• The mutable-store route tests verify persisted state and worker RPC payloads rather than response shape alone.

Recommended Action

1. Fix the four Important issues before merge and add the missing leak/corruption regressions.
2. This PR is authored by app/allyblockcast; the Ally App cannot review its own PR. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; the shared merge-token User review is not gate evidence.

[kkroo]

Reviewed plugin config secret masking/admin boundary. PR body repaired and review gate rerun clean.