diff --git a/pkg/atproto/resolver_timeout_test.go b/pkg/atproto/resolver_timeout_test.go new file mode 100644 index 0000000..2408c58 --- /dev/null +++ b/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") + } +} diff --git a/pkg/auth/servicetoken_scope_test.go b/pkg/auth/servicetoken_scope_test.go new file mode 100644 index 0000000..2a48da4 --- /dev/null +++ b/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") + } + }) + } +} diff --git a/pkg/auth/timeout_test.go b/pkg/auth/timeout_test.go new file mode 100644 index 0000000..44fd46f --- /dev/null +++ b/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) + } +} diff --git a/pkg/auth/token/handler_scope_test.go b/pkg/auth/token/handler_scope_test.go new file mode 100644 index 0000000..7a4f907 --- /dev/null +++ b/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) + } + } +}