fix(webserver): bound OIDC provider calls with an explicit timeout

Discovery ran under the service mutex with no deadline. A provider that
accepted the connection but never answered would therefore hold a login open
indefinitely, and — because discovery is serialized — park every concurrent
login behind the lock.

Bound discovery, token exchange, and ID token verification. The timeout lives
on the service and is carried by its HTTP client rather than by a context
alone: go-oidc refreshes the key set on its own background context, where a
deadline of ours would never apply, so only a client timeout covers that path.
Verified against go-oidc v3.19.0, whose Verifier() documents the background
context and propagates the provider's client to the key set.

Add a regression test that parks a provider mid-request and asserts the call
fails rather than hangs. It passes a context with no deadline on purpose: a
test context with one would pass against the unbounded implementation too, and
prove nothing. Confirmed the test fails ("discovery is not bounded") when the
bound is removed.

Also document four functions flagged by review coverage.

Author: dangerouslaser

internal/webserver/cli_config.go

@@ -106,6 +106,7 @@ func SaveCLIConfig(dbPath string, cfg CLIConfig) error {
 	return nil
 }
 
+// cliConfigPath returns the web config path colocated with dbPath.
 func cliConfigPath(dbPath string) string {
 	return filepath.Join(filepath.Dir(strings.TrimSpace(dbPath)), cliConfigFileName)
 }

internal/webserver/oidc.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/http"
 	"strings"
 	"sync"
 	"time"
@@ -26,6 +27,11 @@ const (
 	// oidcFlowLimit caps in-flight authorization requests so unfinished flows
 	// cannot grow memory without bound.
 	oidcFlowLimit = 64
+	// oidcHTTPTimeout bounds every call to the provider: discovery, token
+	// exchange, and background JWKS refreshes. Without it a hung provider would
+	// hold a login request open indefinitely, and — because discovery runs under
+	// the service mutex — stall every other login behind it.
+	oidcHTTPTimeout = 10 * time.Second
 )
 
 // errOIDCDisabled is returned when an OIDC route is reached while OIDC is off.
@@ -49,6 +55,12 @@ type oidcService struct {
 	cfg  OIDCConfig
 	logf func(string, ...any)
 
+	// httpTimeout bounds each individual call to the provider. The HTTP client
+	// carries the same bound, because go-oidc refreshes the key set on its own
+	// background context, where a deadline of ours would never apply.
+	httpTimeout time.Duration
+	httpClient  *http.Client
+
 	mu       sync.Mutex
 	provider *oidc.Provider
 	verifier *oidc.IDTokenVerifier
@@ -66,9 +78,11 @@ func newOIDCService(cfg OIDCConfig, logf func(string, ...any)) *oidcService {
 		return nil
 	}
 	return &oidcService{
-		cfg:   cfg,
-		logf:  logf,
-		flows: make(map[string]oidcFlow),
+		cfg:         cfg,
+		logf:        logf,
+		httpTimeout: oidcHTTPTimeout,
+		httpClient:  &http.Client{Timeout: oidcHTTPTimeout},
+		flows:       make(map[string]oidcFlow),
 	}
 }
 
@@ -82,8 +96,19 @@ func (s *oidcService) DisableBuiltInLogin() bool {
 	return s.Enabled() && s.cfg.DisableBuiltInLogin
 }
 
+// boundedContext attaches the bounded HTTP client and a deadline, so a provider
+// that stalls fails the request instead of holding it open.
+func (s *oidcService) boundedContext(ctx context.Context) (context.Context, context.CancelFunc) {
+	return context.WithTimeout(oidc.ClientContext(ctx, s.httpClient), s.httpTimeout)
+}
+
 // ensureProvider performs discovery once and caches the result. It is safe to
 // call on every request: after the first success it is a mutex and a nil check.
+//
+// Discovery runs under the mutex so a burst of logins against a cold service
+// performs one discovery rather than one per request. That makes bounding it
+// essential: the deadline below is what stops a hung provider from parking
+// every concurrent login behind this lock.
 func (s *oidcService) ensureProvider(ctx context.Context) error {
 	if !s.Enabled() {
 		return errOIDCDisabled
@@ -95,7 +120,10 @@ func (s *oidcService) ensureProvider(ctx context.Context) error {
 		return nil
 	}
 
-	provider, err := oidc.NewProvider(ctx, s.cfg.Issuer)
+	discoverCtx, cancel := s.boundedContext(ctx)
+	defer cancel()
+
+	provider, err := oidc.NewProvider(discoverCtx, s.cfg.Issuer)
 	if err != nil {
 		return fmt.Errorf("oidc: discover issuer: %s", redaction.RedactValue(err.Error(), nil))
 	}
@@ -140,6 +168,8 @@ func oidcScopeList(scopes string) []string {
 	return list
 }
 
+// supportsPKCE reports whether the provider advertises S256. Only S256 counts:
+// the "plain" method offers no protection worth the name.
 func supportsPKCE(methods []string) bool {
 	for _, method := range methods {
 		if strings.EqualFold(strings.TrimSpace(method), "S256") {
@@ -206,7 +236,12 @@ func (s *oidcService) Exchange(ctx context.Context, state string, code string) (
 		opts = append(opts, oauth2.VerifierOption(flow.CodeVerifier))
 	}
 
-	token, err := oauthCfg.Exchange(ctx, code, opts...)
+	// Bounded like discovery: a provider that stalls mid-exchange must fail the
+	// login, not hold the callback open.
+	exchangeCtx, cancel := s.boundedContext(ctx)
+	defer cancel()
+
+	token, err := oauthCfg.Exchange(exchangeCtx, code, opts...)
 	if err != nil {
 		return nil, fmt.Errorf("oidc: exchange authorization code: %s", redaction.RedactValue(err.Error(), nil))
 	}
@@ -216,7 +251,12 @@ func (s *oidcService) Exchange(ctx context.Context, state string, code string) (
 		return nil, errors.New("oidc: provider response did not include an id_token")
 	}
 
-	idToken, err := verifier.Verify(ctx, rawIDToken)
+	// Verification can trigger a JWKS fetch when the provider has rotated keys,
+	// so it gets its own deadline rather than the exchange's remaining budget.
+	verifyCtx, cancelVerify := s.boundedContext(ctx)
+	defer cancelVerify()
+
+	idToken, err := verifier.Verify(verifyCtx, rawIDToken)
 	if err != nil {
 		return nil, fmt.Errorf("oidc: verify id token: %s", redaction.RedactValue(err.Error(), nil))
 	}
@@ -251,6 +291,9 @@ func (c *oidcClaims) Username() string {
 	return ""
 }
 
+// storeFlow records a pending authorization request, evicting expired entries
+// and — once at the cap — the entry closest to expiry, so abandoned logins
+// cannot grow the map without bound.
 func (s *oidcService) storeFlow(state string, flow oidcFlow) {
 	s.flowMu.Lock()
 	defer s.flowMu.Unlock()

internal/webserver/oidc_test.go

@@ -4,6 +4,7 @@
 package webserver
 
 import (
+	"context"
 	"crypto/rand"
 	"crypto/rsa"
 	"encoding/json"
@@ -501,6 +502,71 @@ func TestOIDCRoutesAreAbsentWhenDisabled(t *testing.T) {
 	}
 }
 
+// TestOIDCDiscoveryIsBounded covers a provider that accepts the connection and
+// then never answers. Discovery runs under the service mutex, so an unbounded
+// call here would not just hang this login — it would park every concurrent one
+// behind the lock. The request must fail instead of hanging.
+func TestOIDCDiscoveryIsBounded(t *testing.T) {
+	blocked := make(chan struct{})
+
+	// Hangs until the test ends: it accepts, then never writes a response.
+	hung := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
+		select {
+		case <-blocked:
+		case <-r.Context().Done():
+		}
+	}))
+	// Release the handler BEFORE closing the server. httptest.Server.Close waits
+	// for in-flight requests, so closing first would deadlock against the very
+	// handler this test is keeping parked.
+	t.Cleanup(func() {
+		close(blocked)
+		hung.Close()
+	})
+
+	srv := newAuthTestServer(t, filepath.Join(t.TempDir(), "db.sqlite"))
+	srv.oidc = newOIDCService(OIDCConfig{
+		Enabled:      true,
+		Issuer:       hung.URL,
+		ClientID:     "test-client-id",
+		ClientSecret: "test-client-secret",
+		RedirectURL:  "https://upbrr.example.test/api/auth/oidc/callback",
+		Scopes:       DefaultOIDCScopes,
+	}, func(string, ...any) {})
+
+	// Shorten the service's own bound so the test does not sit for the full
+	// production timeout.
+	srv.oidc.httpTimeout = 500 * time.Millisecond
+	srv.oidc.httpClient = &http.Client{Timeout: 500 * time.Millisecond}
+
+	// Deliberately a context with NO deadline. If the service did not impose its
+	// own bound, nothing here would ever cancel the call — which is exactly the
+	// failure this test exists to catch. A test context with a deadline would
+	// pass even against the unbounded implementation, proving nothing.
+	done := make(chan error, 1)
+	go func() {
+		_, _, err := srv.oidc.AuthCodeURL(context.Background())
+		done <- err
+	}()
+
+	select {
+	case err := <-done:
+		if err == nil {
+			t.Fatal("AuthCodeURL succeeded against a provider that never responds")
+		}
+	case <-time.After(10 * time.Second):
+		t.Fatal("AuthCodeURL hung: discovery is not bounded")
+	}
+
+	// The failure must not leave a half-built provider cached.
+	srv.oidc.mu.Lock()
+	cached := srv.oidc.provider != nil
+	srv.oidc.mu.Unlock()
+	if cached {
+		t.Fatal("a failed discovery cached a provider")
+	}
+}
+
 func TestOIDCScopeListAlwaysRequestsOpenID(t *testing.T) {
 	tests := []struct {
 		name  string

internal/webserver/routes_oidc.go

@@ -218,6 +218,8 @@ func (s *Server) writeOIDCStateCookie(w http.ResponseWriter, r *http.Request, st
 	})
 }
 
+// clearOIDCStateCookie removes the state cookie once a flow has completed or
+// failed, so a stale state cannot linger in the browser.
 func (s *Server) clearOIDCStateCookie(w http.ResponseWriter, r *http.Request) {
 	//nolint:gosec // State clear cookie sets HttpOnly, SameSite, and Secure for HTTPS requests.
 	http.SetCookie(w, &http.Cookie{