fix(plugins): close plugin-config masking gaps for duplicate identities and conditional schemas (BLO-26530)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Plugins hold their own instance config in plugin_config.config_json, and that row routinely contains a live credential — the production Alertmanager bearer among them
• BLO-20794 found the operator-facing config API returned that row verbatim, so a generic read-side masking boundary was built (fix(plugins): prevent secret-ref diagnostic reflection (BLO-20871) #1221): mask secret-bearing values on the way out, restore them on the way back in so an unmodified round-trip is lossless
• A paranoid re-review of that merged boundary found two ways a credential still escaped it — one that clones a secret onto an operator-supplied endpoint, and one that returns it as plaintext outright
• Both stem from the same root shape: the boundary trusted a partial proof. Uniqueness was checked on one side of a comparison, and the schema walk covered some applicator keywords but not others
• This pull request closes both, and converts the schema-keyword handling from an implicit denylist into an explicit allowlist so the next unhandled keyword fails closed instead of becoming a fresh hiding place
• The benefit is that a plugin credential can no longer be read by a non-admin, cloned onto an attacker-chosen endpoint, or hidden from the mask by an exotic-but-valid manifest

Linked Issues or Issue Description

• Path (A) is not available: this repo's issues live in Paperclip, not GitHub Issues. Primary issue — BLO-26530 (closed by this PR). The bug report itself is reproduced in full under What Changed below, with the exact failing input, so a reviewer has the same fields an issue template would supply.
• Refs parent BLO-20794 (asymmetric read/write authority on plugin config)
• Refs BLO-20871 — the masking boundary this hardens, landed via fix(plugins): prevent secret-ref diagnostic reflection (BLO-20871) #1221
• Related PRs from the dedup search: fix(plugins): prevent secret-ref diagnostic reflection (BLO-20871) #1221 (merged, parent work), fix(plugins): mask plugin-config secrets on read, lossless masked round-trip (BLO-20871) #968 (closed, earlier attempt), fix(plugins): make the masked config round trip concurrency-safe (BLO-26529) #1337 (open sibling — BLO-26529, masked round-trip CAS). No duplicate of this change exists.

What Changed

• mergeArray now proves a designated identity unique on both sides. x-paperclip-identity uniqueness was verified only among stored entries, so two posted entries claiming the same identity both matched the one stored entry and both had its credential restored:

  stored:  [{name:"alpha", url:"https://a",        token:"token-alpha"}]
  posted:  [{name:"alpha", url:"https://a",        token:"__redacted__"},
            {name:"alpha", url:"https://attacker", token:"__redacted__"}]
  before:  BOTH entries come back holding token-alpha
  after:   targets.0 + targets.1 unresolved -> route returns 400, storage untouched
  

  The second entry is operator-supplied, so this handed a live credential to an arbitrary endpoint.
• expandSchemaNodes now walks the conditional and dependent applicators. if / then / else / not / dependentSchemas / draft-07 dependencies apply to the same instance location and were skipped, so a writeOnly, format: "secret-ref" or x-paperclip-secret marker inside one was invisible and the field was returned as plaintext.
• contains and unevaluatedItems added to childNodesForIndex; unevaluatedProperties to childNodesForKey — the same gap at the array-entry and property levels. contains is applied to every index, since it says "at least one entry" without saying which.
• Unknown schema keywords fail closed. Keywords are now an allowlist of walked vs known-inert; a node carrying anything else ($dynamicRef, contentSchema, a future draft's keyword, or a typo) forces the name-heuristic's suspicion, masking every string leaf at and beneath it.
• Applicability is still deliberately not evaluated, matching the existing fail-closed treatment of oneOf branches: if is walked alongside then/else, and dependentSchemas without checking whether its trigger property is present.
• Tests: 51 unit cases in plugin-config-masking.test.ts (98 -> 118 in that suite) and 3 route cases in plugin-routes-authz.test.ts.
• A drift guard on the allowlist itself. TRAVERSED_SCHEMA_KEYWORDS is what exempts a keyword from the fail-closed guard, so a keyword listed there but not actually walked would be silently ignored rather than masked — the exact shape of this defect. A second commit pins one case per member of the set, hiding a marker so only that keyword can reach it, plus a completeness assertion tying the table to the set.

Why suspicion rather than a wholesale mask for unknown keywords

Plaintext is a string, so masking every string leaf fully satisfies "never emit plaintext". Collapsing the location to a single sentinel — the treatment a dangling $ref gets — would leave the operator no editable form and nothing for mergeMaskedPluginConfig to restore into, turning an unrecognised keyword into a config-availability outage. x- vendor extensions stay inert so a UI hint cannot mask a config, and an explicit x-paperclip-secret: false still wins, because the author spoke about that exact field.

Verification

Both defects were reproduced as failing tests against the parent commit before any fix was written.

cd server
npx vitest run src/__tests__/plugin-config-masking.test.ts \
               src/__tests__/plugin-routes-authz.test.ts \
               src/__tests__/plugin-config-validator.test.ts \
               src/__tests__/plugin-secrets-handler.test.ts
#   Test Files  4 passed (4)
#        Tests  218 passed (218)
npx tsc --noEmit -p tsconfig.json      # clean

Route tests assert the duplicate-identity write returns 400 with unresolvedMaskPaths, that upsertConfig is never called, that storage is unchanged, and that the credential appears in neither resulting entry.

Mutation-verified — every traversal and guard is load-bearing. Each mutation was applied in isolation, the suite re-run, then reverted:

mutation removed	failing tests
incoming-side identity uniqueness	2 unit + 1 route
if/then/else/not traversal	4
dependentSchemas/dependencies traversal	2
contains	1
unevaluatedItems	1
unevaluatedProperties	2
unknown-keyword fail-closed guard	5

No over-masking regression on any shipped manifest. An allowlist can only fail by being too narrow, so I measured it rather than assuming: for the real alertmanager, slack, linear and gbrain manifests I synthesized a config from each declared property, masked it under both master and this branch, and compared. Output is byte-identical, and every masked field is explained by a declared marker or the pre-existing name heuristic:

alertmanager: total=9  masked=2 declared-secret=1 byName=[webhookToken]
slack:        total=23 masked=3 declared-secret=3 byName=[]
linear:       total=12 masked=3 declared-secret=1 byName=[linearClientSecret, linearWebhookSigningSecret]
gbrain:       total=12 masked=1 declared-secret=0 byName=[gbrainOauthTokenUrl]

No shipped manifest uses any conditional keyword today, so this change is defense-in-depth. gbrainOauthTokenUrl masking by name heuristic is pre-existing on master, not introduced here.

Risks

• Low risk on today's manifests — behaviour is byte-identical for all four shipped plugins, per the comparison above.
• The allowlist is the one real risk. A manifest using a valid keyword I did not classify will over-mask its string leaves rather than leak them. That is the intended failure direction and it is recoverable — an unmodified round-trip still restores the value, so no data is destroyed — and the fix is one line in INERT_SCHEMA_KEYWORDS. I chose it over a denylist because a denylist fails open, which is how the original defect existed.
• contains over-masks by design. It governs at least one entry without saying which, so a contains-declared secret masks the same field on sibling entries. The alternative is emitting the entry it did govern in the clear.
• Stricter writes are a deliberate behavioural shift. An operator posting the same designated identity twice now gets a 400 instead of a silent restore. Previously that silently cloned a credential, so the 400 is the point.
• No schema/migration changes. No API shape changes. No UI changes.
• TRAVERSED_SCHEMA_KEYWORDS and the actual traversal code must stay in sync — a keyword listed as traversed but not walked would be silently ignored rather than failing closed. This is now an enforced invariant rather than a documented caution: the drift suite fails if a keyword is added to the set without a case proving the walk enters it (verified by appending an unwalked futureApplicator and watching the completeness assertion fail).

Model Used

• Claude Opus 4.6 (claude-opus-5[1m] as configured for this agent), 1M context, extended thinking enabled, with tool use and code execution (ran the test suite, the mutation matrix, and the manifest comparison directly).

Note for the reviewer

server/src/services/json-schema-secret-refs.ts:15 has the same limited traversal (allOf/anyOf/oneOf only), so a format: "secret-ref" field declared behind a conditional would not be discovered for binding sync. That is a functional gap, not a leak — masking now covers those fields — so I deliberately left it out of this diff rather than widen a security-boundary change, and filed it as a follow-up under BLO-20794.

Sibling #1337 (BLO-26529) touches routes/plugins.ts and two other test files. The only file we share is plugin-routes-authz.test.ts, where its hunks sit at lines ~50/~109 and mine are appended at the end, so the two merge cleanly.

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 surface touched
• I have updated relevant documentation to reflect my changes — the module contract lives in plugin-config-masking.ts's JSDoc, which is updated alongside the behaviour
• I have considered and documented any risks above
• All Paperclip CI gates are green — not yet; will confirm before requesting merge
• 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

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-20794
🔗 Paperclip issue: BLO-26529
🔗 Paperclip issue: BLO-26530

[allyblockcast]

🔗 Paperclip issue: BLO-20794
🔗 Paperclip issue: BLO-26529
🔗 Paperclip issue: BLO-26530

[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: ## 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 please review at head 3b108b7ea0a9969a9b05c9605e1364969e369e97.

This PR was opened during the fleet-wide review outage tracked in BLO-26654 (codex provider quota exhaustion, from 2026-08-12T09:24Z) and has never been reviewed on either surface. The provider recovered at ~10:00Z today and you are serving reviews again, so this is a forward-only catch-up request: recovery did not revisit the stranded set automatically.

Review focus: normal full pass at the head above. The branch may be well behind master given how long it waited — please call out anything that reads as stale rather than assuming it is current.

[allyblockcast]

@ally please review at head 3b108b7 — plugins: masking gaps for duplicate identities and conditional schemas. Focus on any remaining path that can emit an unmasked secret.

Context: the original review request on this PR was lost during the codex provider outage (BLO-27123) — codex success sat at 0/min from ~14:50Z to 17:54Z and Ally is pinned to openai/gpt-5.6-terra on that pool. Recovery does not revisit the stranded set, so this is a forward-only re-request. Codex recovered 17:56Z (~55 req/min, near-zero errors) and the path is verified working (#1329, #1341 reviewed at head in ~3 min).

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Incoming-side identity uniqueness prevents one stored credential from being restored onto duplicate operator-supplied entries.
• Conditional, dependent, array, and unevaluated schema applicators are traversed with focused round-trip and route-level coverage.
• The explicit walked/inert keyword allowlist fails closed for unrecognized schema keywords, and the drift tests protect the traversal inventory.
• The current PR checks are green, including build, typecheck, server test shards, and review.

Recommended Action

1. No Critical or Important issues found; the implementation is suitable for merge from the reviewed head.