Use http.Error() instead of doing manual error pumping #139
Conversation
Signed-off-by: Matthias Wessendorf <[email protected]>
WalkthroughReplaced manual 401 response handling in pkg/oauth/middleware.go with http.Error. Removed the log import and logging on write failure. No exported APIs changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/oauth/middleware.go(1 hunks)
⏰ 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: Unit Tests
| if err != nil { | ||
| log.Printf("failed to write response: %v", err) | ||
| } | ||
| http.Error(w, body, http.StatusUnauthorized) |
There was a problem hiding this comment.
Content-Type mismatch for JSON error responses.
http.Error() sets Content-Type: text/plain; charset=utf-8, but the body contains JSON. RFC 6750 (OAuth 2.0 Bearer Token Usage) specifies that error responses should use application/json when including error details in the response body.
Apply this diff to set the correct Content-Type:
w.Header().Set("WWW-Authenticate", fmt.Sprintf("Bearer resource_metadata=%q", fullWellKnownPath))
- http.Error(w, body, http.StatusUnauthorized)
+ w.Header().Set("Content-Type", "application/json")
+ w.WriteHeader(http.StatusUnauthorized)
+ w.Write([]byte(body))Alternatively, if you prefer to keep using http.Error() for simplicity and don't mind the text/plain Content-Type:
w.Header().Set("WWW-Authenticate", fmt.Sprintf("Bearer resource_metadata=%q", fullWellKnownPath))
+ w.Header().Set("Content-Type", "application/json")
http.Error(w, body, http.StatusUnauthorized)Note: Setting Content-Type before calling http.Error() will preserve your custom header.
📝 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.
| http.Error(w, body, http.StatusUnauthorized) | |
| // preserve the WWW-Authenticate header for OAuth 2.0 Bearer error responses | |
| w.Header().Set("WWW-Authenticate", fmt.Sprintf("Bearer resource_metadata=%q", fullWellKnownPath)) | |
| // explicitly set JSON content type and write the status + JSON body without the newline | |
| w.Header().Set("Content-Type", "application/json") | |
| w.WriteHeader(http.StatusUnauthorized) | |
| _, _ = w.Write([]byte(body)) |
🤖 Prompt for AI Agents
In pkg/oauth/middleware.go around line 76, the handler calls http.Error(w, body,
http.StatusUnauthorized) which sets Content-Type to text/plain while body
contains JSON; change it to send a proper application/json response by setting
the Content-Type header to "application/json" and writing the status and JSON
body explicitly (i.e., set w.Header().Set("Content-Type", "application/json"),
call w.WriteHeader(http.StatusUnauthorized), then write the JSON body to the
response writer) — alternatively, if you must keep http.Error(), set the
Content-Type header to "application/json" before calling http.Error() so the
custom header is preserved.
There was a problem hiding this comment.
IIRC the application/json type was required by the mcp clients, so I think we need to keep w.Header().Set("Content-Type", "application/json")
There was a problem hiding this comment.
@creydr, you're absolutely right! The application/json Content-Type is important for proper client parsing.
The good news is that the committable suggestion I provided already includes setting Content-Type to "application/json" explicitly:
w.Header().Set("WWW-Authenticate", fmt.Sprintf("Bearer resource_metadata=%q", fullWellKnownPath))
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusUnauthorized)
_, _ = w.Write([]byte(body))This approach ensures MCP clients receive the correct application/json Content-Type while properly handling the OAuth 2.0 Bearer error response format. The fix reverts to explicit header setting and response writing, which gives you full control over the Content-Type header (unlike http.Error() which would override it with text/plain).
There was a problem hiding this comment.
@creydr the .Error does a .Del on content-type
Is application/json really required?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@matzew I think the application/json required is normally true, because MCP is a JSON RPC protocol a lot of clients assume all the http bits are json iirc
|
/close
Sent from Gmail Mobile
…On Thu 2. Oct 2025 at 17:20, Calum Murray ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/oauth/middleware.go
<#139 (comment)>:
> @@ -74,12 +73,7 @@ func write401(w http.ResponseWriter, r *http.Request, body string) {
fullWellKnownPath := fmt.Sprintf("%s://%s%s", scheme, r.Host, ProtectedResourceMetadataEndpoint)
w.Header().Set("WWW-Authenticate", fmt.Sprintf("Bearer resource_metadata=%q", fullWellKnownPath))
- w.Header().Set("Content-Type", "application/json")
- w.WriteHeader(http.StatusUnauthorized)
- _, err := w.Write([]byte(body))
- if err != nil {
- log.Printf("failed to write response: %v", err)
- }
+ http.Error(w, body, http.StatusUnauthorized)
@matzew <https://github.com/matzew> I think the application/json required
is normally true, because MCP is a JSON RPC protocol a lot of clients
assume all the http bits are json iirc
—
Reply to this email directly, view it on GitHub
<#139 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABGPTQJLGDHD3ZEM7XBUQL3VU4J5AVCNFSM6AAAAACIA4ABACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTEOJVGA2DKNBUHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
3 participants
using standard library for error write
Summary by CodeRabbit