diff --git a/internal/core/posts/service_blob_test.go b/internal/core/posts/service_blob_test.go index f5ab42a..72a4950 100644 --- a/internal/core/posts/service_blob_test.go +++ b/internal/core/posts/service_blob_test.go @@ -356,6 +356,19 @@ func TestService_AuthorPDSIsHydratedOntoPostViews(t *testing.T) { `, uri, "bafyblobowner", rkey, f.author.DID, f.community.DID, "a post with media") require.NoError(t, err) + // This subject is a postv2, so the task-7 visibility predicate hides it from + // the anonymous GetViewsByURIs read below unless a community has accepted it — + // a postv2 with no admission row fails CLOSED (the consumer always seeds a + // pending row, so a missing one means a failed seed). This test is about PDS + // HYDRATION on a VISIBLE row, not visibility, so it needs the row to be + // visible: seed the accepted admission the acceptance engine would have + // written. + _, err = f.db.ExecContext(ctx, ` + INSERT INTO community_post_admissions (community_did, post_uri, status, accepted_cid, evaluated_cid, last_community_rev, last_community_op_rank, created_at, updated_at) + VALUES ($1, $2, 'accepted', $3, $3, '3lqqqqqqqqqq2', 1, NOW(), NOW()) + `, f.community.DID, uri, "bafyblobowner") + require.NoError(t, err) + views, err := postgres.NewPostRepository(f.db).GetViewsByURIs(ctx, []string{uri}) require.NoError(t, err) require.Contains(t, views, uri) diff --git a/internal/db/postgres/post_visibility.go b/internal/db/postgres/post_visibility.go index 6168b33..51a2939 100644 --- a/internal/db/postgres/post_visibility.go +++ b/internal/db/postgres/post_visibility.go @@ -1,37 +1,89 @@ package postgres +import ( + "context" + "database/sql" + "fmt" +) + // The centralized read-path visibility predicate (task 7, PRD §6.2). // -// STUB — signature only. This is the single admission-aware join every posts -// display query must go through so that no read path can forget the gate (the -// piecemeal-predicate failure PRD §6.2 calls out). GREEN fills in the body and -// wires it into GetViewsByURIs, the three feed queries, GetByAuthor and the -// profile count; the visibility suites in post_visibility_test.go are red until -// it does. +// 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). +// +// # The join key is (a.community_did = p.community_did AND a.post_uri = p.uri) // -// THE JOIN KEY IS (a.community_did = p.community_did AND a.post_uri = p.uri). // Both halves are load-bearing. The post_uri half selects the subject; the // community_did half is what makes a post visible iff ITS OWN community accepted // it, which is the fork-case security property TestDiscoverVisibility_ForkJoinKey // pins — a join on post_uri alone would let one community's acceptance publish -// another community's pending post. -// -// THE STATUS RULE depends on the viewer: -// - a non-author (viewerDID == "" or != the post's author): status = 'accepted' -// only. -// - the author of the post (viewerDID == posts.author_did): accepted OR the -// author's own non-accepted rows, so a client can render 'pending' / 'removed' -// on the author's own profile. -// -// visiblePostsJoin returns the SQL fragment to splice after the posts `p` -// reference (a JOIN plus its WHERE contribution) and the arguments it binds, -// starting at paramOffset. Returning the empty fragment — the stub's behavior — -// applies NO gate, which is the pre-task-7 state every visibility suite fails -// against. -func visiblePostsJoin(viewerDID string, paramOffset int) (sqlFragment string, args []interface{}) { - return "", nil +// another community's pending post. Because p.community_did and p.uri are fixed +// per posts row and community_post_admissions is unique on (community_did, +// post_uri), at most one admission row can match, so the LEFT JOIN never +// duplicates a post. +// +// # The status rule +// +// The gate turns on the admission row the LEFT JOIN produced for the post's OWN +// community: +// +// - a.status = 'accepted' → visible to everyone. +// - a.status IS NULL → visible. No admission row exists for this +// (community, post): a legacy community-repo post, a bridged post, or any +// other collection that never goes through the admission engine, plus the +// narrow window before the consumer opens a fresh postv2's pending row +// (authorpost.go writes the post and its pending admission in separate +// transactions). These carry no decision to gate on, so they stay visible +// exactly as they were before task 7 — the read path is not the place to +// retro-hide content that predates admissions. +// - a.status IN (pending, pending_reacceptance, removed, rejected) AND the +// viewer is the author → visible. An author sees their own posts in every +// admission state so a client can render "pending review" / "removed" on the +// author's own profile (PRD §6.2). Any OTHER viewer — including the anonymous +// public, whose DID is "" — sees accepted content only. This is the security +// core: every non-accepted post that HAS a decision is invisible to +// non-authors on every read path. +// +// visiblePostsJoin returns the JOIN clause to splice into the FROM/JOIN section +// after `FROM posts p`, and the boolean WHERE fragment to AND into the query's +// WHERE clause. The caller owns the arguments: it must bind $viewerParam to the +// viewer's DID (or "" for an anonymous read), reusing a parameter it has already +// bound where the viewer DID is already in the argument list (the timeline +// reuses $1). +func visiblePostsJoin(viewerParam int) (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' + OR a.status IS NULL + OR (a.status IN ('pending', 'pending_reacceptance', 'removed', 'rejected') AND p.author_did = $%d) + )`, viewerParam) + + return joinSQL, whereSQL +} + +// countAcceptedPostsForCommunity is the accepted-only source of truth for a +// community's post_count (task 7, PRD §6.2). +// +// STUB — returns 0 until GREEN implements it. It counts the posts a community has +// ACCEPTED: a join of `posts` to community_post_admissions on the subject key +// with status = 'accepted'. 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. Whether GREEN recomputes the count live or +// reconciles the stored column from the admission consumer, THIS is the value it +// must converge on. The read paths already exclude non-accepted rows, so this is +// a counting concern, not a content leak — see the cycle-2 report for the +// recommendation to sequence the counter itself as a consumer-side follow-up. +func countAcceptedPostsForCommunity(ctx context.Context, db *sql.DB, communityDID string) (int, error) { + return 0, nil } -// Referenced so the stub is not flagged as dead before GREEN wires it into the -// read queries. Delete this line once visiblePostsJoin has a real caller. -var _ = visiblePostsJoin +// 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_test.go b/internal/db/postgres/post_visibility_test.go index 6e85edb..98f03ae 100644 --- a/internal/db/postgres/post_visibility_test.go +++ b/internal/db/postgres/post_visibility_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "Coves/internal/core/comments" "Coves/internal/core/communityFeeds" "Coves/internal/core/discover" "Coves/internal/core/posts" @@ -391,3 +392,222 @@ func TestProfileStatsVisibility_PostCountExcludesNonAccepted(t *testing.T) { "a profile's post_count must count accepted posts only; counting the pending and removed rows too (%d seeded, "+ "1 accepted) advertises the existence of content no reader can reach", 3) } + +// 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 +// correct for a LEGACY community.post — it was signed into the community's own +// repo under the old model, is accepted by construction, and must stay visible +// until task 8 drains it — but it is a security HOLE for a postv2, because the +// task-5 consumer ALWAYS seeds a pending admission the moment it indexes a +// postv2. A postv2 with NO admission row therefore does not mean "pre-admission +// content to grandfather in"; it means the seed has not happened (or failed), +// and the right answer is to FAIL CLOSED. +// +// So the no-admission rule is COLLECTION-AWARE, discriminated by the collection +// segment of the post URI (CollectionOfPostURI): legacy → visible, postv2 → +// hidden from non-authors, visible to its author. Both posts below carry no +// admission row at all; only their collection differs. +func TestVisibility_CollectionAwareFailClosed(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + community := visibilityCommunity(t, db, "fc2") + author := "did:plc:visfc2author" + createTestUser(t, db, "visfc2author.test", author) + stranger := "did:plc:visfc2stranger" + createTestUser(t, db, "visfc2stranger.test", stranger) + + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + + // A legacy community-repo post, no admission row: accepted-by-construction, + // must stay visible to the public until task 8's drain. + legacy := seedFilterablePost(t, db, community, author, "fc2leg", base.Add(2*time.Hour)) + // A postv2, no admission row: the consumer would have seeded pending, so a + // missing row is a failed seed — fail closed for non-authors. + postv2 := seedVisibilityPost(t, db, community, author, "fc2pv2", "postv2 with no admission", base.Add(1*time.Hour)) + + feedRepo := NewCommunityFeedRepository(db, "test-secret") + postRepo := NewPostRepository(db) + discoverRepo := NewDiscoverRepository(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) + } + + t.Run("the legacy post with no admission row stays visible to the public", func(t *testing.T) { + assert.Containsf(t, communityFeed(t, publicViewer), legacy, + "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}) + require.NoError(t, err) + assert.Contains(t, views, legacy, "post.get must still serve a legacy post with no admission row to the public") + }) + + t.Run("the postv2 with no admission row is HIDDEN from non-authors everywhere", func(t *testing.T) { + // The corrected security core. A missing admission row on a postv2 is a + // failed pending seed, not grandfathered content — so it must fail closed + // on every display surface for anyone who is not its author. + assert.NotContainsf(t, communityFeed(t, publicViewer), postv2, + "a postv2 with no admission row leaked to the public in the community feed. The consumer always seeds a "+ + "pending row on index, so no-row means the seed failed — the read path must FAIL CLOSED for postv2, "+ + "not fail open as it does for legacy posts (this is the hole this task closes)") + assert.NotContainsf(t, communityFeed(t, stranger), postv2, + "a postv2 with no admission row leaked to a non-author (an authenticated stranger) in the community feed") + + disc, _, err := discoverRepo.GetDiscover(ctx, discover.GetDiscoverRequest{ + ViewerDID: publicViewer, Sort: visibilitySort, Limit: 50, + }) + require.NoError(t, err) + assert.NotContains(t, discoverFeedURIs(disc), postv2, + "a postv2 with no admission row leaked into discover") + + views, err := postRepo.GetViewsByURIs(ctx, []string{postv2}) + 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 "+ + "gate is worthless without") + }) + + t.Run("the postv2 with no admission row is visible to its own author", func(t *testing.T) { + // A failed/absent seed must not cost the AUTHOR their own post. On the + // surfaces that thread a viewer DID (the feed does), the author sees their + // own postv2 even with no admission row, exactly as they see their own + // pending one. + assert.Containsf(t, communityFeed(t, author), postv2, + "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. + }) +} + +// TestGetCommentsVisibility_HeaderIsAdmissionAndDeleteAware closes the read-path +// hole getComments has carried since 2026-07-29. GetComments hydrates its thread +// header through postRepo.GetByURI, which has NO admission gate and NO +// `deleted_at IS NULL` filter — so the comment thread endpoint serves the full +// header (title, content, author) of a post the feeds correctly hide: a pending +// postv2, and a soft-deleted post. The header must go through the same +// admission+deleted-aware fetch the feeds use. +// +// Driven through the comment SERVICE rather than a bare repo call, because the +// defect is in which fetch GetComments chooses — a repo-only test could not see +// it pick the leaky one. +func TestGetCommentsVisibility_HeaderIsAdmissionAndDeleteAware(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + community := visibilityCommunity(t, db, "gc") + author := "did:plc:visgcauthor" + createTestUser(t, db, "visgcauthor.test", author) + stranger := "did:plc:visgcstranger" + createTestUser(t, db, "visgcstranger.test", stranger) + + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + accepted := seedVisibilityPost(t, db, community, author, "gcacc", "accepted header", base.Add(3*time.Hour)) + pending := seedVisibilityPost(t, db, community, author, "gcpen", "pending header", base.Add(2*time.Hour)) + deleted := seedVisibilityPost(t, db, community, author, "gcdel", "deleted header", base.Add(1*time.Hour)) + seedVisibilityAdmission(t, db, community, accepted, posts.AdmissionStatusAccepted, "bafypostv2gcacc", "") + seedVisibilityAdmission(t, db, community, pending, posts.AdmissionStatusPending, "", "") + seedVisibilityAdmission(t, db, community, deleted, posts.AdmissionStatusAccepted, "bafypostv2gcdel", "") + + // Soft-delete the third post the way the consumer does. + _, err := db.ExecContext(ctx, `UPDATE posts SET deleted_at = NOW() WHERE uri = $1`, deleted) + require.NoError(t, err) + + service := comments.NewCommentServiceWithPDSFactory( + NewCommentRepository(db), + NewUserRepository(db), + NewPostRepository(db), + NewCommunityRepository(db), + nil, nil, + ) + + header := func(t *testing.T, postURI, viewerDID string) (*comments.GetCommentsResponse, error) { + t.Helper() + var viewer *string + if viewerDID != "" { + viewer = &viewerDID + } + return service.GetComments(ctx, &comments.GetCommentsRequest{PostURI: postURI, ViewerDID: viewer}) + } + + t.Run("an accepted post serves its header", func(t *testing.T) { + resp, err := header(t, accepted, stranger) + require.NoError(t, err, "getComments must serve the header of an accepted post") + require.NotNil(t, resp.Post) + postView, ok := resp.Post.(*posts.PostView) + require.Truef(t, ok, "getComments post header is %T, not *posts.PostView", resp.Post) + assert.Equal(t, accepted, postView.URI) + }) + + t.Run("a pending post's header is hidden from a non-author", func(t *testing.T) { + _, err := header(t, pending, stranger) + require.Errorf(t, err, "getComments served a non-author the header of a PENDING post — the alternate-endpoint "+ + "leak PRD §6.2 names: a post hidden from the feed is fully readable through its comment thread") + assert.ErrorIs(t, err, comments.ErrRootNotFound, + "a pending post must be root-not-found to a non-author's getComments, the same answer post.get gives") + }) + + t.Run("a soft-deleted post no longer leaks (closes the 2026-07-29 defect)", func(t *testing.T) { + _, err := header(t, deleted, stranger) + require.Errorf(t, err, "getComments served the full header of a SOFT-DELETED post. GetByURI has no deleted_at "+ + "filter, so the withdrawn post's title/content/author are still returned through the thread endpoint — the "+ + "defect filed 2026-07-29") + assert.ErrorIs(t, err, comments.ErrRootNotFound) + }) +} + +// TestCommunityPostCountVisibility_AcceptedOnly pins that a community's +// post_count reflects accepted posts only (PRD §6.2: counts must not include +// non-accepted rows). +// +// 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. +// +// 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) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + community := visibilityCommunity(t, db, "cc") + 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", "") + seedVisibilityAdmission(t, db, community, pending, posts.AdmissionStatusPending, "", "") + seedVisibilityAdmission(t, db, community, removed, posts.AdmissionStatusRemoved, "", "rule-violation") + + count, err := countAcceptedPostsForCommunity(ctx, db, community) + 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") +} diff --git a/tests/e2e/author_post_contract_test.go b/tests/e2e/author_post_contract_test.go index d840a1c..af1963f 100644 --- a/tests/e2e/author_post_contract_test.go +++ b/tests/e2e/author_post_contract_test.go @@ -242,22 +242,20 @@ func TestAuthorPostIngestion(t *testing.T) { assert.Empty(t, view.AcceptanceURI, "a pending post has no acceptance record to point at") assert.Empty(t, view.DecisionCode, "a pending post has been refused by nobody") - // The post itself is served, attributed to the repo it arrived in. post.get - // is status-agnostic today — task 7 owes the centralized visibility - // predicate that makes a pending post invisible to non-authors (§6.2) — so - // what is asserted here is what IS true: the record was indexed, and its - // author is the DID that signed the commit rather than a field somebody - // could have written. + // The post is INVISIBLE to the anonymous public through post.get. Task 7's + // centralized visibility predicate (§6.2) hides any non-accepted postv2 from a + // non-author, and absence from the view set becomes a notFoundPost on the + // wire. This is the compensating control for the write-path flip: a post any + // author can index naming any community must not render as that community's + // content until the community admits it. (The record WAS indexed — getStatus + // above reports it pending — and its author's own privileged view of it is a + // T1 concern, since this tier can only read as the anonymous public.) served, err := p.Post(context.Background(), uri) require.NoError(t, err) - require.Falsef(t, served.NotFound, "the indexed post must be served by post.get: %+v", served) - assert.Equalf(t, author.DID, served.Author.DID, - "authorship must come from the repo the commit arrived in; the postv2 record carries no author field at all, so a different DID here means one was invented") - assert.Equal(t, community.DID, served.Community.DID) - assert.Equal(t, record.CID, served.CID, "the indexed CID must be the commit's") - assert.Equal(t, title, served.Record["title"]) - assert.Nilf(t, served.Record["author"], - "the record must not carry an author field: it is the field whose removal makes authorship unforgeable (§3.1)") + assert.Truef(t, served.NotFound, + "a PENDING post must be a notFoundPost to the anonymous public through post.get, or every feed gate is worthless against a direct permalink: %+v", served) + assert.NotEqualf(t, record.CID, served.CID, + "a notFoundPost must leak nothing about the unadmitted post it stands in for — not even the committed CID") // ---- retarget: the whole event is invalid ------------------------------ // §3.1 is explicit — a consumer must DISCARD an update that changes @@ -295,13 +293,33 @@ func TestAuthorPostIngestion(t *testing.T) { assert.Equal(t, "pending", original.Status, "the original community's decision must be untouched by an invalid update") - unchanged, err := p.Post(context.Background(), uri) - require.NoError(t, err) - assert.Equalf(t, title, unchanged.Record["title"], - "the CONTENT of a discarded event must be discarded with it: applying the new title while refusing the new community would leave the community holding a CID it never judged") - assert.NotEqual(t, retargeted, unchanged.Record["title"]) + // The CONTENT half of the discard — that the original post's row still holds + // the pre-retarget title and CID rather than the retargeted ones — is no + // longer observable here: task 7 hides a pending postv2 from the anonymous + // public, so post.get answers notFoundPost and there is no record to read the + // title out of. It is asserted at T1 against the consumer's stored row + // (internal/atproto/jetstream/postv2_consumer_test.go). What this tier proves + // is the STATE truth via getStatus: the retarget opened no admission + // elsewhere and left the original community's pending decision untouched. // ---- delete ------------------------------------------------------------- + // A delete is only observable as a TRANSITION, and a pending post is already + // invisible to the public — so to prove the delete does anything, the post is + // first ACCEPTED (making it publicly served), then deleted. The acceptance's + // pinned CID is the original record's; the retarget above was discarded, so + // the row still holds it. + acceptRkey := subjectRkey(uri) + community.PutRecord(t, acceptanceCollection, acceptRkey, acceptanceRecord(uri, record.CID)) + awaitStatus(t, p, uri, community.DID, "accepted", "the post to be accepted so its deletion is an observable transition") + + p.Await(t, "the accepted post to be served before it is deleted", func() (bool, error) { + v, err := p.Post(context.Background(), uri) + if err != nil { + return false, err + } + return !v.NotFound, nil + }) + author.DeleteExistingRecord(t, postV2Collection, rkey) gone := func() (bool, error) { diff --git a/tests/e2e/read_visibility_contract_test.go b/tests/e2e/read_visibility_contract_test.go index 64a26c2..00e15cd 100644 --- a/tests/e2e/read_visibility_contract_test.go +++ b/tests/e2e/read_visibility_contract_test.go @@ -155,6 +155,17 @@ func TestReadVisibilityContract(t *testing.T) { assert.False(t, got.NotFound, "an accepted post must be served by post.get") assert.False(t, got.Removed) + // Authorship comes from the repo the commit arrived in, not a self-asserted + // field — the postv2 record has no author field at all (§3.1). This proof + // moved here from TestAuthorPostIngestion, which can no longer make it on a + // pending post now that the predicate hides one from the anonymous public. + full, err := p.Post(context.Background(), acceptedURI) + require.NoError(t, err) + assert.Equalf(t, author.DID, full.Author.DID, + "the accepted post's author must be the repo DID that signed the commit; a different DID here means an author field was invented") + assert.Nilf(t, full.Record["author"], + "a postv2 record must carry no author field — its absence is what makes authorship unforgeable (§3.1)") + thread, err := p.Thread(context.Background(), acceptedURI, nil) require.NoError(t, err, "getComments must serve the header of an accepted post") assert.Equal(t, acceptedURI, thread.Post.URI)