From 054d25cd7d6bddc4e51b686a56d589babf6a40f7 Mon Sep 17 00:00:00 2001 From: Bandana Kaur Date: Sat, 5 Sep 2026 03:01:37 +0530 Subject: [PATCH] Run unauthenticated-exposure probes over a fresh anonymous session REAP's "unauthenticated exposure" probes re-used the session created with --auth-header and merely suppressed the per-request Authorization header via WithNoAuth(). That does not undo transport state established out-of-band: WebSocket auth is bound to the upgrade handshake, streamable HTTP retains a captured Mcp-Session-Id, and legacy SSE keeps an authenticated long-lived stream. On WebSocket especially, WithNoAuth() is ignored entirely, so an authenticated tool list could be reported as anonymous exposure (issue #1). - Add AnonymousSession() to Session, SSESession and WSSession: a fresh, credential-free connection reusing the shared httpx.Client. Add Close() to SSESession (cancels the stream goroutine) and WSSession (closes the socket) so probes can tear down the extra connection. - checks.go: route mcp-unauth-tools-list, mcp-resources-prompts-exposure, the OAuth Bearer-challenge check, and mcp-auth-posture through a separate anonymous session instead of WithNoAuth() on the authenticated one. - Finding-emitting probes run a full anonymous initialize first (anonymousInitializedSession): spec-compliant streamable-HTTP/SSE servers gate enumeration behind initialize + Mcp-Session-Id, so a fresh session that skipped it would under-report a genuinely open server (false negative). - authPostureProbe swaps to a fresh anonymous session while preserving its init-agnostic open/gated/unreached semantics; a refused anonymous connection now reads as gated rather than a false "open". Tests: authenticated-WebSocket false-positive guard, initialize-gated open streamable false-negative guard, and SSESession.Close. Co-Authored-By: Claude Opus 4.8 --- internal/probe/mcp/checks.go | 112 +++++++++++++++++-- internal/probe/mcp/checks_test.go | 143 +++++++++++++++++++++++++ internal/probe/mcp/session.go | 8 ++ internal/probe/mcp/session_sse.go | 41 ++++++- internal/probe/mcp/session_sse_test.go | 44 ++++++++ internal/probe/mcp/session_ws.go | 15 +++ 6 files changed, 354 insertions(+), 9 deletions(-) diff --git a/internal/probe/mcp/checks.go b/internal/probe/mcp/checks.go index 12c24cd..e826b3b 100644 --- a/internal/probe/mcp/checks.go +++ b/internal/probe/mcp/checks.go @@ -61,6 +61,57 @@ func reproBody(method string, params any) string { var httpOnlyTransports = []string{"http-streamable", "http-sse-legacy"} var anyTransport = []string{"*"} +// anonymousSessionProvider is implemented by session types that can hand back +// a separate, credential-free connection. WithNoAuth only omits a per-request +// Authorization header; it cannot undo authenticated transport state that is +// established out-of-band (a WebSocket upgrade authenticated at handshake, a +// captured Mcp-Session-Id, a long-lived authenticated SSE stream). Probes that +// report "unauthenticated exposure" must observe it over one of these fresh +// sessions, never over the authenticated session with the header suppressed. +type anonymousSessionProvider interface { + AnonymousSession() (probe.Session, error) +} + +func anonymousSession(s probe.Session) (probe.Session, error) { + provider, ok := s.(anonymousSessionProvider) + if !ok { + return nil, fmt.Errorf("session does not support a separate anonymous connection") + } + return provider.AnonymousSession() +} + +// closeSession releases persistent transport state when a probe created a +// separate anonymous session. Streamable HTTP needs no explicit cleanup; +// WebSocket and legacy-SSE sessions keep a connection or goroutine open. +func closeSession(s probe.Session) { + if closer, ok := s.(io.Closer); ok { + _ = closer.Close() + } +} + +// anonymousInitializedSession returns a fresh anonymous session that has +// completed the MCP initialize handshake, so an "unauthenticated exposure" +// finding is observed exactly as a real anonymous client would: connect, +// initialize, then enumerate. ok is false when an anonymous caller cannot get +// that far — the transport can't present an anonymous connection, or the +// anonymous handshake was rejected — in which case the surface is NOT reachable +// anonymously and the caller must emit no finding. Spec-compliant +// streamable-HTTP and legacy-SSE servers gate enumeration behind initialize +// (and an Mcp-Session-Id the fresh session captures during that handshake), +// so skipping it would under-report a genuinely open server. The caller owns +// the returned session and must closeSession it. +func anonymousInitializedSession(ctx context.Context, s probe.Session) (probe.Session, bool) { + unauthSess, err := anonymousSession(s) + if err != nil { + return nil, false + } + if _, _, err := InitializeSession(ctx, unauthSess); err != nil { + closeSession(unauthSess) + return nil, false + } + return unauthSess, true +} + // streamableHTTPOnly is for probes that depend on a mechanism specific to // the streamable-HTTP session implementation (e.g. the Mcp-Session-Id // response header it captures) that has no equivalent in legacy-SSE or @@ -150,8 +201,19 @@ func (p *oauthMetadataPostureProbe) Transports() []string { return httpOnlyTrans // signal that must not be reported as "published but incomplete." func (p *oauthMetadataPostureProbe) Run(ctx context.Context, s probe.Session, r *report.Report) error { // --- Bearer challenge: observed on the resource's own 401, not metadata. --- - unauthRaw, unauthErr := s.Do(ctx, "tools/list", map[string]any{}, probe.WithNoAuth()) - sawChallenge := unauthErr == nil && unauthRaw != nil && unauthRaw.StatusCode == http.StatusUnauthorized + // A fresh anonymous connection, not WithNoAuth on the authenticated + // session — the whole point is the challenge an unauthenticated caller + // gets, and reused transport state (a WebSocket authenticated at its + // upgrade) would mask it. Deliberately no initialize handshake here: we + // want the server's raw 401 challenge on an unauthenticated tools/list. + // This probe only runs on HTTP transports, so a missing anonymous session + // just means "skip the challenge check," not a WS anonymous-dial failure. + var unauthRaw *probe.RawResult + if unauthSess, sessErr := anonymousSession(s); sessErr == nil { + defer closeSession(unauthSess) + unauthRaw, _ = unauthSess.Do(ctx, "tools/list", map[string]any{}) + } + sawChallenge := unauthRaw != nil && unauthRaw.StatusCode == http.StatusUnauthorized if sawChallenge { wwwAuth := unauthRaw.Headers.Get("WWW-Authenticate") if !strings.Contains(strings.ToLower(wwwAuth), "bearer") { @@ -561,10 +623,18 @@ func (p *unauthToolsListProbe) Protocol() string { return "mcp" } func (p *unauthToolsListProbe) Transports() []string { return anyTransport } 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()) + // Answer the specific question "can an anonymous caller enumerate tools?" + // over a fresh, unauthenticated connection — never by suppressing the + // header on the authenticated session. WithNoAuth cannot undo transport + // state established out-of-band (a WebSocket authenticated at its upgrade, + // a captured Mcp-Session-Id), which would let an authenticated tool list + // be reported as anonymous exposure. + unauthSess, ok := anonymousInitializedSession(ctx, s) + if !ok { + return nil // no anonymous connection possible / handshake rejected — nothing exposed + } + defer closeSession(unauthSess) + raw, err := unauthSess.Do(ctx, "tools/list", map[string]any{}) if err != nil { return nil // network failure is not a finding; leave silent, CLI logs errors separately } @@ -856,8 +926,17 @@ func (p *resourcesPromptsExposureProbe) Protocol() string { return "mcp" } func (p *resourcesPromptsExposureProbe) Transports() []string { return anyTransport } func (p *resourcesPromptsExposureProbe) Run(ctx context.Context, s probe.Session, r *report.Report) error { + // A fresh anonymous session (not WithNoAuth on the authenticated one) so + // no credential or inherited transport state can make an authenticated + // listing look anonymous. On transports that gate enumeration behind + // initialize, the shared helper runs that handshake first. + unauthSess, ok := anonymousInitializedSession(ctx, s) + if !ok { + return nil + } + defer closeSession(unauthSess) for _, method := range []string{"resources/list", "prompts/list"} { - raw, err := s.Do(ctx, method, map[string]any{}, probe.WithNoAuth()) + raw, err := unauthSess.Do(ctx, method, map[string]any{}) if err != nil || raw.StatusCode != 200 { continue } @@ -1280,7 +1359,24 @@ func (p *authPostureProbe) Protocol() string { return "mcp" } func (p *authPostureProbe) Transports() []string { return anyTransport } func (p *authPostureProbe) Run(ctx context.Context, s probe.Session, r *report.Report) error { - anon, err := listAll(ctx, s, "tools/list", "tools", probe.WithNoAuth()) + // Determine anonymous reach over a fresh, unauthenticated connection. + // WithNoAuth on the authenticated session cannot undo transport state + // bound at connect time (a WebSocket authenticated during its upgrade), + // which would otherwise report a credentialed tool listing as "open." + unauthSess, sessErr := anonymousSession(s) + if sessErr != nil { + // No anonymous connection can be formed at all — e.g. a WebSocket + // whose credentials are bound to the handshake refused an anonymous + // upgrade. That is the answer: an anonymous caller cannot enumerate. + r.Target.AuthState = report.AuthStateGated + if authed, authErr := listAll(ctx, s, "tools/list", "tools"); authErr == nil && authed.OK() { + r.Target.AuthState = report.AuthStateAuthed + } + return nil + } + defer closeSession(unauthSess) + + anon, err := listAll(ctx, unauthSess, "tools/list", "tools") if err != nil { r.Target.AuthState = report.AuthStateUnreached return fmt.Errorf("anonymous tools/list failed: %w", err) diff --git a/internal/probe/mcp/checks_test.go b/internal/probe/mcp/checks_test.go index 21c2b80..cb279a6 100644 --- a/internal/probe/mcp/checks_test.go +++ b/internal/probe/mcp/checks_test.go @@ -1,9 +1,11 @@ package mcp import ( + "bufio" "context" "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "strings" @@ -12,6 +14,7 @@ import ( "github.com/hackwither/reap/internal/httpx" "github.com/hackwither/reap/internal/probe" + "github.com/hackwither/reap/internal/probe/common" "github.com/hackwither/reap/internal/report" "github.com/hackwither/reap/internal/version" ) @@ -550,3 +553,143 @@ func TestOAuthMetadataPostureProbe_BearerCheckedOnProtectedResource(t *testing.T t.Fatalf("expected exactly the bearer-challenge-missing finding, got %d: %+v", len(rep.Findings), rep.Findings) } } + +// --- unauthenticated-exposure transport-state regression ---------------- + +// authRequiredWSServer completes a WebSocket upgrade ONLY when the request +// carries the right Authorization header (auth is bound to the handshake, as +// on a real WS transport), then answers initialize + tools/list over the +// upgraded socket. +func authRequiredWSServer(t *testing.T) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer secret" { + w.WriteHeader(http.StatusUnauthorized) + return + } + key := r.Header.Get("Sec-WebSocket-Key") + hj, ok := w.(http.Hijacker) + if !ok { + http.Error(w, "hijack unsupported", http.StatusInternalServerError) + return + } + conn, buf, err := hj.Hijack() + if err != nil { + return + } + defer conn.Close() + buf.WriteString("HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-WebSocket-Accept: " + common.ExpectedWebSocketAccept(key) + "\r\n\r\n") + _ = buf.Flush() + reader := bufio.NewReader(buf) + for { + fin, opcode, payload, err := readWSFrame(reader) + if err != nil { + return + } + if !fin || opcode != wsOpText { + continue + } + var req struct { + ID int `json:"id"` + Method string `json:"method"` + } + if json.Unmarshal(payload, &req) != nil { + continue + } + result := map[string]any{} + switch req.Method { + case "initialize": + result = map[string]any{"protocolVersion": SupportedProtocolVersions[0], "serverInfo": map[string]any{"name": "auth-ws"}, "capabilities": map[string]any{}} + case "tools/list": + result = map[string]any{"tools": []map[string]any{{"name": "secret_tool"}}} + } + resp, _ := json.Marshal(map[string]any{"jsonrpc": "2.0", "id": req.ID, "result": result}) + if writeServerWSFrame(conn, wsOpText, resp) != nil { + return + } + } + })) +} + +// TestUnauthToolsListProbeDoesNotReuseAuthenticatedWebSocket is the +// false-positive guard for issue #1: on a WebSocket, auth is established at the +// upgrade, so WithNoAuth on the authenticated session cannot make a request +// anonymous. The probe must open a SEPARATE anonymous connection — which this +// server refuses — and therefore report nothing, rather than passing off the +// authenticated tool list as anonymous exposure. +func TestUnauthToolsListProbeDoesNotReuseAuthenticatedWebSocket(t *testing.T) { + srv := authRequiredWSServer(t) + defer srv.Close() + wsURL := "ws" + srv.URL[len("http"):] + + sess, err := NewWSSession(wsURL, "Bearer secret", testClient(t)) + if err != nil { + t.Fatalf("authenticated websocket handshake failed: %v", err) + } + defer sess.conn.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if init, _, err := InitializeSession(ctx, sess); err != nil || init == nil { + t.Fatalf("authenticated initialize failed: %v", err) + } + raw, err := sess.Do(ctx, "tools/list", map[string]any{}) + if err != nil || raw.StatusCode != http.StatusOK { + t.Fatalf("authenticated tools/list failed: status=%d err=%v", raw.StatusCode, err) + } + + rep := newReport(wsURL) + if err := (&unauthToolsListProbe{}).Run(ctx, sess, rep); err != nil { + t.Fatalf("probe failed: %v", err) + } + if len(rep.Findings) != 0 { + t.Fatalf("authenticated websocket response was reported as anonymous exposure: %+v", rep.Findings) + } +} + +// TestUnauthToolsListProbeStillDetectsAnonymousStreamable is the positive +// counterpart: a spec-compliant streamable-HTTP server gates tools/list behind +// an initialize round trip and an Mcp-Session-Id the caller must echo. It needs +// no auth, so the tool list IS anonymously reachable and the probe MUST emit a +// finding. Without the anonymous initialize the fresh session would send +// tools/list with no session id, be rejected, and a genuinely open server would +// be silently under-reported (a false negative). +func TestUnauthToolsListProbeStillDetectsAnonymousStreamable(t *testing.T) { + const sessionID = "anon-session-1" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req struct { + ID int `json:"id"` + Method string `json:"method"` + } + body, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(body, &req) + w.Header().Set("Content-Type", "application/json") + switch req.Method { + case "initialize": + w.Header().Set("Mcp-Session-Id", sessionID) + _ = json.NewEncoder(w).Encode(map[string]any{"jsonrpc": "2.0", "id": req.ID, "result": map[string]any{"protocolVersion": SupportedProtocolVersions[0], "serverInfo": map[string]any{"name": "anon-gateway", "version": "1.0"}, "capabilities": map[string]any{}}}) + case "tools/list": + if r.Header.Get("Mcp-Session-Id") != sessionID { + w.WriteHeader(http.StatusBadRequest) + _ = json.NewEncoder(w).Encode(map[string]any{"jsonrpc": "2.0", "id": req.ID, "error": map[string]any{"code": -32000, "message": "missing session id"}}) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"jsonrpc": "2.0", "id": req.ID, "result": map[string]any{"tools": []map[string]any{{"name": "read_file"}}}}) + default: + // notifications/initialized and anything else: accept silently. + w.WriteHeader(http.StatusOK) + } + })) + defer srv.Close() + + rep := newReport(srv.URL) + if err := (&unauthToolsListProbe{}).Run(context.Background(), testSession(t, srv.URL), rep); err != nil { + t.Fatalf("probe failed: %v", err) + } + if len(rep.Findings) != 1 { + t.Fatalf("expected 1 anonymous-exposure finding for an initialize-gated open server, got %d: %+v", len(rep.Findings), rep.Findings) + } + if rep.Findings[0].ID != "mcp-unauth-tools-list" { + t.Fatalf("expected mcp-unauth-tools-list, got %q", rep.Findings[0].ID) + } +} diff --git a/internal/probe/mcp/session.go b/internal/probe/mcp/session.go index ad2c765..6a328a5 100644 --- a/internal/probe/mcp/session.go +++ b/internal/probe/mcp/session.go @@ -95,6 +95,14 @@ func NewSession(url, authHeader string, client *httpx.Client) *Session { } } +// AnonymousSession returns a fresh streamable-HTTP session with no +// authentication and no inherited Mcp-Session-Id. It is intentionally +// separate from Do(WithNoAuth): an anonymous probe must not reuse the +// session id captured during an authenticated handshake. +func (s *Session) AnonymousSession() (probe.Session, error) { + return NewSession(s.url, "", s.client), nil +} + func (s *Session) TargetURL() string { return s.url } // SessionID is the Mcp-Session-Id observed during the handshake, if any. diff --git a/internal/probe/mcp/session_sse.go b/internal/probe/mcp/session_sse.go index e8705fe..67491e3 100644 --- a/internal/probe/mcp/session_sse.go +++ b/internal/probe/mcp/session_sse.go @@ -16,6 +16,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -32,6 +33,7 @@ import ( type SSESession struct { sseURL string authHeader string + client *httpx.Client httpClient *http.Client connOnce sync.Once @@ -40,6 +42,8 @@ type SSESession struct { mu sync.Mutex postURL string connErr error + closed bool + cancel context.CancelFunc reqID int pending map[int]chan *probe.RawResult } @@ -54,12 +58,38 @@ func NewSSESession(sseURL, authHeader string, client *httpx.Client) (*SSESession return &SSESession{ sseURL: sseURL, authHeader: authHeader, + client: client, httpClient: &streaming, ready: make(chan struct{}), pending: make(map[int]chan *probe.RawResult), }, nil } +// AnonymousSession opens a separate SSE stream and POST channel with no +// credentials. The authenticated long-lived stream cannot be made anonymous +// after its handshake, so a fresh connection is the only faithful way to +// observe what an anonymous caller sees. +func (s *SSESession) AnonymousSession() (probe.Session, error) { + return NewSSESession(s.sseURL, "", s.client) +} + +// Close stops the persistent SSE stream and releases its goroutine. It is +// safe to call more than once. +func (s *SSESession) Close() error { + s.mu.Lock() + if s.closed { + s.mu.Unlock() + return nil + } + s.closed = true + cancel := s.cancel + s.mu.Unlock() + if cancel != nil { + cancel() + } + return nil +} + func (s *SSESession) TargetURL() string { return s.sseURL } // connect opens the persistent GET SSE connection on first use and blocks @@ -68,7 +98,16 @@ func (s *SSESession) TargetURL() string { return s.sseURL } // first caller does any work, everyone else just waits on s.ready. func (s *SSESession) connect(ctx context.Context) error { s.connOnce.Do(func() { - go s.runStream(ctx) + s.mu.Lock() + if s.closed { + s.mu.Unlock() + s.failConnect(errors.New("SSE session is closed")) + return + } + streamCtx, cancel := context.WithCancel(ctx) + s.cancel = cancel + s.mu.Unlock() + go s.runStream(streamCtx) }) select { case <-s.ready: diff --git a/internal/probe/mcp/session_sse_test.go b/internal/probe/mcp/session_sse_test.go index d8d658d..66a6fc4 100644 --- a/internal/probe/mcp/session_sse_test.go +++ b/internal/probe/mcp/session_sse_test.go @@ -141,3 +141,47 @@ func mustSSESession(t *testing.T, url string) *SSESession { } return sess } + +// TestSSESession_CloseStopsStream verifies Close cancels the long-lived GET +// stream (releasing its goroutine and the server-side connection). Close backs +// AnonymousSession cleanup: an anonymous SSE probe opens a second stream and +// must be able to tear it down when the probe finishes. +func TestSSESession_CloseStopsStream(t *testing.T) { + streamClosed := make(chan struct{}) + mux := http.NewServeMux() + mux.HandleFunc("/sse", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + fmt.Fprint(w, "event: endpoint\ndata: /messages\n\n") + w.(http.Flusher).Flush() + <-r.Context().Done() + close(streamClosed) + }) + mux.HandleFunc("/messages", func(w http.ResponseWriter, r *http.Request) { + var body struct { + ID int `json:"id"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + _ = json.NewEncoder(w).Encode(map[string]any{"jsonrpc": "2.0", "id": body.ID, "result": map[string]any{}}) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + sess := mustSSESession(t, srv.URL+"/sse") + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if _, err := sess.Do(ctx, "initialize", map[string]any{}); err != nil { + t.Fatalf("Do failed: %v", err) + } + if err := sess.Close(); err != nil { + t.Fatalf("Close failed: %v", err) + } + select { + case <-streamClosed: + case <-time.After(time.Second): + t.Fatal("Close did not stop the SSE stream") + } + // Idempotent: a second Close must not panic or block. + if err := sess.Close(); err != nil { + t.Fatalf("second Close failed: %v", err) + } +} diff --git a/internal/probe/mcp/session_ws.go b/internal/probe/mcp/session_ws.go index 6995315..23cee83 100644 --- a/internal/probe/mcp/session_ws.go +++ b/internal/probe/mcp/session_ws.go @@ -41,6 +41,7 @@ const ( // frame's JSON-RPC id. type WSSession struct { targetURL string + client *httpx.Client conn net.Conn writeMu sync.Mutex @@ -67,6 +68,7 @@ func NewWSSession(targetURL, authHeader string, client *httpx.Client) (*WSSessio } s := &WSSession{ targetURL: targetURL, + client: client, conn: conn, pending: make(map[int]chan *probe.RawResult), } @@ -74,6 +76,19 @@ func NewWSSession(targetURL, authHeader string, client *httpx.Client) (*WSSessio return s, nil } +// AnonymousSession establishes a separate WebSocket handshake with no +// credentials. WithNoAuth cannot change the credentials of an +// already-upgraded socket — WS authentication happens during the handshake — +// so the only way to observe an anonymous caller is a fresh connection. +func (s *WSSession) AnonymousSession() (probe.Session, error) { + return NewWSSession(s.targetURL, "", s.client) +} + +// Close releases the upgraded connection and stops the background reader. +func (s *WSSession) Close() error { + return s.conn.Close() +} + func (s *WSSession) TargetURL() string { return s.targetURL } // Do sends one JSON-RPC request as a text frame and waits for a response