test: end-to-end smoke script for both HTTP transports - #16
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
This automated review found one issue, posted inline (the EXIT trap can reference unset PID variables and skip workdir cleanup if a go build fails early). Beyond that, I also checked the fixed sleep 0.5 used before probing each spawned server: it only confirms the process hasn't crashed via kill -0, not that it's actually listening, but if a probe fires too early the curl call will just return a mismatched/empty status and the script correctly reports it as a FAIL rather than silently passing, so it's a flakiness risk rather than a correctness bug.
Extended reasoning...
Beyond the inline finding (EXIT trap referencing unset $BACKEND_PID/$HTTP_PID/$SSE_PID under set -u if either go build fails before both PIDs are assigned, leaking the temp workdir), I examined the second candidate from this run's ruled-out list: the fixed sleep 0.5 readiness checks before curling each of the three spawned servers. Tracing through the script, a premature probe (server not yet listening) would cause curl to fail to connect, producing an empty or non-matching status code that the check helper would report as an explicit FAIL rather than a false PASS — so this is at worst a source of CI flakiness, not a bug that masks failures or produces incorrect success. Since this is a low-risk, test-only shell script with no production code paths touched, and the one substantive finding is already captured as an inline comment, a brief defer noting what else was checked and ruled out is appropriate rather than a full approve or a second silent pass.
efdf7c5 to
956b2b4
Compare
|
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.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
b404355 to
e798cb8
Compare
956b2b4 to
96b5c2f
Compare
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
e798cb8 to
4b2b713
Compare
96b5c2f to
1ae575e
Compare
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
1ae575e to
2f30895
Compare
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
2f30895 to
80a1799
Compare
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. Two guards added after review: - --forward-header and --forward-operator-identity now require -hostport, matching the existing -tls-crt+hostport pattern. Without -hostport (stdio) there are no inbound HTTP headers, so either flag previously did nothing silently -- worse than an error for something an operator is relying on for identity attribution. - A --forward-header name (or X-Operator-Identity, for --forward-operator-identity) that collides with a header already set via -header/-bearer-env is now rejected at startup. Both mechanisms are chained last in the headers pipeline and overwrite unconditionally, so an operator configuring e.g. --forward-header=Authorization would let any inbound MCP client silently replace grpcmcp's own trusted backend credential on every call. Both guards verified empirically: reverted, confirmed the tests fail (not hang -- waitForExit bounds every exit-expected check to 2s so a regression fails fast instead of hanging the suite), restored, confirmed green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJKesYekqPnbBrhM3swNHs
There was a problem hiding this comment.
Looks good — both previously flagged issues are now fixed and covered by tests: the EXIT trap no longer risks an unbound-variable abort on early build failure (PID vars pre-declared), and echoHandler now rejects oversized frame lengths before allocating, with TestEchoOversizedFrameRejected/TestEchoRoundTrip exercising the guard.
Extended reasoning...
Overview
This PR adds an end-to-end smoke test script (scripts/e2e_test.sh) plus a hand-rolled example gRPC Echo backend (example/echo.go, example/main.go, example/echo_test.go) that the script exercises, and wires the script into CI. It is stacked on top of prior SSE-restoration work.
Security risks
The example backend is test/demo infrastructure, not part of the production library. The one real risk — an unbounded client-supplied length prefix driving an unbounded allocation before any read — was flagged in a prior round and is now fixed: echoHandler checks length > maxFrameLength (4MiB) before calling make([]byte, length), returning 413 instead. This is now covered by TestEchoOversizedFrameRejected. No other injection, auth-bypass, or data-exposure concerns in this diff.
Level of scrutiny
This warranted a full read given it touches process-lifecycle shell scripting (trap/set -u interactions) and manual wire-level gRPC framing, both categories where subtle bugs are easy to introduce. Both substantive issues found across prior rounds (trap unbound-variable hazard, unbounded allocation) have been fixed with accompanying regression tests, and re-reading the current diff confirms both fixes are correctly placed (PID vars zeroed before trap install; length check strictly precedes allocation).
Other factors
The change is now well covered by new unit tests (echo_test.go) and the e2e script itself checks 8 concrete behaviors across both HTTP transports. Beyond the two previously-raised and now-fixed items, this run's investigation into the e2e script's fixed sleep 0.5 readiness wait (not a strict listener-bound check) was considered a minor test-robustness nit, not a functional bug, and no other issues were found in ci.yaml or main.go's small integration change.
Stacked on #15. Boots the real binary against the example backend and exercises it over actual HTTP: initialize/tools-list/tools-call on Streamable HTTP, the 405/403/400 guards, and the SSE path (deprecation warning, event-stream response, Origin rejection). Complements the in-process unit tests in transport_test.go, which don't prove the binary actually boots and serves correctly end-to-end. Adds a real RPC (echo.v1.EchoService/Echo) to the example backend, built by hand the same way this repo's own tests build descriptors, so grpcmcp's reflection -> tools/call -> backend path gets a genuine round trip with a real argument and a real return value, not just health-check plumbing. Wire-level gRPC framing (length-prefixed unary request/response + a grpc-status trailer) is hand-rolled rather than built on connect-go's generic Handler API, since dynamicpb messages can't be plugged into connect's generic type parameters without a concrete Go type to instantiate. The frame length prefix is client-controlled; echoHandler now rejects a claimed length over 4MiB before allocating a buffer for it, rather than trusting an unbounded 4-byte value. Covered by TestEchoOversizedFrameRejected, plus TestEchoRoundTrip for the normal path. BACKEND_PID/HTTP_PID/SSE_PID are declared (empty) before the EXIT trap is installed. With set -u active, if go build fails before a PID is assigned, the trap referencing an unset var aborted mid-command and skipped rm -rf "$WORKDIR" -- exactly the case the trap exists to handle. Verified empirically: before this fix, a simulated early build failure left the temp dir on disk; after it, the trap runs to completion and removes it. Wired into CI: runs after go test on every push/PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJKesYekqPnbBrhM3swNHs
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. Two guards added after review: - --forward-header and --forward-operator-identity now require -hostport, matching the existing -tls-crt+hostport pattern. Without -hostport (stdio) there are no inbound HTTP headers, so either flag previously did nothing silently -- worse than an error for something an operator is relying on for identity attribution. - A --forward-header name (or X-Operator-Identity, for --forward-operator-identity) that collides with a header already set via -header/-bearer-env is now rejected at startup. Both mechanisms are chained last in the headers pipeline and overwrite unconditionally, so an operator configuring e.g. --forward-header=Authorization would let any inbound MCP client silently replace grpcmcp's own trusted backend credential on every call. Both guards verified empirically: reverted, confirmed the tests fail (not hang -- waitForExit bounds every exit-expected check to 2s so a regression fails fast instead of hanging the suite), restored, confirmed green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJKesYekqPnbBrhM3swNHs
80a1799 to
d9f3b08
Compare
4b2b713 to
3bb6eb1
Compare
1 participant
Stacked on #15.
Boots the real binary against the example backend and exercises it over actual HTTP: initialize/tools-list on Streamable HTTP, the 405/403/400 guards, and the SSE path (deprecation warning, event-stream response, Origin rejection). Complements the in-process unit tests in
transport_test.go, which don't prove the binary actually boots and serves correctly end-to-end.All 8 checks pass locally against a built binary.