diff --git a/internal/accept/admissions.go b/internal/accept/admissions.go --- a/internal/accept/admissions.go +++ b/internal/accept/admissions.go @@ -197,9 +197,14 @@ return &adm, nil } // GetByPostURI returns the admission for a post at-uri alone — the readmit and -// admin-list path, which knows the post but not necessarily its community. The -// post_uri is globally unique (it embeds the author repo), so at most one row -// matches. A miss satisfies errors.IsNotFound. +// admin-list path, which knows the post but not necessarily its community. +// +// At most one row matches, and that is a SCHEMA guarantee, not an assumption +// about at-uris: migration 031's unique index on post_uri. It has to be, because +// this query has no ORDER BY — if two communities could hold one post_uri, +// postgres would pick the winner, and boundCommunityOf (which reads the post's +// community binding through this call) would be asking a question with two +// answers. A miss satisfies errors.IsNotFound. func (a *Admissions) GetByPostURI(ctx context.Context, postURI string) (*Admission, error) { var adm Admission err := a.db.QueryRowContext(ctx, ` diff --git a/internal/accept/community_move_test.go b/internal/accept/community_move_test.go new file mode 100644 --- /dev/null +++ b/internal/accept/community_move_test.go @@ -0,0 +1,198 @@ +package accept + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/errors" + "tidepool/internal/store" +) + +// Community immutability must be decided BEFORE the record is judged on its +// merits. The lexicon check and the immutability check answer different +// questions — "is this a well-formed post?" and "may this event touch this +// community at all?" — and only the second one guards WHERE a decision gets +// written. Deciding merit first means a malformed community-moving edit gets a +// verdict (lexicon-invalid) that AdmitPost then records under the EVENT's +// community, which is precisely the community the move was refused into. +// +// These pin both halves of that: the never-accepted post (a second ledger row +// under the target) and the accepted one (a removal signed into a community +// that never accepted the post). + +// A rejected post edited to name a DIFFERENT community while ALSO carrying a +// lexicon violation must still be discarded as a community move. The lexicon +// verdict must not pre-empt the immutability discard: a verdict is recorded +// under the event's community, so pre-empting writes a SECOND admissions row for +// one post_uri — the invariant GetByPostURI's single-row read depends on. +func TestH3_MalformedCommunityMoveOfRejectedPostWritesNothingToTheTarget(t *testing.T) { + conn := acceptanceDB(t) + ctx := context.Background() + seedBridgedCommunity(t, conn) // community A + seedBridgedCommunityB(t, conn) // community B + repos := newRepos(t, conn) + enq := realEnqueuer(t, conn) + dispatcher := wireDispatcher(t, conn, engineWith(t, conn, repos, enq), enq) + + // Opted out → REJECTED in community A (a ledger row under A, no outbound row). + _, err := store.NewFederationPrefs(conn).Upsert(ctx, store.FederationPref{ + DID: acAuthorDID, Source: store.FederationPrefSourceRecord, + }) + require.NoError(t, err) + require.NoError(t, dispatcher.HandleEvent(ctx, + postEvent("create", acPostRKey, acRevCreate, acPostCID, acPostTimeUS, pv2Record()))) + require.Equal(t, 1, admissionRowCount(t, conn, acCommunityDID, acPostURI), + "precondition: rejected in A") + + // The move, carrying a lexicon violation too (createdAt must be a datetime + // string). Both checks would fire; only the immutability one may decide. + require.NoError(t, dispatcher.HandleEvent(ctx, + postEvent("update", acPostRKey, acRevUpdate, acPostCID2, acPostTimeUS+1, + pv2Record(func(r map[string]any) { + r["community"] = acCommunityB_DID + r["createdAt"] = 12345 + })))) + + assert.Zero(t, admissionRowCount(t, conn, acCommunityB_DID, acPostURI), + "a lexicon-invalid community-moving edit must write NOTHING to the target community: "+ + "the verdict is recorded under the EVENT's community, so deciding merit before "+ + "immutability files the rejection under the community the move was refused into") + assert.Equal(t, 1, admissionRowsForPost(t, conn, acPostURI), + "one post_uri must never hold admissions rows under two communities") + + status, code := admissionOf(t, conn, acCommunityDID, acPostURI) + assert.Equal(t, StatusRejected, status, + "the original community's row keeps its status; a discarded event decides nothing new") + assert.Equal(t, DecisionCommunityImmutable, code, + "the attempted move is annotated on the ORIGINAL community's row (H3 semantics), not "+ + "recorded as a lexicon rejection somewhere else") +} + +// The same malformed move against an ACCEPTED post. Deciding merit first turns +// the event into a re-admission FAILURE, and removeAccepted takes the EVENT's +// community — so a removal record gets signed into a community that never +// accepted the post, while community A's outbound row is tombstoned and a +// Delete{Page} is enqueued at A with A's acceptance left standing. +func TestH3_MalformedCommunityMoveOfAcceptedPostSignsNothingInTheTarget(t *testing.T) { + conn := acceptanceDB(t) + ctx := context.Background() + seedBridgedCommunity(t, conn) + seedBridgedCommunityB(t, conn) + repos := newRepos(t, conn) + enq := realEnqueuer(t, conn) + dispatcher := wireDispatcher(t, conn, engineWith(t, conn, repos, enq), enq) + + admittedCreate(t, dispatcher) // accepted into community A + require.Equal(t, 1, countRows(t, conn, "outbound_activities"), "precondition: one Create{Page}") + + require.NoError(t, dispatcher.HandleEvent(ctx, + postEvent("update", acPostRKey, acRevUpdate, acPostCID2, acPostTimeUS+1, + pv2Record(func(r map[string]any) { + r["community"] = acCommunityB_DID + r["createdAt"] = 12345 + }))), + "a community-moving edit is discarded whole, however malformed it also is") + + // Nothing may be signed under community B — not an acceptance, not a removal. + _, movedIn := acceptanceSubjectCID(t, repos, acCommunityB_DID, acPostURI) + assert.False(t, movedIn, "no acceptance may appear in the target community") + _, removedThere := removalStandsAt(t, repos, acCommunityB_DID, acPostURI) + assert.False(t, removedThere, + "and NO removal may appear either: a removal in B is B's key signing a decision about "+ + "a post B never accepted") + assert.Zero(t, countWhere(t, conn, "repo_state", "did", acCommunityB_DID), + "the target community's repo must not even be genesis-committed: the move never "+ + "reached a signing operation") + assert.Zero(t, admissionRowCount(t, conn, acCommunityB_DID, acPostURI), + "and no ledger row under the target") + assert.Equal(t, 1, admissionRowsForPost(t, conn, acPostURI), + "one post_uri, one community, one row") + + // Community A is left exactly as it was: acceptance standing on the original + // version, nothing withdrawn, nothing enqueued. + cid, ok := acceptanceSubjectCID(t, repos, acCommunityDID, acPostURI) + require.True(t, ok, "the original acceptance stands") + assert.Equal(t, acPostCID, cid, "still pinning the version A accepted") + _, removedHere := removalStandsAt(t, repos, acCommunityDID, acPostURI) + assert.False(t, removedHere, "and no removal stands in A either") + assert.Zero(t, activityKindCount(t, conn, "Delete"), + "no Delete{Page} may be enqueued: the event was discarded, not re-decided") + assert.Equal(t, 1, countRows(t, conn, "outbound_activities"), + "a discarded community-move enqueues nothing") + + stored, err := store.NewOutboundObjects(conn).GetByATURI(ctx, acPostURI) + require.NoError(t, err) + assert.False(t, stored.IsTombstoned(), + "A's outbound state must not be tombstoned by an event decided about B") + assert.Equal(t, acCommunityDID, stored.CommunityDID, "and it still names community A") + + status, _ := admissionOf(t, conn, acCommunityDID, acPostURI) + assert.Equal(t, StatusAccepted, status, + "the post is still accepted in A; the move decided nothing about its admission") +} + +// The invariant the engine's single-row reads depend on — one post_uri, one +// community — is asserted in docs and tests but was enforced nowhere: migration +// 021's PK is (community_did, post_uri), so two communities holding one post_uri +// was a legal insert. Any writer with the ordering bug above silently produced +// it, and GetByPostURI then returned whichever row postgres felt like. +func TestAdmissionsPostURIIsGloballyUnique(t *testing.T) { + conn := acceptanceDB(t) + ctx := context.Background() + admissions := NewAdmissions(conn) + + require.NoError(t, admissions.Record(ctx, Admission{ + AuthorDID: acAuthorDID, + CommunityDID: acCommunityDID, + PostURI: acPostURI, + Status: StatusRejected, + DecisionCode: DecisionOptedOut, + }), "the first community's decision is recorded normally") + + err := admissions.Record(ctx, Admission{ + AuthorDID: acAuthorDID, + CommunityDID: acCommunityB_DID, + PostURI: acPostURI, + Status: StatusRejected, + DecisionCode: DecisionLexiconInvalid, + }) + require.Error(t, err, + "a second admissions row for the same post_uri under a DIFFERENT community must be "+ + "refused by the database: a post is bound to one community forever, and every "+ + "post_uri-keyed read (GetByPostURI, boundCommunityOf, Readmit) is a single-row "+ + "query that silently picks a winner once two rows exist") + + assert.Equal(t, 1, admissionRowsForPost(t, conn, acPostURI), + "exactly one row survives") + adm, gerr := admissions.GetByPostURI(ctx, acPostURI) + require.NoError(t, gerr) + assert.Equal(t, acCommunityDID, adm.CommunityDID, + "and it is the community the post was actually bound to") + + // The (community_did, post_uri) upsert path must still work: re-recording the + // SAME row is an update, not a unique violation. + require.NoError(t, admissions.Record(ctx, Admission{ + AuthorDID: acAuthorDID, + CommunityDID: acCommunityDID, + PostURI: acPostURI, + Status: StatusRejected, + DecisionCode: DecisionCommunityImmutable, + }), "the unique index must not break the ledger's own idempotent re-record") + _, code := admissionOf(t, conn, acCommunityDID, acPostURI) + assert.Equal(t, DecisionCommunityImmutable, code) + + // A different post_uri under the second community is unaffected. + otherURI := "at://" + acAuthorDID + "/social.coves.community.postv2/3lzpostuniq02" + require.NoError(t, admissions.Record(ctx, Admission{ + AuthorDID: acAuthorDID, + CommunityDID: acCommunityB_DID, + PostURI: otherURI, + Status: StatusAccepted, + }), "the index constrains post_uri, not the community") + _, err = admissions.GetByPostURI(ctx, otherURI) + require.NoError(t, err) + require.False(t, errors.IsNotFound(err)) +} diff --git a/internal/accept/engine.go b/internal/accept/engine.go --- a/internal/accept/engine.go +++ b/internal/accept/engine.go @@ -333,11 +333,13 @@ if err != nil { return err } - // Decide admission. The order is deliberate (fail closed first, cheap policy - // last): lexicon-validate → community-immutable → community-followed → opt-out - // → paused → title → rate cap. A discard means the whole event is dropped - // (nothing written to either community); a non-empty code is a rejection/ - // removal cause. + // Decide admission. The order is deliberate (jurisdiction first, then fail + // closed, then cheap policy): community-immutable → lexicon-validate → + // community-followed → ban → opt-out → paused → title → rate cap. A discard + // means the whole event is dropped (nothing written to either community); a + // non-empty code is a rejection/removal cause, and because immutability is + // settled first, a non-empty code is always ABOUT the community this post is + // already bound to (or a post with no binding yet). code, discard, err := e.decide(ctx, did, commit, communityDID, boundCommunity) if err != nil { return err @@ -451,7 +453,26 @@ // author's own preferences are consulted. func (e *Engine) decide(ctx context.Context, did string, commit *consume.CommitEvent, communityDID, boundCommunity string) (code string, discard bool, err error) { - // 1. Strict lexicon validation of the native input, bound to the postv2 + // 1. Community immutability, FIRST. The lexicon marks `community` immutable: + // an UPDATE that names a different community than the post was bound to is a + // retarget, which means writing a NEW post — so the whole event is discarded, + // not partially applied. + // + // It runs before every merit check, INCLUDING lexicon validation, because it + // is the only check that decides WHERE a verdict may be written rather than + // what the verdict is. Every other check returns a code, and AdmitPost records + // that code under the EVENT's community — so a merit check that fires first on + // a moving edit files its rejection under the very community the move is about + // to be refused into: a second admissions row for one post_uri (never-accepted + // post), or a removal record signed with the TARGET community's key for a post + // it never accepted (accepted post), while the original community's outbound + // row is tombstoned and a Delete enqueued at it. Refusing a community move + // needs no valid record: the two DIDs disagree, and that is the whole finding. + if boundCommunity != "" && boundCommunity != communityDID { + return "", true, nil + } + + // 2. Strict lexicon validation of the native input, bound to the postv2 // schema — fail closed. A marshal/unmarshal fault is an INTERNAL error // (retryable), NOT a permanent lexicon-invalid verdict. valid, err := e.lexiconValid(commit.Record) @@ -460,14 +481,6 @@ return "", false, err } if !valid { return DecisionLexiconInvalid, false, nil - } - - // 2. Community immutability. The lexicon marks `community` immutable: an - // UPDATE that names a different community than the post was bound to is a - // retarget, which means writing a NEW post — so the whole event is discarded, - // not partially applied. - if boundCommunity != "" && boundCommunity != communityDID { - return "", true, nil } // 3. Community follow gate (SECURITY): a communities row's mere existence is diff --git a/internal/db/migrations/031_admissions_post_uri_unique.sql b/internal/db/migrations/031_admissions_post_uri_unique.sql new file mode 100644 --- /dev/null +++ b/internal/db/migrations/031_admissions_post_uri_unique.sql @@ -0,0 +1,57 @@ +-- +goose Up +-- One post_uri, one community — enforced by the schema instead of by belief. +-- +-- Migration 021 keys this table (community_did, post_uri), which says "one row +-- per post PER COMMUNITY". But a postv2 is bound to ONE community forever (the +-- lexicon marks `community` immutable), and the engine reads that binding back +-- by post_uri ALONE: GetByPostURI, boundCommunityOf, and Readmit are all +-- single-row queries over `WHERE post_uri = $1` with no ORDER BY. Two rows for +-- one post_uri does not surface as an error anywhere — it surfaces as postgres +-- picking a winner, which means the community a post is "bound" to can change +-- between two reads of the same ledger. +-- +-- The invariant was asserted in the H3 tests and in this table's own comments +-- and enforced nowhere, so the one writer that violated it (decide() judging a +-- record's merit BEFORE refusing a community move, filing the resulting verdict +-- under the TARGET community) did so silently. That ordering is fixed in +-- internal/accept/engine.go; this index is what makes the next such writer fail +-- loudly at the INSERT instead of corrupting the binding. +-- +-- It also makes the ledger's own upsert honest: record() says +-- ON CONFLICT (community_did, post_uri) DO UPDATE, which quietly turns a +-- cross-community duplicate into an INSERT. With this index that insert raises +-- a unique violation and the caller sees it. + +-- Any pre-existing duplicate is the bug's residue, and it must be cleared before +-- the index can exist. The row KEPT is the one the post is actually bound to: +-- the community that holds the post's outbound_objects row (the accepted +-- binding) if there is one, else the oldest decision — which is the community +-- that decided first, and therefore the one a later moving edit was refused +-- into. The discarded rows are, by construction, decisions written about a +-- community that never accepted this post. +DELETE FROM admissions a + WHERE a.community_did <> ( + SELECT keep.community_did + FROM ( + SELECT DISTINCT ON (candidate.post_uri) + candidate.post_uri, candidate.community_did + FROM admissions candidate + LEFT JOIN outbound_objects o + ON o.at_uri = candidate.post_uri + AND o.community_did = candidate.community_did + ORDER BY candidate.post_uri, + (o.at_uri IS NOT NULL) DESC, + candidate.created_at, + candidate.community_did + ) keep + WHERE keep.post_uri = a.post_uri + ); + +CREATE UNIQUE INDEX idx_admissions_post_uri ON admissions (post_uri); + +-- +goose Down +-- The index goes; the rows deleted above do NOT come back. They were decisions +-- recorded about a community that never accepted the post, and re-inserting them +-- is neither possible nor desirable — the ledger is a debug/admin surface that +-- re-derives from a replay. +DROP INDEX IF EXISTS idx_admissions_post_uri;