feat(elicitation): support configurable headers for URL elicitation tokens#1058
feat(elicitation): support configurable headers for URL elicitation tokens#1058Aryanburnwal05 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds ChangesConfigurable URL-elicitation token headers
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/mcp-router/request_handlers_test.go`:
- Around line 1627-1634: Remove the conversational draft comments in
internal/mcp-router/request_handlers_test.go and replace them with a single
terse comment (or none) describing the assertion; then update the failing
assertion that checks the injected Authorization header to expect the new
default format "Bearer ghp_cached_token" instead of "ghp_cached_token" (locate
the header assertion in the test that inspects the Authorization value and the
token variable used for injection). Ensure only a concise comment remains and
the assertion compares to "Bearer "+token.
In `@internal/mcp-router/request_handlers.go`:
- Around line 760-766: This file fails gofmt; run "gofmt -s -w" on
internal/mcp-router/request_handlers.go to apply the standardized formatting
(including the changed hunks around resolveTokenHeaderConfig,
formatTokenHeaderValue and the passThroughHeaders assignment) and re-check the
other modified region around the code referenced at lines roughly 927-930 so the
file is gofmt-clean for CI.
- Around line 760-765: The code unconditionally overwrites the configured header
(notably Authorization) when injecting the token; update the injection logic in
the places that call resolveTokenHeaderConfig and formatTokenHeaderValue so you
only set passThroughHeaders[strings.ToLower(headerName)] = formattedValue if
that key is not already present (i.e., check presence with
strings.ToLower(headerName) before assignment), ensuring Authorization is
injected only when no existing Authorization header exists; apply this same
guard to both token-injection paths where these functions are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa2d37ed-6c81-468a-ab87-1051341291a8
📒 Files selected for processing (5)
api/v1alpha1/types.gointernal/config/types.gointernal/controller/mcpserverregistration_controller.gointernal/mcp-router/request_handlers.gointernal/mcp-router/request_handlers_test.go
…kens Resolves Kuadrant#970 by adding HeaderName and HeaderValueFormat to the TokenURLElicitation struct in the MCPServerRegistration API. This enables connecting to MCP servers that expect the URL elicitation token in a different header (e.g., X-API-Key) or format (e.g., bare token instead of Bearer {token}). Signed-off-by: Aryan Burnwal <burnwalaryan922@gmail.com>
4c1386d to
7e056ed
Compare
jasonmadigan
left a comment
There was a problem hiding this comment.
changes requested. ci is red (lint, crd sync, e2e).
blockers
buildRouterConfigKeyis dead code, never called. remove it (this is why lint fails)- default token format changed from raw to
Bearer {token}.WithAuthinjected the raw value before. any deployment using elicitation with raw PATs (e.g. github) will break silently. either default to{token}for backwards compat or document the migration - crd manifests out of sync -- run
make generate-all
should fix
- no validation on
headerNamein the crd. a cluster admin could set it to:authorityormcp-session-idand overwrite routing-critical headers. add a kubebuilder enum/pattern or runtime blocklist - the "already has authorization" guard in both code paths only checks for
Authorizationspecifically. ifheaderNameisX-API-Keyand the client already sends one, gateway overwrites it silently. generalise the guard to check the configured header name
nits
- guard block between
initializeMCPSeverSessionandresolveUpstreamTokenis repeated. shared helpers help but the guard itself could be extracted - hairpin path lowercases header name, tool/call path preserves case. http/2 normalises anyway but the inconsistency is odd
|
Thanks for the contributions! One thing to flag: our contributing guidelines ask contributors to limit themselves to one open PR at a time. The idea is that getting one change reviewed and merged is more valuable than having several open in parallel. You currently have three open PRs. Could you pick whichever one you think is most ready for review, and we'll focus on getting that one through first? |
|
Closing due to inactivity |
This updates URL elicitation token injection to support configurable header names and value formats instead of always injecting :-
The default behavior remains unchanged, but MCP server registrations can now customize both the header name and token formatting when forwarding elicited credentials upstream.
Examples :-
What changed
Backward compatibility
Existing behavior is preserved when the new fields are omitted :-
remains the default behavior for both standard injection and pass through/hairpin flows.
Tests
Extended the existing router test coverage with cases for :-
Notes
I was not able to regenerate CRD/deepcopy artifacts locally due to a controller-gen /
golang.org/x/toolsissue on my Windows environment (invalid array length -delta * delta).The implementation and tests are complete and passing locally, but generated artifacts may still need to be refreshed from a Linux/WSL environment if CI requires it.
Fixes #970
Summary by CodeRabbit