LCORE-1461: Fix /tools not handling "kubernetes" and static token auth#1349
Open
jrobertboos wants to merge 1 commit intolightspeed-core:mainfrom
Open
LCORE-1461: Fix /tools not handling "kubernetes" and static token auth#1349jrobertboos wants to merge 1 commit intolightspeed-core:mainfrom
/tools not handling "kubernetes" and static token auth#1349jrobertboos wants to merge 1 commit intolightspeed-core:mainfrom
Conversation
Contributor
WalkthroughCentralizes MCP header/token assembly via a new build_mcp_headers() utility, normalizes Authorization values for MCP probes, updates endpoints to use the merged headers, and adjusts Docker Compose and e2e test configs/fixtures to reference renamed token paths and enable previously skipped MCP scenarios. Changes
Sequence DiagramsequenceDiagram
participant Client as Request Handler
participant HeaderBuilder as build_mcp_headers()
participant AuthCheck as check_mcp_auth()
participant ToolRetriever as get_mcp_tools / endpoints
Client->>HeaderBuilder: provide mcp_headers, request_headers, token
activate HeaderBuilder
HeaderBuilder->>HeaderBuilder: merge client mcp_headers per server (case‑insensitive)
HeaderBuilder->>HeaderBuilder: inject server-config auth headers when missing
HeaderBuilder->>HeaderBuilder: format Kubernetes auth as "Bearer <token>" if token provided
HeaderBuilder->>HeaderBuilder: propagate allowlisted request headers when missing
HeaderBuilder-->>Client: complete_mcp_headers
deactivate HeaderBuilder
Client->>AuthCheck: pass complete_mcp_headers
activate AuthCheck
AuthCheck->>AuthCheck: normalize Authorization header (ensure "Bearer " prefix)
AuthCheck->>AuthCheck: probe/validate per-server auth as needed
AuthCheck-->>Client: auth statuses
deactivate AuthCheck
Client->>ToolRetriever: request tools using complete_mcp_headers
activate ToolRetriever
ToolRetriever->>ToolRetriever: select per-toolgroup headers, pop Authorization for extra_query
ToolRetriever-->>Client: tools results (skips servers missing required headers)
deactivate ToolRetriever
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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: 1
🧹 Nitpick comments (1)
src/app/endpoints/tools.py (1)
151-152: Mutating the dict obtained fromcomplete_mcp_headersmodifies the original.
complete_mcp_headers.get(toolgroup.identifier, {})returns a reference to the dict insidecomplete_mcp_headers. The subsequentpop()modifies that dict in-place. While this doesn't cause issues in the current loop structure, it's fragile for future maintenance.♻️ Suggested fix: copy before mutating
- headers = complete_mcp_headers.get(toolgroup.identifier, {}) - authorization = headers.pop("Authorization", None) + headers = dict(complete_mcp_headers.get(toolgroup.identifier, {})) + authorization = headers.pop("Authorization", None)As per coding guidelines: "Avoid in-place parameter modification anti-patterns; return new data structures instead"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/tools.py` around lines 151 - 152, The code mutates the original dict from complete_mcp_headers by calling pop() on headers = complete_mcp_headers.get(toolgroup.identifier, {}); to fix, obtain a shallow copy before mutating (e.g., copy the result of complete_mcp_headers.get(...) into a new dict) so popping "Authorization" does not modify complete_mcp_headers; update the logic around headers and the Authorization extraction (referencing complete_mcp_headers, toolgroup.identifier, and the headers variable) to operate on the copied dict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/mcp_oauth_probe.py`:
- Around line 45-48: In build_mcp_headers (src/utils/mcp_oauth_probe.py) the
Authorization value is being prefixed with "Bearer " unconditionally, causing a
"Bearer Bearer ..." header; change the logic that sets the authorization local
variable (authorization) to use headers.get("Authorization") as-is if it already
starts with "Bearer " or otherwise prepend "Bearer " only when
missing—alternatively just assign headers.get("Authorization") directly if
callers always provide the full token; adjust the branch that currently checks
headers.get("Authorization", None) to perform this conditional prefixing to
avoid double-prefixing.
---
Nitpick comments:
In `@src/app/endpoints/tools.py`:
- Around line 151-152: The code mutates the original dict from
complete_mcp_headers by calling pop() on headers =
complete_mcp_headers.get(toolgroup.identifier, {}); to fix, obtain a shallow
copy before mutating (e.g., copy the result of complete_mcp_headers.get(...)
into a new dict) so popping "Authorization" does not modify
complete_mcp_headers; update the logic around headers and the Authorization
extraction (referencing complete_mcp_headers, toolgroup.identifier, and the
headers variable) to operate on the copied dict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44744b21-e851-45df-a82c-877a9d073977
📒 Files selected for processing (16)
docker-compose-library.yamldocker-compose.yamlsrc/app/endpoints/tools.pysrc/utils/mcp_headers.pysrc/utils/mcp_oauth_probe.pysrc/utils/responses.pytests/e2e/configuration/library-mode/lightspeed-stack-invalid-mcp-file-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-file-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-kubernetes-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp.yamltests/e2e/configuration/server-mode/lightspeed-stack-invalid-mcp-file-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-file-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-kubernetes-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp.yamltests/e2e/features/environment.pytests/e2e/features/mcp.feature
💤 Files with no reviewable changes (1)
- tests/e2e/features/environment.py
uncommented tests fixed k8s config fixed black fixed library e2e addressed coderabbit
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/mcp_oauth_probe.py (1)
45-60:⚠️ Potential issue | 🟠 MajorProbe with the merged header map, not just
Authorization.This block still throws away every non-
Authorizationheader thatbuild_mcp_headers()resolved, then rewrites the remaining value into a Bearer token. Servers that authenticate via another header name continue to be probed unauthenticated, and a fully formed non-BearerAuthorizationvalue gets mangled before the probe. Passing the merged header dict throughprobe_mcp()as-is keeps/toolsaligned with the new “complete MCP headers” flow.Possible fix
async def check_mcp_auth(configuration: AppConfig, mcp_headers: McpHeaders) -> None: """Probe each configured MCP server that expects OAuth or has auth headers. @@ probes = [] for mcp_server in configuration.mcp_servers: - headers = mcp_headers.get(mcp_server.name, {}) - auth_header = headers.get("Authorization") - if auth_header: - authorization = ( - auth_header - if auth_header.startswith("Bearer ") - else f"Bearer {auth_header}" - ) - else: - authorization = None + headers = dict(mcp_headers.get(mcp_server.name, {})) if ( - authorization + headers or constants.MCP_AUTH_OAUTH in mcp_server.resolved_authorization_headers.values() ): - probes.append(probe_mcp(mcp_server.url, authorization=authorization)) + probes.append(probe_mcp(mcp_server.url, headers=headers or None)) if probes: await asyncio.gather(*probes) @@ async def probe_mcp( url: str, - authorization: Optional[str] = None, + headers: Optional[dict[str, str]] = None, ) -> None: @@ - headers: Optional[dict[str, str]] = ( - {"authorization": authorization} if authorization is not None else None - ) try: timeout = aiohttp.ClientTimeout(total=10) async with aiohttp.ClientSession(timeout=timeout) as session: async with session.get(url, headers=headers) as resp:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/mcp_oauth_probe.py` around lines 45 - 60, The code currently extracts only the "Authorization" value (auth_header) and rewrites it as a Bearer token (authorization) before calling probe_mcp, which discards other resolved auth headers and mangles non-Bearer Authorization values; modify the call site so that instead of building a single authorization string you pass the full merged headers dictionary (the one produced by build_mcp_headers / the local variable headers or mcp_server.resolved_authorization_headers) into probe_mcp (probe_mcp(mcp_server.url, headers=merged_headers)) and remove the ad-hoc authorization/authorization header reconstruction logic so probe_mcp receives the complete header map intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/mcp_headers.py`:
- Around line 164-166: The loop over config.mcp_servers assumes
mcp_headers[mcp_server.name] is already a dict[str,str]; instead, before calling
dict(...) normalize and validate raw mcp_headers entries (mcp_headers variable
used in the loop) so only recognized key shapes (server name or URL forms
produced by extract_mcp_headers()/handle_mcp_headers_with_toolgroups) and
string-to-string mappings are accepted; specifically, if
mcp_headers.get(mcp_server.name) (or an entry keyed by a URL form) is not a
mapping, treat it as absent, and when it is a mapping filter out non-str
keys/values and ignore nested non-string payloads, then build server_headers
from that sanitized mapping (adjusting any URL-keyed entries to
mcp_server.name). This change should be made around the loop that defines
server_headers and existing_lower to avoid TypeErrors and silently dropping
URL-keyed payloads.
---
Outside diff comments:
In `@src/utils/mcp_oauth_probe.py`:
- Around line 45-60: The code currently extracts only the "Authorization" value
(auth_header) and rewrites it as a Bearer token (authorization) before calling
probe_mcp, which discards other resolved auth headers and mangles non-Bearer
Authorization values; modify the call site so that instead of building a single
authorization string you pass the full merged headers dictionary (the one
produced by build_mcp_headers / the local variable headers or
mcp_server.resolved_authorization_headers) into probe_mcp
(probe_mcp(mcp_server.url, headers=merged_headers)) and remove the ad-hoc
authorization/authorization header reconstruction logic so probe_mcp receives
the complete header map intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a1859e9-ed67-4780-b7c3-632693d409f4
📒 Files selected for processing (16)
docker-compose-library.yamldocker-compose.yamlsrc/app/endpoints/tools.pysrc/utils/mcp_headers.pysrc/utils/mcp_oauth_probe.pysrc/utils/responses.pytests/e2e/configuration/library-mode/lightspeed-stack-invalid-mcp-file-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-file-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-kubernetes-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp.yamltests/e2e/configuration/server-mode/lightspeed-stack-invalid-mcp-file-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-file-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-kubernetes-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp.yamltests/e2e/features/environment.pytests/e2e/features/mcp.feature
💤 Files with no reviewable changes (1)
- tests/e2e/features/environment.py
✅ Files skipped from review due to trivial changes (9)
- tests/e2e/configuration/server-mode/lightspeed-stack-invalid-mcp-file-auth.yaml
- tests/e2e/configuration/library-mode/lightspeed-stack-mcp-kubernetes-auth.yaml
- tests/e2e/configuration/library-mode/lightspeed-stack-mcp-file-auth.yaml
- tests/e2e/configuration/server-mode/lightspeed-stack-mcp-file-auth.yaml
- tests/e2e/configuration/server-mode/lightspeed-stack-mcp-kubernetes-auth.yaml
- tests/e2e/configuration/library-mode/lightspeed-stack-mcp.yaml
- tests/e2e/configuration/library-mode/lightspeed-stack-invalid-mcp-file-auth.yaml
- src/app/endpoints/tools.py
- docker-compose.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- docker-compose-library.yaml
- tests/e2e/configuration/server-mode/lightspeed-stack-mcp.yaml
Comment on lines
+164
to
+166
| for mcp_server in config.mcp_servers: | ||
| server_headers: dict[str, str] = dict(mcp_headers.get(mcp_server.name, {})) | ||
| existing_lower = {k.lower() for k in server_headers} |
Contributor
There was a problem hiding this comment.
Normalize and validate raw MCP-HEADERS before copying them.
This lookup assumes the incoming entry is already a dict[str, str] keyed by mcp_server.name. In this module, extract_mcp_headers() only validates the top-level JSON object and handle_mcp_headers_with_toolgroups() still documents URL/toolgroup keyed payloads. That means {"server": "token"} can blow up at dict(...), while {"https://server": {...}} is silently dropped. Normalizing the supported key shapes and filtering to string-to-string entries here will avoid 500s and keep downstream Authorization handling consistent.
Possible fix
complete: McpHeaders = {}
for mcp_server in config.mcp_servers:
- server_headers: dict[str, str] = dict(mcp_headers.get(mcp_server.name, {}))
+ if mcp_server.name in mcp_headers:
+ raw_headers = mcp_headers[mcp_server.name]
+ elif mcp_server.url in mcp_headers:
+ raw_headers = mcp_headers[mcp_server.url]
+ else:
+ raw_headers = {}
+
+ if not isinstance(raw_headers, Mapping):
+ logger.warning(
+ "Ignoring invalid MCP headers for %s: expected object, got %s",
+ mcp_server.name,
+ type(raw_headers).__name__,
+ )
+ raw_headers = {}
+
+ server_headers = {
+ (
+ "Authorization"
+ if header_name.lower() == "authorization"
+ else header_name
+ ): header_value
+ for header_name, header_value in raw_headers.items()
+ if isinstance(header_name, str) and isinstance(header_value, str)
+ }
existing_lower = {k.lower() for k in server_headers}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/mcp_headers.py` around lines 164 - 166, The loop over
config.mcp_servers assumes mcp_headers[mcp_server.name] is already a
dict[str,str]; instead, before calling dict(...) normalize and validate raw
mcp_headers entries (mcp_headers variable used in the loop) so only recognized
key shapes (server name or URL forms produced by
extract_mcp_headers()/handle_mcp_headers_with_toolgroups) and string-to-string
mappings are accepted; specifically, if mcp_headers.get(mcp_server.name) (or an
entry keyed by a URL form) is not a mapping, treat it as absent, and when it is
a mapping filter out non-str keys/values and ignore nested non-string payloads,
then build server_headers from that sanitized mapping (adjusting any URL-keyed
entries to mcp_server.name). This change should be made around the loop that
defines server_headers and existing_lower to avoid TypeErrors and silently
dropping URL-keyed payloads.
tisnik
requested changes
Mar 20, 2026
Contributor
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
LGTM in overall. Just few places to be refactored.
e2e will be reviewed by @radofuchs
| headers = mcp_headers.get(mcp_server.name, {}) | ||
| authorization = headers.get("Authorization", None) | ||
| auth_header = headers.get("Authorization") | ||
| if auth_header: |
Contributor
There was a problem hiding this comment.
is not None - you don't want to pass with empty auth header
|
|
||
| complete: McpHeaders = {} | ||
|
|
||
| for mcp_server in config.mcp_servers: |
Contributor
There was a problem hiding this comment.
it would be nice to refactor these nested loops. At least move the internal loop into it's own function - then you will be basically "forced" to name params etc. And it will be better testable
| if mcp_server.authorization_headers: | ||
| unresolved = [ | ||
| h | ||
| for h in mcp_server.authorization_headers |
Contributor
There was a problem hiding this comment.
ideal four lines for new utility function - easy testable, understandable, usable in more places if needed
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
2 participants
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.
Description
Fixed LCORE-1461 which was caused by the
/toolsendpoint not handling "kubernetes" and static token auth.The fix involved creating a function that builds the complete MCP server headers and checking authorization on the complete MCP headers not just the passed MCP-HEADERS.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests