LCORE-1247: E2E Tests for MCP OAuth#1177
Open
jrobertboos wants to merge 16 commits intolightspeed-core:mainfrom
Open
LCORE-1247: E2E Tests for MCP OAuth#1177jrobertboos wants to merge 16 commits intolightspeed-core:mainfrom
jrobertboos wants to merge 16 commits intolightspeed-core:mainfrom
Conversation
…port - Removed the old `mcp-mock-server` service and replaced it with `mock-mcp` in both `docker-compose` files. - Updated health check and dependencies to reflect the new service name. - Modified E2E test configurations to use the new `mock-mcp` service URL. - Added a minimal mock MCP server implementation with OAuth support for testing. - Updated feature tests to check for OAuth authentication requirements and response headers.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds MCP (Model Context Protocol) OAuth test support: a mock OAuth-capable MCP server, docker-compose wiring, MCP-specific e2e configs and feature tests, test harness hooks to swap configs for MCP-tagged scenarios, and broadened tools endpoint authentication error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
rect rgba(200,200,255,0.5)
participant LS as Lightspeed Stack
end
rect rgba(200,255,200,0.5)
participant MCP as Mock-MCP (oauth)
end
Client->>LS: Request (e.g., POST /v1/query) without credentials
LS->>MCP: Probe MCP (HTTP) to determine auth requirement
MCP-->>LS: 401 + WWW-Authenticate: Bearer realm="mock-mcp", error="invalid_token"
LS-->>Client: 401 + JSON error (detail.cause = "MCP server at http://mock-mcp:3001 requires OAuth") + WWW-Authenticate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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 |
…port - Removed the old `mcp-mock-server` service and replaced it with `mock-mcp` in both `docker-compose` files. - Updated health check and dependencies to reflect the new service name. - Modified E2E test configurations to use the new `mock-mcp` service URL. - Added a minimal mock MCP server implementation with OAuth support for testing. - Updated feature tests to check for OAuth authentication requirements and response headers.
- Modified feature files to change the wording for checking response headers from "contains the following" to "contains the following header". - Updated the corresponding step definition in common_http.py to reflect the new header check format.
- Introduced new feature tests for MCP authentication scenarios, including checks for tools, query, and streaming_query endpoints. - Added MCP configuration files for both library and server modes. - Updated the environment setup to switch configurations based on feature tags. - Removed outdated authentication checks from existing feature files to streamline tests.
![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 (4)
tests/e2e/features/steps/common_http.py (1)
191-197: Prefer direct membership test over.keys()lookup.
context.response.headersis arequests.structures.CaseInsensitiveDict. Theinoperator on it already performs a case-insensitive lookup via__contains__, so calling.keys()adds an extraKeysViewindirection unnecessarily.♻️ Suggested simplification
- assert ( - header_name in context.response.headers.keys() - ), f"The response headers '{context.response.headers}' doesn't contain header '{header_name}'" + assert ( + header_name in context.response.headers + ), f"The response headers '{context.response.headers}' doesn't contain header '{header_name}'"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/steps/common_http.py` around lines 191 - 197, The membership test in check_response_headers_contains unnecessarily calls .keys() on context.response.headers; change the assertion to use direct membership (header_name in context.response.headers) since context.response.headers is a CaseInsensitiveDict and its __contains__ already does case-insensitive lookup — update the assert that currently checks header_name in context.response.headers.keys() to use header_name in context.response.headers and keep the rest of the error message unchanged.tests/e2e/mock_mcp_server/Dockerfile (1)
1-5: Root user is optional for test containers;pip installis not needed.
server.pyuses only Python standard library (json, http.server, typing), so the missingpip installstep is not a concern. The Dockerfile will function correctly as-is.The root user issue (Trivy DS-0002) remains a valid security best practice, but for E2E test infrastructure running in CI, adding a non-root user is optional rather than critical.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/mock_mcp_server/Dockerfile` around lines 1 - 5, The Dockerfile currently runs as root (CMD ["python", "server.py"]) which triggers DS-0002; to address this, create a non-root user and switch to it before CMD: add steps to create a group/user (e.g., "app" or "nonroot"), chown /app to that user, and set USER to that account so the container runs python server.py as non-root; ensure ownership change references the WORKDIR (/app) and the CMD entry (python server.py) remains unchanged.tests/e2e/configuration/library-mode/lightspeed-stack-mcp.yaml (1)
24-25: Consider documenting whyAuthorization: "oauth"is intentionally non-standard.The value
"oauth"is not a valid Bearer token (it lacks theBearerprefix), which is what causes the mock MCP server to return 401 — the exact behavior the E2E tests are exercising. Without a comment, this looks like a misconfiguration. The same applies to the server-mode config.✏️ Suggested clarification
mcp_servers: - name: "mcp-oauth" provider_id: "model-context-protocol" url: "http://mock-mcp:3001" authorization_headers: - Authorization: "oauth" + # Intentionally not a valid Bearer token — causes mock-mcp to return 401 + # so E2E tests can verify the OAuth error propagation path. + Authorization: "oauth"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/configuration/library-mode/lightspeed-stack-mcp.yaml` around lines 24 - 25, Add a short inline comment next to the authorization_headers Authorization: "oauth" entry explaining that the value is intentionally non-standard (missing the "Bearer " prefix) to deliberately trigger a 401 from the mock MCP server for the E2E tests; update the equivalent server-mode config similarly so both configs explicitly state this is intentional and not a misconfiguration (refer to the authorization_headers map and the Authorization key when making the change).tests/e2e/configuration/server-mode/lightspeed-stack-mcp.yaml (1)
25-26: Same documentation gap forAuthorization: "oauth"as in the library-mode config.The value
"oauth"is intentionally invalid to exercise the 401 path; a clarifying comment (as suggested for the library-mode YAML) would benefit this file equally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/configuration/server-mode/lightspeed-stack-mcp.yaml` around lines 25 - 26, Add a clarifying inline comment next to the Authorization header entry to indicate that Authorization: "oauth" is intentionally invalid and used to exercise the 401 authentication path; update the authorization_headers -> Authorization value comment so readers/testers understand this is deliberate (refer to the Authorization key under authorization_headers in the lightspeed-stack-mcp.yaml snippet).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/features/mcp.feature`:
- Line 23: The scenario titles in the feature file contain a duplicated word
"error error"; update both Scenario lines (the one shown and the other at the
same file referenced) to remove the duplicate so they read e.g. "Scenario: Check
if query endpoint reports error when mcp requires authentication" (apply the
same change to the other occurrence).
---
Nitpick comments:
In `@tests/e2e/configuration/library-mode/lightspeed-stack-mcp.yaml`:
- Around line 24-25: Add a short inline comment next to the
authorization_headers Authorization: "oauth" entry explaining that the value is
intentionally non-standard (missing the "Bearer " prefix) to deliberately
trigger a 401 from the mock MCP server for the E2E tests; update the equivalent
server-mode config similarly so both configs explicitly state this is
intentional and not a misconfiguration (refer to the authorization_headers map
and the Authorization key when making the change).
In `@tests/e2e/configuration/server-mode/lightspeed-stack-mcp.yaml`:
- Around line 25-26: Add a clarifying inline comment next to the Authorization
header entry to indicate that Authorization: "oauth" is intentionally invalid
and used to exercise the 401 authentication path; update the
authorization_headers -> Authorization value comment so readers/testers
understand this is deliberate (refer to the Authorization key under
authorization_headers in the lightspeed-stack-mcp.yaml snippet).
In `@tests/e2e/features/steps/common_http.py`:
- Around line 191-197: The membership test in check_response_headers_contains
unnecessarily calls .keys() on context.response.headers; change the assertion to
use direct membership (header_name in context.response.headers) since
context.response.headers is a CaseInsensitiveDict and its __contains__ already
does case-insensitive lookup — update the assert that currently checks
header_name in context.response.headers.keys() to use header_name in
context.response.headers and keep the rest of the error message unchanged.
In `@tests/e2e/mock_mcp_server/Dockerfile`:
- Around line 1-5: The Dockerfile currently runs as root (CMD ["python",
"server.py"]) which triggers DS-0002; to address this, create a non-root user
and switch to it before CMD: add steps to create a group/user (e.g., "app" or
"nonroot"), chown /app to that user, and set USER to that account so the
container runs python server.py as non-root; ensure ownership change references
the WORKDIR (/app) and the CMD entry (python server.py) remains unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docker-compose-library.yamldocker-compose.yamlsrc/app/endpoints/tools.pytests/e2e/configuration/library-mode/lightspeed-stack-mcp.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp.yamltests/e2e/features/environment.pytests/e2e/features/info.featuretests/e2e/features/mcp.featuretests/e2e/features/query.featuretests/e2e/features/steps/common_http.pytests/e2e/features/streaming_query.featuretests/e2e/mock_mcp_server/Dockerfiletests/e2e/mock_mcp_server/server.pytests/e2e/test_list.txt
- Also removed authentication from `mcp.feature`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/mock_mcp_server/server.py`:
- Around line 53-106: The Content-Length header parsing in do_POST currently
does int(self.headers.get("Content-Length", 0)) which can raise ValueError for
non-numeric headers and crash the handler; update do_POST to defensively parse
Content-Length (e.g., inspect headers.get("Content-Length"), try/except
ValueError around int(), or validate with str.isdigit()) and fall back to 0 on
parse failure, then use that safe length value for reading from self.rfile and
subsequent JSON decoding.
Comment on lines
+53
to
+106
| def do_POST(self) -> None: # pylint: disable=invalid-name | ||
| """Handle POST requests.""" | ||
| if self._parse_auth() is None: | ||
| self._require_oauth() | ||
| return | ||
|
|
||
| length = int(self.headers.get("Content-Length", 0)) | ||
| raw = self.rfile.read(length) if length else b"{}" | ||
| try: | ||
| req = json.loads(raw.decode("utf-8")) | ||
| req_id = req.get("id", 1) | ||
| method = req.get("method", "") | ||
| except (json.JSONDecodeError, UnicodeDecodeError): | ||
| req_id = 1 | ||
| method = "" | ||
|
|
||
| if method == "initialize": | ||
| self._json_response( | ||
| { | ||
| "jsonrpc": "2.0", | ||
| "id": req_id, | ||
| "result": { | ||
| "protocolVersion": "2024-11-05", | ||
| "capabilities": {"tools": {}}, | ||
| "serverInfo": {"name": "mock-mcp-e2e", "version": "1.0.0"}, | ||
| }, | ||
| } | ||
| ) | ||
| elif method == "tools/list": | ||
| self._json_response( | ||
| { | ||
| "jsonrpc": "2.0", | ||
| "id": req_id, | ||
| "result": { | ||
| "tools": [ | ||
| { | ||
| "name": "mock_tool_e2e", | ||
| "description": "Mock tool for E2E", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "message": { | ||
| "type": "string", | ||
| "description": "Test message", | ||
| } | ||
| }, | ||
| }, | ||
| } | ||
| ], | ||
| }, | ||
| } | ||
| ) | ||
| else: | ||
| self._json_response({"jsonrpc": "2.0", "id": req_id, "result": {}}) |
Contributor
There was a problem hiding this comment.
Guard invalid Content-Length to avoid 500s on malformed headers.
int() can raise ValueError if the header is non-numeric, which would crash the request handler. Consider a safe fallback.
🛠️ Suggested defensive parse
- length = int(self.headers.get("Content-Length", 0))
+ try:
+ length = int(self.headers.get("Content-Length", 0))
+ except ValueError:
+ length = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/mock_mcp_server/server.py` around lines 53 - 106, The
Content-Length header parsing in do_POST currently does
int(self.headers.get("Content-Length", 0)) which can raise ValueError for
non-numeric headers and crash the handler; update do_POST to defensively parse
Content-Length (e.g., inspect headers.get("Content-Length"), try/except
ValueError around int(), or validate with str.isdigit()) and fall back to 0 on
parse failure, then use that safe length value for reading from self.rfile and
subsequent JSON decoding.
- Implemented scenarios to verify successful and error responses for the 'tools', 'query', and 'streaming_query' endpoints when valid and invalid MCP auth tokens are provided. - Enhanced the common HTTP step definitions to support setting headers dynamically. - Updated the mock MCP server to handle invalid tokens appropriately.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/e2e/mock_mcp_server/server.py (1)
53-66:⚠️ Potential issue | 🟡 MinorGuard Content-Length parsing to avoid 500s on malformed headers.
int()will raiseValueErrorfor non-numeric values.🛠️ Suggested defensive parse
- length = int(self.headers.get("Content-Length", 0)) + try: + length = int(self.headers.get("Content-Length", 0)) + except ValueError: + length = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/mock_mcp_server/server.py` around lines 53 - 66, In do_POST, parsing Content-Length with int(self.headers.get("Content-Length", 0)) can raise ValueError for non-numeric headers; wrap that conversion in a try/except (catch ValueError and TypeError) and fallback to 0 (or a safe default) before reading self.rfile, so length is always an integer; update the logic around the length, raw, and subsequent json.loads handling to use the guarded length value (references: do_POST, length, self.headers.get("Content-Length"), raw).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/e2e/mock_mcp_server/server.py`:
- Around line 53-66: In do_POST, parsing Content-Length with
int(self.headers.get("Content-Length", 0)) can raise ValueError for non-numeric
headers; wrap that conversion in a try/except (catch ValueError and TypeError)
and fallback to 0 (or a safe default) before reading self.rfile, so length is
always an integer; update the logic around the length, raw, and subsequent
json.loads handling to use the guarded length value (references: do_POST,
length, self.headers.get("Content-Length"), raw).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/e2e/features/mcp.featuretests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.pytests/e2e/mock_mcp_server/server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/features/steps/common_http.py
tisnik
requested changes
Feb 24, 2026
radofuchs
reviewed
Feb 25, 2026
| context.response = requests.post(url, json=data, timeout=DEFAULT_LLM_TIMEOUT) | ||
|
|
||
|
|
||
| @step('I use "{endpoint}" to ask question with headers') |
Contributor
There was a problem hiding this comment.
why do you need to define new step for this? It only adds confusion since you do not change the body in any way
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
3 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
Created e2e tests for MCP OAuth. The tests that were created are only meant to verify that upon attempted access of a secured MCP server a
401 Unauthorizedis returned that contains thewww-authenticateheader.These were tested for the following endpoints:
/query/streaming_query/toolsAdditionally a bug was fixed for the
/toolsendpoint which would suffer internal failure when in library mode due to a nonllama-stack-clientdefined error being raised. See/src/app/endpoints/tools.pyfor the bug fix.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