oauth: slow loopback clients survive bounded shutdown

[PierrunoYT]

Version / branch / commit

main at 1b5db1765672820caac1684b168c9898b5ba3593.

OS and environment

Linux x86-64, kernel 6.1, official Go 1.26.6.

Steps to reproduce

The shared internal/oauth loopback listener exposes the lifecycle directly enough for a deterministic package test:

func TestSlowHeaderSurvivesLoopbackClose(t *testing.T) {
    listener, err := NewLoopbackListener("audit-state")
    if err != nil { t.Fatal(err) }
    conn, err := net.Dial("tcp", listener.listener.Addr().String())
    if err != nil { t.Fatal(err) }
    defer conn.Close()

    fmt.Fprintf(conn,
        "GET /callback?code=audit-code&state=audit-state HTTP/1.1\r\nHost: %s\r\nX-Stall:",
        listener.listener.Addr().String())
    time.Sleep(100 * time.Millisecond) // let Serve accept and read the partial header

    started := time.Now()
    listener.Close()
    if elapsed := time.Since(started); elapsed < 900*time.Millisecond {
        t.Fatalf("Close returned after %s; expected its one-second deadline", elapsed)
    }

    conn.SetReadDeadline(time.Now().Add(100 * time.Millisecond))
    var one [1]byte
    _, err = conn.Read(one[:])
    if netErr, ok := err.(net.Error); !ok || !netErr.Timeout() {
        t.Fatalf("read after Close = %v; wanted timeout proving the connection remains open", err)
    }
}

Focused reproduction:

go test ./internal/oauth -run '^TestSlowHeaderSurvivesLoopbackClose$' -count=1 -v
--- PASS: TestSlowHeaderSurvivesLoopbackClose (1.20s)
PASS

The temporary test was removed after verification and was not committed.

The same construction appears in:

• internal/oauth/loopback.go around lines 53-54 and 102-106;
• internal/mcp/oauth.go around lines 497-520; and
• internal/provideroauth/openrouter.go around lines 82-106.

Each starts http.Server.Serve in a goroutine, configures no ReadHeaderTimeout/ReadTimeout/IdleTimeout, ignores the Serve result, calls Shutdown with a one-second context, ignores that result, and does not join Serve completion.

Expected behavior

Closing/canceling a short-lived loopback OAuth callback server should bound both caller latency and owned resources: accepted slow clients should be terminated within the close budget and the Serve goroutine should be joined or its terminal result made observable.

Actual behavior

A loopback client that sends a partial request header causes Close to consume the full one-second shutdown deadline. Close then returns without reporting the deadline and without closing the accepted connection; the client remains open and the server goroutine remains blocked until the client completes or closes the header.

This is loopback-only lifecycle/resource robustness, not a remotely exposed server or demonstrated credential vulnerability. The existing loopback bind, state validation, and PKCE controls are strong and should remain unchanged.

Suggested fix and regression tests

• configure conservative header/read/idle bounds appropriate for a single-use callback;
• retain and join the Serve completion result;
• make shutdown idempotent and report or otherwise observe a missed deadline;
• if graceful shutdown reaches its budget, force-close the owned server connections; and
• add slow-header, caller-cancel, repeated-close, successful-callback, and goroutine-completion tests for all three implementations.

Reported from codebase audit finding CON-02.

[Vasanthdev2004]

Confirmed on main. LoopbackListener.Close calls Shutdown with a one-second context and nothing else, and Shutdown only closes idle connections; a connection that has sent part of a request header is active, so when the deadline passes Close returns and the connection stays open with its goroutine. Low severity, the listener is loopback-only and short-lived, but it is a real leak past the end of the flow. Approving. Follow the timed-out Shutdown with server.Close(), and a ReadHeaderTimeout on the server bounds the slow-header case on its own. Keep the test from the report; it is the right shape.