feat: add --forward-header for general trusted-proxy identity headers

[DanielPBak]

Stacked on #16.

Generalizes the --forward-operator-identity mechanism: a repeatable
--forward-header=NAME flag copies any named header from the inbound MCP
request onto the outbound gRPC call, if present. Same trust model
(the header must be minted by a trusted proxy in front of this server;
grpcmcp does not verify it) but not tied to the X-Operator-Identity name
specifically.

Motivation: an OAuth-terminating reverse proxy in front of grpcmcp (e.g.
obot-platform/mcp-oauth-proxy) asserts caller identity via its own header
names (X-Forwarded-User, X-Forwarded-Access-Token), not
X-Operator-Identity. --forward-operator-identity is unchanged and still
useful for the existing internal-mTLS-proxy case.

Extends the e2e script (a fourth grpcmcp instance with
--forward-header=X-Forwarded-User) and the example backend (echoes back
a forwarded header if present) to prove this crosses grpcmcp -> real gRPC
call -> real backend, not just the in-process unit test.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01CJKesYekqPnbBrhM3swNHs

[DanielPBak]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• feat: add --forward-header for general trusted-proxy identity headers #17 👈 (View in Graphite)
• test: end-to-end smoke script for both HTTP transports #16
• feat!: stateless Streamable HTTP by default; SSE kept as a deprecated -transport option #15
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[claude]

Beyond the inline finding, I also checked whether req.Header.Get(name) returning only the first value matters here — the forwarded headers (X-Forwarded-User etc.) are documented as single trusted-proxy-asserted identity values, not comma/list-style headers like X-Forwarded-For, so first-value-only lookup is not a bug for this use case.

Extended reasoning...

This run's inline finding (main.go:65, forwardHeaders unconditionally overwriting operator-configured backend auth headers such as Authorization) already flags the primary concern for human review, so a full structured review body is unnecessary. I additionally verified that req.Header.Get(name) in forwardHeaders returning only the first value of a possibly-repeated header is not a functional bug given the documented use case (single-value identity headers minted once by a trusted proxy, not accumulator-style headers like X-Forwarded-For), so that candidate concern is ruled out rather than left open. The prior review's inline comment about requiring --hostport for --forward-header/--forward-operator-identity remains unaddressed in this diff, but it was marked optional in the earlier review and restating it would be redundant noise given the "don't be a broken record" guidance.

[claude]

Code review found no new issues

No new issues were found in this update; 2 findings from earlier reviews are still open above.

[claude]

Code review found no new issues

No new issues were found in this update; 2 findings from earlier reviews are still open above.

Still open from earlier reviews (2):

• 🔴 main.go:65 — forwardHeaders unconditionally overwrites any header in --forward-header's list, including ones grpcmcp itself sets for…
• Also unresolved: 1 minor or pre-existing.

[claude]

I reviewed this updated PR and didn't find any new bugs — the latest commit adds the -hostport requirement and the Authorization/X-Operator-Identity collision guards that address the two issues raised in earlier reviews, each backed by a dedicated test. Because this PR touches authentication/header-forwarding trust boundaries, a human look would still be worthwhile before merging.

What was reviewed:

• The new -hostport requirement for -forward-header/-forward-operator-identity and the collision checks against -header/-bearer-env values in main.go, cross-checked against TestForwardHeaderRequiresHostport, TestForwardHeaderCollisionRejected, and TestForwardOperatorIdentityCollisionRejected.
• The forwardHeaders copy logic, its placement last in the provider chain, and the e2e script's new fourth instance exercising --forward-header=X-Forwarded-User against the real echo backend.
• Confirmed the collision check uses http.Header.Get, which canonicalizes header names, so casing differences between -header and -forward-header can't bypass the guard.

Extended reasoning...

Overview

This PR adds a repeatable --forward-header=NAME flag to grpcmcp that generalizes the existing --forward-operator-identity mechanism, copying any named inbound MCP request header onto the outbound gRPC call. The diff touches main.go (new forwardHeaders wrapper, flag parsing, two startup validation checks), the example echo backend (to make forwarded headers observable end-to-end), two new Go test files, the e2e shell script, and README documentation. The single commit in this diff (65341d6) is the entire PR history as squashed/rebased against the base, and its commit message explicitly states it adds "two guards added after review" — matching the two findings this bug-hunting system posted on earlier pushed versions of this PR (the missing -hostport requirement, and the Authorization/identity-header collision risk).

Security risks

The core risk in this feature is header-forwarding trust: forwardHeaders copies inbound MCP request headers verbatim onto outbound gRPC metadata, with no verification that they were minted by a trusted proxy (this is by design, per the PR's stated trust model, mirroring the pre-existing --forward-operator-identity). The two concrete risks previously identified — (1) the flag silently no-opping on stdio transports where headers don't exist, and (2) an inbound client being able to override a statically configured Authorization header set via -header/-bearer-env — are both now guarded against with explicit startup checks that os.Exit(-1) with a descriptive stderr message, matching the existing -tls-crt/-hostport pattern already in the codebase. I verified the collision check operates on http.Header.Get, which normalizes header name casing, so a case-variant collision (e.g. authorization vs Authorization) cannot slip past the guard. No other new security-relevant surface (injection, auth bypass beyond what's discussed, data exposure) was found in the diff.

Level of scrutiny

This is exactly the kind of change that warrants a higher bar: it is security-sensitive (authentication/identity header handling crossing a trust boundary into backend gRPC calls), and it directly follows up on findings this same review system raised on prior pushes. The fixes appear correct and are covered by new, specific tests (TestForwardHeaderRequiresHostport, TestForwardHeaderCollisionRejected, TestForwardOperatorIdentityCollisionRejected, TestForwardHeaderNoCollisionStarts), which is a good sign, but a human maintainer's sign-off is still the right call given the trust-boundary nature of the feature, consistent with not auto-approving auth-adjacent changes even when this run found nothing further.

Other factors

No CODEOWNERS/ANTOWNERS file exists in the repo, no outstanding third-party (non-bot) review objections are visible in the timeline, and this run's bug hunt exited via dry_streak (ran to completion, not budget-cut), so a defer with "no new bugs found, but human review still warranted" is the appropriate signal rather than approving a change to authentication-forwarding logic outright.

[claude]

Both concerns from my earlier reviews are now fixed: -forward-header/-forward-operator-identity now require -hostport (erroring out on stdio instead of silently doing nothing), and any forwarded header name that collides with one already set via -header/-bearer-env (e.g. Authorization) causes the process to reject the config at startup. Both fixes have dedicated tests (forward_header_flag_validation_test.go) confirming the exit code and stderr message.

Extended reasoning...

Overview

This commit (c0c27a2) is the sole commit in the diff range and represents the final, review-addressed state of the "add --forward-header" PR. It adds the forward_header copying mechanism, plus two pieces of startup validation directly responding to my two earlier inline findings: (1) both -forward-operator-identity and -forward-header now require -hostport, exiting non-zero with a clear message if not (main.go ~274-278), mirroring the existing -tls-crt + -hostport check; and (2) any forwarded header name colliding with one already set via -header/-bearer-env (checked after the bearer-env Authorization header is populated, so ordering is correct) causes a non-zero exit with a message naming the collision (main.go ~328-343).

Security risks

The main risk previously identified — an inbound MCP client silently overriding a trusted backend credential (e.g. Authorization) via --forward-header=Authorization — is now closed by the collision check, which runs after -bearer-env populates headers, so it correctly catches that case too. The stdio silent-no-op risk (operator believes forwarding is active but it never fires) is closed by the new -hostport requirement. I re-read forwardHeaders and operatorIdentityHeaders and confirmed the guard is applied before either provider is chained onto headersProvider.

Level of scrutiny

This is security-adjacent (auth/identity header handling), so it warrants care, which is why I read the full diff rather than skimming. Both fixes are small, targeted, and directly mirror an existing pattern in the same file (the TLS+hostport check), reducing the chance of a subtle new bug. New tests (forward_header_flag_validation_test.go) exercise both the hostport requirement and both collision paths (forward-header and forward-operator-identity) with real process spawns, plus a no-collision case to confirm the guard isn't overly strict.

Other factors

There was a previously-flagged nit (main.go:362 in an earlier revision, marked minor/pre-existing) that I cannot re-verify precisely since its content wasn't shown to me this round, but it was already characterized as minor and not blocking. No CODEOWNERS file exists for the changed paths. Given both substantive findings are now fixed with matching test coverage and no new issues were found in this round, I'm approving.