auth: cover the two batch-09 commits that shipped without tests

9d4ad84 (read-only app password -> 403) and e6959e6 (bounded HTTP clients on
the token path) both landed with no test at all. These are the ones a
regression would be silent in: a revert of either leaves every existing test
green.

Each test was mutation-verified against the defect it claims to catch, in a
throwaway worktree, and required to fail:

  * revert ResolveHoldDID to http.DefaultClient  -> SlowHoldIsCutOff fails
  * revert getServiceAuth to http.DefaultClient  -> SlowPDSIsCutOff fails
  * NewSessionValidator back to &http.Client{}   -> ClientsAreBounded fails
  * drop the InsufficientScope classification    -> IsClassified fails
  * drop the handler's errors.Is branch          -> Returns403 fails, and the
    body it returns is the exact retry-inviting 503 UNAVAILABLE the commit
    exists to remove

The slow-path tests wait on an outer deadline rather than on the call itself.
With an unbounded client these calls never return, so a test that simply
awaited the result would hang the suite instead of failing it, and a hung
suite reports nothing.

The client caps are asserted twice on purpose: once as a field value, which
guards the production 10s/15s numbers, and once functionally, which proves the
call site routes through the bounded client rather than merely declaring one.
Neither half catches the other's regression.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh

pkg/atproto/resolver_timeout_test.go

@@ -0,0 +1,72 @@
+package atproto
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+)
+
+// ResolveHoldDID used http.DefaultClient, which has no timeout, so an
+// unreachable hold held /auth/token open indefinitely — well past Docker's own
+// token-fetch deadline, with nothing to cut it off.
+//
+// The client is package-level, so this also fixes every other ResolveHoldDID
+// caller (GC, Jetstream backfill, the hold-health worker). That widened blast
+// radius is the reason the cap is asserted here rather than only at the token
+// path.
+func TestResolveHoldDID_UsesBoundedClient(t *testing.T) {
+	if holdDIDResolveClient.Timeout != 10*time.Second {
+		t.Errorf("holdDIDResolveClient.Timeout = %v, want 10s — an unbounded client here stalls /auth/token, GC and Jetstream alike",
+			holdDIDResolveClient.Timeout)
+	}
+}
+
+// The field assertion above proves the client is configured; this proves
+// ResolveHoldDID actually routes through it. Reverting the call site to
+// http.DefaultClient leaves the assertion above green and fails this one.
+func TestResolveHoldDID_SlowHoldIsCutOff(t *testing.T) {
+	// The hold never answers until the test releases it. httptest.Server.Close
+	// blocks on in-flight handlers, so the release has to happen before Close
+	// or the teardown pays the full stall it is simulating.
+	release := make(chan struct{})
+	hold := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		<-release
+		_, _ = w.Write([]byte("did:web:hold.example.com"))
+	}))
+	t.Cleanup(func() { close(release); hold.Close() })
+
+	restore := holdDIDResolveClient
+	holdDIDResolveClient = &http.Client{Timeout: 100 * time.Millisecond}
+	t.Cleanup(func() { holdDIDResolveClient = restore })
+
+	type result struct {
+		err     error
+		elapsed time.Duration
+	}
+	done := make(chan result, 1)
+	go func() {
+		start := time.Now()
+		_, err := ResolveHoldDID(context.Background(), hold.URL)
+		done <- result{err, time.Since(start)}
+	}()
+
+	// The deadline is what keeps an unbounded client from hanging the suite
+	// instead of failing it: with http.DefaultClient this call never returns on
+	// its own, and a test that hangs reports nothing useful.
+	select {
+	case res := <-done:
+		if res.err == nil {
+			t.Fatal("expected a timeout error from an unresponsive hold, got none")
+		}
+		// A whole second is ten times the configured cap, so this distinguishes
+		// "the bounded client was used" from "the request ran to completion"
+		// without being flaky under load.
+		if res.elapsed > time.Second {
+			t.Errorf("ResolveHoldDID took %v with a 100ms client cap — the call is not going through holdDIDResolveClient", res.elapsed)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("ResolveHoldDID never returned against a hold that never answers — the call is not going through holdDIDResolveClient")
+	}
+}

pkg/auth/servicetoken_scope_test.go

@@ -0,0 +1,92 @@
+package auth
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+)
+
+// A read-only app password authenticates fine through createSession but cannot
+// call com.atproto.server.getServiceAuth, which is privileged: the PDS answers
+// 403 InsufficientScope. Before this was classified it fell through to the
+// generic non-200 branch and surfaced as a retryable 503, so clients looped on
+// a request that can never succeed.
+//
+// The classification is deliberately narrow — status 403 AND the atproto error
+// name — because the sentinel drives a permanent 403 at /auth/token. Widening it
+// would turn a transient outage into a dead end for the user.
+func TestAppPasswordServiceToken_InsufficientScopeIsClassified(t *testing.T) {
+	tests := []struct {
+		name         string
+		status       int
+		body         string
+		wantSentinel bool
+	}{
+		{
+			name:         "403 InsufficientScope is the read-only app password",
+			status:       http.StatusForbidden,
+			body:         `{"error":"InsufficientScope","message":"Bad token scope"}`,
+			wantSentinel: true,
+		},
+		{
+			name:         "403 with an unrelated error name stays generic",
+			status:       http.StatusForbidden,
+			body:         `{"error":"AccountTakedown","message":"Account has been taken down"}`,
+			wantSentinel: false,
+		},
+		{
+			name:         "InsufficientScope on a non-403 status stays generic",
+			status:       http.StatusBadRequest,
+			body:         `{"error":"InsufficientScope"}`,
+			wantSentinel: false,
+		},
+		{
+			name:         "503 from the PDS stays generic and retryable",
+			status:       http.StatusServiceUnavailable,
+			body:         `{"error":"UpstreamFailure"}`,
+			wantSentinel: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			pds := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(tt.status)
+				_, _ = w.Write([]byte(tt.body))
+			}))
+			defer pds.Close()
+
+			did := "did:plc:" + strings.ToLower(strings.ReplaceAll(t.Name(), "/", ""))
+			holdDID := "did:web:hold.example.com"
+
+			GetGlobalTokenCache().Set(did, "app-password-access-token", time.Hour)
+			t.Cleanup(func() { GetGlobalTokenCache().Delete(did) })
+
+			_, err := GetOrFetchServiceTokenWithAppPassword(
+				context.Background(), did, holdDID, pds.URL,
+			)
+			if err == nil {
+				t.Fatal("expected an error from a non-200 PDS response")
+			}
+
+			gotSentinel := errors.Is(err, ErrAppPasswordInsufficientScope)
+			if gotSentinel != tt.wantSentinel {
+				t.Errorf("errors.Is(err, ErrAppPasswordInsufficientScope) = %v, want %v (err = %v)",
+					gotSentinel, tt.wantSentinel, err)
+			}
+
+			// The bearer token is valid — it is only scoped too narrowly — so it
+			// must survive. Evicting it would force a re-authentication that
+			// produces another read-only token, which is the loop this change
+			// exists to stop.
+			if _, stillCached := GetGlobalTokenCache().Get(did); tt.wantSentinel && !stillCached {
+				t.Error("the app-password token was evicted on an under-scoped grant; it is valid and re-authenticating cannot widen it")
+			}
+		})
+	}
+}

pkg/auth/timeout_test.go

@@ -0,0 +1,104 @@
+package auth
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+)
+
+// Both of these ran on http.DefaultClient, which has no timeout, so a slow or
+// unreachable PDS could hold /auth/token open indefinitely — past Docker's own
+// token-fetch deadline, with no way to shed the request.
+//
+// The OAuth refresh path is deliberately excluded: its POSTs go through
+// refreshDetachTransport and cancelling one mid-rotation strands a rotated
+// refresh token. Do not "fix" that one by symmetry.
+func TestTokenPathHTTPClientsAreBounded(t *testing.T) {
+	if appPasswordServiceAuthClient.Timeout != 10*time.Second {
+		t.Errorf("appPasswordServiceAuthClient.Timeout = %v, want 10s", appPasswordServiceAuthClient.Timeout)
+	}
+	if got := NewSessionValidator().httpClient.Timeout; got != 15*time.Second {
+		t.Errorf("SessionValidator.httpClient.Timeout = %v, want 15s — createSession on an unreachable PDS otherwise never returns", got)
+	}
+}
+
+// Proves the getServiceAuth call actually routes through the bounded client.
+// Reverting the call site to http.DefaultClient keeps the field assertion above
+// green and fails this.
+func TestAppPasswordServiceToken_SlowPDSIsCutOff(t *testing.T) {
+	release := make(chan struct{})
+	pds := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		<-release
+		_, _ = w.Write([]byte(`{"token":"never-arrives"}`))
+	}))
+	t.Cleanup(func() { close(release); pds.Close() })
+
+	restore := appPasswordServiceAuthClient
+	appPasswordServiceAuthClient = &http.Client{Timeout: 100 * time.Millisecond}
+	t.Cleanup(func() { appPasswordServiceAuthClient = restore })
+
+	did := "did:plc:slowpds"
+	GetGlobalTokenCache().Set(did, "app-password-access-token", time.Hour)
+	t.Cleanup(func() { GetGlobalTokenCache().Delete(did) })
+
+	done := make(chan callResult, 1)
+	go func() {
+		start := time.Now()
+		_, err := GetOrFetchServiceTokenWithAppPassword(
+			context.Background(), did, "did:web:hold.example.com", pds.URL,
+		)
+		done <- callResult{err, time.Since(start)}
+	}()
+	assertCutOff(t, done, "GetOrFetchServiceTokenWithAppPassword", "appPasswordServiceAuthClient")
+}
+
+// Same, for createSession. The client here is a struct field rather than a
+// package var, so the swap goes through the validator.
+func TestSessionValidator_SlowPDSIsCutOff(t *testing.T) {
+	release := make(chan struct{})
+	pds := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		<-release
+		_, _ = w.Write([]byte(`{"did":"did:plc:alice","handle":"alice.test","accessJwt":"x","refreshJwt":"y"}`))
+	}))
+	t.Cleanup(func() { close(release); pds.Close() })
+
+	v := NewSessionValidator()
+	v.httpClient = &http.Client{Timeout: 100 * time.Millisecond}
+
+	done := make(chan callResult, 1)
+	go func() {
+		start := time.Now()
+		_, err := v.createSession(context.Background(), pds.URL, "alice.test", "app-password")
+		done <- callResult{err, time.Since(start)}
+	}()
+	assertCutOff(t, done, "createSession", "v.httpClient")
+}
+
+type callResult struct {
+	err     error
+	elapsed time.Duration
+}
+
+// assertCutOff waits for a call that should have been cut off by a 100ms client
+// cap. The outer deadline is what makes an unbounded client fail the test rather
+// than hang it: on http.DefaultClient these calls never return on their own, and
+// a hung suite reports nothing.
+func assertCutOff(t *testing.T, done <-chan callResult, call, client string) {
+	t.Helper()
+	select {
+	case res := <-done:
+		if res.err == nil {
+			t.Fatalf("%s: expected a timeout error from an unresponsive PDS, got none", call)
+		}
+		// A whole second is ten times the configured cap, so this separates "the
+		// bounded client was used" from "the request ran to completion" without
+		// being flaky under load.
+		if res.elapsed > time.Second {
+			t.Errorf("%s took %v with a 100ms client cap — the call is not going through %s", call, res.elapsed, client)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatalf("%s never returned against a PDS that never answers — the call is not going through %s", call, client)
+	}
+}

pkg/auth/token/handler_scope_test.go

@@ -0,0 +1,76 @@
+package token
+
+import (
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+
+	"atcr.io/pkg/auth"
+)
+
+// A read-only app password is a permanent authorization failure, not an outage.
+// Returning the generic 503 tells the client to retry a request that can never
+// succeed and says nothing about what is wrong; the fix is only discoverable by
+// reading appview logs the user does not have.
+//
+// This is the sibling of TestHandler_ServiceAuthFetcher_FailureReturns503: same
+// path, same stub, and the only difference is which error the fetcher reports.
+func TestHandler_ServiceAuthFetcher_InsufficientScopeReturns403(t *testing.T) {
+	keyPath := getSharedTestKey(t)
+	issuer, err := NewIssuer(keyPath, "atcr.io", "registry", 5*time.Minute)
+	if err != nil {
+		t.Fatalf("NewIssuer() error = %v", err)
+	}
+
+	deviceStore, database := setupTestDeviceStore(t)
+	deviceSecret := createTestDevice(t, deviceStore, database, "did:plc:alice123", "alice.bsky.social")
+
+	handler := NewHandler(issuer, deviceStore)
+	// Wrapped, not bare: the production path returns
+	// fmt.Errorf("%w: %s", ErrAppPasswordInsufficientScope, body), so a handler
+	// that compared with == instead of errors.Is would still 503.
+	stub := &stubServiceAuthFetcher{
+		err: fmt.Errorf("%w: %s", auth.ErrAppPasswordInsufficientScope,
+			`{"error":"InsufficientScope","message":"Bad token scope"}`),
+	}
+	handler.SetServiceAuthFetcher(stub)
+
+	req := httptest.NewRequest(http.MethodGet, "/auth/token?service=registry&scope=repository:alice.bsky.social/myapp:pull,push", nil)
+	req.SetBasicAuth("alice", deviceSecret)
+	w := httptest.NewRecorder()
+
+	handler.ServeHTTP(w, req)
+
+	if w.Code != http.StatusForbidden {
+		t.Fatalf("expected 403 for an under-scoped app password, got %d. Body: %s", w.Code, w.Body.String())
+	}
+
+	// The status alone is not the deliverable: the body has to name the remedy,
+	// because "app password" and "read-only" are the only words that tell the
+	// user what to change.
+	var body struct {
+		Errors []struct {
+			Code    string `json:"code"`
+			Message string `json:"message"`
+		} `json:"errors"`
+	}
+	if err := json.NewDecoder(w.Body).Decode(&body); err != nil {
+		t.Fatalf("decode error response: %v", err)
+	}
+	if len(body.Errors) == 0 {
+		t.Fatal("expected an OCI error payload, got none")
+	}
+	if body.Errors[0].Code != "DENIED" {
+		t.Errorf("expected error code DENIED, got %q", body.Errors[0].Code)
+	}
+	msg := strings.ToLower(body.Errors[0].Message)
+	for _, want := range []string{"app password", "read-only"} {
+		if !strings.Contains(msg, want) {
+			t.Errorf("error message does not mention %q, so it does not tell the user what to fix: %q", want, body.Errors[0].Message)
+		}
+	}
+}