fix(go-adk): forward allowedHeaders from incoming request to MCP tool calls

[mesutoezdil]

Closes #1679

The allowedHeaders field on McpServer was being populated by the translator and stored in HttpMcpServerConfig/SseMcpServerConfig, but the Go runtime never used it. Headers from the incoming A2A request were silently dropped even when the operator explicitly listed them in allowedHeaders. The Python runtime handled this correctly already.

What changed:

• allowedRequestHeaders helper added to registry.go. It reads the a2asrv.CallContext already present in the Go context and returns only the header values whose names appear in the configured allowedHeaders list (case-insensitive). No intermediate context key, no extra copy.
• headerRoundTripper gets an allowedHeaders field. On each outbound MCP HTTP request the transport calls allowedRequestHeaders and sets the matching headers. Static headers from the server spec are applied last so they always take precedence, mirroring the Python runtime behaviour.
• CreateToolsets now passes AllowedHeaders from each MCP server config into the transport params.
• executor.go no longer needs to touch headers at all. The A2A server already stores the CallContext in the Go context before Execute is called, so the round tripper picks it up automatically.

The change is fully backwards compatible. If allowedHeaders is empty, no extra headers are forwarded.

Tests use a2asrv.NewRequestMeta and a2asrv.WithCallContext to build the test context, matching what the real A2A server does. Coverage includes: normal forwarding, static-header-wins override, no A2A context, non-allowed header filter, and empty allowed list.

Signed-off-by: mesutoezdil mesudozdil@gmail.com

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[jmhbh]

header forwarding changes lgtm just curious about the bedrock changes in this PR since they aren't explicitly tied to the issue at hand, do we have a separate issue for that?

[mesutoezdil]

Good catch on both counts. @jmhbh

The Bedrock changes crept in because this branch was cut from the wrong base. Rebased onto main so only the allowedHeaders fix is here now, single clean commit.

On the nit: removed the private withRequestHeaders wrapper and inlined the header extraction directly in Execute alongside withBearerToken. The exported mcp.WithRequestHeaders is kept since registry_test.go needs it to set up context state for the round-tripper tests, but the extra layer in executor.go is gone.

[EItanya]

Why are we moving these values from the original context value into a custom one, what value does that bring us? I'd rather just create a helper function in registry.go that pulls these values, what do you think?

[mesutoezdil]

Done. Replaced the WithRequestHeaders/requestHeadersFromContext approach with a single allowedRequestHeaders helper in registry.go that reads straight from the a2asrv.CallContext already present in the Go context. No intermediate copy, no exported helper needed. Tests use a2asrv.NewRequestMeta + a2asrv.WithCallContext to build the test context, matching what the real server does.

[mesutoezdil]

I've now updated the top section as well. @EItanya @jmhbh

[jsonmp-k8]

Bug: allowedRequestHeaders reinvents RequestMeta.Get() poorly

The entire inner loop is unnecessary and introduces bugs:

for _, name := range allowed {
    lower := strings.ToLower(name)
    for key, vals := range meta.List() {
        if strings.ToLower(key) == lower && len(vals) > 0 && vals[0] != "" {
            result[key] = vals[0]
            break
        }
    }
}

RequestMeta.Get(key) already does case-insensitive O(1) lookup (it lowercases keys at construction in NewRequestMeta, and Get lowercases the lookup key). This loop is O(n*m) for no reason. Worse, since List() yields already-lowercased keys, result[key] stores the lowercased form — so req.Header.Set("authorization", v) is what actually happens. Go's Header.Set canonicalizes it so it works by accident, but it's still wrong in intent.

Replace the entire function body with:

func allowedRequestHeaders(ctx context.Context, allowed []string) map[string]string {
    if len(allowed) == 0 {
        return nil
    }
    callCtx, ok := a2asrv.CallContextFrom(ctx)
    if !ok {
        return nil
    }
    meta := callCtx.RequestMeta()
    if meta == nil {
        return nil
    }
    result := make(map[string]string)
    for _, name := range allowed {
        if vals, ok := meta.Get(name); ok && len(vals) > 0 && vals[0] != "" {
            result[name] = vals[0]
        }
    }
    if len(result) == 0 {
        return nil
    }
    return result
}

This preserves the original casing from the allowedHeaders config, does O(n) instead of O(n*m), and avoids returning an empty map (the callers range over it, so nil is cleaner).

Missing: no test for case mismatch between config and request

The PR claims case-insensitive matching but none of the tests exercise it. Add a test where allowedHeaders says "authorization" but the A2A request sends "Authorization" (or vice versa). Without this, the case-insensitivity claim is untested.

Missing: no test for multi-value headers

RequestMeta stores []string per key. The PR takes vals[0] unconditionally, silently dropping all subsequent values. This is probably fine for headers like Authorization, but the PR should document this choice (first value wins) and test that a multi-value header only forwards the first value. If a header like X-Forwarded-For gets multiple values, dropping all but the first is data loss.

Concern: context propagation is assumed, not verified

The entire approach relies on the A2A CallContext surviving from the executor through the Google ADK runner, through the agent invocation, through the MCP tool call, all the way down to headerRoundTripper.RoundTrip(req). The PR doesn't verify this end-to-end. The unit tests construct the context manually with a2asrv.WithCallContext — they don't prove the real request path propagates it.

This is the kind of thing that works in tests and breaks in production when some middleware swaps the context. The PR description says "The A2A server already stores the CallContext in the Go context before Execute is called, so the round tripper picks it up automatically" — but "automatically" is doing a lot of work there. An integration test that actually sends an A2A request and verifies the MCP server receives the forwarded header would catch this.

Nit: empty map allocation when no headers match

If all allowed header names are present in the config but none appear in the request metadata, the function allocates a map[string]string and returns it empty. Callers range over it harmlessly, but nil is more idiomatic and avoids the allocation.

Tests: no t.Parallel()

All tests spin up real HTTP servers. They should call t.Parallel() to avoid unnecessarily serializing the test suite.

[mesutoezdil]

Thanks for the thorough review @jsonmp-k8. Applied the fixes in 76e7c1a:

• allowedRequestHeaders now uses RequestMeta.Get for an O(1) case-insensitive lookup instead of iterating meta.List. Result keys preserve the casing from the allowedHeaders config rather than the lowercased form from List. Returns nil instead of an empty map when nothing matches.
• strings import removed since it is no longer needed.
• New test TestAllowedRequestHeaders_CaseInsensitiveLookup exercises both directions (config lowercase vs incoming capitalized, and the reverse). Multi-value test locks in the first-wins behaviour. Empty-match test verifies the nil return.
• t.Parallel added to every test function and subtest.

On the end-to-end context propagation concern: agreed that the unit tests do not prove the CallContext survives from executor through the ADK runner down to the round tripper. The current e2e suite does not have a hook that sends an A2A request and asserts headers on the MCP server side, so I did not add one here. Happy to follow up with an e2e test in a separate PR if the maintainers think the scope fits, otherwise this relies on the a2asrv runtime contract that the CallContext is stored in ctx before Execute is called.