diff --git a/cmd/server/wiring.go b/cmd/server/wiring.go index dfb5de6..c671fbc 100644 --- a/cmd/server/wiring.go +++ b/cmd/server/wiring.go @@ -92,7 +92,11 @@ type application struct { // route options). userRepo users.UserRepository communityRepo communities.Repository - postRepo posts.Repository + // postRepo is held as the CONCRETE repository rather than posts.Repository: + // the comment service's PostReader requires the admission-aware + // VisibleHeaderView as well, and storing the narrower interface here would + // erase it before the wiring could hand it over. + postRepo *postgresRepo.PostRepository voteRepo votes.Repository commentRepo comments.Repository userBlockRepo userblocks.Repository diff --git a/docs/PRD_AUTHOR_OWNED_POSTS.md b/docs/PRD_AUTHOR_OWNED_POSTS.md index c96756d..0a10ea5 100644 --- a/docs/PRD_AUTHOR_OWNED_POSTS.md +++ b/docs/PRD_AUTHOR_OWNED_POSTS.md @@ -775,10 +775,21 @@ the loop's throwaway tracker (file as issues; none blocks the current feature): writers-stopped maintenance window (`TRUNCATE post_submissions` alongside) — a live retype strands in-flight dedupe reservations. Ripples through `enhanceExternalEmbed`/`postV2From`. -- **`community.post_count` incrementer:** wire onto - `countAcceptedPostsForCommunity` (increment on →accepted, decrement on - accepted→removed/rejected in the admission consumer). Cosmetic — display - already excludes non-accepted; no leak. +- ~~**`community.post_count` incrementer**~~ **— DONE, and done the other way.** + The served `postCount` is now a LIVE, visibility-gated subquery over the same + predicate the feeds run (`visiblePostCountSubquery`, `post_visibility.go`), + wired into `community.get`/`.list`/`.search` and into the `sort=active` key, + which was previously ordering by a uniformly-zero column. A stored counter + needed advancing on →accepted and decrementing on removal, re-acceptance + drift, rejection and author tombstone — five chances to disagree with what a + reader can actually reach; the subquery cannot disagree because it *is* the + read path's answer. It is also collection-aware, where the accepted-only + count it replaced would have undercounted every legacy + `social.coves.community.post` (accepted by construction, no admission row). + **Remaining follow-up:** the stored `communities.post_count` column, + `IncrementPostCount` and its `communities.Repository` entry are now vestigial + — nothing reads what they write. Drop them with a migration when the legacy + drain lands (they are annotated as vestigial at the source in the meantime). - **Orphan `community_post_admissions` sweep on community deletion.** - **Ingestion-lane abuse hardening:** per-source-DID token bucket on the shared posts consumer (remaining ~4.2s transient-retry stall on @@ -789,10 +800,17 @@ the loop's throwaway tracker (file as issues; none blocks the current feature): appview container late, so `.ci-out/appview.log` loses the early-run window (capture continuously). Never `go mod tidy` (breaks a go-log transitive; go-car is pinned indirect). -- **Deferred product/UX:** author-self-view on `post.get` (needs a viewer-aware - `GetViewsByURIs`; author reaches own posts via `actor.getPosts` + `getStatus` - today); self-hoster SSRF allowance for private-address unfurl targets - (`IS_DEV_ENV` is the only current escape hatch). +- ~~**Author-self-view on `post.get`**~~ **— DONE.** `GetViewsByURIs` takes a + viewer DID and `post.get` threads `req.ViewerDID` into it, so a permalink now + gives an author the same answer their own profile, the feeds and the + `getComments` thread header already gave them. `""` remains the fail-closed + anonymous value. One narrow case stays hidden from the author by design: an + `accepted` row whose `accepted_cid` does not match `posts.cid` (the §5.5 + drifted window, and the representable NULL-pin variant) is hidden from + everyone, author included — `post.getStatus` reports it from the admission row + instead. +- **Deferred product/UX:** self-hoster SSRF allowance for private-address + unfurl targets (`IS_DEV_ENV` is the only current escape hatch). **Open product question for the owner:** comments bypass admission entirely — a banned author can still comment (PRD open question #2). Decide whether bans diff --git a/internal/api/handlers/actor/get_comments_test.go b/internal/api/handlers/actor/get_comments_test.go index fd2b3d9..cf2379c 100644 --- a/internal/api/handlers/actor/get_comments_test.go +++ b/internal/api/handlers/actor/get_comments_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "Coves/internal/api/middleware" "Coves/internal/core/comments" "Coves/internal/core/posts" "Coves/internal/core/users" @@ -568,6 +569,83 @@ func TestGetCommentsHandler_WithCommunityFilter(t *testing.T) { } } +// TestGetCommentsHandler_ViewerDIDComesFromTheAuthMiddleware is the provenance +// pin for actor.getComments, and it became a security pin the moment the +// COMMUNITY FILTER started resolving each comment's root through the read-path +// visibility predicate (PRD §6.2). +// +// With `?community=` set, the repository only lists comments whose root that +// community admitted — except for the root's own AUTHOR, who keeps the carve-out +// over their pending / rejected / removed posts. The viewer DID is what selects +// that branch, so a caller who could name the viewer from the query string could +// ask for a victim's comment history in a community AS the victim and receive +// the threads rooted at posts the community never accepted. +func TestGetCommentsHandler_ViewerDIDComesFromTheAuthMiddleware(t *testing.T) { + const victim = "did:plc:victimauthor" + const adversarialQuery = "actor=" + victim + + "&community=did:plc:community123" + + "&viewer=" + victim + + "&viewerDid=" + victim + + "&viewer_did=" + victim + + "&as=" + victim + + capture := func(t *testing.T, ctxDID string) *comments.GetActorCommentsRequest { + t.Helper() + + var got *comments.GetActorCommentsRequest + mockComments := &mockCommentService{ + getActorCommentsFunc: func(ctx context.Context, req *comments.GetActorCommentsRequest) (*comments.GetActorCommentsResponse, error) { + got = req + return &comments.GetActorCommentsResponse{Comments: []*comments.CommentView{}}, nil + }, + } + handler := NewGetCommentsHandler(mockComments, &mockUserServiceForComments{}, &mockVoteServiceForComments{}) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.actor.getComments?"+adversarialQuery, nil) + if ctxDID != "" { + req = req.WithContext(middleware.SetTestUserDID(req.Context(), ctxDID)) + } + handler.HandleGetComments(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body: %s)", rec.Code, rec.Body.String()) + } + if got == nil { + t.Fatal("the service was never called") + } + return got + } + + t.Run("an unauthenticated request carries no viewer, whatever it asks for", func(t *testing.T) { + t.Parallel() + + got := capture(t, "") + + if got.ViewerDID != nil { + t.Errorf("ViewerDID = %q for an UNAUTHENTICATED request, want nil. A query parameter became the viewer "+ + "identity, so an anonymous caller can read %q's comments rooted at posts the named community never "+ + "admitted", *got.ViewerDID, victim) + } + }) + + t.Run("an authenticated request carries the context DID, and no parameter displaces it", func(t *testing.T) { + t.Parallel() + + const authenticated = "did:plc:realsessionviewer" + got := capture(t, authenticated) + + if got.ViewerDID == nil { + t.Fatal("ViewerDID = nil; the DID the auth middleware put on the context was dropped, which downgrades an " + + "author's own community-filtered history to an anonymous read") + } + if *got.ViewerDID != authenticated { + t.Errorf("ViewerDID = %q, want %q — the viewer identity must come from the auth middleware and never from "+ + "the query string", *got.ViewerDID, authenticated) + } + }) +} + func TestGetCommentsHandler_ServiceError_Returns500(t *testing.T) { // Test that generic service errors (database failures, etc.) return 500 mockComments := &mockCommentService{ diff --git a/internal/api/handlers/actor/get_posts_test.go b/internal/api/handlers/actor/get_posts_test.go index 44780be..5963d83 100644 --- a/internal/api/handlers/actor/get_posts_test.go +++ b/internal/api/handlers/actor/get_posts_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "testing" + "Coves/internal/api/middleware" "Coves/internal/core/blueskypost" "Coves/internal/core/posts" "Coves/internal/core/users" @@ -332,6 +333,85 @@ func TestGetPostsHandler_HandleResolution(t *testing.T) { } } +// TestGetPostsHandler_ViewerDIDComesFromTheAuthMiddleware is a PROVENANCE pin, +// and on this endpoint it is the sharpest of the set: actor.getPosts is the one +// read path whose whole job is "show me THIS author's posts". +// +// Under author-owned posts, ViewerDID no longer only drives block filtering — it +// unlocks the author carve-out inside the read-path visibility predicate +// (`p.author_did = $viewer` in visiblePostsJoin), which is what lets an author +// see their own pending / rejected / removed posts on their own profile. So a +// caller who could set both `actor` and the viewer identity from the query string +// would be able to ask for a victim's profile AS that victim and receive every +// post the victim's communities never admitted. +// +// The current handler is correct — it takes the DID from middleware.GetUserDID +// and assigns it after parseRequest — but nothing pinned that, and the mutation a +// reviewer demonstrated (a three-line "preview as user" override) left the whole +// suite green. Both halves are asserted: absent without auth even when the query +// begs for it, and exactly the context DID when authenticated. +func TestGetPostsHandler_ViewerDIDComesFromTheAuthMiddleware(t *testing.T) { + const victim = "did:plc:victimauthor" + const adversarialQuery = "actor=" + victim + + "&viewer=" + victim + + "&viewerDid=" + victim + + "&viewer_did=" + victim + + "&author=" + victim + + "&as=" + victim + + capture := func(t *testing.T, ctxDID string) posts.GetAuthorPostsRequest { + t.Helper() + + var got posts.GetAuthorPostsRequest + mockPosts := &mockPostService{ + getAuthorPostsFunc: func(ctx context.Context, req posts.GetAuthorPostsRequest) (*posts.GetAuthorPostsResponse, error) { + got = req + return &posts.GetAuthorPostsResponse{Feed: []*posts.FeedViewPost{}}, nil + }, + } + handler := NewGetPostsHandler(mockPosts, &mockUserService{}, &mockVoteService{}, &mockBlueskyService{}) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.actor.getPosts?"+adversarialQuery, nil) + if ctxDID != "" { + req = req.WithContext(middleware.SetTestUserDID(req.Context(), ctxDID)) + } + handler.HandleGetPosts(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body: %s)", rec.Code, rec.Body.String()) + } + return got + } + + t.Run("an unauthenticated request carries no viewer, whatever it asks for", func(t *testing.T) { + t.Parallel() + + got := capture(t, "") + + if got.ActorDID != victim { + t.Fatalf("ActorDID = %q, want %q — the fixture must actually be asking for the victim's profile", got.ActorDID, victim) + } + if got.ViewerDID != "" { + t.Errorf("ViewerDID = %q for an UNAUTHENTICATED request, want \"\". A query parameter became the viewer "+ + "identity, so an anonymous caller can read %q's pending, rejected and removed posts through the "+ + "visibility predicate's author branch", got.ViewerDID, victim) + } + }) + + t.Run("an authenticated request carries the context DID, and no parameter displaces it", func(t *testing.T) { + t.Parallel() + + const authenticated = "did:plc:realsessionviewer" + got := capture(t, authenticated) + + if got.ViewerDID != authenticated { + t.Errorf("ViewerDID = %q, want %q — the viewer identity must come from the auth middleware's context "+ + "value and never from the query string, however the query spells it", got.ViewerDID, authenticated) + } + }) +} + func TestGetPostsHandler_DirectDIDPassthrough(t *testing.T) { receivedDID := "" mockPosts := &mockPostService{ diff --git a/internal/api/handlers/communityFeed/get_community_test.go b/internal/api/handlers/communityFeed/get_community_test.go new file mode 100644 index 0000000..ea948ae --- /dev/null +++ b/internal/api/handlers/communityFeed/get_community_test.go @@ -0,0 +1,189 @@ +package communityFeed + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "Coves/internal/api/middleware" + "Coves/internal/core/communityFeeds" +) + +// fakeCommunityFeedService captures the request the handler built so a test can +// assert on it. It never reaches a database, so the whole file is T0. +type fakeCommunityFeedService struct { + got *communityFeeds.GetCommunityFeedRequest + callErr error +} + +func (f *fakeCommunityFeedService) GetCommunityFeed( + ctx context.Context, + req communityFeeds.GetCommunityFeedRequest, +) (*communityFeeds.FeedResponse, error) { + captured := req + f.got = &captured + if f.callErr != nil { + return nil, f.callErr + } + return &communityFeeds.FeedResponse{Feed: []*communityFeeds.FeedViewPost{}}, nil +} + +// getCommunity drives the handler over a raw query string and returns the +// recorder plus the service that captured the request. +func getCommunity(t *testing.T, query string, ctxDID string) (*httptest.ResponseRecorder, *fakeCommunityFeedService) { + t.Helper() + + svc := &fakeCommunityFeedService{} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.communityFeed.getCommunity?"+query, nil) + if ctxDID != "" { + req = req.WithContext(middleware.SetTestUserDID(req.Context(), ctxDID)) + } + + NewGetCommunityHandler(svc, nil, nil).HandleGetCommunity(rec, req) + return rec, svc +} + +// TestGetCommunity_ViewerDIDComesFromTheAuthMiddleware is a PROVENANCE pin, and +// under author-owned posts it is a security pin rather than a correctness one. +// +// ViewerDID used to drive block filtering only, so a wrong value was a +// self-inflicted wound. It now also unlocks the author carve-out inside the +// read-path visibility predicate (`p.author_did = $viewer` in visiblePostsJoin): +// whoever controls that string sees that author's PENDING, REJECTED and REMOVED +// posts — content no community has agreed to carry. The assignment lives inside +// parseRequest, the same function that reads the query string, so the only thing +// standing between an unauthenticated caller and any author's unadmitted posts is +// that nobody adds `?viewer=` to that function. A reviewer demonstrated the +// mutation: three lines of "preview as user" in parseRequest, whole suite still +// green. +// +// This test is that missing tripwire: no auth context plus adversarial query +// parameters must produce an EMPTY viewer, and an authenticated request must +// carry the context's DID with no query parameter able to displace it. +func TestGetCommunity_ViewerDIDComesFromTheAuthMiddleware(t *testing.T) { + // Every name a "preview as user" parameter might plausibly be given, plus the + // ones this endpoint already reads, so a mutation cannot hide behind a + // spelling this test forgot. + const adversarialQuery = "community=did:plc:targetcommunity" + + "&viewer=did:plc:victimauthor" + + "&viewerDid=did:plc:victimauthor" + + "&viewer_did=did:plc:victimauthor" + + "&actor=did:plc:victimauthor" + + "&author=did:plc:victimauthor" + + "&as=did:plc:victimauthor" + + t.Run("an unauthenticated request carries no viewer, whatever it asks for", func(t *testing.T) { + t.Parallel() + + rec, svc := getCommunity(t, adversarialQuery, "") + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body: %s)", rec.Code, rec.Body.String()) + } + if svc.got == nil { + t.Fatal("the service was never called") + } + if svc.got.ViewerDID != "" { + t.Errorf("ViewerDID = %q for an UNAUTHENTICATED request, want \"\". A query parameter reached the viewer "+ + "identity: the anonymous internet can now name any author and read that author's pending, rejected and "+ + "removed posts through visiblePostsJoin's author carve-out", svc.got.ViewerDID) + } + }) + + t.Run("an authenticated request carries the context DID, and no parameter displaces it", func(t *testing.T) { + t.Parallel() + + const authenticated = "did:plc:realsessionviewer" + rec, svc := getCommunity(t, adversarialQuery, authenticated) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body: %s)", rec.Code, rec.Body.String()) + } + if svc.got == nil { + t.Fatal("the service was never called") + } + if svc.got.ViewerDID != authenticated { + t.Errorf("ViewerDID = %q, want %q — the viewer identity must come from the auth middleware's context "+ + "value and from nowhere else", svc.got.ViewerDID, authenticated) + } + }) +} + +// TestGetCommunity_ParsesQueryParameters pins the rest of parseRequest so the +// provenance test above is not the only thing describing this handler: a +// defaulted sort/limit is what makes an unbounded feed read impossible. +func TestGetCommunity_ParsesQueryParameters(t *testing.T) { + t.Parallel() + + t.Run("defaults", func(t *testing.T) { + t.Parallel() + _, svc := getCommunity(t, "community=did:plc:c", "") + if svc.got.Sort != "hot" { + t.Errorf("Sort = %q, want %q", svc.got.Sort, "hot") + } + if svc.got.Limit != 15 { + t.Errorf("Limit = %d, want 15", svc.got.Limit) + } + if svc.got.Cursor != nil { + t.Errorf("Cursor = %q, want nil — an absent cursor must not become an empty page token", *svc.got.Cursor) + } + if svc.got.Timeframe != "" { + t.Errorf("Timeframe = %q, want empty for a non-top sort", svc.got.Timeframe) + } + }) + + t.Run("top sort defaults the timeframe", func(t *testing.T) { + t.Parallel() + _, svc := getCommunity(t, "community=did:plc:c&sort=top", "") + if svc.got.Timeframe != "day" { + t.Errorf("Timeframe = %q, want %q", svc.got.Timeframe, "day") + } + }) + + t.Run("explicit values are forwarded", func(t *testing.T) { + t.Parallel() + _, svc := getCommunity(t, "community=cats.coves.social&sort=new&limit=42&cursor=abc", "") + if svc.got.Community != "cats.coves.social" { + t.Errorf("Community = %q, want %q", svc.got.Community, "cats.coves.social") + } + if svc.got.Sort != "new" { + t.Errorf("Sort = %q, want %q", svc.got.Sort, "new") + } + if svc.got.Limit != 42 { + t.Errorf("Limit = %d, want 42", svc.got.Limit) + } + if svc.got.Cursor == nil || *svc.got.Cursor != "abc" { + t.Errorf("Cursor = %v, want %q", svc.got.Cursor, "abc") + } + }) +} + +// TestGetCommunity_RejectsNonGET keeps the query surface a query: an XRPC query +// answering POST would be a CSRF-shaped write surface. +func TestGetCommunity_RejectsNonGET(t *testing.T) { + t.Parallel() + + for _, method := range []string{http.MethodPost, http.MethodPut, http.MethodDelete} { + method := method + t.Run(method, func(t *testing.T) { + t.Parallel() + svc := &fakeCommunityFeedService{} + rec := httptest.NewRecorder() + req := httptest.NewRequest(method, "/xrpc/social.coves.communityFeed.getCommunity?community=did:plc:c", nil) + + NewGetCommunityHandler(svc, nil, nil).HandleGetCommunity(rec, req) + + if rec.Code != http.StatusMethodNotAllowed { + t.Errorf("status = %d, want %d", rec.Code, http.StatusMethodNotAllowed) + } + if svc.got != nil { + t.Error("the service was called for a non-GET request") + } + }) + } +} + +// compile-time guard: the fake must stay a communityFeeds.Service. +var _ communityFeeds.Service = (*fakeCommunityFeedService)(nil) diff --git a/internal/api/handlers/discover/get_discover_test.go b/internal/api/handlers/discover/get_discover_test.go new file mode 100644 index 0000000..5428463 --- /dev/null +++ b/internal/api/handlers/discover/get_discover_test.go @@ -0,0 +1,162 @@ +package discover + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "Coves/internal/api/middleware" + "Coves/internal/core/discover" +) + +// fakeDiscoverService captures the request the handler built. Nothing here +// reaches a database, so the file is T0. +type fakeDiscoverService struct { + got *discover.GetDiscoverRequest +} + +func (f *fakeDiscoverService) GetDiscover(ctx context.Context, req discover.GetDiscoverRequest) (*discover.DiscoverResponse, error) { + captured := req + f.got = &captured + return &discover.DiscoverResponse{Feed: []*discover.FeedViewPost{}}, nil +} + +func getDiscover(t *testing.T, query string, ctxDID string) (*httptest.ResponseRecorder, *fakeDiscoverService) { + t.Helper() + + svc := &fakeDiscoverService{} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getDiscover?"+query, nil) + if ctxDID != "" { + req = req.WithContext(middleware.SetTestUserDID(req.Context(), ctxDID)) + } + + NewGetDiscoverHandler(svc, nil, nil).HandleGetDiscover(rec, req) + return rec, svc +} + +// TestGetDiscover_ViewerDIDComesFromTheAuthMiddleware is the provenance pin for +// the public feed, and it is the one that matters most: getDiscover is the +// UNAUTHENTICATED surface, spanning every community. +// +// ViewerDID now unlocks the author carve-out inside visiblePostsJoin +// (`p.author_did = $viewer`), so a viewer string taken from the query string +// would let an anonymous caller enumerate any author's pending, rejected and +// removed posts across the whole network from one endpoint. The assignment lives +// inside parseRequest — the same function that reads the query — so nothing but +// this test stands between the current correct code and the "preview as user" +// mutation a reviewer demonstrated. +func TestGetDiscover_ViewerDIDComesFromTheAuthMiddleware(t *testing.T) { + const adversarialQuery = "sort=new" + + "&viewer=did:plc:victimauthor" + + "&viewerDid=did:plc:victimauthor" + + "&viewer_did=did:plc:victimauthor" + + "&actor=did:plc:victimauthor" + + "&author=did:plc:victimauthor" + + "&as=did:plc:victimauthor" + + t.Run("an unauthenticated request carries no viewer, whatever it asks for", func(t *testing.T) { + t.Parallel() + + rec, svc := getDiscover(t, adversarialQuery, "") + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body: %s)", rec.Code, rec.Body.String()) + } + if svc.got == nil { + t.Fatal("the service was never called") + } + if svc.got.ViewerDID != "" { + t.Errorf("ViewerDID = %q for an UNAUTHENTICATED request, want \"\". A query parameter reached the viewer "+ + "identity, which means the anonymous internet can name any author and read that author's unadmitted "+ + "posts through the visibility predicate's author branch", svc.got.ViewerDID) + } + }) + + t.Run("an authenticated request carries the context DID, and no parameter displaces it", func(t *testing.T) { + t.Parallel() + + const authenticated = "did:plc:realsessionviewer" + rec, svc := getDiscover(t, adversarialQuery, authenticated) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body: %s)", rec.Code, rec.Body.String()) + } + if svc.got == nil { + t.Fatal("the service was never called") + } + if svc.got.ViewerDID != authenticated { + t.Errorf("ViewerDID = %q, want %q — the viewer identity must come from the auth middleware and from "+ + "nowhere else", svc.got.ViewerDID, authenticated) + } + }) +} + +// TestGetDiscover_ParsesQueryParameters pins the parameter defaults alongside the +// provenance rule. +func TestGetDiscover_ParsesQueryParameters(t *testing.T) { + t.Parallel() + + t.Run("defaults", func(t *testing.T) { + t.Parallel() + _, svc := getDiscover(t, "", "") + if svc.got.Sort != "hot" { + t.Errorf("Sort = %q, want %q", svc.got.Sort, "hot") + } + if svc.got.Limit != 15 { + t.Errorf("Limit = %d, want 15", svc.got.Limit) + } + if svc.got.Cursor != nil { + t.Errorf("Cursor = %q, want nil", *svc.got.Cursor) + } + }) + + t.Run("top sort defaults the timeframe", func(t *testing.T) { + t.Parallel() + _, svc := getDiscover(t, "sort=top", "") + if svc.got.Timeframe != "day" { + t.Errorf("Timeframe = %q, want %q", svc.got.Timeframe, "day") + } + }) + + t.Run("explicit values are forwarded", func(t *testing.T) { + t.Parallel() + _, svc := getDiscover(t, "sort=new&limit=7&cursor=xyz", "") + if svc.got.Sort != "new" { + t.Errorf("Sort = %q, want %q", svc.got.Sort, "new") + } + if svc.got.Limit != 7 { + t.Errorf("Limit = %d, want 7", svc.got.Limit) + } + if svc.got.Cursor == nil || *svc.got.Cursor != "xyz" { + t.Errorf("Cursor = %v, want %q", svc.got.Cursor, "xyz") + } + }) +} + +// TestGetDiscover_RejectsNonGET keeps the query surface a query. +func TestGetDiscover_RejectsNonGET(t *testing.T) { + t.Parallel() + + for _, method := range []string{http.MethodPost, http.MethodPut, http.MethodDelete} { + method := method + t.Run(method, func(t *testing.T) { + t.Parallel() + svc := &fakeDiscoverService{} + rec := httptest.NewRecorder() + req := httptest.NewRequest(method, "/xrpc/social.coves.feed.getDiscover", nil) + + NewGetDiscoverHandler(svc, nil, nil).HandleGetDiscover(rec, req) + + if rec.Code != http.StatusMethodNotAllowed { + t.Errorf("status = %d, want %d", rec.Code, http.StatusMethodNotAllowed) + } + if svc.got != nil { + t.Error("the service was called for a non-GET request") + } + }) + } +} + +var _ discover.Service = (*fakeDiscoverService)(nil) diff --git a/internal/api/handlers/timeline/get_timeline_test.go b/internal/api/handlers/timeline/get_timeline_test.go new file mode 100644 index 0000000..da3be2f --- /dev/null +++ b/internal/api/handlers/timeline/get_timeline_test.go @@ -0,0 +1,176 @@ +package timeline + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "Coves/internal/api/middleware" + "Coves/internal/core/timeline" +) + +// fakeTimelineService captures the request the handler built. No infrastructure, +// so this file is T0. +type fakeTimelineService struct { + got *timeline.GetTimelineRequest +} + +func (f *fakeTimelineService) GetTimeline(ctx context.Context, req timeline.GetTimelineRequest) (*timeline.TimelineResponse, error) { + captured := req + f.got = &captured + return &timeline.TimelineResponse{Feed: []*timeline.FeedViewPost{}}, nil +} + +func getTimeline(t *testing.T, query string, ctxDID string) (*httptest.ResponseRecorder, *fakeTimelineService) { + t.Helper() + + svc := &fakeTimelineService{} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/xrpc/social.coves.feed.getTimeline?"+query, nil) + if ctxDID != "" { + req = req.WithContext(middleware.SetTestUserDID(req.Context(), ctxDID)) + } + + NewGetTimelineHandler(svc, nil, nil).HandleGetTimeline(rec, req) + return rec, svc +} + +// TestGetTimeline_UserDIDComesFromTheAuthMiddleware is the provenance pin for the +// subscribed feed. +// +// The timeline's UserDID is both the subscription key AND the viewer identity the +// repository binds into the read-path visibility predicate, where it unlocks the +// author carve-out (`p.author_did = $viewer`). A query-supplied value would +// therefore do two things at once: read a stranger's subscriptions, and surface +// that stranger's pending/rejected/removed posts. The endpoint is RequireAuth, so +// the pin has a third obligation the other feeds do not — an unauthenticated call +// must be refused outright rather than served with an empty viewer. +func TestGetTimeline_UserDIDComesFromTheAuthMiddleware(t *testing.T) { + const adversarialQuery = "sort=new" + + "&viewer=did:plc:victimauthor" + + "&viewerDid=did:plc:victimauthor" + + "&viewer_did=did:plc:victimauthor" + + "&user=did:plc:victimauthor" + + "&userDid=did:plc:victimauthor" + + "&actor=did:plc:victimauthor" + + "&as=did:plc:victimauthor" + + t.Run("an unauthenticated request is refused, whatever it asks for", func(t *testing.T) { + t.Parallel() + + rec, svc := getTimeline(t, adversarialQuery, "") + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 (body: %s)", rec.Code, rec.Body.String()) + } + if svc.got != nil { + t.Errorf("the service ran for an unauthenticated request with UserDID = %q; a query parameter became an "+ + "identity, which reads a stranger's subscriptions AND their unadmitted posts", svc.got.UserDID) + } + }) + + t.Run("an authenticated request carries the context DID, and no parameter displaces it", func(t *testing.T) { + t.Parallel() + + const authenticated = "did:plc:realsessionviewer" + rec, svc := getTimeline(t, adversarialQuery, authenticated) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body: %s)", rec.Code, rec.Body.String()) + } + if svc.got == nil { + t.Fatal("the service was never called") + } + if svc.got.UserDID != authenticated { + t.Errorf("UserDID = %q, want %q — the identity must come from the auth middleware and from nowhere else", + svc.got.UserDID, authenticated) + } + }) + + t.Run("a non-DID context value is refused", func(t *testing.T) { + t.Parallel() + + // The handler requires a did: prefix, so a context value that is not a DID + // (a handle, a truncated token) cannot become a viewer identity. + rec, svc := getTimeline(t, "sort=new", "not-a-did") + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 (body: %s)", rec.Code, rec.Body.String()) + } + if svc.got != nil { + t.Errorf("the service ran with a non-DID identity %q", svc.got.UserDID) + } + }) +} + +// TestGetTimeline_ParsesQueryParameters pins the defaults next to the provenance +// rule so the parameter surface is described in one place. +func TestGetTimeline_ParsesQueryParameters(t *testing.T) { + t.Parallel() + + const viewer = "did:plc:timelineviewer" + + t.Run("defaults", func(t *testing.T) { + t.Parallel() + _, svc := getTimeline(t, "", viewer) + if svc.got.Sort != "hot" { + t.Errorf("Sort = %q, want %q", svc.got.Sort, "hot") + } + if svc.got.Limit != 15 { + t.Errorf("Limit = %d, want 15", svc.got.Limit) + } + if svc.got.Cursor != nil { + t.Errorf("Cursor = %q, want nil", *svc.got.Cursor) + } + }) + + t.Run("top sort defaults the timeframe", func(t *testing.T) { + t.Parallel() + _, svc := getTimeline(t, "sort=top", viewer) + if svc.got.Timeframe != "day" { + t.Errorf("Timeframe = %q, want %q", svc.got.Timeframe, "day") + } + }) + + t.Run("explicit values are forwarded", func(t *testing.T) { + t.Parallel() + _, svc := getTimeline(t, "sort=new&limit=9&cursor=pqr", viewer) + if svc.got.Sort != "new" { + t.Errorf("Sort = %q, want %q", svc.got.Sort, "new") + } + if svc.got.Limit != 9 { + t.Errorf("Limit = %d, want 9", svc.got.Limit) + } + if svc.got.Cursor == nil || *svc.got.Cursor != "pqr" { + t.Errorf("Cursor = %v, want %q", svc.got.Cursor, "pqr") + } + }) +} + +// TestGetTimeline_RejectsNonGET keeps the query surface a query. +func TestGetTimeline_RejectsNonGET(t *testing.T) { + t.Parallel() + + for _, method := range []string{http.MethodPost, http.MethodPut, http.MethodDelete} { + method := method + t.Run(method, func(t *testing.T) { + t.Parallel() + svc := &fakeTimelineService{} + rec := httptest.NewRecorder() + req := httptest.NewRequest(method, "/xrpc/social.coves.feed.getTimeline", nil) + req = req.WithContext(middleware.SetTestUserDID(req.Context(), "did:plc:timelineviewer")) + + NewGetTimelineHandler(svc, nil, nil).HandleGetTimeline(rec, req) + + if rec.Code != http.StatusMethodNotAllowed { + t.Errorf("status = %d, want %d", rec.Code, http.StatusMethodNotAllowed) + } + if svc.got != nil { + t.Error("the service was called for a non-GET request") + } + }) + } +} + +var _ timeline.Service = (*fakeTimelineService)(nil) diff --git a/internal/atproto/jetstream/bridged_stats_test.go b/internal/atproto/jetstream/bridged_stats_test.go index 935a5f3..b61595e 100644 --- a/internal/atproto/jetstream/bridged_stats_test.go +++ b/internal/atproto/jetstream/bridged_stats_test.go @@ -126,7 +126,7 @@ func TestPostConsumer_Create_WithBridgedStats(t *testing.T) { // Read-path fold: displayed stats include bridged counts. repo := postgres.NewPostRepository(db) - views, err := repo.GetViewsByURIs(ctx, []string{uri}) + views, err := repo.GetViewsByURIs(ctx, []string{uri}, "") require.NoError(t, err) view := views[uri] require.NotNil(t, view) @@ -351,7 +351,7 @@ func TestPostConsumer_InclusiveScore_NativeVotesStackOnBridged(t *testing.T) { // Displayed stats fold native + bridged. repo := postgres.NewPostRepository(db) - views, err := repo.GetViewsByURIs(ctx, []string{uri}) + views, err := repo.GetViewsByURIs(ctx, []string{uri}, "") require.NoError(t, err) assert.Equal(t, 31, views[uri].Stats.Upvotes) assert.Equal(t, 2, views[uri].Stats.Downvotes) diff --git a/internal/core/blobs/blob_upload_integration_test.go b/internal/core/blobs/blob_upload_integration_test.go index 392109d..f6d833d 100644 --- a/internal/core/blobs/blob_upload_integration_test.go +++ b/internal/core/blobs/blob_upload_integration_test.go @@ -134,7 +134,7 @@ func TestBlobUpload_E2E_PostWithImages(t *testing.T) { // STEP 5: Verify post was indexed with blob reference postURI := fmt.Sprintf("at://%s/social.coves.community.post/%s", community.DID, rkey) - indexedPost, err := postRepo.GetByURI(ctx, postURI) + indexedPost, err := postRepo.GetRawIndexedRow(ctx, postURI) require.NoError(t, err, "Post should be indexed") // Verify embed contains blob (Embed is stored as *string JSON in DB) @@ -277,7 +277,7 @@ func TestBlobUpload_E2E_PostWithImages(t *testing.T) { // Verify all images indexed postURI := fmt.Sprintf("at://%s/social.coves.community.post/%s", community.DID, rkey) - indexedPost, err := postRepo.GetByURI(ctx, postURI) + indexedPost, err := postRepo.GetRawIndexedRow(ctx, postURI) require.NoError(t, err, "Multi-image post should be indexed") // Parse embed JSON @@ -336,7 +336,7 @@ func TestBlobUpload_E2E_PostWithImages(t *testing.T) { // Verify thumbnail blob indexed postURI := fmt.Sprintf("at://%s/social.coves.community.post/%s", community.DID, rkey) - indexedPost, err := postRepo.GetByURI(ctx, postURI) + indexedPost, err := postRepo.GetRawIndexedRow(ctx, postURI) require.NoError(t, err, "External embed post should be indexed") // Parse embed JSON diff --git a/internal/core/comments/comment.go b/internal/core/comments/comment.go index 0c99f7f..0c36c23 100644 --- a/internal/core/comments/comment.go +++ b/internal/core/comments/comment.go @@ -92,6 +92,14 @@ type SelfLabel struct { type ListByCommenterRequest struct { CommenterDID string // Required: DID of the commenter CommunityDID *string // Optional: filter to comments in a specific community - Limit int // Max comments to return (1-100) - Cursor *string // Pagination cursor from previous response + // ViewerDID is the AUTHENTICATED caller's DID ("" for an anonymous read), and + // it is a security parameter rather than a personalization one: the community + // filter resolves each comment's root through the read-path visibility + // predicate, where the viewer unlocks the author's own carve-out over their + // pending / rejected / removed posts (PRD §6.2). It MUST come from the auth + // middleware — never from a query parameter — or any caller can read any + // author's unadmitted content. + ViewerDID string + Limit int // Max comments to return (1-100) + Cursor *string // Pagination cursor from previous response } diff --git a/internal/core/comments/comment_actor_viewer_test.go b/internal/core/comments/comment_actor_viewer_test.go new file mode 100644 index 0000000..6fa232e --- /dev/null +++ b/internal/core/comments/comment_actor_viewer_test.go @@ -0,0 +1,80 @@ +package comments + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// GetActorComments' viewer DID is a SECURITY parameter on the community-filtered +// path, not a personalization one. +// +// The repository resolves each comment's root through the read-path visibility +// predicate when a community is named (PRD §6.2), and that predicate's author +// branch is `p.author_did = $viewer`: the bound viewer is what decides whether +// the caller may see comments rooted at pending / rejected / removed posts. A +// service that forgot to thread it would silently downgrade every authenticated +// author to an anonymous read — the author's own pending threads vanish from +// their own profile — while threading the WRONG string would hand a caller +// another author's unadmitted content. Both directions are pinned here, at T0, +// because the repository cannot tell a dropped viewer from a genuinely anonymous +// one. +func TestGetActorComments_ViewerDIDReachesTheRepository(t *testing.T) { + const actorDID = "did:plc:actorwithcomments" + const viewerDID = "did:plc:authenticatedviewer" + + capture := func(t *testing.T, viewer *string) ListByCommenterRequest { + t.Helper() + + commentRepo := newMockCommentRepo() + var got ListByCommenterRequest + commentRepo.listByCommenterWithCursorFunc = func(ctx context.Context, req ListByCommenterRequest) ([]*Comment, *string, error) { + got = req + return []*Comment{}, nil, nil + } + + service := NewCommentService(commentRepo, newMockUserRepo(), newMockPostRepo(), newMockCommunityRepo(), nil, nil, nil) + _, err := service.GetActorComments(context.Background(), &GetActorCommentsRequest{ + ActorDID: actorDID, + Community: "did:plc:somecommunity", + ViewerDID: viewer, + Limit: 50, + }) + require.NoError(t, err) + return got + } + + t.Run("an authenticated viewer is forwarded verbatim", func(t *testing.T) { + t.Parallel() + + viewer := viewerDID + got := capture(t, &viewer) + + assert.Equal(t, viewerDID, got.ViewerDID, + "the authenticated viewer never reached the repository, so the community-filtered listing runs as an "+ + "anonymous read and an author loses their own pending threads from their own profile") + }) + + t.Run("an anonymous request forwards an empty viewer", func(t *testing.T) { + t.Parallel() + + got := capture(t, nil) + + assert.Equal(t, "", got.ViewerDID, + "an unauthenticated request must bind an EMPTY viewer DID: any non-empty value unlocks the visibility "+ + "predicate's author carve-out for whoever that DID names") + }) + + t.Run("the community filter is resolved alongside it", func(t *testing.T) { + t.Parallel() + + viewer := viewerDID + got := capture(t, &viewer) + + require.NotNil(t, got.CommunityDID, "a DID-form community must be forwarded as the filter") + assert.Equal(t, "did:plc:somecommunity", *got.CommunityDID) + assert.Equal(t, actorDID, got.CommenterDID) + }) +} diff --git a/internal/core/comments/comment_header_gate_test.go b/internal/core/comments/comment_header_gate_test.go new file mode 100644 index 0000000..c1d0c8d --- /dev/null +++ b/internal/core/comments/comment_header_gate_test.go @@ -0,0 +1,151 @@ +package comments + +import ( + "context" + "testing" + + "Coves/internal/core/posts" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The getComments THREAD HEADER is a posts read path: it serves the viewed +// post's title, content, author and admission context above the comment tree. It +// must therefore answer the same question every other posts read path answers — +// has this community admitted this post, and if not, is the caller its author? +// +// The gate used to be reached through an OPTIONAL type assertion against a +// private capability interface, with a miss falling back to the pre-admission +// header builder. Today's wiring happened to satisfy it, so nothing leaked — but +// the security property rested on convention: any future decorator around the +// post repository (metrics, caching, tracing) that implemented `posts.Repository` +// and nothing more would silently turn the gate OFF, and the in-suite fakes +// exercised precisely that unsafe path. A capability you can forget to implement +// is a capability that will be forgotten. +// +// The requirement now lives in the dependency's TYPE (PostReader), so the +// compiler refuses the wiring instead of the service degrading at runtime. These +// tests pin both halves of that: the behavior when the gate says "hidden", and +// the fact that a bare posts.Repository does not satisfy the dependency. + +// headerlessPostRepo is the shape a future decorator would have: it satisfies +// posts.Repository — the embedded interface gives it every method, whatever that +// interface grows next — and NOTHING ELSE. It is the type this package must be +// unable to build a service on. +type headerlessPostRepo struct { + posts.Repository +} + +// TestCommentService_HeaderGateIsACompileTimeRequirement pins the structural half +// of the fix. +// +// The negative assertion is the load-bearing one: headerlessPostRepo is a +// complete posts.Repository, and it must NOT satisfy PostReader. Because +// PostReader is the type the constructors take, that is what makes +// `NewCommentService(..., headerlessPostRepo{}, ...)` a compile error rather than +// a service that quietly serves ungated headers. If someone widens the +// constructor back to posts.Repository to "fix a build", this pin is the note +// explaining why the build was right to fail. +func TestCommentService_HeaderGateIsACompileTimeRequirement(t *testing.T) { + t.Parallel() + + var decorator any = headerlessPostRepo{} + + _, isRepository := decorator.(posts.Repository) + require.True(t, isRepository, + "the fixture must be a complete posts.Repository, or the negative assertion below proves nothing") + + _, isReader := decorator.(PostReader) + assert.False(t, isReader, + "a bare posts.Repository satisfied PostReader, so the visibility-aware header lookup is no longer a "+ + "REQUIREMENT of the comment service's dependency — a decorator that implements only the repository "+ + "interface can be wired again, and the thread header would serve pending, rejected and removed posts to "+ + "anyone with the permalink") + + // And the fake this package's unit tests actually use must carry the gate, so + // no T0 test can exercise a service whose header lookup is absent. + var fake any = newMockPostRepo() + _, fakeIsReader := fake.(PostReader) + assert.True(t, fakeIsReader, + "the in-suite post-repository fake stopped implementing the visibility-aware header lookup. Every getComments "+ + "unit test would then be exercising a shape production can no longer wire") +} + +// TestCommentService_GetComments_HiddenHeaderIsRootNotFound is the behavioral +// half: when the gate answers "not visible to this viewer", the endpoint answers +// root-not-found — it never falls back to a softer check. +func TestCommentService_GetComments_HiddenHeaderIsRootNotFound(t *testing.T) { + const postURI = "at://did:plc:someauthor/social.coves.community.postv2/hiddenroot" + + newService := func(t *testing.T, hidden bool) Service { + t.Helper() + + postRepo := newMockPostRepo() + post := createTestPost(postURI, "did:plc:someauthor", "did:plc:somecommunity") + require.NoError(t, postRepo.Create(context.Background(), post)) + if hidden { + postRepo.hideFromHeader(postURI) + } + + return NewCommentService(newMockCommentRepo(), newMockUserRepo(), postRepo, newMockCommunityRepo(), nil, nil, nil) + } + + t.Run("a hidden root is not found, and no header is built", func(t *testing.T) { + t.Parallel() + + resp, err := newService(t, true).GetComments(context.Background(), &GetCommentsRequest{ + PostURI: postURI, + Sort: "hot", + Depth: 3, + Limit: 10, + }) + + require.ErrorIsf(t, err, ErrRootNotFound, + "the thread endpoint served a post the visibility gate hid. A permalink is all an attacker has to guess, "+ + "and the header carries the full title and content of a post no community admitted") + assert.Nil(t, resp, "no response may be built for a hidden root") + }) + + t.Run("a visible root is served with its header", func(t *testing.T) { + t.Parallel() + + resp, err := newService(t, false).GetComments(context.Background(), &GetCommentsRequest{ + PostURI: postURI, + Sort: "hot", + Depth: 3, + Limit: 10, + }) + + require.NoError(t, err) + require.NotNil(t, resp.Post, "an admitted post must still render its thread header") + header, ok := resp.Post.(*posts.PostView) + require.Truef(t, ok, "the thread header must be the admission-hydrated post view, got %T", resp.Post) + assert.Equal(t, postURI, header.URI) + }) + + t.Run("a soft-deleted root is not found", func(t *testing.T) { + t.Parallel() + + // The gate owns the soft-delete answer too: the fake's VisibleHeaderView + // hides a deleted row exactly as the repository's predicate does, so this + // case cannot regress into a second, separate check in the service. + postRepo := newMockPostRepo() + post := createTestPost(postURI, "did:plc:someauthor", "did:plc:somecommunity") + deletedAt := post.CreatedAt + post.DeletedAt = &deletedAt + require.NoError(t, postRepo.Create(context.Background(), post)) + + service := NewCommentService(newMockCommentRepo(), newMockUserRepo(), postRepo, newMockCommunityRepo(), nil, nil, nil) + _, err := service.GetComments(context.Background(), &GetCommentsRequest{ + PostURI: postURI, + Sort: "hot", + Depth: 3, + Limit: 10, + }) + + require.ErrorIs(t, err, ErrRootNotFound, + "a soft-deleted post's thread header must not be served: the 2026-07-29 defect was exactly this row "+ + "reaching an anonymous caller in full") + }) +} diff --git a/internal/core/comments/comment_service.go b/internal/core/comments/comment_service.go index 6184873..596dad7 100644 --- a/internal/core/comments/comment_service.go +++ b/internal/core/comments/comment_service.go @@ -73,12 +73,41 @@ type GetCommentsRequest struct { Limit int } +// PostReader is the post-repository surface the comment service depends on, and +// it is deliberately NARROWER TO IMPLEMENT than posts.Repository: a type must +// carry the visibility-aware header lookup as well. +// +// This is a security requirement expressed in the type system. The thread header +// getComments serves is a posts read path — full title, content and admission +// context of the viewed post — so it has to run the same admission predicate as +// post.get and the feeds. That gate used to be reached through an OPTIONAL type +// assertion, with a miss silently degrading to the pre-admission header builder; +// production happened to satisfy it, so nothing leaked, but the property rested +// on convention. Any decorator that wrapped the repository for metrics, caching +// or tracing and forwarded only posts.Repository would have turned the gate off +// with no build error, no test failure and no log line. +// +// Making it the dependency's TYPE moves that failure to compile time: a +// repository without VisibleHeaderView cannot be handed to the constructors at +// all, so there is no runtime path left that can serve an ungated header. +type PostReader interface { + posts.Repository + + // VisibleHeaderView returns the hydrated post view IFF the post is visible to + // viewerDID under the read-path visibility predicate, and (nil, nil) when it is + // hidden — a pending, rejected or removed admission for a viewer who is not the + // author, a postv2 whose admission row never seeded, or a soft-deleted row. + // The returned view is the admission-hydrated one, so status/acceptanceUri + // survive onto the served header. + VisibleHeaderView(ctx context.Context, uri, viewerDID string) (*posts.PostView, error) +} + // commentService implements the Service interface // Coordinates between repository layer and view model construction type commentService struct { commentRepo Repository // Comment data access userRepo users.UserRepository // User lookup for author hydration - postRepo posts.Repository // Post lookup for building post views + postRepo PostReader // Post lookup + the admission-aware header gate communityRepo communities.Repository // Community lookup for community hydration oauthClient *oauthclient.OAuthClient // OAuth client for PDS authentication oauthStore oauth.ClientAuthStore // OAuth session store @@ -91,7 +120,7 @@ type commentService struct { func NewCommentService( commentRepo Repository, userRepo users.UserRepository, - postRepo posts.Repository, + postRepo PostReader, communityRepo communities.Repository, oauthClient *oauthclient.OAuthClient, oauthStore oauth.ClientAuthStore, @@ -116,7 +145,7 @@ func NewCommentService( func NewCommentServiceWithPDSFactory( commentRepo Repository, userRepo users.UserRepository, - postRepo posts.Repository, + postRepo PostReader, communityRepo communities.Repository, logger *slog.Logger, factory PDSClientFactory, @@ -152,7 +181,7 @@ func (s *commentService) GetComments(ctx context.Context, req *GetCommentsReques defer cancel() // 2. Fetch post for context - post, err := s.postRepo.GetByURI(ctx, req.PostURI) + post, err := s.postRepo.GetRawIndexedRow(ctx, req.PostURI) if err != nil { // Translate post not-found errors to comment-layer errors for proper HTTP status if posts.IsNotFound(err) { @@ -214,20 +243,10 @@ func (s *commentService) GetComments(ctx context.Context, req *GetCommentsReques }, nil } -// postHeaderVisibilityChecker is the viewer-aware, admission-aware slice of the -// post repository the thread-header gate needs. The real postgres repository -// implements it (VisibleHeaderView); unit-test fakes do not, and a service built -// on a fake keeps the pre-admission behavior — see resolveVisibleHeader. -type postHeaderVisibilityChecker interface { - // VisibleHeaderView returns the hydrated post view iff the post is visible to - // viewerDID under the read-path predicate, or nil when it is hidden. - VisibleHeaderView(ctx context.Context, uri, viewerDID string) (*posts.PostView, error) -} - // resolveVisibleHeader returns the post view to render as the getComments thread // header, or ErrRootNotFound when the post must not be shown to this viewer. // -// It consults a REAL admission lookup, never the collection: the previous gate +// It consults a REAL admission lookup, never the collection: an earlier gate // inferred visibility from the URI's collection, which leaked a moderator-REMOVED // legacy community.post (a legacy row CAN carry a removed admission — applyRemoval // has no collection guard). VisibleHeaderView runs the same predicate as post.get @@ -236,33 +255,26 @@ type postHeaderVisibilityChecker interface { // header carries status/acceptanceUri instead of a second view rebuilt from the // raw Post (which dropped them). // -// A unit-test fake postRepo does not implement the checker; such a service keeps -// the pre-admission behavior, gated only on the soft-delete the raw Post carries. -// This is safe because a fake is never wired to real admission data — production -// always uses the real repository, which always implements the checker. +// THERE IS NO FALLBACK, deliberately. The lookup used to be reached through an +// optional type assertion whose miss served the pre-admission header, so a +// repository that merely FORGOT the capability disabled the gate silently. The +// requirement now lives in the PostReader dependency type, so a type without it +// cannot be wired at all — the compiler refuses the wiring instead of this +// function refusing the request, and every path through here is gated. func (s *commentService) resolveVisibleHeader(ctx context.Context, post *posts.Post, viewerDID *string) (*posts.PostView, error) { - if checker, ok := s.postRepo.(postHeaderVisibilityChecker); ok { - viewer := "" - if viewerDID != nil { - viewer = *viewerDID - } - view, err := checker.VisibleHeaderView(ctx, post.URI, viewer) - if err != nil { - return nil, fmt.Errorf("checking post header visibility: %w", err) - } - if view == nil { - return nil, ErrRootNotFound - } - return view, nil + viewer := "" + if viewerDID != nil { + viewer = *viewerDID } - // Fake repository (unit tests): no admission-aware fetch available. Honor only - // the gate the raw Post carries — a soft delete — and build the header from it - // as before. - if post.DeletedAt != nil { + view, err := s.postRepo.VisibleHeaderView(ctx, post.URI, viewer) + if err != nil { + return nil, fmt.Errorf("checking post header visibility: %w", err) + } + if view == nil { return nil, ErrRootNotFound } - return s.buildPostView(ctx, post, viewerDID), nil + return view, nil } // getCommentSubtree returns the subtree rooted at the comment identified by req.ParentRkey @@ -1303,9 +1315,22 @@ func (s *commentService) GetActorComments(ctx context.Context, req *GetActorComm } // 3. Fetch comments from repository + // + // The viewer DID is threaded because the community filter resolves each + // comment's root through the read-path visibility predicate (PRD §6.2): it + // decides whether a caller may see comments rooted at posts the named + // community has not admitted. An authenticated author keeps the carve-out over + // their own pending/rejected/removed roots; everyone else, including the + // anonymous "" viewer, sees admitted roots only. + var viewerDID string + if req.ViewerDID != nil { + viewerDID = *req.ViewerDID + } + repoReq := ListByCommenterRequest{ CommenterDID: req.ActorDID, CommunityDID: communityDID, + ViewerDID: viewerDID, Limit: req.Limit, Cursor: req.Cursor, } diff --git a/internal/core/comments/comment_service_test.go b/internal/core/comments/comment_service_test.go index ffba5d0..2b5195c 100644 --- a/internal/core/comments/comment_service_test.go +++ b/internal/core/comments/comment_service_test.go @@ -261,15 +261,61 @@ func (m *mockUserRepo) UpdateProfile(ctx context.Context, did string, input user return user, nil } -// mockPostRepo is a mock implementation of the posts.Repository interface +// mockPostRepo is a mock implementation of the PostReader interface — which is +// posts.Repository PLUS the admission-aware header lookup the comment service +// requires. Implementing the whole thing is not optional: the header gate is the +// dependency's type, so a fake that dropped VisibleHeaderView could not be handed +// to the constructors at all. That is deliberate — the fake used to be the ONE +// shape that exercised the old fail-open fallback, which meant every getComments +// unit test ran through a path production could never take. type mockPostRepo struct { posts map[string]*posts.Post + // hidden marks posts the visibility predicate refuses for the calling viewer: + // a pending / rejected / removed admission, or a postv2 whose admission row + // never seeded. The fake keeps it a plain set because what these tests assert + // is the SERVICE's reaction to a hidden answer, not the SQL that produces one + // (that is pinned at T1 in internal/db/postgres/post_visibility_test.go). + hidden map[string]bool } func newMockPostRepo() *mockPostRepo { return &mockPostRepo{ - posts: make(map[string]*posts.Post), + posts: make(map[string]*posts.Post), + hidden: make(map[string]bool), + } +} + +// hideFromHeader makes the visibility predicate refuse this post, as it does for +// any non-accepted admission state when the viewer is not the author. +func (m *mockPostRepo) hideFromHeader(uri string) { + m.hidden[uri] = true +} + +// VisibleHeaderView models the real repository's gate: nothing for an unindexed +// post, nothing for a soft-deleted one, nothing for a post the test marked +// hidden, and otherwise the hydrated header. +func (m *mockPostRepo) VisibleHeaderView(ctx context.Context, uri, viewerDID string) (*posts.PostView, error) { + post, ok := m.posts[uri] + if !ok || post.DeletedAt != nil || m.hidden[uri] { + return nil, nil + } + + view := &posts.PostView{ + URI: post.URI, + CID: post.CID, + RKey: post.RKey, + CreatedAt: post.CreatedAt, + IndexedAt: post.IndexedAt, + Author: &posts.AuthorView{DID: post.AuthorDID, Handle: post.AuthorDID}, + Community: &posts.CommunityRef{DID: post.CommunityDID, Handle: post.CommunityDID, Name: post.CommunityDID}, + Stats: &posts.PostStats{ + Upvotes: post.UpvoteCount, + Downvotes: post.DownvoteCount, + Score: post.Score, + CommentCount: post.CommentCount, + }, } + return view, nil } func (m *mockPostRepo) Create(ctx context.Context, post *posts.Post) error { @@ -277,19 +323,29 @@ func (m *mockPostRepo) Create(ctx context.Context, post *posts.Post) error { return nil } -func (m *mockPostRepo) GetByURI(ctx context.Context, uri string) (*posts.Post, error) { +func (m *mockPostRepo) GetRawIndexedRow(ctx context.Context, uri string) (*posts.Post, error) { if p, ok := m.posts[uri]; ok { return p, nil } return nil, posts.NewNotFoundError("post", uri) } +func (m *mockPostRepo) GetRawIndexedRowsByURIs(ctx context.Context, uris []string) (map[string]*posts.Post, error) { + out := make(map[string]*posts.Post, len(uris)) + for _, uri := range uris { + if p, ok := m.posts[uri]; ok { + out[uri] = p + } + } + return out, nil +} + func (m *mockPostRepo) GetByAuthor(ctx context.Context, req posts.GetAuthorPostsRequest) ([]*posts.PostView, *string, error) { // Mock implementation - returns empty for tests return nil, nil, nil } -func (m *mockPostRepo) GetViewsByURIs(ctx context.Context, uris []string) (map[string]*posts.PostView, error) { +func (m *mockPostRepo) GetViewsByURIs(ctx context.Context, uris []string, viewerDID string) (map[string]*posts.PostView, error) { // Mock implementation - returns empty for tests return map[string]*posts.PostView{}, nil } diff --git a/internal/core/comments/comment_write_test.go b/internal/core/comments/comment_write_test.go index 67ec95f..36d2af8 100644 --- a/internal/core/comments/comment_write_test.go +++ b/internal/core/comments/comment_write_test.go @@ -229,7 +229,7 @@ func TestCommentWrite_CreateTopLevelComment(t *testing.T) { // Verify post comment count updated t.Logf("\n🔍 Verifying post comment count updated...") - updatedPost, err := postRepo.GetByURI(ctx, postURI) + updatedPost, err := postRepo.GetRawIndexedRow(ctx, postURI) if err != nil { t.Fatalf("Failed to get updated post: %v", err) } diff --git a/internal/core/posts/consumer_comment_count_test.go b/internal/core/posts/consumer_comment_count_test.go index e524e58..6ffa261 100644 --- a/internal/core/posts/consumer_comment_count_test.go +++ b/internal/core/posts/consumer_comment_count_test.go @@ -152,7 +152,7 @@ func TestPostConsumer_ReconcilesCommentCountWhenCommentsArriveFirst(t *testing.T require.NoError(t, postConsumer.HandleEvent(ctx, postEvent("post-rev", postRkey, "bafypost", "Post arriving after comment"))) - post, err := postRepo.GetByURI(ctx, postURI) + post, err := postRepo.GetRawIndexedRow(ctx, postURI) require.NoError(t, err, "the post should be indexed") require.Equal(t, 1, post.CommentCount, "the post consumer should have counted the comment that arrived before it") @@ -176,7 +176,7 @@ func TestPostConsumer_ReconcilesCommentCountWhenCommentsArriveFirst(t *testing.T require.NoError(t, postConsumer.HandleEvent(ctx, postEvent("post2-rev", postRkey, "bafypost2", "Post with 3 pre-existing comments"))) - post, err := postRepo.GetByURI(ctx, postURI) + post, err := postRepo.GetRawIndexedRow(ctx, postURI) require.NoError(t, err) require.Equal(t, 3, post.CommentCount, "reconciliation counts every pre-existing comment, not just the first") @@ -193,7 +193,7 @@ func TestPostConsumer_ReconcilesCommentCountWhenCommentsArriveFirst(t *testing.T require.NoError(t, postConsumer.HandleEvent(ctx, postEvent("post3-rev", postRkey, "bafypost3", "Post with before and after comments"))) - post, err := postRepo.GetByURI(ctx, postURI) + post, err := postRepo.GetRawIndexedRow(ctx, postURI) require.NoError(t, err) require.Equal(t, 2, post.CommentCount) @@ -202,7 +202,7 @@ func TestPostConsumer_ReconcilesCommentCountWhenCommentsArriveFirst(t *testing.T // bug seen from the other end. commentOnPost(t, "after-rev", "bafyafter", "Comment after post exists", postURI, "bafypost3") - post, err = postRepo.GetByURI(ctx, postURI) + post, err = postRepo.GetRawIndexedRow(ctx, postURI) require.NoError(t, err) require.Equal(t, 3, post.CommentCount, "a comment arriving after the post should increment the reconciled count") @@ -217,7 +217,7 @@ func TestPostConsumer_ReconcilesCommentCountWhenCommentsArriveFirst(t *testing.T event := postEvent("idem-post-rev", postRkey, "bafyidempost", "Idempotent test post") require.NoError(t, postConsumer.HandleEvent(ctx, event)) - post, err := postRepo.GetByURI(ctx, postURI) + post, err := postRepo.GetRawIndexedRow(ctx, postURI) require.NoError(t, err) require.Equal(t, 1, post.CommentCount) @@ -226,7 +226,7 @@ func TestPostConsumer_ReconcilesCommentCountWhenCommentsArriveFirst(t *testing.T // it had just reconciled. require.NoError(t, postConsumer.HandleEvent(ctx, event), "a replayed post event should be a no-op, not an error") - post, err = postRepo.GetByURI(ctx, postURI) + post, err = postRepo.GetRawIndexedRow(ctx, postURI) require.NoError(t, err) require.Equal(t, 1, post.CommentCount, "replaying the post event must not reset comment_count") diff --git a/internal/core/posts/decider.go b/internal/core/posts/decider.go index cda3b16..5274858 100644 --- a/internal/core/posts/decider.go +++ b/internal/core/posts/decider.go @@ -73,8 +73,15 @@ func TrustedAggregatorDIDs() map[string]bool { // PostLookup reads the indexed post a decision is about. Satisfied by // Repository. +// +// It is the RAW, ungated row by design and this is one of the few places that is +// correct: the decider is what DECIDES whether a post becomes visible, so +// reading it through the visibility predicate would make every undecided post +// invisible to the thing that has to decide about it. See the danger banner on +// Repository.GetRawIndexedRow before copying this pattern anywhere a reader can +// see the result. type PostLookup interface { - GetByURI(ctx context.Context, uri string) (*Post, error) + GetRawIndexedRow(ctx context.Context, uri string) (*Post, error) } // AdmissionCounter is the narrow slice of AdmissionRepository the quota needs. @@ -161,7 +168,7 @@ func (d *AdmissionEngineDecider) DecideAdmission(ctx context.Context, communityD // lookup and both gate the policy: whether there is any content to judge, // and who wrote it — and the author is what the actor class is derived // from, so nothing about privilege can be decided before this returns. - post, err := d.deps.Posts.GetByURI(ctx, postURI) + post, err := d.deps.Posts.GetRawIndexedRow(ctx, postURI) switch { case err != nil && IsNotFound(err): // Absent. An admission row can legitimately exist with no post — an diff --git a/internal/core/posts/decider_quota_test.go b/internal/core/posts/decider_quota_test.go index 8913446..3d401e3 100644 --- a/internal/core/posts/decider_quota_test.go +++ b/internal/core/posts/decider_quota_test.go @@ -64,7 +64,7 @@ func (quotaBans) GetMembership(context.Context, string, string) (*communities.Me // quotaPosts serves whichever post the subject names. type quotaPosts struct{ posts map[string]*posts.Post } -func (q *quotaPosts) GetByURI(_ context.Context, uri string) (*posts.Post, error) { +func (q *quotaPosts) GetRawIndexedRow(_ context.Context, uri string) (*posts.Post, error) { if p, ok := q.posts[uri]; ok { return p, nil } diff --git a/internal/core/posts/decider_test.go b/internal/core/posts/decider_test.go index dc078dc..5c0c2b4 100644 --- a/internal/core/posts/decider_test.go +++ b/internal/core/posts/decider_test.go @@ -51,7 +51,7 @@ type stubPostLookup struct { calls int } -func (s *stubPostLookup) GetByURI(_ context.Context, _ string) (*Post, error) { +func (s *stubPostLookup) GetRawIndexedRow(_ context.Context, _ string) (*Post, error) { s.calls++ if s.err != nil { return nil, s.err diff --git a/internal/core/posts/engine_matrix_test.go b/internal/core/posts/engine_matrix_test.go index 9feca96..fb37da0 100644 --- a/internal/core/posts/engine_matrix_test.go +++ b/internal/core/posts/engine_matrix_test.go @@ -192,6 +192,11 @@ type fakeAdmissions struct { row *Admission getErr error + // byPostURIs / byPostURIsErr back GetByPostURIs, the batched lookup post.get's + // removal-tombstone path runs. The engine never reads them. + byPostURIs map[string][]*Admission + byPostURIsErr error + acceptanceResult AdmissionResult removalResult AdmissionResult rejectionResult AdmissionResult @@ -253,7 +258,10 @@ func (a *fakeAdmissions) RepinAcceptedCID(_ context.Context, _ RepinAcceptanceCo func (a *fakeAdmissions) GetByPostURIs(_ context.Context, _ []string) (map[string][]*Admission, error) { a.rec.record("GetByPostURIs") - return nil, nil + if a.byPostURIsErr != nil { + return nil, a.byPostURIsErr + } + return a.byPostURIs, nil } func (a *fakeAdmissions) ListByStatusForCommunity(_ context.Context, _ string, _ AdmissionStatus, _ int, _ *string) ([]*Admission, *string, error) { diff --git a/internal/core/posts/interfaces.go b/internal/core/posts/interfaces.go index 81cdbc9..7c01e78 100644 --- a/internal/core/posts/interfaces.go +++ b/internal/core/posts/interfaces.go @@ -85,15 +85,53 @@ type Repository interface { // Called by Jetstream consumer after post is created on PDS Create(ctx context.Context, post *Post) error - // GetByURI retrieves a post by its AT-URI - // Used for E2E test verification and single-record lookups (returns the raw - // record without author/community joins) - GetByURI(ctx context.Context, uri string) (*Post, error) + // ──────────────────────────────────────────────────────────────────────── + // DANGER — GetRawIndexedRow IS NOT A DISPLAY READ. It applies NEITHER the + // admission visibility predicate NOR `deleted_at IS NULL`, so it returns + // the full title and content of a post that is pending, rejected, removed + // by a moderator, or soft-deleted by its own author. + // + // It is named for what it is — the raw indexed row — precisely so that it + // cannot be reached for by accident, because misuse is SILENT: it selects + // bare columns with no join a compiler could miss and no error a test would + // see, just a hidden post's content on the wire. + // + // If you are hydrating anything a reader will SEE, use one of: + // • GetViewsByURIs(ctx, uris, viewerDID) — batch, hydrated, gated + // • VisibleHeaderView(ctx, uri, viewerDID) — single post, hydrated, gated + // (on the concrete postgres repo; the comment service requires it + // through comments.PostReader, so the binding is a compile error to omit) + // + // Legitimate callers are the ones that must see a row REGARDLESS of who may + // look at it: the admission decider (it decides visibility, so it cannot + // depend on it) and the post.get removal path (a tombstone is emitted for a + // post the predicate has already hidden). Both check what they need + // themselves — the removal path re-checks deleted_at explicitly. + // ──────────────────────────────────────────────────────────────────────── + GetRawIndexedRow(ctx context.Context, uri string) (*Post, error) + + // GetRawIndexedRowsByURIs is the batched GetRawIndexedRow: same ungated raw + // rows, one round trip. THE SAME DANGER APPLIES — read the banner above + // before calling it. URIs with no indexed row are absent from the map. + // + // It exists because the post.get removal path needs the community and + // soft-delete state of every absent URI in a caller-supplied batch, and + // looping GetRawIndexedRow there put an N+1 on a public endpoint whose URI + // list the caller controls. + GetRawIndexedRowsByURIs(ctx context.Context, uris []string) (map[string]*Post, error) // GetViewsByURIs retrieves full post views (with author + community joins) for a // set of canonical DID-based AT-URIs. Returns a map keyed by URI; missing or // soft-deleted posts are simply absent from the map. Backs social.coves.community.post.get. - GetViewsByURIs(ctx context.Context, uris []string) (map[string]*PostView, error) + // + // viewerDID scopes the admission visibility gate and is REQUIRED: pass the + // authenticated viewer's DID, or "" for an anonymous read. "" is the + // fail-closed value — accepted content only — so an implementation that + // ignores this argument narrows nothing and widens nothing for the public, + // while an author passed here reaches their own pending/rejected/removed + // posts, the same author-self-view contract actor.getPosts and the feeds + // honor (PRD §6.2). + GetViewsByURIs(ctx context.Context, uris []string, viewerDID string) (map[string]*PostView, error) // GetByAuthor retrieves posts authored by a specific user // Supports filtering by post type and community diff --git a/internal/core/posts/service.go b/internal/core/posts/service.go index 3e4db44..3e5127f 100644 --- a/internal/core/posts/service.go +++ b/internal/core/posts/service.go @@ -1295,7 +1295,14 @@ func (s *postService) GetPosts(ctx context.Context, req GetPostsRequest) ([]*Pos for uri := range uniqueSet { unique = append(unique, uri) } - views, err := s.repo.GetViewsByURIs(ctx, unique) + // The viewer scopes the visibility gate. An anonymous permalink read passes + // "" and gets accepted content only; an AUTHOR reading their own + // pending/rejected/removed post gets it, exactly as actor.getPosts, the feeds + // and the getComments thread header already give it to them (PRD §6.2). This + // is the same req.ViewerDID the block filter below runs on — post.get used to + // consult it for blocks and ignore it for admission, which made the permalink + // the one surface that told an author their own post did not exist. + views, err := s.repo.GetViewsByURIs(ctx, unique, req.ViewerDID) if err != nil { return nil, fmt.Errorf("failed to fetch post views: %w", err) } @@ -1306,7 +1313,10 @@ func (s *postService) GetPosts(ctx context.Context, req GetPostsRequest) ([]*Pos // (PRD §3.4/§6.2). The visibility predicate hides a removed post from // GetViewsByURIs exactly as it hides a pending one, so the removal is // recovered here from the admission row rather than from the (absent) view. - removed := s.removedMarkers(ctx, req.URIs, views) + removed, err := s.removedMarkers(ctx, req.URIs, views) + if err != nil { + return nil, err + } results := make([]*PostResult, len(req.URIs)) for i, uri := range req.URIs { switch { @@ -1341,10 +1351,21 @@ func (s *postService) GetPosts(ctx context.Context, req GetPostsRequest) ([]*Pos // // It is a no-op when the admissions store is not wired (minimal setups and unit // tests), leaving every absent URI a plain notFound — the pre-task-7 behavior. -func (s *postService) removedMarkers(ctx context.Context, uris []string, views map[string]*PostView) map[string]string { +// That is a CONFIGURATION fact, known before any lookup runs, and it is the only +// thing that silently degrades to notFound. +// +// A LOOKUP FAILURE IS AN ERROR, NOT A NOTFOUND. Both lookups here used to be +// best-effort: a database blip turned a standing removal into notFoundPost, so +// the same request answered with a different union member depending on the +// health of the database, and a client (or a moderator checking their own +// removal) could not tell "this post was taken down" from "we could not find +// out". post.get answering 5xx is the honest response to "we do not know"; +// silently downgrading the tombstone is not, and it is unfalsifiable from the +// wire. Callers propagate the error. +func (s *postService) removedMarkers(ctx context.Context, uris []string, views map[string]*PostView) (map[string]string, error) { markers := make(map[string]string) if s.admissions == nil { - return markers + return markers, nil } // Collect the absent URIs once (deduped), then resolve their admissions in a @@ -1362,12 +1383,27 @@ func (s *postService) removedMarkers(ctx context.Context, uris []string, views m absent = append(absent, uri) } if len(absent) == 0 { - return markers + return markers, nil } admissionsByURI, err := s.admissions.GetByPostURIs(ctx, absent) if err != nil { - return markers // best-effort: on failure every absent URI stays a plain notFound + return nil, fmt.Errorf("failed to resolve removal state for post.get: %w", err) + } + + // The post rows are fetched in ONE batched round trip. Looping a per-URI + // lookup here was an N+1 on a public endpoint whose URI list the caller + // controls: 25 URIs (MaxGetPostsURIs) meant up to 25 sequential queries per + // request, all of them for URIs the visibility predicate had already refused. + // + // These are RAW rows on purpose — the predicate has already hidden every URI + // in `absent`, so a gated read would return nothing and there would be no + // removal to report. The raw row is used for exactly two facts, both checked + // below and neither of them content: which community owns the post, and + // whether its author withdrew it. + postsByURI, err := s.repo.GetRawIndexedRowsByURIs(ctx, absent) + if err != nil { + return nil, fmt.Errorf("failed to resolve removal state for post.get: %w", err) } for _, uri := range absent { @@ -1375,8 +1411,8 @@ func (s *postService) removedMarkers(ctx context.Context, uris []string, views m // row still stands and its own community — the key the admission is scoped // by — comes straight off it. A URI with no row is genuinely not-indexed // and stays a notFound. - post, err := s.repo.GetByURI(ctx, uri) - if err != nil { + post := postsByURI[uri] + if post == nil { continue } // A soft-deleted post is GONE, not a tombstone: the author withdrew it, so @@ -1387,6 +1423,11 @@ func (s *postService) removedMarkers(ctx context.Context, uris []string, views m } for _, admission := range admissionsByURI[uri] { + // The community half of this comparison is the fork oracle, and it is + // load-bearing: a post can carry a removal from a community that FORKED + // it while its own community has said nothing. Emitting that as a + // tombstone would let any community publish a moderation verdict about + // a post it does not host. if admission.CommunityDID == post.CommunityDID && admission.Status == AdmissionStatusRemoved { code := "" if admission.DecisionCode != nil { @@ -1397,7 +1438,7 @@ func (s *postService) removedMarkers(ctx context.Context, uris []string, views m } } } - return markers + return markers, nil } // applyViewerBlocks rewrites found posts whose author the viewer has blocked into diff --git a/internal/core/posts/service_author_posts_test.go b/internal/core/posts/service_author_posts_test.go index 363ff87..1a63063 100644 --- a/internal/core/posts/service_author_posts_test.go +++ b/internal/core/posts/service_author_posts_test.go @@ -8,20 +8,54 @@ import ( // mockRepository implements Repository for testing type mockRepository struct { getByAuthorFunc func(ctx context.Context, req GetAuthorPostsRequest) ([]*PostView, *string, error) - getViewsByURIsFunc func(ctx context.Context, uris []string) (map[string]*PostView, error) + getViewsByURIsFunc func(ctx context.Context, uris []string, viewerDID string) (map[string]*PostView, error) + + // rawRows backs both raw lookups; rawRowsErr makes them fail, which is how a + // test drives the "we could not find out" path. + rawRows map[string]*Post + rawRowsErr error + + // gotViewsViewerDID records the viewer the service threaded into the + // visibility gate — "" is a real value here (the anonymous read), so the + // separate flag is what distinguishes it from "never called". + gotViewsViewerDID string + getViewsByURIsCalls int + rawBatchCalls int } func (m *mockRepository) Create(ctx context.Context, post *Post) error { return nil } -func (m *mockRepository) GetByURI(ctx context.Context, uri string) (*Post, error) { - return nil, nil +func (m *mockRepository) GetRawIndexedRow(ctx context.Context, uri string) (*Post, error) { + if m.rawRowsErr != nil { + return nil, m.rawRowsErr + } + if p, ok := m.rawRows[uri]; ok { + return p, nil + } + return nil, ErrNotFound +} + +func (m *mockRepository) GetRawIndexedRowsByURIs(ctx context.Context, uris []string) (map[string]*Post, error) { + m.rawBatchCalls++ + if m.rawRowsErr != nil { + return nil, m.rawRowsErr + } + out := make(map[string]*Post, len(uris)) + for _, uri := range uris { + if p, ok := m.rawRows[uri]; ok { + out[uri] = p + } + } + return out, nil } -func (m *mockRepository) GetViewsByURIs(ctx context.Context, uris []string) (map[string]*PostView, error) { +func (m *mockRepository) GetViewsByURIs(ctx context.Context, uris []string, viewerDID string) (map[string]*PostView, error) { + m.getViewsByURIsCalls++ + m.gotViewsViewerDID = viewerDID if m.getViewsByURIsFunc != nil { - return m.getViewsByURIsFunc(ctx, uris) + return m.getViewsByURIsFunc(ctx, uris, viewerDID) } return map[string]*PostView{}, nil } diff --git a/internal/core/posts/service_blob_test.go b/internal/core/posts/service_blob_test.go index 72a4950..05c3bdc 100644 --- a/internal/core/posts/service_blob_test.go +++ b/internal/core/posts/service_blob_test.go @@ -369,7 +369,7 @@ func TestService_AuthorPDSIsHydratedOntoPostViews(t *testing.T) { `, f.community.DID, uri, "bafyblobowner") require.NoError(t, err) - views, err := postgres.NewPostRepository(f.db).GetViewsByURIs(ctx, []string{uri}) + views, err := postgres.NewPostRepository(f.db).GetViewsByURIs(ctx, []string{uri}, "") require.NoError(t, err) require.Contains(t, views, uri) diff --git a/internal/core/posts/service_get_posts_test.go b/internal/core/posts/service_get_posts_test.go index d4d9eaf..2a0221f 100644 --- a/internal/core/posts/service_get_posts_test.go +++ b/internal/core/posts/service_get_posts_test.go @@ -3,7 +3,9 @@ package posts import ( "context" "errors" + "fmt" "testing" + "time" ) // fakeBlockChecker is an in-memory BlockChecker for unit tests. It records the most @@ -192,7 +194,7 @@ func TestGetPosts_OrderingAndNotFound(t *testing.T) { missing := didPostURI("missing1") repo := &mockRepository{ - getViewsByURIsFunc: func(ctx context.Context, uris []string) (map[string]*PostView, error) { + getViewsByURIsFunc: func(ctx context.Context, uris []string, viewerDID string) (map[string]*PostView, error) { // Only the "found" URI exists in the AppView return map[string]*PostView{ found: {URI: found, CID: "cid-found"}, @@ -228,6 +230,50 @@ func TestGetPosts_OrderingAndNotFound(t *testing.T) { } } +// TestGetPosts_ThreadsViewerIntoVisibilityGate pins that post.get asks the repository +// the question the CALLER asked, not an anonymous one. +// +// The service already had req.ViewerDID in hand (it runs the block filter on it) and +// passed "" to the visibility gate anyway, which made post.get the only surface that +// refused an author their own pending post — actor.getPosts, the feeds and the +// getComments thread header all show it. This is the service half of that contract; +// TestPostGetVisibility_AuthorSeesOwnPendingPost is the SQL half. +func TestGetPosts_ThreadsViewerIntoVisibilityGate(t *testing.T) { + uri := didPostURI("v1") + + t.Run("an authenticated viewer reaches the gate", func(t *testing.T) { + repo := &mockRepository{} + s := &postService{repo: repo} + + if _, err := s.GetPosts(context.Background(), GetPostsRequest{ + URIs: []string{uri}, + ViewerDID: "did:plc:theviewer", + }); err != nil { + t.Fatalf("GetPosts returned error: %v", err) + } + if repo.getViewsByURIsCalls != 1 { + t.Fatalf("GetViewsByURIs called %d times, want 1", repo.getViewsByURIsCalls) + } + if repo.gotViewsViewerDID != "did:plc:theviewer" { + t.Errorf("GetViewsByURIs got viewer %q, want %q — post.get must gate on the caller's own viewer, "+ + "or an author is told their own pending post does not exist", + repo.gotViewsViewerDID, "did:plc:theviewer") + } + }) + + t.Run("an anonymous caller is the explicit fail-closed empty viewer", func(t *testing.T) { + repo := &mockRepository{} + s := &postService{repo: repo} + + if _, err := s.GetPosts(context.Background(), GetPostsRequest{URIs: []string{uri}}); err != nil { + t.Fatalf("GetPosts returned error: %v", err) + } + if repo.gotViewsViewerDID != "" { + t.Errorf("GetViewsByURIs got viewer %q, want \"\" for an anonymous read", repo.gotViewsViewerDID) + } + }) +} + // TestGetPosts_ViewerBlocksAuthor verifies that when an authenticated viewer has blocked // a post's author, that post comes back as a blockedPost marker (blockedBy "author") // while posts by unblocked authors and not-found URIs are unaffected, and request order @@ -241,7 +287,7 @@ func TestGetPosts_ViewerBlocksAuthor(t *testing.T) { missingURI := didPostURI("missing1") repo := &mockRepository{ - getViewsByURIsFunc: func(ctx context.Context, uris []string) (map[string]*PostView, error) { + getViewsByURIsFunc: func(ctx context.Context, uris []string, viewerDID string) (map[string]*PostView, error) { return map[string]*PostView{ blockedURI: viewWithAuthor(blockedURI, blockedAuthor), okURI: viewWithAuthor(okURI, okAuthor), @@ -300,7 +346,7 @@ func TestGetPosts_DedupesAuthorDIDsForBlockCheck(t *testing.T) { uri1, uri2 := didPostURI("a"), didPostURI("b") repo := &mockRepository{ - getViewsByURIsFunc: func(ctx context.Context, uris []string) (map[string]*PostView, error) { + getViewsByURIsFunc: func(ctx context.Context, uris []string, viewerDID string) (map[string]*PostView, error) { return map[string]*PostView{ uri1: viewWithAuthor(uri1, author), uri2: viewWithAuthor(uri2, author), @@ -329,7 +375,7 @@ func TestGetPosts_SkipsBlockFilter(t *testing.T) { uri := didPostURI("p1") newRepo := func() *mockRepository { return &mockRepository{ - getViewsByURIsFunc: func(ctx context.Context, uris []string) (map[string]*PostView, error) { + getViewsByURIsFunc: func(ctx context.Context, uris []string, viewerDID string) (map[string]*PostView, error) { return map[string]*PostView{uri: viewWithAuthor(uri, author)}, nil }, } @@ -367,7 +413,7 @@ func TestGetPosts_SkipsBlockFilter(t *testing.T) { func TestGetPosts_BlockCheckErrorFailsClosed(t *testing.T) { uri := didPostURI("p1") repo := &mockRepository{ - getViewsByURIsFunc: func(ctx context.Context, uris []string) (map[string]*PostView, error) { + getViewsByURIsFunc: func(ctx context.Context, uris []string, viewerDID string) (map[string]*PostView, error) { return map[string]*PostView{uri: viewWithAuthor(uri, "did:plc:author")}, nil }, } @@ -380,6 +426,193 @@ func TestGetPosts_BlockCheckErrorFailsClosed(t *testing.T) { } } +// TestGetPosts_RemovedMarkers is the tombstone matrix of post.get's removal path. +// +// A post the visibility predicate hides is a notFoundPost by default; it is upgraded +// to a #removedPost carrying the moderation code ONLY when the post's OWN community +// removed it and its author has not withdrawn it. Three things have to hold at once +// and each has a distinct failure mode: +// +// - the fork oracle: a removal published by a community that merely FORKED the post +// must not become a verdict about the post's own community. Dropping the +// community comparison would let any community publish a takedown notice for a +// post it does not host. +// - the withdrawal rule: a soft-deleted post is GONE, not a tombstone. Emitting a +// moderation reason for a post its own author took down advertises both the +// removal and the post's continued existence. +// - honesty about failure: a lookup that FAILED is not "not removed". Both lookups +// used to be best-effort, so a database blip silently changed which union member +// a standing removal produced — unfalsifiable from the wire, and different on +// every retry. +func TestGetPosts_RemovedMarkers(t *testing.T) { + const ownCommunity = "did:plc:owncommunity" + const forkCommunity = "did:plc:forkcommunity" + uri := didPostURI("rm1") + code := "rule-violation" + + rawRow := func(deleted bool) map[string]*Post { + p := &Post{URI: uri, CommunityDID: ownCommunity} + if deleted { + at := time.Now() + p.DeletedAt = &at + } + return map[string]*Post{uri: p} + } + admission := func(communityDID string, status AdmissionStatus, decisionCode *string) []*Admission { + return []*Admission{{CommunityDID: communityDID, PostURI: uri, Status: status, DecisionCode: decisionCode}} + } + + tests := []struct { + name string + admissions map[string][]*Admission + admissionsErr error + rawRows map[string]*Post + rawRowsErr error + wantErr bool + wantRemoved bool + wantCode string + why string + }{ + { + name: "pending in its own community is a plain notFound", + admissions: map[string][]*Admission{uri: admission(ownCommunity, AdmissionStatusPending, nil)}, + rawRows: rawRow(false), + why: "a post awaiting a decision has no verdict to report; answering #removedPost would invent a " + + "moderation act, and answering anything but notFound tells the public a pending post exists", + }, + { + name: "removed by its OWN community is a tombstone carrying the code", + admissions: map[string][]*Admission{uri: admission(ownCommunity, AdmissionStatusRemoved, &code)}, + rawRows: rawRow(false), + wantRemoved: true, + wantCode: code, + why: "a removed post is a tombstone the author is owed the reason for, not a blank permalink", + }, + { + name: "removed by a DIFFERENT community is NOT a tombstone (the fork oracle)", + admissions: map[string][]*Admission{uri: admission(forkCommunity, AdmissionStatusRemoved, &code)}, + rawRows: rawRow(false), + why: "the removal belongs to a community that FORKED the post, not to the community that hosts it. " + + "Dropping the community comparison lets any community publish a takedown verdict about a post " + + "it does not host — the same fork hazard the read predicate's join key closes", + }, + { + name: "soft-deleted AND removed is a notFound, not a tombstone", + admissions: map[string][]*Admission{uri: admission(ownCommunity, AdmissionStatusRemoved, &code)}, + rawRows: rawRow(true), + why: "the author withdrew the post. Rendering the moderation reason anyway advertises both the " + + "takedown and the fact that the post still exists in the index", + }, + { + name: "an unindexed URI stays a notFound", + admissions: map[string][]*Admission{uri: admission(ownCommunity, AdmissionStatusRemoved, &code)}, + rawRows: map[string]*Post{}, + why: "no indexed row means there is no community to compare the removal against — the admission is " + + "about a post this AppView has never seen", + }, + { + name: "an admission lookup FAILURE is an error, not a notFound", + admissionsErr: errors.New("admissions db down"), + rawRows: rawRow(false), + wantErr: true, + why: "collapsing the failure into notFound makes the answer depend on database health: the same " + + "request returns a different union member on every retry and the client cannot tell", + }, + { + name: "a post-row lookup FAILURE is an error, not a notFound", + admissions: map[string][]*Admission{uri: admission(ownCommunity, AdmissionStatusRemoved, &code)}, + rawRowsErr: errors.New("posts db down"), + wantErr: true, + why: "same reason: 'we could not find out' is not 'it was not removed'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo := &mockRepository{rawRows: tt.rawRows, rawRowsErr: tt.rawRowsErr} + admissions := &fakeAdmissions{ + rec: &engineRecorder{}, + byPostURIs: tt.admissions, + byPostURIsErr: tt.admissionsErr, + } + s := &postService{repo: repo, admissions: admissions} + + results, err := s.GetPosts(context.Background(), GetPostsRequest{URIs: []string{uri}}) + if tt.wantErr { + if err == nil { + t.Fatalf("expected an error, got results %+v — %s", results, tt.why) + } + return + } + if err != nil { + t.Fatalf("GetPosts returned error: %v", err) + } + if len(results) != 1 { + t.Fatalf("expected 1 result, got %d", len(results)) + } + + got := results[0] + if got.Post != nil { + t.Fatalf("a hidden post must never come back as a postView, got %+v", got.Post) + } + if !tt.wantRemoved { + if got.Removed != nil { + t.Fatalf("expected notFound, got removed %+v — %s", got.Removed, tt.why) + } + if got.NotFound == nil || got.NotFound.URI != uri || !got.NotFound.NotFound { + t.Fatalf("expected notFound{%q,true}, got %+v — %s", uri, got.NotFound, tt.why) + } + return + } + if got.NotFound != nil { + t.Fatalf("expected removed, got notFound %+v — %s", got.NotFound, tt.why) + } + if got.Removed == nil || got.Removed.URI != uri || !got.Removed.Removed { + t.Fatalf("expected removed{%q,true}, got %+v — %s", uri, got.Removed, tt.why) + } + if got.Removed.Code != tt.wantCode { + t.Errorf("removal code = %q, want %q — %s", got.Removed.Code, tt.wantCode, tt.why) + } + }) + } +} + +// TestGetPosts_RemovalPathBatchesPostLookups pins that the removal path resolves the +// whole absent set in ONE round trip. +// +// post.get accepts a caller-controlled list of up to MaxGetPostsURIs URIs, and the +// removal path runs over exactly the ones the visibility predicate already refused — +// so a per-URI lookup there is an unauthenticated N+1 whose multiplier the caller +// chooses. The admission lookup was already batched; the post lookup was not. +func TestGetPosts_RemovalPathBatchesPostLookups(t *testing.T) { + uris := make([]string, 0, MaxGetPostsURIs) + for i := 0; i < MaxGetPostsURIs; i++ { + uris = append(uris, didPostURI(fmt.Sprintf("batch%d", i))) + } + + repo := &mockRepository{rawRows: map[string]*Post{}} + admissions := &fakeAdmissions{rec: &engineRecorder{}} + s := &postService{repo: repo, admissions: admissions} + + if _, err := s.GetPosts(context.Background(), GetPostsRequest{URIs: uris}); err != nil { + t.Fatalf("GetPosts returned error: %v", err) + } + if repo.rawBatchCalls != 1 { + t.Errorf("raw post lookup ran %d times for %d absent URIs, want exactly 1 — post.get is public and the "+ + "URI list is caller-controlled, so a per-URI round trip is an N+1 with a caller-chosen multiplier", + repo.rawBatchCalls, len(uris)) + } + admissionLookups := 0 + for _, call := range admissions.rec.calls { + if call == "GetByPostURIs" { + admissionLookups++ + } + } + if admissionLookups != 1 { + t.Errorf("admission lookup ran %d times, want exactly 1", admissionLookups) + } +} + // TestPostResult_Member verifies the union accessor returns exactly the populated member // and reports the empty (invalid) result so callers can avoid emitting a null union entry. func TestPostResult_Member(t *testing.T) { diff --git a/internal/core/unfurl/post_unfurl_integration_test.go b/internal/core/unfurl/post_unfurl_integration_test.go index e911689..c3a8272 100644 --- a/internal/core/unfurl/post_unfurl_integration_test.go +++ b/internal/core/unfurl/post_unfurl_integration_test.go @@ -404,7 +404,7 @@ func TestPostUnfurl_E2E_WithJetstream(t *testing.T) { // Verify post was indexed with unfurl metadata uri := fmt.Sprintf("at://%s/social.coves.community.post/%s", community.DID, rkey) - indexedPost, err := postRepo.GetByURI(ctx, uri) + indexedPost, err := postRepo.GetRawIndexedRow(ctx, uri) require.NoError(t, err, "Post should be indexed") // Verify embed was stored diff --git a/internal/db/postgres/author_avatar_hydration_test.go b/internal/db/postgres/author_avatar_hydration_test.go index abaf03b..11d0a31 100644 --- a/internal/db/postgres/author_avatar_hydration_test.go +++ b/internal/db/postgres/author_avatar_hydration_test.go @@ -63,7 +63,7 @@ func TestAuthorProfileHydration(t *testing.T) { postRepo := postgres.NewPostRepository(db) t.Run("GetViewsByURIs", func(t *testing.T) { - views, err := postRepo.GetViewsByURIs(ctx, []string{postURI}) + views, err := postRepo.GetViewsByURIs(ctx, []string{postURI}, "") require.NoError(t, err) require.Contains(t, views, postURI) assertAuthorHydrated(t, views[postURI].Author, "GetViewsByURIs") @@ -94,7 +94,7 @@ func TestAuthorProfileHydration(t *testing.T) { bareDID := fmt.Sprintf("did:plc:bare%s", testID) bareURI := fixtures.Post(t, db, communityDID, bareDID, "Bare author post", 1, time.Now()) - views, err := postRepo.GetViewsByURIs(ctx, []string{bareURI}) + views, err := postRepo.GetViewsByURIs(ctx, []string{bareURI}, "") require.NoError(t, err) require.Contains(t, views, bareURI) author := views[bareURI].Author diff --git a/internal/db/postgres/comment_repo.go b/internal/db/postgres/comment_repo.go index 3eb4b17..96ae09b 100644 --- a/internal/db/postgres/comment_repo.go +++ b/internal/db/postgres/comment_repo.go @@ -468,14 +468,43 @@ func (r *postgresCommentRepo) ListByCommenterWithCursor(ctx context.Context, req // Build community filter if provided // Parameter numbering: $1=commenterDID, $2=limit+1 (for pagination detection) - // Cursor values (if present) use $3 and $4, community DID comes after + // Cursor values (if present) use $3 and $4, then the community DID and the + // viewer DID the visibility predicate binds. + // + // SECURITY (PRD §6.2 — "comment community filters" are in the must-convert + // inventory): naming a community in this query is a claim about that + // community's scope, so the roots it selects must be the posts that community + // actually ADMITTED. Under author-owned posts anyone can write a postv2 naming + // any community, and the comment consumer indexes a comment whatever its root + // is — so a bare `community_did = $n` subquery lets an attacker comment on + // their own never-accepted post and have the comment served back, in full, + // inside a listing clients render as "this user's comments in ". + // That is the removed write barrier being rebuilt one table over. + // + // The subquery therefore runs the SAME centralized predicate every other posts + // read path runs (visiblePostsJoin) plus the soft-delete gate, with the + // viewer's DID bound so the author keeps the carve-out over their own + // pending/rejected/removed posts and nobody else gains it. Reusing the helper + // rather than hand-rolling the status rule is the point: a second copy of the + // predicate is a second thing to forget to update. var communityFilter string var communityValue []interface{} paramOffset := 2 + len(cursorValues) // Start after $1, $2, and any cursor params if req.CommunityDID != nil && *req.CommunityDID != "" { paramOffset++ - communityFilter = fmt.Sprintf("AND c.root_uri IN (SELECT uri FROM posts WHERE community_did = $%d)", paramOffset) - communityValue = append(communityValue, *req.CommunityDID) + communityParam := paramOffset + paramOffset++ + viewerParam := paramOffset + + visJoin, visWhere := visiblePostsJoin(viewerParam) + communityFilter = fmt.Sprintf(`AND c.root_uri IN ( + SELECT p.uri + FROM posts p%s + WHERE p.community_did = $%d + AND p.deleted_at IS NULL + AND %s + )`, visJoin, communityParam, visWhere) + communityValue = append(communityValue, *req.CommunityDID, req.ViewerDID) } // Build complete query with JOINs and filters diff --git a/internal/db/postgres/comment_repo_visibility_test.go b/internal/db/postgres/comment_repo_visibility_test.go new file mode 100644 index 0000000..a4c51ec --- /dev/null +++ b/internal/db/postgres/comment_repo_visibility_test.go @@ -0,0 +1,333 @@ +//go:build integration + +package postgres + +import ( + "context" + "database/sql" + "testing" + "time" + + "Coves/internal/core/comments" + "Coves/internal/core/posts" + "Coves/tests/testkit" + + _ "github.com/lib/pq" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// actor.getComments' COMMUNITY FILTER is a posts read path wearing a comments +// costume (PRD §6.2 names "comment community filters" in the must-convert +// inventory), and it is the one surface where the removed write barrier can be +// rebuilt one table over. +// +// Under community-owned posts, only a key holder could put content under a +// community's name. Under author-owned posts ANY user writes a postv2 naming ANY +// community, and only the admission row makes it that community's content. The +// comment consumer indexes a comment whatever its root is — it does not validate +// that the root exists, let alone that it was admitted — so an attacker can: +// +// 1. write a postv2 naming community C, which C never accepts (banned author, +// rejected content, quota) and which is therefore correctly hidden in every +// feed, in post.get, and on the profile; +// 2. write a comment rooted at that hidden post; +// 3. call getComments?actor=&community=C. +// +// If the community filter is a bare `root_uri IN (SELECT uri FROM posts WHERE +// community_did = $n)`, step 3 returns the attacker's comment — content, facets, +// embeds, labels, score — inside a listing every client renders as "this user's +// comments in community C". C admitted nothing and is publishing the attacker +// anyway. +// +// So the filter must ask the same question every other posts read path asks: +// visiblePostsJoin plus deleted_at IS NULL, with the VIEWER bound, so that the +// author's own carve-out survives and nobody else's does. +// +// What this suite must NOT change is the UNFILTERED listing: a person's comments +// are their own public speech and stay listed even when the root is hidden — that +// is TestActorCommentsVisibility_RootIsReferenceOnly in post_visibility_test.go, +// and the reference-only response shape is what makes it safe. The community +// filter is different precisely because the filter itself is a claim about the +// community: naming C in the query is asking for C's scope, and C's scope is what +// C admitted. + +// seedActorComment inserts one comment by commenter rooted at rootURI. +func seedActorComment(t *testing.T, db *sql.DB, commenter, rootURI, rkey string, createdAt time.Time) string { + t.Helper() + + uri := "at://" + commenter + "/social.coves.community.comment/" + rkey + _, err := db.ExecContext(context.Background(), ` + INSERT INTO comments (uri, cid, rkey, commenter_did, root_uri, root_cid, parent_uri, parent_cid, content, created_at) + VALUES ($1, $2, $3, $4, $5, $6, $5, $6, $7, $8) + `, uri, "bafycmt"+rkey, rkey, commenter, rootURI, "bafyroot"+rkey, "comment "+rkey, createdAt) + require.NoErrorf(t, err, "seeding comment %s", rkey) + return uri +} + +// seedLegacyPost inserts a DEPRECATED community-repo post +// (social.coves.community.post), whose authority is the COMMUNITY's DID. It +// carries no admission row by construction — it was never routed through the +// admission engine — and visiblePostsJoin grandfathers exactly that shape until +// task 8's drain. The community filter must keep serving comments on these or the +// drain window goes dark. +func seedLegacyPost(t *testing.T, db *sql.DB, communityDID, authorDID, rkey, title string, createdAt time.Time) string { + t.Helper() + + uri := "at://" + communityDID + "/" + posts.LegacyPostCollection + "/" + rkey + _, err := db.ExecContext(context.Background(), ` + INSERT INTO posts (uri, cid, rkey, author_did, community_did, title, created_at, score, upvote_count, downvote_count) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, 0) + `, uri, "bafylegacy"+rkey, rkey, authorDID, communityDID, title, createdAt, 1, 1) + require.NoErrorf(t, err, "seeding legacy post %s", rkey) + return uri +} + +// listActorComments runs the community-filtered profile listing as a given +// viewer and returns the comment URIs it served. +func listActorComments(t *testing.T, repo comments.Repository, commenter, communityDID, viewerDID string) []string { + t.Helper() + + req := comments.ListByCommenterRequest{ + CommenterDID: commenter, + Limit: 50, + ViewerDID: viewerDID, + } + if communityDID != "" { + req.CommunityDID = &communityDID + } + page, _, err := repo.ListByCommenterWithCursor(context.Background(), req) + require.NoError(t, err, "the community-filtered profile listing must not fail") + return commentURIs(page) +} + +// TestActorCommentsCommunityFilter_AdmissionGated is the leak test: the +// attacker's comment on their own never-admitted post must not come back under +// the community's scope. +func TestActorCommentsCommunityFilter_AdmissionGated(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + + community := visibilityCommunity(t, db, "acf") + attacker := "did:plc:visacfattacker" + createTestUser(t, db, "visacfattacker.test", attacker) + + base := time.Date(2026, 2, 1, 0, 0, 0, 0, time.UTC) + + // The four admission states a postv2 can hold in its own community, plus the + // comment each one carries. + accepted := seedVisibilityPost(t, db, community, attacker, "acfacc", "accepted", base.Add(5*time.Hour)) + pending := seedVisibilityPost(t, db, community, attacker, "acfpen", "pending", base.Add(4*time.Hour)) + rejected := seedVisibilityPost(t, db, community, attacker, "acfrej", "rejected", base.Add(3*time.Hour)) + removed := seedVisibilityPost(t, db, community, attacker, "acfrem", "removed", base.Add(2*time.Hour)) + // And a postv2 whose admission row never got seeded at all — the failed-seed + // case visiblePostsJoin fails CLOSED on. + unseeded := seedVisibilityPost(t, db, community, attacker, "acfnos", "no admission row", base.Add(time.Hour)) + + seedVisibilityAdmission(t, db, community, accepted, posts.AdmissionStatusAccepted, "bafypostv2acfacc", "") + seedVisibilityAdmission(t, db, community, pending, posts.AdmissionStatusPending, "", "") + seedVisibilityAdmission(t, db, community, rejected, posts.AdmissionStatusRejected, "", "spam") + seedVisibilityAdmission(t, db, community, removed, posts.AdmissionStatusRemoved, "", "rule-violation") + + onAccepted := seedActorComment(t, db, attacker, accepted, "acfc1", base.Add(5*time.Hour)) + onPending := seedActorComment(t, db, attacker, pending, "acfc2", base.Add(4*time.Hour)) + onRejected := seedActorComment(t, db, attacker, rejected, "acfc3", base.Add(3*time.Hour)) + onRemoved := seedActorComment(t, db, attacker, removed, "acfc4", base.Add(2*time.Hour)) + onUnseeded := seedActorComment(t, db, attacker, unseeded, "acfc5", base.Add(time.Hour)) + + repo := NewCommentRepository(db) + + t.Run("a public caller sees only comments on admitted posts", func(t *testing.T) { + got := listActorComments(t, repo, attacker, community, publicViewer) + + assert.ElementsMatchf(t, []string{onAccepted}, got, + "the community-filtered profile listing leaked comments rooted at posts community %s never admitted. "+ + "An attacker writes a postv2 naming a community, the community rejects or never accepts it, and then "+ + "comments on their own hidden post: this endpoint is where that content re-enters the community's "+ + "scope in full — content, facets, embeds, labels, score — under a heading the client renders as "+ + "'this user's comments in %s'. The filter must run visiblePostsJoin, not a bare community_did match. "+ + "(pending=%s rejected=%s removed=%s no-admission-row=%s)", + community, community, onPending, onRejected, onRemoved, onUnseeded) + }) + + t.Run("a third-party viewer sees the same thing", func(t *testing.T) { + // The author carve-out is keyed to the POST's author. A logged-in stranger + // must not inherit it just by being authenticated. + stranger := "did:plc:visacfstranger" + createTestUser(t, db, "visacfstranger.test", stranger) + + got := listActorComments(t, repo, attacker, community, stranger) + + assert.ElementsMatchf(t, []string{onAccepted}, got, + "an authenticated stranger saw comments on unadmitted posts. The visibility predicate's author branch is "+ + "`p.author_did = $viewer`, so only the POST's own author may see it — being logged in is not the gate") + }) + + t.Run("the post's own author still sees their own unadmitted roots", func(t *testing.T) { + // PRD §6.2: an author sees their own posts in every admission state so a + // client can render "pending review" / "removed" on their own profile. The + // same must hold for their comment history, or the author's own pending + // thread disappears from their own view. + got := listActorComments(t, repo, attacker, community, attacker) + + assert.ElementsMatchf(t, + []string{onAccepted, onPending, onRejected, onRemoved, onUnseeded}, got, + "the author's own community-filtered comment history dropped comments on their own posts. The viewer DID "+ + "must be bound into visiblePostsJoin so the author carve-out survives the filter") + }) + + t.Run("the unfiltered listing is unchanged", func(t *testing.T) { + // The reference-only guarantee (TestActorCommentsVisibility_RootIsReferenceOnly) + // still holds: without a community in the query there is no claim about a + // community's scope, and a person's comments are their own public speech. + got := listActorComments(t, repo, attacker, "", publicViewer) + + assert.ElementsMatchf(t, + []string{onAccepted, onPending, onRejected, onRemoved, onUnseeded}, got, + "the UNFILTERED profile listing must keep serving every comment the actor wrote — the root is carried as a "+ + "reference (uri/cid) only, so a hidden root leaks nothing, and suppressing the comment would delete a "+ + "person's own speech from their own profile") + }) +} + +// TestActorCommentsCommunityFilter_DeletedRoot pins the second half of the fix: +// the old subquery had no deleted_at filter either, so an author who WITHDREW +// their post still had its comment thread listed under the community. +func TestActorCommentsCommunityFilter_DeletedRoot(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + community := visibilityCommunity(t, db, "acd") + actor := "did:plc:visacdactor" + createTestUser(t, db, "visacdactor.test", actor) + + base := time.Date(2026, 2, 1, 0, 0, 0, 0, time.UTC) + live := seedVisibilityPost(t, db, community, actor, "acdliv", "live", base.Add(2*time.Hour)) + withdrawn := seedVisibilityPost(t, db, community, actor, "acddel", "withdrawn", base.Add(time.Hour)) + seedVisibilityAdmission(t, db, community, live, posts.AdmissionStatusAccepted, "bafypostv2acdliv", "") + seedVisibilityAdmission(t, db, community, withdrawn, posts.AdmissionStatusAccepted, "bafypostv2acddel", "") + + _, err := db.ExecContext(ctx, `UPDATE posts SET deleted_at = NOW() WHERE uri = $1`, withdrawn) + require.NoError(t, err) + + onLive := seedActorComment(t, db, actor, live, "acdc1", base.Add(2*time.Hour)) + seedActorComment(t, db, actor, withdrawn, "acdc2", base.Add(time.Hour)) + + repo := NewCommentRepository(db) + + t.Run("a public caller does not reach the withdrawn root's thread", func(t *testing.T) { + got := listActorComments(t, repo, actor, community, publicViewer) + assert.ElementsMatchf(t, []string{onLive}, got, + "a comment rooted at a SOFT-DELETED post was still listed under the community. The author withdrew that "+ + "post; the community filter must exclude deleted roots (deleted_at IS NULL) exactly as every other "+ + "posts read path does") + }) + + t.Run("the author does not reach it either", func(t *testing.T) { + // A soft delete is terminal for display on every path, including the + // author's own — unlike a non-accepted admission, which the author may see. + got := listActorComments(t, repo, actor, community, actor) + assert.ElementsMatchf(t, []string{onLive}, got, + "the author self-view must not resurrect a deleted root: deleted_at is a separate gate from admission "+ + "status and the author carve-out does not cross it") + }) +} + +// TestActorCommentsCommunityFilter_LegacyRootStaysVisible is the drain-window +// pin. A deprecated community-repo post carries no admission row, and +// visiblePostsJoin treats a missing row as VISIBLE for non-postv2 collections +// (§11's gated follow-up retires that branch only once the drain is confirmed). +// A filter that required an accepted row outright would blank every legacy +// thread's comment history on the profile. +func TestActorCommentsCommunityFilter_LegacyRootStaysVisible(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + + community := visibilityCommunity(t, db, "acl") + actor := "did:plc:visaclactor" + createTestUser(t, db, "visaclactor.test", actor) + + base := time.Date(2026, 2, 1, 0, 0, 0, 0, time.UTC) + legacy := seedLegacyPost(t, db, community, actor, "aclleg", "legacy community-repo post", base.Add(time.Hour)) + onLegacy := seedActorComment(t, db, actor, legacy, "aclc1", base.Add(time.Hour)) + + repo := NewCommentRepository(db) + got := listActorComments(t, repo, actor, community, publicViewer) + + assert.ElementsMatchf(t, []string{onLegacy}, got, + "the community filter dropped a comment on a DEPRECATED community-repo post (%s). Those rows never had an "+ + "admission row — they live in the community's own repo and are accepted by construction — so the filter "+ + "must reuse visiblePostsJoin's collection-aware rule rather than demanding an accepted row", legacy) + + t.Run("a legacy root that was MODERATOR-REMOVED is still hidden", func(t *testing.T) { + // applyRemoval has no collection guard, so a legacy row CAN carry a removed + // admission — the leak the getComments header gate was rebuilt to close. + // The community filter inherits the same rule for free by reusing the join. + removedLegacy := seedLegacyPost(t, db, community, actor, "aclrem", "removed legacy post", base) + seedVisibilityAdmission(t, db, community, removedLegacy, posts.AdmissionStatusRemoved, "", "rule-violation") + seedActorComment(t, db, actor, removedLegacy, "aclc2", base) + + stranger := "did:plc:visaclstranger" + createTestUser(t, db, "visaclstranger.test", stranger) + + got := listActorComments(t, repo, actor, community, stranger) + assert.ElementsMatchf(t, []string{onLegacy}, got, + "a comment on a moderator-REMOVED legacy post was listed under the community. Removal is a real admission "+ + "row on a legacy URI, and the collection-aware rule only grandfathers the ABSENCE of a row") + }) +} + +// TestActorCommentsCommunityFilter_PagesWithCursor is the parameter-arithmetic +// pin. ListByCommenterWithCursor builds its bind numbers by hand: $1/$2 fixed, +// two cursor values, then the community DID — and now the viewer DID as well. +// Page two of a community-filtered profile is the only request that binds all six +// at once, and an off-by-one there is not a wrong answer, it is a "there is no +// parameter $6" from Postgres in production only. +func TestActorCommentsCommunityFilter_PagesWithCursor(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + + community := visibilityCommunity(t, db, "acp") + actor := "did:plc:visacpactor" + createTestUser(t, db, "visacpactor.test", actor) + + base := time.Date(2026, 2, 1, 0, 0, 0, 0, time.UTC) + root := seedVisibilityPost(t, db, community, actor, "acproo", "accepted root", base.Add(10*time.Hour)) + seedVisibilityAdmission(t, db, community, root, posts.AdmissionStatusAccepted, "bafypostv2acproo", "") + + hidden := seedVisibilityPost(t, db, community, actor, "acphid", "pending root", base.Add(9*time.Hour)) + seedVisibilityAdmission(t, db, community, hidden, posts.AdmissionStatusPending, "", "") + seedActorComment(t, db, actor, hidden, "acphc", base.Add(9*time.Hour)) + + var want []string + for i, rkey := range []string{"acpp1", "acpp2", "acpp3", "acpp4", "acpp5"} { + want = append(want, seedActorComment(t, db, actor, root, rkey, base.Add(time.Duration(5-i)*time.Hour))) + } + + repo := NewCommentRepository(db) + + var visited []string + req := comments.ListByCommenterRequest{ + CommenterDID: actor, + CommunityDID: &community, + ViewerDID: publicViewer, + Limit: 2, + } + for pages := 0; ; pages++ { + require.Lessf(t, pages, 10, "the cursor stopped advancing: %d pages and counting", pages) + page, next, err := repo.ListByCommenterWithCursor(context.Background(), req) + require.NoError(t, err, + "paging a community-filtered profile failed. The community filter, the cursor filter and the viewer DID "+ + "each compute their own bind numbers, and page two is the only request that binds all of them") + visited = append(visited, commentURIs(page)...) + if next == nil { + break + } + req.Cursor = next + } + + assert.Equalf(t, want, visited, + "paging a community-filtered profile must visit every comment on an admitted root exactly once, newest first, "+ + "and never surface the one rooted at a pending post") +} diff --git a/internal/db/postgres/community_repo.go b/internal/db/postgres/community_repo.go index 6b1a12c..a2494b1 100644 --- a/internal/db/postgres/community_repo.go +++ b/internal/db/postgres/community_repo.go @@ -12,6 +12,25 @@ import ( "github.com/lib/pq" ) +// The served `postCount` is a LIVE count over the read-path visibility +// predicate, not the stored `communities.post_count` column. +// +// The column is a leftover of the community-repo write path: its only +// incrementer (IncrementPostCount, community_repo_memberships.go) lost its +// caller when posts became author-owned, so every community served postCount 0 +// and `sort=active` ordered by a uniformly zero key. Rehabilitating a stored +// counter means advancing it on the accept transition and decrementing it on +// removal, re-acceptance drift, rejection and author tombstone — five chances to +// disagree with what the feed will render. The subquery cannot disagree, because +// it runs the same predicate the feed runs. See visiblePostCountSubquery. +// +// Two spellings only because the queries differ in whether `communities` carries +// an alias. Both come from the one predicate builder. +var ( + communityPostCountUnqualified = visiblePostCountSubquery("communities.did") + communityPostCountAliasedC = visiblePostCountSubquery("c.did") +) + type postgresCommunityRepo struct { db *sql.DB } @@ -135,7 +154,7 @@ func (r *postgresCommunityRepo) GetByDID(ctx context.Context, did string) (*comm END as pds_refresh_token, pds_url, visibility, allow_external_discovery, moderation_type, content_warnings, - member_count, subscriber_count, post_count, + member_count, subscriber_count, ` + communityPostCountUnqualified + ` AS post_count, federated_from, federated_id, created_at, updated_at, record_uri, record_cid FROM communities @@ -202,7 +221,7 @@ func (r *postgresCommunityRepo) GetByHandle(ctx context.Context, handle string) SELECT id, did, handle, name, display_name, description, description_facets, avatar_cid, banner_cid, owner_did, created_by_did, hosted_by_did, visibility, allow_external_discovery, moderation_type, content_warnings, - member_count, subscriber_count, post_count, + member_count, subscriber_count, ` + communityPostCountUnqualified + ` AS post_count, federated_from, federated_id, created_at, updated_at, record_uri, record_cid FROM communities @@ -390,8 +409,15 @@ func (r *postgresCommunityRepo) List(ctx context.Context, req communities.ListCo sortColumn = "c.subscriber_count" sortOrder = "DESC" case "active": - // Most posts/activity - sortColumn = "c.post_count" + // Most posts/activity. This is the OUTPUT column named post_count — the + // live visibility-gated subquery aliased in the SELECT list below — not + // the stored c.post_count it replaced, which is uniformly zero and made + // this sort a no-op. PostgreSQL resolves a bare name in ORDER BY to an + // output column in preference to an input column, and the SELECT list + // emits exactly one column by this name, so the binding is unambiguous; + // it is spelled without the `c.` qualifier precisely because a qualified + // name would bind to the stale table column instead. + sortColumn = "post_count" sortOrder = "DESC" case "new": // Recently created @@ -412,7 +438,7 @@ func (r *postgresCommunityRepo) List(ctx context.Context, req communities.ListCo SELECT c.id, c.did, c.handle, c.name, c.display_name, c.description, c.description_facets, c.avatar_cid, c.banner_cid, c.owner_did, c.created_by_did, c.hosted_by_did, c.visibility, c.allow_external_discovery, c.moderation_type, c.content_warnings, - c.member_count, c.subscriber_count, c.post_count, + c.member_count, c.subscriber_count, `+communityPostCountAliasedC+` AS post_count, c.federated_from, c.federated_id, c.created_at, c.updated_at, c.record_uri, c.record_cid, c.pds_url FROM communities c @@ -522,7 +548,7 @@ func (r *postgresCommunityRepo) Search(ctx context.Context, req communities.Sear SELECT id, did, handle, name, display_name, description, description_facets, avatar_cid, banner_cid, owner_did, created_by_did, hosted_by_did, visibility, allow_external_discovery, moderation_type, content_warnings, - member_count, subscriber_count, post_count, + member_count, subscriber_count, ` + communityPostCountUnqualified + ` AS post_count, federated_from, federated_id, created_at, updated_at, record_uri, record_cid, pds_url, similarity(name, $1) + similarity(COALESCE(description, ''), $1) as relevance diff --git a/internal/db/postgres/community_repo_list_test.go b/internal/db/postgres/community_repo_list_test.go index 36963e7..6abb7f9 100644 --- a/internal/db/postgres/community_repo_list_test.go +++ b/internal/db/postgres/community_repo_list_test.go @@ -4,6 +4,7 @@ package postgres import ( "Coves/internal/core/communities" + "Coves/internal/core/posts" "Coves/tests/testkit" "context" "database/sql" @@ -35,7 +36,15 @@ import ( // It writes through the repository rather than raw SQL so the row goes in the // same way the consumer puts it there, and so a schema change breaks this in the // same place it breaks production. -func seedListableCommunity(t *testing.T, repo communities.Repository, name, visibility string, subscribers, posts int, createdAt time.Time) *communities.Community { +// +// `posts` seeds that many REAL, publicly visible posts — accepted postv2 rows — +// rather than a number in the community's post_count column. The served +// postCount, and the `sort=active` key with it, is a live count over the +// read-path visibility predicate; the stored column is vestigial and is +// deliberately left at 0 here, so a fixture that only wrote the column would +// rank every community equal and this file's sort assertions would stop meaning +// anything. +func seedListableCommunity(t *testing.T, db *sql.DB, repo communities.Repository, name, visibility string, subscribers, posts int, createdAt time.Time) *communities.Community { t.Helper() community := &communities.Community{ @@ -49,7 +58,9 @@ func seedListableCommunity(t *testing.T, repo communities.Repository, name, visi Visibility: visibility, AllowExternalDiscovery: true, SubscriberCount: subscribers, - PostCount: posts, + // Deliberately NOT `posts`: nothing serves this column any more, and + // leaving it at zero is what proves the sort reads the live count. + PostCount: 0, CreatedAt: createdAt, UpdatedAt: createdAt, RecordURI: "at://did:plc:list" + name + "/social.coves.community.profile/self", @@ -57,9 +68,40 @@ func seedListableCommunity(t *testing.T, repo communities.Repository, name, visi stored, err := repo.Create(context.Background(), community) require.NoErrorf(t, err, "seeding community %s", name) + seedVisibleCommunityPosts(t, db, stored.DID, name, posts, createdAt) + stored.PostCount = posts return stored } +// seedVisibleCommunityPosts inserts `count` accepted, publicly visible postv2 +// rows for a community: a content row plus the acceptance that pins its exact +// CID, which is what the read-path predicate requires before it will render (or +// count) anything. +func seedVisibleCommunityPosts(t *testing.T, db *sql.DB, communityDID, label string, count int, createdAt time.Time) { + t.Helper() + if count == 0 { + return + } + + authorDID := "did:plc:lister" + label + ctx := context.Background() + _, err := db.ExecContext(ctx, ` + INSERT INTO posts (uri, cid, rkey, author_did, community_did, title, created_at, score, upvote_count, downvote_count) + SELECT 'at://' || $1 || '/' || $2 || '/' || $3 || i, + 'bafy' || $3 || i, $3 || i, $1, $4, 'seeded post ' || i, $5, 0, 0, 0 + FROM generate_series(1, $6) AS i + `, authorDID, posts.PostV2Collection, label, communityDID, createdAt, count) + require.NoErrorf(t, err, "seeding %d visible posts for %s", count, label) + + _, err = db.ExecContext(ctx, ` + INSERT INTO community_post_admissions (community_did, post_uri, status, accepted_cid, evaluated_cid, created_at, updated_at) + SELECT $1, p.uri, 'accepted', p.cid, p.cid, NOW(), NOW() + FROM posts p WHERE p.community_did = $1 + ON CONFLICT (community_did, post_uri) DO NOTHING + `, communityDID) + require.NoErrorf(t, err, "accepting the seeded posts for %s", label) +} + // namesOf renders a listing's names in order, which is the whole assertion for // a sort test and the only readable form for its failure message. func namesOf(listed []*communities.Community) []string { @@ -87,9 +129,9 @@ func seedSortFixture(t *testing.T, db *sql.DB) communities.Repository { repo := NewCommunityRepository(db) base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) - seedListableCommunity(t, repo, "alpha", "public", 1, 30, base) - seedListableCommunity(t, repo, "bravo", "public", 30, 1, base.Add(time.Hour)) - seedListableCommunity(t, repo, "charlie", "public", 10, 10, base.Add(2*time.Hour)) + seedListableCommunity(t, db, repo, "alpha", "public", 1, 30, base) + seedListableCommunity(t, db, repo, "bravo", "public", 30, 1, base.Add(time.Hour)) + seedListableCommunity(t, db, repo, "charlie", "public", 10, 10, base.Add(2*time.Hour)) return repo } @@ -155,8 +197,8 @@ func TestCommunityRepo_ListVisibilityFilter(t *testing.T) { repo := NewCommunityRepository(db) base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) - public := seedListableCommunity(t, repo, "openone", "public", 5, 5, base) - unlisted := seedListableCommunity(t, repo, "quietone", "unlisted", 5, 5, base) + public := seedListableCommunity(t, db, repo, "openone", "public", 5, 5, base) + unlisted := seedListableCommunity(t, db, repo, "quietone", "unlisted", 5, 5, base) ctx := context.Background() diff --git a/internal/db/postgres/community_repo_memberships.go b/internal/db/postgres/community_repo_memberships.go index 7a2f21f..f01e761 100644 --- a/internal/db/postgres/community_repo_memberships.go +++ b/internal/db/postgres/community_repo_memberships.go @@ -269,6 +269,18 @@ func (r *postgresCommunityRepo) DecrementSubscriberCount(ctx context.Context, co return nil } +// IncrementPostCount advances the STORED communities.post_count column. +// +// VESTIGIAL, AND NOTHING READS WHAT IT WRITES. It has had no production caller +// since posts became author-owned (the community-repo write path that called it +// is gone), and the served `postCount` is now a live visibility-gated subquery +// rather than this column — see communityPostCountUnqualified in +// community_repo.go. Calling this therefore has no observable effect on any API +// response, which is exactly the trap worth naming: wiring it up to "fix the +// counter" would advance a column no reader consults, and would look like it had +// worked. Removing the column, this method and its interface entry is filed in +// PRD_AUTHOR_OWNED_POSTS.md §12; it is left standing only because deleting it is +// a schema + interface change with its own blast radius. func (r *postgresCommunityRepo) IncrementPostCount(ctx context.Context, communityDID string) error { query := `UPDATE communities SET post_count = post_count + 1 WHERE did = $1` _, err := r.db.ExecContext(ctx, query, communityDID) diff --git a/internal/db/postgres/community_repo_memberships_test.go b/internal/db/postgres/community_repo_memberships_test.go index 39eb008..b5b539d 100644 --- a/internal/db/postgres/community_repo_memberships_test.go +++ b/internal/db/postgres/community_repo_memberships_test.go @@ -4,6 +4,7 @@ package postgres import ( "context" + "database/sql" "testing" "time" @@ -43,7 +44,18 @@ import ( // test names only the thing it is about. func memberOf(t *testing.T) (communities.Repository, *communities.Community) { t.Helper() - repo := NewCommunityRepository(testkit.DB(t)) + repo, _, community := memberOfWithDB(t) + return repo, community +} + +// memberOfWithDB is memberOf, additionally handing back the pool. The stored +// `communities.post_count` column is no longer SERVED — GetByDID computes +// postCount live from the visibility predicate — so a test about the stored +// counter has to read the column rather than the API value. +func memberOfWithDB(t *testing.T) (communities.Repository, *sql.DB, *communities.Community) { + t.Helper() + db := testkit.DB(t) + repo := NewCommunityRepository(db) id := testkit.UniqueID(t) community, err := repo.Create(context.Background(), &communities.Community{ DID: "did:plc:mem" + id, @@ -57,7 +69,17 @@ func memberOf(t *testing.T) (communities.Repository, *communities.Community) { UpdatedAt: time.Now(), }) require.NoError(t, err, "seeding the community the memberships hang off") - return repo, community + return repo, db, community +} + +// storedPostCount reads the raw communities.post_count column — the vestigial +// stored counter IncrementPostCount advances, which nothing serves any more. +func storedPostCount(t *testing.T, db *sql.DB, communityDID string) int { + t.Helper() + var count int + require.NoError(t, db.QueryRowContext(context.Background(), + `SELECT post_count FROM communities WHERE did = $1`, communityDID).Scan(&count)) + return count } func aMembership(userDID, communityDID string) *communities.Membership { @@ -480,13 +502,14 @@ func TestCommunityRepo_Counters(t *testing.T) { t.Run("each counter moves only its own column", func(t *testing.T) { t.Parallel() - repo, community := memberOf(t) + repo, db, community := memberOfWithDB(t) require.NoError(t, repo.IncrementMemberCount(ctx, community.DID)) members, subscribers, posts := countsOf(t, repo, community.DID) assert.Equal(t, 1, members) assert.Zero(t, subscribers, "incrementing members moved the subscriber count") assert.Zero(t, posts) + assert.Zero(t, storedPostCount(t, db, community.DID), "incrementing members moved the post count column") require.NoError(t, repo.IncrementSubscriberCount(ctx, community.DID)) require.NoError(t, repo.IncrementSubscriberCount(ctx, community.DID)) @@ -494,7 +517,18 @@ func TestCommunityRepo_Counters(t *testing.T) { members, subscribers, posts = countsOf(t, repo, community.DID) assert.Equal(t, 1, members) assert.Equal(t, 2, subscribers) - assert.Equal(t, 1, posts) + assert.Equal(t, 1, storedPostCount(t, db, community.DID), + "IncrementPostCount must still move its own column and only its own column") + + // The SERVED postCount is not that column. It is a live count over the + // read-path visibility predicate, and this community has no posts — so + // advancing the stored counter changes nothing a client can see. That + // asymmetry is the point: the stored column is vestigial (PRD §12), and + // a reader who assumes wiring the incrementer would fix postCount needs + // to meet this assertion rather than discover it in production. + assert.Zerof(t, posts, + "the SERVED postCount followed the stored column. It must be the live visibility-gated count — this "+ + "community has zero posts, so the only honest answer is 0 no matter what IncrementPostCount did") }) t.Run("decrements come back down", func(t *testing.T) { @@ -535,11 +569,10 @@ func TestCommunityRepo_Counters(t *testing.T) { // on the record rather than something a reader has to notice. t.Run("the post count only goes up", func(t *testing.T) { t.Parallel() - repo, community := memberOf(t) + repo, db, community := memberOfWithDB(t) require.NoError(t, repo.IncrementPostCount(ctx, community.DID)) - _, _, posts := countsOf(t, repo, community.DID) - assert.Equal(t, 1, posts) + assert.Equal(t, 1, storedPostCount(t, db, community.DID)) assert.NotImplements(t, (*interface { DecrementPostCount(context.Context, string) error })(nil), repo, diff --git a/internal/db/postgres/concurrent_writes_test.go b/internal/db/postgres/concurrent_writes_test.go index e4b11c0..63b35f2 100644 --- a/internal/db/postgres/concurrent_writes_test.go +++ b/internal/db/postgres/concurrent_writes_test.go @@ -140,7 +140,7 @@ func TestConcurrentVoting_MultipleUsersOnSamePost(t *testing.T) { t.Errorf("Expected no errors during concurrent voting, got %d errors", errorCount) } - post, err := postRepo.GetByURI(ctx, postURI) + post, err := postRepo.GetRawIndexedRow(ctx, postURI) if err != nil { t.Fatalf("Failed to get post: %v", err) } @@ -247,7 +247,7 @@ func TestConcurrentVoting_MultipleUsersOnSamePost(t *testing.T) { t.Errorf("Expected no errors during concurrent voting, got %d errors", errorCount) } - post, err := postRepo.GetByURI(ctx, testPost2URI) + post, err := postRepo.GetRawIndexedRow(ctx, testPost2URI) if err != nil { t.Fatalf("Failed to get post: %v", err) } @@ -378,7 +378,7 @@ func TestConcurrentCommenting_MultipleUsersOnSamePost(t *testing.T) { t.Errorf("Expected no errors during concurrent commenting, got %d errors", errorCount) } - post, err := postRepo.GetByURI(ctx, postURI) + post, err := postRepo.GetRawIndexedRow(ctx, postURI) if err != nil { t.Fatalf("Failed to get post: %v", err) } diff --git a/internal/db/postgres/post_repo.go b/internal/db/postgres/post_repo.go index 2e0d5a2..c9d2847 100644 --- a/internal/db/postgres/post_repo.go +++ b/internal/db/postgres/post_repo.go @@ -23,7 +23,9 @@ import ( // is what says which one a row came from. const legacyPostCollection = posts.LegacyPostCollection -type postgresPostRepo struct { +// PostRepository is the PostgreSQL posts read/write surface. It is EXPORTED +// because it carries more than posts.Repository does — see NewPostRepository. +type PostRepository struct { db *sql.DB } @@ -54,14 +56,26 @@ const postViewSelectColumns = ` p.upvote_count + p.bridged_upvote_count AS upvote_count, p.downvote_count + p.bridged_downvote_count AS downvote_count, p.score, p.comment_count, a.status AS admission_status, a.acceptance_uri AS admission_acceptance_uri` -// NewPostRepository creates a new PostgreSQL post repository -func NewPostRepository(db *sql.DB) posts.Repository { - return &postgresPostRepo{db: db} +// NewPostRepository creates a new PostgreSQL post repository. +// +// It returns the CONCRETE type, not posts.Repository, and that is load-bearing +// rather than stylistic: VisibleHeaderView — the viewer-aware, admission-aware +// header lookup the comment thread endpoint must go through — is not on +// posts.Repository, so a constructor returning the interface would erase it and +// leave callers that need the gated read unable to reach it. Callers that only +// want the interface still get it by assignment; the ones that need the gate get +// a compile error instead of a silent downgrade. +func NewPostRepository(db *sql.DB) *PostRepository { + return &PostRepository{db: db} } +// Compile-time proof that the concrete repository still satisfies the interface +// every consumer other than the comment header gate depends on. +var _ posts.Repository = (*PostRepository)(nil) + // Create inserts a new post into the posts table // Called by Jetstream consumer after post is created on PDS -func (r *postgresPostRepo) Create(ctx context.Context, post *posts.Post) error { +func (r *PostRepository) Create(ctx context.Context, post *posts.Post) error { // Serialize JSON fields for storage var facetsJSON, embedJSON sql.NullString @@ -124,43 +138,118 @@ func (r *postgresPostRepo) Create(ctx context.Context, post *posts.Post) error { return nil } -// GetByURI retrieves a post by its AT-URI -// Used for E2E test verification and future GET endpoint +// rawIndexedRowColumns is the ordered SELECT list for an UNGATED raw post row, +// shared by GetRawIndexedRow and GetRawIndexedRowsByURIs so the two cannot drift +// apart. It must stay byte-aligned with scanRawIndexedRow's positional Scan. +const rawIndexedRowColumns = ` + id, uri, cid, rkey, author_did, community_did, + title, content, content_facets, embed, content_labels, + created_at, edited_at, indexed_at, deleted_at, + upvote_count + bridged_upvote_count AS upvote_count, downvote_count + bridged_downvote_count AS downvote_count, score, comment_count` + +// ════════════════════════════════════════════════════════════════════════════ +// DANGER — GetRawIndexedRow IS NOT A DISPLAY READ. // -// KNOWN DEFECT (issue 2026-07-29-deleted-posts-still-served-by-getcomments.md): this is the one post read path with no -// `deleted_at IS NULL` predicate, so -// it serves a soft-deleted post in full — title, body, facets and all. It is not a dead -// path: comments.GetComments calls it to build the thread's post header and never inspects -// DeletedAt, so social.coves.community.comment.getComments still returns the whole of a -// withdrawn post to an anonymous caller. Same shape as the comment-thread hole found in -// task 12, one table over. (see TestPostRepo_SoftDelete) -func (r *postgresPostRepo) GetByURI(ctx context.Context, uri string) (*posts.Post, error) { - query := ` - SELECT - id, uri, cid, rkey, author_did, community_did, - title, content, content_facets, embed, content_labels, - created_at, edited_at, indexed_at, deleted_at, - upvote_count + bridged_upvote_count AS upvote_count, downvote_count + bridged_downvote_count AS downvote_count, score, comment_count +// It is the ONLY post read in this file that applies NEITHER the admission +// visibility predicate NOR `deleted_at IS NULL`. It returns the full title and +// content of a post that is pending, rejected, moderator-removed, or +// soft-deleted by its own author. +// +// Misuse is SILENT. Unlike every gated query here it selects bare columns with +// no `a.` reference and no join, so a caller that reaches for it by mistake gets +// no compile error and no runtime error — just a hidden post's content on the +// wire. That is why it is named for the row it returns rather than for the +// lookup a reader would naturally ask for. +// +// If a reader will SEE the result, call one of these instead: +// - GetViewsByURIs(ctx, uris, viewerDID) — batch, hydrated, gated +// - VisibleHeaderView(ctx, uri, viewerDID) — single post, hydrated, gated +// +// Legitimate callers are the ones that must see the row regardless of who may +// look at it: the admission decider (it DECIDES visibility, so it cannot depend +// on it), the ingestion/consumer paths, and post.get's removal-tombstone path, +// which re-checks deleted_at itself before emitting anything. +// ════════════════════════════════════════════════════════════════════════════ +func (r *PostRepository) GetRawIndexedRow(ctx context.Context, uri string) (*posts.Post, error) { + query := `SELECT` + rawIndexedRowColumns + ` FROM posts - WHERE uri = $1 - ` + WHERE uri = $1` + post, err := scanRawIndexedRow(r.db.QueryRowContext(ctx, query, uri)) + if errors.Is(err, sql.ErrNoRows) { + return nil, posts.ErrNotFound + } + if err != nil { + return nil, fmt.Errorf("failed to get post by URI: %w", err) + } + return post, nil +} + +// GetRawIndexedRowsByURIs is the batched GetRawIndexedRow. THE SAME DANGER +// APPLIES — read the banner above before calling it. +// +// URIs with no indexed row are absent from the returned map; that is not an +// error, it is the answer ("this URI is not indexed here"), and it is what lets +// the caller tell a genuine lookup FAILURE (a returned error) apart from a URI +// the AppView has never seen. +func (r *PostRepository) GetRawIndexedRowsByURIs(ctx context.Context, uris []string) (map[string]*posts.Post, error) { + result := make(map[string]*posts.Post, len(uris)) + if len(uris) == 0 { + return result, nil + } + + // Bound through a single array parameter (= ANY($1)) rather than an + // interpolated IN list, so the SQL stays fully parameterized and the plan is + // cached regardless of batch size. + query := `SELECT` + rawIndexedRowColumns + ` + FROM posts + WHERE uri = ANY($1)` + + rows, err := r.db.QueryContext(ctx, query, pq.Array(uris)) + if err != nil { + return nil, fmt.Errorf("failed to query raw post rows by URIs: %w", err) + } + defer func() { + if err := rows.Close(); err != nil { + slog.Warn("failed to close rows", "error", err) + } + }() + + for rows.Next() { + post, err := scanRawIndexedRow(rows) + if err != nil { + return nil, fmt.Errorf("failed to scan raw post row: %w", err) + } + result[post.URI] = post + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("error iterating raw post rows: %w", err) + } + + return result, nil +} + +// rowScanner is the Scan surface shared by *sql.Row and *sql.Rows, so one scan +// body serves both the single-row and batched raw reads. +type rowScanner interface { + Scan(dest ...interface{}) error +} + +// scanRawIndexedRow scans one rawIndexedRowColumns row into a posts.Post. The +// Scan order below MUST stay byte-aligned with that column list. +func scanRawIndexedRow(row rowScanner) (*posts.Post, error) { var post posts.Post var facetsJSON, embedJSON, labelsJSON sql.NullString - err := r.db.QueryRowContext(ctx, query, uri).Scan( + err := row.Scan( &post.ID, &post.URI, &post.CID, &post.RKey, &post.AuthorDID, &post.CommunityDID, &post.Title, &post.Content, &facetsJSON, &embedJSON, &labelsJSON, &post.CreatedAt, &post.EditedAt, &post.IndexedAt, &post.DeletedAt, &post.UpvoteCount, &post.DownvoteCount, &post.Score, &post.CommentCount, ) - - if errors.Is(err, sql.ErrNoRows) { - return nil, posts.ErrNotFound - } if err != nil { - return nil, fmt.Errorf("failed to get post by URI: %w", err) + return nil, err } // Convert SQL types back to Go types @@ -178,13 +267,15 @@ func (r *postgresPostRepo) GetByURI(ctx context.Context, uri string) (*posts.Pos return &post, nil } -// GetViewsByURIs retrieves full post views for a set of canonical (DID-based) AT-URIs. -// Returns a map keyed by URI; URIs that are missing or soft-deleted are simply absent -// from the map (the caller emits notFoundPost markers for those). +// GetViewsByURIs retrieves full post views for a set of canonical (DID-based) AT-URIs +// VISIBLE to viewerDID. +// Returns a map keyed by URI; URIs that are missing, soft-deleted or hidden from this +// viewer are simply absent from the map (the caller emits notFoundPost markers for +// those, or upgrades a standing removal to a #removedPost tombstone). // Row scanning goes through the shared scanPostView, whose Scan order is kept aligned // with the single source of truth for the SELECT list, postViewSelectColumns. // Backs the social.coves.community.post.get endpoint (feed hydration + permalinks). -func (r *postgresPostRepo) GetViewsByURIs(ctx context.Context, uris []string) (map[string]*posts.PostView, error) { +func (r *PostRepository) GetViewsByURIs(ctx context.Context, uris []string, viewerDID string) (map[string]*posts.PostView, error) { result := make(map[string]*posts.PostView, len(uris)) if len(uris) == 0 { return result, nil @@ -194,13 +285,16 @@ func (r *postgresPostRepo) GetViewsByURIs(ctx context.Context, uris []string) (m // than an interpolated IN list, so the SQL stays fully parameterized and the // plan is cached regardless of batch size. // - // post.get is the public permalink surface, so the visibility gate runs with - // an ANONYMOUS viewer ($2 = ""): accepted posts (and legacy/bridged rows) - // only. A pending/rejected/removed post is absent from the result, which the + // The visibility gate runs with the CALLER's viewer bound to $2. An anonymous + // permalink read passes "" and sees accepted posts (plus legacy/bridged rows) + // only; a pending/rejected/removed post is absent from the result, which the // service renders as notFoundPost — or, for a removal, upgrades to a - // #removedPost tombstone from the admission row. An author's privileged view - // of their own pending posts is served by actor.getPosts (GetByAuthor), which - // threads a real viewer DID. + // #removedPost tombstone from the admission row. An AUTHOR passing their own + // DID additionally reaches their own non-accepted posts, which is what keeps + // post.get agreeing with actor.getPosts, the feeds and the getComments thread + // header about a post the author can see (PRD §6.2). "" is fail-closed, so a + // caller that has no viewer to offer degrades to the public answer rather + // than to an ungated one. visJoin, visWhere := visiblePostsJoin(2) query := ` SELECT` + postViewSelectColumns + ` @@ -209,7 +303,7 @@ func (r *postgresPostRepo) GetViewsByURIs(ctx context.Context, uris []string) (m INNER JOIN communities c ON p.community_did = c.did` + visJoin + ` WHERE p.uri = ANY($1) AND p.deleted_at IS NULL AND ` + visWhere - rows, err := r.db.QueryContext(ctx, query, pq.Array(uris), "") + rows, err := r.db.QueryContext(ctx, query, pq.Array(uris), viewerDID) if err != nil { return nil, fmt.Errorf("failed to query posts by URIs: %w", err) } @@ -238,21 +332,21 @@ func (r *postgresPostRepo) GetViewsByURIs(ctx context.Context, uris []string) (m // to viewerDID under the read-path predicate, and nil when it is hidden (or does // not exist). // -// It is the viewer-aware companion to GetViewsByURIs, which GetViewsByURIs itself -// cannot be because posts.Repository's signature is frozen at two arguments -// (three in-suite fakes implement it). getComments needs both halves the -// anonymous batch fetch cannot give it — a VIEWER (so an author reaches their own -// pending post's thread header) and the HYDRATED view (so the served header -// carries status/acceptanceUri, PRD §6.2). It is deliberately NOT on the -// Repository interface: the comment service reaches it by an optional type -// assertion, so a unit-test fake without it degrades gracefully rather than -// forcing every fake to grow a method. +// It is the SINGLE-post companion to GetViewsByURIs — same predicate, same +// viewer threading, one URI and one row instead of a batch and a map. getComments +// needs both halves it gives — a VIEWER (so an author reaches their own pending +// post's thread header) and the HYDRATED view (so the served header carries +// status/acceptanceUri, PRD §6.2). It is not on posts.Repository; the comment +// service names it in comments.PostReader instead, so a repository that lacks it +// fails to compile at the wiring site. That is deliberate: the earlier optional +// type assertion here failed OPEN, and any future decorator satisfying only +// posts.Repository would have silently disabled the gate. // // It runs the SAME predicate as every other display query — visiblePostsJoin, // admission status + pinned-CID + collection fail-closed + author self-view — and // the same deleted_at filter and scanPostView hydration, so the thread header can // never diverge from post.get or the feeds on what is visible. -func (r *postgresPostRepo) VisibleHeaderView(ctx context.Context, uri, viewerDID string) (*posts.PostView, error) { +func (r *PostRepository) VisibleHeaderView(ctx context.Context, uri, viewerDID string) (*posts.PostView, error) { visJoin, visWhere := visiblePostsJoin(2) query := ` SELECT` + postViewSelectColumns + ` @@ -289,7 +383,7 @@ func (r *postgresPostRepo) VisibleHeaderView(ctx context.Context, uri, viewerDID // Supports filter options: posts_with_replies (default), posts_no_replies, posts_with_media // Uses cursor-based pagination with created_at + uri for stable ordering // Returns []*PostView, next cursor, and error -func (r *postgresPostRepo) GetByAuthor(ctx context.Context, req posts.GetAuthorPostsRequest) ([]*posts.PostView, *string, error) { +func (r *PostRepository) GetByAuthor(ctx context.Context, req posts.GetAuthorPostsRequest) ([]*posts.PostView, *string, error) { // Build WHERE clauses based on filters whereConditions := []string{ "p.author_did = $1", @@ -408,7 +502,7 @@ func (r *postgresPostRepo) GetByAuthor(ctx context.Context, req posts.GetAuthorP // Uses simple | delimiter since this is an internal cursor (not signed like feed cursors) // Returns filter clause, arguments, and error. Error is returned for malformed cursors // to provide clear feedback rather than silently returning the first page. -func (r *postgresPostRepo) parseAuthorPostsCursor(cursor *string, paramOffset int) (string, []interface{}, error) { +func (r *PostRepository) parseAuthorPostsCursor(cursor *string, paramOffset int) (string, []interface{}, error) { if cursor == nil || *cursor == "" { return "", nil, nil } @@ -453,7 +547,7 @@ func (r *postgresPostRepo) parseAuthorPostsCursor(cursor *string, paramOffset in // buildAuthorPostsCursor creates pagination cursor from last post // Cursor format: base64(created_at|uri) -func (r *postgresPostRepo) buildAuthorPostsCursor(post *posts.PostView) string { +func (r *PostRepository) buildAuthorPostsCursor(post *posts.PostView) string { cursorStr := fmt.Sprintf("%s|%s", post.CreatedAt.Format(time.RFC3339Nano), post.URI) return base64.URLEncoding.EncodeToString([]byte(cursorStr)) } @@ -461,7 +555,7 @@ func (r *postgresPostRepo) buildAuthorPostsCursor(post *posts.PostView) string { // SoftDelete marks a post as deleted by setting deleted_at // Called by Jetstream consumer after post is deleted from PDS // Idempotent: Returns success if post already deleted or doesn't exist -func (r *postgresPostRepo) SoftDelete(ctx context.Context, uri string) error { +func (r *PostRepository) SoftDelete(ctx context.Context, uri string) error { query := ` UPDATE posts SET deleted_at = NOW() diff --git a/internal/db/postgres/post_repo_cursor_test.go b/internal/db/postgres/post_repo_cursor_test.go index 6275cfe..50fcfae 100644 --- a/internal/db/postgres/post_repo_cursor_test.go +++ b/internal/db/postgres/post_repo_cursor_test.go @@ -11,7 +11,7 @@ import ( ) func TestParseAuthorPostsCursor(t *testing.T) { - repo := &postgresPostRepo{db: nil} // db not needed for cursor parsing + repo := &PostRepository{db: nil} // db not needed for cursor parsing // Helper to create a valid cursor makeCursor := func(timestamp, uri string) string { @@ -119,7 +119,7 @@ func TestParseAuthorPostsCursor(t *testing.T) { } func TestBuildAuthorPostsCursor(t *testing.T) { - repo := &postgresPostRepo{db: nil} + repo := &PostRepository{db: nil} now := time.Now() post := &posts.PostView{ @@ -149,7 +149,7 @@ func TestBuildAuthorPostsCursor(t *testing.T) { } func TestBuildAndParseCursorRoundTrip(t *testing.T) { - repo := &postgresPostRepo{db: nil} + repo := &PostRepository{db: nil} now := time.Now() post := &posts.PostView{ @@ -223,15 +223,19 @@ func (m *mockPostRepository) Create(ctx context.Context, post *posts.Post) error return nil } -func (m *mockPostRepository) GetByURI(ctx context.Context, uri string) (*posts.Post, error) { +func (m *mockPostRepository) GetRawIndexedRow(ctx context.Context, uri string) (*posts.Post, error) { return nil, nil } +func (m *mockPostRepository) GetRawIndexedRowsByURIs(ctx context.Context, uris []string) (map[string]*posts.Post, error) { + return map[string]*posts.Post{}, nil +} + func (m *mockPostRepository) GetByAuthor(ctx context.Context, req posts.GetAuthorPostsRequest) ([]*posts.PostView, *string, error) { return nil, nil, nil } -func (m *mockPostRepository) GetViewsByURIs(ctx context.Context, uris []string) (map[string]*posts.PostView, error) { +func (m *mockPostRepository) GetViewsByURIs(ctx context.Context, uris []string, viewerDID string) (map[string]*posts.PostView, error) { return map[string]*posts.PostView{}, nil } diff --git a/internal/db/postgres/post_repo_write_test.go b/internal/db/postgres/post_repo_write_test.go index f17e8a7..4f94589 100644 --- a/internal/db/postgres/post_repo_write_test.go +++ b/internal/db/postgres/post_repo_write_test.go @@ -121,7 +121,7 @@ func TestPostRepo_Create(t *testing.T) { assert.False(t, post.IndexedAt.Equal(authored), "indexed_at must be when the AppView saw the record, not when the author wrote it") - stored, err := repo.GetByURI(ctx, post.URI) + stored, err := repo.GetRawIndexedRow(ctx, post.URI) require.NoError(t, err) assert.Equal(t, post.CID, stored.CID) assert.Equal(t, post.RKey, stored.RKey) @@ -156,7 +156,7 @@ func TestPostRepo_Create(t *testing.T) { post := postRecord(authorDID, communityDID, "bare"+testkit.UniqueID(t)) require.NoError(t, repo.Create(ctx, post)) - stored, err := repo.GetByURI(ctx, post.URI) + stored, err := repo.GetRawIndexedRow(ctx, post.URI) require.NoError(t, err) assert.Nil(t, stored.Title, "a post with no title must read back as absent; an empty string "+ "renders as a blank heading rather than as no heading") @@ -175,7 +175,7 @@ func TestPostRepo_Create(t *testing.T) { post := postRecord(authorDID, communityDID, "counts"+testkit.UniqueID(t)) require.NoError(t, repo.Create(ctx, post)) - stored, err := repo.GetByURI(ctx, post.URI) + stored, err := repo.GetRawIndexedRow(ctx, post.URI) require.NoError(t, err) assert.Zero(t, stored.UpvoteCount) assert.Zero(t, stored.DownvoteCount) @@ -210,7 +210,7 @@ func TestPostRepo_Create(t *testing.T) { "the translation, and a consumer that could not recognise a replay would treat it as "+ "an infrastructure failure and retry forever") - stored, err := repo.GetByURI(ctx, first.URI) + stored, err := repo.GetRawIndexedRow(ctx, first.URI) require.NoError(t, err) require.NotNil(t, stored.Title) assert.Equal(t, "As indexed", *stored.Title, @@ -248,7 +248,7 @@ func TestPostRepo_Create(t *testing.T) { "an author with no users row must index: under author-owned posts that is a federated "+ "author, not an ordering artefact waiting on a backfill") - stored, err := repo.GetByURI(ctx, post.URI) + stored, err := repo.GetRawIndexedRow(ctx, post.URI) require.NoError(t, err, "the post must be readable back, not half-written") assert.Equal(t, unknownAuthor, stored.AuthorDID, "the author DID is carried on the row itself; it is the only identity the AppView has "+ @@ -311,7 +311,7 @@ func TestPostRepo_Create(t *testing.T) { err := repo.Create(ctx, post) require.Errorf(t, err, "%s: a JSONB column accepted a value that is not JSON", tc.name) - _, err = repo.GetByURI(ctx, post.URI) + _, err = repo.GetRawIndexedRow(ctx, post.URI) assert.ErrorIsf(t, err, posts.ErrNotFound, "%s: the rejected post was indexed anyway", tc.name) } }) @@ -375,7 +375,7 @@ func TestPostRepo_SoftDelete(t *testing.T) { t.Parallel() fixture := seed(t) - views, err := fixture.repo.GetViewsByURIs(ctx, []string{fixture.deletedURI, fixture.survivorURI}) + views, err := fixture.repo.GetViewsByURIs(ctx, []string{fixture.deletedURI, fixture.survivorURI}, "") require.NoError(t, err) assert.NotContains(t, views, fixture.deletedURI, "a deleted post must be absent from the map so the caller emits a notFoundPost marker; "+ @@ -414,7 +414,7 @@ func TestPostRepo_SoftDelete(t *testing.T) { t.Parallel() fixture := seed(t) - stored, err := fixture.repo.GetByURI(ctx, fixture.deletedURI) + stored, err := fixture.repo.GetRawIndexedRow(ctx, fixture.deletedURI) require.NoError(t, err, "IF THIS FAILED (issue 2026-07-29-deleted-posts-still-served-by-getcomments.md) the defect is FIXED — delete this pin. The right behaviour is "+ "posts.ErrNotFound (or a caller that checks DeletedAt): a post withdrawn from its "+ @@ -477,7 +477,7 @@ func TestPostRepo_SoftDelete(t *testing.T) { "improvement for a consumer that wants to know a delete arrived before its create — "+ "assert the new error here rather than reverting") - _, err := repo.GetByURI(ctx, absent) + _, err := repo.GetRawIndexedRow(ctx, absent) assert.ErrorIs(t, err, posts.ErrNotFound, "and no row was conjured by the delete") }) @@ -485,7 +485,7 @@ func TestPostRepo_SoftDelete(t *testing.T) { t.Parallel() fixture := seed(t) - survivor, err := fixture.repo.GetByURI(ctx, fixture.survivorURI) + survivor, err := fixture.repo.GetRawIndexedRow(ctx, fixture.survivorURI) require.NoError(t, err) assert.Nil(t, survivor.DeletedAt, "deleting one post marked another as deleted; only the URI predicate separates them") diff --git a/internal/db/postgres/post_visibility.go b/internal/db/postgres/post_visibility.go index bbf4b27..89e5183 100644 --- a/internal/db/postgres/post_visibility.go +++ b/internal/db/postgres/post_visibility.go @@ -1,8 +1,6 @@ package postgres import ( - "context" - "database/sql" "fmt" "Coves/internal/core/posts" @@ -12,9 +10,21 @@ import ( // // This is the single admission-aware gate every posts display query goes // through so that no read path can forget it (the piecemeal-predicate failure -// PRD §6.2 calls out). It is wired into GetViewsByURIs, the three feed queries -// and GetByAuthor; the profile/community counts apply the same accepted rule -// inline (a COUNT subquery has no row to hydrate). +// PRD §6.2 calls out). ONE predicate string serves all of them — +// visiblePostsPredicate below — and everything else in this file is a different +// way of BINDING ITS VIEWER, never a second copy of the rule: +// +// - visiblePostsJoin(n) — viewer bound to $n. GetViewsByURIs, +// VisibleHeaderView, GetByAuthor, the three feed queries, and the profile +// post_count. +// - visiblePostCountSubquery(expr) — viewer bound to the anonymous literal, +// wrapped in a COUNT over one community. The served community postCount. +// +// The counts used to hand-copy the predicate instead. A reviewer demonstrated +// three separate mutations to that copy — dropping the collection check, +// dropping the pinned-CID equality, dropping the community half of the join key +// — that no test caught, because a copy has no way to disagree with itself. +// There is nothing to copy now. // // # The join key is (a.community_did = p.community_did AND a.post_uri = p.uri) // @@ -71,56 +81,75 @@ import ( // bound where the viewer DID is already in the argument list (the timeline // reuses $1). func visiblePostsJoin(viewerParam int) (joinSQL, whereSQL string) { + return visiblePostsPredicate(fmt.Sprintf("$%d", viewerParam)) +} + +// anonymousViewerSQL is the viewer expression for a read that HAS no viewer: a +// literal empty string, which is the same value visiblePostsJoin's callers bind +// for an anonymous read. +// +// It is a SQL literal rather than a bound parameter because there is no input +// here to bind — the public count of a community's posts is definitionally +// viewer-independent, so there is no caller-supplied value that could reach it. +// The constant is spliced into a query built entirely from other constants; no +// user data is concatenated into SQL anywhere in this file. +const anonymousViewerSQL = `''` + +// visiblePostsPredicate is THE read-path predicate, and the only place it is +// spelled. viewerExpr is whatever SQL evaluates to the viewer's DID — a bind +// parameter reference for a query that has a viewer to bind, or +// anonymousViewerSQL for one that structurally does not. +// +// Everything above about the join key, the collection-aware status rule and the +// pinned CID describes THIS function; visiblePostsJoin is the parameterized +// spelling of it. Two viewer bindings, one predicate: a count and a display +// query cannot disagree about what "visible" means, because there is only one +// string. +func visiblePostsPredicate(viewerExpr string) (joinSQL, whereSQL string) { joinSQL = ` LEFT JOIN community_post_admissions a ON a.community_did = p.community_did AND a.post_uri = p.uri` whereSQL = fmt.Sprintf(`( (a.status = 'accepted' AND a.accepted_cid = p.cid) - OR (a.status IS NULL AND (split_part(p.uri, '/', 4) <> '%s' OR p.author_did = $%d)) - OR (a.status IN ('pending', 'pending_reacceptance', 'removed', 'rejected') AND p.author_did = $%d) - )`, posts.PostV2Collection, viewerParam, viewerParam) + OR (a.status IS NULL AND (split_part(p.uri, '/', 4) <> '%s' OR p.author_did = %s)) + OR (a.status IN ('pending', 'pending_reacceptance', 'removed', 'rejected') AND p.author_did = %s) + )`, posts.PostV2Collection, viewerExpr, viewerExpr) return joinSQL, whereSQL } -// countAcceptedPostsForCommunity is the accepted-only source of truth for a -// community's post_count (task 7, PRD §6.2). +// visiblePostCountSubquery renders a scalar subquery counting the posts that are +// PUBLICLY visible in the community named by communityExpr — the same predicate +// every display query runs, with the anonymous viewer. // -// It counts the posts a community has ACCEPTED: a join of `posts` to -// community_post_admissions on the subject key with status = 'accepted', -// excluding soft-deleted rows. +// It exists because `communities.post_count` was a STORED column, and the only +// thing that ever incremented it (community_repo_memberships.go's +// IncrementPostCount) belonged to the retired community-repo write path, so +// nothing has advanced it since posts became author-owned: every community's +// postCount served 0 on community.get/.list/.search, and `ORDER BY post_count` +// was a sort on a uniformly-zero key. A stored counter also has to be advanced +// on the accept transition AND decremented on remove/unaccept/tombstone, and +// every one of those is a chance to drift out of agreement with what the read +// path will actually render. A live subquery over the same predicate cannot +// drift by construction: it IS the read path's answer. // -// It exists because community.post_count is a STORED column whose only -// incrementer is the old community-repo write path (community_repo_memberships.go) -// — nothing advances it on an acceptance, so it is stale under author-owned -// posts. THIS is the accepted-only value the counter must converge on. +// communityExpr must be a column reference or a bind parameter — it is spliced, +// never a caller-supplied value. // -// DEFERRED, deliberately not wired here: reconciling the stored column is a -// consumer-side follow-up (increment on the accept transition, decrement on -// remove/unaccept), sequenced to task 8 — not a read-path concern. There is NO -// content leak in the meantime: every display query already excludes -// non-accepted rows via visiblePostsJoin, so the stale column is a cosmetic -// count, not reachable content. This helper lands the semantics (and its pin, -// TestCommunityPostCountVisibility_AcceptedOnly) now; the incrementer follows. -func countAcceptedPostsForCommunity(ctx context.Context, db *sql.DB, communityDID string) (int, error) { - var count int - err := db.QueryRowContext(ctx, ` - SELECT COUNT(*) - FROM posts p - JOIN community_post_admissions a - ON a.community_did = p.community_did AND a.post_uri = p.uri - WHERE p.community_did = $1 - AND p.deleted_at IS NULL - AND a.status = 'accepted' - AND a.accepted_cid = p.cid - `, communityDID).Scan(&count) - if err != nil { - return 0, fmt.Errorf("counting accepted posts for community %s: %w", communityDID, err) - } - return count, nil +// It is COLLECTION-AWARE, which an accepted-only count is not. A legacy +// social.coves.community.post lives in the community's own repo, is accepted by +// construction, and carries no admission row at all (§3.0) — an accepted-only +// count silently undercounts every community that still holds legacy posts, +// which today is all of them. Counting exactly what the predicate renders is +// also the only definition that cannot advertise unreachable content: the count +// and the feed answer the same question. +func visiblePostCountSubquery(communityExpr string) string { + joinSQL, whereSQL := visiblePostsPredicate(anonymousViewerSQL) + return `(SELECT COUNT(*) + FROM posts p` + joinSQL + ` + WHERE p.community_did = ` + communityExpr + ` + AND p.deleted_at IS NULL + AND ` + whereSQL + `)` } -// Referenced so the count stub is not flagged as dead before GREEN wires it into -// the community counter. Delete this line once it has a real caller. -var _ = countAcceptedPostsForCommunity diff --git a/internal/db/postgres/post_visibility_fixtures_test.go b/internal/db/postgres/post_visibility_fixtures_test.go index a9f52a0..6f512a7 100644 --- a/internal/db/postgres/post_visibility_fixtures_test.go +++ b/internal/db/postgres/post_visibility_fixtures_test.go @@ -90,6 +90,28 @@ func seedVisibilityPostWithEmbed(t *testing.T, db *sql.DB, communityDID, authorD return uri } +// driftedAcceptedCID is the pinned CID of an acceptance the post's content has +// moved PAST (PRD §5.5): the community attested to this CID, an edit landed, and +// posts.cid advanced before the accepted → pending_reacceptance transition +// committed. It is deliberately a value seedVisibilityPost can never mint, so a +// row carrying it is un-attested by construction and the read predicate must +// hide it from everyone. +const driftedAcceptedCID = "bafyDRIFTEDacceptedcid" + +// postContentCID reads back the content CID a seeded post actually carries, so +// an acceptance can PIN it. The read path gates on a.accepted_cid = p.cid, which +// makes "what CID does this post have" load-bearing rather than incidental: an +// acceptance pinning anything else is an acceptance of content that no longer +// stands. +func postContentCID(t *testing.T, db *sql.DB, postURI string) string { + t.Helper() + + var cid string + err := db.QueryRowContext(context.Background(), `SELECT cid FROM posts WHERE uri = $1`, postURI).Scan(&cid) + require.NoErrorf(t, err, "reading the content CID of %s (seed the post before its admission)", postURI) + return cid +} + // seedAdmission writes one community's decision about one post directly into // community_post_admissions. // @@ -99,6 +121,18 @@ func seedVisibilityPostWithEmbed(t *testing.T, db *sql.DB, communityDID, authorD // a community event (accept/remove) advances the (rev, op_rank) watermark, while // a pending observation and a local rejection do not. Callers that only care // about the STATUS a reader keys off can ignore the detail and pass "". +// +// acceptedCID == "" MEANS "PIN THE POST'S REAL CONTENT CID", not "pin some +// placeholder". This is the intuitive-call trap the fixture used to set: the +// default was a fixed literal ("bafyaccepted") that can never equal the CID +// seedVisibilityPost derives from the rkey, so the obvious `…, Accepted, "", ""` +// call seeded an acceptance pinning content the post does not have and produced +// an INVISIBLE post. Every accepted seed in the suite had to hand-repeat the +// "bafypostv2"+rkey literal to work, and the next surface's test would have +// silently asserted against a post nobody can see. The default now looks the CID +// up, so the intuitive call means what it reads as. To seed the §5.5 drifted +// case deliberately, call seedVisibilityAdmissionDriftedCID; for an accepted row +// with NO pin at all, seedVisibilityAdmissionUnpinned. func seedVisibilityAdmission(t *testing.T, db *sql.DB, communityDID, postURI string, status posts.AdmissionStatus, acceptedCID, decisionCode string) { t.Helper() @@ -112,10 +146,23 @@ func seedVisibilityAdmission(t *testing.T, db *sql.DB, communityDID, postURI str acceptanceURI = sql.NullString{String: "at://" + communityDID + "/" + posts.AcceptanceCollection + "/acc" + postURI[len(postURI)-6:], Valid: true} acceptanceRkey = sql.NullString{String: "acc" + postURI[len(postURI)-6:], Valid: true} if acceptedCID == "" { - acceptedCID = "bafyaccepted" + if status == posts.AdmissionStatusPendingReacceptance { + // pending_reacceptance IS the drifted state — the standing + // acceptance pins content the post has moved past — so pinning the + // post's current CID here would seed a row that contradicts its own + // status. + acceptedCID = driftedAcceptedCID + } else { + acceptedCID = postContentCID(t, db, postURI) + } } accCID = sql.NullString{String: acceptedCID, Valid: true} - evalCID = sql.NullString{String: acceptedCID, Valid: true} + // evaluated_cid is "the exact content CID the AppView has indexed", which + // is the POST's CID whatever the acceptance pins — that is precisely how a + // drifted acceptance is recognisable as drifted (accepted_cid <> + // evaluated_cid). Mirroring accepted_cid into it, as this fixture used to, + // seeds a row that claims the acceptance still matches the content. + evalCID = sql.NullString{String: postContentCID(t, db, postURI), Valid: true} rev = sql.NullString{String: "3lqqqqqqqqqq2", Valid: true} opRank = sql.NullInt16{Int16: int16(posts.CommunityOpPut), Valid: true} case posts.AdmissionStatusRemoved: @@ -163,6 +210,41 @@ func seedVisibilityAdmission(t *testing.T, db *sql.DB, communityDID, postURI str require.NoErrorf(t, err, "seeding admission %s for %s in %s", status, postURI, communityDID) } +// seedVisibilityAdmissionDriftedCID seeds the §5.5 read-side hazard explicitly: +// a row that still says status='accepted' while the acceptance pins content the +// post has moved past. The admission consumer commits an edit's new content +// (posts.cid advances) in a SEPARATE transaction from the accepted → +// pending_reacceptance transition, so this state is reachable in production for +// as long as that window is open, and in it the attested content is gone with +// un-attested content standing in its place. The predicate must hide it from +// EVERYONE, author included — there is nothing here anyone agreed to carry. +func seedVisibilityAdmissionDriftedCID(t *testing.T, db *sql.DB, communityDID, postURI string) { + t.Helper() + require.NotEqualf(t, driftedAcceptedCID, postContentCID(t, db, postURI), + "seedVisibilityAdmissionDriftedCID needs a post whose CID differs from the drifted pin, or it seeds the matched case") + seedVisibilityAdmission(t, db, communityDID, postURI, posts.AdmissionStatusAccepted, driftedAcceptedCID, "") +} + +// seedVisibilityAdmissionUnpinned seeds an accepted row whose accepted_cid is +// NULL. The column is nullable and migration 034 attaches no CHECK tying it to +// the status, so an acceptance with nothing pinned is REPRESENTABLE — a partial +// write, an acceptance record indexed before its subject's content, or a future +// writer that forgets the column. `a.accepted_cid = p.cid` is NULL-propagating, +// so such a row can never satisfy the accepted branch and the post is hidden +// from everyone but its author. That is the fail-closed answer (an acceptance +// that attests to no CID attests to nothing) and this helper exists so the case +// is stated rather than inferred. +func seedVisibilityAdmissionUnpinned(t *testing.T, db *sql.DB, communityDID, postURI string) { + t.Helper() + + seedVisibilityAdmission(t, db, communityDID, postURI, posts.AdmissionStatusAccepted, "", "") + _, err := db.ExecContext(context.Background(), ` + UPDATE community_post_admissions SET accepted_cid = NULL + WHERE community_did = $1 AND post_uri = $2 + `, communityDID, postURI) + require.NoErrorf(t, err, "unpinning the acceptance of %s in %s", postURI, communityDID) +} + // visibilityCommunity creates a community row plus its owner, returning the DID. // A thin wrapper over createTestCommunity that mints the owner too, so a suite // can stand up two communities (the fork case) without hand-rolling owners. diff --git a/internal/db/postgres/post_visibility_test.go b/internal/db/postgres/post_visibility_test.go index 1a888be..2d36271 100644 --- a/internal/db/postgres/post_visibility_test.go +++ b/internal/db/postgres/post_visibility_test.go @@ -4,10 +4,12 @@ package postgres import ( "context" + "database/sql" "testing" "time" "Coves/internal/core/comments" + "Coves/internal/core/communities" "Coves/internal/core/communityFeeds" "Coves/internal/core/discover" "Coves/internal/core/posts" @@ -238,7 +240,7 @@ func TestPostGetVisibility_PublicSeesAcceptedOnly(t *testing.T) { seedVisibilityAdmission(t, db, community, removed, posts.AdmissionStatusRemoved, "", "rule-violation") repo := NewPostRepository(db) - views, err := repo.GetViewsByURIs(ctx, []string{accepted, pending, rejected, removed}) + views, err := repo.GetViewsByURIs(ctx, []string{accepted, pending, rejected, removed}, publicViewer) require.NoError(t, err) require.Containsf(t, views, accepted, "an accepted post must be served by post.get") @@ -258,7 +260,7 @@ func TestPostGetVisibility_PublicSeesAcceptedOnly(t *testing.T) { unknownPost := seedVisibilityPost(t, db, community, unknownAuthor, "pgunk", "federated", base.Add(5*time.Hour)) seedVisibilityAdmission(t, db, community, unknownPost, posts.AdmissionStatusAccepted, "bafypostv2pgunk", "") - got, err := repo.GetViewsByURIs(ctx, []string{unknownPost}) + got, err := repo.GetViewsByURIs(ctx, []string{unknownPost}, publicViewer) require.NoError(t, err) require.Containsf(t, got, unknownPost, "an accepted post whose author has no users row was dropped by post.get. The author join must be a LEFT "+ @@ -271,6 +273,85 @@ func TestPostGetVisibility_PublicSeesAcceptedOnly(t *testing.T) { }) } +// TestPostGetVisibility_AuthorSeesOwnPendingPost is post.get's AUTHOR branch. +// +// The author-self-view contract (PRD §6.2) is one contract, not a per-endpoint +// courtesy: an author sees their own posts in every admission state so a client +// can render "pending review" / "removed". actor.getPosts, the feeds and the +// getComments thread header all honor it, and post.get — the permalink surface a +// client sends the author to from their own profile — must give the same answer, +// or an author following a link to their own pending post is told it does not +// exist while the profile they came from just listed it. +// +// The viewer is threaded through GetViewsByURIs, so the anonymous read is the +// EXPLICIT "" case rather than the only case. Every non-author — the anonymous +// public and an authenticated stranger alike — still sees accepted content only. +func TestPostGetVisibility_AuthorSeesOwnPendingPost(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + community := visibilityCommunity(t, db, "as") + author := "did:plc:visasauthor" + createTestUser(t, db, "visasauthor.test", author) + stranger := "did:plc:visasstranger" + createTestUser(t, db, "visasstranger.test", stranger) + + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + accepted := seedVisibilityPost(t, db, community, author, "asacc", "accepted", base.Add(5*time.Hour)) + pending := seedVisibilityPost(t, db, community, author, "aspen", "pending", base.Add(4*time.Hour)) + rejected := seedVisibilityPost(t, db, community, author, "asrej", "rejected", base.Add(3*time.Hour)) + removed := seedVisibilityPost(t, db, community, author, "asrem", "removed", base.Add(2*time.Hour)) + // No admission row at all: the failed-seed postv2 case, which must also + // resolve for its author and for nobody else. + unseeded := seedVisibilityPost(t, db, community, author, "asuns", "no admission row", base.Add(1*time.Hour)) + + seedVisibilityAdmission(t, db, community, accepted, posts.AdmissionStatusAccepted, "", "") + seedVisibilityAdmission(t, db, community, pending, posts.AdmissionStatusPending, "", "") + seedVisibilityAdmission(t, db, community, rejected, posts.AdmissionStatusRejected, "", "spam") + seedVisibilityAdmission(t, db, community, removed, posts.AdmissionStatusRemoved, "", "rule-violation") + + repo := NewPostRepository(db) + all := []string{accepted, pending, rejected, removed, unseeded} + read := func(t *testing.T, viewer string) map[string]*posts.PostView { + t.Helper() + views, err := repo.GetViewsByURIs(ctx, all, viewer) + require.NoError(t, err) + return views + } + + t.Run("the author sees their own posts in every admission state", func(t *testing.T) { + views := read(t, author) + for _, uri := range all { + assert.Containsf(t, views, uri, + "post.get hid a post from its own AUTHOR (%s). The author-self-view contract is one contract across "+ + "surfaces: actor.getPosts, the feeds and the getComments header all show the author their own "+ + "pending/rejected/removed posts, so a permalink that answers notFound makes post.get the one surface "+ + "that disagrees (PRD §6.2)", uri) + } + }) + + t.Run("an authenticated stranger sees accepted only", func(t *testing.T) { + views := read(t, stranger) + assert.Contains(t, views, accepted) + for _, uri := range []string{pending, rejected, removed, unseeded} { + assert.NotContainsf(t, views, uri, + "post.get served a non-accepted post (%s) to an authenticated stranger. Threading a viewer must widen the "+ + "view for the AUTHOR only — anyone else is exactly the anonymous public", uri) + } + }) + + t.Run("the anonymous public sees accepted only", func(t *testing.T) { + views := read(t, publicViewer) + assert.Contains(t, views, accepted) + for _, uri := range []string{pending, rejected, removed, unseeded} { + assert.NotContainsf(t, views, uri, + "post.get served a non-accepted post (%s) to the anonymous public. The empty viewer DID is the fail-closed "+ + "default and must stay indistinguishable from a stranger", uri) + } + }) +} + // TestPostGetVisibility_AuthorMediaResolvesOnVisibleRow pins that the admission // predicate does not cost the visible row its media owner. A postv2 post's blobs // live in the AUTHOR's repository (blobOwnerOf, PRD §3.1), so the row must carry @@ -294,7 +375,7 @@ func TestPostGetVisibility_AuthorMediaResolvesOnVisibleRow(t *testing.T) { seedVisibilityAdmission(t, db, community, accepted, posts.AdmissionStatusAccepted, "bafypostv2pmacc", "") repo := NewPostRepository(db) - views, err := repo.GetViewsByURIs(ctx, []string{accepted}) + views, err := repo.GetViewsByURIs(ctx, []string{accepted}, publicViewer) require.NoError(t, err) require.Contains(t, views, accepted) @@ -393,6 +474,68 @@ func TestProfileStatsVisibility_PostCountExcludesNonAccepted(t *testing.T) { "1 accepted) advertises the existence of content no reader can reach", 3) } +// TestProfileStatsVisibility_PostCountRunsTheWholePredicate pins the three parts +// of the read-path rule that the profile count used to re-implement inline, and +// that a reviewer showed could each be deleted from that copy with no test +// noticing: +// +// 1. the collection check — drop it and a failed-seed postv2 counts +// 2. the pinned-CID equality — drop it and a §5.5 drifted post counts +// 3. the community join half — drop it and another community's acceptance counts +// +// The count now calls visiblePostsJoin, so a copy cannot drift; this is the +// assertion that says so out loud, and it is the one that fails if anyone +// re-inlines the rule. The oracle is actor.getPosts read anonymously — the count +// must equal the number of posts a stranger can actually list. +func TestProfileStatsVisibility_PostCountRunsTheWholePredicate(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + community := visibilityCommunity(t, db, "pw") + forker := visibilityCommunity(t, db, "pwF") + author := "did:plc:vispwauthor" + createTestUser(t, db, "vispwauthor.test", author) + + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + accepted := seedVisibilityPost(t, db, community, author, "pwacc", "accepted", base.Add(5*time.Hour)) + legacy := seedFilterablePost(t, db, community, author, "pwleg", base.Add(4*time.Hour)) + // (1) postv2, no admission row: the pending seed failed. Fail closed. + seedVisibilityPost(t, db, community, author, "pwuns", "no admission row", base.Add(3*time.Hour)) + // (2) accepted, but the acceptance pins content the post has moved past. + drifted := seedVisibilityPost(t, db, community, author, "pwdri", "drifted", base.Add(2*time.Hour)) + // (3) pending at home, accepted by a community that forked it. + forked := seedVisibilityPost(t, db, community, author, "pwfrk", "forked elsewhere", base.Add(1*time.Hour)) + + seedVisibilityAdmission(t, db, community, accepted, posts.AdmissionStatusAccepted, "", "") + seedVisibilityAdmissionDriftedCID(t, db, community, drifted) + seedVisibilityAdmission(t, db, community, forked, posts.AdmissionStatusPending, "", "") + seedVisibilityAdmission(t, db, forker, forked, posts.AdmissionStatusAccepted, "", "") + + // The oracle: exactly what a stranger can list on this author's profile. + views, _, err := NewPostRepository(db).GetByAuthor(ctx, posts.GetAuthorPostsRequest{ + ActorDID: author, ViewerDID: publicViewer, Limit: 50, + }) + require.NoError(t, err) + listed := make([]string, 0, len(views)) + for _, v := range views { + listed = append(listed, v.URI) + } + require.ElementsMatchf(t, []string{accepted, legacy}, listed, + "the fixture no longer means what this test says it means: expected the accepted postv2 and the legacy post "+ + "to be the only publicly listable posts, got %v", listed) + + stats, err := NewUserRepository(db).GetProfileStats(ctx, author) + require.NoError(t, err) + assert.Equalf(t, len(listed), stats.PostCount, + "a profile's post_count (%d) must equal what actor.getPosts lists to a stranger (%d: %v). The three ways "+ + "this diverges are the three mutations the inline copy of the predicate admitted: counting the "+ + "no-admission postv2 (the collection check), counting the drifted-CID post (the accepted_cid = p.cid "+ + "equality, §5.5), and counting the post another community forked (the a.community_did = p.community_did "+ + "half of the join key). Any of them advertises content no reader can reach", + stats.PostCount, len(listed), listed) +} + // TestVisibility_CollectionAwareFailClosed is the corrected core of task 7, and // the single most important assertion in this suite. Cycle 1's predicate went // FAIL-OPEN: a post with no admission row was visible to everyone. That is @@ -446,7 +589,7 @@ func TestVisibility_CollectionAwareFailClosed(t *testing.T) { "a legacy community.post with no admission row must stay visible — it was accepted by construction under the "+ "old model and task 7 must not retro-hide content that predates admissions (visible until task 8's drain)") - views, err := postRepo.GetViewsByURIs(ctx, []string{legacy}) + views, err := postRepo.GetViewsByURIs(ctx, []string{legacy}, publicViewer) require.NoError(t, err) assert.Contains(t, views, legacy, "post.get must still serve a legacy post with no admission row to the public") }) @@ -469,7 +612,7 @@ func TestVisibility_CollectionAwareFailClosed(t *testing.T) { assert.NotContains(t, discoverFeedURIs(disc), postv2, "a postv2 with no admission row leaked into discover") - views, err := postRepo.GetViewsByURIs(ctx, []string{postv2}) + views, err := postRepo.GetViewsByURIs(ctx, []string{postv2}, publicViewer) require.NoError(t, err) assert.NotContainsf(t, views, postv2, "post.get served a postv2 with no admission row to the public — permalink is the alternate path the feed "+ @@ -485,13 +628,15 @@ func TestVisibility_CollectionAwareFailClosed(t *testing.T) { "the author of a postv2 with no admission row cannot see their own post in the community feed; a missing "+ "seed must fail closed for OTHERS, never for the author") - // NOTE — post.get's author path is a known follow-up, not covered here. - // GetViewsByURIs takes no viewer DID (posts.Repository's 2-arg signature), - // so post.get currently hides a non-accepted postv2 from its author too. - // Threading a viewer would break the three in-suite Repository fakes, so it - // is deferred: the author reaches their own pending/no-admission posts - // through actor.getPosts (TestActorPostsVisibility_AuthorVsNonAuthor) and - // getStatus, which is sufficient. Flagged in the cycle-2 report. + // post.get honors the same branch. It used to be the one surface that did + // not — GetViewsByURIs took no viewer — so an author following a permalink + // to a post their own profile had just listed was told it did not exist. + authorViews, err := postRepo.GetViewsByURIs(ctx, []string{postv2}, author) + require.NoError(t, err) + assert.Containsf(t, authorViews, postv2, + "post.get hid a postv2 with no admission row from its own AUTHOR. Every other surface (the feeds, "+ + "actor.getPosts, the getComments header) shows it, so the permalink must too — see "+ + "TestPostGetVisibility_AuthorSeesOwnPendingPost for the full state matrix") }) } @@ -616,44 +761,97 @@ func TestGetCommentsVisibility_HeaderIsAdmissionAndDeleteAware(t *testing.T) { }) } -// TestCommunityPostCountVisibility_AcceptedOnly pins that a community's -// post_count reflects accepted posts only (PRD §6.2: counts must not include -// non-accepted rows). +// TestCommunityPostCountVisibility_MatchesWhatTheFeedRenders pins that the +// postCount a community SERVES is the number of posts a reader can actually +// reach in it (PRD §6.2: counts must not include non-accepted rows — and, just +// as much, must not omit rows the feed does show). // -// UNLIKE the user post_count, which is a live COUNT this task can gate directly, -// community.post_count is a STORED column with a write-time incrementer -// (community_repo_memberships.go) left over from the old community-repo write -// path. Under author-owned posts nothing increments it on acceptance, so it is -// already stale — and the honest fix is consumer-side (increment on the accept -// transition, decrement on remove/unaccept), NOT a read predicate. +// It used to be a STORED column whose only incrementer belonged to the retired +// community-repo write path, so under author-owned posts nothing advanced it: +// every community served postCount 0, and `sort=active` ordered by a uniformly +// zero key. It is now a live subquery over the SAME predicate the feeds run, so +// the count and the feed cannot disagree. // -// This pins the accepted-only SEMANTICS against countAcceptedPostsForCommunity — -// the source of truth GREEN would drive the counter from, whether it recomputes -// live or reconciles the stored column from the admission consumer. See the -// cycle-2 report for the sequencing recommendation (this is a consumer-side -// follow-up, not part of the read-path predicate). -func TestCommunityPostCountVisibility_AcceptedOnly(t *testing.T) { +// The seeded set is deliberately the whole matrix, because the two obvious +// wrong answers each fail on half of it: an accepted-only count drops the legacy +// post (accepted by construction, no admission row — §3.0), and a +// status-only count keeps the drifted-CID post the feed hides (§5.5). +func TestCommunityPostCountVisibility_MatchesWhatTheFeedRenders(t *testing.T) { t.Parallel() db := testkit.DB(t) ctx := context.Background() community := visibilityCommunity(t, db, "cc") + forker := visibilityCommunity(t, db, "ccF") author := "did:plc:visccauthor" createTestUser(t, db, "visccauthor.test", author) base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) - accepted := seedVisibilityPost(t, db, community, author, "ccacc", "accepted", base.Add(3*time.Hour)) - pending := seedVisibilityPost(t, db, community, author, "ccpen", "pending", base.Add(2*time.Hour)) - removed := seedVisibilityPost(t, db, community, author, "ccrem", "removed", base.Add(1*time.Hour)) - seedVisibilityAdmission(t, db, community, accepted, posts.AdmissionStatusAccepted, "bafypostv2ccacc", "") + accepted := seedVisibilityPost(t, db, community, author, "ccacc", "accepted", base.Add(7*time.Hour)) + pending := seedVisibilityPost(t, db, community, author, "ccpen", "pending", base.Add(6*time.Hour)) + removed := seedVisibilityPost(t, db, community, author, "ccrem", "removed", base.Add(5*time.Hour)) + drifted := seedVisibilityPost(t, db, community, author, "ccdri", "drifted past its acceptance", base.Add(4*time.Hour)) + // Accepted, then withdrawn by its author: gone, and gone from the count. + deleted := seedVisibilityPost(t, db, community, author, "ccdel", "author withdrew it", base.Add(3*time.Hour)) + // A postv2 whose pending seed never landed: fail closed, so uncounted. + seedVisibilityPost(t, db, community, author, "ccuns", "no admission row", base.Add(2*time.Hour)) + // A LEGACY community-repo post with no admission row: accepted by + // construction, rendered by the feed, and therefore counted. + legacy := seedFilterablePost(t, db, community, author, "cclegacy", base.Add(1*time.Hour)) + // The fork case: pending here, accepted by a DIFFERENT community — which + // also holds one accepted post of its own, so the forker's count has a + // nonzero right answer and "the acceptance leaked into the wrong community's + // count" is distinguishable from "every count is zero". + forked := seedVisibilityPost(t, db, community, author, "ccfrk", "forked elsewhere", base.Add(30*time.Minute)) + forkerOwn := seedVisibilityPost(t, db, forker, author, "ccfown", "the forker's own post", base.Add(20*time.Minute)) + seedVisibilityAdmission(t, db, forker, forkerOwn, posts.AdmissionStatusAccepted, "", "") + + seedVisibilityAdmission(t, db, community, accepted, posts.AdmissionStatusAccepted, "", "") seedVisibilityAdmission(t, db, community, pending, posts.AdmissionStatusPending, "", "") seedVisibilityAdmission(t, db, community, removed, posts.AdmissionStatusRemoved, "", "rule-violation") + seedVisibilityAdmissionDriftedCID(t, db, community, drifted) + seedVisibilityAdmission(t, db, community, deleted, posts.AdmissionStatusAccepted, "", "") + seedVisibilityAdmission(t, db, community, forked, posts.AdmissionStatusPending, "", "") + seedVisibilityAdmission(t, db, forker, forked, posts.AdmissionStatusAccepted, "", "") + _, err := db.ExecContext(ctx, `UPDATE posts SET deleted_at = NOW() WHERE uri = $1`, deleted) + require.NoError(t, err) + + // The oracle: whatever the community feed renders to the anonymous public is + // exactly what the count must report. Asserting against the FEED rather than + // a hand-counted literal is what keeps the two from drifting. + feed, _, err := NewCommunityFeedRepository(db, "test-secret").GetCommunityFeed(ctx, + communityFeeds.GetCommunityFeedRequest{Community: community, ViewerDID: publicViewer, Sort: visibilitySort, Limit: 50}) + require.NoError(t, err) + visible := feedURIs(feed) + require.ElementsMatchf(t, []string{accepted, legacy}, visible, + "the fixture no longer means what this test says it means: expected exactly the accepted postv2 and the "+ + "legacy post to be publicly visible, got %v", visible) + + got, err := NewCommunityRepository(db).GetByDID(ctx, community) + require.NoError(t, err) + assert.Equalf(t, len(visible), got.PostCount, + "community.postCount (%d) disagrees with what the community feed serves (%d posts: %v). It must be the SAME "+ + "predicate: an accepted-only count drops the legacy post (accepted by construction, no admission row), a "+ + "status-only count keeps the drifted-CID post the feed hides (§5.5), and a stored counter — which is what "+ + "this was — drifts to 0 the moment its incrementer stops being called", + got.PostCount, len(visible), visible) + assert.Equalf(t, 1, communityPostCount(t, ctx, db, forker), + "the forking community's count must be 1 — its OWN accepted post (%s) and nothing else. It also holds an "+ + "accepted admission for %s, a post whose home community is %s, and counting that would publish another "+ + "community's post into this one's headline number. The count runs the same (community_did, post_uri) join "+ + "key the feeds do, so an acceptance can only ever count toward the community that issued it ABOUT a post "+ + "it hosts", + forkerOwn, forked, community) +} - count, err := countAcceptedPostsForCommunity(ctx, db, community) +// communityPostCount reads one community's SERVED postCount — the number the +// API hands a client, not a recomputation of it, so the assertion is about the +// wire value rather than about a query the test wrote itself. +func communityPostCount(t *testing.T, ctx context.Context, db *sql.DB, communityDID string) int { + t.Helper() + community, err := NewCommunityRepository(db).GetByDID(ctx, communityDID) require.NoError(t, err) - assert.Equalf(t, 1, count, - "a community's accepted-post count must be 1 (3 seeded: accepted, pending, removed); a count that includes "+ - "non-accepted rows advertises content no reader can reach") + return community.PostCount } // TestPostGetVisibility_AcceptedBranchHonorsPinnedCID pins §5.5 at the READ path: @@ -690,7 +888,7 @@ func TestPostGetVisibility_AcceptedBranchHonorsPinnedCID(t *testing.T) { seedVisibilityAdmission(t, db, community, matched, posts.AdmissionStatusAccepted, "bafypostv2pcmat", "") postRepo := NewPostRepository(db) - views, err := postRepo.GetViewsByURIs(ctx, []string{mismatched, matched}) + views, err := postRepo.GetViewsByURIs(ctx, []string{mismatched, matched}, publicViewer) require.NoError(t, err) assert.Containsf(t, views, matched, "an accepted post whose pinned CID still matches its content must be visible") @@ -711,6 +909,207 @@ func TestPostGetVisibility_AcceptedBranchHonorsPinnedCID(t *testing.T) { "the community feed rendered an accepted post whose content has drifted past its pinned CID (§5.5 read-side leak)") } +// TestPostGetVisibility_AcceptedWithNoPinnedCIDIsHidden covers the state the +// schema allows and nothing had exercised: status='accepted' with accepted_cid +// NULL. +// +// Migration 034 makes accepted_cid nullable and attaches no CHECK tying it to +// the status, so the row is representable — a partial write, an acceptance +// indexed before its subject's content, or a future writer that simply forgets +// the column. `a.accepted_cid = p.cid` is NULL-propagating, so the accepted +// branch evaluates to NULL rather than TRUE and the post is hidden. That is the +// fail-closed answer and it should stay one: an acceptance that attests to NO +// CID attests to nothing, and rendering it would publish content on the strength +// of a row that never named it. +// +// It is hidden from its AUTHOR too, and that is the same deliberate answer the +// §5.5 drifted-CID case gets (see visiblePostsJoin's doc comment: an accepted row +// whose pin does not match is hidden from EVERYONE). The author branch keys on +// the non-accepted statuses, and this row says 'accepted' — so there is no +// widening here that would not also un-hide drifted content. The author's +// recourse is post.getStatus, which reads the admission row directly rather than +// through the display predicate, and reports exactly this state. +func TestPostGetVisibility_AcceptedWithNoPinnedCIDIsHidden(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + community := visibilityCommunity(t, db, "np") + author := "did:plc:visnpauthor" + createTestUser(t, db, "visnpauthor.test", author) + stranger := "did:plc:visnpstranger" + createTestUser(t, db, "visnpstranger.test", stranger) + + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + unpinned := seedVisibilityPost(t, db, community, author, "npunp", "accepted, nothing pinned", base.Add(2*time.Hour)) + pinned := seedVisibilityPost(t, db, community, author, "nppin", "accepted and pinned", base.Add(1*time.Hour)) + seedVisibilityAdmissionUnpinned(t, db, community, unpinned) + seedVisibilityAdmission(t, db, community, pinned, posts.AdmissionStatusAccepted, "", "") + + repo := NewPostRepository(db) + for _, viewer := range []struct{ name, did string }{ + {"the anonymous public", publicViewer}, + {"an authenticated stranger", stranger}, + } { + t.Run(viewer.name+" cannot see it", func(t *testing.T) { + views, err := repo.GetViewsByURIs(ctx, []string{unpinned, pinned}, viewer.did) + require.NoError(t, err) + assert.Contains(t, views, pinned, "the control — an acceptance that pins the current content — must be visible") + assert.NotContainsf(t, views, unpinned, + "post.get served a post whose acceptance pins NO CID. accepted_cid is nullable with no CHECK tying it "+ + "to the status, so this row is representable in production; `a.accepted_cid = p.cid` is "+ + "NULL-propagating and that NULL is what keeps it closed. A predicate rewritten to COALESCE, or to "+ + "gate on status alone, would publish content on the strength of an acceptance that never named it") + }) + } + + t.Run("not even its author sees it, matching the drifted-CID case", func(t *testing.T) { + views, err := repo.GetViewsByURIs(ctx, []string{unpinned, pinned}, author) + require.NoError(t, err) + assert.Contains(t, views, pinned, "the author's own accepted-and-pinned post is unaffected") + assert.NotContainsf(t, views, unpinned, + "an accepted row with no pinned CID is hidden from EVERYONE, its author included — the same answer the "+ + "§5.5 drifted-CID case gets, because the author branch keys on the non-accepted statuses and this row "+ + "says 'accepted'. If you are changing this to show the author, note that the same change un-hides "+ + "drifted content, and that post.getStatus already reports this state from the admission row directly") + }) +} + +// TestVisibility_PendingReacceptanceIsAuthorOnly covers the one admission status +// the visibility suite had never exercised, though the schema, the consumer and +// the fixture all support it. +// +// pending_reacceptance is the §5.5 settled state: an acceptance stands but pins +// content the author has since edited past, so the community has attested to +// something that is no longer there. The post is therefore NOT public — the +// community has not agreed to carry what the post now says — while remaining +// visible to its author, who is precisely the person who needs to see that their +// edit is awaiting re-acceptance. It is listed in the predicate's author branch +// alongside pending/removed/rejected; nothing proved it. +func TestVisibility_PendingReacceptanceIsAuthorOnly(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + community := visibilityCommunity(t, db, "pr") + author := "did:plc:visprauthor" + createTestUser(t, db, "visprauthor.test", author) + stranger := "did:plc:visprstranger" + createTestUser(t, db, "visprstranger.test", stranger) + + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + reaccepting := seedVisibilityPost(t, db, community, author, "prrea", "edited, awaiting re-acceptance", base.Add(2*time.Hour)) + accepted := seedVisibilityPost(t, db, community, author, "pracc", "accepted", base.Add(1*time.Hour)) + seedVisibilityAdmission(t, db, community, reaccepting, posts.AdmissionStatusPendingReacceptance, "", "") + seedVisibilityAdmission(t, db, community, accepted, posts.AdmissionStatusAccepted, "", "") + + postRepo := NewPostRepository(db) + feedRepo := NewCommunityFeedRepository(db, "test-secret") + communityFeed := func(t *testing.T, viewer string) []string { + t.Helper() + feed, _, err := feedRepo.GetCommunityFeed(ctx, communityFeeds.GetCommunityFeedRequest{ + Community: community, ViewerDID: viewer, Sort: visibilitySort, Limit: 50, + }) + require.NoError(t, err) + return feedURIs(feed) + } + + for _, viewer := range []struct{ name, did string }{ + {"the anonymous public", publicViewer}, + {"an authenticated stranger", stranger}, + } { + t.Run(viewer.name+" cannot reach a pending_reacceptance post", func(t *testing.T) { + assert.NotContainsf(t, communityFeed(t, viewer.did), reaccepting, + "the community feed served a pending_reacceptance post. The standing acceptance pins content the "+ + "author has edited past (§5.5), so what the post says NOW is un-attested — the community agreed "+ + "to carry something else") + + views, err := postRepo.GetViewsByURIs(ctx, []string{reaccepting}, viewer.did) + require.NoError(t, err) + assert.NotContains(t, views, reaccepting, + "post.get served a pending_reacceptance post to a non-author — the permalink is the alternate path "+ + "the feed gate is worthless without") + }) + } + + t.Run("its author sees it in both surfaces", func(t *testing.T) { + assert.Containsf(t, communityFeed(t, author), reaccepting, + "an author must see their own post awaiting re-acceptance — it is how a client renders 'your edit is "+ + "waiting for the moderators' rather than silently losing the post") + views, err := postRepo.GetViewsByURIs(ctx, []string{reaccepting}, author) + require.NoError(t, err) + require.Contains(t, views, reaccepting) + assert.Equalf(t, string(posts.AdmissionStatusPendingReacceptance), views[reaccepting].Status, + "the author's own view must carry the admission status, or the client has nothing to render the "+ + "'awaiting re-acceptance' state from") + }) + + t.Run("the accepted control is public", func(t *testing.T) { + assert.Contains(t, communityFeed(t, publicViewer), accepted) + }) +} + +// TestCommunityListVisibility_ActiveSortOrdersByVisiblePosts pins the sort key +// that `sort=active` orders on. +// +// It used to be the stored communities.post_count, whose incrementer lost its +// caller when posts became author-owned — so every community's key was 0 and +// "most active" returned an arbitrary order while looking like it worked. It is +// now the same live, visibility-gated count community.get serves, which means +// this sort can only ever be as wrong as the feed is. +func TestCommunityListVisibility_ActiveSortOrdersByVisiblePosts(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + // The QUIET community is created first on purpose: with the old stale sort key + // every community ties at 0, and a tie falls back to whatever order the plan + // produces — so a test whose expected winner is also the first row inserted + // passes against the broken key by luck. Seeding the loser first makes the + // assertion depend on the key rather than on the scan. + quiet := visibilityCommunity(t, db, "sa2") + busy := visibilityCommunity(t, db, "sa1") + author := "did:plc:vissaauthor" + createTestUser(t, db, "vissaauthor.test", author) + + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + // The busy community: two accepted posts. + for i, rkey := range []string{"sabu1", "sabu2"} { + uri := seedVisibilityPost(t, db, busy, author, rkey, "accepted", base.Add(time.Duration(i)*time.Hour)) + seedVisibilityAdmission(t, db, busy, uri, posts.AdmissionStatusAccepted, "", "") + } + // The quiet community: one accepted post, plus three the predicate hides. A + // sort keyed on raw row count would put this community FIRST. + visible := seedVisibilityPost(t, db, quiet, author, "saqu1", "accepted", base.Add(3*time.Hour)) + seedVisibilityAdmission(t, db, quiet, visible, posts.AdmissionStatusAccepted, "", "") + for i, rkey := range []string{"saqu2", "saqu3", "saqu4"} { + uri := seedVisibilityPost(t, db, quiet, author, rkey, "pending", base.Add(time.Duration(4+i)*time.Hour)) + seedVisibilityAdmission(t, db, quiet, uri, posts.AdmissionStatusPending, "", "") + } + + listed, err := NewCommunityRepository(db).List(ctx, communities.ListCommunitiesRequest{ + Sort: "active", Limit: 100, + }) + require.NoError(t, err) + + rank := map[string]int{} + counts := map[string]int{} + for i, c := range listed { + rank[c.DID] = i + counts[c.DID] = c.PostCount + } + require.Containsf(t, rank, busy, "the busy community is missing from the list") + require.Containsf(t, rank, quiet, "the quiet community is missing from the list") + + assert.Equal(t, 2, counts[busy], "the busy community has two accepted posts") + assert.Equal(t, 1, counts[quiet], "the quiet community has one accepted post and three the predicate hides") + assert.Lessf(t, rank[busy], rank[quiet], + "sort=active ranked the community with ONE visible post (%d of 4 rows) above the one with TWO. The key must "+ + "be the visibility-gated count: keyed on the stale stored column every community ties at 0 and the order "+ + "is arbitrary, and keyed on a raw row count the pending posts nobody can see decide the ranking", + counts[quiet]) +} + // TestFeedsVisibility_UnknownAuthorAccepted guards the §5.3 open-posting promise // on EVERY feed, not just post.get. An accepted post by an author with no `users` // row (a federated author the AppView has not hydrated) must appear, with its diff --git a/internal/db/postgres/user_repo.go b/internal/db/postgres/user_repo.go index 4faea8b..c14d3b5 100644 --- a/internal/db/postgres/user_repo.go +++ b/internal/db/postgres/user_repo.go @@ -1,7 +1,6 @@ package postgres import ( - "Coves/internal/core/posts" "Coves/internal/core/users" "context" "database/sql" @@ -225,6 +224,15 @@ func (r *postgresUserRepo) GetByDIDs(ctx context.Context, dids []string) (map[st return result, nil } +// anonymousProfileViewer is the viewer bound into the profile's post_count. +// +// A profile's stats are a PUBLIC surface — the same numbers go to every caller, +// so there is no viewer to thread and the author self-view branches must not +// fire. Naming it rather than passing a bare "" at the call site is the point: +// an empty string in an argument list reads like an oversight, and this one is a +// decision. +const anonymousProfileViewer = "" + // GetProfileStats retrieves aggregated statistics for a user profile // This performs a single query with scalar subqueries for efficiency func (r *postgresUserRepo) GetProfileStats(ctx context.Context, did string) (*users.ProfileStats, error) { @@ -237,31 +245,43 @@ func (r *postgresUserRepo) GetProfileStats(ctx context.Context, did string) (*us // Reputation represents historical contributions, while membership_count // reflects current active community access. A banned user keeps their // earned reputation but loses the membership count. + // // post_count counts VISIBLE posts only: a profile advertising posts no reader - // can reach is a side channel onto non-accepted content (PRD §6.2). This is - // the public count — the anonymous, collection-aware rule of visiblePostsJoin, - // with no author self-view branch: a post with a decision counts only once its - // own community accepted it AND the acceptance still pins the current content - // (§5.5 — a drifted-accepted post is un-attested and does not count); a row - // with no admission counts iff it is NOT an author-owned postv2 (legacy/bridged - // stays counted, a postv2 with a missing/failed pending seed does not — fail - // closed). The collection is the AT-URI's fourth '/'-segment (split_part), same - // as CollectionOfPostURI; the postv2 collection literal is the shared constant. - query := fmt.Sprintf(` + // can reach is a side channel onto non-accepted content (PRD §6.2). It runs + // THE read-path predicate (visiblePostsJoin) rather than a copy of it — an + // earlier revision inlined the rule here, and a reviewer showed three + // mutations of that copy (dropping the collection check, the pinned-CID + // equality, or the community half of the join key) that no test could catch, + // because a count nobody cross-checks against a feed is unfalsifiable. $2 is + // bound to the empty viewer, which is exactly what makes this the PUBLIC + // count: the author self-view branches turn on `p.author_did = $2`, and no + // DID is "". + // + // comment_count is DELIBERATELY ROOT-BLIND — it counts the actor's comments + // whatever the admission state of the post they hang under, and that is not + // an oversight to be fixed by symmetry with post_count above. A comment is + // the actor's own public speech, and actor.getComments already LISTS it when + // its root is pending or removed, carrying the root as a bare uri/cid + // reference that leaks nothing and resolves through the gated post.get + // (TestActorCommentsVisibility_RootIsReferenceOnly pins that shape). Gating + // the count would put the profile's headline number in disagreement with the + // list the same profile renders, and would leak in the other direction: a + // comment count that visibly drops tells the reader a root they cannot see + // was moderated. The asymmetry with post_count is real and intended — a + // post's visibility IS its community's decision, a comment's is not. + visJoin, visWhere := visiblePostsJoin(2) + query := ` SELECT - (SELECT COUNT(*) FROM posts p - LEFT JOIN community_post_admissions a ON a.community_did = p.community_did AND a.post_uri = p.uri - WHERE p.author_did = $1 AND p.deleted_at IS NULL - AND ((a.status = 'accepted' AND a.accepted_cid = p.cid) - OR (a.status IS NULL AND split_part(p.uri, '/', 4) <> '%s'))) as post_count, + (SELECT COUNT(*) FROM posts p` + visJoin + ` + WHERE p.author_did = $1 AND p.deleted_at IS NULL AND ` + visWhere + `) as post_count, (SELECT COUNT(*) FROM comments WHERE commenter_did = $1 AND deleted_at IS NULL) as comment_count, (SELECT COUNT(*) FROM community_subscriptions WHERE user_did = $1) as community_count, (SELECT COUNT(*) FROM community_memberships WHERE user_did = $1 AND is_banned = false) as membership_count, (SELECT COALESCE(SUM(reputation_score), 0) FROM community_memberships WHERE user_did = $1) as reputation - `, posts.PostV2Collection) + ` stats := &users.ProfileStats{} - err := r.db.QueryRowContext(ctx, query, did).Scan( + err := r.db.QueryRowContext(ctx, query, did, anonymousProfileViewer).Scan( &stats.PostCount, &stats.CommentCount, &stats.CommunityCount,