fix(mcp-market): isolate registry header variables from URLs - #706
Conversation
Registry header declarations must not authorize substitution in URL paths or queries. Select placeholder rules per field while preserving legacy URL and stdio tokens, header-local defaults and fixed values. Cover the same-name boundary through registry mapping, resolution and real host persistence, following the review on vastsa#646.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new ADR is missing from the Chinese ADR index, leaving the documented decision map out of sync.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR fixes MCP Registry header-variable mapping so {name} credentials resolve only in headers while URL templates retain ${UPPERCASE} behavior.
Changes:
- Adds header-scoped editable/fixed bindings and collision-safe inputs.
- Preserves literals and URL scope during resolution and persistence.
- Adds regression, E2E, ADR, and specification coverage.
| File | Description |
|---|---|
packages/shared/src/mcp-registry.ts |
Maps registry header variables into scoped bindings. |
packages/shared/src/mcp-catalog.ts |
Resolves header and URL templates with separate matchers. |
packages/shared/src/mcp-registry.test.ts |
Covers registry mapping and isolation cases. |
packages/shared/src/mcp-catalog.test.ts |
Covers catalog binding validation and resolution. |
scripts/e2e-mcp-market.mjs |
Adds host persistence scope validation. |
docs/spec/06-delivery/04-e2e-test-plan.md |
Documents new E2E scenarios. |
docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md |
Mirrors E2E documentation. |
docs/adr/registry-header-variable-spelling.md |
Records the design decision. |
docs/adr/README.md |
Adds the ADR to the English index. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | 0298 | [The app ships no fonts](0298-remove-bundled-fonts.md) | Accepted (D598; amends ADR 0083 / D232) | | ||
| | turn-process-and-thinking-display | [Turn process and thinking presentation](turn-process-and-thinking-display.md) | Accepted | | ||
| | provider-display-order | [Provider display order](provider-display-order.md) | Accepted | | ||
| | registry-header-variable-spelling | [Remote header variables accept the registry's `{name}` spelling](registry-header-variable-spelling.md) | Proposed | |
vastsa
left a comment
vastsa
left a comment
There was a problem hiding this comment.
Thanks for the focused registry-boundary fix. I found one blocking scope issue in packages/shared/src/mcp-catalog.ts (the collect/fill paths around the header-binding handling): when headerBindings contains bindings for one header but another header has {token} text without its own binding map, the latter falls back to the global requiredEnv set and can substitute a credential declared for the first header. That can leak a header credential into another header.
Please make unbound header tokens literal whenever header bindings are in use (or reject partial binding maps), and add a regression covering two headers with the same token plus a partial headerBindings map. The current DIRTY merge state and missing current code CI are additional blockers. Thanks.
Keep the proposed registry header ADR alongside the upstream certificate\ndecision while incorporating the current base without rewriting the\npublished request branch.
Treat the presence of header binding metadata as an explicit scope for all headers, so a missing per-header map cannot consume another header input or default through the legacy global fallback. Cover partial and empty maps in discovery and resolution, preserve legacy catalogs, and verify isolated values through real host persistence.
Include the latest upstream base so the validated request meets the repository ancestry gate without rewriting the published branch.
|
Addressed the blocking partial-map scope issue in Added seven native cases: six reproduce failures on the previous resolver and all pass with the fix, including the two-header/same-token partial-map case, empty maps and an undeclared token that must not be collected. Current validation: 461 shared source tests, shared typecheck/build, 9 desktop market tests, a separate 12-case scope matrix and 5/5 real-host protocol E2E. The new E2E checks resolution → upsert → list → persisted config with disabled remote entries and synthetic values. The ADR-index conflict is resolved without dropping upstream content or rewriting branch history. GitHub now reports The current-head CI, Docs check and PR base runs were triggered but are |
Registry headers using
{name}now resolve through header-local bindings while URL templates retain their existing${UPPERCASE}behavior. Inputs, defaults, optional flags and fixed literals stay scoped to their declaring header. The host protocol and persisted config format are unchanged.When
headerBindingsis present, even as an empty or partial map, unbound tokens in every header remain literal. For example, with onlyAuthorization: Bearer {token}bound,X-Unbound: {token}stays literal instead of receiving the Authorization credential. Catalogs with no binding metadata retain their legacy behavior. Placeholder discovery and resolution use the same rule.This completes the scope fixes requested in #646 and the blocking review on this PR. The branch incorporates current main
ec944a7150c4498a1b51d174daec3d9180e55b31without rewriting the published history; the ADR-index conflict keeps both upstream and proposed entries.Validation on
30559d28cda1e364e3e89c656c2cad3683c483f5(the final base refresh leaves the separately checked shared, Rust host and desktop market sources identical; all five protocol E2E cases were rerun on this head):cargo build -p host-core --locked --offline; protocol E2E 5/5 passed, including partial bindings through resolution,mcp.upsert,mcp.listand disk persistence.The remote HTTP fixtures use synthetic values, disabled servers and temporary storage. No live MCP service, model, production credential or Electron UI was exercised. Local checks do not replace current-head upstream CI or maintainer approval. Implementation and tests used AI assistance and a separate automated code review; no human-review claim is made.