Skip to content

Use http.Error() instead of doing manual error pumping #139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Use http.Error() instead of doing manual error pumping #139

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions pkg/oauth/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package oauth

import (
"fmt"
"log"
"net/http"
"slices"
"strings"
Expand Down Expand Up @@ -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)
Copy link

@coderabbitai coderabbitai bot Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

Copy link
Member

@creydr creydr Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link

@coderabbitai coderabbitai bot Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Collaborator Author

@matzew matzew Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@creydr the .Error does a .Del on content-type

Is application/json really required?

Copy link

@coderabbitai coderabbitai bot Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

@Cali0707 Cali0707 Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

}

func ProtectedResourceMetadataHandler(config *mcpfile.MCPServer) http.HandlerFunc {
Expand Down