diff --git a/internal/ingest/moderation.go b/internal/ingest/moderation.go index 89a8620..d01ced6 100644 --- a/internal/ingest/moderation.go +++ b/internal/ingest/moderation.go @@ -55,6 +55,13 @@ var ( // do not understand" — which, unlike most parse failures, would otherwise // have become a permanent ban. BlockExpiryUnreadable = expvar.NewInt("tidepool_block_expiry_unreadable") + // BlockUndoStaleRefused counts announced Undo{Block}s that were NOT applied + // because the ban standing for that pair outlives the one they reverse — an + // old unban replayed after the moderators banned the author again. It is its + // own number because it is the one refusal on this path that leaves a user + // EXCLUDED: if it ever moves for a community's genuine unbans, an author is + // serving a ban nobody is enforcing on the far side. + BlockUndoStaleRefused = expvar.NewInt("tidepool_block_undo_stale_refused") ) // timeNow is the clock the ban path weighs an expiry against. A package @@ -217,9 +224,11 @@ func (h *Handler) applyBan(ctx context.Context, block *ap.Object, announcer *sto "store it as a permanent ban") } // ALREADY OVER when it arrived — delayed in a queue, redelivered after an - // outage, replayed from a backfill. The ROW is still written below: it is a - // faithful account of what the moderator sent, it makes a redelivery - // idempotent, and Standing() reads the expiry so it excludes nobody. + // outage, replayed from a backfill. The ROW is still written below UNLESS a + // ban that IS in force stands for this pair: an account of a ban that ended + // is faithful and idempotent, but written over a live exclusion it ENDS one + // the moderators never lifted (Ban() holds that guard, beside the statement, + // because it is the same predicate Standing() reads). // // What a lapsed ban must NOT do is ACT. Every consequence here is one no // later activity can undo — a cancelled delivery is never re-queued, and a @@ -259,9 +268,10 @@ func (h *Handler) applyBan(ctx context.Context, block *ap.Object, announcer *sto if lapsed { BlockLapsedIgnored.Add(1) return skip(block.ID, - "announced Block expired before it arrived: the ban is recorded as sent, but it "+ - "is not in force — nothing was cancelled and nothing was removed, because both "+ - "are irreversible and this exclusion is already over") + "announced Block expired before it arrived: the ban is recorded as sent (unless a "+ + "ban that IS in force stands for this author here, which a lapsed replay may not "+ + "weaken), but it is not in force — nothing was cancelled and nothing was removed, "+ + "because both are irreversible and this exclusion is already over") } if !ban.RemoveData { return nil @@ -288,6 +298,15 @@ func (h *Handler) applyBan(ctx context.Context, block *ap.Object, announcer *sto // liftBan is Undo{Block}: the exclusion goes, and NOTHING ELSE does. // +// UNLESS THE UNDO IS STALE. An Undo can reach us twice — redriven from the +// dead-letter queue, replayed from a backfill — under an activity id the inbox +// has never seen, and by then the ban it reverses may have been replaced by a +// stronger one. Deleting the row on the strength of an old unban leaves the +// author unbanned here for good: Lemmy sends its Block once, and sends nothing +// afterwards that would say the ban is still on. The expiry the undone Block +// names is the guard (see store.CommunityBans.Lift), and it is a partial one — +// an Undo of a PERMANENT ban carries nothing to compare. +// // Content removed under removeData STAYS REMOVED. Lemmy models restoration as a // separate restore_data flag, so republishing here would reverse a decision // nobody reversed and push the author's posts back at the community that removed @@ -297,10 +316,40 @@ func (h *Handler) liftBan(ctx context.Context, block *ap.Object, announcer *stor if h.bans == nil { return fmt.Errorf("ingest: no community-ban store is wired, so this ban cannot be lifted") } - lifted, err := h.bans.Lift(ctx, announcer.DID, subjectDID) + // The expiry the UNDONE Block names, which is the only description of the + // reversed ban an Undo carries — and therefore the only thing that can tell a + // current unban from an old one redriven under a new activity id. An expiry + // that is present but UNREADABLE is passed as absent rather than refused: + // applyBan poisons on that shape because reading it wrong makes a permanent + // ban nobody asked for, while here the worst case is the pre-existing + // behaviour (an unconditional lift), and refusing would leave an author + // excluded by a ban the moderators have already reversed. + var undoneExpiry *time.Time + if expiry := block.BanExpiry(); expiry != nil && expiry.Valid { + when := expiry.Time + undoneExpiry = &when + } + lifted, retained, err := h.bans.Lift(ctx, announcer.DID, subjectDID, undoneExpiry) if err != nil { return fmt.Errorf("ingest: lift ban on %s in %s: %w", subjectDID, announcer.APGroupID, err) } + if retained { + // A ban IS standing and it outlives the one this Undo reverses, so this + // Undo is not about it: an old unban, redriven or replayed after the + // moderators banned this author again. Lifting it would be irreversible + // (Lemmy sends no second Block), so it is refused — LOUDLY, because the + // other reading is that a community's genuine unban did not take effect, + // and only an operator can tell those apart. + BlockUndoStaleRefused.Add(1) + h.logger.Warn("announced Undo{Block} reverses a ban that is no longer the one in force", + "community", announcer.APGroupID, "subject_did", subjectDID, + "undone_expiry", undoneExpiry, "activity", block.ID) + return skip(block.ID, fmt.Sprintf( + "announced Undo{Block} for %s in %s undoes a ban expiring %s, but the ban standing "+ + "there outlives it: the exclusion is KEPT, because an Undo replayed after a "+ + "re-ban would lift it permanently and Lemmy will send no second Block", + subjectDID, announcer.APGroupID, undoneExpiry)) + } if !lifted { return skip(block.ID, fmt.Sprintf( "announced Undo{Block} for %s in %s, which held no standing ban: nothing to lift "+ diff --git a/internal/ingest/moderation_ban_replay_test.go b/internal/ingest/moderation_ban_replay_test.go new file mode 100644 index 0000000..8a12466 --- /dev/null +++ b/internal/ingest/moderation_ban_replay_test.go @@ -0,0 +1,168 @@ +package ingest + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/accept" + "tidepool/internal/ap" +) + +// SECOND-OPINION (chunk 7, finding 5) — AN OLD MODERATION ACTIVITY ARRIVING +// TWICE MUST NOT UNDO THE CURRENT ONE. +// +// The inbox's dedup is by ACTIVITY ID, and every path that presents a ban a +// second time presents it under a NEW id: a dead-letter redrive re-wraps it, a +// backfill replays it, a delayed redelivery arrives after the moderators have +// already moved on. So the ban path has to survive out-of-order arrival on its +// own, and it has exactly one durable row per (community, author) to do it with. +// +// Two sequences, both ending with an author who is banned in Lemmy and unbanned +// here, permanently, with no activity left that could ever correct it: +// +// (1) temp ban lapses → permanent ban → the OLD Block is redriven. +// (2) temp ban → moderators lift it → permanent ban → the OLD Undo is redriven. +// +// Nothing else in the ban suite exercises a second Block or a second Undo for +// the same pair, so before these tests both sequences were entirely unpinned. +const ( + mbrLapsedBlock = "https://lemmy.world/activities/announce/block/mbr-lapsed" + mbrPermBlock = "https://lemmy.world/activities/announce/block/mbr-permanent" + mbrTempBlock = "https://lemmy.world/activities/announce/block/mbr-temp" + mbrUndo = "https://lemmy.world/activities/announce/undo/mbr-undo" + mbrUndoRedrive = "https://lemmy.world/activities/announce/undo/mbr-undo-redrive" + mbrPostRKey = "3lzmbrpost00001" + mbrPostAfterRKey = "3lzmbrpost00002" +) + +// TestAReplayedLapsedBlockCannotLiftAStandingBan is sequence (1). +// +// The stale Block is a faithful record of a ban that is over. Written over a +// PERMANENT ban with last-writer-wins it becomes something else entirely: the +// exclusion the moderators are currently enforcing, ended by our own redrive. +func TestAReplayedLapsedBlockCannotLiftAStandingBan(t *testing.T) { + h := newHarness(t) + world := newModerationWorld(t, h) + + // --- GIVEN: the ban that stands. Permanent, because that is what the + // moderators escalated to after the timed one ran out. + h.announceBlock(world.groupA, mbrPermBlock, mtAuthorDID, groupID, + map[string]any{"summary": "repeated spam"}) + ban, found := banFor(t, h.db, world.communityADID, mtAuthorDID) + require.True(t, found, "precondition: the permanent ban is recorded") + require.False(t, ban.expires.Valid, "precondition: with no expiry") + + // --- WHEN: the OLD Block — the timed one that ended long ago — is redriven + // from the dead-letter queue under its own activity id, so the inbox's + // dedup has never seen it and cannot stop it. + h.announceBlock(world.groupA, mbrLapsedBlock, mtAuthorDID, groupID, map[string]any{ + "expires": "2020-01-01T00:00:00Z", + "summary": "three days, served", + }) + + // --- THEN: the standing ban is untouched. + ban, found = banFor(t, h.db, world.communityADID, mtAuthorDID) + require.True(t, found, + "the ban row survives: the redrive is a message about a ban that ended, not an unban") + assert.False(t, ban.expires.Valid, + "AND IT IS STILL PERMANENT. A past expires_at written here reads as unbanned from "+ + "this moment on, and nothing re-applies it — Lemmy sent its Block once, and sends "+ + "NOTHING to say a ban is still in force") + assert.Equal(t, "repeated spam", ban.reason, + "with the standing ban's reason: the stale one describes a different, finished ban") + + // --- AND: the gate that actually excludes the author still refuses. + admitPost(t, world, mtAuthorDID, mbrPostAfterRKey, world.communityADID, + "3lzmbrrev0001", 1_775_000_030_000_001) + postURI := mbPostATURI(mtAuthorDID, mbrPostAfterRKey) + status, code := admissionFor(t, h.db, world.communityADID, postURI) + assert.Equal(t, accept.StatusRejected, status, + "the author is still banned where it counts: an admission that resumes here signs "+ + "the community's name to content from somebody it is currently excluding") + assert.Equal(t, "author-banned", code, "for the reason the moderators gave") +} + +// TestAStaleUndoBlockCannotLiftANewerBan is sequence (2). +// +// The Undo carries no id of the ban it lifts that we store, and Lemmy mints a +// fresh Block inside it — so the ONLY thing separating "the unban the moderators +// just sent" from "the unban they sent a month ago, redriven" is the expiry the +// undone Block names. An Undo of a ban that ended at T cannot be an unban of a +// ban that outlives T. +func TestAStaleUndoBlockCannotLiftANewerBan(t *testing.T) { + h := newHarness(t) + world := newModerationWorld(t, h) + + // --- GIVEN: a timed ban, and the moderators lifting it early. + h.announceBlock(world.groupA, mbrTempBlock, mtAuthorDID, groupID, + map[string]any{"expires": "2030-01-01T00:00:00Z", "summary": "two weeks"}) + require.True(t, hasBan(t, h, world), "precondition: the timed ban landed") + + h.announceUndoBlockWithExpiry(world.groupA, mbrUndo, mbrTempBlock+"/block", + mtAuthorDID, groupID, map[string]any{"expires": "2030-01-01T00:00:00Z"}) + require.False(t, hasBan(t, h, world), + "precondition: the unban lifted it — an Undo whose ban matches must still work, or "+ + "this guard has broken the ordinary case it was written to protect") + + // --- AND: the author earns a permanent ban afterwards. + h.announceBlock(world.groupA, mbrPermBlock, mtAuthorDID, groupID, + map[string]any{"summary": "came back and did it again"}) + ban, found := banFor(t, h.db, world.communityADID, mtAuthorDID) + require.True(t, found, "precondition: the permanent ban is recorded") + require.False(t, ban.expires.Valid, "precondition: with no expiry") + + // --- WHEN: the OLD Undo is redriven under a new activity id. + h.announceUndoBlockWithExpiry(world.groupA, mbrUndoRedrive, mbrTempBlock+"/block", + mtAuthorDID, groupID, map[string]any{"expires": "2030-01-01T00:00:00Z"}) + + // --- THEN: the permanent ban survives it. + ban, found = banFor(t, h.db, world.communityADID, mtAuthorDID) + require.True(t, found, + "THE NEWER BAN SURVIVES. An Undo of a two-week ban cannot lift a permanent one: the "+ + "moderators reversed a decision that is no longer the decision, and a delete that "+ + "took the row anyway leaves the author unbanned here forever — Lemmy will send no "+ + "second Block") + assert.False(t, ban.expires.Valid, "unchanged, expiry and all") + assert.Equal(t, "came back and did it again", ban.reason) + + admitPost(t, world, mtAuthorDID, mbrPostRKey, world.communityADID, + "3lzmbrrev0002", 1_775_000_031_000_001) + status, code := admissionFor(t, h.db, world.communityADID, mbPostATURI(mtAuthorDID, mbrPostRKey)) + assert.Equal(t, accept.StatusRejected, status, + "and admission still refuses, which is the only place the author feels it") + assert.Equal(t, "author-banned", code) +} + +func hasBan(t *testing.T, h *harness, world moderationWorld) bool { + t.Helper() + _, found := banFor(t, h.db, world.communityADID, mtAuthorDID) + return found +} + +// announceUndoBlockWithExpiry is announceUndoBlock with the undone Block's own +// fields spelled out — the expiry in particular, which is the only description +// of the ban an Undo carries. +func (h *harness) announceUndoBlockWithExpiry(group *remoteActor, activityID, blockActivityID, + subjectDID, target string, extra map[string]any) { + + h.t.Helper() + require.Equal(h.t, http.StatusAccepted, h.deliver(group, map[string]any{ + "id": activityID, + "type": "Announce", + "actor": group.id, + "audience": group.id, + "cc": []any{group.id + "/followers"}, + "object": map[string]any{ + "id": activityID + "/undo", + "type": ap.TypeUndo, + "actor": modActorID, + "audience": group.id, + "cc": []any{group.id}, + "object": blockActivity(group, blockActivityID, subjectDID, target, extra), + }, + })) + h.drain() +} diff --git a/internal/store/community_bans.go b/internal/store/community_bans.go index c546175..178a357 100644 --- a/internal/store/community_bans.go +++ b/internal/store/community_bans.go @@ -3,6 +3,7 @@ package store import ( "context" "database/sql" + stderrors "errors" "fmt" "time" @@ -52,8 +53,35 @@ func (r *postgresCommunityBans) Ban(ctx context.Context, ban CommunityBan) (canc // banned_at is preserved on conflict: a re-delivered Block is the same ban // arriving twice, not a new one. Everything the moderator can CHANGE by // re-issuing (the expiry, the reason, whether content goes) is taken from - // the new activity. - if _, err := tx.ExecContext(ctx, ` + // the new activity — with ONE exception, which is the WHERE clause below. + // + // A BAN THAT IS OVER MAY NOT WEAKEN ONE THAT IS IN FORCE. Without that + // clause the row is last-writer-wins, and the last writer is not the last + // moderator: a dead-letter redrive, a backfill replay and a delayed + // redelivery all present an OLD Block again under a NEW activity id, which + // the inbox's dedup cannot recognize as a repeat. Sequence — a three-day ban + // lapses, the moderators escalate to permanent, the old Block is redriven — + // and expires_at goes back to a timestamp in the past. Standing() reads + // unbanned from that instant, PERMANENTLY: Lemmy sent its Block exactly once + // and sends nothing at all to say a ban is still on, so no later activity can + // correct it. The same last-writer-wins rewrites `reason` and `remove_data`, + // which is why the guard covers the whole SET rather than the expiry alone. + // + // It is deliberately NARROW — it refuses only writes that are already dead on + // arrival. An arriving ban that IS in force is a moderator re-issuing, and + // shortening a live ban is a decision they are entitled to make; that shape + // is indistinguishable from a stale replay with the state this table holds, + // so it is allowed through (residual, and the mild one: the ban stays a ban). + // + // BOTH SIDES ARE WEIGHED AGAINST THE DATABASE'S now(), the same clock + // Standing() reads, so "in force" cannot mean one thing at the write and + // another at the read. + // + // RETURNING answers two questions in the one statement: no row comes back + // when the guard refused the update (the standing ban was left alone), and + // the boolean says whether the row that IS there now is in force. + var inForce bool + if err := tx.QueryRowContext(ctx, ` INSERT INTO community_bans ( community_did, subject_did, community_ap_id, expires_at, reason, remove_data) VALUES ($1, $2, $3, $4, $5, $6) @@ -62,23 +90,35 @@ func (r *postgresCommunityBans) Ban(ctx context.Context, ban CommunityBan) (canc expires_at = EXCLUDED.expires_at, reason = EXCLUDED.reason, remove_data = EXCLUDED.remove_data, - updated_at = now()`, + updated_at = now() + WHERE EXCLUDED.expires_at IS NULL + OR EXCLUDED.expires_at > now() + OR NOT (community_bans.expires_at IS NULL OR community_bans.expires_at > now()) + RETURNING expires_at IS NULL OR expires_at > now()`, ban.CommunityDID, ban.SubjectDID, ban.CommunityAPID, - ban.ExpiresAt, ban.Reason, ban.RemoveData); err != nil { - return 0, fmt.Errorf("ban %q in %q: %w", ban.SubjectDID, ban.CommunityDID, err) + ban.ExpiresAt, ban.Reason, ban.RemoveData).Scan(&inForce); err != nil { + if !stderrors.Is(err, sql.ErrNoRows) { + return 0, fmt.Errorf("ban %q in %q: %w", ban.SubjectDID, ban.CommunityDID, err) + } + // The guard refused: a lapsed Block over a ban that is still in force. + // Nothing was written and nothing may be cancelled — the exclusion that + // stands is not this activity's, and it did its own cancelling when it + // landed. + inForce = false } - // THE ROW IS RECORDED EITHER WAY; THE CANCELLATION IS NOT. A Block whose - // expiry has already passed when it reaches us — delayed, redelivered after - // an outage, replayed from a backfill — is a faithful record of a ban that is - // over, and storing it keeps the audit trail honest (and idempotent, since a - // later redelivery finds the same row). But it is not in force, so it must - // not cancel work by an author nobody is currently excluding: a cancelled - // delivery is never re-queued. + // THE ROW IS RECORDED EITHER WAY (unless it would weaken a standing ban); + // THE CANCELLATION IS NOT. A Block whose expiry has already passed when it + // reaches us is a faithful record of a ban that is over, and storing it keeps + // the audit trail honest (and idempotent, since a later redelivery finds the + // same row). But it is not in force, so it must not cancel work by an author + // nobody is currently excluding: a cancelled delivery is never re-queued. // - // The condition is the SAME predicate Standing() reads, kept here rather than - // at the call site so no caller can cancel on a ban that does not apply. - if ban.ExpiresAt == nil || ban.ExpiresAt.After(time.Now()) { + // The condition is the SAME predicate Standing() reads — read off the STORED + // row above, on the database's clock, rather than compared in Go against + // another one — and it is kept here rather than at the call site so no caller + // can cancel on a ban that does not apply. + if inForce { cancelled, err = cancelPendingForActorInCommunity(ctx, tx, ban.SubjectDID, ban.CommunityAPID) if err != nil { return 0, err @@ -90,9 +130,11 @@ func (r *postgresCommunityBans) Ban(ctx context.Context, ban CommunityBan) (canc return cancelled, nil } -func (r *postgresCommunityBans) Lift(ctx context.Context, communityDID, subjectDID string) (lifted bool, err error) { +func (r *postgresCommunityBans) Lift(ctx context.Context, communityDID, subjectDID string, + undoneExpiry *time.Time) (lifted, retained bool, err error) { + if communityDID == "" || subjectDID == "" { - return false, errors.NewValidationError("ban", "community_did and subject_did must not be empty") + return false, false, errors.NewValidationError("ban", "community_did and subject_did must not be empty") } // The row is DELETED rather than marked lifted. A ban is current state, not // a log — the admissions ledger already records what happened to each post — @@ -102,17 +144,50 @@ func (r *postgresCommunityBans) Lift(ctx context.Context, communityDID, subjectD // It lifts ONLY the ban. Content removed under removeData stays removed: // Lemmy models restoration as a separate restore_data flag, so republishing // here would reverse a decision nobody reversed. - result, err := r.db.ExecContext(ctx, - `DELETE FROM community_bans WHERE community_did = $1 AND subject_did = $2`, - communityDID, subjectDID) - if err != nil { - return false, fmt.Errorf("lift ban on %q in %q: %w", subjectDID, communityDID, err) - } - affected, err := result.RowsAffected() - if err != nil { - return false, fmt.Errorf("lift ban on %q in %q: rows affected: %w", subjectDID, communityDID, err) + // + // THE DELETE IS CONDITIONAL, for the reason Ban()'s upsert is guarded: an + // Undo{Block} can be redriven or replayed under a new activity id long after + // the ban it reversed stopped being the ban in force, and an unconditional + // delete then lifts the NEWER one — permanently, since Lemmy will not send a + // second Block. + // + // undoneExpiry is the only description of the reversed ban an Undo carries + // (the row holds no activity id to match, and Lemmy mints a fresh Block + // inside the Undo, so ids cannot be compared). The rule it supports: an Undo + // of a ban that ended at T cannot lift a ban that outlives T — a permanent + // row (NULL) or one expiring later is a STRONGER ban than the one being + // undone, so it is not the ban this Undo is about. + // + // What is protected is a ban IN FORCE, exactly as in Ban(), and on the same + // clock: a row that has lapsed excludes nobody, so there is nothing there for + // a stale Undo to take away and the delete proceeds. Without that third + // clause the guard would fire on rows whose removal changes nothing, and an + // operator would be handed a warning about an author who is not banned. + // + // RESIDUAL, and it is real: an Undo naming NO expiry — the shape Lemmy sends + // for a permanent ban, and the common one — is indistinguishable from its own + // replay, so it still lifts whatever stands. Closing that needs state this + // table does not hold (the activity id of the ban in force, or the moment the + // Undo was first seen); it is not inferable from the row. + if err := r.db.QueryRowContext(ctx, ` + WITH standing AS ( + SELECT 1 FROM community_bans + WHERE community_did = $1 AND subject_did = $2 + ), gone AS ( + DELETE FROM community_bans + WHERE community_did = $1 AND subject_did = $2 + AND ($3::timestamptz IS NULL + OR (expires_at IS NOT NULL AND expires_at <= $3::timestamptz) + OR NOT (expires_at IS NULL OR expires_at > now())) + RETURNING 1 + ) + SELECT EXISTS (SELECT 1 FROM gone), EXISTS (SELECT 1 FROM standing)`, + communityDID, subjectDID, undoneExpiry).Scan(&lifted, &retained); err != nil { + return false, false, fmt.Errorf("lift ban on %q in %q: %w", subjectDID, communityDID, err) } - return affected > 0, nil + // retained is "a ban was there and is STILL there" — a refusal, not a + // no-op — and the caller has to be able to say which of the two happened. + return lifted, retained && !lifted, nil } func (r *postgresCommunityBans) Standing(ctx context.Context, communityDID, subjectDID string) (bool, error) { diff --git a/internal/store/community_bans_replay_test.go b/internal/store/community_bans_replay_test.go new file mode 100644 index 0000000..5dd2be6 --- /dev/null +++ b/internal/store/community_bans_replay_test.go @@ -0,0 +1,326 @@ +package store + +import ( + "context" + "database/sql" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// SECOND-OPINION (chunk 7, finding 5) — A BAN ROW IS NOT LAST-WRITER-WINS. +// +// Every ban write is an UPSERT on (community_did, subject_did), and the activity +// that carries it can arrive at any time in any order: Lemmy's Block is sent +// ONCE, but our own dead-letter redrive, a backfill replay and a delayed +// redelivery can all present an OLD Block again, under a new activity id that +// the inbox's dedup cannot recognize. Taking EXCLUDED.expires_at unconditionally +// makes the last message to arrive the current ban — so a lapsed three-day ban +// redriven a month later overwrites the PERMANENT ban the moderators issued in +// the meantime, Standing() reads unbanned from that moment on, and NOTHING ever +// re-applies it: the community already sent its Block, and no activity exists +// that says "that ban is still on". +// +// The rule these tests pin: an arriving ban that is NOT IN FORCE may never +// weaken one that IS. It is deliberately narrow — an arriving ban that IS in +// force is a moderator re-issuing, expiry included, and shortening a ban is a +// decision they are entitled to make. +const ( + cbrCommunityDID = "did:plc:cbrcommunity00001" + cbrCommunityAP = "https://lemmy.world/c/technology" + cbrSubjectDID = "did:plc:cbrsubject000001" +) + +// storedBan is the row as the table holds it — the only place the difference +// between "recorded" and "in force" is visible. +type storedBan struct { + communityAPID string + expires sql.NullTime + reason string + removeData bool +} + +func storedBanFor(t *testing.T, database *sql.DB, communityDID, subjectDID string) (storedBan, bool) { + t.Helper() + var ban storedBan + err := database.QueryRowContext(context.Background(), ` + SELECT community_ap_id, expires_at, reason, remove_data + FROM community_bans + WHERE community_did = $1 AND subject_did = $2`, + communityDID, subjectDID).Scan( + &ban.communityAPID, &ban.expires, &ban.reason, &ban.removeData) + if err == sql.ErrNoRows { + return storedBan{}, false + } + require.NoError(t, err, "read the ban row for %s in %s", subjectDID, communityDID) + return ban, true +} + +// TestCommunityBans_ALapsedReplayCannotWeakenAStandingBan is the finding itself. +// +// A temporary ban lapses; the moderators escalate to a permanent one; the OLD +// Block is redriven. Under last-writer-wins the permanent exclusion is gone — +// silently, and for good. +func TestCommunityBans_ALapsedReplayCannotWeakenAStandingBan(t *testing.T) { + database := standingTestDB(t) + repo := NewCommunityBans(database) + ctx := context.Background() + + // The ban that stands: permanent, no expiry, the moderators' current ruling. + _, err := repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: cbrSubjectDID, + CommunityAPID: cbrCommunityAP, + Reason: "repeated spam", + }) + require.NoError(t, err) + standing, err := repo.Standing(ctx, cbrCommunityDID, cbrSubjectDID) + require.NoError(t, err) + require.True(t, standing, "precondition: the permanent ban is in force") + + // The replay: the older, already-expired Block arriving again. + lapsed := time.Now().Add(-72 * time.Hour) + cancelled, err := repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: cbrSubjectDID, + CommunityAPID: cbrCommunityAP, + ExpiresAt: &lapsed, + Reason: "a three-day ban that ended before this arrived", + RemoveData: true, + }) + require.NoError(t, err, "a stale replay is not an error: it is a message we decline to apply") + assert.EqualValues(t, 0, cancelled, + "and it cancels nothing, because it is not in force") + + standing, err = repo.Standing(ctx, cbrCommunityDID, cbrSubjectDID) + require.NoError(t, err) + assert.True(t, standing, + "THE STANDING BAN SURVIVES. A lapsed Block that overwrote expires_at would end a "+ + "permanent exclusion the moderators never lifted — and nothing could ever restore "+ + "it, because Lemmy sent its Block exactly once and sends nothing at all when a "+ + "ban is still on") + + ban, found := storedBanFor(t, database, cbrCommunityDID, cbrSubjectDID) + require.True(t, found, "the row is still there") + assert.False(t, ban.expires.Valid, + "with its PERMANENT expiry intact: a past timestamp here is the bug, whether or not "+ + "any reader has noticed yet") + assert.Equal(t, "repeated spam", ban.reason, + "and the standing ban's reason, not the stale one's: the same last-writer-wins that "+ + "moves the expiry rewrites the words an operator reads when asking why") + assert.False(t, ban.removeData, + "and its removeData: a replayed flag would misreport what was done to this author's "+ + "content under a ban that is not the one in force") +} + +// TestCommunityBans_ALapsedBanIsStillRecordedWhenNothingStands keeps the guard +// honest in the other direction. A lapsed Block against a pair with NO standing +// ban is still written — it is a faithful account of what the moderators sent, +// and the row is what makes a redelivery idempotent. Only weakening is refused. +func TestCommunityBans_ALapsedBanIsStillRecordedWhenNothingStands(t *testing.T) { + database := standingTestDB(t) + repo := NewCommunityBans(database) + ctx := context.Background() + + lapsed := time.Now().Add(-72 * time.Hour) + cancelled, err := repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: cbrSubjectDID, + CommunityAPID: cbrCommunityAP, + ExpiresAt: &lapsed, + Reason: "three days, served", + }) + require.NoError(t, err) + assert.EqualValues(t, 0, cancelled, "a ban that is over cancels nothing") + + ban, found := storedBanFor(t, database, cbrCommunityDID, cbrSubjectDID) + require.True(t, found, + "the row IS written: the audit trail is the point, and without the row a redelivery "+ + "is not idempotent") + require.True(t, ban.expires.Valid, "carrying the expiry it arrived with") + assert.True(t, ban.expires.Time.Before(time.Now()), "in the past") + + standing, err := repo.Standing(ctx, cbrCommunityDID, cbrSubjectDID) + require.NoError(t, err) + assert.False(t, standing, "and it excludes nobody: the expiry IS the lift") + + // A second lapsed delivery of the same ban: still nothing in force, so the + // guard must not fire — the row keeps taking the newest account of itself. + _, err = repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: cbrSubjectDID, + CommunityAPID: cbrCommunityAP, + ExpiresAt: &lapsed, + Reason: "three days, served (redelivered)", + }) + require.NoError(t, err) + ban, _ = storedBanFor(t, database, cbrCommunityDID, cbrSubjectDID) + assert.Equal(t, "three days, served (redelivered)", ban.reason, + "a lapsed row is not protected from a lapsed write: nothing is in force, so there "+ + "is nothing to weaken") +} + +// TestCommunityBans_AStaleUndoCannotLiftAStrongerBan is the same finding at the +// other door. Undo{Block} DELETES the row, so a redriven old unban lifts +// whatever is standing — including the permanent ban issued after it. +// +// The Undo carries no ban id we hold (Lemmy mints a fresh Block inside it), so +// the only thing to compare is the expiry it names: an Undo of a ban that ended +// at T is not an unban of a ban that outlives T. +func TestCommunityBans_AStaleUndoCannotLiftAStrongerBan(t *testing.T) { + database := standingTestDB(t) + repo := NewCommunityBans(database) + ctx := context.Background() + + twoWeeks := time.Now().Add(14 * 24 * time.Hour) + _, err := repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: cbrSubjectDID, + CommunityAPID: cbrCommunityAP, + ExpiresAt: &twoWeeks, + Reason: "two weeks", + }) + require.NoError(t, err) + + // THE ORDINARY UNBAN STILL WORKS. This assertion is the guard's own guard: + // a Lift that refused the matching Undo would leave every timed ban standing + // after the moderators reversed it, which is the harm this finding is about + // pointing the other way. + lifted, retained, err := repo.Lift(ctx, cbrCommunityDID, cbrSubjectDID, &twoWeeks) + require.NoError(t, err) + require.True(t, lifted, "an Undo of the ban that IS standing lifts it") + require.False(t, retained) + + // The moderators ban the author again, permanently. + _, err = repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: cbrSubjectDID, + CommunityAPID: cbrCommunityAP, + Reason: "came back and did it again", + }) + require.NoError(t, err) + + // The redrive: the SAME old Undo, arriving again. + lifted, retained, err = repo.Lift(ctx, cbrCommunityDID, cbrSubjectDID, &twoWeeks) + require.NoError(t, err, "a stale unban is not an error: it is a message we decline to apply") + assert.False(t, lifted, + "AN UNDO OF THE TWO-WEEK BAN CANNOT LIFT THE PERMANENT ONE: it reverses a decision "+ + "that is no longer the decision, and the delete is irreversible — Lemmy will send "+ + "no second Block to re-apply what this removed") + assert.True(t, retained, + "and the caller is TOLD the ban was kept: 'nothing to lift' and 'we refused to lift "+ + "this' are opposite findings for an operator, and only one of them means an author "+ + "may still be excluded here after a genuine unban") + + standing, err := repo.Standing(ctx, cbrCommunityDID, cbrSubjectDID) + require.NoError(t, err) + assert.True(t, standing, "the permanent ban still stands") + + // An Undo naming NO expiry is the documented residual: nothing in the row + // distinguishes it from its own replay, so it lifts. Pinned so the gap is + // visible rather than assumed closed. + lifted, retained, err = repo.Lift(ctx, cbrCommunityDID, cbrSubjectDID, nil) + require.NoError(t, err) + assert.True(t, lifted, + "an Undo of a PERMANENT ban carries nothing to compare, so it lifts — the residual "+ + "this guard cannot close without storing the ban's activity id or the moment the "+ + "Undo was first seen") + assert.False(t, retained) + + // A row that has LAPSED is not protected: it excludes nobody, so a stale + // Undo takes nothing away by removing it — and a guard that fired here would + // warn an operator about an author who is not banned. + longAgo := time.Now().Add(-30 * 24 * time.Hour) + _, err = repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: cbrSubjectDID, + CommunityAPID: cbrCommunityAP, + ExpiresAt: &longAgo, + Reason: "served, a month ago", + }) + require.NoError(t, err) + evenLongerAgo := time.Now().Add(-60 * 24 * time.Hour) + lifted, retained, err = repo.Lift(ctx, cbrCommunityDID, cbrSubjectDID, &evenLongerAgo) + require.NoError(t, err) + assert.True(t, lifted, + "an Undo of an older ban still lifts a row that OUTLIVES it but has lapsed anyway: "+ + "what the guard protects is a ban IN FORCE, not a row — refusing here would hand "+ + "an operator a warning about an author nobody is excluding") + assert.False(t, retained) + + // And nothing to lift at all is a third answer, distinct from both. + lifted, retained, err = repo.Lift(ctx, cbrCommunityDID, cbrSubjectDID, nil) + require.NoError(t, err) + assert.False(t, lifted, "a re-delivered Undo with no ban left finds nothing") + assert.False(t, retained, "and retains nothing, because there was nothing there") +} + +// TestCommunityBans_AnInForceBanStillRewritesTheRow is the guard's blast-radius +// test: the ONLY thing refused is a lapsed write over a standing ban. Everything +// a moderator can genuinely re-issue must still land, including a re-issue that +// SHORTENS a permanent ban to a timed one — indistinguishable from a stale +// replay except that this one is still in force. +func TestCommunityBans_AnInForceBanStillRewritesTheRow(t *testing.T) { + database := standingTestDB(t) + repo := NewCommunityBans(database) + ctx := context.Background() + + _, err := repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: cbrSubjectDID, + CommunityAPID: cbrCommunityAP, + Reason: "permanent, for now", + }) + require.NoError(t, err) + + // Re-issued as a timed ban that has NOT run out. + future := time.Now().Add(48 * time.Hour) + _, err = repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: cbrSubjectDID, + CommunityAPID: cbrCommunityAP, + ExpiresAt: &future, + Reason: "reduced to two days on appeal", + }) + require.NoError(t, err) + + ban, found := storedBanFor(t, database, cbrCommunityDID, cbrSubjectDID) + require.True(t, found) + require.True(t, ban.expires.Valid, + "an IN-FORCE re-issue still takes effect: the guard is about bans that are OVER, and "+ + "a guard that also froze live re-issues would leave moderators unable to shorten "+ + "a ban they had already issued") + assert.WithinDuration(t, future, ban.expires.Time, time.Second) + assert.Equal(t, "reduced to two days on appeal", ban.reason) + + standing, err := repo.Standing(ctx, cbrCommunityDID, cbrSubjectDID) + require.NoError(t, err) + assert.True(t, standing, "and it is still a ban") + + // And the reverse order: a permanent ban over a row that has lapsed. The + // escalation this whole finding is about must land. + lapsed := time.Now().Add(-1 * time.Hour) + _, err = repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: "did:plc:cbrsubject000002", + CommunityAPID: cbrCommunityAP, + ExpiresAt: &lapsed, + Reason: "served", + }) + require.NoError(t, err) + _, err = repo.Ban(ctx, CommunityBan{ + CommunityDID: cbrCommunityDID, + SubjectDID: "did:plc:cbrsubject000002", + CommunityAPID: cbrCommunityAP, + Reason: "escalated to permanent", + }) + require.NoError(t, err) + ban, found = storedBanFor(t, database, cbrCommunityDID, "did:plc:cbrsubject000002") + require.True(t, found) + assert.False(t, ban.expires.Valid, + "a permanent ban over a lapsed row is the moderators escalating, and it must land — "+ + "this is the very sequence the replay guard exists to protect") + assert.Equal(t, "escalated to permanent", ban.reason) +} diff --git a/internal/store/interfaces.go b/internal/store/interfaces.go index 2e98538..8ea77fe 100644 --- a/internal/store/interfaces.go +++ b/internal/store/interfaces.go @@ -283,12 +283,31 @@ type CommunityBans interface { // account of what the moderator sent, and a redelivery must find the same row // — but it cancels nothing, because it is not in force and a cancelled // delivery is never re-queued. + // + // AND IT NEVER WEAKENS A BAN THAT IS IN FORCE. An old Block redriven from the + // dead-letter queue or replayed from a backfill arrives under a new activity + // id that inbox dedup cannot recognize; written last-writer-wins over the + // permanent ban the moderators escalated to, its past expiry reads as + // unbanned forever after, and no activity exists that could correct it. So a + // write that is already expired on arrival is dropped when a standing ban + // would lose by it — expiry, reason and removeData together. Ban(ctx context.Context, ban CommunityBan) (cancelled int64, err error) - // Lift removes the ban (Undo{Block}), reporting whether one was standing. + // Lift removes the ban (Undo{Block}). lifted says a ban was removed; + // retained says one was there and was DELIBERATELY LEFT — the two are + // different answers and both are false only when there was nothing at all. + // // It lifts ONLY the exclusion: content removed under removeData stays // removed, because Lemmy models restoration as a separate restore_data flag. - Lift(ctx context.Context, communityDID, subjectDID string) (lifted bool, err error) + // + // undoneExpiry is the expiry the UNDONE Block named (nil when it named none), + // and it is the replay guard: an Undo can be redriven under a new activity id + // after the ban it reversed has been replaced, and an unconditional delete + // then lifts the newer ban forever. An Undo of a ban ending at T therefore + // cannot remove a ban that outlives T. An Undo naming NO expiry still lifts + // unconditionally — nothing in the row can distinguish it from its own + // replay. + Lift(ctx context.Context, communityDID, subjectDID string, undoneExpiry *time.Time) (lifted, retained bool, err error) // Standing reports whether the author is CURRENTLY banned from the // community — expiry included, because a lapsed ban must read exactly like