fix(serve): forward caller identity on every backend method by avelino · Pull Request #112 · avelino/mcp

avelino · 2026-09-17T17:13:15Z

Review fixes on top of #110. Two blockers, four corrections, two nits, and the test that was missing on the seam between the two halves of the feature.

Extends #110 (luisaniceto-infra/mcp:feat/forward-caller-identity).
Part of #109.

This targets the feature branch, not main. It does not close #109 on its own, #110 does that when it lands. Nothing here is a new feature, it is the review of #110 applied as code instead of left as a comment thread.

Changes

The refusal skipped the audit funnel. dispatch_request builds one response and finish_audit stamps every one of them, which is why the other three early returns in that function all go through it. The identity refusal returned directly instead, so a call the ACL had already allowed, stopped by the proxy, produced no audit entry, no OTel metric and no 2026-07-28 result envelope. It now leaves as Err and flows into the same match resolved as everything else. The audit attribution moved above the check too, otherwise the entry came back without naming the tool.

A static headers entry of the same name was not checked. build_request applies the identity headers first and the server's configured headers after, and reqwest's builder calls HeaderMap::append, not insert (reqwest-0.12.28/src/async_impl/request.rs:223). So headers: {"X-MCP-Subject": "svc"} together with forward_identity.header: "X-MCP-Subject" sent two lines of the same field, and the backend picked the winner. Go's Header.Get takes the first, several proxies take the last. #110 already refuses roles_header == header on the argument that a duplicate is resolved by the receiver and that decision is not ours. Same rule, and this is the collision an operator actually hits when migrating off a hardcoded subject header.

Identity only rode on tools/call. resources/read and prompts/get reach the backend too. A resource read is where per-user data comes back, so a per-user backend was answering every caller as the service account while the docs promised "per request". Rather than growing *_with twins, request_raw, read_resource_raw and get_prompt_raw now take extra_headers the way call_tool_raw_with does. The twins would have been dead code, since this crate is binary only.

An unauthenticated caller was forwarded as anonymous. With no serverAuth.providers, build_auth_provider returns NoAuth and every subject is the literal anonymous, which is the documented default for stdio and local dev. The backend, which trusts this header by contract, was recording that as a person. Now refused, with ANONYMOUS_SUBJECT and AuthIdentity::is_anonymous() so it is recognizable rather than a loose string. Servers that did not opt into forward_identity are untouched and still work unauthenticated.

mcp update erased the field. merge_entry preserves by allowlist, and neither forward_identity nor tool_acl was on it, so mcp update <server> silently handed back a weaker config. tool_acl has been losing ACL overrides this way since before #110, so that half is a pre-existing bug, not a regression from this feature.

Nits. Two error strings said forwardIdentity, and the config key is forward_identity. The x_mcp_header module went back to private, with pub(crate) use x_mcp_header::is_tchar exporting only the predicate serve::proxy actually needs instead of HeaderParam and headers_for riding along.

Dedup pass. call_tool_raw was a copy of call_tool_raw_with with the Mcp-Param-* extraction repeated, now one line. The refusal message lived in three places and is now in identity_refusal, for the same reason #110 promoted is_tchar to shared: two copies is how one of them drifts.

Tests

cargo test               839 passed, 0 failed   (was 825)
cargo fmt --all --check   clean
cargo clippy              no new warning

The seam had no test. identity_headers and the header merge were each well covered alone, so passing &[] from dispatch kept all 825 green. The new e2e tests assert on what reached the stub backend, on all three methods. Checked by sabotage: swapping &identity_headers for &[] turns them red immediately.

Also covered: the refusal appears in the audit log with tool and server, none of the refusals let the call reach the backend, and a config written before #110 loads unchanged and forwards nothing.

Docs

forward_identity only existed in the config reference. The authentication guide is the one page about caller identity and said nothing about what the proxy does with it on the way out, so it has a section now, with a cross-reference from proxy mode. cli.md lists the fields mcp update preserves.

Not fixed here

forward_identity inside a stdio server still deserializes without complaint and becomes a no-op. The enum is untagged with no deny_unknown_fields anywhere in the repo, and warning about it needs the raw JSON at boot, which does not survive deserialization. Left as it was.

Follow-ups worth their own issue

No issue exists for either of these today.

mcp update preserving by allowlist means any new user-settable field is erased until someone remembers to add it. Inverting it to a denylist is the real fix, and it is independent of this feature.
main's CI has been red on cargo clippy -- -D warnings for result_large_err in dispatch.rs client_or_connect since dcd75dc. Not from this branch, and the check stays red until it is handled. Boxing JsonRpcResponse or an #[allow] like the one on resolve_tool_call are both on the table.

Checklist

Tests added/updated
Docs updated
Follows project conventions
No breaking change

…rver The proxy authenticates the caller and enforces the ACL, then talks to every backend with one credential resolved at startup. That is the right model when the backend's data has no per-row owner — Sentry, Slack, Databricks: the shared token IS the service account, and the proxy decides who may reach it. It stops being the right model when the backend's data is owned per user: an internal app catalogue, a notes service, anything that records an author. There a shared credential makes every write land under the same name, and it does not fail loudly — it records the wrong owner, silently. New per-server option on HTTP backends: "hub": { "url": "http://hub.internal/mcp", "headers": { "Authorization": "Bearer ${HUB_SERVICE_TOKEN}" }, "forward_identity": { "header": "X-MCP-Subject" } } The subject travels per request, from the identity the proxy already resolved in dispatch. `roles_header` is optional and off by default — most backends only need to know who, and sending roles a backend does not read is avoidable exposure. Scoped deliberately: - HTTP backends only. A stdio/CLI backend is a process the proxy owns, and there is no per-request channel to carry a header into a long-lived stdin pipe. - Opt-in per server, never on by default: a backend that never asked for identity must not start receiving one on upgrade. - The forwarded header WINS over a backend's own `x-mcp-header` annotation with the same name, and the argument-derived one is dropped rather than appended. Otherwise a tool could declare an argument mapped to `X-MCP-Subject` and let the caller pick their own subject — privilege escalation, tested. - A subject or role containing a control character REFUSES the call instead of falling back to the shared credential: falling back is the silent-wrong-owner outcome this feature exists to remove. - Header names are validated with the same `is_tchar` predicate the `x-mcp-header` path already uses (now `pub(crate)`) — two copies of a header-injection check is how one of them drifts. The safety condition the proxy cannot verify is documented on the type, in the config reference and in the code: the backend must not be reachable by anyone who could set the header themselves, or the header is a complete authentication bypass. That is why this is opt-in. Tests: 8 unit tests on the header builder (no config, stdio backend, unknown server, default and custom header names, roles, CRLF in subject, control char in roles, four malformed header names) and 2 client tests (shadowing refused, additive behaviour unchanged). Deny cases outnumber allow cases, per CONTRIBUTING's checklist for trusted-header changes. Full suite: 822 passing.

Four findings from review, all of them real. Two were mine to own. **1. Displaced doc comments took `#[allow]` with them — this is what broke CI.** Inserting `identity_headers` between `resolve_tool_call`'s doc block and the function moved BOTH the doc and `#[allow(clippy::result_large_err)]` onto the new function. `resolve_tool_call` lost its allow, so `cargo clippy -- -D warnings` failed on it. The same mistake sits in client.rs and review did not catch it: the MRTR doc block belonged to `call_tool_raw` and ended up on `call_tool_raw_with`, leaving `call_tool_raw` undocumented. Both restored. This also corrects the PR's claim that the two `result_large_err` errors pre-existed on main. Only ONE does — main's CI has been red since 2026-08-26 on `dispatch.rs` `client_or_connect`. The `proxy.rs` one was mine. Verified against main's own CI log, not a local run: the local run used `--all-targets` without `-D warnings`, so it printed them as warnings and I read both as inherited. **2. `roles_header` equal to `header` is now refused.** Two same-named headers with unrelated values leave the choice to the receiver, which could read the roles value as the subject. Refused for the same reason as a control character. Checked in `identity_headers` and not at load time because the existing config validation only warns at boot, and a warning would let the ambiguous call out. **3. The shadowing threat was overstated, and the test was vacuous.** `x-mcp-header` annotations always emit `Mcp-Param-{Name}`, so a tool annotating `X-MCP-Subject` produces `Mcp-Param-X-MCP-Subject` and cannot claim the identity header by name — the prefix isolates it with no help from the filter. There is no privilege escalation through that path, and the doc, the docs page and the PR body all claimed there was. `forwarded_identity_cannot_be_shadowed_by_x_mcp_header` asserted only that the forwarded subject was the value under `x-mcp-subject`, which was true for that structural reason — it passed with the filter deleted. Replaced by two tests: one documenting the prefix guarantee (and saying plainly that it does not exercise the filter), one exercising the only reachable collision, which is an operator configuring `header` under the `Mcp-Param-` prefix. **4. Doc said `forwardIdentity`; the field is `forward_identity`.** Fixed. Also translated the Portuguese comments and assertion messages this branch introduced — the rest of the codebase is in English and mixing the two is noise for a reader. Verified in a rust:1-slim container: cargo fmt --all --check clean cargo clippy -- -D warnings 1 error, the pre-existing dispatch.rs one cargo test 825 passed; 0 failed The three new tests were mutation-checked — removing the collision filter or disabling the roles_header check fails exactly them, and nothing else. The prefix-guarantee test keeps passing under those mutations, which is the point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The refusal path returned early and skipped finish_audit, so a call the ACL allowed and the proxy then stopped left no audit entry, no metric and no result envelope. Identity also rode only on tools/call, which let a backend that owns data per user answer resources/read as the service account. Refusals now leave as Err and go out through the same funnel as every other response. read_resource_raw and get_prompt_raw take extra headers the way call_tool_raw does, so all three backend methods carry the subject. Two more things get refused for the reason roles_header equal to header already was: an unauthenticated caller, whose subject is the placeholder anonymous, and a static headers entry sharing a name with a forwarded one, which the transport appends rather than replaces. mcp update stops erasing forward_identity and tool_acl on the way through. The seam itself had no test. identity_headers and the header merge were each covered alone, so passing an empty sliceite green. The new e2e tests assert on what reached the backend instead. cargo fmt --all --check clean, cargo test 839 passed. The one clippy result_large_err error on dispatch.rs cl-existing one, unchanged. Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>

Copilot

🟡 Changes recommended

Resolve transport-auth header collisions, reject empty subjects, and distinguish authenticated users named “anonymous.”

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates caller-identity forwarding across all proxied backend methods, with auditing, configuration preservation, tests, and documentation.

Changes:

Forwards identity on tools, resources, and prompts.
Validates anonymous, empty, and colliding identity headers.
Preserves forwarding settings during updates.
Expands end-to-end tests and documentation.

File summaries

File	Description
`src/server_auth/mod.rs`	Adds anonymous-identity handling.
`src/serve/proxy.rs`	Builds and validates forwarded headers.
`src/serve/e2e_tests.rs`	Adds forwarding and refusal tests.
`src/serve/dispatch.rs`	Applies identity headers and audits refusals.
`src/output.rs`	Updates HTTP configuration fixtures.
`src/manager.rs`	Preserves forwarding and ACL settings.
`src/config.rs`	Defines forwarding configuration.
`src/client.rs`	Supports per-request extra headers.
`docs/reference/config-file.md`	Documents forwarding configuration.
`docs/reference/cli.md`	Documents preserved update fields.
`docs/guides/proxy-mode.md`	Links identity-forwarding guidance.
`docs/guides/authentication.md`	Explains backend identity forwarding.

Review details

Suppressed comments (1)

src/serve/proxy.rs:1088

An empty subject is accepted here and then emitted as an empty identity header. BearerToken accepts empty legacy/extended subjects, and an empty header is indistinguishable from no caller identity to many backends; with forward_identity enabled this should refuse the call rather than assert an ambiguous identity. Include value.is_empty() in this validation (and keep the same refusal path).

fn valid_header_value(value: &str, what: &str) -> std::result::Result<String, String> {
    if value.bytes().any(|b| b < 0x20 || b == 0x7f) {
        return Err(format!("caller {what} contains a control character"));
    }
    Ok(value.to_string())

Files reviewed: 12/12 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

main has been red on this lint since dcd75dc and the branch inherits it, so CI here failed at the clippy step and skipped cargo test entirely. The 839 tests on this branch had never actually run in CI. Same allow the module already carries on resolve_tool_call, for the same reason: JsonRpcResponse is the error type across dispatch by design, because a failure here is the reply the caller relays. Boxing it is the other answer and it changes the error type on every dispatch path at once, which does not belong in this branch. The hunk is byte for byte identical to the one in PR #111, so whichever of the two lands first the other still merges clean. Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>

Copilot

🔵 Needs a closer look

Resolve transport-managed header collisions and avoid misclassifying authenticated identities with the subject “anonymous.”

Review details

Suppressed comments (2)

src/serve/proxy.rs:885

This collision check covers only configured static headers, but the HTTP transport can append a saved or refreshed bearer Authorization header after these per-request headers when no static Authorization is configured (src/transport/http.rs:156-177). Thus forward_identity.header: "Authorization" can emit two Authorization values, and the retry path guarantees the duplicate after a 401; the backend still decides which identity wins. Reject transport-managed header names as well, or define a replacement/merge rule in the transport so forwarded identity cannot be duplicated.

        let static_headers = entry.static_headers();
        let subject_name = valid_header_name(&cfg.header)?;
        reject_static_collision(static_headers, &subject_name, "header")?;

src/server_auth/mod.rs:53

is_anonymous() treats every identity whose subject is the literal "anonymous" as unauthenticated. A valid bearer, forwarded, or OAuth identity can legitimately use that subject, and AuthIdentity::new("anonymous", ...) is otherwise a named identity; forwarding is then refused even though authentication succeeded. Track authentication provenance explicitly (or reserve/reject this subject at provider validation) instead of inferring it from the user-controlled subject string.

    pub fn is_anonymous(&self) -> bool {
        self.subject == ANONYMOUS_SUBJECT

Files reviewed: 12/12 changed files
Comments generated: 0 new
Review effort level: Lite

Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>

Copilot

🟡 Changes recommended

Discovery requests do not forward caller identity, and transport-owned headers are not fully rejected.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/config.rs:92

This says forwarding applies to every backend-reaching method, but discovery still calls list_tools, list_resources, and list_prompts from discover_pending_backends without any extra_headers. For an opted-in backend, those requests still use only the service credential, so per-user discovery/authorization either fails or can cache metadata from the wrong identity. Please either thread the caller identity through discovery and make its cache identity-aware, or narrow this contract and document that discovery/list requests are intentionally not forwarded.

/// The identity travels on every method that reaches the backend —
/// `tools/call`, `resources/read`, `prompts/get` — because a read of per-user
/// data needs to know whose data it is as much as a write does.

Files reviewed: 12/12 changed files
Comments generated: 1
Review effort level: Lite

Replaced 'mcp-session-id' with 'content-type' and 'accept' in TRANSPORT_OWNED_HEADERS. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>

Copilot

🟡 Changes recommended

Add Mcp-Session-Id to transport-owned headers to prevent a collision with the transport session header.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 12/12 changed files
Comments generated: 1
Review effort level: Lite

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>

Copilot

🟡 Changes recommended

Reject roles containing the comma delimiter and document the transport-owned Content-Type and Accept headers.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

docs/reference/config-file.md:122

The documented transport-owned header list is incomplete: the implementation also rejects Content-Type and Accept in TRANSPORT_OWNED_HEADERS (src/serve/proxy.rs:1112-1121). An operator following this list can configure either name and receive a refusal that the reference does not predict; please include both headers here (or narrow the implementation if they are intended to be allowed).

- Neither header may be one the HTTP transport writes itself: `Authorization`,
  `Mcp-Session-Id`, `Mcp-Method`, `Mcp-Name`, `Mcp-Protocol-Version`, `traceparent`,
  `tracestate`. `Authorization` is the one worth naming. The transport re-adds a saved
  OAuth token whenever the config map has no usable one, and the 401 retry path removes
  the config entry before doing so, so a forwarded header of that name would travel
  beside a bearer token that the `headers` check below never sees.

Files reviewed: 12/12 changed files
Comments generated: 1
Review effort level: Lite

+            out.push((
+                roles_name,
+                valid_header_value(&identity.roles.join(","), "roles")?,
+            ));


luisaniceto-infra and others added 3 commits September 2, 2026 18:35

Copilot AI lite review requested due to automatic review settings September 17, 2026 17:13

Copilot started reviewing on behalf of avelino September 17, 2026 17:13

View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread src/serve/proxy.rs Resolved

Comment thread src/server_auth/mod.rs Resolved

Copilot AI review requested due to automatic review settings September 17, 2026 17:28

Copilot started reviewing on behalf of avelino September 17, 2026 17:29

View session

8000

Copilot AI reviewed Sep 17, 2026

View reviewed changes

fix: correct what identity forwarding refuses

5ee8aa5

Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 17, 2026 17:33

Copilot started reviewing on behalf of avelino September 17, 2026 17:34

View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread src/serve/proxy.rs Resolved

Copilot AI review requested due to automatic review settings September 17, 2026 17:49

Copilot started reviewing on behalf of avelino September 17, 2026 17:49

View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread src/serve/proxy.rs Resolved

Add 'mcp-session-id' to headers list

7087ab8

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 17, 2026 17:53

Copilot started reviewing on behalf of avelino September 17, 2026 17:53

View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread src/serve/proxy.rs

Comment on lines +912 to +915

out.push((

roles_name,

valid_header_value(&identity.roles.join(","), "roles")?,

));

avelino merged commit 83cf95b into main Sep 17, 2026
10 checks passed

avelino deleted the pr-110 branch September 17, 2026 18:01

Navigation Menu

Repository navigation

fix(serve): forward caller identity on every backend method - #112

fix(serve): forward caller identity on every backend method#112
avelino merged 7 commits into
mainfrom
pr-110

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Uh oh!

Changes

Tests

Docs

Not fixed here

Follow-ups worth their own issue

Checklist

Uh oh!

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants