From f4d0c8bf0550bad852f8dccf7af71bf5041d5a93 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Mon, 24 Aug 2026 21:16:55 -0500 Subject: [PATCH] auth: verify the hold before caching a captain record on the third path 6758996 started verifying captain records against the publishing DID's atcr_hold service before caching them, because any account can write an io.atcr.hold.captain record into its own repo and only a real hold advertises that service. It covered the two Jetstream writers -- the processor and the batch backfill -- and left RemoteHoldAuthorizer.GetCaptainRecord alone. That third path is reachable. A user's sailor profile decides which hold their content routes to, blob authorization calls CheckReadAccess/CheckWriteAccess with that DID, and both go through GetCaptainRecord. ResolveHoldURL falls back to the DID's #atproto_pds endpoint when there is no #atcr_hold service, so a user who points defaultHold at their own DID serves themselves a captain record of their own writing -- and it was cached. The row is the problem, not the fetch. GetAvailableHolds offers every hold_captain_records row with allow_all_crew=1 to every user's hold picker, so one unverified row puts an arbitrary DID in front of everyone as a place to store blobs. GetAccessibleHoldDIDs reads the same table to scope visibility. Gate the cache write only, not the authorization decision. Failing closed here would turn a PLC resolution blip into a rejected push, and the freshly fetched record is no less trustworthy than it was before this commit -- it just must not become durable. This matches the processor's "skip rather than fail" handling, where periodic backfill retries an unresolvable DID later. hasHoldService becomes a package var so the negative case is testable at all: the real implementation trusts any did:web in test mode, which is the shape every test here uses. The three new tests are mutation-verified -- removing the gate caches a row for both a non-hold DID and an unresolvable one, while the inverse test keeps the gate from degrading into "never cache", which would cost an XRPC round trip on every authorization while still looking like a pass. Note in passing: TestFetchCaptainRecordFromXRPC discards its result (`_ = record; _ = err`) and asserts nothing. Left alone here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB --- pkg/auth/hold_remote.go | 38 +++++- pkg/auth/hold_remote_captain_verify_test.go | 136 ++++++++++++++++++++ 2 files changed, 172 insertions(+), 2 deletions(-) create mode 100644 pkg/auth/hold_remote_captain_verify_test.go 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) + } +}