fix: Shim for MCP Auth#1872
Draft
edwinjosechittilappilly wants to merge 1 commit into
Draft
Conversation
github-actions
Bot
added
backend
Contributor
WalkthroughAdds an ASGI middleware ( ChangesMCP JWT Forwarding Shim
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 |
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/app/routes/mcp.py`:
- Around line 44-52: The code currently only appends the Authorization header as
the target when the target header is not present (when has_target is false),
which allows client-supplied target headers to take precedence. Instead, you
need to strip any inbound target header from the headers list first, then
unconditionally append the Authorization value as the target header when auth is
present. Modify the logic to filter the headers list to remove any headers where
the lowercased name equals the target value (removing client-supplied target
headers), then if auth is not none, append it as the target header in the
modified scope headers list.
In `@src/dependencies.py`:
- Around line 854-859: The JWT header retrieval logic in the raw_jwt assignment
uses `or` to fallback from the primary JWT auth header to the MCP forwarded JWT
header. However, if the primary header (from get_jwt_auth_header()) contains
only whitespace, it evaluates as truthy and prevents the fallback, causing valid
MCP forwarded JWTs to be ignored. Fix this by stripping whitespace from the
primary header before using the `or` operator, so that whitespace-only values
are treated as absent and the fallback to get_mcp_forwarded_jwt_header() is
properly triggered.
🪄 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: 637bc3bf-7c6e-46f3-9ed5-a609e4a2c7b1
📒 Files selected for processing (3)
src/app/routes/mcp.pysrc/config/settings.pysrc/dependencies.py
Comment on lines
+44
to
+52
| for name, value in headers: | ||
| lname = name.lower() | ||
| if lname == b"authorization": | ||
| auth = value | ||
| elif lname == target: | ||
| has_target = True | ||
| if auth is not None and not has_target: | ||
| scope = dict(scope) | ||
| scope["headers"] = list(headers) + [(target, auth)] |
Contributor
There was a problem hiding this comment.
Overwrite client-supplied forwarded JWT headers.
has_target lets an inbound X-OpenRAG-JWT value win over the authenticated Authorization value. After FastMCP strips Authorization, /v1 can consume that client-controlled fallback header as identity/roles. Strip any inbound target header, then append only the value copied from Authorization.
🛡️ Proposed fix
- headers = scope.get("headers", [])
+ headers = list(scope.get("headers", []))
auth = None
- has_target = False
+ forwarded_headers = []
for name, value in headers:
lname = name.lower()
if lname == b"authorization":
auth = value
- elif lname == target:
- has_target = True
- if auth is not None and not has_target:
+ forwarded_headers.append((name, value))
+ elif lname != target:
+ forwarded_headers.append((name, value))
+ if auth and auth.strip():
scope = dict(scope)
- scope["headers"] = list(headers) + [(target, auth)]
+ scope["headers"] = forwarded_headers + [(target, auth)]
+ elif len(forwarded_headers) != len(headers):
+ scope = dict(scope)
+ scope["headers"] = forwarded_headers🤖 Prompt for 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.
In `@src/app/routes/mcp.py` around lines 44 - 52, The code currently only appends
the Authorization header as the target when the target header is not present
(when has_target is false), which allows client-supplied target headers to take
precedence. Instead, you need to strip any inbound target header from the
headers list first, then unconditionally append the Authorization value as the
target header when auth is present. Modify the logic to filter the headers list
to remove any headers where the lowercased name equals the target value
(removing client-supplied target headers), then if auth is not none, append it
as the target header in the modified scope headers list.
Comment on lines
+854
to
+859
| # Primary: the gateway-forwarded JWT header (default Authorization). Fallback: | ||
| # the header the /mcp shim copies the JWT into, because FastMCP strips | ||
| # Authorization before proxying an MCP tool call to this /v1 handler. | ||
| raw_jwt = request.headers.get(get_jwt_auth_header(), "") or request.headers.get( | ||
| get_mcp_forwarded_jwt_header(), "" | ||
| ) |
Contributor
There was a problem hiding this comment.
Treat blank primary JWT headers as absent before falling back.
A whitespace-only primary header is truthy for or, so a valid MCP forwarded JWT is ignored and the request falls through as unauthenticated.
🔧 Proposed fix
- raw_jwt = request.headers.get(get_jwt_auth_header(), "") or request.headers.get(
- get_mcp_forwarded_jwt_header(), ""
- )
+ primary_jwt = request.headers.get(get_jwt_auth_header(), "").strip()
+ raw_jwt = primary_jwt or request.headers.get(get_mcp_forwarded_jwt_header(), "").strip()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Primary: the gateway-forwarded JWT header (default Authorization). Fallback: | |
| # the header the /mcp shim copies the JWT into, because FastMCP strips | |
| # Authorization before proxying an MCP tool call to this /v1 handler. | |
| raw_jwt = request.headers.get(get_jwt_auth_header(), "") or request.headers.get( | |
| get_mcp_forwarded_jwt_header(), "" | |
| ) | |
| # Primary: the gateway-forwarded JWT header (default Authorization). Fallback: | |
| # the header the /mcp shim copies the JWT into, because FastMCP strips | |
| # Authorization before proxying an MCP tool call to this /v1 handler. | |
| primary_jwt = request.headers.get(get_jwt_auth_header(), "").strip() | |
| raw_jwt = primary_jwt or request.headers.get(get_mcp_forwarded_jwt_header(), "").strip() |
🤖 Prompt for 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.
In `@src/dependencies.py` around lines 854 - 859, The JWT header retrieval logic
in the raw_jwt assignment uses `or` to fallback from the primary JWT auth header
to the MCP forwarded JWT header. However, if the primary header (from
get_jwt_auth_header()) contains only whitespace, it evaluates as truthy and
prevents the fallback, causing valid MCP forwarded JWTs to be ignored. Fix this
by stripping whitespace from the primary header before using the `or` operator,
so that whitespace-only values are treated as absent and the fallback to
get_mcp_forwarded_jwt_header() is properly triggered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: Shim for MCP Auth
Summary by CodeRabbit
New Features
Chores