Files
zero/internal/oauth/loopback.go
Pujitha PaladuguandPujitha Paladugu 176f5a242b fix(oauth): force-close loopback callback connections past the shutdown deadline (#1051)
* fix(oauth): force-close loopback callback connections past the shutdown deadline

Motivation:
LoopbackListener.Close shuts the callback server down with a 1-second
context and discards the result. http.Server.Shutdown only closes idle
connections, so a client that sends a partial request header and never
finishes it counts as active; Shutdown blocks out the full deadline,
times out, and Close returns while that connection and its Serve
goroutine remain open indefinitely. This is a loopback-only (127.0.0.1),
short-lived, single-use server, so the impact is a resource leak past
the end of the flow, not a remote vulnerability or crash.

Approach:
- add a ReadHeaderTimeout to the server so a stalled request header is
  bounded on its own;
- follow a timed-out or otherwise failed Shutdown with server.Close()
  to forcibly tear down any connection still open past the deadline;
- join the Serve goroutine (via a done channel) before Close returns,
  so its completion is observable instead of left dangling.

internal/mcp/oauth.go and internal/provideroauth/openrouter.go build
their own loopback servers with the same pattern; they are left
untouched here to keep this change scoped to the file the issue's
reproduction demonstrates.

Validation:
- go build ./internal/oauth/... and go vet ./internal/oauth/...: clean.
- gofmt -l internal/oauth/: clean (make fmt-check clean repo-wide).
- go test ./internal/oauth/... -race -v -count=1: all pass, including
  three new tests (TestLoopbackCloseForceClosesSlowHeaderConnection,
  TestLoopbackCloseIsIdempotent,
  TestLoopbackCloseReturnsPromptlyAfterSuccessfulCallback).
- Confirmed TestLoopbackCloseForceClosesSlowHeaderConnection fails
  against the pre-fix loopback.go (with the new test kept) and passes
  with the fix applied.
- Full repo go build ./..., go vet ./..., and go test ./... pass.
- Could not run make lint-static (golangci-lint) locally: it needs a
  very large dependency download and the local environment ran out of
  disk space fetching it. That check is continue-on-error (advisory)
  in .github/workflows/ci.yml, not a required gate.

Report: https://github.com/Gitlawb/zero/issues/1028
Signed-off-by: Pujitha Paladugu <[email protected]>
Assisted-by: claude-sonnet-5 (via Claude Code)

* test(oauth): synchronize loopback regression tests instead of sleeping

TestLoopbackCloseForceClosesSlowHeaderConnection relied on a fixed
100ms sleep to let Serve accept a connection before sending a stalled
header; under load this could fail or silently skip the force-close
path it's meant to exercise. Use an http.Server.ConnState hook (via a
new unexported newLoopbackListener, set before Serve starts to avoid
a race) to wait for the real StateNew signal instead.

TestLoopbackCloseReturnsPromptlyAfterSuccessfulCallback measured
Close's elapsed time without joining its http.Get goroutine or
draining/closing the response body; join it first so the timing
starts from a connection that's actually back to idle.

Also register t.Cleanup(l.Close) in both tests so a Fatalf between
construction and the existing explicit Close doesn't leak the
listener's Serve goroutine.

Signed-off-by: Pujitha Paladugu <[email protected]>

---------

Signed-off-by: Pujitha Paladugu <[email protected]>
Co-authored-by: Pujitha Paladugu <[email protected]>
2026-09-15 08:37:58 +08:00

153 lines
5.3 KiB
Go

package oauth
import (
"context"
"errors"
"fmt"
"io"
"net"
"net/http"
"net/url"
"strings"
"time"
)
// LoopbackListener is a single-use loopback HTTP server that captures an OAuth
// redirect (?code=&state=) on 127.0.0.1 only: bind an OS-assigned loopback port,
// verify the CSRF state, hand back the code, then close.
type LoopbackListener struct {
listener net.Listener
state string
result chan callbackResult
server *http.Server
done chan struct{}
}
type callbackResult struct {
code string
err error
}
// NewLoopbackListener binds 127.0.0.1:0 (loopback only, OS-assigned port) and
// begins serving. state is the CSRF value the callback must echo back. Call
// RedirectURI to build the redirect_uri, then Wait for the code. Always Close.
func NewLoopbackListener(state string) (*LoopbackListener, error) {
return NewLoopbackListenerOnPort(state, 0)
}
// NewLoopbackListenerOnPort is like NewLoopbackListener but binds a specific
// port (0 = OS-assigned). Used by ChatGPT OAuth which requires a fixed
// redirect_uri of http://localhost:1455/auth/callback.
func NewLoopbackListenerOnPort(state string, port int) (*LoopbackListener, error) {
return newLoopbackListener(state, port, nil)
}
// newLoopbackListener is NewLoopbackListenerOnPort with an optional ConnState
// hook, set before the Serve goroutine starts so tests can synchronize on
// connection lifecycle events (e.g. a stalled request header being accepted)
// without racing the running server.
func newLoopbackListener(state string, port int, connState func(net.Conn, http.ConnState)) (*LoopbackListener, error) {
if strings.TrimSpace(state) == "" {
return nil, errors.New("oauth: loopback listener requires a non-empty CSRF state")
}
ln, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", port))
if err != nil {
return nil, fmt.Errorf("oauth: start loopback redirect listener: %w", err)
}
l := &LoopbackListener{
listener: ln,
state: state,
result: make(chan callbackResult, 1),
done: make(chan struct{}),
}
l.server = &http.Server{
Handler: http.HandlerFunc(l.handle),
// This is a single-use, loopback-only callback server: a client that
// never finishes sending its request header must not be able to hold
// the accepted connection (and Close, below) open indefinitely.
ReadHeaderTimeout: 5 * time.Second,
ConnState: connState,
}
go func() {
_ = l.server.Serve(ln)
close(l.done)
}()
return l, nil
}
// RedirectURI returns the http://127.0.0.1:<port>/callback redirect URI.
func (l *LoopbackListener) RedirectURI() string {
return fmt.Sprintf("http://%s/callback", l.listener.Addr().String())
}
// RedirectURIWithHost returns a redirect URI using the given host (e.g.
// "localhost") and path (e.g. "/auth/callback"). The listener still binds
// 127.0.0.1, but the OAuth client may require "localhost" in the redirect_uri
// (OpenAI's ChatGPT client registration does). The listener accepts both
// /callback and the given path.
func (l *LoopbackListener) RedirectURIWithHost(host, path string) string {
_, port, _ := net.SplitHostPort(l.listener.Addr().String())
return fmt.Sprintf("http://%s:%s%s", host, port, path)
}
func (l *LoopbackListener) handle(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/callback" && r.URL.Path != "/auth/callback" {
http.NotFound(w, r)
return
}
code, err := parseCallback(r.URL.Query(), l.state)
if err != nil {
w.WriteHeader(http.StatusBadRequest)
_, _ = io.WriteString(w, "Authorization failed. You may close this window.")
} else {
_, _ = io.WriteString(w, "Authorization complete. You may close this window.")
}
select {
case l.result <- callbackResult{code: code, err: err}:
default:
}
}
// Wait blocks until the callback arrives or ctx is done, returning the
// authorization code.
func (l *LoopbackListener) Wait(ctx context.Context) (string, error) {
select {
case res := <-l.result:
return res.code, res.err
case <-ctx.Done():
return "", fmt.Errorf("oauth: timed out waiting for authorization callback: %w", ctx.Err())
}
}
// Close shuts the listener down (bounded), idempotent. If a client leaves a
// connection open past the shutdown deadline (e.g. a stalled request header),
// Close forcibly closes it rather than leaking it past the end of the flow,
// and waits for the Serve goroutine to finish before returning.
func (l *LoopbackListener) Close() {
shutdownCtx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()
if err := l.server.Shutdown(shutdownCtx); err != nil {
_ = l.server.Close()
}
<-l.done
}
// parseCallback validates the redirect query and returns the authorization code,
// rejecting a mismatched state (CSRF) and surfacing provider errors.
func parseCallback(values url.Values, wantState string) (string, error) {
if got := values.Get("state"); got != wantState {
return "", ErrStateMismatch
}
if providerErr := strings.TrimSpace(values.Get("error")); providerErr != "" {
if desc := strings.TrimSpace(values.Get("error_description")); desc != "" {
return "", fmt.Errorf("oauth: authorization server returned error %q: %s", providerErr, desc)
}
return "", fmt.Errorf("oauth: authorization server returned error %q", providerErr)
}
code := strings.TrimSpace(values.Get("code"))
if code == "" {
return "", errors.New("oauth: callback missing authorization code")
}
return code, nil
}