fix(oauth): check for OAuth error responses even when status code is 200 #646
Conversation
WalkthroughReads and buffers HTTP response bodies in OAuth flows, detects and returns OAuth-specific errors by unmarshalling the response body, and decodes tokens from the buffered bytes in both Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
sd2k
force-pushed
the
oauth-handle-200-error-responses
branch
from
1b6c866 to
61e9593
Compare
sd2k
changed the title
sd2k
force-pushed
the
oauth-handle-200-error-responses
branch
from
61e9593 to
3b9c415
Compare
sd2k
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
client/transport/oauth.go (2)
255-260: Minor redundancy: double JSON unmarshal of OAuth error.The code successfully parses
oauthErrbut then callsextractOAuthErrorwhich parses the same body again. Consider returning the error directly:// GitHub returns HTTP 200 even for errors, with error details in the JSON body // Check if the response contains an error field before parsing as Token var oauthErr OAuthError if err := json.Unmarshal(body, &oauthErr); err == nil && oauthErr.ErrorCode != "" { - return nil, extractOAuthError(body, resp.StatusCode, "refresh token request failed") + return nil, fmt.Errorf("refresh token request failed: %w", oauthErr) }
701-706: Same redundancy as in refreshToken.Same optional refactor applies here - the parsed
oauthErrcan be returned directly.// GitHub returns HTTP 200 even for errors, with error details in the JSON body // Check if the response contains an error field before parsing as Token var oauthErr OAuthError if err := json.Unmarshal(body, &oauthErr); err == nil && oauthErr.ErrorCode != "" { - return extractOAuthError(body, resp.StatusCode, "token request failed") + return fmt.Errorf("token request failed: %w", oauthErr) }client/oauth_test.go (2)
129-165: Consider using testify for test assertions.As per coding guidelines, tests should use
testify/assertandtestify/require. The current tests use manualt.Errorfcalls. Consider refactoring to table-driven tests with testify assertions for consistency with codebase conventions.Example refactor for
TestGetResourceMetadataURL:func TestGetResourceMetadataURL(t *testing.T) { metadataURL := "https://auth.example.com/.well-known/oauth-protected-resource" tests := []struct { name string err error expected string }{ { name: "OAuth error with metadata URL", err: &transport.OAuthAuthorizationRequiredError{ Handler: transport.NewOAuthHandler(transport.OAuthConfig{}), AuthorizationRequiredError: transport.AuthorizationRequiredError{ ResourceMetadataURL: metadataURL, }, }, expected: metadataURL, }, // ... additional cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { assert.Equal(t, tt.expected, GetResourceMetadataURL(tt.err)) }) } }
185-196: Consider adding assertion for embedded type compatibility.Since
OAuthAuthorizationRequiredErrorembedsAuthorizationRequiredError, you might want to verify thatIsAuthorizationRequiredError(oauthErr)also returnstrue. This would confirm the embedded type is correctly recognized byerrors.As.// Verify IsOAuthAuthorizationRequiredError returns true if !IsOAuthAuthorizationRequiredError(oauthErr) { t.Errorf("Expected IsOAuthAuthorizationRequiredError to return true for OAuthAuthorizationRequiredError") } + + // Verify IsAuthorizationRequiredError also returns true (embedded type) + if !IsAuthorizationRequiredError(oauthErr) { + t.Errorf("Expected IsAuthorizationRequiredError to return true for OAuthAuthorizationRequiredError (embedded)") + }client/transport/streamable_http_oauth_test.go (1)
287-334: Good table-driven test coverage.The test cases comprehensively cover various WWW-Authenticate header formats per RFC9728. Consider adding one more edge case for
resource_metadatawithout quotes (malformed but might appear in the wild).+ { + name: "Malformed resource_metadata (no quotes)", + wwwAuth: `Bearer resource_metadata=https://example.com/metadata`, + expectedURL: "", + },client/transport/streamable_http.go (1)
330-349: Consider extracting duplicated 401 handling logic.The 401 handling in
SendRequest(lines 330-349) andSendNotification(lines 619-638) is nearly identical. Consider extracting to a helper method:// handleUnauthorizedResponse creates the appropriate authorization error for 401 responses func (c *StreamableHTTP) handleUnauthorizedResponse(resp *http.Response) error { metadataURL := extractResourceMetadataURL(resp.Header.Get("WWW-Authenticate")) if c.oauthHandler != nil { return &OAuthAuthorizationRequiredError{ Handler: c.oauthHandler, AuthorizationRequiredError: AuthorizationRequiredError{ ResourceMetadataURL: metadataURL, }, } } return &AuthorizationRequiredError{ ResourceMetadataURL: metadataURL, } }Then call
return nil, c.handleUnauthorizedResponse(resp)in both locations.Also applies to: 619-638
client/transport/sse.go (1)
168-186: Consider extracting 401 handling into a helper method.The 401 unauthorized handling logic is duplicated across
Start(),SendRequest(), andSendNotification(). A helper could reduce duplication.// handleUnauthorized returns the appropriate error for a 401 response. func (c *SSE) handleUnauthorized(resp *http.Response) error { metadataURL := extractResourceMetadataURL(resp.Header.Get("WWW-Authenticate")) if c.oauthHandler != nil { return &OAuthAuthorizationRequiredError{ Handler: c.oauthHandler, AuthorizationRequiredError: AuthorizationRequiredError{ ResourceMetadataURL: metadataURL, }, } } return &AuthorizationRequiredError{ResourceMetadataURL: metadataURL} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/ci.yml(1 hunks)client/oauth.go(2 hunks)client/oauth_test.go(1 hunks)client/transport/oauth.go(4 hunks)client/transport/oauth_test.go(2 hunks)client/transport/sse.go(6 hunks)client/transport/sse_oauth_test.go(1 hunks)client/transport/streamable_http.go(4 hunks)client/transport/streamable_http_oauth_test.go(1 hunks)examples/oauth_client/README.md(3 hunks)examples/oauth_client/main.go(1 hunks)server/session.go(1 hunks)server/streamable_http_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/streamable_http_test.goserver/session.goclient/transport/oauth.goexamples/oauth_client/main.goclient/transport/sse_oauth_test.goclient/oauth.goclient/transport/oauth_test.goclient/oauth_test.goclient/transport/sse.goclient/transport/streamable_http_oauth_test.goclient/transport/streamable_http.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/streamable_http_test.goclient/transport/sse_oauth_test.goclient/transport/oauth_test.goclient/oauth_test.goclient/transport/streamable_http_oauth_test.go
🧠 Learnings (5)
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-03-04T06:59:43.882Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
client/transport/oauth_test.go
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Applied to files:
client/transport/oauth_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Use the provided commands for testing, coverage, linting, and code generation (go test ./... -race, coverage command with exclusions, golangci-lint run, go generate ./...)
Applied to files:
.github/workflows/ci.yml
🧬 Code graph analysis (6)
examples/oauth_client/main.go (1)
client/oauth.go (1)
GetResourceMetadataURL(91-105)
client/oauth.go (1)
client/transport/streamable_http.go (2)
AuthorizationRequiredError(266-268)OAuthAuthorizationRequiredError(280-283)
client/transport/oauth_test.go (2)
client/transport/oauth.go (4)
NewMemoryTokenStore(99-101)OAuthConfig(21-44)TokenStore(55-66)NewOAuthHandler(157-169)client/oauth.go (3)
NewMemoryTokenStore(23-23)OAuthConfig(11-11)TokenStore(17-17)
client/oauth_test.go (1)
client/oauth.go (6)
OAuthAuthorizationRequiredError(64-64)OAuthConfig(11-11)AuthorizationRequiredError(61-61)GetResourceMetadataURL(91-105)IsAuthorizationRequiredError(67-70)IsOAuthAuthorizationRequiredError(73-76)
client/transport/sse.go (2)
client/oauth.go (2)
AuthorizationRequiredError(61-61)OAuthAuthorizationRequiredError(64-64)client/transport/streamable_http.go (2)
AuthorizationRequiredError(266-268)OAuthAuthorizationRequiredError(280-283)
client/transport/streamable_http.go (3)
client/oauth.go (2)
AuthorizationRequiredError(61-61)OAuthAuthorizationRequiredError(64-64)client/transport/error.go (1)
Error(6-8)client/transport/oauth.go (1)
OAuthHandler(144-154)
🔇 Additional comments (30)
server/streamable_http_test.go (1)
2254-2341: Well-structured regression test for issue #638.The test properly validates that adding a tool during an in-flight tool call doesn't break the response. The timing strategy (50ms wait before adding tool vs 100ms tool delay) provides sufficient overlap to trigger the race condition scenario.
Minor observation: the result struct could use a named type for clarity, but this is acceptable for a localized test.
.github/workflows/ci.yml (2)
10-17: LGTM - CI workflow properly configured.The test job correctly uses
go-version-filefor version management and includes-raceflag for race detection as per coding guidelines.
19-35: Verify-codegen job looks good.The codegen verification properly checks for uncommitted changes after running
go generate ./....server/session.go (4)
92-106: Well-designed interface extension for SSE upgrade.The
SessionWithStreamableHTTPConfiginterface follows the established pattern in this file (e.g.,SessionWithLogging,SessionWithTools). The documentation clearly references the MCP protocol specification for the SSE upgrade behavior.
174-176: SSE upgrade timing is correct.The upgrade is invoked before sending the notification to the channel, ensuring the HTTP response can switch to SSE format before any data is written. This is the correct ordering per the protocol specification.
201-226: Consistent SSE upgrade handling in specific client notification path.The same upgrade pattern is applied here as in
sendNotificationToAllClients, maintaining consistency across notification paths.
279-308: Core notification function properly handles SSE upgrade.The
sendNotificationCorefunction correctly upgrades to SSE before attempting to send. The error handling for blocked channels remains intact.examples/oauth_client/README.md (2)
8-8: Good addition of RFC9728 feature highlight.The feature list now prominently documents the automatic OAuth metadata discovery capability.
65-78: Helpful RFC9728 section with working code example.The documentation clearly explains the metadata discovery flow and provides a code snippet demonstrating
GetResourceMetadataURL. This helps users understand how to leverage the new functionality.examples/oauth_client/main.go (1)
111-115: Example code correctly demonstrates RFC9728 metadata URL discovery.The code properly uses
client.GetResourceMetadataURL(err)to extract the metadata URL from authorization errors. The informational output helps users understand when the server provides this URL.client/transport/sse_oauth_test.go (1)
243-304: Good test coverage for RFC9728 metadata URL extraction in SSE.The test correctly validates that:
- The WWW-Authenticate header is parsed for
resource_metadataparameter- The extracted URL is propagated through the
OAuthAuthorizationRequiredErrorThe test setup with a valid token ensures the request actually reaches the server and receives the 401 response with the header.
client/transport/oauth_test.go (8)
12-14: Testify imports added per coding guidelines.The addition of
testify/assertandtestify/requirealigns with the coding guidelines for**/*_test.gofiles.
946-1005: Core test for the PR fix - validates GitHub's HTTP 200 error response handling.This test directly addresses the issue described in the PR: GitHub returns HTTP 200 but with an error object in the response body. The test correctly:
- Simulates GitHub's non-compliant behavior with
bad_refresh_tokenerror in a 200 response- Verifies the error is properly detected and contains the OAuth error details
This is the essential test demonstrating the fix works.
1007-1065: Test for empty access_token response parsing.This test documents that mcp-go parses responses as-is without validating empty tokens - that responsibility lies with the integration layer. The test comments clearly explain this design decision.
Note: Uses
require.NoErrorandassert.Equalfrom testify, which is the recommended pattern per coding guidelines.
1067-1153: Comprehensive refresh token rotation test.Tests GitHub's token rotation behavior where each refresh returns a new refresh token. The test validates:
- First refresh uses original token, receives rotated token
- Second refresh uses the new token correctly
- Tokens are actually different after rotation
1155-1249: Single-use refresh token behavior test.Tests that reusing an already-used refresh token fails appropriately, simulating GitHub's single-use behavior. The test correctly verifies that:
- First use succeeds
- Reuse fails with
bad_refresh_token- Using the NEW token works
This covers an important edge case in token management.
1251-1314: Authorization code exchange error detection test.Tests that errors in the token exchange response (during authorization code flow) are properly detected even when returned with HTTP 200. Also verifies no empty token is saved.
1316-1378: Test for preserving old refresh token when not rotated.Important test for providers that don't return a new refresh token on each refresh - the old one should be preserved. This prevents losing refresh capability.
1380-1433: Spec-compliant HTTP 400 error handling test.Validates that proper HTTP 400 error responses (per OAuth spec) are still handled correctly. This ensures the new HTTP 200 error detection doesn't break standard error handling.
client/transport/oauth.go (2)
35-38: LGTM!The new
ProtectedResourceMetadataURLfield is well-documented with appropriate GoDoc explaining RFC9728 context and typical usage.
378-388: LGTM!The priority-based approach for protected resource metadata URL discovery is well-structured with clear comments explaining the fallback behavior.
client/oauth.go (2)
60-70: LGTM!The type alias and helper function are well-implemented with proper GoDoc comments and correct use of
errors.Asfor type checking.
87-105: LGTM!
GetResourceMetadataURLcorrectly prioritizesOAuthAuthorizationRequiredError(which contains the embedded type) before falling back to the baseAuthorizationRequiredError. This ensures the most specific error type is checked first.client/transport/streamable_http_oauth_test.go (1)
222-285: LGTM!Well-structured integration test that validates the end-to-end flow of extracting
resource_metadatafrom the WWW-Authenticate header and propagating it throughOAuthAuthorizationRequiredError.client/transport/streamable_http.go (3)
240-262: LGTM!The
extractResourceMetadataURLfunction correctly implements RFC9728 parsing with proper bounds checking. The sentinel error and function are well-documented.
264-291: LGTM!The error types are well-designed:
AuthorizationRequiredErrorimplementsUnwrap()for proper error chain compatibility witherrors.IsOAuthAuthorizationRequiredErroruses struct embedding for clean composition
438-444: Acceptable empty ResourceMetadataURL in pre-request error path.Setting
ResourceMetadataURL: ""is correct here since there's no HTTP response to extract it from whenGetAuthorizationHeaderfails. The comment makes this clear.client/transport/sse.go (3)
168-186: LGTM - 401 handling with RFC9728 metadata extraction.The logic correctly extracts the resource metadata URL from the
WWW-Authenticateheader and returns the appropriate error type based on OAuth handler presence.
553-559: LGTM - Correct error checking pattern.This uses
errors.Is(err, ErrOAuthAuthorizationRequired)which is the proper approach. The other methods (Start()andSendRequest()) should be updated to match this pattern.
579-600: LGTM - Consistent 401 handling.The 401 handling logic properly extracts the metadata URL and returns the appropriate error type.
| if err.Error() == "no valid token available, authorization required" { | ||
| return &OAuthAuthorizationRequiredError{ | ||
| Handler: c.oauthHandler, | ||
| AuthorizationRequiredError: AuthorizationRequiredError{ | ||
| ResourceMetadataURL: "", // No response available in this code path | ||
| }, | ||
| } |
There was a problem hiding this comment.
Inconsistent error checking pattern across methods.
This uses string comparison (err.Error() == "...") while SendNotification at line 553 uses errors.Is(err, ErrOAuthAuthorizationRequired). The string comparison is fragile and can break if the error message changes.
Apply this diff for consistency with SendNotification:
- if err.Error() == "no valid token available, authorization required" {
+ if errors.Is(err, ErrOAuthAuthorizationRequired) {📝 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.
| if err.Error() == "no valid token available, authorization required" { | |
| return &OAuthAuthorizationRequiredError{ | |
| Handler: c.oauthHandler, | |
| AuthorizationRequiredError: AuthorizationRequiredError{ | |
| ResourceMetadataURL: "", // No response available in this code path | |
| }, | |
| } | |
| if errors.Is(err, ErrOAuthAuthorizationRequired) { | |
| return &OAuthAuthorizationRequiredError{ | |
| Handler: c.oauthHandler, | |
| AuthorizationRequiredError: AuthorizationRequiredError{ | |
| ResourceMetadataURL: "", // No response available in this code path | |
| }, | |
| } |
🤖 Prompt for AI Agents
In client/transport/sse.go around lines 148 to 154, replace the fragile string
comparison err.Error() == "no valid token available, authorization required"
with a proper errors.Is(err, ErrOAuthAuthorizationRequired) check used
elsewhere; update the conditional to use errors.Is and keep the returned
OAuthAuthorizationRequiredError construction the same (preserve Handler and
empty ResourceMetadataURL), ensuring the errors package is imported if not
already.
| if err.Error() == "no valid token available, authorization required" { | ||
| return nil, &OAuthAuthorizationRequiredError{ | ||
| Handler: c.oauthHandler, | ||
| AuthorizationRequiredError: AuthorizationRequiredError{ | ||
| ResourceMetadataURL: "", // No response available in this code path | ||
| }, | ||
| } |
There was a problem hiding this comment.
Same fragile string comparison as in Start().
Apply consistent error checking using errors.Is:
- if err.Error() == "no valid token available, authorization required" {
+ if errors.Is(err, ErrOAuthAuthorizationRequired) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In client/transport/sse.go around lines 394 to 400, replace the fragile string
equality check err.Error() == "no valid token available, authorization required"
with a proper errors.Is check against a sentinel error (e.g. ErrNoValidToken) or
the package-level error exported by the auth/oauth package; if no sentinel
exists, add one (var ErrNoValidToken = errors.New("no valid token available,
authorization required")). Also add the errors import if missing, and keep
returning the same OAuthAuthorizationRequiredError struct when errors.Is(...)
matches.
client/transport/sse.go
Outdated
| // Handle unauthorized error | ||
| if resp.StatusCode == http.StatusUnauthorized { | ||
| // Extract discovered metadata URL per RFC9728 | ||
| metadataURL := extractResourceMetadataURL(resp.Header.Get("WWW-Authenticate")) | ||
|
|
||
| // If OAuth handler exists, return OAuth-specific error | ||
| if c.oauthHandler != nil { | ||
| return nil, &OAuthAuthorizationRequiredError{ | ||
| Handler: c.oauthHandler, | ||
| AuthorizationRequiredError: AuthorizationRequiredError{ | ||
| ResourceMetadataURL: metadataURL, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| // Handle OAuth unauthorized error | ||
| if resp.StatusCode == http.StatusUnauthorized && c.oauthHandler != nil { | ||
| return nil, &OAuthAuthorizationRequiredError{ | ||
| Handler: c.oauthHandler, | ||
| // No OAuth handler, return base authorization error | ||
| return nil, &AuthorizationRequiredError{ | ||
| ResourceMetadataURL: metadataURL, | ||
| } |
There was a problem hiding this comment.
Resource leak: response channel not cleaned up on 401 error path.
When returning early due to a 401 (or other non-OK status), the response channel registered at lines 418-420 is never removed from c.responses. This causes a memory leak as orphaned channels accumulate.
Add cleanup before returning:
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusAccepted {
+ deleteResponseChan()
// Handle unauthorized error
if resp.StatusCode == http.StatusUnauthorized {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In client/transport/sse.go around lines 443 to 461, the code returns early on a
401 without removing the response channel that was registered earlier (lines
~418-420), leaking the channel and map entry; before each early return on this
non-OK path remove the channel from c.responses (under the same mutex used when
adding it) and close the channel to free resources — i.e., lock, delete
c.responses[requestID], unlock, close(respCh) (or close under lock if your
convention requires), then return the OAuthAuthorizationRequiredError or
AuthorizationRequiredError as currently done.
Certain providers (**cough** GitHub **cough**) return HTTP 200 responses with an error in the body for their OAuth token exchange and token refresh endpoints. Previously, we only checked for non-200 status codes, which could lead to us missing the errors, attempting to unmarshal the body as a token, failing to detect the error, and even saving the empty token. This commit attempts to unmarshal the body as an OAuth error response even when the status code is 200. If the body is not an OAuth error response, we fall back to the previous behavior.
sd2k
force-pushed
the
oauth-handle-200-error-responses
branch
from
3b9c415 to
f6887ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/transport/oauth_test.go (1)
947-1005: Prefer testify assertions for consistency.Most of the new test functions use traditional
if/t.Fatal/t.Errorfpatterns, whileTestOAuthHandler_RefreshToken_EmptyAccessTokencorrectly usesrequire.NoErrorandassert.Equal. As per coding guidelines and the learnings retrieved, all tests should use testify/assert and testify/require for consistency.Example refactor for lines 994-1004:
- // Should detect the error even though status code is 200 - if err == nil { - t.Fatal("Expected error when GitHub returns error in 200 response, got nil") - } - - // Should contain the OAuth error details - if !strings.Contains(err.Error(), "bad_refresh_token") { - t.Errorf("Expected error to contain 'bad_refresh_token', got: %v", err) - } - if !strings.Contains(err.Error(), "incorrect or expired") { - t.Errorf("Expected error to contain error description, got: %v", err) - } + // Should detect the error even though status code is 200 + require.Error(t, err, "Expected error when GitHub returns error in 200 response") + + // Should contain the OAuth error details + assert.Contains(t, err.Error(), "bad_refresh_token") + assert.Contains(t, err.Error(), "incorrect or expired")Apply similar refactoring to the other test functions for consistency. Based on coding guidelines.
Also applies to: 1067-1153, 1155-1249, 1251-1314, 1316-1378, 1380-1433
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/transport/oauth.go(2 hunks)client/transport/oauth_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/transport/oauth.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/oauth_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/oauth_test.go
🧠 Learnings (2)
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
client/transport/oauth_test.go
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Applied to files:
client/transport/oauth_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
client/transport/oauth_test.go (1)
12-14: Good addition of testify imports.The testify/assert and testify/require imports align with the coding guidelines for Go tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/transport/oauth_test.go (1)
947-1371: New OAuth refresh/authorization tests look solid; consider also asserting token store is not mutated on errorThe new tests around GitHub-style HTTP 200-with-error bodies, refresh-token rotation, single-use refresh tokens, and proper HTTP 400 handling correctly exercise the updated behavior and use
require/assertidiomatically. To more directly guard against the original bug (saving an empty token when the body is actually an OAuth error), it would be worthwhile to also assertTokenStorecontents in the refresh error paths, similar to what you already do inTestOAuthHandler_ProcessAuthorizationResponse_ErrorIn200.For example, you could extend the error-in-200 and HTTP 400 refresh tests like this:
func TestOAuthHandler_RefreshToken_GitHubErrorIn200Response(t *testing.T) { @@ ctx := context.Background() _, err := handler.RefreshToken(ctx, "bad-refresh-token") // Should detect the error even though status code is 200 require.Error(t, err, "Expected error when GitHub returns error in 200 response") assert.Contains(t, err.Error(), "bad_refresh_token", "Error should contain OAuth error code") assert.Contains(t, err.Error(), "incorrect or expired", "Error should contain error description") + + // And we shouldn't persist an empty/bad token + if savedToken, getErr := tokenStore.GetToken(ctx); getErr == nil { + assert.NotEmpty(t, savedToken.AccessToken, "Empty access token should not be saved after OAuth error") + } }func TestOAuthHandler_RefreshToken_SingleUseRefreshToken(t *testing.T) { @@ // Try to use the SAME refresh token again - should fail _, err = handler.RefreshToken(ctx, "ghr_original") require.Error(t, err, "Expected error when reusing old refresh token") assert.Contains(t, err.Error(), "bad_refresh_token", "Error should contain bad_refresh_token") assert.Contains(t, err.Error(), "OAuth error", "Error should be wrapped as OAuth error") + + // Ensure the previously successful token is still stored after the failed reuse + savedToken, getErr := tokenStore.GetToken(ctx) + require.NoError(t, getErr) + assert.Equal(t, token1.RefreshToken, savedToken.RefreshToken, "Stored refresh token should remain unchanged after failed reuse") @@ }func TestOAuthHandler_RefreshToken_ProperHTTP400Error(t *testing.T) { @@ // Attempt refresh _, err := handler.RefreshToken(ctx, "invalid-token") // Should fail with OAuth error require.Error(t, err, "Expected error for HTTP 400 response") assert.Contains(t, err.Error(), "invalid_grant", "Error should contain invalid_grant") + + // And no empty token should be persisted on HTTP 400 error + if savedToken, getErr := tokenStore.GetToken(ctx); getErr == nil { + assert.NotEmpty(t, savedToken.AccessToken, "Empty access token should not be saved for HTTP 400 error") + } }These additions keep the tests focused while explicitly asserting that error responses (both 200+error and 400+error) do not result in saving an empty or bogus token, which is exactly the regression this PR is aiming to prevent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/transport/oauth.go(2 hunks)client/transport/oauth_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/transport/oauth.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/oauth_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/oauth_test.go
🧠 Learnings (1)
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
client/transport/oauth_test.go
🧬 Code graph analysis (1)
client/transport/oauth_test.go (2)
client/transport/oauth.go (4)
NewMemoryTokenStore(95-97)OAuthConfig(21-40)TokenStore(51-62)NewOAuthHandler(153-165)client/oauth.go (3)
NewMemoryTokenStore(23-23)OAuthConfig(11-11)TokenStore(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
client/transport/oauth_test.go (1)
13-14: Test imports updated to use testify correctlyUsing
github.com/stretchr/testify/assertandrequirehere aligns with the repo’s testing guidelines for_test.gofiles, and the import grouping (std → third-party) remains correct.Based on learnings, this matches the preferred testing style.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
client/transport/oauth_test.go (4)
999-1001: Consider asserting no token is saved rather than checking if saved token is non-empty.The comment states "Empty access token should not be saved after OAuth error", but the current logic checks "if a token exists, it should not be empty". Since the bug being fixed is about saving empty tokens on error, a clearer assertion would verify no token is saved at all:
- // Verify no empty token was saved to token store (regression test for original bug) - if savedToken, getErr := tokenStore.GetToken(ctx); getErr == nil { - assert.NotEmpty(t, savedToken.AccessToken, "Empty access token should not be saved after OAuth error in 200 response") - } + // Verify no token was saved to token store (regression test for original bug) + _, getErr := tokenStore.GetToken(ctx) + assert.ErrorIs(t, getErr, ErrNoToken, "No token should be saved after OAuth error in 200 response")Note: The current code works correctly (the if-condition won't be entered if no token is saved), but this makes the intent clearer.
1095-1098: Usestrconv.Itoaorfmt.Sprintffor clearer integer-to-string conversion.The pattern
string(rune('0'+callCount))works for single digits but is not idiomatic Go. Consider using standard conversion functions for better readability:- "access_token": "ghu_access_" + string(rune('0'+callCount)), + "access_token": "ghu_access_" + strconv.Itoa(callCount), "token_type": "bearer", "expires_in": 28800, - "refresh_token": "ghr_refresh_" + string(rune('0'+callCount)), // New token each time + "refresh_token": "ghr_refresh_" + strconv.Itoa(callCount), // New token each timeAdd
strconvto imports if not already present.
1269-1273: Consider asserting no token is saved rather than checking if saved token is non-empty.Similar to the earlier test, the assertion logic could be clearer:
- // Verify no empty token was saved - savedToken, getErr := tokenStore.GetToken(ctx) - if getErr == nil { - assert.NotEmpty(t, savedToken.AccessToken, "Empty access token should not have been saved") - } + // Verify no token was saved + _, getErr := tokenStore.GetToken(ctx) + assert.ErrorIs(t, getErr, ErrNoToken, "No token should be saved after OAuth error in authorization response")
1382-1385: Consider asserting no token is saved rather than checking if saved token is non-empty.For consistency with the other tests, consider the clearer assertion pattern:
- // Verify no empty token was saved to token store (regression test for original bug) - if savedToken, getErr := tokenStore.GetToken(ctx); getErr == nil { - assert.NotEmpty(t, savedToken.AccessToken, "Empty access token should not be saved for HTTP 400 error") - } + // Verify no token was saved to token store (regression test for original bug) + _, getErr := tokenStore.GetToken(ctx) + assert.ErrorIs(t, getErr, ErrNoToken, "No token should be saved after HTTP 400 error")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/oauth_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/oauth_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/oauth_test.go
🧠 Learnings (1)
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
client/transport/oauth_test.go
🧬 Code graph analysis (1)
client/transport/oauth_test.go (3)
client/transport/oauth.go (4)
NewMemoryTokenStore(95-97)OAuthConfig(21-40)TokenStore(51-62)NewOAuthHandler(153-165)client/oauth.go (3)
NewMemoryTokenStore(23-23)OAuthConfig(11-11)TokenStore(17-17)mcptest/mcptest.go (1)
NewServer(44-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
client/transport/oauth_test.go (4)
12-14: LGTM! Correct use of testify per coding guidelines.The imports correctly follow the standard library → third-party → local packages ordering, and testify/assert and testify/require are properly imported as specified in the coding guidelines.
1004-1062: LGTM! Good documentation of responsibility boundaries.This test clearly documents that mcp-go parses responses without validation, while the integration layer (MCPTokenStore) handles validation. The test structure and assertions are appropriate.
1132-1214: LGTM! Comprehensive test for single-use token behavior.This test thoroughly covers GitHub's single-use refresh token behavior, including successful initial use, failed reuse with appropriate error, and correct state preservation in the token store.
1276-1331: LGTM! Clear test for refresh token preservation.This test properly verifies the behavior when an OAuth provider returns a new access token but doesn't rotate the refresh token, ensuring backward compatibility with providers that don't implement token rotation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/transport/oauth_test.go (3)
1064-1130: Refresh token rotation behavior is well exercised; consider also asserting persisted stateThe rotation test correctly checks:
- That each
/tokencall receives the expected refresh token.- That returned refresh tokens change over successive refreshes.
- That access tokens differ across rotations.
As an optional enhancement, you could also fetch the token from
tokenStoreafter each successful refresh and assert it matches the returned token, to verify the store is kept in sync with the rotated credentials.
1216-1272: Auth-code exchange 200 + error path is correctly exercised; consider state-reset assertionThis test cleanly verifies that a 200-with-error response during authorization-code exchange:
- Surfaces an OAuth error containing
invalid_grant.- Does not result in any token being persisted.
Optionally, you could also assert that
expectedStateis cleared after the failed exchange to guarantee state isn’t accidentally reused across attempts, even when the provider responds with an OAuth error.
1274-1329: Keep-old-refresh-token behavior is captured clearlyThe “keeps old refresh token” test nicely documents the intended behavior when providers omit
refresh_tokenin a successful refresh response: the old refresh token passed intoRefreshTokenis preserved. As an optional extra guard, you could confirm the same value is stored by callingtokenStore.GetToken(ctx)and checking itsRefreshToken.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/oauth_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/oauth_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/oauth_test.go
🧠 Learnings (1)
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
client/transport/oauth_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
client/transport/oauth_test.go (5)
9-15: Imports and assertion helpers are appropriateThe added imports (
strconvplus assertion helpers) are all used in the new tests, and the stdlib/third‑party ordering matches the usual Go style. Using assertion helpers for the new cases improves test readability and failure diagnostics.
947-1002: Strong regression test for GitHub-style 200 + error on refreshThis test accurately simulates the GitHub behavior (HTTP 200 with an OAuth error body), asserts that an error is surfaced, and verifies that no token is written to the store, which directly guards against the original bug (saving an empty/invalid token). The coverage here looks solid and focused on the PR objective.
1004-1062: Good coverage of “empty access_token” parsing semanticsThis test clearly documents and verifies the intended contract: mcp-go parses what it receives (including an empty
access_token) and leaves validation to the integration layer. It helps ensure the new error-detection logic doesn’t misclassify such responses as OAuth errors. No issues spotted.
1132-1214: Single-use refresh token semantics and error-on-reuse are well coveredThe server stub that tracks
usedTokensnicely models single-use behavior. The test validates:
- First use succeeds.
- Reuse of the same refresh token yields an OAuth-style error surfaced to the caller.
- The previously successful token remains stored and can be used to obtain another token.
This gives good confidence that error-in-200 responses on reuse don’t clobber a valid stored token.
1331-1383: Spec-compliant HTTP 400 + OAuth error handling is validatedThis test complements the 200+error cases by asserting that a proper HTTP 400 with an OAuth error body:
- Produces an error containing
invalid_grant.- Leaves the token store empty.
That combination gives good regression coverage across both spec-compliant and non-compliant provider behaviors.
1 participant
Description
Certain providers (cough GitHub cough) return HTTP 200 responses
with an error in the body for their OAuth token exchange and
token refresh endpoints. Previously, we only checked for non-200 status
codes, which could lead to us missing the errors, attempting to
unmarshal the body as a token, failing to detect the error, and even
saving the empty token.
This commit attempts to unmarshal the body as an OAuth error response
even when the status code is 200. If the body is not an OAuth error
response, we fall back to the previous behavior.
I think this is really an error in GitHub's implementation but unfortunately GitHub is quite a popular provider so we'll need to deal with it 😕
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Here's a curl command I used to test this:
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.