diff --git a/pkg/auth/hold_remote.go b/pkg/auth/hold_remote.go index 65cc4ab..187c650 100644 --- a/pkg/auth/hold_remote.go +++ b/pkg/auth/hold_remote.go @@ -138,8 +138,20 @@ func (a *RemoteHoldAuthorizer) GetCaptainRecord(ctx context.Context, holdDID str return nil, fmt.Errorf("failed to get captain record for %s: %w", holdDID, err) } - // Update cache - if a.db != nil { + // Cache only records published by a DID that actually runs a hold. + // + // Any account can put an io.atcr.hold.captain record in its own repo, and + // ResolveHoldURL falls back to the DID's PDS endpoint when there is no + // atcr_hold service — so a user who points their sailor profile at their own + // DID reaches this path with a record they wrote themselves. Cached, it + // becomes a row in hold_captain_records, and GetAvailableHolds offers every + // row with allow_all_crew=1 to every user's hold picker. + // + // 69307c0 closed this for the two Jetstream writers and left this third one + // open. Gate the CACHE only, not the authorization decision: an unresolvable + // DID during a PLC blip would otherwise fail a legitimate push, and the + // freshly-fetched record is no less trustworthy than it was before. + if a.db != nil && a.cacheableHold(ctx, holdDID) { if err := a.setCachedCaptainRecord(holdDID, record); err != nil { // Log error but don't fail - caching is best-effort slog.Warn("Failed to cache captain record", "error", err, "holdDID", holdDID) @@ -149,6 +161,11 @@ func (a *RemoteHoldAuthorizer) GetCaptainRecord(ctx context.Context, holdDID str return record, nil } +// hasHoldService reports whether a DID advertises an atcr_hold service. It is a +// package var so tests can exercise the negative case: the real implementation +// trusts any did:web in test mode, which is exactly the shape most tests use. +var hasHoldService = atproto.HasHoldService + // captainRecordWithMeta includes UpdatedAt for cache management type captainRecordWithMeta struct { *atproto.CaptainRecord @@ -202,6 +219,23 @@ func (a *RemoteHoldAuthorizer) getCachedCaptainRecord(holdDID string) (*captainR }, nil } +// cacheableHold reports whether a captain record from holdDID may be cached. +// Errors are treated as "do not cache" rather than "deny": the record is still +// returned to the caller, it just does not become a durable row. +func (a *RemoteHoldAuthorizer) cacheableHold(ctx context.Context, holdDID string) bool { + isHold, err := hasHoldService(ctx, holdDID) + if err != nil { + slog.Warn("Not caching captain record; hold DID unresolvable", + "holdDID", holdDID, "error", err) + return false + } + if !isHold { + slog.Info("Not caching captain record from non-hold DID", "holdDID", holdDID) + return false + } + return true +} + // setCachedCaptainRecord stores a captain record in database cache func (a *RemoteHoldAuthorizer) setCachedCaptainRecord(holdDID string, record *atproto.CaptainRecord) error { query := ` diff --git a/pkg/auth/hold_remote_captain_verify_test.go b/pkg/auth/hold_remote_captain_verify_test.go new file mode 100644 index 0000000..d304212 --- /dev/null +++ b/pkg/auth/hold_remote_captain_verify_test.go @@ -0,0 +1,136 @@ +package auth + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "atcr.io/pkg/atproto" +) + +// 69307c0 verified captain records against the publishing DID's atcr_hold +// service before caching them, but only on the two Jetstream writers. This +// covers the third: RemoteHoldAuthorizer.GetCaptainRecord, reached during blob +// authorization with a hold DID the user chose via their sailor profile. +// +// The row matters because GetAvailableHolds offers every hold_captain_records +// row with allow_all_crew=1 to every user's hold picker, so an unverified row +// puts an arbitrary DID in front of everyone as a storage option. + +// captainServer serves a captain record for any repo, with allowAllCrew set — +// the value that makes a row visible to every user rather than just its author. +func captainServer(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{ + "uri": "at://x/io.atcr.hold.captain/self", + "cid": "bafytest", + "value": map[string]any{ + "$type": atproto.CaptainCollection, + "owner": "did:plc:attacker", + "public": true, + "allowAllCrew": true, + }, + }) + })) + t.Cleanup(srv.Close) + return srv +} + +func didFromServer(url string) string { + return "did:web:" + strings.ReplaceAll(strings.TrimPrefix(url, "http://"), ":", "%3A") +} + +func captainRows(t *testing.T, a *RemoteHoldAuthorizer, holdDID string) int { + t.Helper() + var n int + if err := a.db.QueryRow( + `SELECT COUNT(*) FROM hold_captain_records WHERE hold_did = ?`, holdDID, + ).Scan(&n); err != nil { + t.Fatalf("count captain rows: %v", err) + } + return n +} + +func newVerifyAuthorizer(t *testing.T) *RemoteHoldAuthorizer { + t.Helper() + atproto.SetTestMode(true) + t.Cleanup(func() { atproto.SetTestMode(false) }) + return &RemoteHoldAuthorizer{ + db: setupTestDB(t), + httpClient: &http.Client{Timeout: 5 * time.Second}, + cacheTTL: time.Hour, + testMode: true, + } +} + +// TestGetCaptainRecord_NonHoldDIDIsNotCached is the one that fails without the +// gate. A DID with no atcr_hold service still serves the record over its PDS +// endpoint, so the fetch succeeds and the caller gets an answer — but nothing +// durable may be written, or the picker inherits it. +func TestGetCaptainRecord_NonHoldDIDIsNotCached(t *testing.T) { + a := newVerifyAuthorizer(t) + srv := captainServer(t) + holdDID := didFromServer(srv.URL) + + prev := hasHoldService + hasHoldService = func(context.Context, string) (bool, error) { return false, nil } + t.Cleanup(func() { hasHoldService = prev }) + + rec, err := a.GetCaptainRecord(context.Background(), holdDID) + if err != nil { + t.Fatalf("GetCaptainRecord: %v", err) + } + if rec == nil || !rec.AllowAllCrew { + t.Fatalf("expected the fetch itself to still succeed, got %+v", rec) + } + if n := captainRows(t, a, holdDID); n != 0 { + t.Errorf("hold_captain_records holds %d row(s) for a DID that runs no hold; "+ + "GetAvailableHolds would offer it to every user's hold picker", n) + } +} + +// TestGetCaptainRecord_RealHoldIsStillCached is the inverse, so the gate cannot +// degrade into "never cache anything" — which would look like a pass above +// while quietly costing an XRPC round trip on every authorization. +func TestGetCaptainRecord_RealHoldIsStillCached(t *testing.T) { + a := newVerifyAuthorizer(t) + srv := captainServer(t) + holdDID := didFromServer(srv.URL) + + prev := hasHoldService + hasHoldService = func(context.Context, string) (bool, error) { return true, nil } + t.Cleanup(func() { hasHoldService = prev }) + + if _, err := a.GetCaptainRecord(context.Background(), holdDID); err != nil { + t.Fatalf("GetCaptainRecord: %v", err) + } + if n := captainRows(t, a, holdDID); n != 1 { + t.Errorf("hold_captain_records holds %d rows for a verified hold, want 1", n) + } +} + +// TestGetCaptainRecord_UnresolvableDIDIsNotCached: a resolution failure is not +// evidence that the DID runs a hold, so it must not seed a durable row either. +func TestGetCaptainRecord_UnresolvableDIDIsNotCached(t *testing.T) { + a := newVerifyAuthorizer(t) + srv := captainServer(t) + holdDID := didFromServer(srv.URL) + + prev := hasHoldService + hasHoldService = func(context.Context, string) (bool, error) { + return false, context.DeadlineExceeded + } + t.Cleanup(func() { hasHoldService = prev }) + + if _, err := a.GetCaptainRecord(context.Background(), holdDID); err != nil { + t.Fatalf("GetCaptainRecord should still answer from the live fetch: %v", err) + } + if n := captainRows(t, a, holdDID); n != 0 { + t.Errorf("cached %d row(s) for an unresolvable DID", n) + } +}