fix(platform-api): return ValidationFailed on upstream MCP server auth errors - #3180
fix(platform-api): return ValidationFailed on upstream MCP server auth errors#3180lahiruudayakumara wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMCP initialization now returns validation errors for upstream HTTP authorization and other non-success responses. A regression test verifies that an upstream 401 does not become an unauthorized error. ChangesMCP initialization error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
platform-api/internal/service/mcp_test.go (2)
261-264: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEnsure the test reaches MCP initialization.
FetchServerInfoperformsCheckURLReachabilitybeforeFetchMCPServerInfo. Because this handler returns 401 for every request, the test does not prove that the changed initialize request handled the 401.Return success for the reachability probe, return 401 only for the initialize
POST, and assert that thePOSTwas received.Based on the supplied service flow, the reachability check runs before MCP initialization.
🤖 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 `@platform-api/internal/service/mcp_test.go` around lines 261 - 264, Update the httptest server handler in the MCP initialization test to return a successful response for the reachability probe, but return 401 only for the MCP initialize POST request. Track and assert that the initialize POST was received, ensuring the test exercises the changed initialization behavior after CheckURLReachability.
270-276: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover all new non-success status branches.
The production code adds separate handling for 403 and other non-2xx responses, but this test covers only 401. Add table-driven cases for 403 and a representative status such as 500. If HTTP 400 is part of the public contract, add one handler-level assertion because the current service-level test does not verify the mapped HTTP status.
Based on the PR objective and the changed status branches, the regression test should cover the complete error-classification contract.
🤖 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 `@platform-api/internal/service/mcp_test.go` around lines 270 - 276, Expand the FetchServerInfo error test around the existing 401 case into table-driven coverage for 401, 403, and a representative non-2xx status such as 500, asserting each status maps to the expected error classification. If HTTP 400 is part of the public contract, add a handler-level assertion for its mapped response status.
🤖 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 `@platform-api/internal/utils/mcp.go`:
- Around line 304-306: Update the non-2xx response handling in the MCP
initialize request to stop passing string(body) directly to WithLogMessage.
Preserve the status code while logging only bounded, sanitized metadata or an
established redacted diagnostic value, ensuring untrusted response content and
log-control characters are never written verbatim.
---
Nitpick comments:
In `@platform-api/internal/service/mcp_test.go`:
- Around line 261-264: Update the httptest server handler in the MCP
initialization test to return a successful response for the reachability probe,
but return 401 only for the MCP initialize POST request. Track and assert that
the initialize POST was received, ensuring the test exercises the changed
initialization behavior after CheckURLReachability.
- Around line 270-276: Expand the FetchServerInfo error test around the existing
401 case into table-driven coverage for 401, 403, and a representative non-2xx
status such as 500, asserting each status maps to the expected error
classification. If HTTP 400 is part of the public contract, add a handler-level
assertion for its mapped response status.
🪄 Autofix
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 Plus
Run ID: 1d0e6fc5-376e-49b3-a53e-2260d81a305c
📒 Files selected for processing (2)
platform-api/internal/service/mcp_test.goplatform-api/internal/utils/mcp.go
1 participant
Purpose
When setting up an MCP proxy for a secured server, omitting or supplying invalid required credentials triggers an unexpected workspace logout. The platform API returned an HTTP 401 Unauthorized error for upstream authentication failures, which the SPA API client interpreted as an expired user session.
Resolves #3173
Goals
Ensure that upstream MCP server authentication failures during endpoint validation (
fetch-server-info) return an HTTP 400 Bad Request (apperror.ValidationFailed) instead of an HTTP 401 Unauthorized. This presents a clear error notification in the UI without terminating the user's active workspace session.Approach
Updated
initializeMCPServerinplatform-api/internal/utils/mcp.go:apperror.Unauthorized.New()withapperror.ValidationFailed.New(...)when the target MCP server returns HTTP 401 Unauthorized during initialize.apperror.ValidationFailed(HTTP 400 Bad Request).platform-api/internal/service/mcp_test.goto assert that upstream 401 Unauthorized returnsapperror.ValidationFailed.User stories
As an AI Workspace user creating or configuring an MCP proxy, when I fetch info for an upstream MCP server with invalid or missing credentials, I should see an inline validation error message rather than being logged out of the workspace.
Documentation
N/A - Internal error mapping fix for MCP proxy validation; no user documentation impact.
Automation tests
TestFetchServerInfoUpstreamUnauthorizedReturnsValidationFailedinplatform-api/internal/service/mcp_test.go.platform-api/internal/service/...andplatform-api/internal/utils/...pass cleanly.Security checks
Samples
N/A
Related PRs
N/A
Test environment