Fix unauthenticated probes reusing authenticated transport state - #2
Closed
sanjayy0612 wants to merge 6 commits into
Closed
sanjayy0612 wants to merge 6 commits into
sanjayy0612 wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes false-positive “unauthenticated exposure” findings caused by reusing authenticated transport state (especially for persistent transports like WebSocket/SSE) by introducing transport-aware creation of fresh anonymous sessions and updating the relevant probes to use them.
Changes:
- Add
AnonymousSession()support to MCP session implementations (streamable HTTP, SSE legacy, WebSocket) to create fresh unauthenticated connections without inherited session state. - Update unauthenticated exposure probes (tools list, resources/prompts exposure, OAuth bearer-challenge detection) to use separate anonymous sessions instead of
WithNoAuth(). - Add a WebSocket regression test ensuring unauthenticated probing does not reuse an authenticated WebSocket connection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/probe/mcp/session.go | Add timeout retention and an anonymous streamable-HTTP session constructor. |
| internal/probe/mcp/session_ws.go | Add timeout retention and anonymous WebSocket session constructor. |
| internal/probe/mcp/session_sse.go | Add timeout retention and anonymous legacy-SSE session constructor. |
| internal/probe/mcp/checks.go | Introduce anonymous-session helper and update unauthenticated probes to use fresh sessions. |
| internal/probe/mcp/checks_test.go | Add regression test for authenticated WebSocket not being reused by unauth probes. |
Suppressed comments (1)
internal/probe/mcp/checks.go:1112
- resourcesPromptsExposureProbe creates a fresh anonymous session, which may allocate a new persistent WebSocket connection. The session isn't closed after the probe completes, potentially leaking connections/goroutines. Defer Close() when the returned session implements io.Closer.
func (p *resourcesPromptsExposureProbe) Run(ctx context.Context, s probe.Session, r *report.Report) error {
unauthSess, err := anonymousSession(s)
if err != nil {
return nil
}
for _, method := range []string{"resources/list", "prompts/list"} {
raw, err := unauthSess.Do(ctx, method, map[string]any{})
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
3
to
15
| @@ -9,9 +10,64 @@ import ( | |||
| "time" | |||
|
|
|||
| "github.com/hackwither/reap/internal/probe" | |||
| "github.com/hackwither/reap/internal/probe/common" | |||
| "github.com/hackwither/reap/internal/report" | |||
| ) | |||
Comment on lines
+24
to
+27
| key := r.Header.Get("Sec-WebSocket-Key") | ||
| hj, _ := w.(http.Hijacker) | ||
| conn, buf, err := hj.Hijack() | ||
| if err != nil { return } |
Comment on lines
+76
to
82
| // AnonymousSession establishes a separate WebSocket handshake without auth. | ||
| // WithNoAuth cannot change the credentials of an already-upgraded socket. | ||
| func (s *WSSession) AnonymousSession() (probe.Session, error) { | ||
| return NewWSSession(s.targetURL, "", s.timeout) | ||
| } | ||
|
|
||
| func (s *WSSession) TargetURL() string { return s.targetURL } |
Comment on lines
749
to
+756
| func (p *unauthToolsListProbe) Run(ctx context.Context, s probe.Session, r *report.Report) error { | ||
| // Re-issue tools/list explicitly WITHOUT the auth header, regardless of | ||
| // whether the initial handshake used one. This answers the specific | ||
| // question: "can an anonymous caller enumerate tools?" | ||
| raw, err := s.Do(ctx, "tools/list", map[string]any{}, probe.WithNoAuth()) | ||
| // Use a fresh connection so no Authorization header, MCP session ID, or | ||
| // authenticated persistent transport state can affect this observation. | ||
| unauthSess, err := anonymousSession(s) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| raw, err := unauthSess.Do(ctx, "tools/list", map[string]any{}) |
…corresponding tests
Author
|
Addressed locally: the missing JSON import, safer WebSocket test-server error handling, and cleanup for persistent anonymous WebSocket/SSE sessions. go test ./... passes |
Owner
|
Thanks @sanjayy0612, root-cause analysis was spot on. `main` was refactored to a shared httpx.Client after you forked, so I reimplemented the fix on top of that, keeping the anonymous-session approach and your WS regression test. closing in favor of that. |
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
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.
Summary
Unauthenticated MCP exposure probes were reusing the session created with
--auth-header. This caused false positives for persistent transports,especially WebSocket.
Root cause
WithNoAuth()can omit the Authorization header from an individual HTTPrequest, but it cannot make an already-authenticated WebSocket connection
anonymous. WebSocket authentication happens during the handshake.
As a result,
mcp-unauth-tools-listcould receive tools over an authenticatedWebSocket and incorrectly report them as accessible without authentication.
The same session-state problem could affect legacy SSE sessions and inherited
MCP session IDs.
Changes
mcp-unauth-tools-listmcp-resources-prompts-exposureother probes.
connection.
Tests
Added a regression test using an authentication-required WebSocket MCP server.
The test verifies that:
tools/listsucceeds.Verification
git diff --checkpasses.The Go verification commands could not be executed in the development
environment because
goandgofmtwere not available onPATH.Requested commands:
gofmt -l .go vet ./...go test ./...go build ./...