diff --git a/cmd/server/cdn_purge_wiring.go b/cmd/server/cdn_purge_wiring.go index 5cac00c..74467f8 100644 --- a/cmd/server/cdn_purge_wiring.go +++ b/cmd/server/cdn_purge_wiring.go @@ -71,7 +71,7 @@ func buildCDNPurgeQueue(cfg *config.Config, apiBase string, store moderation.CDN // buildCDNPurger returns nil when no Cloudflare edge invalidation is configured. func buildCDNPurger(purge config.CDNPurgeConfig, apiBase string) (moderation.CDNPurger, error) { if !purge.Enabled() { - slog.Info("CDN purge is off; shared caches can keep an image for at most a day") + slog.Info("CDN purge is off; responses now allow shared caches one day (copies cached under the old one-year header need the one-time Purge Everything, PRD_CSAM_SCANNING.md rollout step 4)") return nil, nil } return imageproxy.NewCloudflarePurger(imageproxy.CloudflarePurgerConfig{ diff --git a/cmd/server/image_proxy_wiring.go b/cmd/server/image_proxy_wiring.go index ecbdeb9..e68c4c8 100644 --- a/cmd/server/image_proxy_wiring.go +++ b/cmd/server/image_proxy_wiring.go @@ -2,6 +2,9 @@ package main import ( "context" + "log/slog" + "sync" + "time" "Coves/internal/atproto/identity" "Coves/internal/config" @@ -34,7 +37,44 @@ func buildImageProxyService(cfg imageproxy.Config, purge config.CDNPurgeConfig, if err != nil { return nil, nil, nil, err } - stopPurge := service.StartActiveBlockPurgeJob(dependencies.lister, cfg.CleanupInterval) + stopPurge := startBlockedMediaPurgeJob(service, dependencies.lister, cfg.CleanupInterval) handler := imageproxyhandlers.NewHandler(service, dependencies.resolver, handlerOptions...) return service, handler, stopPurge, nil } + +// blockedMediaStartupPurgeTimeout bounds the single startup sweep that runs +// when the purge interval is zero, matching the default cleanup interval. +const blockedMediaStartupPurgeTimeout = time.Hour + +// blockedMediaPurger is the sweep startBlockedMediaPurgeJob drives. +type blockedMediaPurger interface { + PurgeActiveBlocks(ctx context.Context, lister imageproxy.BlockedBlobLister) error +} + +// startBlockedMediaPurgeJob sweeps straight away, which completes any purge a +// restart interrupted, and then every interval. An interval of zero or less +// runs only the startup sweep. Each cycle recovers its own panic and runs under +// a deadline, so one bad cycle costs one cycle rather than the job. The +// returned function stops the job and waits for an in-flight sweep to end. +func startBlockedMediaPurgeJob(purger blockedMediaPurger, lister imageproxy.BlockedBlobLister, interval time.Duration) context.CancelFunc { + ctx, cancel := context.WithCancel(context.Background()) + var waitGroup sync.WaitGroup + work := func(ctx context.Context) { + if err := purger.PurgeActiveBlocks(ctx, lister); err != nil && ctx.Err() == nil { + slog.Error("[IMAGE-PROXY] blocked media purge failed", "error", err) + } + } + if interval > 0 { + runTicker(ctx, &waitGroup, "blocked-media-purge", interval, work) + } else { + waitGroup.Add(1) + go func() { + defer waitGroup.Done() + runGuarded(ctx, "blocked-media-purge", blockedMediaStartupPurgeTimeout, work) + }() + } + return func() { + cancel() + waitGroup.Wait() + } +} diff --git a/cmd/server/image_proxy_wiring_test.go b/cmd/server/image_proxy_wiring_test.go index 24d08a3..fadccca 100644 --- a/cmd/server/image_proxy_wiring_test.go +++ b/cmd/server/image_proxy_wiring_test.go @@ -2,8 +2,10 @@ package main import ( "context" + "fmt" "net/http" "net/http/httptest" + "sync/atomic" "testing" "time" @@ -11,6 +13,7 @@ import ( "Coves/internal/config" "Coves/internal/core/imageproxy" "Coves/internal/core/moderation" + "Coves/tests/testkit" "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" @@ -156,3 +159,39 @@ func TestBuildImageProxyServiceStartupSweepAndCacheHeaders(t *testing.T) { }) } } + +type cdnWiringPanickingLister struct{ calls, withoutDeadline atomic.Int64 } + +func (lister *cdnWiringPanickingLister) ListActiveBlockedBlobs(ctx context.Context) ([]imageproxy.BlockedBlob, error) { + if lister.calls.Add(1) == 1 { + panic("first blocked media purge cycle explodes") + } + if _, hasDeadline := ctx.Deadline(); !hasDeadline { + lister.withoutDeadline.Add(1) + } + return nil, nil +} + +// One panicking sweep must cost one cycle, not the job: a dead purge job +// leaves blocked bytes the disk refused to drop in the cache until restart. +func TestBuildImageProxyServiceBlockedMediaPurgeSurvivesAPanickingCycle(t *testing.T) { + cache, err := imageproxy.NewDiskCache(t.TempDir(), 1, 0) + require.NoError(t, err) + cfg := imageproxy.DefaultConfig() + cfg.CachePath = t.TempDir() + cfg.CleanupInterval = time.Millisecond + lister := &cdnWiringPanickingLister{} + + _, _, stop, err := buildImageProxyService(cfg, config.CDNPurgeConfig{}, nil, imageProxyDependencies{ + cache: cache, processor: cdnWiringFailProcessor{t}, fetcher: cdnWiringFailFetcher{t}, + blocks: cdnWiringUnblocked{}, lister: lister, resolver: cdnWiringFailResolver{t: t}, + }) + require.NoError(t, err) + t.Cleanup(stop) + + testkit.WaitFor(t, 5*time.Second, func() (bool, error) { + return lister.calls.Load() >= 3, nil + }, testkit.WithDescription("the blocked media purge job keeps cycling after a panicking cycle"), + testkit.WithDiagnostics(func() string { return fmt.Sprintf("list calls: %d", lister.calls.Load()) })) + assert.Zero(t, lister.withoutDeadline.Load(), "every purge cycle must run under a deadline") +} diff --git a/docs/LEXICON_PUBLISHING.md b/docs/LEXICON_PUBLISHING.md index 643c12e..9239388 100644 --- a/docs/LEXICON_PUBLISHING.md +++ b/docs/LEXICON_PUBLISHING.md @@ -311,5 +311,7 @@ enabled: a 25-URI response containing normal posts plus an unknown union variant, render `deletionReason: moderator`, tolerate the optional `moderation` object, and handle thread `NotFound` without a crash loop. Until it passes, - instance removals stay `#notFoundPost` for all callers of that endpoint; - never substitute the community-only `#removedPost`. + do not enable admin actions (leave `MODERATION_ADMINS` empty): + `community.post.get` serves `#moderatedPost` for every instance removal and + has no `#notFoundPost` fallback switch. Never substitute the community-only + `#removedPost`. diff --git a/docs/PRD_ADMIN_MODERATION.md b/docs/PRD_ADMIN_MODERATION.md index 02a67ba..032c2aa 100644 --- a/docs/PRD_ADMIN_MODERATION.md +++ b/docs/PRD_ADMIN_MODERATION.md @@ -1,6 +1,6 @@ # PRD: Admin Moderation, Public Modlog, and Federated Decisions -**Status:** Draft for product review; not an implementation authorization +**Status:** Local milestone (phases 1a and 1: authority, remove/restore, NSFW apply/retract, read-path enforcement, media blocks, public/admin modlog, optional CDN purge) implemented on the backend. The federated milestone (§7 labeler, label publication/ingestion, §10 items 10–16) is not implemented. **Date:** 2026-09-17 @@ -10,7 +10,7 @@ ## 1. Summary and recommendation -Give each Coves instance multiple DID-authenticated administrators who can remove and restore posts and comments from that instance's served views, and apply or retract NSFW labels on posts. NSFW uses the existing client blur/reveal treatment without excluding the post from feeds; full AppView removal is a separate action. Removed comments retain the machine-readable `moderator-removed` state, with authority scope selecting **[Removed by server admin]** or, later, **[Removed by community moderator]**. Record each moderation action in a public, paginated modlog, with private notes and report details available only to authorized admins. +Give each Coves instance multiple DID-authenticated administrators who can remove and restore posts and comments from that instance's served views, and apply or retract NSFW labels on posts. NSFW uses the existing client blur/reveal treatment without excluding the post from feeds; full AppView removal is a separate action. Removed comments retain the machine-readable `moderator-removed` state, with authority scope selecting **[Removed by server admin]** or, later, **[Removed by community moderator]**. Record each removal and restoration in a public, paginated modlog, and every action, NSFW included, in an admin-only log with private notes. **Use atProto labels to distribute moderation decisions, with a separate action history and local enforcement policy.** Labels are the right federation primitive for “instance Y chooses to follow Coves.social's removals.” They do not themselves delete content, grant moderation permissions, provide a complete audit log, or force another instance to comply. @@ -82,7 +82,7 @@ The local milestone is useful on its own, but **the federation requirement is co - A full Ozone replacement, general activity logging of every admin interaction, or migrating every existing admin-like endpoint into this release. - Guaranteed removal of copies already downloaded or independently cached by third parties. -The initial modlog covers every successful remove, restore, NSFW application, and NSFW retraction. In-app editing, amendment, and redaction of modlog entries are deferred. The public log has no free-text field in this release, so no disclosure-correction procedure is needed yet; define one before public explanations are ever added. Failed authorization and failed requests belong in private operational/security audit records, not the public modlog. +The admin log covers every successful remove, restore, NSFW application, and NSFW retraction; the public log covers removals and restorations only (Q-LABEL-LOG). In-app editing, amendment, and redaction of modlog entries are deferred. The public log has no free-text field in this release, so no disclosure-correction procedure is needed yet; define one before public explanations are ever added. Failed authorization and failed requests belong in private operational/security audit records, not the public modlog. ## 3. Existing foundations and gaps @@ -188,7 +188,7 @@ The initial UI actions are **Mark NSFW**, **Retract NSFW label**, and the separa 4. Retraction references the active local NSFW action and clears only that authority/scope/value. Another authority's NSFW label or an author's self-label keeps the existing sensitive-content presentation active. A deleted/unavailable post does not prevent retracting a decision about its URI. 5. A removal always takes precedence over blur. Applying or retracting NSFW cannot restore removed content; restoring content cannot clear NSFW labels. If the removal is lifted while NSFW remains, the post returns with the existing NSFW treatment. A Show/reveal control never reveals an AppView-removed post. -“Remove from the entire AppView” means the independent instance-removal action applies across all ordinary serving surfaces, including direct reads, feeds, search, profile activity, quotes, and cached hydration. This remains reversible local enforcement, not deletion from the author's PDS. NSFW classification alone is intentionally revealable and is not a security boundary for fetching content. Coves-controlled media serving and shared-blob details for actual removals remain an implementation decision in section 12. +“Remove from the entire AppView” means the independent instance-removal action applies across all ordinary serving surfaces, including direct reads, feeds, search, profile activity, quotes, and cached hydration. This remains reversible local enforcement, not deletion from the author's PDS. NSFW classification alone is intentionally revealable and is not a security boundary for fetching content. Coves-controlled media serving and shared-blob rules are decided (section 9, Media and retained content). The web currently derives sensitivity from `post.record?.labels`; mobile's `PostView.isSensitive` also reads only the record's self-labels. Both must consume the new view metadata before moderator labeling is usable. Test existing non-image concealment and accessibility behavior too, not just image blur. Existing supported-client rollout checks must include NSFW metadata; do not fake self-labels to make an old client appear compatible. @@ -369,7 +369,7 @@ Choose one authoritative action store and a reliable projection into labels; do 4. A removal survives an edit, duplicate firehose delivery, restart, and delete/recreate of the same URI. A different URI is a different subject; repost detection is not implied. 5. Restore appends history and releases only its own decision. It cannot revive author-deleted content, bypass admission, or clear another source's removal. 6. Concurrent admins and request retries yield a coherent final state and exactly one action per accepted mutation, with explicit conflict/no-op responses. Cover cross-actor key reuse, same-key/different-body conflicts, authorization revocation, and stale retries after key-retention expiry. -7. The public modlog lists every successful moderation mutation with stable pagination; private notes/report data and restricted subjects do not leak. Original actor attribution survives role revocation. +7. The public modlog lists every successful remove and restore with stable pagination; private notes/report data and restricted subjects do not leak. Original actor attribution survives role revocation. 8. A storage failure cannot leave an unlogged effective action or a success entry for an unenforced action. Local-only decisions have no pending-publication claim. Moderating a locally hosted community's content leaves its community acceptance/removal records untouched. 9. Web and mobile render agreed placeholders without crashing or revealing removed content. Coves-controlled media behavior matches the approved policy, including warm-cache requests. @@ -377,10 +377,10 @@ Choose one authoritative action store and a reliable projection into labels; do - An admin marks an otherwise eligible post NSFW: web and mobile use the existing badge/blur/concealment and Show/Hide behavior for the viewer's existing settings. The post stays in the same default feed/search/profile eligibility and receives no NSFW-specific ranking penalty. - The author's record, CID, and self-labels are untouched by the moderation write. Ordinary viewers and the author cannot clear an admin label through the author-edit API. Post edits do not silently erase the active URI-scoped decision. -- Application and retraction each create one attributed modlog action; retries do not duplicate them. A non-admin or an unsupported subject/value cannot create or retract a moderator classification. +- Application and retraction each create one attributed admin-log action (not in the public modlog); retries do not duplicate them. A non-admin or an unsupported subject/value cannot create or retract a moderator classification. - Retracting one source's label leaves NSFW presentation active if another applicable source or the author's self-label remains. A post with no remaining NSFW source returns to its ordinary presentation on refreshed state. - A post can be both NSFW and removed. Removal suppresses it across the AppView even after it was revealed or cached; NSFW retraction does not restore it, and restoring it preserves any remaining NSFW treatment. -- Moderator label metadata reaches feeds, direct post reads, quotes, and refreshed cached views. Supported clients do not accidentally treat omission from a truncated source preview as the absence of NSFW. +- Moderator label metadata reaches feeds, direct post reads, and refreshed cached views. Supported clients do not accidentally treat omission from a truncated source preview as the absence of NSFW. ### Federated milestone @@ -434,7 +434,7 @@ Consulted 2026-09-17; pin dependency versions and recheck wire behavior during i ## 14. Lexicon change inventory -**Status:** Proposed implementation inventory, checked against discussion 4245 and the published style guide on 2026-09-17. No schema JSON is changed or published by this PRD. Final wire definitions depend on the product decisions in section 12; neither this inventory nor schema publication grants admin authority. +**Status:** Implementation inventory, checked against discussion 4245 and the published style guide on 2026-09-17. The local-milestone schema JSON in this inventory is implemented; publication follows `LEXICON_PUBLISHING.md`. Final wire definitions depend on the product decisions in section 12; neither this inventory nor schema publication grants admin authority. Paths below are relative to `internal/atproto/lexicon/social/coves/`. A schema `object` used in API responses is not a repository `record`: the moderation service's action history does not need a new public repository collection just to have a typed XRPC API. @@ -530,7 +530,7 @@ The published `comment.defs#commentView` requires a verbatim `record`, but the A | `community/post/get.json` | Add `#moderatedPost` to the existing open `posts.items` union and serve it for instance removals (decided 2026-09-20), subject to the old-reader check below. | Schema-additive, so schema evolution alone does not require `post.getV2`; runtime client compatibility must still be demonstrated. Unknown variants must render unavailable rather than crash a batch or expose raw JSON. | | `actor/getComments.json` | Keep its normal `commentView` response. Filter moderated comments, comments under a removed post, and any author-deleted placeholders from profile activity before hydration. | No schema change. Audit whether placeholders are currently returned here; profile tombstones have no use. | | `actor/getPosts.json`, `feed/defs.json`, `feed/getCommunity.json`, `feed/getDiscover.json`, `feed/getTimeline.json`, `feed/getAll.json`, `feed/searchPosts.json` | Keep normal feed views and filter removed subjects; hydrate the new optional post metadata for NSFW without filtering/ranking changes. Test pagination, cached candidates, and hydration. | The referenced `postView` gains optional metadata; no feed-array type change is needed. Do not broaden all feed unions just because direct views need tombstones. | -| `embed/post.json#view.resolved` | Document/test the content-free unavailable result for a removed quoted subject; preserve its current `unknown` field type. Reuse a typed tombstone object when it fits the existing rendering contract, or define the concrete unavailable object before shipping. Carry active NSFW metadata when hydrating a visible Coves quoted post so the quote uses the same sensitive-content treatment. | Redact a removed quote's preview, not the containing post; this is not an embed-only moderation action. Do not narrow the published `unknown` to a union in place or turn the record's strongRef into a hydrated object. No removed record data may survive in cached `resolved` objects. | +| `embed/post.json#view.resolved` | Document/test the content-free unavailable result for a removed quoted subject; preserve its current `unknown` field type. Reuse a typed tombstone object when it fits the existing rendering contract, or define the concrete unavailable object before shipping. D-QUOTES (decided): Coves-quoted posts are not hydrated server-side. The served quote is the strongRef only, so it carries no copied content and no NSFW metadata; `post.get` on a removed quoted URI returns `#moderatedPost`. | Redact a removed quote's preview, not the containing post; this is not an embed-only moderation action. Do not narrow the published `unknown` to a union in place or turn the record's strongRef into a hydrated object. No removed record data may survive in cached `resolved` objects. | | `community/post/getStatus.json` | Keep community admission status and its meaning. Audit serving/disclosure separately; do not overwrite `accepted` with an instance-removal status. | No moderation-driven schema change. `getSubjectState` and tombstone views describe the separate instance restriction. | **Old-reader contract:** comments need no separate old-reader path, because removed comments use the wire shape installed clients already decode and a removed post's thread uses the already-declared `NotFound`. Before enabling actions, pin with tests that supported web and mobile builds render `deletionReason: moderator`, tolerate the optional `moderation` object, and handle thread `NotFound` without a crash loop. Every other still-served endpoint must redact using only a response its published schema allows. @@ -561,4 +561,4 @@ Before publishing implementation schemas: 5. Update the runbook's moderation holdback policy for these specific reviewed schemas. Because the new content-view definitions reference `moderation.defs`, resolving its publication is a hard prerequisite for tombstones as well as the modlog. The existing `#banView` (a generic `uri`/`cid`/`record`/`indexedAt` record view referenced only by the held-back `banUser`, `listBans`, and `getBanStatus`) is published as it stands (2026-09-20); it cannot be removed afterwards. Then enable `_lexicon.moderation.coves.social` delegation, publish the reviewed shared definitions, and verify their full dependency closure resolves. Only then publish the additions to `community.comment.defs` and `community.post.defs`, and finally the endpoints that reference them. Use explicit file allowlists, **not the whole moderation directory**. Do not silently publish unreviewed governance contracts or remove an existing definition casually. 6. Resolve all published definitions through DNS/PDS and pass the older-decoder gate above before emitting new variants. The corrected `commentView` placeholder, removed-post thread `NotFound`, the selected old-reader behavior, and effective NSFW hydration/presentation must all be exercised against the running AppView before enabling actions. Ignoring optional moderator label metadata is schema-compatible but does not satisfy the supported client's NSFW behavior contract. -Inventory completion means each row has a selected wire contract and compatibility classification before coding; JSON validation/linting and network publication are later implementation/release checks, not checks this documentation edit claims to have passed. +Inventory completion means each row has a selected wire contract and compatibility classification before coding; network publication remains a release step under `LEXICON_PUBLISHING.md`. diff --git a/internal/api/routes/moderation_cdn_purge_integration_test.go b/internal/api/routes/moderation_cdn_purge_integration_test.go index e079351..dbb05e9 100644 --- a/internal/api/routes/moderation_cdn_purge_integration_test.go +++ b/internal/api/routes/moderation_cdn_purge_integration_test.go @@ -543,9 +543,9 @@ func TestModerationCDNPurgeSweepRecordsOutcomesAfterContextEnds(t *testing.T) { "an acknowledged pair completes even though the sweep's context ended") interrupted := moderationCDNTarget(t, h.db, h.ownerA, second) assert.Equal(t, "pending", interrupted.state) - assert.Equal(t, 1, interrupted.attempts) - assert.Equal(t, "interrupted", interrupted.failureCode) - assert.Equal(t, time.Date(2026, 9, 28, 12, 2, 30, 0, time.UTC), interrupted.nextAttemptAt) + assert.Equal(t, 0, interrupted.attempts, "an interrupted pair was never refused, so it is not a failed attempt") + assert.Empty(t, interrupted.failureCode) + assert.Equal(t, time.Date(2026, 9, 28, 12, 1, 30, 0, time.UTC), interrupted.nextAttemptAt, "an interrupted pair is due again at once") var warnings int for _, record := range logs.snapshot() { attrs := make(map[string]slog.Value) diff --git a/internal/api/routes/moderation_label_modlog_integration_test.go b/internal/api/routes/moderation_label_modlog_integration_test.go index ead1a0a..986c7e1 100644 --- a/internal/api/routes/moderation_label_modlog_integration_test.go +++ b/internal/api/routes/moderation_label_modlog_integration_test.go @@ -35,6 +35,14 @@ func (f *modlogFixture) labelPost(t *testing.T) moderation.StrongRef { VALUES ($1, $2, $3, $4, $5, 'modlog labelled post', 'indexed body', $6) `, subject.URI, subject.CID, rkey, f.authorDID, f.communityDID, time.Now()) require.NoError(t, err) + // Accepted, so the post is public and the admin log names it as the + // action's subject rather than as a restricted privateSubject. + _, err = f.db.ExecContext(t.Context(), ` + 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.communityDID, subject.URI, subject.CID) + require.NoError(t, err) return subject } diff --git a/internal/api/routes/moderation_modlog_integration_test.go b/internal/api/routes/moderation_modlog_integration_test.go index 7147a96..642a142 100644 --- a/internal/api/routes/moderation_modlog_integration_test.go +++ b/internal/api/routes/moderation_modlog_integration_test.go @@ -671,7 +671,15 @@ func TestModerationModlogCommunityAssociationSurvivesReindex(t *testing.T) { require.NoError(t, err) _, err = f.db.ExecContext(t.Context(), `TRUNCATE TABLE comments, posts CASCADE`) require.NoError(t, err) - check(t) + // With nothing indexed to show the subject is public, it is restricted: + // listed without its subject, and not selectable by community. + for _, community := range []string{f.communityDID, f.communityHandle} { + _, items := modlogPage(t, f.list(t, false, "", url.Values{"community": {community}}), false) + assert.Empty(t, items) + } + _, items := modlogPage(t, f.list(t, false, "", nil), false) + require.Len(t, items, 1) + assert.NotContains(t, modlogAction(t, items[0], false), "subject") _, err = f.db.ExecContext(t.Context(), `INSERT INTO posts (uri,cid,rkey,author_did,community_did,title,created_at) VALUES ($1,$2,$3,$4,$5,$6,NOW())`, f.postURI, f.postCID, f.postURI[strings.LastIndex(f.postURI, "/")+1:], f.authorDID, f.communityDID, "reindexed post") require.NoError(t, err) _, err = f.db.ExecContext(t.Context(), `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,NOW())`, uri, cid, uri[strings.LastIndex(uri, "/")+1:], f.authorDID, f.postURI, f.postCID, "reindexed comment") diff --git a/internal/api/routes/moderation_post_media_integration_test.go b/internal/api/routes/moderation_post_media_integration_test.go index 6c0b3df..b6ac70f 100644 --- a/internal/api/routes/moderation_post_media_integration_test.go +++ b/internal/api/routes/moderation_post_media_integration_test.go @@ -150,11 +150,11 @@ func TestModerationPostCDNFailureDoesNotUndoLocalRemoval(t *testing.T) { }) assert.NotContains(t, text.String(), token, "no log record may expose the Cloudflare token") assert.NotContains(t, text.String(), bodySentinel, "no log record may expose the Cloudflare response") - if record.Level == slog.LevelError && attrs["did"] == authorDID && attrs["cid"] == imageCID && attrs["code"] == "http_500" { + if record.Level == slog.LevelError && attrs["code"] == "http_500" && attrs["count"] == "1" { matchingErrors++ } } - assert.Equal(t, 1, matchingErrors, "exactly one Error record must carry did, cid and http_500 for the first failure") + assert.Equal(t, 1, matchingErrors, "exactly one aggregated Error record must carry http_500 and count=1 for the first failure") slog.SetDefault(previousLogger) newQueue := func() *moderation.CDNPurgeQueue { diff --git a/internal/atproto/jetstream/authorpost.go b/internal/atproto/jetstream/authorpost.go index de441d7..ed40a6f 100644 --- a/internal/atproto/jetstream/authorpost.go +++ b/internal/atproto/jetstream/authorpost.go @@ -718,6 +718,11 @@ func (c *PostEventConsumer) upsertAuthorPost(ctx context.Context, authorDID stri if found && stored.communityDID != record.Community { log.Printf("🚨 SECURITY: ignoring the whole %s update for %s - community is immutable (stored %s, incoming %s)", PostV2Collection, uri, stored.communityDID, record.Community) + // The author's repo still serves the ignored record's images, so a + // removed post blocks them for the author (PRD Q-I6). + if err := c.blockIgnoredUpdateMedia(ctx, uri, authorDID, commit.Rev, timeUS, stored.indexedAt, record.Embed); err != nil { + return err + } return nil } @@ -831,6 +836,33 @@ func (c *PostEventConsumer) upsertAuthorPost(ctx context.Context, authorDID stri return nil } +// blockIgnoredUpdateMedia blocks, for the author only, the images of a postv2 +// update the consumer ignores, when the post has an active removal. A stale +// event (older by rev or by event time than the indexed state) blocks nothing: +// the newer state supersedes it. Neither guard advances anything, because the +// ignored content is never applied. +func (c *PostEventConsumer) blockIgnoredUpdateMedia(ctx context.Context, uri, authorDID, rev string, timeUS int64, storedIndexedAt time.Time, embed map[string]interface{}) error { + if evTime, ok := eventTime(timeUS); ok && !storedIndexedAt.Before(evTime) { + return nil + } + stale, err := recordRevIsStale(ctx, c.db, uri, rev) + if err != nil { + return fmt.Errorf("failed to check rev of ignored post update: %w", err) + } + if stale { + logSkippedStaleRev(ConsumerPosts, "update", uri, rev) + return nil + } + _, embedJSON, _, err := serializePostContent(nil, embed, nil) + if err != nil { + return err + } + if err := c.blockIncomingMedia(ctx, uri, authorDID, embedJSON); err != nil { + return fmt.Errorf("failed to block media of ignored post update: %w", err) + } + return nil +} + // revIsCurrent reports whether the gate's stored rev for this record is exactly // the one this event carries — i.e. whether the event is the newest the AppView // has seen rather than an older copy. diff --git a/internal/atproto/jetstream/comment_consumer.go b/internal/atproto/jetstream/comment_consumer.go index 3f8ad77..731db25 100644 --- a/internal/atproto/jetstream/comment_consumer.go +++ b/internal/atproto/jetstream/comment_consumer.go @@ -1,11 +1,6 @@ package jetstream import ( - "Coves/internal/atproto/utils" - "Coves/internal/core/comments" - "Coves/internal/core/moderation" - "Coves/internal/core/posts" - "Coves/internal/core/richtext" "context" "database/sql" "encoding/json" @@ -15,6 +10,13 @@ import ( "strings" "time" + "Coves/internal/atproto/utils" + "Coves/internal/core/comments" + "Coves/internal/core/embeds" + "Coves/internal/core/moderation" + "Coves/internal/core/posts" + "Coves/internal/core/richtext" + "github.com/lib/pq" ) @@ -46,8 +48,11 @@ type CommentEventConsumer struct { type CommentEventConsumerOption func(*CommentEventConsumer) // MediaReconciler blocks images introduced on removed comments or posts. +// ReconcileIncomingTx covers incoming content the consumer does not index, +// because the stored row cannot name its blobs. type MediaReconciler interface { ReconcileTx(ctx context.Context, tx *sql.Tx, subjectURI string) ([]moderation.MediaBlock, error) + ReconcileIncomingTx(ctx context.Context, tx *sql.Tx, subjectURI, ownerDID string, blobCIDs []string) ([]moderation.MediaBlock, error) Purge(ctx context.Context, blocks []moderation.MediaBlock) } @@ -82,6 +87,38 @@ func commitMediaWrite(ctx context.Context, tx *sql.Tx, uri string, reconciler Me return nil } +// commitIncomingCommentMedia blocks, for the commenter only, the images of an +// incoming comment record the consumer does not index, when the comment has an +// active removal. It commits tx unless reconciliation fails, and purges cached +// copies only after the blocks commit. +func (c *CommentEventConsumer) commitIncomingCommentMedia(ctx context.Context, tx *sql.Tx, uri, commenterDID string, blobCIDs []string) error { + if c.mediaReconciler == nil || len(blobCIDs) == 0 { + return tx.Commit() + } + blocks, err := c.mediaReconciler.ReconcileIncomingTx(ctx, tx, uri, commenterDID, blobCIDs) + if err != nil { + return fmt.Errorf("reconcile incoming comment media: %w", err) + } + if err := tx.Commit(); err != nil { + return err + } + c.mediaReconciler.Purge(ctx, blocks) + return nil +} + +// incomingCommentImageCIDs returns the image CIDs of a serialized incoming +// comment embed. A malformed embed has no served images to block. +func incomingCommentImageCIDs(embed *string) []string { + if embed == nil { + return nil + } + var decoded map[string]interface{} + if err := json.Unmarshal([]byte(*embed), &decoded); err != nil { + return nil + } + return embeds.CommentImageCIDs(decoded) +} + // WithCommentBridgeTrust installs the provenance gate that decides which user repos may // assert bridgedStats on their comments. Without it, bridgedStats are default-denied. func WithCommentBridgeTrust(bt *BridgeTrust) CommentEventConsumerOption { @@ -322,6 +359,12 @@ func (c *CommentEventConsumer) updateComment(ctx context.Context, repoDID string log.Printf(" Incoming root: %s (CID: %s)", commentRecord.Reply.Root.URI, commentRecord.Reply.Root.CID) log.Printf(" Existing parent: %s (CID: %s)", storedParentURI, storedParentCID) log.Printf(" Incoming parent: %s (CID: %s)", commentRecord.Reply.Parent.URI, commentRecord.Reply.Parent.CID) + // The commenter's repo still serves the rejected record's images, so a + // removed comment blocks them for the commenter (PRD Q-I6). The recency + // guard above already ran; a rev-stale event blocks nothing either. + if err := c.blockRejectedUpdateMedia(ctx, uri, repoDID, commit.Rev, commentRecord.Embed); err != nil { + return err + } // PERMANENT: threading reassignment is a policy rejection that no retry or // redrive can ever make valid. return fmt.Errorf("%w: comment threading references cannot be changed after creation", ErrPermanentEvent) @@ -461,6 +504,38 @@ func (c *CommentEventConsumer) updateComment(ctx context.Context, repoDID string return nil } +// blockRejectedUpdateMedia blocks the images of a rejected comment update for +// the commenter when the comment has an active removal. A rev-stale event +// blocks nothing, and the read-only rev check advances nothing, because the +// rejected content is never applied. +func (c *CommentEventConsumer) blockRejectedUpdateMedia(ctx context.Context, uri, commenterDID, rev string, embed map[string]interface{}) error { + blobCIDs := embeds.CommentImageCIDs(embed) + if c.mediaReconciler == nil || len(blobCIDs) == 0 { + return nil + } + stale, err := recordRevIsStale(ctx, c.db, uri, rev) + if err != nil { + return fmt.Errorf("failed to check rev of rejected comment update: %w", err) + } + if stale { + logSkippedStaleRev(ConsumerComments, "update", uri, rev) + return nil + } + tx, err := c.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("failed to begin media block transaction: %w", err) + } + defer func() { + if rollbackErr := tx.Rollback(); rollbackErr != nil && rollbackErr != sql.ErrTxDone { + log.Printf("Failed to rollback transaction: %v", rollbackErr) + } + }() + if err := c.commitIncomingCommentMedia(ctx, tx, uri, commenterDID, blobCIDs); err != nil { + return fmt.Errorf("failed to block media of rejected comment update: %w", err) + } + return nil +} + // deleteComment soft-deletes a comment, blanking content to preserve thread // structure while respecting user privacy: the row remains and is shown as // "[deleted]" in thread views, so parent counts are intentionally NOT @@ -646,7 +721,10 @@ func (c *CommentEventConsumer) indexCommentAndUpdateCounts(ctx context.Context, // a dead-lettered delete AND a cross-thread rkey reuse inside the // redrive window. Documented rather than fixed. log.Printf("Comment already indexed: %s (idempotent replay)", comment.URI) - if commitErr := tx.Commit(); commitErr != nil { + // The dropped record's images are still served from the commenter's + // repo, so a removed comment blocks them for the commenter (PRD Q-I6). + if commitErr := c.commitIncomingCommentMedia(ctx, tx, comment.URI, comment.CommenterDID, + incomingCommentImageCIDs(comment.Embed)); commitErr != nil { return fmt.Errorf("failed to commit transaction: %w", commitErr) } return nil diff --git a/internal/atproto/jetstream/moderation_rejected_media_test.go b/internal/atproto/jetstream/moderation_rejected_media_test.go new file mode 100644 index 0000000..7c362c9 --- /dev/null +++ b/internal/atproto/jetstream/moderation_rejected_media_test.go @@ -0,0 +1,194 @@ +//go:build integration + +package jetstream + +import ( + "net/http" + "testing" + "time" + + "Coves/internal/core/moderation" + "Coves/tests/testkit" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// rejectedMediaCase seeds a subject, optionally removes it, then delivers an +// event whose content the consumer refuses to index. The author's repository +// still serves the images the refused record names, so a removed subject must +// block them for the author only (PRD Q-I6), while the refused content itself +// stays unindexed. +type rejectedMediaCase struct { + h *editMediaHarness + revs []string + started time.Time + postRkey string + post, otherPost moderation.StrongRef + commentRkey string + subject moderation.StrongRef + imageA, imageB string + removedActionID string + subjectIsComment bool +} + +func newRejectedMediaCase(t *testing.T, name string, comment, removed bool) *rejectedMediaCase { + t.Helper() + h := newEditMediaHarness(t) + c := &rejectedMediaCase{ + h: h, revs: increasingTIDs(t, 4), started: time.Now().Add(-time.Minute), + imageA: editMediaCID(name + " image A"), imageB: editMediaCID(name + " image B"), + subjectIsComment: comment, + } + for _, owner := range []string{pv2Author, h.otherOwner} { + h.pds.know(owner, c.imageA, c.imageB) + } + c.postRkey = testkit.TID() + c.post = moderation.StrongRef{URI: pv2URI(pv2Author, c.postRkey), CID: editMediaCID(name + " post record")} + require.NoError(t, h.posts.HandleEvent(t.Context(), pv2Event(pv2Author, "create", c.postRkey, c.revs[0], + c.post.CID, c.started.UnixMicro(), postModerationRecord(c.imageA)))) + otherRkey := testkit.TID() + c.otherPost = moderation.StrongRef{URI: pv2URI(pv2Author, otherRkey), CID: editMediaCID(name + " other post record")} + require.NoError(t, h.posts.HandleEvent(t.Context(), pv2Event(pv2Author, "create", otherRkey, c.revs[0], + c.otherPost.CID, c.started.UnixMicro(), postModerationRecord()))) + c.subject = c.post + if comment { + c.commentRkey = testkit.TID() + c.subject = moderation.StrongRef{ + URI: "at://" + pv2Author + "/" + moderation.CommentCollection + "/" + c.commentRkey, + CID: editMediaCID(name + " comment record"), + } + require.NoError(t, h.comments.HandleEvent(t.Context(), editMediaCommentEvent(c.post, + "create", c.commentRkey, c.revs[0], c.subject.CID, c.started, c.imageA))) + } + if removed { + c.removedActionID = h.remove(t, c.subject, "v0").Action.ID + } + return c +} + +// postCommunityChange is a postv2 update that retargets the community, which +// the consumer ignores whole. +func (c *rejectedMediaCase) postCommunityChange(rev string, at time.Time) *JetstreamEvent { + record := postModerationRecord(c.imageA, c.imageB) + record["community"] = pv2Prefix + "community2" + return pv2Event(pv2Author, "update", c.postRkey, rev, editMediaCID("retargeted post record "+rev), at.UnixMicro(), record) +} + +// assertContentUnchanged proves the refused event indexed nothing. +func (c *rejectedMediaCase) assertContentUnchanged(t *testing.T) { + t.Helper() + table := "posts" + if c.subjectIsComment { + table = "comments" + } + var storedCID, storedEmbed string + require.NoError(t, c.h.db.QueryRowContext(t.Context(), + `SELECT cid, embed::text FROM `+table+` WHERE uri = $1`, c.subject.URI).Scan(&storedCID, &storedEmbed)) + assert.Equal(t, c.subject.CID, storedCID, "the refused event must not replace the indexed record") + assert.NotContains(t, storedEmbed, c.imageB, "the refused event's image must not be indexed") + if c.subjectIsComment { + var parentURI string + require.NoError(t, c.h.db.QueryRowContext(t.Context(), + `SELECT parent_uri FROM comments WHERE uri = $1`, c.subject.URI).Scan(&parentURI)) + assert.Equal(t, c.post.URI, parentURI, "threading references must stay immutable") + } else { + var communityDID string + require.NoError(t, c.h.db.QueryRowContext(t.Context(), + `SELECT community_did FROM posts WHERE uri = $1`, c.subject.URI).Scan(&communityDID)) + assert.Equal(t, pv2Community, communityDID, "the community must stay immutable") + } +} + +func (c *rejectedMediaCase) assertImageBBlockedForAuthorOnly(t *testing.T) { + t.Helper() + assert.Equal(t, 1, countRows(t, c.h.db, `SELECT count(*) FROM moderation_media_blocks + WHERE action_id = $1 AND owner_did = $2 AND blob_cid = $3 AND active`, c.removedActionID, pv2Author, c.imageB), + "the refused event's new image needs an active author-owned block on the removal") + assert.Zero(t, countRows(t, c.h.db, `SELECT count(*) FROM moderation_media_blocks + WHERE blob_cid = $1 AND owner_did IS NULL AND active`, c.imageB), + "an image added after removal must never get an every-owner block") + assert.Equal(t, http.StatusNotFound, c.h.imageStatus(t, pv2Author, c.imageB), "the author's new image must not be served") + assert.Equal(t, http.StatusOK, c.h.imageStatus(t, c.h.otherOwner, c.imageB), "another owner's copy must stay available") +} + +func (c *rejectedMediaCase) assertImageBNotBlocked(t *testing.T) { + t.Helper() + assert.Zero(t, countRows(t, c.h.db, `SELECT count(*) FROM moderation_media_blocks WHERE blob_cid = $1`, c.imageB), + "no block may be added for this event") + assert.Equal(t, http.StatusOK, c.h.imageStatus(t, pv2Author, c.imageB), "the author's image must stay available") +} + +func TestModerationRejectedEventBlocksIncomingImagesForAuthor(t *testing.T) { + for _, variant := range []struct { + name string + comment bool + deliver func(t *testing.T, c *rejectedMediaCase) + }{ + { + name: "postv2 update that changes community", + deliver: func(t *testing.T, c *rejectedMediaCase) { + require.NoError(t, c.h.posts.HandleEvent(t.Context(), c.postCommunityChange(c.revs[1], c.started.Add(time.Second))), + "an illegal retarget is ignored, not dead-lettered") + }, + }, + { + name: "comment update that changes threading references", comment: true, + deliver: func(t *testing.T, c *rejectedMediaCase) { + err := c.h.comments.HandleEvent(t.Context(), editMediaCommentEvent(c.otherPost, "update", c.commentRkey, c.revs[1], + editMediaCID("rethreaded comment record"), c.started.Add(time.Second), c.imageA, c.imageB)) + require.ErrorIs(t, err, ErrPermanentEvent, "the threading change itself must still be rejected") + }, + }, + { + name: "same-rkey comment re-create with a changed parent", comment: true, + deliver: func(t *testing.T, c *rejectedMediaCase) { + require.NoError(t, c.h.comments.HandleEvent(t.Context(), editMediaCommentEvent(c.otherPost, "create", c.commentRkey, c.revs[1], + editMediaCID("recreated comment record"), c.started.Add(time.Second), c.imageB)), + "a re-create with a changed parent is kept as a duplicate") + }, + }, + } { + t.Run(variant.name+" on a removed subject", func(t *testing.T) { + c := newRejectedMediaCase(t, variant.name, variant.comment, true) + variant.deliver(t, c) + c.assertContentUnchanged(t) + c.assertImageBBlockedForAuthorOnly(t) + }) + t.Run(variant.name+" on a subject that is not removed", func(t *testing.T) { + c := newRejectedMediaCase(t, variant.name, variant.comment, false) + variant.deliver(t, c) + c.assertContentUnchanged(t) + c.assertImageBNotBlocked(t) + }) + } +} + +// A refused event that is older than state already applied must not block its +// images: the newer state supersedes it. +func TestModerationStaleRejectedEventAddsNoBlocks(t *testing.T) { + t.Run("postv2 community change older by rev", func(t *testing.T) { + c := newRejectedMediaCase(t, "stale post retarget", false, true) + require.NoError(t, c.h.posts.HandleEvent(t.Context(), pv2Event(pv2Author, "update", c.postRkey, c.revs[2], + editMediaCID("newer post record"), c.started.Add(2*time.Second).UnixMicro(), postModerationRecord(c.imageA)))) + // A cross-feed copy carries a newer emission time, so only rev orders it. + require.NoError(t, c.h.posts.HandleEvent(t.Context(), c.postCommunityChange(c.revs[1], c.started.Add(3*time.Second)))) + c.assertImageBNotBlocked(t) + }) + t.Run("postv2 community change older by event time", func(t *testing.T) { + c := newRejectedMediaCase(t, "late post retarget", false, true) + require.NoError(t, c.h.posts.HandleEvent(t.Context(), pv2Event(pv2Author, "update", c.postRkey, c.revs[1], + editMediaCID("newer post record"), c.started.Add(2*time.Second).UnixMicro(), postModerationRecord(c.imageA)))) + require.NoError(t, c.h.posts.HandleEvent(t.Context(), c.postCommunityChange(c.revs[2], c.started.Add(time.Second)))) + c.assertImageBNotBlocked(t) + }) + t.Run("comment threading change older by rev", func(t *testing.T) { + c := newRejectedMediaCase(t, "stale comment rethread", true, true) + require.NoError(t, c.h.comments.HandleEvent(t.Context(), editMediaCommentEvent(c.post, "update", c.commentRkey, c.revs[2], + editMediaCID("newer comment record"), c.started.Add(2*time.Second), c.imageA))) + err := c.h.comments.HandleEvent(t.Context(), editMediaCommentEvent(c.otherPost, "update", c.commentRkey, c.revs[1], + editMediaCID("rethreaded comment record"), c.started.Add(3*time.Second), c.imageA, c.imageB)) + require.ErrorIs(t, err, ErrPermanentEvent) + c.assertImageBNotBlocked(t) + }) +} diff --git a/internal/atproto/jetstream/moderation_removal_edit_lock_integration_test.go b/internal/atproto/jetstream/moderation_removal_edit_lock_integration_test.go new file mode 100644 index 0000000..c20b80d --- /dev/null +++ b/internal/atproto/jetstream/moderation_removal_edit_lock_integration_test.go @@ -0,0 +1,167 @@ +//go:build integration + +package jetstream + +import ( + "context" + "fmt" + "testing" + "time" + + "Coves/internal/core/moderation" + "Coves/internal/db/postgres" + "Coves/tests/fixtures" + "Coves/tests/testkit" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// pausingModerationStore parks a mutation inside its transaction at +// InsertAction, after it has read the indexed subject and taken that read's +// content-row lock, until release is closed. +type pausingModerationStore struct { + moderation.Store + paused chan struct{} + release chan struct{} +} + +func (s *pausingModerationStore) InTransaction(ctx context.Context, fn func(context.Context, moderation.Transaction) error) error { + return s.Store.InTransaction(ctx, func(ctx context.Context, tx moderation.Transaction) error { + return fn(ctx, &pausingModerationTransaction{Transaction: tx, store: s}) + }) +} + +type pausingModerationTransaction struct { + moderation.Transaction + store *pausingModerationStore +} + +func (tx *pausingModerationTransaction) InsertAction(ctx context.Context, action moderation.Action) (*moderation.Action, error) { + close(tx.store.paused) + select { + case <-tx.store.release: + case <-ctx.Done(): + return nil, ctx.Err() + } + return tx.Transaction.InsertAction(ctx, action) +} + +// An author edit that adds image Y must not commit between the removal's read +// of the indexed subject and the removal's commit. If it could, the edit's +// media reconciliation would find no active removal yet, and the removal would +// block only the images it read, leaving Y served. The content-row share lock +// taken with that read orders the two: the edit waits for the removal, then its +// reconciliation sees the active removal and blocks Y. +func TestModerationRemovalHoldsContentRowAgainstAuthorEdit(t *testing.T) { + imageY := postModerationCIDTwo + for _, scenario := range []struct { + name string + // setup indexes the subject with one image and returns it with the + // consumer and event that edit it to add imageY. + setup func(t *testing.T, f postModerationConsumerFixture) (moderation.StrongRef, func(context.Context) error) + }{ + {name: "postv2", setup: func(t *testing.T, f postModerationConsumerFixture) (moderation.StrongRef, func(context.Context) error) { + update := pv2Event(pv2Author, "update", f.rkey, f.revs[1], editMediaCID("locked post edit"), + f.createdAt+1_000_000, postModerationRecord(postModerationCIDOne, imageY)) + return moderation.StrongRef{URI: f.uri, CID: postModerationRecordCID}, + func(ctx context.Context) error { return f.consumer.HandleEvent(ctx, update) } + }}, + {name: "comment", setup: func(t *testing.T, f postModerationConsumerFixture) (moderation.StrongRef, func(context.Context) error) { + reconciler := moderation.NewMediaReconciler(postgres.NewModerationRepository(f.db), fixtures.InstanceDID(), nil) + consumer := NewCommentEventConsumer(postgres.NewCommentRepository(f.db), f.db, WithCommentMediaReconciler(reconciler)) + root := moderation.StrongRef{URI: f.uri, CID: postModerationRecordCID} + rkey := testkit.TID() + created := editMediaCID("locked comment create") + now := time.Now() + require.NoError(t, consumer.HandleEvent(t.Context(), + editMediaCommentEvent(root, "create", rkey, f.revs[2], created, now, postModerationCIDOne))) + update := editMediaCommentEvent(root, "update", rkey, f.revs[3], editMediaCID("locked comment edit"), + now.Add(time.Second), postModerationCIDOne, imageY) + return moderation.StrongRef{URI: "at://" + pv2Author + "/" + moderation.CommentCollection + "/" + rkey, CID: created}, + func(ctx context.Context) error { return consumer.HandleEvent(ctx, update) } + }}, + } { + t.Run(scenario.name, func(t *testing.T) { + f := newPostModerationConsumerFixture(t) + subject, edit := scenario.setup(t, f) + // The pool is three connections: the removal, the edit and this one. + observer, err := f.db.Conn(t.Context()) + require.NoError(t, err) + t.Cleanup(func() { _ = observer.Close() }) + store := &pausingModerationStore{ + Store: postgres.NewModerationRepository(f.db), + paused: make(chan struct{}), release: make(chan struct{}), + } + moderator := moderation.NewService( + moderation.NewRepositorySubjectReader(f.postRepository, postgres.NewCommentRepository(f.db)), + store, moderation.Config{InstanceDID: fixtures.InstanceDID(), IdempotencyRetention: 24 * time.Hour, MaxLiveIdempotencyKeys: 1000}, + ) + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + released := false + releaseRemoval := func() { + if !released { + released = true + close(store.release) + } + } + t.Cleanup(releaseRemoval) + + removalDone := make(chan error, 1) + go func() { + _, removeErr := moderator.RemoveContent(ctx, fixtures.DID("lockedremovaladmin"), moderation.RemoveContentRequest{ + Subject: subject, ExpectedVersion: "v0", IdempotencyKey: "locked-removal", + Reason: "social.coves.moderation.defs#reasonSpam", + }) + removalDone <- removeErr + }() + testkit.WaitFor(t, 5*time.Second, func() (bool, error) { + select { + case <-store.paused: + return true, nil + case removeErr := <-removalDone: + return false, fmt.Errorf("removal finished before reaching InsertAction: %v", removeErr) + default: + return false, nil + } + }, testkit.WithDescription("removal paused after reading the indexed subject")) + + editDone := make(chan error, 1) + go func() { editDone <- edit(ctx) }() + // Either the edit waits on a row lock, or nothing orders it and it + // commits while the removal is still paused. + testkit.WaitFor(t, 5*time.Second, func() (bool, error) { + if len(editDone) > 0 { + return true, nil + } + var waiting int + err := observer.QueryRowContext(t.Context(), ` + SELECT count(*) FROM pg_stat_activity + WHERE datname = current_database() AND pid <> pg_backend_pid() + AND wait_event_type = 'Lock' AND wait_event <> 'advisory' + `).Scan(&waiting) + return waiting > 0, err + }, testkit.WithDescription("author edit waiting on a lock or finished")) + assert.Empty(t, editDone, "the author edit must not commit while the removal holds its read of the subject") + var indexedCID string + require.NoError(t, observer.QueryRowContext(t.Context(), ` + SELECT cid FROM posts WHERE uri = $1 UNION ALL SELECT cid FROM comments WHERE uri = $1 + `, subject.URI).Scan(&indexedCID)) + assert.Equal(t, subject.CID, indexedCID, "the edit must not be visible while the removal is paused") + + releaseRemoval() + require.NoError(t, <-removalDone) + require.NoError(t, <-editDone) + require.NoError(t, observer.QueryRowContext(t.Context(), ` + SELECT cid FROM posts WHERE uri = $1 UNION ALL SELECT cid FROM comments WHERE uri = $1 + `, subject.URI).Scan(&indexedCID)) + assert.NotEqual(t, subject.CID, indexedCID, "the edit must be indexed after the removal commits") + blocked, err := postgres.NewModerationRepository(f.db).IsBlocked(t.Context(), pv2Author, imageY) + require.NoError(t, err) + assert.True(t, blocked, "the image the edit added must be blocked under the removal") + }) + } +} + +var _ moderation.Store = (*pausingModerationStore)(nil) diff --git a/internal/atproto/jetstream/post_consumer.go b/internal/atproto/jetstream/post_consumer.go index 2975997..7714b98 100644 --- a/internal/atproto/jetstream/post_consumer.go +++ b/internal/atproto/jetstream/post_consumer.go @@ -29,7 +29,7 @@ type PostEventConsumer struct { db *sql.DB // Direct DB access for atomic count reconciliation // nil keeps ingestion independent of moderation media reconciliation. - mediaReconciler PostMediaReconciler + mediaReconciler MediaReconciler // bridgeTrust gates whether a post's author repo may assert bridgedStats. // nil means default-deny (bridgedStats are ignored for every post). bridgeTrust *BridgeTrust @@ -683,15 +683,8 @@ func parseRecordCreatedAt(raw, uri string) time.Time { return createdAt } -// PostMediaReconciler also blocks the blobs of incoming post content that the -// consumer does not index, because the stored row cannot name them. -type PostMediaReconciler interface { - MediaReconciler - ReconcileIncomingTx(ctx context.Context, tx *sql.Tx, subjectURI, ownerDID string, blobCIDs []string) ([]moderation.MediaBlock, error) -} - // WithPostMediaReconciler reconciles media blocks when a removed post is created or edited. -func WithPostMediaReconciler(reconciler PostMediaReconciler) PostEventConsumerOption { +func WithPostMediaReconciler(reconciler MediaReconciler) PostEventConsumerOption { return func(c *PostEventConsumer) { c.mediaReconciler = reconciler } } diff --git a/internal/core/comments/post_removal_read_paths_integration_test.go b/internal/core/comments/post_removal_read_paths_integration_test.go index 0ff173c..2fda303 100644 --- a/internal/core/comments/post_removal_read_paths_integration_test.go +++ b/internal/core/comments/post_removal_read_paths_integration_test.go @@ -69,6 +69,20 @@ func removeFixturePost(t *testing.T, service moderation.Service, subject moderat return removed } +// labelFixtureRoot applies an active NSFW label, which is a moderation decision +// but not a removal, to the root of comment, so read paths that exclude removed +// roots must still serve it. +func labelFixtureRoot(t *testing.T, db *sql.DB, service moderation.Service, comment string) { + t.Helper() + var root moderation.StrongRef + require.NoError(t, db.QueryRowContext(t.Context(), `SELECT root_uri, root_cid FROM comments WHERE uri = $1`, comment).Scan(&root.URI, &root.CID)) + labelled, err := service.LabelContent(t.Context(), fixtures.DID(testkit.UniqueIDWithPrefix(t, "rootlabeler")), moderation.LabelContentRequest{ + Subject: root, LabelValue: moderation.LabelNSFW, ExpectedVersion: "v0", IdempotencyKey: "label-root", + }) + require.NoError(t, err) + require.Equal(t, moderation.OutcomeApplied, labelled.Outcome) +} + func TestPostRemovalGetCommentsHidesRootForEveryViewer(t *testing.T) { db, commentService, moderationService, subject, _, _, _, _, _ := postRemovalFixture(t) baseline, err := commentService.GetComments(t.Context(), &comments.GetCommentsRequest{PostURI: subject.URI, Sort: "new", Depth: 1, Limit: 10}) @@ -99,11 +113,12 @@ func TestPostRemovalGetCommentsHidesRootForEveryViewer(t *testing.T) { } func TestPostRemovalActorCommentsFiltersBeforePagination(t *testing.T) { - _, commentService, moderationService, subject, communityDID, commenterDID, first, visible, second := postRemovalFixture(t) + db, commentService, moderationService, subject, communityDID, commenterDID, first, visible, second := postRemovalFixture(t) baseline := postRemovalActorComments(t, commentService, commenterDID, "", 10, nil) require.Len(t, baseline.Comments, 3) require.Equal(t, []string{first, visible, second}, []string{baseline.Comments[0].URI, baseline.Comments[1].URI, baseline.Comments[2].URI}) removeFixturePost(t, moderationService, subject) + labelFixtureRoot(t, db, moderationService, visible) for _, filter := range []struct{ name, community string }{{"all communities", ""}, {"community filtered", communityDID}} { t.Run(filter.name, func(t *testing.T) { whole := postRemovalActorComments(t, commentService, commenterDID, filter.community, 10, nil) @@ -165,7 +180,8 @@ func TestPostRemovalRestoreServesIndexedThreadAndActorComments(t *testing.T) { // The profile's commentCount must agree with the list actor.getComments // serves: both exclude comments under an instance-removed root and comments -// that are themselves instance-removed. +// that are themselves instance-removed, and both keep a comment whose root is +// only labelled. func TestPostRemovalProfileCommentCountMatchesActorComments(t *testing.T) { db, commentService, moderationService, subject, _, commenterDID, _, visible, _ := postRemovalFixture(t) var visibleRootURI, visibleRootCID string @@ -183,6 +199,7 @@ func TestPostRemovalProfileCommentCountMatchesActorComments(t *testing.T) { }) require.NoError(t, err) require.Equal(t, moderation.OutcomeApplied, removedComment.Outcome) + labelFixtureRoot(t, db, moderationService, visible) listed := postRemovalActorComments(t, commentService, commenterDID, "", 50, nil) require.Len(t, listed.Comments, 1) diff --git a/internal/core/imageproxy/moderation_test.go b/internal/core/imageproxy/moderation_test.go index 31a7b8d..2304244 100644 --- a/internal/core/imageproxy/moderation_test.go +++ b/internal/core/imageproxy/moderation_test.go @@ -499,20 +499,3 @@ func TestImageProxyService_PurgeActiveBlocksRemovesBlockedEntries(t *testing.T) err = service.PurgeActiveBlocks(t.Context(), blockedBlobListerFunc(func(context.Context) ([]BlockedBlob, error) { return nil, listError })) assert.ErrorIs(t, err, listError) } - -func TestImageProxyService_ActiveBlockPurgeJobSweepsAtStartup(t *testing.T) { - cache := NewMockCache() - cache.SetCacheData("avatar", moderationTestOwner, moderationTestCID, []byte("left by a lost purge")) - service, err := NewService(cache, NewMockProcessor(nil, nil), NewMockFetcher(nil, nil), allowAllBlockChecker(), DefaultConfig()) - require.NoError(t, err) - lister := blockedBlobListerFunc(func(context.Context) ([]BlockedBlob, error) { - return []BlockedBlob{{OwnerDID: moderationTestOwner, CID: moderationTestCID}}, nil - }) - - stop := service.StartActiveBlockPurgeJob(lister, 0) - t.Cleanup(stop) - testkit.WaitFor(t, 5*time.Second, func() (bool, error) { - _, found, err := cache.Get("avatar", moderationTestOwner, moderationTestCID) - return !found, err - }, testkit.WithDescription("startup sweep removes bytes a lost purge left on disk")) -} diff --git a/internal/core/imageproxy/service.go b/internal/core/imageproxy/service.go index 4cd5c49..2288f3b 100644 --- a/internal/core/imageproxy/service.go +++ b/internal/core/imageproxy/service.go @@ -526,42 +526,6 @@ func (s *ImageProxyService) PurgeActiveBlocks(ctx context.Context, lister Blocke return errors.Join(errs...) } -// StartActiveBlockPurgeJob runs PurgeActiveBlocks once straight away, which -// completes any purge a restart interrupted, and then every interval. An -// interval of zero or less runs only the startup sweep. The returned function -// stops the job. -func (s *ImageProxyService) StartActiveBlockPurgeJob(lister BlockedBlobLister, interval time.Duration) context.CancelFunc { - ctx, cancel := context.WithCancel(context.Background()) - go func() { - defer func() { - if r := recover(); r != nil { - slog.Error("[IMAGE-PROXY] CRITICAL: blocked media purge job panicked", "panic", r) - } - }() - s.runActiveBlockPurge(ctx, lister) - if interval <= 0 { - return - } - ticker := time.NewTicker(interval) - defer ticker.Stop() - for { - select { - case <-ctx.Done(): - return - case <-ticker.C: - s.runActiveBlockPurge(ctx, lister) - } - } - }() - return cancel -} - -func (s *ImageProxyService) runActiveBlockPurge(ctx context.Context, lister BlockedBlobLister) { - if err := s.PurgeActiveBlocks(ctx, lister); err != nil && ctx.Err() == nil { - slog.Error("[IMAGE-PROXY] blocked media purge failed", "error", err) - } -} - // lockPublication takes cid's publication stripe, giving up when ctx ends. func (s *ImageProxyService) lockPublication(ctx context.Context, cid string) (release func(), err error) { lock := s.publicationLock(cid) diff --git a/internal/core/moderation/cdn_purge_attempt_test.go b/internal/core/moderation/cdn_purge_attempt_test.go new file mode 100644 index 0000000..99e50bd --- /dev/null +++ b/internal/core/moderation/cdn_purge_attempt_test.go @@ -0,0 +1,192 @@ +package moderation_test + +import ( + "context" + "log/slog" + "strings" + "sync" + "testing" + "time" + + "Coves/internal/core/imageproxy" + "Coves/internal/core/moderation" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type cdnAttemptReschedule struct { + blob imageproxy.BlockedBlob + nextAttemptAt time.Time + failureCode string +} + +// cdnAttemptStore hands out its targets on the first claim and records how +// each outcome is written. +type cdnAttemptStore struct { + mu sync.Mutex + targets []moderation.CDNPurgeTarget + claimed bool + reschedules []cdnAttemptReschedule + completed []imageproxy.BlockedBlob +} + +func (store *cdnAttemptStore) RecordCDNPurgeTargets(_ context.Context, blobs []imageproxy.BlockedBlob) ([]imageproxy.BlockedBlob, error) { + return blobs, nil +} + +func (store *cdnAttemptStore) ClaimDueCDNPurgeTargets(context.Context, moderation.CDNPurgeClaim) ([]moderation.CDNPurgeTarget, error) { + store.mu.Lock() + defer store.mu.Unlock() + if store.claimed { + return nil, nil + } + store.claimed = true + return store.targets, nil +} + +func (store *cdnAttemptStore) CompleteCDNPurgeTarget(_ context.Context, target moderation.CDNPurgeTarget) error { + store.mu.Lock() + defer store.mu.Unlock() + store.completed = append(store.completed, target.Blob) + return nil +} + +func (store *cdnAttemptStore) RescheduleCDNPurgeTarget(_ context.Context, target moderation.CDNPurgeTarget, nextAttemptAt time.Time, failureCode string) error { + store.mu.Lock() + defer store.mu.Unlock() + store.reschedules = append(store.reschedules, cdnAttemptReschedule{blob: target.Blob, nextAttemptAt: nextAttemptAt, failureCode: failureCode}) + return nil +} + +type cdnAttemptPurgerFunc func(context.Context, []imageproxy.BlockedBlob) imageproxy.CDNPurgeResult + +func (purge cdnAttemptPurgerFunc) PurgeBlobs(ctx context.Context, blobs []imageproxy.BlockedBlob) imageproxy.CDNPurgeResult { + return purge(ctx, blobs) +} + +var cdnAttemptNow = time.Date(2026, 10, 3, 12, 0, 0, 0, time.UTC) + +func cdnAttemptQueue(store *cdnAttemptStore, purger moderation.CDNPurger) *moderation.CDNPurgeQueue { + return moderation.NewCDNPurgeQueue(store, purger, moderation.CDNPurgeQueueConfig{ + WriteTimeout: time.Second, + Now: func() time.Time { return cdnAttemptNow }, + }) +} + +// A target the sweep's context cut off was never refused by the edge, so it +// must neither count as a failed attempt nor back off. +func TestCDNPurgeQueueInterruptedTargetIsDueAgainWithoutCountingAnAttempt(t *testing.T) { + refused := imageproxy.BlockedBlob{OwnerDID: cdnRecordOwnerDID, CID: cdnRecordCanonicalCID} + cutOff := imageproxy.BlockedBlob{OwnerDID: cdnRecordOwnerDID, CID: cdnRecordSecondCID} + store := &cdnAttemptStore{targets: []moderation.CDNPurgeTarget{ + {Blob: refused, Attempts: 2}, + {Blob: cutOff, Attempts: 3}, + }} + sweepCtx, cancelSweep := context.WithCancel(t.Context()) + defer cancelSweep() + queue := cdnAttemptQueue(store, cdnAttemptPurgerFunc(func(context.Context, []imageproxy.BlockedBlob) imageproxy.CDNPurgeResult { + cancelSweep() + return imageproxy.CDNPurgeResult{Failed: []imageproxy.CDNPurgeFailure{ + {Blob: refused, Code: "http_500"}, + {Blob: cutOff, Code: "transport"}, + }} + })) + + assert.ErrorIs(t, queue.Sweep(sweepCtx), context.Canceled) + + assert.ElementsMatch(t, []cdnAttemptReschedule{ + {blob: refused, nextAttemptAt: cdnAttemptNow.Add(4 * time.Minute), failureCode: "http_500"}, + {blob: cutOff, nextAttemptAt: cdnAttemptNow, failureCode: ""}, + }, store.reschedules) +} + +// Each claimed batch's lease runs cdnPurgeLease from its claim, so the request +// for that batch must end before the lease can pass to another worker even when +// the caller's context carries no deadline. +func TestCDNPurgeQueueSweepBoundsEachAttempt(t *testing.T) { + blob := imageproxy.BlockedBlob{OwnerDID: cdnRecordOwnerDID, CID: cdnRecordCanonicalCID} + store := &cdnAttemptStore{targets: []moderation.CDNPurgeTarget{{Blob: blob}}} + var deadline time.Time + var hasDeadline bool + queue := cdnAttemptQueue(store, cdnAttemptPurgerFunc(func(ctx context.Context, blobs []imageproxy.BlockedBlob) imageproxy.CDNPurgeResult { + deadline, hasDeadline = ctx.Deadline() + return imageproxy.CDNPurgeResult{Acknowledged: blobs} + })) + + require.NoError(t, queue.Sweep(context.Background())) + + require.True(t, hasDeadline, "a sweep attempt must run under its own deadline") + assert.WithinDuration(t, time.Now(), deadline, 2*time.Minute, "the attempt deadline must fall inside the claim lease") +} + +type cdnAttemptLogCapture struct { + mu sync.Mutex + records []slog.Record +} + +func (*cdnAttemptLogCapture) Enabled(context.Context, slog.Level) bool { return true } + +func (capture *cdnAttemptLogCapture) Handle(_ context.Context, record slog.Record) error { + capture.mu.Lock() + defer capture.mu.Unlock() + capture.records = append(capture.records, record.Clone()) + return nil +} + +func (capture *cdnAttemptLogCapture) WithAttrs([]slog.Attr) slog.Handler { return capture } +func (capture *cdnAttemptLogCapture) WithGroup(string) slog.Handler { return capture } + +func (capture *cdnAttemptLogCapture) errors() []slog.Record { + capture.mu.Lock() + defer capture.mu.Unlock() + var errors []slog.Record + for _, record := range capture.records { + if record.Level == slog.LevelError { + errors = append(errors, record) + } + } + return errors +} + +// A refused credential fails every target of every attempt. One line per +// attempt that names the cause is a signal; one line per target is noise. +// +// Not parallel: this test replaces slog.Default while the sweep runs. +func TestCDNPurgeQueueLogsOneAggregatedErrorPerFailureCode(t *testing.T) { + blobs := []imageproxy.BlockedBlob{ + {OwnerDID: cdnRecordOwnerDID, CID: cdnRecordCanonicalCID}, + {OwnerDID: cdnRecordOwnerDID, CID: cdnRecordSecondCID}, + {OwnerDID: "did:plc:cdnrecordsecondowner", CID: cdnRecordCanonicalCID}, + } + targets := make([]moderation.CDNPurgeTarget, len(blobs)) + for i, blob := range blobs { + targets[i] = moderation.CDNPurgeTarget{Blob: blob} + } + store := &cdnAttemptStore{targets: targets} + queue := cdnAttemptQueue(store, cdnAttemptPurgerFunc(func(_ context.Context, blobs []imageproxy.BlockedBlob) imageproxy.CDNPurgeResult { + var result imageproxy.CDNPurgeResult + for _, blob := range blobs { + result.Failed = append(result.Failed, imageproxy.CDNPurgeFailure{Blob: blob, Code: "http_403"}) + } + return result + })) + logs := &cdnAttemptLogCapture{} + previousLogger := slog.Default() + slog.SetDefault(slog.New(logs)) + t.Cleanup(func() { slog.SetDefault(previousLogger) }) + + require.NoError(t, queue.Sweep(t.Context())) + + errors := logs.errors() + require.Len(t, errors, 1, "one error line per failure code per attempt") + attrs := make(map[string]slog.Value) + errors[0].Attrs(func(attr slog.Attr) bool { + attrs[attr.Key] = attr.Value + return true + }) + assert.Equal(t, "http_403", attrs["code"].String()) + assert.Equal(t, int64(3), attrs["count"].Int64()) + assert.NotContains(t, attrs, "cid") + assert.Contains(t, strings.ToLower(errors[0].Message), "credentials") +} diff --git a/internal/core/moderation/cdn_purge_targets.go b/internal/core/moderation/cdn_purge_targets.go index 21db0a1..d7fda27 100644 --- a/internal/core/moderation/cdn_purge_targets.go +++ b/internal/core/moderation/cdn_purge_targets.go @@ -67,17 +67,20 @@ const ( // cdnPurgeLease is how long a claim holds a target before another worker // may claim it. cdnPurgeLease = 2 * time.Minute - // cdnPurgeImmediateAttemptTimeout bounds an immediate post-commit attempt. - // With cdnPurgeOutcomeWriteTimeout it stays below cdnPurgeLease, so the - // attempt's outcome is written before its lease can pass to a sweep. - cdnPurgeImmediateAttemptTimeout = 90 * time.Second + // cdnPurgeAttemptTimeout bounds one claimed batch's purge request, both for + // an immediate post-commit attempt and for each batch of a sweep. With + // cdnPurgeOutcomeWriteTimeout it stays below cdnPurgeLease, so the + // attempt's outcome is written before its lease can pass to another worker. + cdnPurgeAttemptTimeout = 90 * time.Second // cdnPurgeOutcomeWriteTimeout bounds writing an attempt's outcomes, which // runs after the attempt's own context may have ended. cdnPurgeOutcomeWriteTimeout = 5 * time.Second // cdnPurgeImmediateAttemptLimit caps immediate attempts in flight. Beyond // it, recorded targets wait for the next sweep. cdnPurgeImmediateAttemptLimit = 4 - // cdnPurgeInterruptedCode records an attempt cut off by its context. + // cdnPurgeInterruptedCode marks a target whose attempt its context cut off. + // It is never stored: the target is due again at once and its attempt is + // not counted, because the edge never refused it. cdnPurgeInterruptedCode = "interrupted" ) @@ -152,7 +155,7 @@ func (q *CDNPurgeQueue) PurgeAfterCommit(ctx context.Context, blobs []imageproxy } q.mu.Unlock() }() - attemptCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), cdnPurgeImmediateAttemptTimeout) + attemptCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), cdnPurgeAttemptTimeout) defer cancel() for start := 0; start < len(owned); start += q.config.BatchSize { if attemptCtx.Err() != nil { @@ -184,7 +187,9 @@ func (q *CDNPurgeQueue) claim(blobs []imageproxy.BlockedBlob, dueAt time.Time) C // Sweep drains targets due at the start of this pass. Freezing the due cutoff // keeps a target rescheduled while a request is held from being retried by the // same sweep if the clock advances during that request. The store still samples -// the live clock after locking rows for each target's completion anchor. +// the live clock after locking rows for each target's completion anchor. Each +// batch's request is bounded by cdnPurgeAttemptTimeout so it ends inside the +// batch's claim lease. func (q *CDNPurgeQueue) Sweep(ctx context.Context) error { dueAt := q.config.Now() var attempted []imageproxy.BlockedBlob @@ -198,7 +203,10 @@ func (q *CDNPurgeQueue) Sweep(ctx context.Context) error { if len(targets) == 0 { return ctx.Err() } - if err := q.attempt(ctx, targets); err != nil { + attemptCtx, cancel := context.WithTimeout(ctx, cdnPurgeAttemptTimeout) + err = q.attempt(attemptCtx, targets) + cancel() + if err != nil { return err } for _, target := range targets { @@ -258,6 +266,8 @@ func (q *CDNPurgeQueue) attempt(ctx context.Context, targets []CDNPurgeTarget) e } codes := make([]string, len(targets)) interruptedCount := 0 + failedCounts := make(map[string]int) + var failedCodes []string for i, target := range targets { if acknowledged[target.Blob] { continue @@ -272,7 +282,14 @@ func (q *CDNPurgeQueue) attempt(ctx context.Context, targets []CDNPurgeTarget) e code = "unacknowledged" } codes[i] = code - slog.Error("moderation CDN purge failed", "did", target.Blob.OwnerDID, "cid", target.Blob.CID, "code", code) + if failedCounts[code] == 0 { + failedCodes = append(failedCodes, code) + } + failedCounts[code]++ + slog.Debug("moderation CDN purge target failed", "did", target.Blob.OwnerDID, "cid", target.Blob.CID, "code", code) + } + for _, code := range failedCodes { + logCDNPurgeFailure(code, failedCounts[code]) } if interruptedCount > 0 { slog.Warn("moderation CDN purge interrupted; targets rescheduled", "count", interruptedCount) @@ -286,6 +303,8 @@ func (q *CDNPurgeQueue) attempt(ctx context.Context, targets []CDNPurgeTarget) e var err error operation := "reschedule" switch { + case codes[i] == cdnPurgeInterruptedCode: + err = q.store.RescheduleCDNPurgeTarget(writeCtx, target, attemptedAt, "") case codes[i] != "": err = q.store.RescheduleCDNPurgeTarget(writeCtx, target, attemptedAt.Add(cdnPurgeRetryDelay(target.Attempts+1)), codes[i]) case !attemptedAt.Before(target.EarliestCompletionAt): @@ -302,6 +321,18 @@ func (q *CDNPurgeQueue) attempt(ctx context.Context, targets []CDNPurgeTarget) e return errors.Join(writeErrors...) } +// logCDNPurgeFailure writes one line per failure code per attempt. A refused +// credential or zone fails every target of every attempt, so a line per target +// would bury the cause; the per-target detail is at Debug. +func logCDNPurgeFailure(code string, count int) { + if code == "http_401" || code == "http_403" { + slog.Error("moderation CDN purge refused: Cloudflare rejected the purge credentials or zone; removed images may stay at the edge until the API token and zone ID are fixed", + "code", code, "count", count) + return + } + slog.Error("moderation CDN purge failed", "code", code, "count", count) +} + // WithCDNPurgeTargets enables transactionally recorded CDN targets. func WithCDNPurgeTargets(targets CDNPurgeTargets) MediaReconcilerOption { return func(reconciler *MediaReconciler) { reconciler.cdnPurgeTargets = targets } diff --git a/internal/core/moderation/modlog.go b/internal/core/moderation/modlog.go index 97e209c..9721999 100644 --- a/internal/core/moderation/modlog.go +++ b/internal/core/moderation/modlog.go @@ -57,6 +57,17 @@ func hiddenAction(action Action) bool { return false } +// restrictedSubject reports a subject whose existence the public log must not +// reveal (PRD §6): its URI, CID and community stay out of the public view. +func restrictedSubject(action Action) bool { + return action.SubjectAccess == SubjectAccessRestricted +} + +// privateSubject reports an action whose subject reference is admin-only. +func privateSubject(action Action) bool { + return hiddenAction(action) || restrictedSubject(action) +} + // ActionView is the public defs#actionView projection of an action. type ActionView struct { Ref ActionRefView `json:"ref"` @@ -115,9 +126,13 @@ func NewActionView(action Action) ActionView { Reason: action.Reason, LabelValue: action.LabelValue, } - if !hiddenAction(action) { + if !privateSubject(action) { view.Subject = &SubjectRefView{URI: action.SubjectURI, CID: action.ObservedCID} } + if restrictedSubject(action) { + // A community scope would tie the restricted subject to its community. + view.Scope.CommunityDID = "" + } if action.ActorDID != "" { view.Actor = &ActorRefView{DID: action.ActorDID} } @@ -134,7 +149,7 @@ func NewAdminActionView(action Action) AdminActionView { ActorDID: action.ActorDID, PrivateNote: action.PrivateNote, } - if hiddenAction(action) { + if privateSubject(action) { view.PrivateSubject = &SubjectRefView{URI: action.SubjectURI, CID: action.ObservedCID} } return view @@ -214,6 +229,12 @@ func listActionPage[T any](ctx context.Context, s *service, params ListActionsPa return nil, "", fmt.Errorf("%w: list %s actions: %w", ErrModerationUnavailable, visibility, err) } for _, row := range rows { + if row.SubjectAccess != SubjectAccessPublic && row.SubjectAccess != SubjectAccessRestricted { + return nil, "", fmt.Errorf("%w: list %s actions: store returned action %s with subject access %q", ErrModerationUnavailable, visibility, row.ID, row.SubjectAccess) + } + if query.ExcludeRestricted && restrictedSubject(row) { + return nil, "", fmt.Errorf("%w: list %s actions: store returned restricted action %s despite ExcludeRestricted", ErrModerationUnavailable, visibility, row.ID) + } if query.ExcludeHidden && hiddenAction(row) { return nil, "", fmt.Errorf("%w: list %s actions: store returned hidden action %s despite ExcludeHidden", ErrModerationUnavailable, visibility, row.ID) } @@ -303,7 +324,10 @@ func (s *service) actionListQuery(ctx context.Context, params ListActionsParams, } query.Before = &cursor.key } + // A filter that selects by subject, collection or community would confirm + // what the projection withholds, so such rows are not listed at all. query.ExcludeHidden = !admin && (params.Subject != "" || params.Collection != "" || params.Community != "") + query.ExcludeRestricted = query.ExcludeHidden query.ExcludeLabelActions = !admin return query, digest, nil } diff --git a/internal/core/moderation/modlog_filters_test.go b/internal/core/moderation/modlog_filters_test.go index 60a77ce..9329af1 100644 --- a/internal/core/moderation/modlog_filters_test.go +++ b/internal/core/moderation/modlog_filters_test.go @@ -254,6 +254,7 @@ func TestModlogFilterHiddenExclusionDependsOnlyOnSubjectDetail(t *testing.T) { require.NoError(t, err) require.Len(t, store.listQueries, 1) assert.Equal(t, test.hidden && !admin, store.listQueries[0].ExcludeHidden) + assert.Equal(t, test.hidden && !admin, store.listQueries[0].ExcludeRestricted) }) } } diff --git a/internal/core/moderation/modlog_restricted_test.go b/internal/core/moderation/modlog_restricted_test.go new file mode 100644 index 0000000..a644137 --- /dev/null +++ b/internal/core/moderation/modlog_restricted_test.go @@ -0,0 +1,94 @@ +package moderation_test + +import ( + "fmt" + "testing" + "time" + + "Coves/internal/core/moderation" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A restricted subject is projected like a hidden one: the public view names +// neither the subject nor a community scope, and the admin view keeps the +// subject in privateSubject. +func TestModlogRestrictedSubjectProjection(t *testing.T) { + for _, reason := range []string{modlogReasonSpam, modlogReasonIllegal} { + t.Run(reason, func(t *testing.T) { + action := modlogTestAction() + action.Reason = reason + action.SubjectAccess = moderation.SubjectAccessRestricted + action.ScopeKind = "community" + action.ScopeCommunityDID = "did:plc:scopecommunity" + public := moderation.NewActionView(action) + object, raw := modlogJSON(t, public) + assert.Nil(t, public.Subject) + assert.NotContains(t, object, "subject") + assert.Equal(t, moderation.ScopeView{Kind: "community"}, public.Scope) + for _, private := range []string{action.SubjectURI, action.ObservedCID, action.ScopeCommunityDID, action.SubjectCommunityDID} { + assert.NotContains(t, string(raw), private) + } + assert.Equal(t, action.Reason, public.Reason) + + admin := moderation.NewAdminActionView(action) + assert.Equal(t, public, admin.Action) + assert.Equal(t, &moderation.SubjectRefView{URI: action.SubjectURI, CID: action.ObservedCID}, admin.PrivateSubject) + }) + } + public := modlogTestAction() + public.ScopeCommunityDID = "did:plc:scopecommunity" + view := moderation.NewActionView(public) + assert.Equal(t, &moderation.SubjectRefView{URI: public.SubjectURI, CID: public.ObservedCID}, view.Subject) + assert.Equal(t, public.ScopeCommunityDID, view.Scope.CommunityDID) + assert.Nil(t, moderation.NewAdminActionView(public).PrivateSubject) +} + +// SQL and Go must agree on restricted subjects. A restricted row returned under +// ExcludeRestricted means they disagree, and the public page fails closed. +func TestModlogListFailsClosedWhenStoreReturnsRestrictedRowUnderExclusion(t *testing.T) { + for _, lookAhead := range []bool{false, true} { + t.Run(fmt.Sprintf("look-ahead=%t", lookAhead), func(t *testing.T) { + rows := modlogCursorRows() + index := 0 + if lookAhead { + index = 2 // The unprojected look-ahead row still proves the disagreement. + } + rows[index].SubjectAccess = moderation.SubjectAccessRestricted + store := newInMemoryModerationStore(time.Now()) + store.listRows = rows + service := modlogFilterService(store, nil, nil) + params := moderation.ListActionsParams{Limit: modlogLimit(2), Community: "did:plc:community"} + page, err := modlogFilterList(t, service, false, params, "") + require.ErrorIs(t, err, moderation.ErrModerationUnavailable) + assert.Nil(t, page) + + unfiltered, err := service.ListActions(t.Context(), moderation.ListActionsParams{Limit: modlogLimit(2)}) + require.NoError(t, err, "an unfiltered public page projects restricted rows without their subject") + if !lookAhead { + assert.Nil(t, unfiltered.Actions[0].Subject) + } + _, err = modlogFilterList(t, service, true, params, "") + require.NoError(t, err, "the admin log never excludes restricted rows") + }) + } +} + +// Every listed row must say whether its subject is public. A row without that +// state cannot be projected safely, on either log. +func TestModlogListFailsClosedOnUndecidedSubjectAccess(t *testing.T) { + for _, access := range []moderation.SubjectAccess{"", "unknown"} { + for _, admin := range []bool{false, true} { + t.Run(fmt.Sprintf("%q/admin=%t", access, admin), func(t *testing.T) { + rows := modlogCursorRows() + rows[2].SubjectAccess = access + store := newInMemoryModerationStore(time.Now()) + store.listRows = rows + page, err := modlogFilterList(t, modlogFilterService(store, nil, nil), admin, moderation.ListActionsParams{}, "") + require.ErrorIs(t, err, moderation.ErrModerationUnavailable) + assert.Nil(t, page) + }) + } + } +} diff --git a/internal/core/moderation/modlog_test.go b/internal/core/moderation/modlog_test.go index 5d45a37..298a52e 100644 --- a/internal/core/moderation/modlog_test.go +++ b/internal/core/moderation/modlog_test.go @@ -36,6 +36,7 @@ func modlogTestAction() moderation.Action { PrivateNote: "SECRET-NOTE-operator-only 🔒\nsecond line", Origin: moderation.OriginLocal, CreatedAt: time.Date(2026, 9, 28, 15, 4, 5, 123456789, time.FixedZone("UTC+3", 3*60*60)), + SubjectAccess: moderation.SubjectAccessPublic, } } diff --git a/internal/core/moderation/remove.go b/internal/core/moderation/remove.go index 4530ae4..8073088 100644 --- a/internal/core/moderation/remove.go +++ b/internal/core/moderation/remove.go @@ -147,6 +147,12 @@ func (s *service) removeContent(ctx context.Context, actorDID string, request Re if err := tx.SetSubjectVersion(ctx, request.Subject.URI, version+1); err != nil { return unavailable(err) } + // An empty owner DID on a media block means every owner, so a + // subject that lost its owner is refused rather than blocking its + // images for everyone. + if subject.OwnerDID == "" && len(subject.BlobCIDs) > 0 { + return unavailable(errors.New("indexed subject has images but no owner DID")) + } newlyBlocked = imageMediaBlocks(subject, action) if len(newlyBlocked) > 0 { if err := tx.InsertMediaBlocks(ctx, newlyBlocked); err != nil { diff --git a/internal/core/moderation/remove_rules_test.go b/internal/core/moderation/remove_rules_test.go index 855f769..ca34af4 100644 --- a/internal/core/moderation/remove_rules_test.go +++ b/internal/core/moderation/remove_rules_test.go @@ -167,6 +167,23 @@ func TestRemoveContentRules(t *testing.T) { assert.Equal(t, []string{removeRulesFirstImage, removeRulesSecondImage}, scenario.purger.blobPurges) }) + // An empty owner DID on a media block means every owner, so an indexed + // subject that lost its owner must not turn a spam removal into an + // every-owner block. + t.Run("indexed subject with images and no owner is refused without blocks", func(t *testing.T) { + scenario := newRemoveRulesScenario() + comment := scenario.store.state.indexedComments[removeRulesURI] + comment.OwnerDID = "" + scenario.store.state.indexedComments[removeRulesURI] = comment + result, err := scenario.service.RemoveContent(t.Context(), removeRulesAdminDID, scenario.request) + require.ErrorIs(t, err, moderation.ErrModerationUnavailable) + assert.Nil(t, result) + assert.Empty(t, scenario.store.state.mediaBlocks) + assert.Empty(t, scenario.store.state.actions) + assert.Empty(t, scenario.purger.ownerPurges) + assert.Empty(t, scenario.purger.blobPurges) + }) + for _, test := range []struct { name string change func(*removeRulesScenario) diff --git a/internal/core/moderation/store.go b/internal/core/moderation/store.go index a602dcc..2aa6792 100644 --- a/internal/core/moderation/store.go +++ b/internal/core/moderation/store.go @@ -53,8 +53,25 @@ type Action struct { ReversedActionReason string Origin string CreatedAt time.Time + // SubjectAccess is whether the public may know the subject exists, decided + // when the log is read. Store.ListActions must set it on every row; the + // service refuses a row that leaves it unset. It is read-time state, so it + // stays out of stored idempotency results. + SubjectAccess SubjectAccess `json:"-"` } +// SubjectAccess says whether an action's subject is content the anonymous +// public can see. It follows post.get's anonymous admission rule: a post is +// public when its own community admitted it (or, outside postv2, when it has +// no admission row), and a comment follows its root post. A subject with no +// indexed content row to decide from is restricted. +type SubjectAccess string + +const ( + SubjectAccessPublic SubjectAccess = "public" + SubjectAccessRestricted SubjectAccess = "restricted" +) + // IndexedComment is the indexed comment row a mutation inspects, read under a // share lock so consumer writes serialize against the CID check. type IndexedComment struct { @@ -116,9 +133,11 @@ type Store interface { InTransaction(ctx context.Context, fn func(ctx context.Context, tx Transaction) error) error SubjectModeration(ctx context.Context, authorityDID, subjectURI string) (*SubjectModeration, error) // ListActions returns up to query.Limit actions, newest first, each with - // ReversedActionReason populated. Under ExcludeHidden it must omit every - // hidden action, and under ExcludeLabelActions every action whose kind is - // in PublicExcludedActions; the service fails closed if either comes back. + // ReversedActionReason and SubjectAccess populated. Under ExcludeHidden it + // must omit every hidden action, under ExcludeLabelActions every action + // whose kind is in PublicExcludedActions, and under ExcludeRestricted every + // action whose subject is not SubjectAccessPublic; the service fails closed + // if any of them comes back. ListActions(ctx context.Context, query ActionListQuery) ([]Action, error) } @@ -139,6 +158,8 @@ type ActionListQuery struct { Since *time.Time Until *time.Time ExcludeHidden bool + // ExcludeRestricted omits every action whose subject is restricted. + ExcludeRestricted bool // ExcludeLabelActions omits every action kind in PublicExcludedActions. ExcludeLabelActions bool } diff --git a/internal/db/migrations/052_moderation_media_purges.sql b/internal/db/migrations/052_moderation_media_purges.sql index 4f243d3..ca7ea83 100644 --- a/internal/db/migrations/052_moderation_media_purges.sql +++ b/internal/db/migrations/052_moderation_media_purges.sql @@ -1,6 +1,6 @@ -- +goose Up CREATE TABLE moderation_media_purges ( - owner_did TEXT NOT NULL, + owner_did TEXT NOT NULL CHECK (owner_did <> ''), blob_cid TEXT NOT NULL, state TEXT NOT NULL DEFAULT 'pending' CHECK (state IN ('pending', 'completed')), attempts INTEGER NOT NULL DEFAULT 0 CHECK (attempts >= 0), diff --git a/internal/db/postgres/moderation_action_log_integration_test.go b/internal/db/postgres/moderation_action_log_integration_test.go index f1967ad..f770ddd 100644 --- a/internal/db/postgres/moderation_action_log_integration_test.go +++ b/internal/db/postgres/moderation_action_log_integration_test.go @@ -237,6 +237,8 @@ func TestModerationActionLogHiddenReasonsAndFullRowMapping(t *testing.T) { want.ReversedActionReason = byID[want.ReversesActionID].Reason } want.CreatedAt = want.CreatedAt.UTC() + // With no indexed content to decide from, every subject is restricted. + want.SubjectAccess = moderation.SubjectAccessRestricted assert.Equal(t, want, byID[want.ID], want.ID) assert.Equal(t, time.UTC, byID[want.ID].CreatedAt.Location(), want.ID) } diff --git a/internal/db/postgres/moderation_action_log_restricted_integration_test.go b/internal/db/postgres/moderation_action_log_restricted_integration_test.go new file mode 100644 index 0000000..f4d0c28 --- /dev/null +++ b/internal/db/postgres/moderation_action_log_restricted_integration_test.go @@ -0,0 +1,213 @@ +//go:build integration + +package postgres_test + +import ( + "database/sql" + "encoding/json" + "testing" + "time" + + "Coves/internal/core/moderation" + "Coves/internal/db/postgres" + "Coves/tests/fixtures" + "Coves/tests/testkit" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func restrictedLogService(db *sql.DB) moderation.Service { + return moderation.NewService( + moderation.NewRepositorySubjectReader(postgres.NewPostRepository(db), postgres.NewCommentRepository(db)), + postgres.NewModerationRepository(db), + moderation.Config{ + InstanceDID: fixtures.InstanceDID(), IdempotencyRetention: 24 * time.Hour, + MaxLiveIdempotencyKeys: 1000, CursorSecret: "restricted-log-cursor-secret", + }, + ) +} + +func restrictedLogComment(t *testing.T, db *sql.DB, authorDID string, root moderation.StrongRef) moderation.StrongRef { + t.Helper() + rkey := testkit.TID() + subject := moderation.StrongRef{URI: "at://" + authorDID + "/" + moderation.CommentCollection + "/" + rkey, CID: moderationCommentCID} + _, err := db.ExecContext(t.Context(), ` + 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, 'restricted log comment', NOW()) + `, subject.URI, subject.CID, rkey, authorDID, root.URI, root.CID) + require.NoError(t, err) + return subject +} + +func restrictedLogRemove(t *testing.T, service moderation.Service, subject moderation.StrongRef) string { + t.Helper() + result, err := service.RemoveContent(t.Context(), fixtures.DID("restrictedlogadmin"), moderation.RemoveContentRequest{ + Subject: subject, ExpectedVersion: "v0", IdempotencyKey: testkit.UniqueIDWithPrefix(t, "remove"), + Reason: "social.coves.moderation.defs#reasonSpam", + }) + require.NoError(t, err) + require.Equal(t, moderation.OutcomeApplied, result.Outcome) + require.NotNil(t, result.Action) + return result.Action.ID +} + +func restrictedLogPage(t *testing.T, service moderation.Service, params moderation.ListActionsParams) (*moderation.ActionPage, string) { + t.Helper() + page, err := service.ListActions(t.Context(), params) + require.NoError(t, err) + encoded, err := json.Marshal(page) + require.NoError(t, err) + return page, string(encoded) +} + +func restrictedLogIDs(page *moderation.ActionPage) []string { + ids := make([]string, 0, len(page.Actions)) + for _, action := range page.Actions { + ids = append(ids, action.Ref.ActionID) + } + return ids +} + +// A removal of content the public cannot see must not disclose it through the +// public modlog (PRD §6 "restricted subjects do not leak"). Restricted-ness is +// post.get's anonymous admission rule, evaluated when the log is read: a +// pending, rejected, community-removed or unadmitted postv2, and a comment +// whose root post is one of those, is listed without its subject and is +// excluded whenever a subject, collection or community filter could tie it to +// its target. A post admitted later shows normally. +func TestModerationActionLogRestrictsNonPublicSubjects(t *testing.T) { + db := testkit.DB(t) + communityDID, authorDID := moderationPostTombstoneActors(t, db) + service := restrictedLogService(db) + + post := func(title, admissionSQL string) moderation.StrongRef { + subject := indexedModerationPost(t, db, moderation.PostV2Collection, communityDID, authorDID, title, "") + if admissionSQL != "" { + _, err := db.ExecContext(t.Context(), admissionSQL, communityDID, subject.URI) + require.NoError(t, err) + } + return subject + } + const pendingSQL = `UPDATE community_post_admissions SET status = 'pending', accepted_cid = NULL, updated_at = NOW() + WHERE community_did = $1 AND post_uri = $2` + accepted := post("accepted", "") + pending := post("pending", pendingSQL) + rejected := post("rejected", `UPDATE community_post_admissions SET status = 'rejected', accepted_cid = NULL, + decision_code = 'off-topic', decision_at = NOW(), updated_at = NOW() WHERE community_did = $1 AND post_uri = $2`) + communityRemoved := post("community removed", `UPDATE community_post_admissions SET status = 'removed', accepted_cid = NULL, + decision_code = 'rule-violation', decision_at = NOW(), updated_at = NOW() WHERE community_did = $1 AND post_uri = $2`) + unadmitted := post("unadmitted", `DELETE FROM community_post_admissions WHERE community_did = $1 AND post_uri = $2`) + legacy := indexedModerationPost(t, db, moderation.LegacyPostCollection, communityDID, authorDID, "legacy", "") + pendingRoot := post("pending root", pendingSQL) + publicComment := restrictedLogComment(t, db, authorDID, accepted) + restrictedComment := restrictedLogComment(t, db, authorDID, pendingRoot) + + type entry struct { + subject moderation.StrongRef + restricted bool + } + entries := map[string]entry{} + var publicIDs []string + for _, subject := range []struct { + ref moderation.StrongRef + restricted bool + }{ + {accepted, false}, + {pending, true}, + {rejected, true}, + {communityRemoved, true}, + {unadmitted, true}, + {legacy, false}, + {publicComment, false}, + {restrictedComment, true}, + } { + id := restrictedLogRemove(t, service, subject.ref) + entries[id] = entry{subject.ref, subject.restricted} + if !subject.restricted { + publicIDs = append([]string{id}, publicIDs...) + } + } + + t.Run("unfiltered public log lists restricted actions without their subjects", func(t *testing.T) { + page, encoded := restrictedLogPage(t, service, moderation.ListActionsParams{}) + require.Len(t, page.Actions, len(entries)) + for _, action := range page.Actions { + want, found := entries[action.Ref.ActionID] + require.True(t, found, action.Ref.ActionID) + assert.Empty(t, action.Scope.CommunityDID, action.Ref.ActionID) + if want.restricted { + assert.Nil(t, action.Subject, "a restricted subject must not appear in the public log") + assert.NotContains(t, encoded, want.subject.URI) + } else { + assert.Equal(t, &moderation.SubjectRefView{URI: want.subject.URI, CID: want.subject.CID}, action.Subject) + } + } + }) + + t.Run("admin log keeps restricted subjects private", func(t *testing.T) { + page, err := service.ListAdminActions(t.Context(), moderation.ListAdminActionsParams{}) + require.NoError(t, err) + require.Len(t, page.Actions, len(entries)) + for _, action := range page.Actions { + want := entries[action.Action.Ref.ActionID] + ref := &moderation.SubjectRefView{URI: want.subject.URI, CID: want.subject.CID} + if want.restricted { + assert.Nil(t, action.Action.Subject) + assert.Equal(t, ref, action.PrivateSubject) + } else { + assert.Equal(t, ref, action.Action.Subject) + assert.Nil(t, action.PrivateSubject) + } + } + }) + + t.Run("filters never select a restricted subject", func(t *testing.T) { + page, _ := restrictedLogPage(t, service, moderation.ListActionsParams{Community: communityDID}) + assert.Equal(t, publicIDs, restrictedLogIDs(page), "the community filter must not tie restricted subjects to the community") + page, _ = restrictedLogPage(t, service, moderation.ListActionsParams{Collection: moderation.PostV2Collection}) + assert.Equal(t, []string{publicIDs[2]}, restrictedLogIDs(page), "only the accepted postv2 may match its collection") + page, _ = restrictedLogPage(t, service, moderation.ListActionsParams{Collection: moderation.CommentCollection}) + assert.Equal(t, []string{publicIDs[0]}, restrictedLogIDs(page), "only the comment on an accepted root may match its collection") + never := "at://" + authorDID + "/" + moderation.PostV2Collection + "/" + testkit.TID() + _, neverBody := restrictedLogPage(t, service, moderation.ListActionsParams{Subject: never}) + for id, want := range entries { + page, body := restrictedLogPage(t, service, moderation.ListActionsParams{Subject: want.subject.URI}) + if want.restricted { + assert.Equal(t, neverBody, body, "a restricted subject filter must answer exactly as a never-moderated subject") + } else { + assert.Equal(t, []string{id}, restrictedLogIDs(page)) + } + } + }) + + t.Run("filtered pagination skips restricted rows before the limit", func(t *testing.T) { + one := 1 + params := moderation.ListActionsParams{Community: communityDID, Limit: &one} + var walked []string + for pages := 1; ; pages++ { + require.LessOrEqual(t, pages, len(publicIDs), "every page of the walk must hold a public action") + page, _ := restrictedLogPage(t, service, params) + require.Len(t, page.Actions, 1, "a page must not come back short because restricted rows used its limit") + walked = append(walked, restrictedLogIDs(page)...) + if page.Cursor == "" { + break + } + params.Cursor = page.Cursor + } + assert.Equal(t, publicIDs, walked) + }) + + t.Run("a post admitted later shows normally", func(t *testing.T) { + _, err := db.ExecContext(t.Context(), ` + UPDATE community_post_admissions SET status = 'accepted', accepted_cid = $3, updated_at = NOW() + WHERE community_did = $1 AND post_uri = $2 + `, communityDID, pending.URI, pending.CID) + require.NoError(t, err) + page, _ := restrictedLogPage(t, service, moderation.ListActionsParams{Subject: pending.URI}) + require.Len(t, page.Actions, 1) + assert.Equal(t, &moderation.SubjectRefView{URI: pending.URI, CID: pending.CID}, page.Actions[0].Subject) + page, _ = restrictedLogPage(t, service, moderation.ListActionsParams{Community: communityDID}) + assert.Len(t, page.Actions, len(publicIDs)+1) + }) +} diff --git a/internal/db/postgres/moderation_concurrency_integration_test.go b/internal/db/postgres/moderation_concurrency_integration_test.go index 3063fb2..bc058e9 100644 --- a/internal/db/postgres/moderation_concurrency_integration_test.go +++ b/internal/db/postgres/moderation_concurrency_integration_test.go @@ -191,6 +191,10 @@ func (g *gatedMediaReconciler) ReconcileTx(ctx context.Context, tx *sql.Tx, subj return blocks, err } +func (g *gatedMediaReconciler) ReconcileIncomingTx(ctx context.Context, tx *sql.Tx, subjectURI, ownerDID string, blobCIDs []string) ([]moderation.MediaBlock, error) { + return g.inner.ReconcileIncomingTx(ctx, tx, subjectURI, ownerDID, blobCIDs) +} + func (g *gatedMediaReconciler) Purge(ctx context.Context, blocks []moderation.MediaBlock) { g.inner.Purge(ctx, blocks) } diff --git a/internal/db/postgres/moderation_label_precedence_integration_test.go b/internal/db/postgres/moderation_label_precedence_integration_test.go index 8f0dd7f..7d5f6e1 100644 --- a/internal/db/postgres/moderation_label_precedence_integration_test.go +++ b/internal/db/postgres/moderation_label_precedence_integration_test.go @@ -38,6 +38,7 @@ func requireLabelPrecedenceTombstone(t *testing.T, result *posts.PostResult, sub assert.Equal(t, subject.URI, result.Moderated.URI) require.NotNil(t, result.Moderated.Moderation) assert.Equal(t, moderation.ModerationStateRemoved, result.Moderated.Moderation.State) + assert.Len(t, result.Moderated.Moderation.Sources, 1, "only the removal is a removal source, never the label") assert.Empty(t, result.Moderated.Moderation.ContentLabels) encoded := labelPrecedenceJSON(t, result) assert.Contains(t, string(encoded), `"$type":"social.coves.community.post.defs#moderatedPost"`) diff --git a/internal/db/postgres/moderation_media_purges_owner_integration_test.go b/internal/db/postgres/moderation_media_purges_owner_integration_test.go new file mode 100644 index 0000000..52aaa4a --- /dev/null +++ b/internal/db/postgres/moderation_media_purges_owner_integration_test.go @@ -0,0 +1,20 @@ +//go:build integration + +package postgres_test + +import ( + "testing" + + "Coves/tests/testkit" + + "github.com/stretchr/testify/require" +) + +// An empty owner DID means every owner on a media block, but a CDN purge +// target always names one owner's URLs, so the table refuses an empty owner. +func TestModerationMediaPurgesRejectEmptyOwnerDID(t *testing.T) { + db := testkit.DB(t) + _, err := db.ExecContext(t.Context(), + `INSERT INTO moderation_media_purges (owner_did, blob_cid) VALUES ('', $1)`, moderationImageCIDOne) + require.ErrorContains(t, err, "moderation_media_purges_owner_did_check") +} diff --git a/internal/db/postgres/moderation_post_tombstone_integration_test.go b/internal/db/postgres/moderation_post_tombstone_integration_test.go index 0387efd..cf3b216 100644 --- a/internal/db/postgres/moderation_post_tombstone_integration_test.go +++ b/internal/db/postgres/moderation_post_tombstone_integration_test.go @@ -58,8 +58,10 @@ func TestModerationPostActiveRemovalsByURIs(t *testing.T) { active := indexedModerationPost(t, db, moderation.PostV2Collection, communityDID, authorDID, "active decision", "") restored := indexedModerationPost(t, db, moderation.PostV2Collection, communityDID, authorDID, "restored decision", "") control := indexedModerationPost(t, db, moderation.PostV2Collection, communityDID, authorDID, "never removed", "") + labelled := indexedModerationPost(t, db, moderation.PostV2Collection, communityDID, authorDID, "labelled only", "") service := newPostgresModerationService(db) removeIndexedModerationPost(t, service, active) + labelPost(t, service, fixtures.DID("postlabeladmin"), labelled, "v0", "label-only-post") previous, err := service.RemoveContent(t.Context(), fixtures.DID("postremovaladminb"), moderation.RemoveContentRequest{ Subject: restored, ExpectedVersion: "v0", IdempotencyKey: "remove-restored-post", Reason: "social.coves.moderation.defs#reasonSpam", @@ -69,8 +71,9 @@ func TestModerationPostActiveRemovalsByURIs(t *testing.T) { require.NotNil(t, previous.Action) restoreModerationPost(t, service, restored, previous) - got, err := repo.ActiveRemovalsByURIs(t.Context(), []string{control.URI, active.URI, restored.URI, active.URI}) + got, err := repo.ActiveRemovalsByURIs(t.Context(), []string{control.URI, active.URI, restored.URI, labelled.URI, active.URI}) require.NoError(t, err) + assert.NotContains(t, got, labelled.URI, "an active label is not a removal") assert.Equal(t, map[string][]posts.RemovalSource{ active.URI: {{AuthorityDID: fixtures.InstanceDID(), ScopeKind: "instance"}}, }, got, "only active decisions may appear, even in a batch with restored and unremoved URIs") @@ -165,6 +168,27 @@ func TestModerationPostGetTombstone(t *testing.T) { } } +// A label is a decision row too, but never a removal: a community-removed post +// carrying only an NSFW label stays the community's #removedPost. +func TestModerationPostGetCommunityRemovedLabelledPost(t *testing.T) { + db := testkit.DB(t) + communityDID, authorDID := moderationPostTombstoneActors(t, db) + subject := indexedModerationPost(t, db, moderation.PostV2Collection, communityDID, authorDID, "community removed, labelled", "") + _, err := db.ExecContext(t.Context(), ` + UPDATE community_post_admissions SET status = 'removed', accepted_cid = NULL, + decision_code = 'rule-violation', decision_at = NOW(), updated_at = NOW() + WHERE community_did = $1 AND post_uri = $2 + `, communityDID, subject.URI) + require.NoError(t, err) + labelPost(t, newPostgresModerationService(db), fixtures.DID("tombstonelabeladmin"), subject, "v0", "community-removed-label") + results, err := moderationPostGetService(db, postgres.NewPostRepository(db)).GetPosts(t.Context(), posts.GetPostsRequest{URIs: []string{subject.URI}}) + require.NoError(t, err) + require.Len(t, results, 1) + require.NotNil(t, results[0].Removed, "with no instance removal, the community's #removedPost must stand") + assert.Equal(t, "rule-violation", results[0].Removed.Code) + assert.Nil(t, results[0].Moderated) +} + func TestModerationPostGetBatchPreservesOrder(t *testing.T) { db := testkit.DB(t) communityDID, authorDID := moderationPostTombstoneActors(t, db) diff --git a/internal/db/postgres/moderation_repo.go b/internal/db/postgres/moderation_repo.go index 576b6e6..8f6511f 100644 --- a/internal/db/postgres/moderation_repo.go +++ b/internal/db/postgres/moderation_repo.go @@ -673,15 +673,37 @@ func (r *ModerationRepository) BindTransaction(tx *sql.Tx) moderation.MediaTrans } // ListActions reads a page of the action log. +// +// Each row's SubjectAccess is decided now, not when the action was recorded, by +// the anonymous viewer's admission rule post.get applies before it discloses a +// removed post (admittedPostsPredicate): a post admitted after its removal is +// public from then on. A comment follows its root post. A subject with no +// indexed post to decide from is restricted. ExcludeRestricted filters in the +// WHERE clause, before LIMIT, so a filtered page is never short. func (r *ModerationRepository) ListActions(ctx context.Context, query moderation.ActionListQuery) ([]moderation.Action, error) { var beforeCreatedAt, beforeID any if query.Before != nil { beforeCreatedAt, beforeID = query.Before.CreatedAt, query.Before.ID } + // The admission join aliases community_post_admissions as a, shadowing the + // action alias inside the EXISTS, so the root URI is resolved one level out. + admissionJoin, admitted := admittedPostsPredicate(anonymousViewerSQL) rows, err := r.db.QueryContext(ctx, ` - SELECT `+moderationActionColumns+`, reversed.reason + SELECT `+moderationActionColumns+`, reversed.reason, subject_access.access FROM moderation_actions a LEFT JOIN moderation_actions reversed ON reversed.id = a.reverses_action_id + LEFT JOIN comments subject_comment + ON a.subject_collection = $18 AND subject_comment.uri = a.subject_uri + CROSS JOIN LATERAL ( + SELECT CASE WHEN a.subject_collection = $18 THEN subject_comment.root_uri + ELSE a.subject_uri END AS post_uri + ) subject_root + CROSS JOIN LATERAL ( + SELECT CASE WHEN EXISTS ( + SELECT 1 FROM posts p`+admissionJoin+` + WHERE p.uri = subject_root.post_uri AND `+admitted+` + ) THEN $20::text ELSE $21::text END AS access + ) subject_access WHERE ($2::timestamptz IS NULL OR (a.created_at, a.id) < ($2::timestamptz, $3::text)) AND (NULLIF($4::text, '') IS NULL OR a.subject_uri = $4) AND (NULLIF($5::text, '') IS NULL OR a.subject_collection = $5) @@ -697,12 +719,15 @@ func (r *ModerationRepository) ListActions(ctx context.Context, query moderation COALESCE(a.reason = ANY($15::text[]), FALSE) OR COALESCE(reversed.reason = ANY($15::text[]), FALSE))) AND (NOT $16::boolean OR a.action <> ALL($17::text[])) + AND (NOT $19::boolean OR subject_access.access = $20::text) ORDER BY a.created_at DESC, a.id DESC LIMIT $1 `, query.Limit, beforeCreatedAt, beforeID, query.SubjectURI, query.SubjectCollection, query.Action, query.Origin, query.AuthorityDID, query.ActorDID, query.CommunityDID, query.ActionID, query.Since, query.Until, query.ExcludeHidden, pq.Array(moderation.HiddenActionReasons()), - query.ExcludeLabelActions, pq.Array(moderation.PublicExcludedActions())) + query.ExcludeLabelActions, pq.Array(moderation.PublicExcludedActions()), + moderation.CommentCollection, query.ExcludeRestricted, + string(moderation.SubjectAccessPublic), string(moderation.SubjectAccessRestricted)) if err != nil { return nil, fmt.Errorf("list moderation actions: %w", err) } @@ -710,13 +735,15 @@ func (r *ModerationRepository) ListActions(ctx context.Context, query moderation var actions []moderation.Action for rows.Next() { var reversedReason sql.NullString - action, err := scanModerationAction(rows, nil, &reversedReason) + var access string + action, err := scanModerationAction(trailingColumns{row: rows, extra: []any{&access}}, nil, &reversedReason) if err != nil { return nil, fmt.Errorf("scan moderation action: %w", err) } if action == nil { return nil, errors.New("scan moderation action: row has no action id") } + action.SubjectAccess = moderation.SubjectAccess(access) actions = append(actions, *action) } if err := rows.Err(); err != nil { @@ -725,6 +752,16 @@ func (r *ModerationRepository) ListActions(ctx context.Context, query moderation return actions, nil } +// trailingColumns scans extra columns after the ones its caller asks for. +type trailingColumns struct { + row moderationRow + extra []any +} + +func (columns trailingColumns) Scan(dest ...any) error { + return columns.row.Scan(append(dest, columns.extra...)...) +} + // RecordCDNPurgeTargets commits targets independently of any moderation action // transaction. Unlike the removal upsert it leaves a pending target untouched, // so repeated recordings neither reset its backoff nor void its claim; a diff --git a/tests/e2e/moderation_contract_test.go b/tests/e2e/moderation_contract_test.go index 10fe2dc..3f2ba8d 100644 --- a/tests/e2e/moderation_contract_test.go +++ b/tests/e2e/moderation_contract_test.go @@ -899,12 +899,12 @@ func TestModerationModlogWiringContract(t *testing.T) { } `json:"state"` } var created []string - var restored testkit.Record + var restored strongRef var restoredRemoval mutation for index := range 2 { - post := author.PutRecord(t, postV2Collection, testkit.TID(), - postV2Record(community.DID, "modlog wiring "+testkit.UniqueID(t), "a post for the action log")) - awaitStatus(t, p, post.URI, community.DID, "pending", "the post to reach the AppView index") + // Accepted, because the public log never ties a post the public cannot + // see to its community. + post := indexedPost(t, p, community, author, "modlog wiring "+testkit.UniqueID(t)) var removal mutation err := p.AppView.As(admin.ServiceAuth(t, communityInstanceDID, removeContentMethod)).Procedure( t.Context(), removeContentMethod, map[string]any{