Repository navigation
Conversation
|
Hi @a-effort — same two process asks as on #6772, if you have a moment:
Happy to turn any review feedback around quickly. Thanks! |
AmirF194
left a comment
There was a problem hiding this comment.
Read compute_passthrough_headers_cached in mcpgateway/utils/passthrough_headers.py to confirm the None-vs-empty-list contract this fix relies on: effective_allowed = gateway_passthrough_headers if gateway_passthrough_headers is not None else allowed_headers, so None inherits the global allowlist and [] is an explicit override that forwards nothing.
The same coercion this PR removes at tool_service.py:1351 still exists for A2A agents, at tool_service.py:6815 on this head:
a2a_allowlist = a2a_agent_passthrough_headers or []a2a_agent_passthrough_headers is read straight from a2a_agent.passthrough_headers a few lines above (an Optional[List[str]] DB field, None by default), so an A2A agent with no explicit allowlist gets a2a_allowlist = [], which is then passed as gateway_passthrough_headers=a2a_allowlist into compute_passthrough_headers_cached. That reads as an explicit empty override, so the global passthrough headers never reach an A2A tool call unless the agent has its own non-empty list configured, the same bug this PR's title and tests describe for MCP gateway tools, just at a different call site.
Fix would be the same shape as this PR's own change: a2a_allowlist = a2a_agent_passthrough_headers (keep None distinct from []), since compute_passthrough_headers_cached already handles None correctly.
I did not run this, it is a direct read of the same function this PR's own tests already exercise, not a new execution claim.
|
The A2A coercion at |
fd4ecc9 to
a974543
Compare
|
Rebased onto current
The source fix is unchanged. The fail-before/pass-after check was re-run on the rebased tree, and the affected suites pass (1056 passed, 1 skipped). CI workflows still need maintainer approval to run. |
|
@a-effort could you approve the workflows and triage if you have time? Thanks! |
|
This fix matches the root cause we reproduced in our ContextForge deployment. We operate an MCP gateway in front of a per-user SugarCRM connector. With a global passthrough allowlist and an unset gateway-level allowlist, no headers reached the upstream connector. Adding the same list directly to the gateway made forwarding work, which confirms that the cache payload was converting an unset value into an explicit empty override. The proposed change preserves the required contract:
This is compatible with our SugarCRM connector and removes the need to duplicate the identity-header allowlist onto every gateway. We will apply this commit to our ContextForge fork and run the affected regression tests plus the end-to-end header-forwarding test. One related review observation appears out of scope for this PR: the A2A path may contain a similar From our side, the MCP gateway fix looks correctly scoped and addresses the production behavior described in #6963. |
Preserve null gateway allowlists in cached tool payloads. Unset allowlists inherit the global configuration; explicit empty lists block forwarding. Cover inherited, empty, and explicit allowlists through real MCP calls. Verify disabled forwarding, unauthenticated requests, and insufficient token permissions. Closes IBM#6963 Signed-off-by: Michael Holroyd <meekohi@gmail.com> Signed-off-by: Michael Holroyd <michael.holroyd@ga-ccri.com> {"fundingSource": "RD1191", "team": "CTO", "fshGit": "d3463a9a", "fshDocker": "sha256:62e82fe2"}
a974543 to
aaaf3fc
Compare
|
Updated this PR to sync with the latest main branch. |
Summary
MCP gateway tools drop globally allowed headers when their gateway has an unset header allowlist. Preserve inheritance while retaining explicit overrides.
Reproduction Steps
Enable header passthrough and configure a global allowlist. Register an MCP gateway without a gateway allowlist. Invoke its tool with an allowed header; the upstream does not receive it.
Closes #6963.
Root Cause
ToolService._build_tool_cache_payload()convertsgateway.passthrough_headers=Noneto[]. The forwarding policy interprets any list as a gateway override.Fix Description
Preserve
Nonein cached payloads. Unset allowlists inherit global settings; explicit empty lists block forwarding. A2A behavior remains outside this change.Verification
Regression coverage includes real MCP calls, repeated tool lookups, disabled forwarding, unauthenticated requests, and insufficient token permissions.
A2A allowlist behavior remains outside this change.
Validation: focused regressions, six black-box cases, static checks, secret scanning, package verification, and production image construction pass.
Production MCP/RBAC E2E has 129 passing cases after correcting local fixture connectivity and installing Chromium; 14 feature-specific cases skip.
The full suite has 24,519 passes. Seven upstream setup errors resolve against the correct isolated upstream.
Doctests have 1,241 passes; overall coverage is 98%.
Validation exceptions: three A2A JWT-fixture failures reproduce on main and remain outside this PR.
Full Bandit reports 12 unchanged findings outside the diff.
Coverage also exposes a validator-cache test-order failure reproduced on main and a timing failure that passes independently.
These existing failures require exceptions to the full repository gate.