diff --git a/docs/PRD_AUTHOR_OWNED_POSTS.md b/docs/PRD_AUTHOR_OWNED_POSTS.md index c4cdf80..0a349a9 100644 --- a/docs/PRD_AUTHOR_OWNED_POSTS.md +++ b/docs/PRD_AUTHOR_OWNED_POSTS.md @@ -28,7 +28,14 @@ tuple CAS over a removal. Stale/terminal skips are outcome values, not errors (033 precedent — sentinels would dead-letter healthy skips). Rev 2.4 (2026-08-08): rejection narrowed to pending-only CAS with judged CID; op-rank derived repo-side; NULL-evaluated acceptance treated as pin-trusting -(task-2 second-opinion catches).** +(task-2 second-opinion catches). +Rev 2.5 (2026-08-08): §4.1 corrected by task-3 plan review — ban source is +community_memberships.is_banned behind a BanLookup interface (no +moderation.ban ingestion exists; no production ban writer yet); rate +limits/dedupe get a synchronous post_submissions ledger (migration 035) — +the posts table is unusable as a limiter substrate (ingestion lag, +author-supplied created_at, delete-to-evade); per-origin-PDS quota +explicitly deferred to Beta.** **Supersedes** the write-path architecture in `docs/federation-prd.md`: that document solves cross-instance posting by service-auth-forwarding the write to @@ -271,9 +278,34 @@ authorization + rate limits — nothing else**. The docstring's Therefore `admitPost` (§5.6) is **extraction plus new policy**, not a behavior-preserving refactor. New checks arriving with it, each with an -explicit error code and tests: ban lookup against indexed -`social.coves.moderation.ban` state, and per-author/per-community submission -rate limits (§8). The spec stops claiming otherwise. +explicit error code and tests: ban enforcement, per-author/per-community +submission rate limits, and duplicate-submission dedupe (§8). + +**Ban source, honestly (task-3 plan-review correction):** the only ban state +in the system is `community_memberships.is_banned` — and no production code +path writes it today (the memberships repo has no non-test callers; no +`social.coves.moderation.ban` consumer exists and the collection is not in +`consumerWantedCollections`). `admitPost` therefore enforces bans through a +`BanLookup` interface backed by that column, making enforcement live the +moment a ban writer ships (moderation write path and/or ban-record +ingestion — future scope, not this loop). Non-membership reads as +not-banned; any lookup FAILURE fails the request closed — failing open on a +ban would turn a database blip into a global unban. + +**Rate-limit substrate:** limits and dedupe are backed by a synchronous +`post_submissions` ledger (migration 035, mirroring `aggregator_posts`) with +a canonical-record fingerprint and a UNIQUE-insert dedupe gate, +reserve-then-confirm around the PDS write. The `posts` table cannot back +them: it is firehose-fed (ingestion lag hides the very burst being limited), +its `created_at` is author-supplied (attacker-controlled windows once writes +flip to author repos), and its indexes exclude soft-deleted rows +(delete-to-evade). Refused submissions consume no quota. Dedupe precedes the +rate limit (a client retry storm must not burn quota) and applies to every +actor class; trusted aggregators keep their historical no-limit status and +registered aggregators are governed by their existing limiter only. +§8's per-origin-PDS quota is **deferred** to the Beta remote path (it +requires PDS resolution, §7) — recorded here so §8 does not silently become +fiction. ### 4.2 Flow diff --git a/internal/api/handlers/post/errors_test.go b/internal/api/handlers/post/errors_test.go index 2209ed3..9807f83 100644 --- a/internal/api/handlers/post/errors_test.go +++ b/internal/api/handlers/post/errors_test.go @@ -157,6 +157,26 @@ func TestAggregatorErrorCodes(t *testing.T) { } } +// A submission refused as a repeat is a 409, and it must not be confused with +// anything else. +// +// Two client behaviours depend on the distinction. A 409 says "your post +// already exists, stop retrying and go look for it", which is exactly what a +// client whose response was lost needs to hear; a 429 says "wait", and a client +// told to wait would resend the same content on a timer forever. And the code +// must be its own — folding it into the generic AlreadyExists that +// coreerrors.ConflictError produces would leave a client unable to tell a +// refused submission from a record the indexer already holds. +func TestDuplicateSubmissionIsItsOwnConflict(t *testing.T) { + rec := httptest.NewRecorder() + handleServiceError(rec, fmt.Errorf("createPost: %w", posts.ErrDuplicateSubmission)) + + body := assertXRPCError(t, rec, http.StatusConflict, "DuplicateSubmission") + if strings.Contains(body.Message, "createPost") { + t.Errorf("wrapper context leaked into the client message: %q", body.Message) + } +} + // posts.ErrCommunityNotFound must keep beating the generic not-found rule that // also matches it. func TestCommunityNotFoundBeatsGenericNotFound(t *testing.T) { diff --git a/internal/config/config.go b/internal/config/config.go index 4ea98cc..a816ee1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -106,6 +106,9 @@ type Config struct { Signup SignupConfig Media MediaConfig + // Submissions bounds what one author may post into one community. + Submissions SubmissionsConfig + // CursorSecret is the HMAC key that signs pagination cursors, preventing // clients from forging or tampering with them. CursorSecret string @@ -326,6 +329,34 @@ type MediaConfig struct { AllowUnproxiedMedia bool } +// SubmissionsConfig bounds what one author may submit to one community +// (docs/PRD_AUTHOR_OWNED_POSTS.md §8). +// +// It mirrors posts.SubmissionLimits field for field rather than embedding it. +// The duplication is deliberate: this package is imported by everything that +// starts a process, and giving it a dependency on a core domain package would +// make the domain's import graph the startup path's problem. The mapping is one +// struct literal at wiring time. +// +// EVERY FIELD IS REQUIRED. There is no "unset means unlimited" reading, which +// is the whole reason these are validated at startup: a quota that evaporates +// when someone forgets an environment variable is indistinguishable, in +// production, from having no quota at all — and it fails open, silently, on the +// one path that exists to bound abuse. +type SubmissionsConfig struct { + // MaxPerAuthorPerCommunity is how many posts one author may have admitted + // to one community inside Window. + MaxPerAuthorPerCommunity int + + // Window is the rolling window the quota is counted over. + Window time.Duration + + // DedupeWindow scopes how long an identical resubmission is refused as a + // repeat. It is separate from Window because the two answer different + // questions: one bounds volume, the other catches retries. + DedupeWindow time.Duration +} + // TokenEndpointEnabled reports whether the signup-token endpoint can operate. // It needs both the captcha secret and (from PDSConfig) an admin password to // mint invite codes, so the caller passes the latter in. diff --git a/internal/config/submissions_test.go b/internal/config/submissions_test.go new file mode 100644 index 0000000..39b62db --- /dev/null +++ b/internal/config/submissions_test.go @@ -0,0 +1,115 @@ +package config + +import ( + "strings" + "testing" + "time" +) + +// The per-author submission quota of docs/PRD_AUTHOR_OWNED_POSTS.md §8 is +// configuration, and configuration that goes missing must stop the process. +// +// The failure this guards against is specific: an operator who never sets +// POST_SUBMISSIONS_MAX_PER_COMMUNITY gets a zero, a limit check written as +// `count >= limit` then refuses everything (or, written the other way, admits +// everything), and either way the behaviour is decided by an omission rather +// than by a decision. §8's quotas exist to absorb the fact that anyone can +// write unlimited records naming any community — so "unset" cannot be allowed +// to mean "unlimited", and validating at startup is the only place the answer +// is cheap. + +func TestLoad_SubmissionQuotaHasWorkingDefaults(t *testing.T) { + clearEnv(t) + t.Setenv("IS_DEV_ENV", "true") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() returned error: %v", err) + } + + if cfg.Submissions.MaxPerAuthorPerCommunity <= 0 { + t.Errorf("Submissions.MaxPerAuthorPerCommunity = %d, want a positive default; "+ + "a zero here is a quota decided by omission", + cfg.Submissions.MaxPerAuthorPerCommunity) + } + if cfg.Submissions.Window <= 0 { + t.Errorf("Submissions.Window = %s, want a positive rolling window", cfg.Submissions.Window) + } + if cfg.Submissions.DedupeWindow <= 0 { + t.Errorf("Submissions.DedupeWindow = %s, want a positive dedupe window", cfg.Submissions.DedupeWindow) + } +} + +func TestLoad_SubmissionQuotaIsReadFromTheEnvironment(t *testing.T) { + clearEnv(t) + t.Setenv("IS_DEV_ENV", "true") + t.Setenv("POST_SUBMISSIONS_MAX_PER_COMMUNITY", "7") + t.Setenv("POST_SUBMISSIONS_WINDOW", "30m") + t.Setenv("POST_SUBMISSIONS_DEDUPE_WINDOW", "10m") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() returned error: %v", err) + } + + if cfg.Submissions.MaxPerAuthorPerCommunity != 7 { + t.Errorf("MaxPerAuthorPerCommunity = %d, want 7", cfg.Submissions.MaxPerAuthorPerCommunity) + } + if cfg.Submissions.Window != 30*time.Minute { + t.Errorf("Window = %s, want 30m", cfg.Submissions.Window) + } + if cfg.Submissions.DedupeWindow != 10*time.Minute { + t.Errorf("DedupeWindow = %s, want 10m", cfg.Submissions.DedupeWindow) + } +} + +// A config assembled with the quota left at its zero value must not validate. +// This is the assertion that makes "unset means unlimited" unrepresentable +// rather than merely discouraged. +func TestValidate_RejectsAnUnsetSubmissionQuota(t *testing.T) { + base := func() *Config { + return &Config{ + IsDevEnv: true, + Database: DatabaseConfig{URL: "postgres://u:p@db/coves", MaxOpenConns: 25, MaxIdleConns: 25}, + Server: ServerConfig{Port: "8080", ReadHeaderTimeout: time.Second, ReadTimeout: 30 * time.Second, WriteTimeout: 30 * time.Second, IdleTimeout: 60 * time.Second, ShutdownTimeout: 15 * time.Second}, + Instance: InstanceConfig{DID: "did:web:coves.social", Domain: "coves.social"}, + CursorSecret: devCursorSecret, + Jetstream: JetstreamConfig{FeedsSpec: "self=ws://localhost:6008"}, // coves:allow-host-literal: a non-empty spec so Validate's unrelated JETSTREAM_FEEDS rule is satisfied; parsing lives elsewhere and nothing here dials it + Submissions: SubmissionsConfig{ + MaxPerAuthorPerCommunity: 10, + Window: time.Hour, + DedupeWindow: time.Hour, + }, + } + } + + // The control: the fully-specified config validates, so a failure below is + // about the field that was cleared and not about the fixture. + if err := base().Validate(); err != nil { + t.Fatalf("the fully-specified config must validate; got: %v", err) + } + + for _, tc := range []struct { + name string + clear func(*Config) + want string + }{ + {"no per-community limit", func(c *Config) { c.Submissions.MaxPerAuthorPerCommunity = 0 }, "POST_SUBMISSIONS_MAX_PER_COMMUNITY"}, + {"no window", func(c *Config) { c.Submissions.Window = 0 }, "POST_SUBMISSIONS_WINDOW"}, + {"no dedupe window", func(c *Config) { c.Submissions.DedupeWindow = 0 }, "POST_SUBMISSIONS_DEDUPE_WINDOW"}, + {"a negative limit", func(c *Config) { c.Submissions.MaxPerAuthorPerCommunity = -1 }, "POST_SUBMISSIONS_MAX_PER_COMMUNITY"}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := base() + tc.clear(cfg) + + err := cfg.Validate() + if err == nil { + t.Fatal("Validate() accepted a submission quota that is not a quota; the process would start with abuse limits silently disabled") + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error should name %s so an operator can fix it; got:\n%s", tc.want, err.Error()) + } + }) + } +} diff --git a/internal/config/testing.go b/internal/config/testing.go index fe91146..c74f268 100644 --- a/internal/config/testing.go +++ b/internal/config/testing.go @@ -33,6 +33,8 @@ var loadedEnvVars = []string{ "IMAGE_PROXY_CLEANUP_INTERVAL_MINUTES", "IMAGE_PROXY_FETCH_TIMEOUT_SECONDS", "IMAGE_PROXY_MAX_SOURCE_SIZE_MB", "ALLOW_UNPROXIED_MEDIA", + "POST_SUBMISSIONS_MAX_PER_COMMUNITY", "POST_SUBMISSIONS_WINDOW", + "POST_SUBMISSIONS_DEDUPE_WINDOW", } // ClearEnvForTest blanks every environment variable Load reads, restoring them diff --git a/internal/core/posts/admissions.go b/internal/core/posts/admissions.go index 93c84da..660563f 100644 --- a/internal/core/posts/admissions.go +++ b/internal/core/posts/admissions.go @@ -341,3 +341,58 @@ type AdmissionRepository interface { // make a moderator re-review what they had already cleared. ListByStatusForCommunity(ctx context.Context, communityDID string, status AdmissionStatus, limit int, cursor *string) ([]*Admission, *string, error) } + +// DecisionCode is the reason a post was refused or removed — the value stored +// in community_post_admissions.decision_code and, for the subset that a +// community publishes, in a social.coves.community.removal record's `code`. +// +// It is a plain named string with NO validation function, deliberately. The +// removal lexicon spells `code` as `knownValues`, which is an OPEN set by +// definition — "values are not limited to this set" (§3.3) — so that new codes +// ship without a lexicon break. A Go-side IsValid would re-close what the +// lexicon deliberately left open, and would start rejecting perfectly legal +// codes minted by a remote community running a newer build than ours. +type DecisionCode string + +// The six codes the removal lexicon names (§3.3). These are the vocabulary a +// COMMUNITY publishes: they appear in removal records, so a client reading the +// firehose meets them. Kebab-case is the lexicon style guide's convention for +// fixed strings. +const ( + DecisionRuleViolation DecisionCode = "rule-violation" + DecisionSpam DecisionCode = "spam" + DecisionOffTopic DecisionCode = "off-topic" + DecisionIllegalContent DecisionCode = "illegal-content" + DecisionAuthorBanned DecisionCode = "author-banned" + DecisionModeratorDiscretion DecisionCode = "moderator-discretion" +) + +// The admission-time codes. These never reach a community repo: §3.3 is +// explicit that a submission refused before it was ever accepted writes NO +// record, because spam must not bloat the community's repository. They live in +// the same vocabulary anyway — the admissions table's decision_code column +// stores both kinds, and getStatus serves both to the author who asked why. +const ( + // DecisionRateLimitExceeded: the author is over their per-community + // submission quota (§8). + DecisionRateLimitExceeded DecisionCode = "rate-limit-exceeded" + + // DecisionDuplicateSubmission: an identical submission from this author to + // this community is already on the ledger for the current window. + DecisionDuplicateSubmission DecisionCode = "duplicate-submission" + + // DecisionCommunityNotFound: the at-identifier names no community this + // AppView has indexed. + DecisionCommunityNotFound DecisionCode = "community-not-found" + + // DecisionCommunityPrivate: a private community refusing a regular user. + // + // It is deliberately the answer for a BANNED user of a private community + // too — see admitPost's check order, which explains why a ban must not be + // disclosed through a privacy wall. + DecisionCommunityPrivate DecisionCode = "community-private" + + // DecisionAggregatorNotAuthorized: a registered aggregator the community + // has not authorized (or whose authorization it has revoked). + DecisionAggregatorNotAuthorized DecisionCode = "aggregator-not-authorized" +) diff --git a/internal/core/posts/admit.go b/internal/core/posts/admit.go new file mode 100644 index 0000000..23a0b41 --- /dev/null +++ b/internal/core/posts/admit.go @@ -0,0 +1,329 @@ +package posts + +import ( + "context" + "time" + + "Coves/internal/core/communities" +) + +// admitPost: the single decision point for "may this submission become a post +// in this community?" (PRD_AUTHOR_OWNED_POSTS.md §4.1, §5.6, §8). +// +// It is extraction PLUS new policy, and §4.1 is blunt about which is which. +// What CreatePost enforces today is community existence, a private-visibility +// block for regular users, and aggregator authorization + the aggregator's own +// hourly quota — nothing else. The service docstring's "membership/ban +// validation" was aspirational: there is no ban lookup anywhere on the write +// path, and no per-author rate limiting at all. Both arrive here. +// +// WHY A SEPARATE FUNCTION RATHER THAN MORE STEPS IN CreatePost. The same +// decision has to be made from three places: the synchronous local-community +// fast path (§4.2 step 4), the firehose consumer when a post for a community we +// host arrives from someone else's PDS (§5.6), and the notify endpoint (§7). +// A decision that lived inside CreatePost would be reachable only from the +// first, so the other two would silently admit what the first refuses. + +// ActorClass is what the CALLER has already established the submitter to be. +// +// It is an INPUT rather than something this decision derives, and that is the +// whole point. Today's classification reads TRUSTED_AGGREGATOR_DIDS (falling +// back to KAGI_AGGREGATOR_DID) out of the process environment inside CreatePost +// (service.go step 3). A decision function that reached for os.Getenv itself +// could not have its trusted-actor branch tested alongside t.Parallel — Go's +// own testing package refuses t.Setenv there — and, worse, would hide "who is +// trusted" from the call site of the security decision it governs. +type ActorClass string + +const ( + // ActorUser is a person posting on their own behalf. Every check applies. + ActorUser ActorClass = "user" + + // ActorRegisteredAggregator is a service the AppView has indexed a + // social.coves.aggregator.service declaration for. It is held to the + // community's authorization record and to its OWN hourly quota + // (aggregators.ValidateAggregatorPost), not to membership or visibility. + ActorRegisteredAggregator ActorClass = "registered_aggregator" + + // ActorTrustedAggregator is a service named in TRUSTED_AGGREGATOR_DIDS — + // the temporary env-var mechanism that predates a real authorization + // endpoint. It skips visibility, ban and authorization checks entirely, + // which is the existing behaviour and is preserved deliberately. + ActorTrustedAggregator ActorClass = "trusted_aggregator" +) + +// AdmissionRequest is one submission, described in the terms the decision needs +// and no others. There is no record and no blob here: admitPost runs BEFORE any +// of that work, so that a refusal costs a lookup rather than an upload. +type AdmissionRequest struct { + // Actor is the class the caller resolved. See ActorClass. + Actor ActorClass + + // AuthorDID is the authenticated author. CreatePost has already proven it + // matches the DID on the request; this decision trusts that. + AuthorDID string + + // Community is the at-identifier as the client sent it — a handle + // (!gardening.communities.coves.social) or a DID. Resolving it is the + // decision's first step, because a community that does not resolve is the + // first thing that can refuse a submission. + Community string + + // Fingerprint identifies WHAT is being submitted: the hash of the canonical + // record with createdAt removed (see submissionFingerprint). It is the + // dedupe key, and it must exclude the timestamp or every resubmission of + // identical content would look new. + Fingerprint string +} + +// AdmissionDecision is the answer: admitted, or refused with a code. +// +// A refusal is a VALUE rather than an error, matching AdmissionOutcome above +// and the project's standing preference for error codes over booleans. The +// caller has to translate the code into whatever its transport speaks — a +// sentinel error for CreatePost, an admissions row for the firehose engine — +// and a refusal returned as an error would push the second of those into the +// dead-letter queue, which is meant to hold genuine failures. +type AdmissionDecision struct { + // Code is the reason for a refusal, and empty for an admission. + Code DecisionCode + + // Community is the resolved community, populated on admission so that + // CreatePost does not fetch it a second time. It is the one piece of state + // the decision has already paid for that its caller would otherwise re-buy. + Community *communities.Community + + // Reservation is the ledger row that was inserted for this submission. It + // is present on admission and must be released if the PDS write that + // follows fails — see SubmissionLedger. + Reservation *SubmissionReservation + + // Cause carries the underlying error behind a refusal, when there is one, + // so the caller can wrap it and keep it matchable. + // + // It exists for exactly one case today: aggregator authorization. The API + // boundary maps that refusal through aggregators.IsUnauthorized and + // aggregators.IsRateLimited (internal/api/handlers/post/errors.go), which + // are predicates over the AGGREGATORS package's sentinels. Collapsing that + // error into a bare DecisionCode would turn a 403 "stop asking" and a 429 + // "ask later" into the same answer, and a well-behaved aggregator would + // retry a permanent refusal forever. + Cause error +} + +// Admitted reports whether the submission may proceed. There is no separate +// bool field: two representations of one fact drift, and the code is the one +// that has to be right. +func (d AdmissionDecision) Admitted() bool { return d.Code == "" } + +// SubmissionReservation identifies the ledger row admitPost inserted for a +// submission, so a caller whose subsequent PDS write failed can release it. +type SubmissionReservation struct { + ID int64 +} + +// SubmissionLimits bounds what one author may submit to one community (§8). +// +// Every field is required. There is deliberately no "zero means unlimited" +// reading: a quota that silently disappears when an environment variable is +// missing is not a quota, so config.Validate refuses to start the process with +// any of these unset. +type SubmissionLimits struct { + // MaxPerAuthorPerCommunity is how many submissions one author may have + // admitted to one community inside Window. + MaxPerAuthorPerCommunity int + + // Window is the rolling window the quota is counted over, matching the + // aggregator limiter's semantics (aggregators.RateLimitWindow): a COUNT of + // ledger rows newer than now-Window, not a fixed bucket that empties on the + // hour and lets an author spend twice across the boundary. + Window time.Duration + + // DedupeWindow is the width of the bucket that scopes dedupe uniqueness. + // Without it the ledger's unique constraint would forbid an author from + // ever reposting identical content again, which is a different and much + // stronger policy than "do not accept the same thing twice right now". + DedupeWindow time.Duration +} + +// Clock is the decision's only source of time. +// +// Injected rather than called directly so that window expiry is testable +// without waiting for one: docs/TEST_ARCHITECTURE.md §3.3 records that +// time.Sleep in a test fails the audit, and that a rate limiter's window is +// crossed through an injected clock. +type Clock func() time.Time + +// CommunityLookup resolves an at-identifier and fetches the community behind +// it. Satisfied by communities.Service. +type CommunityLookup interface { + // ResolveCommunityIdentifier turns a handle or a DID into a DID. + ResolveCommunityIdentifier(ctx context.Context, identifier string) (string, error) + + // GetByDID returns the indexed community. + GetByDID(ctx context.Context, did string) (*communities.Community, error) +} + +// BanLookup answers whether an author is banned from a community, by returning +// the membership row that carries the answer. +// +// It returns the whole membership rather than a bool so that the translation of +// "no membership row" into "not banned" happens in ONE place — inside +// admitPost, next to the comment that explains why an error is not the same +// thing. Satisfied by communities.Service. +type BanLookup interface { + // GetMembership returns the author's membership of the community, or an + // error wrapping communities.ErrMembershipNotFound when there is none. + GetMembership(ctx context.Context, userDID, communityIdentifier string) (*communities.Membership, error) +} + +// AggregatorAuthorizer checks a registered aggregator's authorization and its +// own quota. Satisfied by aggregators.Service. +type AggregatorAuthorizer interface { + ValidateAggregatorPost(ctx context.Context, aggregatorDID, communityDID string) error +} + +// ReserveSubmissionCommand is one row of the post_submissions ledger. +type ReserveSubmissionCommand struct { + AuthorDID string + CommunityDID string + + // Fingerprint is the content hash — see AdmissionRequest.Fingerprint. + Fingerprint string + + // DedupeBucket is the index of the DedupeWindow this submission falls in, + // derived from the injected clock. It is part of the unique key, which is + // what makes dedupe expire. + DedupeBucket int64 +} + +// SubmissionLedger records admitted submissions, and IS the dedupe gate. +// +// RESERVE-THEN-CONFIRM. The row goes in BEFORE the PDS write and is released if +// that write fails. The alternative — record after a successful write — leaves +// a window in which two concurrent identical submissions both pass the check +// and both get written, which is precisely the double-tap this exists to stop. +// A leaked reservation (process died between the insert and the release) costs +// the author one quota slot until the window rolls; a missed one costs the +// community a duplicate post. The asymmetry decides the direction. +// +// THE INSERT IS THE CHECK. Dedupe is not a SELECT followed by an INSERT: it is +// the INSERT, with the unique constraint as the arbiter. A read-then-write +// would reopen the same race under concurrency, and the database is the only +// participant that can serialize it. +type SubmissionLedger interface { + // Reserve inserts the ledger row for a submission. A unique-constraint + // violation means an identical submission is already recorded for this + // window, and is reported as ErrDuplicateSubmission rather than as a driver + // error — the caller has to tell "someone already posted this" apart from + // "the database is unwell". + Reserve(ctx context.Context, cmd ReserveSubmissionCommand) (SubmissionReservation, error) + + // Release removes a reservation whose submission never became a post. It is + // idempotent: releasing a row that is already gone is not an error, because + // the caller reaches this path while already handling a failure and must + // not be handed a second one. + Release(ctx context.Context, reservation SubmissionReservation) error + + // CountSince counts one author's submissions to one community at or after + // `since` — the rolling-window quota query. + CountSince(ctx context.Context, authorDID, communityDID string, since time.Time) (int, error) +} + +// AdmissionPolicy is the collaborator set the new §8 policy needs, over and +// above what postService already holds. +type AdmissionPolicy struct { + Ledger SubmissionLedger + Bans BanLookup + Limits SubmissionLimits + Now Clock +} + +// WithAdmissionPolicy enables the ban check, dedupe and per-author rate limit +// on CreatePost. +func WithAdmissionPolicy(policy AdmissionPolicy) PostServiceOption { + return func(s *postService) { s.admission = &policy } +} + +// admissionDeps is everything admitPost reads, gathered so the decision is a +// function of its arguments rather than of a service's field set. +type admissionDeps struct { + communities CommunityLookup + bans BanLookup + aggregators AggregatorAuthorizer + ledger SubmissionLedger + limits SubmissionLimits + now Clock +} + +// admitPost decides whether one submission may become a post. +// +// CHECK ORDER — each step its own refusal, and the order is load-bearing: +// +// 1. Community resolution. Nothing else can be evaluated against a community +// that does not exist. +// +// 2. Private visibility, for regular users. A banned member of a PRIVATE +// community is refused with DecisionCommunityPrivate, NOT with +// DecisionAuthorBanned — the ban lookup is not even consulted. A private +// community's moderation state is behind the same wall as its content, and +// answering "you are banned" would confirm to an outsider both that the +// community exists and that a moderator has acted on them. +// +// 3. Ban, for regular users in public communities. A membership row with no +// ban, or NO membership row at all, is not a ban — that is the ordinary +// case, since posting to a public community does not require joining it. +// Any OTHER lookup error FAILS the request. Failing open here would turn a +// Postgres blip into a global unban for its duration, which is the one +// failure mode a ban check exists to prevent. +// +// 4. Aggregator authorization, for registered aggregators. Existing +// semantics, existing sentinels, carried on the decision's Cause. +// +// 5. Dedupe, for EVERY actor class. An aggregator re-polling an RSS feed and +// resubmitting an identical item is the canonical case, so exempting +// trusted actors here would exempt the exact traffic the check is for. +// +// 6. Rate limit, for regular users only. +// +// DEDUPE BEFORE RATE LIMIT. A client whose response was lost retries; if the +// retry burned quota, a flaky connection would rate-limit a user who posted +// once. Dedupe recognises the retry for what it is and refuses it without +// charging for it. +// +// TRUSTED AGGREGATORS skip 2, 3, 4 (existing behaviour) and also skip 6: they +// have no submission limit today, and inventing one here would be a silent +// production behaviour change smuggled in under a refactor. REGISTERED +// aggregators skip 6 for a different reason — they are already governed by +// their own hourly quota inside ValidateAggregatorPost (step 4), and applying +// the new per-author limit as well would silently halve an authorized +// aggregator's throughput. +// +// A REFUSAL CONSUMES NO QUOTA: no refusal leaves a ledger row behind, including +// the rate-limit refusal itself, whose reservation is released before it +// returns. Otherwise an author who kept retrying past their limit would extend +// their own lockout indefinitely. +// +// A non-nil error means the decision could NOT be made — a lookup failed — and +// is distinct from a refusal, which is a decision. +func admitPost(ctx context.Context, deps admissionDeps, req AdmissionRequest) (AdmissionDecision, error) { + return AdmissionDecision{}, nil +} + +// dedupeBucket is the index of the window `now` falls in, so that two +// submissions in the same window collide on the ledger's unique key and two +// submissions a window apart do not. +func dedupeBucket(now time.Time, window time.Duration) int64 { + return 0 +} + +// submissionFingerprint hashes what a moderator would judge about a record: +// everything except createdAt. +// +// The timestamp has to go. It is stamped by the server at submission time +// (service.go step 9), so it differs on every attempt — including the retry +// after a lost response, which is the case dedupe exists to catch. A +// fingerprint that included it would never match anything. +func submissionFingerprint(record PostRecord) string { + return "" +} diff --git a/internal/core/posts/admit_matrix_test.go b/internal/core/posts/admit_matrix_test.go new file mode 100644 index 0000000..0f77e17 --- /dev/null +++ b/internal/core/posts/admit_matrix_test.go @@ -0,0 +1,983 @@ +package posts + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "Coves/internal/core/aggregators" + "Coves/internal/core/communities" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The admitPost decision matrix (docs/PRD_AUTHOR_OWNED_POSTS.md §4.1, §5.6, §8). +// +// admitPost is a pure function over injected lookups and an injected clock, so +// every branch is reachable here without Postgres, without a PDS, and without +// waiting for a rate-limit window to roll. That is the point of the extraction: +// the same checks used to be interleaved with blob uploads and PDS writes +// inside CreatePost, where the only way to reach the private-community branch +// was to provision a private community on a real PDS. +// +// The outer contract — that CreatePost actually consults this, against real +// rows, in the right place in its flow — is service_admission_test.go. What is +// proven HERE is the policy itself, at the width the policy has. +// +// THE LEDGER FAKE IS A MODEL, NOT A RECORDER. stubLedger enforces the same +// unique key the real table does and answers CountSince from the same rows it +// accepted, so "ten admitted then the eleventh refused" is a genuine boundary +// crossing rather than a canned answer. A recorder-shaped fake would let an +// implementation that never consulted the count pass every case below. + +const ( + admitAuthorDID = "did:plc:aaaaaaaaaaaaaaaaaaaaaaaa" + admitAggregatorDID = "did:plc:bbbbbbbbbbbbbbbbbbbbbbbb" + admitCommunityDID = "did:plc:cccccccccccccccccccccccc" + admitCommunityHandle = "!gardening.communities.coves.social" +) + +// --------------------------------------------------------------------------- +// Fakes +// --------------------------------------------------------------------------- + +// stubCommunities answers with one community, or with whichever failure it was +// handed. It counts its calls so a test can prove a later check never ran. +type stubCommunities struct { + community *communities.Community + resolveErr error + getErr error + + resolveCalls int + getCalls int +} + +func (s *stubCommunities) ResolveCommunityIdentifier(_ context.Context, _ string) (string, error) { + s.resolveCalls++ + if s.resolveErr != nil { + return "", s.resolveErr + } + return s.community.DID, nil +} + +func (s *stubCommunities) GetByDID(_ context.Context, _ string) (*communities.Community, error) { + s.getCalls++ + if s.getErr != nil { + return nil, s.getErr + } + return s.community, nil +} + +// stubBans is the community_memberships lookup. Its default — the zero value — +// is the ordinary case: no membership row, because posting in a public +// community has never required joining it. +type stubBans struct { + membership *communities.Membership + err error + + calls int + lastIdentifier string + lastAuthorDID string +} + +func (s *stubBans) GetMembership(_ context.Context, userDID, communityIdentifier string) (*communities.Membership, error) { + s.calls++ + s.lastAuthorDID = userDID + s.lastIdentifier = communityIdentifier + if s.err != nil { + return nil, s.err + } + if s.membership == nil { + return nil, communities.ErrMembershipNotFound + } + return s.membership, nil +} + +// stubAggregatorAuthorizer stands in for aggregators.Service, whose own +// authorization and hourly-quota rules have their own tests. +type stubAggregatorAuthorizer struct { + err error + calls int +} + +func (s *stubAggregatorAuthorizer) ValidateAggregatorPost(_ context.Context, _, _ string) error { + s.calls++ + return s.err +} + +// ledgerRow is one live reservation. +type ledgerRow struct { + id int64 + cmd ReserveSubmissionCommand + at time.Time +} + +// stubLedger models post_submissions in memory: the same unique key, the same +// rolling-window count, and Release genuinely removing the row. +type stubLedger struct { + now Clock + + rows []ledgerRow + nextID int64 + + // reserveErr and countErr force the infrastructure-failure paths. A + // duplicate is NOT set this way — it emerges from the unique key, like it + // does in Postgres. + reserveErr error + countErr error + + reserveCalls []ReserveSubmissionCommand + releaseCalls []SubmissionReservation +} + +func (l *stubLedger) Reserve(_ context.Context, cmd ReserveSubmissionCommand) (SubmissionReservation, error) { + l.reserveCalls = append(l.reserveCalls, cmd) + if l.reserveErr != nil { + return SubmissionReservation{}, l.reserveErr + } + for _, row := range l.rows { + if row.cmd == cmd { + return SubmissionReservation{}, ErrDuplicateSubmission + } + } + l.nextID++ + l.rows = append(l.rows, ledgerRow{id: l.nextID, cmd: cmd, at: l.now()}) + return SubmissionReservation{ID: l.nextID}, nil +} + +func (l *stubLedger) Release(_ context.Context, reservation SubmissionReservation) error { + l.releaseCalls = append(l.releaseCalls, reservation) + kept := l.rows[:0] + for _, row := range l.rows { + if row.id != reservation.ID { + kept = append(kept, row) + } + } + l.rows = kept + return nil +} + +func (l *stubLedger) CountSince(_ context.Context, authorDID, communityDID string, since time.Time) (int, error) { + if l.countErr != nil { + return 0, l.countErr + } + count := 0 + for _, row := range l.rows { + if row.cmd.AuthorDID == authorDID && row.cmd.CommunityDID == communityDID && !row.at.Before(since) { + count++ + } + } + return count, nil +} + +// liveRows is what the ledger holds after the decision — the assertion behind +// "a refusal consumes no quota". +func (l *stubLedger) liveRows() int { return len(l.rows) } + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +// admitHarness is the default world: a public community, an author with no +// membership row, an authorized aggregator, an empty ledger, and a clock that +// only moves when a test moves it. +type admitHarness struct { + communities *stubCommunities + bans *stubBans + aggregators *stubAggregatorAuthorizer + ledger *stubLedger + limits SubmissionLimits + now time.Time +} + +func newAdmitHarness() *admitHarness { + h := &admitHarness{ + communities: &stubCommunities{community: &communities.Community{ + DID: admitCommunityDID, + Handle: admitCommunityHandle, + Visibility: "public", + }}, + bans: &stubBans{}, + aggregators: &stubAggregatorAuthorizer{}, + limits: SubmissionLimits{ + MaxPerAuthorPerCommunity: 3, + Window: time.Hour, + DedupeWindow: time.Hour, + }, + now: time.Date(2026, 8, 1, 12, 0, 0, 0, time.UTC), + } + h.ledger = &stubLedger{now: h.clock()} + return h +} + +// clock hands out a Clock that reads the harness's mutable instant, so +// advancing time after the ledger was built still moves the ledger's clock. +func (h *admitHarness) clock() Clock { + return func() time.Time { return h.now } +} + +func (h *admitHarness) advance(d time.Duration) { h.now = h.now.Add(d) } + +func (h *admitHarness) deps() admissionDeps { + return admissionDeps{ + communities: h.communities, + bans: h.bans, + aggregators: h.aggregators, + ledger: h.ledger, + limits: h.limits, + now: h.clock(), + } +} + +// admit runs the decision for a user submitting `fingerprint`. +func (h *admitHarness) admit(t *testing.T, actor ActorClass, fingerprint string) (AdmissionDecision, error) { + t.Helper() + authorDID := admitAuthorDID + if actor != ActorUser { + authorDID = admitAggregatorDID + } + return admitPost(context.Background(), h.deps(), AdmissionRequest{ + Actor: actor, + AuthorDID: authorDID, + Community: admitCommunityHandle, + Fingerprint: fingerprint, + }) +} + +// banned is a membership row with the ban flag set. +func banned() *communities.Membership { + return &communities.Membership{ + UserDID: admitAuthorDID, + CommunityDID: admitCommunityDID, + IsBanned: true, + } +} + +// --------------------------------------------------------------------------- +// The matrix +// --------------------------------------------------------------------------- + +func TestAdmitPost_DecisionMatrix(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + actor ActorClass + setup func(*admitHarness) + wantCode DecisionCode + why string + }{ + { + name: "a community nobody has indexed", + actor: ActorUser, + setup: func(h *admitHarness) { + h.communities.resolveErr = communities.ErrCommunityNotFound + }, + wantCode: DecisionCommunityNotFound, + why: "nothing else can be evaluated against a community that does not exist", + }, + { + name: "an identifier that resolves to a community the index has since lost", + actor: ActorUser, + setup: func(h *admitHarness) { + h.communities.getErr = communities.ErrCommunityNotFound + }, + wantCode: DecisionCommunityNotFound, + why: "resolution and fetch are two lookups, and either failing to find it is the same answer to the client", + }, + { + name: "a regular user submitting to a private community", + actor: ActorUser, + setup: func(h *admitHarness) { + h.communities.community.Visibility = "private" + }, + wantCode: DecisionCommunityPrivate, + why: "Alpha admits public and unlisted only; membership for private communities is Beta", + }, + { + name: "an unlisted community is not a private one", + actor: ActorUser, + setup: func(h *admitHarness) { + h.communities.community.Visibility = "unlisted" + }, + wantCode: "", + why: "unlisted means undiscoverable, not closed — only 'private' blocks a submission", + }, + { + name: "a BANNED user of a PRIVATE community", + actor: ActorUser, + setup: func(h *admitHarness) { + h.communities.community.Visibility = "private" + h.bans.membership = banned() + }, + wantCode: DecisionCommunityPrivate, + why: "a ban must not be disclosed through a privacy wall: answering author-banned would " + + "confirm to an outsider both that the community exists and that a moderator has acted on them", + }, + { + name: "a banned member of a public community", + actor: ActorUser, + setup: func(h *admitHarness) { + h.bans.membership = banned() + }, + wantCode: DecisionAuthorBanned, + why: "the check §4.1 admits does not exist yet, and this is it", + }, + { + name: "a member in good standing", + actor: ActorUser, + setup: func(h *admitHarness) { + h.bans.membership = &communities.Membership{ + UserDID: admitAuthorDID, CommunityDID: admitCommunityDID, IsBanned: false, + } + }, + wantCode: "", + why: "a membership row is not itself a refusal", + }, + { + name: "a non-member of a public community", + actor: ActorUser, + setup: func(h *admitHarness) { h.bans.membership = nil }, + wantCode: "", + why: "ErrMembershipNotFound is a VALUE meaning 'not banned' — posting in a public " + + "community has never required joining it, so the absent row is the common case", + }, + { + name: "a registered aggregator the community never authorized", + actor: ActorRegisteredAggregator, + setup: func(h *admitHarness) { + h.aggregators.err = aggregators.ErrNotAuthorized + }, + wantCode: DecisionAggregatorNotAuthorized, + why: "existing semantics: being registered with the instance is not permission to write anywhere", + }, + { + name: "a registered aggregator over its OWN hourly quota", + actor: ActorRegisteredAggregator, + setup: func(h *admitHarness) { + h.aggregators.err = aggregators.ErrRateLimitExceeded + }, + wantCode: DecisionAggregatorNotAuthorized, + why: "the aggregator limiter answers through the same call; the sentinel it carries is what tells 403 from 429", + }, + { + name: "a registered aggregator submitting into a PRIVATE community", + actor: ActorRegisteredAggregator, + setup: func(h *admitHarness) { + h.communities.community.Visibility = "private" + }, + wantCode: "", + why: "aggregators are authorized services rather than members, so visibility says nothing " + + "about them — this is today's behaviour and the extraction must not change it", + }, + { + name: "a trusted aggregator submitting into a PRIVATE community it is banned from", + actor: ActorTrustedAggregator, + setup: func(h *admitHarness) { + h.communities.community.Visibility = "private" + h.bans.membership = banned() + h.aggregators.err = aggregators.ErrNotAuthorized + }, + wantCode: "", + why: "a trusted aggregator skips visibility, ban and authorization — all three, deliberately", + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + tc.setup(h) + + decision, err := h.admit(t, tc.actor, "fingerprint-1") + require.NoErrorf(t, err, "a policy refusal is a decision, not an error: %s", tc.why) + + assert.Equalf(t, tc.wantCode, decision.Code, "%s", tc.why) + assert.Equalf(t, tc.wantCode == "", decision.Admitted(), + "Admitted() must agree with the code it is derived from") + + if decision.Admitted() { + require.NotNil(t, decision.Community, + "an admission carries the resolved community so CreatePost does not fetch it a second time") + assert.Equal(t, admitCommunityDID, decision.Community.DID) + require.NotNil(t, decision.Reservation, + "an admission carries the ledger row it reserved, so a failed PDS write can release it") + assert.Equal(t, 1, h.ledger.liveRows()) + return + } + + // The other half of every refusal: it cost nothing. + assert.Zerof(t, h.ledger.liveRows(), + "a refused submission left a ledger row behind, so it burned quota it was never granted") + }) + } +} + +// --------------------------------------------------------------------------- +// Check order +// --------------------------------------------------------------------------- + +// The nondisclosure rule is not just about the CODE returned — the ban lookup +// must not run at all. A private community that queried moderation state before +// refusing would still leak through timing, and would make a banned outsider's +// probe indistinguishable from a member's in the logs. +func TestAdmitPost_PrivateCommunityNeverConsultsModerationState(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.communities.community.Visibility = "private" + h.bans.membership = banned() + + decision, err := h.admit(t, ActorUser, "probe") + require.NoError(t, err) + + require.Equal(t, DecisionCommunityPrivate, decision.Code) + assert.Zero(t, h.bans.calls, + "the privacy wall must refuse before moderation state is read, not after") +} + +// A community that does not resolve short-circuits everything downstream. An +// implementation that gathered every input before deciding would issue a ban +// lookup, an authorization check and a ledger insert against a community DID it +// had just failed to find. +func TestAdmitPost_AnAbsentCommunityStopsEveryLaterCheck(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.communities.resolveErr = communities.ErrCommunityNotFound + + decision, err := h.admit(t, ActorUser, "probe") + require.NoError(t, err) + require.Equal(t, DecisionCommunityNotFound, decision.Code) + + assert.Zero(t, h.bans.calls, "a ban lookup ran against a community that does not exist") + assert.Zero(t, h.aggregators.calls, "an authorization check ran against a community that does not exist") + assert.Empty(t, h.ledger.reserveCalls, "a ledger row was reserved against a community that does not exist") +} + +// The ban lookup is scoped to the RESOLVED community, not to whatever +// at-identifier the client happened to send. Handles are mutable; a ban keyed +// by handle stops applying the moment a community renames itself. +func TestAdmitPost_BanIsLookedUpByResolvedDID(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + _, err := h.admit(t, ActorUser, "probe") + require.NoError(t, err) + + require.Equal(t, 1, h.bans.calls) + assert.Equal(t, admitCommunityDID, h.bans.lastIdentifier, + "the ban must be looked up against the resolved DID: a handle is mutable, and a ban keyed to one stops applying at rename") + assert.Equal(t, admitAuthorDID, h.bans.lastAuthorDID) +} + +// Neither aggregator class is subject to the ban lookup. Registered and trusted +// aggregators are services rather than members; there is no membership row to +// find, and asking for one on every syndicated item is a query per post for an +// answer that is structurally always the same. +func TestAdmitPost_AggregatorsAreNotBanChecked(t *testing.T) { + t.Parallel() + + for _, actor := range []ActorClass{ActorRegisteredAggregator, ActorTrustedAggregator} { + t.Run(string(actor), func(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.bans.membership = banned() + + decision, err := h.admit(t, actor, "syndicated") + require.NoError(t, err) + + assert.True(t, decision.Admitted(), "an aggregator was refused by a membership rule that does not apply to it") + assert.Zero(t, h.bans.calls, "the ban lookup ran for an actor class that has no membership") + }) + } +} + +// Only registered aggregators meet the authorization check. A trusted one is +// authorized by configuration, and a regular user has no aggregator identity to +// check at all. +func TestAdmitPost_OnlyRegisteredAggregatorsAreAuthorizationChecked(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + actor ActorClass + wantCalls int + }{ + {ActorUser, 0}, + {ActorRegisteredAggregator, 1}, + {ActorTrustedAggregator, 0}, + } { + t.Run(string(tc.actor), func(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + _, err := h.admit(t, tc.actor, "item") + require.NoError(t, err) + + assert.Equal(t, tc.wantCalls, h.aggregators.calls) + }) + } +} + +// An aggregator refusal must carry the aggregators-package sentinel that +// caused it. +// +// The two refusals share one DecisionCode but mean opposite things to a machine +// client: 403 says stop asking, 429 says ask later. The API boundary tells them +// apart with aggregators.IsUnauthorized and aggregators.IsRateLimited over the +// error CreatePost returns (internal/api/handlers/post/errors.go), so the +// sentinel has to survive the decision. Collapsed into a bare code, a +// well-behaved aggregator would retry a permanent refusal forever. +func TestAdmitPost_AnAggregatorRefusalKeepsItsSentinel(t *testing.T) { + t.Parallel() + + for _, sentinel := range []error{aggregators.ErrNotAuthorized, aggregators.ErrRateLimitExceeded} { + t.Run(sentinel.Error(), func(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.aggregators.err = fmt.Errorf("aggregators: %w", sentinel) + + decision, err := h.admit(t, ActorRegisteredAggregator, "item") + require.NoError(t, err) + require.Equal(t, DecisionAggregatorNotAuthorized, decision.Code) + + require.NotNil(t, decision.Cause, + "the refusal dropped the aggregator sentinel, so the boundary cannot tell 403 from 429") + assert.ErrorIs(t, decision.Cause, sentinel) + }) + } +} + +// --------------------------------------------------------------------------- +// Failing closed +// --------------------------------------------------------------------------- + +// A ban lookup that fails for any reason OTHER than "no such membership" must +// fail the request. +// +// This is the single most consequential line in the whole decision. Treating an +// unreachable database as "not banned" would turn a Postgres blip into a global +// unban for its duration — every banned author in every community able to post +// again, with nothing in the logs but a warning. The safe direction is to +// refuse a submission we cannot evaluate. +func TestAdmitPost_ABanLookupFailureFailsTheRequest(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.bans.err = errors.New("dial tcp 10.0.0.4:5432: connect: connection refused") + + decision, err := h.admit(t, ActorUser, "probe") + + require.Error(t, err, "an unevaluable ban check must fail the request, never fall through to admitted") + assert.False(t, decision.Admitted()) + assert.Empty(t, h.ledger.reserveCalls, "a submission we could not evaluate must not reserve quota") +} + +// The infrastructure failures that are NOT policy answers. Each is a decision +// that could not be made, which is a different thing from a refusal — and a +// caller that mapped them to a 4xx would tell a client its perfectly good +// request was its own fault. +func TestAdmitPost_InfrastructureFailuresAreErrorsNotRefusals(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + setup func(*admitHarness) + }{ + { + name: "the community index is unreachable", + setup: func(h *admitHarness) { h.communities.resolveErr = errors.New("connection reset by peer") }, + }, + { + name: "the community fetch fails after resolution succeeded", + setup: func(h *admitHarness) { h.communities.getErr = errors.New("connection reset by peer") }, + }, + { + name: "the ledger insert fails for a reason that is not a duplicate", + setup: func(h *admitHarness) { h.ledger.reserveErr = errors.New("deadlock detected") }, + }, + { + name: "the quota count fails", + setup: func(h *admitHarness) { h.ledger.countErr = errors.New("statement timeout") }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + tc.setup(h) + + decision, err := h.admit(t, ActorUser, "probe") + require.Error(t, err) + assert.False(t, decision.Admitted(), + "a decision that could not be made must not read as an admission") + assert.Emptyf(t, decision.Code, + "an infrastructure failure must not be dressed up as a policy code (%q); the client would be told to stop retrying something that will work in a second", decision.Code) + }) + } +} + +// A malformed community identifier is a client error, and it has to stay one: +// the API boundary turns a validation error into a 400 naming the bad field, +// while an unclassified error becomes an opaque 500. +func TestAdmitPost_AMalformedIdentifierStaysAValidationError(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.communities.resolveErr = communities.NewValidationError("community", "handle must start with !") + + _, err := h.admit(t, ActorUser, "probe") + require.Error(t, err) + assert.True(t, IsValidationError(err), + "a malformed identifier must reach the boundary as a validation error, not as an unclassified failure: %v", err) +} + +// A quota refusal must leave the ledger exactly as it found it. The reservation +// is inserted before the count is taken — that is what closes the concurrent +// double-tap — so the refusal path is responsible for taking it back out. +// Without that, an author who kept retrying past their limit would extend their +// own lockout with every attempt. +func TestAdmitPost_ARateLimitRefusalReleasesItsOwnReservation(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + for i := 0; i < h.limits.MaxPerAuthorPerCommunity; i++ { + decision, err := h.admit(t, ActorUser, fmt.Sprintf("inside-quota-%d", i)) + require.NoError(t, err) + require.Truef(t, decision.Admitted(), "submission %d of %d was refused inside the quota with %q", + i+1, h.limits.MaxPerAuthorPerCommunity, decision.Code) + } + + decision, err := h.admit(t, ActorUser, "one-too-many") + require.NoError(t, err) + require.Equal(t, DecisionRateLimitExceeded, decision.Code) + + assert.Equal(t, h.limits.MaxPerAuthorPerCommunity, h.ledger.liveRows(), + "the refused submission left its reservation on the ledger, so retrying extends the author's own lockout") + assert.NotEmpty(t, h.ledger.releaseCalls, + "the rate-limit path must release the reservation it took before counting") +} + +// --------------------------------------------------------------------------- +// Quota +// --------------------------------------------------------------------------- + +// The boundary itself: N admitted, N+1 refused. Asserting only "the fourth +// fails" would pass against an implementation that refused the third too. +func TestAdmitPost_ExactlyTheLimitIsAdmitted(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.limits.MaxPerAuthorPerCommunity = 5 + + for i := 0; i < 5; i++ { + decision, err := h.admit(t, ActorUser, fmt.Sprintf("item-%d", i)) + require.NoError(t, err) + assert.Truef(t, decision.Admitted(), + "submission %d of 5 was refused with %q, so the limit is being applied one short", i+1, decision.Code) + } + + decision, err := h.admit(t, ActorUser, "item-5") + require.NoError(t, err) + assert.Equal(t, DecisionRateLimitExceeded, decision.Code, + "the sixth submission must be refused, or the limit is being applied one too generously") +} + +// The window is ROLLING, not a bucket that empties on the hour: rows age out +// individually, so an author who filled their quota gets one slot back exactly +// one window after the submission that took it — not all of them at a +// boundary, which would let them spend a double quota either side of it. +func TestAdmitPost_QuotaRecoversAsTheWindowRolls(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.limits.MaxPerAuthorPerCommunity = 2 + h.limits.Window = time.Hour + + first, err := h.admit(t, ActorUser, "first") + require.NoError(t, err) + require.True(t, first.Admitted()) + + h.advance(30 * time.Minute) + second, err := h.admit(t, ActorUser, "second") + require.NoError(t, err) + require.True(t, second.Admitted()) + + third, err := h.admit(t, ActorUser, "third") + require.NoError(t, err) + require.Equal(t, DecisionRateLimitExceeded, third.Code, "the quota is two and both are inside the window") + + // Cross the window relative to the FIRST submission only. One slot frees; + // the second submission is still 30 minutes inside the window. + h.advance(31 * time.Minute) + fourth, err := h.admit(t, ActorUser, "fourth") + require.NoError(t, err) + assert.True(t, fourth.Admitted(), + "the first submission has aged out of the rolling window, so its slot must be available again") + + fifth, err := h.admit(t, ActorUser, "fifth") + require.NoError(t, err) + assert.Equal(t, DecisionRateLimitExceeded, fifth.Code, + "only ONE slot aged out; a window that emptied wholesale would admit this too") +} + +// Neither aggregator class is metered by the new per-author limit. +// +// A trusted aggregator has no submission limit today and inventing one here +// would be a production behaviour change smuggled in under a refactor. A +// registered one is already metered by its own hourly quota inside +// ValidateAggregatorPost, and counting it twice would silently halve the +// throughput every authorized aggregator was granted. +func TestAdmitPost_AggregatorsAreNotMeteredByTheNewLimit(t *testing.T) { + t.Parallel() + + for _, actor := range []ActorClass{ActorRegisteredAggregator, ActorTrustedAggregator} { + t.Run(string(actor), func(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.limits.MaxPerAuthorPerCommunity = 2 + + for i := 0; i < 5; i++ { + decision, err := h.admit(t, actor, fmt.Sprintf("syndicated-%d", i)) + require.NoError(t, err) + assert.Truef(t, decision.Admitted(), + "item %d was refused with %q; %s is not subject to the per-author limit", i+1, decision.Code, actor) + } + }) + } +} + +// --------------------------------------------------------------------------- +// Dedupe +// --------------------------------------------------------------------------- + +// Every actor class deduplicates. An RSS aggregator re-polling a feed and +// resubmitting an identical item is the canonical case, so exempting trusted +// actors would exempt precisely the traffic this check exists for. +func TestAdmitPost_IdenticalResubmissionIsRefusedForEveryActorClass(t *testing.T) { + t.Parallel() + + for _, actor := range []ActorClass{ActorUser, ActorRegisteredAggregator, ActorTrustedAggregator} { + t.Run(string(actor), func(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + + first, err := h.admit(t, actor, "the same item") + require.NoError(t, err) + require.True(t, first.Admitted()) + + second, err := h.admit(t, actor, "the same item") + require.NoError(t, err) + assert.Equal(t, DecisionDuplicateSubmission, second.Code, + "an identical resubmission must be refused as a repeat") + assert.Equal(t, 1, h.ledger.liveRows(), + "the refused duplicate must not add a second row") + }) + } +} + +// Dedupe runs BEFORE the rate limit, and this is the case that tells them +// apart: an author already at their quota who retries something they have +// already sent. If the order were reversed, a client whose response was lost +// would be told to slow down when what actually happened is that its post +// already exists — and its retry would have burned a quota slot for a post it +// did not make. +func TestAdmitPost_DedupeAnswersBeforeTheQuotaDoes(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.limits.MaxPerAuthorPerCommunity = 2 + + for i := 0; i < 2; i++ { + decision, err := h.admit(t, ActorUser, fmt.Sprintf("item-%d", i)) + require.NoError(t, err) + require.True(t, decision.Admitted()) + } + + // At quota AND a repeat. Both refusals apply; dedupe is the honest one. + decision, err := h.admit(t, ActorUser, "item-0") + require.NoError(t, err) + assert.Equal(t, DecisionDuplicateSubmission, decision.Code, + "a retry of an already-accepted submission must be reported as a repeat, not as a quota breach") +} + +// Dedupe expires. The ledger's unique key is scoped to a window bucket +// precisely so that "do not accept the same thing twice right now" does not +// silently become "this author may never post this content again". +func TestAdmitPost_DedupeExpiresWithItsWindow(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.limits.DedupeWindow = time.Hour + h.limits.MaxPerAuthorPerCommunity = 10 + + first, err := h.admit(t, ActorUser, "a link worth reposting") + require.NoError(t, err) + require.True(t, first.Admitted()) + + h.advance(2 * time.Hour) + + second, err := h.admit(t, ActorUser, "a link worth reposting") + require.NoError(t, err) + assert.True(t, second.Admitted(), + "identical content a dedupe window later is a repost, not a duplicate submission") + + require.Len(t, h.ledger.reserveCalls, 2) + assert.NotEqual(t, h.ledger.reserveCalls[0].DedupeBucket, h.ledger.reserveCalls[1].DedupeBucket, + "the two submissions must fall in different dedupe buckets, or the unique key would still collide") +} + +// Two submissions inside one window share a bucket — the other half of the +// property above, and the one that actually makes the unique key bite. +func TestAdmitPost_SubmissionsInsideOneWindowShareABucket(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.limits.DedupeWindow = time.Hour + + _, err := h.admit(t, ActorUser, "first") + require.NoError(t, err) + + h.advance(5 * time.Minute) + _, err = h.admit(t, ActorUser, "second") + require.NoError(t, err) + + require.Len(t, h.ledger.reserveCalls, 2) + assert.Equal(t, h.ledger.reserveCalls[0].DedupeBucket, h.ledger.reserveCalls[1].DedupeBucket, + "submissions five minutes apart in an hourly window must share a bucket") +} + +// What the ledger is actually asked to store. The fingerprint is the client's +// content hash and must arrive unmodified, and the community must be the +// resolved DID for the same reason the ban lookup is. +func TestAdmitPost_TheReservationDescribesTheSubmission(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + _, err := h.admit(t, ActorUser, "sha256-of-the-canonical-record") + require.NoError(t, err) + + require.Len(t, h.ledger.reserveCalls, 1) + reserved := h.ledger.reserveCalls[0] + assert.Equal(t, admitAuthorDID, reserved.AuthorDID) + assert.Equal(t, admitCommunityDID, reserved.CommunityDID, + "the ledger is keyed by resolved DID; a handle would let a rename reset both the quota and the dedupe key") + assert.Equal(t, "sha256-of-the-canonical-record", reserved.Fingerprint) +} + +// Quota and dedupe are per (author, community): a user at their limit in one +// community must still be able to post in another. A limit that leaked across +// communities would make one busy community silence its author everywhere. +func TestAdmitPost_QuotaIsScopedToOneCommunity(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.limits.MaxPerAuthorPerCommunity = 1 + + first, err := h.admit(t, ActorUser, "item") + require.NoError(t, err) + require.True(t, first.Admitted()) + + refused, err := h.admit(t, ActorUser, "another item") + require.NoError(t, err) + require.Equal(t, DecisionRateLimitExceeded, refused.Code) + + // The same author, the same content, a different community. + h.communities.community = &communities.Community{ + DID: "did:plc:dddddddddddddddddddddddd", + Handle: "!woodworking.communities.coves.social", + Visibility: "public", + } + + elsewhere, err := h.admit(t, ActorUser, "item") + require.NoError(t, err) + assert.True(t, elsewhere.Admitted(), + "the quota is per community; being at the limit in one must not silence the author in another") +} + +// --------------------------------------------------------------------------- +// The sentinel's wording +// --------------------------------------------------------------------------- + +// IsConflict classifies by substring, so a duplicate SUBMISSION worded like a +// duplicate KEY would be misread as an indexing conflict — a post refused at +// the admission gate reported to its author as one that already exists. +func TestErrDuplicateSubmissionIsNotAStorageConflict(t *testing.T) { + t.Parallel() + + assert.False(t, IsConflict(ErrDuplicateSubmission), + "ErrDuplicateSubmission's message collides with IsConflict's substring match (%q); reword the sentinel", + ErrDuplicateSubmission.Error()) + assert.False(t, IsConflict(fmt.Errorf("createPost: %w", ErrDuplicateSubmission)), + "the wrapped form must not be misclassified either — that is the shape the boundary actually sees") +} + +// --------------------------------------------------------------------------- +// Fingerprint +// --------------------------------------------------------------------------- + +// The fingerprint is what makes two submissions "identical". createdAt is +// stamped per attempt, so including it would make every retry look new and +// dedupe would never fire; everything a moderator would judge must be included, +// or two genuinely different posts would collide and the second would be +// refused as a repeat of the first. +func TestSubmissionFingerprint(t *testing.T) { + t.Parallel() + + base := func() PostRecord { + title, content := "A title", "Some body text" + return PostRecord{ + Type: "social.coves.community.postv2", + Community: admitCommunityDID, + Author: admitAuthorDID, + Title: &title, + Content: &content, + CreatedAt: "2026-08-01T12:00:00Z", + } + } + + t.Run("createdAt is excluded", func(t *testing.T) { + t.Parallel() + + later := base() + later.CreatedAt = "2026-08-01T12:00:09Z" + + assert.Equal(t, submissionFingerprint(base()), submissionFingerprint(later), + "the server stamps createdAt per attempt, so a fingerprint that included it would never match a retry") + }) + + t.Run("a non-empty fingerprint", func(t *testing.T) { + t.Parallel() + + assert.NotEmpty(t, submissionFingerprint(base()), + "an empty fingerprint would make every submission collide with every other") + }) + + for _, tc := range []struct { + field string + mutate func(*PostRecord) + }{ + {"title", func(r *PostRecord) { title := "A different title"; r.Title = &title }}, + {"content", func(r *PostRecord) { content := "Different body text"; r.Content = &content }}, + {"community", func(r *PostRecord) { r.Community = "did:plc:dddddddddddddddddddddddd" }}, + {"author", func(r *PostRecord) { r.Author = "did:plc:eeeeeeeeeeeeeeeeeeeeeeee" }}, + {"embed", func(r *PostRecord) { + r.Embed = map[string]interface{}{"$type": "social.coves.embed.external"} + }}, + } { + t.Run("a different "+tc.field+" is a different submission", func(t *testing.T) { + t.Parallel() + + changed := base() + tc.mutate(&changed) + assert.NotEqual(t, submissionFingerprint(base()), submissionFingerprint(changed), + "two posts differing in %s would collide, and the second would be refused as a repeat of the first", tc.field) + }) + } +} diff --git a/internal/core/posts/errors.go b/internal/core/posts/errors.go index 8d2dffd..713d9de 100644 --- a/internal/core/posts/errors.go +++ b/internal/core/posts/errors.go @@ -34,6 +34,20 @@ var ( // ErrActorNotFound is returned when the requested actor does not exist ErrActorNotFound = errors.New("actor not found") + + // ErrDuplicateSubmission is returned when an author resubmits content + // identical to something already on the submission ledger for the current + // dedupe window (PRD_AUTHOR_OWNED_POSTS.md §8). + // + // THE WORDING IS LOAD-BEARING. IsConflict below classifies an error by + // looking for "duplicate key", "already exists" or "already indexed" in its + // text, because a genuine index conflict arrives from the driver as a + // string rather than as a typed error. This sentinel must therefore avoid + // all three phrasings: a duplicate SUBMISSION is a client being refused at + // the admission gate, while a conflict is the indexer meeting a record it + // already has, and collapsing them would let a refused post be reported as + // successfully indexed. + ErrDuplicateSubmission = errors.New("an identical submission from this author to this community was refused as a repeat") ) // ValidationError is the shared validation error type. It is aliased rather diff --git a/internal/core/posts/service.go b/internal/core/posts/service.go index 877a0c9..99d0870 100644 --- a/internal/core/posts/service.go +++ b/internal/core/posts/service.go @@ -34,6 +34,7 @@ type postService struct { unfurlService unfurl.Service blueskyService blueskypost.Service blockChecker BlockChecker + admission *AdmissionPolicy pdsURL string } diff --git a/internal/core/posts/service_admission_test.go b/internal/core/posts/service_admission_test.go new file mode 100644 index 0000000..6b43841 --- /dev/null +++ b/internal/core/posts/service_admission_test.go @@ -0,0 +1,394 @@ +//go:build integration + +package posts_test + +import ( + "context" + "database/sql" + "fmt" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" + + "Coves/internal/api/middleware" + "Coves/internal/core/communities" + "Coves/internal/core/posts" + "Coves/internal/db/postgres" + "Coves/tests/testkit" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The outer contract for admission: what CreatePost does when the admission +// policy of docs/PRD_AUTHOR_OWNED_POSTS.md §4.1/§8 refuses a submission. +// +// The decision itself is covered at width in admit_matrix_test.go against +// fakes. What is unproven without these is the WIRING, and the wiring is where +// this kind of check historically goes wrong: +// +// - that CreatePost consults the policy AT ALL. §4.1 corrects rev 1 of the +// spec on exactly this point — the service docstring has claimed +// "membership/ban validation" since the beginning and no ban lookup has +// ever existed on the write path. +// - that it consults it BEFORE the PDS write, so a refusal leaves no record +// in a community that refused it. +// - that the ledger rows the quota is counted against are the ones CreatePost +// itself writes. Like the aggregator quota (service_aggregator_test.go), +// the producer and the consumer of that counter are the same code path, and +// a service that never wrote them would pass every unit test and never rate +// limit anything. +// - that a failed PDS write RELEASES the row it reserved. This is the one +// behaviour no fake can prove, because it is about what survives in the +// database after a write that did not happen. +// +// ON SEEDING is_banned DIRECTLY. Nothing in production writes +// community_memberships at all today — not CreateMembership, not +// UpdateMembership; both are reachable only from tests. There is no ban +// endpoint, no moderation consumer, and no firehose path that sets the column. +// So these tests seed it through the repository, and that is an honest +// admission of an incomplete feature rather than a shortcut: §4.1 specifies the +// ban LOOKUP, and the record type that will eventually write it +// (social.coves.moderation.ban) is not this task's work. When it lands, this +// seeding becomes the moderation path and these assertions do not change. + +// admissionFixture is the post service wired with the §8 admission policy over +// real Postgres, a real PDS, and a clock the test controls. +type admissionFixture struct { + base *postFixture + service posts.Service + repo communities.Repository + clock *testClock + limits posts.SubmissionLimits +} + +// testClock is the injected Clock. Time moves only when a test moves it, which +// is what lets a rolling window be crossed without a sleep — docs/ +// TEST_ARCHITECTURE.md §3.3 bans the alternative outright. +// +// It is mutex-guarded because CreatePost may read it from more than one +// goroutine, and the race detector is on in CI. +type testClock struct { + mu sync.Mutex + now time.Time +} + +func (c *testClock) Now() time.Time { + c.mu.Lock() + defer c.mu.Unlock() + return c.now +} + +func (c *testClock) Advance(d time.Duration) { + c.mu.Lock() + defer c.mu.Unlock() + c.now = c.now.Add(d) +} + +// newAdmissionFixture provisions a community on the test PDS and points a +// policy-wired post service at it. +// +// The limit is deliberately small. Each admitted submission costs a real +// createRecord against a real PDS, and the assertion that matters is the +// BOUNDARY — N admitted, N+1 refused — which three proves exactly as well as +// three hundred would. +func newAdmissionFixture(t *testing.T) *admissionFixture { + t.Helper() + + base := newPostFixture(t) + limits := posts.SubmissionLimits{ + MaxPerAuthorPerCommunity: 3, + Window: time.Hour, + DedupeWindow: time.Hour, + } + + // Anchored at the real present rather than at a fixed date: post_submissions + // stamps created_at server-side (NOW()), while the rolling-window query is + // computed from THIS clock, so the two have to agree about roughly when + // "now" is. Advancing forwards is always safe — it ages real rows out of the + // window, which is the direction every test here moves. + clock := &testClock{now: time.Now().UTC()} + + return &admissionFixture{ + base: base, + service: posts.NewPostService( + postgres.NewPostRepository(base.db), base.communityService, + nil, nil, nil, nil, base.pds.URL(), + posts.WithAdmissionPolicy(posts.AdmissionPolicy{ + Ledger: postgres.NewSubmissionLedger(base.db), + Bans: base.communityService, + Limits: limits, + Now: clock.Now, + })), + repo: postgres.NewCommunityRepository(base.db), + clock: clock, + limits: limits, + } +} + +// submit posts as the fixture's author. Unlike postFixture.createPost it +// returns the error, because every test here is about a refusal. +func (f *admissionFixture) submit(t *testing.T, communityDID, title string) (*posts.CreatePostResponse, error) { + t.Helper() + + content := "a body that makes this a complete post" + return f.service.CreatePost( + middleware.SetTestUserDID(context.Background(), f.base.author.DID), + posts.CreatePostRequest{ + Community: communityDID, + Title: &title, + Content: &content, + AuthorDID: f.base.author.DID, + }) +} + +// setBanned seeds (or updates) the author's membership of the fixture's +// community with the given ban state. +func (f *admissionFixture) setBanned(t *testing.T, banned bool) { + t.Helper() + + ctx := context.Background() + membership := &communities.Membership{ + UserDID: f.base.author.DID, + CommunityDID: f.base.community.DID, + JoinedAt: time.Now().UTC(), + LastActiveAt: time.Now().UTC(), + IsBanned: banned, + } + + if _, err := f.repo.GetMembership(ctx, f.base.author.DID, f.base.community.DID); err != nil { + require.ErrorIs(t, err, communities.ErrMembershipNotFound) + _, createErr := f.repo.CreateMembership(ctx, membership) + require.NoError(t, createErr) + return + } + + _, err := f.repo.UpdateMembership(ctx, membership) + require.NoError(t, err) +} + +// ledgerRows counts what the submission ledger holds for the author in one +// community — the number the quota is enforced against. +// +// Read with raw SQL rather than through the repository on purpose: the +// repository is the thing under test here, and a test that asked it to report +// its own state would pass against an implementation that recorded nothing. +func (f *admissionFixture) ledgerRows(t *testing.T, communityDID string) int { + t.Helper() + return countSubmissions(t, f.base.db, f.base.author.DID, communityDID) +} + +func countSubmissions(t *testing.T, db *sql.DB, authorDID, communityDID string) int { + t.Helper() + + var count int + require.NoError(t, db.QueryRowContext(context.Background(), ` + SELECT count(*) FROM post_submissions WHERE author_did = $1 AND community_did = $2 + `, authorDID, communityDID).Scan(&count), + "the post_submissions ledger (migration 035) must exist for the quota to be countable") + return count +} + +// anotherCommunity provisions a second community on the same PDS, so that a +// per-community rule can be shown to be per-community. +func (f *admissionFixture) anotherCommunity(t *testing.T) *communities.Community { + t.Helper() + + name := testkit.UniqueIDWithPrefix(t, "ad") + require.LessOrEqualf(t, len("c-"+name), testkit.MaxIDLength, + "the generated community name %q makes a handle label the PDS will refuse", name) + + community, err := f.base.communityService.CreateCommunity(context.Background(), communities.CreateCommunityRequest{ + Name: name, + DisplayName: "Somewhere else", + Description: "a second community, to prove the quota is scoped to one", + Visibility: "public", + CreatedByDID: f.base.author.DID, + }) + require.NoError(t, err) + return community +} + +// --------------------------------------------------------------------------- + +// A ban stops the next post, and lifting it lets the author back in. +// +// Both halves matter. A ban that could not be lifted would be a data-loss bug +// dressed as a moderation feature, and it is the kind that only shows up when a +// moderator tries to undo a mistake. +func TestService_ABannedMemberIsRefusedAndAnUnbanRestoresThem(t *testing.T) { + t.Parallel() + + f := newAdmissionFixture(t) + + // Before the ban, the author is an ordinary poster in a public community. + _, err := f.submit(t, f.base.community.DID, "posted while in good standing") + require.NoError(t, err) + + f.setBanned(t, true) + + _, err = f.submit(t, f.base.community.DID, "posted after the ban") + require.Error(t, err) + assert.ErrorIsf(t, err, posts.ErrBanned, + "the handler maps this sentinel to a 403 Banned, so the sentinel identity is the contract; got: %v", err) + + // Refused means nothing was written and nothing was billed. The check runs + // ahead of the PDS write; an implementation that reordered them would leave + // a banned author's post in the community's repository, from which the + // firehose would index it before anything noticed. + assert.Equal(t, 1, f.ledgerRows(t, f.base.community.DID), + "the refused submission consumed quota it was never granted") + + f.setBanned(t, false) + + _, err = f.submit(t, f.base.community.DID, "posted after the unban") + assert.NoError(t, err, "lifting a ban must take effect on the next post, not on the next restart") +} + +// The per-author quota: N admitted, N+1 refused, and the limit is per +// community. +// +// Asserting only that "the fourth fails" would pass against an implementation +// that refused the third too, which is why every submission inside the quota is +// individually required to succeed. +func TestService_TheAuthorQuotaStopsTheNextSubmission(t *testing.T) { + t.Parallel() + + f := newAdmissionFixture(t) + + for i := 0; i < f.limits.MaxPerAuthorPerCommunity; i++ { + _, err := f.submit(t, f.base.community.DID, fmt.Sprintf("submission %d", i)) + require.NoErrorf(t, err, "submission %d of %d was refused inside the quota", + i+1, f.limits.MaxPerAuthorPerCommunity) + } + require.Equal(t, f.limits.MaxPerAuthorPerCommunity, f.ledgerRows(t, f.base.community.DID)) + + _, err := f.submit(t, f.base.community.DID, "one submission too many") + require.Error(t, err) + assert.ErrorIsf(t, err, posts.ErrRateLimitExceeded, + "the handler maps this to a 429; got: %v", err) + + // A refused submission is not billed, so an author cannot spend past their + // quota by ignoring the error — and, more to the point, cannot extend their + // own lockout by retrying. + assert.Equal(t, f.limits.MaxPerAuthorPerCommunity, f.ledgerRows(t, f.base.community.DID)) + + // The same author, at their limit here, is unaffected there. A quota that + // leaked across communities would let one busy community silence its author + // everywhere on the instance. + elsewhere := f.anotherCommunity(t) + _, err = f.submit(t, elsewhere.DID, "a submission somewhere else") + assert.NoError(t, err, "the quota is per (author, community); being at the limit in one must not close the others") +} + +// An identical resubmission is a repeat, not a new post. +// +// The canonical case is a client that retried after a lost response, and the +// answer has to be distinguishable from a quota breach: 409 tells the client its +// post already exists, 429 tells it to wait. A submission refused as a duplicate +// must also not be billed, or a flaky connection would rate-limit a user who +// posted once. +func TestService_AnIdenticalResubmissionIsRefusedAsADuplicate(t *testing.T) { + t.Parallel() + + f := newAdmissionFixture(t) + + _, err := f.submit(t, f.base.community.DID, "the very same post") + require.NoError(t, err) + + _, err = f.submit(t, f.base.community.DID, "the very same post") + require.Error(t, err) + assert.ErrorIsf(t, err, posts.ErrDuplicateSubmission, + "the handler maps this to a 409 DuplicateSubmission; got: %v", err) + + assert.Equal(t, 1, f.ledgerRows(t, f.base.community.DID), + "the duplicate must be refused without adding a second ledger row") + + // And a genuinely different post from the same author still goes through: + // the refusal is about identical content, not about having posted recently. + _, err = f.submit(t, f.base.community.DID, "a different post entirely") + assert.NoError(t, err) +} + +// A PDS write that fails must give the reservation back. +// +// The ledger row goes in BEFORE the record is written — that ordering is what +// closes the concurrent double-tap, since the unique constraint is the only +// arbiter that both racing requests can agree on. The cost of that choice is +// that the failure path owes the author their slot back. If it does not pay, +// every PDS hiccup permanently consumes one submission from the author's quota +// AND blocks them from retrying the same content at all, which turns a +// transient outage into a per-user lockout that outlives it. +func TestService_AFailedPDSWriteReleasesTheReservation(t *testing.T) { + t.Parallel() + + f := newAdmissionFixture(t) + + // A PDS that refuses every write. Pointing the community's stored pds_url at + // it is how the failure is injected: createPostOnPDS reads the URL off the + // community row it just fetched (service.go, "each community can be hosted + // on a different PDS instance"), so this is the real write path failing for + // a real reason rather than a stubbed-out client. + broken := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, `{"error":"InternalServerError"}`, http.StatusInternalServerError) + })) + t.Cleanup(broken.Close) + + healthyURL := communityPDSURL(t, f.base.db, f.base.community.DID) + setCommunityPDSURL(t, f.base.db, f.base.community.DID, broken.URL) + + const repeatable = "a post whose write will fail the first time" + _, err := f.submit(t, f.base.community.DID, repeatable) + require.Error(t, err, "the PDS refused the write, so CreatePost must report a failure") + + assert.Zerof(t, f.ledgerRows(t, f.base.community.DID), + "the reservation for a post that was never written is still on the ledger: it has burned a quota slot and will refuse the retry as a duplicate") + + setCommunityPDSURL(t, f.base.db, f.base.community.DID, healthyURL) + + // The retry a client would actually send: byte-identical content. It must be + // admitted, which is only possible if the reservation was released. + resp, err := f.submit(t, f.base.community.DID, repeatable) + require.NoErrorf(t, err, "the identical retry after a failed write was refused, so the failure path leaked its reservation") + require.NotEmpty(t, resp.URI) + + assert.Equal(t, 1, f.ledgerRows(t, f.base.community.DID), + "exactly one submission survived: the failed attempt released its row and the retry took a fresh one") + + // The record really is in the community's repo, so "admitted" here means a + // post exists rather than merely that no error came back. + record := f.base.communityAccount(t).GetRecord(t, postCollection, rkeyOf(t, resp.URI)) + assert.Equal(t, f.base.author.DID, record.Value["author"]) +} + +// communityPDSURL reads a community's stored PDS, so a test that repoints it +// can put back what was actually there rather than what it assumed. +func communityPDSURL(t *testing.T, db *sql.DB, communityDID string) string { + t.Helper() + + var pdsURL string + require.NoError(t, db.QueryRowContext(context.Background(), + `SELECT pds_url FROM communities WHERE did = $1`, communityDID).Scan(&pdsURL)) + require.NotEmpty(t, pdsURL) + return pdsURL +} + +// setCommunityPDSURL repoints a community at a different PDS. +// +// Written with SQL because there is no service method for it: a community's PDS +// is chosen when it is provisioned and never moves. That is exactly why it is a +// usable seam for an unreachable-PDS test — the value is read fresh on every +// write (EnsureFreshToken re-fetches the row), and nothing caches it. +func setCommunityPDSURL(t *testing.T, db *sql.DB, communityDID, pdsURL string) { + t.Helper() + + result, err := db.ExecContext(context.Background(), + `UPDATE communities SET pds_url = $1 WHERE did = $2`, pdsURL, communityDID) + require.NoError(t, err) + + affected, err := result.RowsAffected() + require.NoError(t, err) + require.EqualValues(t, 1, affected, "no community row was repointed, so the test would prove nothing") +} diff --git a/internal/db/postgres/admission_repo_schema_test.go b/internal/db/postgres/admission_repo_schema_test.go index 2eb7499..498d943 100644 --- a/internal/db/postgres/admission_repo_schema_test.go +++ b/internal/db/postgres/admission_repo_schema_test.go @@ -318,9 +318,13 @@ func TestMigration034_DownRestoresTheAuthorForeignKeyUnvalidated(t *testing.T) { require.NoError(t, err, "with fk_author dropped, a federated author's post must index even though no users row exists for them") - // The expected-version parameter is the tripwire: when migration 035 lands, - // this call fails with the remedy in its message instead of silently - // rolling back 035's Down and leaving 034's untested. + // The expected-version parameter is the tripwire, and it has fired once + // already: migration 035 (post_submissions) now sits on top of 034, so it + // has to come off first. Rolling back explicitly, one asserted step at a + // time, is what keeps the assertions below pointed at 034's Down rather than + // at whatever happens to be newest. + require.EqualValues(t, 35, testkit.MigrateDownOne(t, db, 35), + "035 sits on top of 034 and must be rolled back first; asserting which migration came off is what stops this test drifting onto a newer one") assert.EqualValues(t, 34, testkit.MigrateDownOne(t, db, 34), "this test asserts on 034's Down section; rolling back a different migration would prove nothing about it") @@ -361,7 +365,7 @@ func requireTableExists(t *testing.T, db *sql.DB, table string) { SELECT count(*) FROM information_schema.tables WHERE table_schema = current_schema() AND table_name = $1 `, table).Scan(&count)) - require.Equalf(t, 1, count, "table %s does not exist; migration 034 has not been written", table) + require.Equalf(t, 1, count, "table %s does not exist; the migration that creates it has not been written", table) } // primaryKeyColumns returns the table's primary key columns in key order. diff --git a/internal/db/postgres/submission_ledger_repo.go b/internal/db/postgres/submission_ledger_repo.go new file mode 100644 index 0000000..648d5dd --- /dev/null +++ b/internal/db/postgres/submission_ledger_repo.go @@ -0,0 +1,39 @@ +package postgres + +import ( + "context" + "database/sql" + "time" + + "Coves/internal/core/posts" +) + +// PostgreSQL storage for the post_submissions ledger (migration 035) — the +// rows that both deduplicate submissions and meter the per-author quota of +// docs/PRD_AUTHOR_OWNED_POSTS.md §8. +// +// It mirrors the aggregator limiter of migration 012: a row per accepted +// submission, a rolling-window COUNT over an index that leads with the pair +// being metered. The difference is that Reserve is written BEFORE the PDS +// write rather than after it, because here the insert is also the dedupe gate +// — see posts.SubmissionLedger for why that ordering is the safe one. +type submissionLedger struct { + db *sql.DB +} + +// NewSubmissionLedger creates the post_submissions repository. +func NewSubmissionLedger(db *sql.DB) posts.SubmissionLedger { + return &submissionLedger{db: db} +} + +func (l *submissionLedger) Reserve(ctx context.Context, cmd posts.ReserveSubmissionCommand) (posts.SubmissionReservation, error) { + return posts.SubmissionReservation{}, nil +} + +func (l *submissionLedger) Release(ctx context.Context, reservation posts.SubmissionReservation) error { + return nil +} + +func (l *submissionLedger) CountSince(ctx context.Context, authorDID, communityDID string, since time.Time) (int, error) { + return 0, nil +} diff --git a/internal/db/postgres/submission_ledger_schema_test.go b/internal/db/postgres/submission_ledger_schema_test.go new file mode 100644 index 0000000..1f1038d --- /dev/null +++ b/internal/db/postgres/submission_ledger_schema_test.go @@ -0,0 +1,270 @@ +//go:build integration + +package postgres + +import ( + "context" + "database/sql" + "fmt" + "strings" + "testing" + + "Coves/tests/fixtures" + "Coves/tests/testkit" + + "github.com/lib/pq" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Migration 035's shape, read out of the catalog rather than out of the .sql +// file — the same distinction admission_repo_schema_test.go draws for 034: a +// grep proves the text was written, these prove the database ended up in the +// state the text was supposed to produce. +// +// Three things here are load-bearing and silent when wrong: +// +// - THE UNIQUE CONSTRAINT IS THE DEDUPE GATE. admitPost does not SELECT and +// then INSERT; it INSERTs and reads the unique violation as the answer, +// because the database is the only participant two racing double-taps both +// talk to. Without the constraint, dedupe still "works" under every +// sequential test and silently stops working under concurrency. +// - NO FOREIGN KEYS. Migration 034 dropped posts.fk_author precisely because +// a federated author has no users row (§5.3); a ledger that referenced one +// would make the very submissions 034 exists to admit unrecordable, and the +// refusal would arrive as a dead letter rather than as a decision. +// - THE RATE-LIMIT INDEX. The quota is a COUNT over (author, community) in a +// rolling window, run on the write path of every post. Migration 012 built +// idx_aggregator_posts_rate_limit for the identical query shape; without +// the equivalent here the count degrades to a scan of every submission the +// instance has ever accepted. + +const submissionsTable = "post_submissions" + +func TestSubmissionsTable_Columns(t *testing.T) { + t.Parallel() + + db := testkit.DB(t) + ctx := context.Background() + + requireTableExists(t, db, submissionsTable) + + type columnShape struct { + dataType string + nullable bool + } + want := map[string]columnShape{ + // The surrogate key exists so a reservation can be RELEASED by identity. + // Releasing by the natural key would work too, right up until the row + // being released is not the one this request inserted. + "id": {"bigint", false}, + "author_did": {"text", false}, + "community_did": {"text", false}, + + // The hash of the canonical record minus createdAt. Text rather than + // bytea so it is greppable in an incident and comparable in psql; the + // column is never interpreted, only equated. + "fingerprint": {"text", false}, + + // The window index the dedupe key is scoped to, derived from the + // application's injected clock. It is an integer rather than a + // timestamp deliberately: a timestamp here invites comparison against + // created_at, and the two come from different clocks — one the app's, + // one the database's. + "dedupe_bucket": {"bigint", false}, + + // Server-stamped, because it is what the rolling window is measured + // against and a client-supplied time would be a client-supplied quota. + "created_at": {"timestamp with time zone", false}, + } + + rows, err := db.QueryContext(ctx, ` + SELECT column_name, data_type, is_nullable, coalesce(column_default, '') + FROM information_schema.columns + WHERE table_schema = current_schema() AND table_name = $1 + `, submissionsTable) + require.NoError(t, err) + defer func() { _ = rows.Close() }() + + got := map[string]columnShape{} + defaults := map[string]string{} + for rows.Next() { + var name, dataType, isNullable, columnDefault string + require.NoError(t, rows.Scan(&name, &dataType, &isNullable, &columnDefault)) + got[name] = columnShape{dataType: dataType, nullable: isNullable == "YES"} + defaults[name] = columnDefault + } + require.NoError(t, rows.Err()) + + for name, wantShape := range want { + gotShape, ok := got[name] + if !assert.Truef(t, ok, "%s.%s is missing", submissionsTable, name) { + continue + } + assert.Equalf(t, wantShape.dataType, gotShape.dataType, "%s.%s type", submissionsTable, name) + assert.Equalf(t, wantShape.nullable, gotShape.nullable, "%s.%s nullability", submissionsTable, name) + } + + assert.Containsf(t, strings.ToLower(defaults["created_at"]), "now()", + "created_at must be stamped by the server: the rolling window is measured against it, so a caller that could set it could set its own quota") + + assert.Equal(t, []string{"id"}, primaryKeyColumns(t, db, submissionsTable), + "the primary key is the surrogate id, so a reservation can be released by identity") +} + +// The dedupe gate itself. Both halves are asserted — that the constraint exists +// over exactly the right columns, and that Postgres actually refuses the second +// insert — because a constraint over the wrong column set is present in the +// catalog and useless in practice. +func TestSubmissionsTable_DedupeKeyIsUniqueAndEnforced(t *testing.T) { + t.Parallel() + + db := testkit.DB(t) + requireTableExists(t, db, submissionsTable) + + wantKey := []string{"author_did", "community_did", "fingerprint", "dedupe_bucket"} + + t.Run("the constraint exists over the dedupe key", func(t *testing.T) { + found := uniqueKeyColumnSets(t, db, submissionsTable) + var matched bool + for _, columns := range found { + if assert.ObjectsAreEqual(wantKey, columns) { + matched = true + } + } + assert.Truef(t, matched, + "no UNIQUE key over %v; the INSERT is the dedupe gate, so without it two concurrent identical submissions both succeed. Unique keys found: %v", + wantKey, found) + }) + + t.Run("a repeat inside the same bucket is refused", func(t *testing.T) { + author, community := newSubmissionSubject(t) + + require.NoError(t, insertSubmission(t, db, author, community, "fp-repeat", 100)) + assert.Error(t, insertSubmission(t, db, author, community, "fp-repeat", 100), + "an identical submission in the same dedupe window must be refused by the database, not merely by a prior SELECT") + }) + + t.Run("the same content in a later bucket is a repost, not a duplicate", func(t *testing.T) { + author, community := newSubmissionSubject(t) + + require.NoError(t, insertSubmission(t, db, author, community, "fp-later", 100)) + assert.NoError(t, insertSubmission(t, db, author, community, "fp-later", 101), + "the bucket is what makes dedupe expire; without it an author could never repost the same content again") + }) + + t.Run("the key is scoped per author and per community", func(t *testing.T) { + author, community := newSubmissionSubject(t) + otherAuthor, otherCommunity := newSubmissionSubject(t) + + require.NoError(t, insertSubmission(t, db, author, community, "fp-scope", 100)) + assert.NoError(t, insertSubmission(t, db, otherAuthor, community, "fp-scope", 100), + "two authors posting the same link are not duplicates of each other") + assert.NoError(t, insertSubmission(t, db, author, otherCommunity, "fp-scope", 100), + "cross-posting the same content to a second community is not a duplicate") + }) +} + +func TestSubmissionsTable_HasNoForeignKeys(t *testing.T) { + t.Parallel() + + db := testkit.DB(t) + ctx := context.Background() + requireTableExists(t, db, submissionsTable) + + rows, err := db.QueryContext(ctx, ` + SELECT conname, pg_get_constraintdef(oid) FROM pg_constraint + WHERE conrelid = $1::regclass AND contype = 'f' + `, submissionsTable) + require.NoError(t, err) + defer func() { _ = rows.Close() }() + + for rows.Next() { + var name, definition string + require.NoError(t, rows.Scan(&name, &definition)) + assert.Failf(t, "the submission ledger has a foreign key", + "constraint %s: %s — migration 034 dropped posts.fk_author because a federated author has no users row, "+ + "and a community may be one this AppView has not indexed; an FK here turns an ordinary submission into an insert failure", + name, definition) + } + require.NoError(t, rows.Err()) + + // And the behaviour that follows from it: a DID this instance has never + // heard of can still be metered. + unknownAuthor := fixtures.DID(testkit.UniqueID(t)) + unknownCommunity := fixtures.DID(testkit.UniqueID(t)) + assert.NoError(t, insertSubmission(t, db, unknownAuthor, unknownCommunity, "fp-federated", 100), + "a submission from an author with no users row must record; that author is exactly who §5.3 exists for") +} + +func TestSubmissionsTable_RateLimitIndex(t *testing.T) { + t.Parallel() + + db := testkit.DB(t) + requireTableExists(t, db, submissionsTable) + + definitions := indexDefinitions(t, db, submissionsTable) + + var matched string + for name, definition := range definitions { + if indexColumns(definition) == "author_did, community_did, created_at" { + matched = name + } + } + assert.NotEmptyf(t, matched, + "no index on (author_did, community_did, created_at); that is the rolling-window quota query, run on the write path of every post — migration 012 built idx_aggregator_posts_rate_limit for the identical shape. Indexes found: %v", + definitions) +} + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +// newSubmissionSubject returns an author and a community that no other subtest +// shares, so subtests of one parallel test cannot collide on the dedupe key. +// Neither needs to exist anywhere else — that is the point of the no-FK rule. +func newSubmissionSubject(t *testing.T) (authorDID, communityDID string) { + t.Helper() + return fixtures.DID(testkit.UniqueID(t)), fixtures.DID(testkit.UniqueID(t)) +} + +func insertSubmission(t *testing.T, db *sql.DB, authorDID, communityDID, fingerprint string, bucket int64) error { + t.Helper() + + _, err := db.ExecContext(context.Background(), fmt.Sprintf(` + INSERT INTO %s (author_did, community_did, fingerprint, dedupe_bucket) + VALUES ($1, $2, $3, $4) + `, submissionsTable), authorDID, communityDID, fingerprint, bucket) + return err +} + +// uniqueKeyColumnSets returns the column sets covered by a UNIQUE constraint or +// a unique index, in key order. +// +// Both spellings are accepted because both enforce the same thing and Postgres +// reports them differently: a table constraint appears in pg_constraint, while +// a bare CREATE UNIQUE INDEX appears only in pg_index. Insisting on one would +// fail a migration that closed the race perfectly well the other way. +func uniqueKeyColumnSets(t *testing.T, db *sql.DB, table string) [][]string { + t.Helper() + + rows, err := db.QueryContext(context.Background(), ` + SELECT array_agg(a.attname ORDER BY k.ord) + FROM pg_index i + JOIN unnest(i.indkey) WITH ORDINALITY AS k(attnum, ord) ON true + JOIN pg_attribute a ON a.attrelid = i.indrelid AND a.attnum = k.attnum + WHERE i.indrelid = $1::regclass AND i.indisunique + GROUP BY i.indexrelid + `, table) + require.NoError(t, err) + defer func() { _ = rows.Close() }() + + var sets [][]string + for rows.Next() { + var columns []string + require.NoError(t, rows.Scan(pq.Array(&columns))) + sets = append(sets, columns) + } + require.NoError(t, rows.Err()) + return sets +} -- 2.51.2 From e210f5a248a0d56dd6fb5ce765ffa2bfb7278e9e Mon Sep 17 00:00:00 2001 From: Bretton Date: Fri, 7 Aug 2026 19:02:45 -0700 Subject: [PATCH 2/3] =?UTF-8?q?feat(posts):=20GREEN=20=E2=80=94=20admitPos?= =?UTF-8?q?t=20policy,=20submission=20ledger,=20wiring=20(task=203)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migration 035 post_submissions (UNIQUE dedupe gate, no FKs, rate index); pure AdmissionPolicy over injected lookups + clock; ban enforcement via memberships.is_banned (fail-closed lookup); rolling per-author/community quota + fingerprint dedupe with reserve-then- confirm around the PDS write; config-validated limits; 409 DuplicateSubmission mapping. BANS ARE NOW ENFORCED on the write path (intended policy change, PRD §4.1 rev 2.5). Co-Authored-By: Claude Fable 5 --- .env.dev.example | 28 ++ .env.prod.example | 28 ++ cmd/server/wiring.go | 16 + internal/api/handlers/post/errors.go | 9 + internal/config/config.go | 64 ++++ internal/core/posts/admit.go | 298 +++++++++++++++++- internal/core/posts/service.go | 244 ++++++++------ .../035_create_post_submissions.sql | 80 +++++ .../db/postgres/submission_ledger_repo.go | 64 +++- 9 files changed, 712 insertions(+), 119 deletions(-) create mode 100644 internal/db/migrations/035_create_post_submissions.sql diff --git a/.env.dev.example b/.env.dev.example index 590b64f..d36418a 100644 --- a/.env.dev.example +++ b/.env.dev.example @@ -173,3 +173,31 @@ OTEL_ENABLED=false # TELEGRAM_ALERT_REASONS= # Values above 10 are capped by the AppView's own 10s backstop. # TELEGRAM_TIMEOUT_SECONDS=5 + +# ============================================================================= +# Optional: Post submission limits +# ============================================================================= +# Anyone can write unlimited records naming any community, so the limits that +# matter are enforced when a submission is ADMITTED rather than when a record is +# written (docs/PRD_AUTHOR_OWNED_POSTS.md section 8). Both quotas are scoped to +# one (author, community) pair: being at the limit in a busy community must not +# silence an author everywhere on the instance. +# +# All three have working defaults, and the server REFUSES TO START if any of +# them is set to zero or a negative value — "unset" must never be readable as +# "unlimited", and a zero limit does not relax the rule, it inverts it. +# +# How many posts one author may have admitted to one community inside +# POST_SUBMISSIONS_WINDOW. Counted over a ROLLING window, so slots come back +# individually rather than all at once on the hour (default: 10). +# POST_SUBMISSIONS_MAX_PER_COMMUNITY=10 +# +# The width of that rolling window (default: 1h). +# POST_SUBMISSIONS_WINDOW=1h +# +# How long an identical resubmission is refused as a repeat. Separate from the +# window above because the two answer different questions: one bounds volume, +# the other recognises the retry a client sends after a lost response. Raising +# it makes reposting the same content take longer to become possible again +# (default: 1h). +# POST_SUBMISSIONS_DEDUPE_WINDOW=1h diff --git a/.env.prod.example b/.env.prod.example index 4dd0c67..18b97d5 100644 --- a/.env.prod.example +++ b/.env.prod.example @@ -368,6 +368,34 @@ OTEL_ENABLED=false # own 10s backstop around the whole alert. # TELEGRAM_TIMEOUT_SECONDS=5 +# ============================================================================= +# Optional: Post submission limits +# ============================================================================= +# Anyone can write unlimited records naming any community, so the limits that +# matter are enforced when a submission is ADMITTED rather than when a record is +# written (docs/PRD_AUTHOR_OWNED_POSTS.md section 8). Both quotas are scoped to +# one (author, community) pair: being at the limit in a busy community must not +# silence an author everywhere on the instance. +# +# All three have working defaults, and the server REFUSES TO START if any of +# them is set to zero or a negative value — "unset" must never be readable as +# "unlimited", and a zero limit does not relax the rule, it inverts it. +# +# How many posts one author may have admitted to one community inside +# POST_SUBMISSIONS_WINDOW. Counted over a ROLLING window, so slots come back +# individually rather than all at once on the hour (default: 10). +# POST_SUBMISSIONS_MAX_PER_COMMUNITY=10 +# +# The width of that rolling window (default: 1h). +# POST_SUBMISSIONS_WINDOW=1h +# +# How long an identical resubmission is refused as a repeat. Separate from the +# window above because the two answer different questions: one bounds volume, +# the other recognises the retry a client sends after a lost response. Raising +# it makes reposting the same content take longer to become possible again +# (default: 1h). +# POST_SUBMISSIONS_DEDUPE_WINDOW=1h + # ============================================================================= # Optional: Versioning # ============================================================================= diff --git a/cmd/server/wiring.go b/cmd/server/wiring.go index 2f704d7..d57e8eb 100644 --- a/cmd/server/wiring.go +++ b/cmd/server/wiring.go @@ -326,10 +326,26 @@ func (a *application) buildServices(ctx context.Context) error { // userBlockRepo backs viewer block enforcement on GetPosts, keeping // permalink and cold-load reads consistent with feed/timeline filtering. + // The admission policy is what makes the ban lookup and the per-author + // submission quota live (PRD_AUTHOR_OWNED_POSTS §4.1, §8). A post service + // built without it enforces only what CreatePost enforced before those + // existed, so this is not an optional enrichment — it is the enforcement. + // The limits come from config, which refuses to start the process with a + // non-positive one rather than letting an omission read as "unlimited". a.postService = posts.NewPostService( a.postRepo, a.communityService, a.aggregatorService, blobService, unfurlService, a.blueskyService, a.cfg.PDS.URL, posts.WithBlockChecker(a.userBlockRepo), + posts.WithAdmissionPolicy(posts.AdmissionPolicy{ + Ledger: postgresRepo.NewSubmissionLedger(a.db), + Bans: a.communityService, + Limits: posts.SubmissionLimits{ + MaxPerAuthorPerCommunity: a.cfg.Submissions.MaxPerAuthorPerCommunity, + Window: a.cfg.Submissions.Window, + DedupeWindow: a.cfg.Submissions.DedupeWindow, + }, + Now: time.Now, + }), ) // Subject existence is deliberately not validated: the vote is written to diff --git a/internal/api/handlers/post/errors.go b/internal/api/handlers/post/errors.go index 57bb45c..aafd53e 100644 --- a/internal/api/handlers/post/errors.go +++ b/internal/api/handlers/post/errors.go @@ -32,6 +32,15 @@ var errorMapper = xrpc.NewMapper("post", xrpc.Sentinel(posts.ErrNotFound, http.StatusNotFound, "NotFound", "Post not found"), + // A submission refused at the admission gate, which is NOT the generic + // AlreadyExists that a storage conflict produces: 409 DuplicateSubmission + // tells a client whose response was lost that its post already exists and + // it should stop resending, where a 429 would have it retry on a timer + // forever. Ahead of the shared ConflictError rule, which answers with the + // less specific code. + xrpc.Sentinel(posts.ErrDuplicateSubmission, http.StatusConflict, + "DuplicateSubmission", "You have already submitted this post to this community"), + xrpc.Match(aggregators.IsUnauthorized, http.StatusForbidden, "NotAuthorized", "Aggregator not authorized to post in this community"), xrpc.Match(aggregators.IsRateLimited, http.StatusTooManyRequests, diff --git a/internal/config/config.go b/internal/config/config.go index a816ee1..1ba1f98 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -393,6 +393,9 @@ func Load() (*Config, error) { if err := cfg.loadJetstream(); err != nil { return nil, err } + if err := cfg.loadSubmissions(); err != nil { + return nil, err + } cfg.PDS = PDSConfig{ URL: stringVar("PDS_URL", "http://localhost:3001"), @@ -627,6 +630,41 @@ func (c *Config) loadJetstream() error { return nil } +// Default submission quotas. They apply in every environment, dev and +// production alike, because the alternative — requiring the variables in +// production — makes an omission fail closed on the wrong side: the process +// refuses to boot over an abuse limit rather than running with a conservative +// one. The bounds are deliberately generous enough that no ordinary author +// meets them and tight enough to make scripted flooding expensive, and every +// instance can move them. +const ( + defaultMaxSubmissionsPerCommunity = 10 + defaultSubmissionWindow = time.Hour + defaultSubmissionDedupeWindow = time.Hour +) + +func (c *Config) loadSubmissions() error { + maxPerCommunity, err := intVar("POST_SUBMISSIONS_MAX_PER_COMMUNITY", defaultMaxSubmissionsPerCommunity) + if err != nil { + return err + } + window, err := durationVar("POST_SUBMISSIONS_WINDOW", defaultSubmissionWindow) + if err != nil { + return err + } + dedupeWindow, err := durationVar("POST_SUBMISSIONS_DEDUPE_WINDOW", defaultSubmissionDedupeWindow) + if err != nil { + return err + } + + c.Submissions = SubmissionsConfig{ + MaxPerAuthorPerCommunity: maxPerCommunity, + Window: window, + DedupeWindow: dedupeWindow, + } + return nil +} + // Validate enforces the constraints that Load's defaults cannot express, // notably the ones that differ between dev and production. It returns every // problem at once so a misconfigured deployment can be fixed in a single pass @@ -708,6 +746,32 @@ func (c *Config) Validate() error { "INSTANCE_DID must be a DID (got %q)", c.Instance.DID)) } + // The submission quotas, in every environment. §8's limits exist because + // anyone can write unlimited records naming any community, so "unset" must + // not be readable as "unlimited" — and a zero is worse than unlimited: a + // limit check written as `count > limit` refuses every post, one written the + // other way admits every post, and either way the behaviour was decided by + // an omission. Load supplies defaults, so reaching any of these means an + // operator set the variable to something that is not a quota. + if c.Submissions.MaxPerAuthorPerCommunity <= 0 { + problems = append(problems, fmt.Sprintf( + "POST_SUBMISSIONS_MAX_PER_COMMUNITY must be greater than 0 (got %d); "+ + "a non-positive per-author quota disables or inverts the abuse limit rather than relaxing it", + c.Submissions.MaxPerAuthorPerCommunity)) + } + if c.Submissions.Window <= 0 { + problems = append(problems, fmt.Sprintf( + "POST_SUBMISSIONS_WINDOW must be greater than 0 (got %s); "+ + "the quota is counted over a rolling window, and a zero-width one counts nothing", + c.Submissions.Window)) + } + if c.Submissions.DedupeWindow <= 0 { + problems = append(problems, fmt.Sprintf( + "POST_SUBMISSIONS_DEDUPE_WINDOW must be greater than 0 (got %s); "+ + "it scopes the ledger's uniqueness bucket, and without a width every repost collides with the original forever", + c.Submissions.DedupeWindow)) + } + if !c.IsDevEnv { switch { case c.OAuth.SealSecret == "": diff --git a/internal/core/posts/admit.go b/internal/core/posts/admit.go index 23a0b41..b03789c 100644 --- a/internal/core/posts/admit.go +++ b/internal/core/posts/admit.go @@ -2,6 +2,12 @@ package posts import ( "context" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "log" "time" "Coves/internal/core/communities" @@ -101,20 +107,34 @@ type AdmissionDecision struct { // Cause carries the underlying error behind a refusal, when there is one, // so the caller can wrap it and keep it matchable. // - // It exists for exactly one case today: aggregator authorization. The API - // boundary maps that refusal through aggregators.IsUnauthorized and - // aggregators.IsRateLimited (internal/api/handlers/post/errors.go), which - // are predicates over the AGGREGATORS package's sentinels. Collapsing that - // error into a bare DecisionCode would turn a 403 "stop asking" and a 429 - // "ask later" into the same answer, and a well-behaved aggregator would - // retry a permanent refusal forever. + // The refusal case is aggregator authorization. The API boundary maps that + // refusal through aggregators.IsUnauthorized and aggregators.IsRateLimited + // (internal/api/handlers/post/errors.go), which are predicates over the + // AGGREGATORS package's sentinels. Collapsing that error into a bare + // DecisionCode would turn a 403 "stop asking" and a 429 "ask later" into + // the same answer, and a well-behaved aggregator would retry a permanent + // refusal forever. + // + // It is ALSO set on the decision returned alongside a non-nil error, which + // is what keeps a decision that could not be made from reading as an + // admission — see Admitted. Cause error } -// Admitted reports whether the submission may proceed. There is no separate -// bool field: two representations of one fact drift, and the code is the one -// that has to be right. -func (d AdmissionDecision) Admitted() bool { return d.Code == "" } +// Admitted reports whether the submission may proceed. +// +// There are three states here, not two: admitted, refused with a code, and +// NOT DECIDED — a lookup failed and nothing was concluded either way. The third +// is why this is not simply `Code == ""`. An infrastructure failure must not be +// dressed up as a policy code (the client would be told to stop retrying +// something that will work in a second), so those returns carry an empty Code; +// a bare code test would then read the zero value as an admission, which is the +// most dangerous default available on a security decision. A decision is an +// admission only when there is neither a refusal code nor a cause. +// +// There is still no separate `admitted` bool: two representations of one fact +// drift, and the code is the one that has to be right. +func (d AdmissionDecision) Admitted() bool { return d.Code == "" && d.Cause == nil } // SubmissionReservation identifies the ledger row admitPost inserted for a // submission, so a caller whose subsequent PDS write failed can release it. @@ -245,6 +265,114 @@ func WithAdmissionPolicy(policy AdmissionPolicy) PostServiceOption { return func(s *postService) { s.admission = &policy } } +// completeAdmissionPolicy fills in the collaborators a policy did not name, so +// that CreatePost has exactly ONE decision path to run. +// +// The alternative — branching on whether a policy was supplied, and keeping the +// pre-policy checks inline for the other branch — would leave two copies of the +// community/visibility/authorization sequence, and §4.1 of the PRD exists +// because the one copy we had already drifted from what its docstring claimed. +// +// The substitutes are named for what they are. A service constructed without a +// policy enforces exactly what CreatePost enforced before this decision existed: +// community existence, private visibility, and aggregator authorization. It is +// the shape every test fixture that predates §8 uses, and cmd/server always +// supplies the real policy — which is what makes the ban lookup and the quota +// live in production. +func completeAdmissionPolicy(policy *AdmissionPolicy) *AdmissionPolicy { + complete := AdmissionPolicy{} + if policy != nil { + complete = *policy + } + if complete.Ledger == nil { + complete.Ledger = unmeteredLedger{} + } + if complete.Bans == nil { + complete.Bans = unenforcedBans{} + } + if complete.Now == nil { + complete.Now = time.Now + } + return &complete +} + +// unmeteredLedger stands in when no submission ledger was wired: it reserves +// nothing, so neither dedupe nor the per-author quota applies. +// +// It cannot silently disable a configured limiter — it is only ever reachable +// when AdmissionPolicy.Ledger is nil, which cmd/server never leaves so. +type unmeteredLedger struct{} + +func (unmeteredLedger) Reserve(context.Context, ReserveSubmissionCommand) (SubmissionReservation, error) { + return SubmissionReservation{}, nil +} + +func (unmeteredLedger) Release(context.Context, SubmissionReservation) error { return nil } + +// CountSince answers zero, which admits: with no ledger there is nothing to +// count, and refusing on an absent substrate would take a service that never +// asked for a quota and stop it posting at all. +func (unmeteredLedger) CountSince(context.Context, string, string, time.Time) (int, error) { + return 0, nil +} + +// unenforcedBans stands in when no ban lookup was wired, answering the way an +// author with no membership row does. It returns the sentinel rather than a nil +// membership so that it travels the same branch a real absent row does — the +// "no membership means not banned" translation stays in one place. +type unenforcedBans struct{} + +func (unenforcedBans) GetMembership(context.Context, string, string) (*communities.Membership, error) { + return nil, communities.ErrMembershipNotFound +} + +// admissionDeps assembles the decision's inputs from the service's +// collaborators. s.admission is never nil — NewPostService completes it — so +// this cannot silently hand admitPost a missing ledger or clock. +func (s *postService) admissionDeps() admissionDeps { + return admissionDeps{ + communities: s.communityService, + bans: s.admission.Bans, + aggregators: s.aggregatorService, + ledger: s.admission.Ledger, + limits: s.admission.Limits, + now: s.admission.Now, + } +} + +// refusalError translates a refusal into the sentinel the API boundary maps. +// +// The codes and the sentinels are separate vocabularies on purpose: a code is +// what the admissions table stores and what a federated peer is told, while a +// sentinel is what internal/api/handlers/post turns into a status. This is the +// one place they meet. +func refusalError(decision AdmissionDecision) error { + switch decision.Code { + case DecisionCommunityNotFound: + return ErrCommunityNotFound + case DecisionCommunityPrivate: + // Unchanged from the pre-§8 behaviour: a private community answers the + // same 403 to a member-less user it always did. + return ErrNotAuthorized + case DecisionAuthorBanned: + return ErrBanned + case DecisionAggregatorNotAuthorized: + // The aggregators-package sentinel has to survive: the boundary tells a + // permanent 403 from a retryable 429 by matching on it, and the wording + // is the one CreatePost has always used. + return fmt.Errorf("aggregator not authorized: %w", decision.Cause) + case DecisionDuplicateSubmission: + return ErrDuplicateSubmission + case DecisionRateLimitExceeded: + return ErrRateLimitExceeded + default: + // A code minted without a mapping. Answering with a bare 500 would be + // wrong twice over — the submission WAS refused, and the operator would + // have nothing to search for — so the code itself goes in the error. + return fmt.Errorf("submission refused: %s", decision.Code) + } +} + // admissionDeps is everything admitPost reads, gathered so the decision is a // function of its arguments rather than of a service's field set. type admissionDeps struct { @@ -307,14 +435,140 @@ type admissionDeps struct { // A non-nil error means the decision could NOT be made — a lookup failed — and // is distinct from a refusal, which is a decision. func admitPost(ctx context.Context, deps admissionDeps, req AdmissionRequest) (AdmissionDecision, error) { - return AdmissionDecision{}, nil + // 1. Community resolution. Two lookups — the at-identifier to a DID, then + // the DID to the indexed row — and either failing to find it is the same + // answer to the client. + communityDID, err := deps.communities.ResolveCommunityIdentifier(ctx, req.Community) + if err != nil { + switch { + case errors.Is(err, communities.ErrCommunityNotFound): + return AdmissionDecision{Code: DecisionCommunityNotFound}, nil + case communities.IsValidationError(err): + // A malformed identifier is the client's mistake and has to reach + // the boundary as one: a validation error becomes a 400 naming the + // bad field, while an unclassified error becomes an opaque 500. + return undecided(NewValidationError("community", err.Error())) + default: + return undecided(fmt.Errorf("failed to resolve community identifier: %w", err)) + } + } + + community, err := deps.communities.GetByDID(ctx, communityDID) + if err != nil { + if errors.Is(err, communities.ErrCommunityNotFound) { + return AdmissionDecision{Code: DecisionCommunityNotFound}, nil + } + return undecided(fmt.Errorf("failed to fetch community: %w", err)) + } + + if req.Actor == ActorUser { + // 2. The privacy wall, and it stands ahead of the ban lookup rather + // than beside it: moderation state must not be read at all for a + // community the submitter cannot see. See the check-order note above. + if community.Visibility == "private" { + return AdmissionDecision{Code: DecisionCommunityPrivate}, nil + } + + // 3. The ban. Looked up against the RESOLVED DID — handles are mutable, + // and a ban keyed to one stops applying the moment a community renames + // itself. + membership, err := deps.bans.GetMembership(ctx, req.AuthorDID, community.DID) + switch { + case errors.Is(err, communities.ErrMembershipNotFound): + // A VALUE meaning "not banned". Posting in a public community has + // never required joining it, so an absent row is the common case. + case err != nil: + // Fail closed. Treating an unreachable database as "not banned" + // would turn a Postgres blip into a global unban for its duration. + return undecided(fmt.Errorf("failed to look up community membership: %w", err)) + case membership != nil && membership.IsBanned: + return AdmissionDecision{Code: DecisionAuthorBanned}, nil + } + } + + // 4. Aggregator authorization, which carries the aggregators-package + // sentinel that caused it: the boundary tells 403 from 429 by matching on + // it, and a bare code would have a well-behaved aggregator retry a + // permanent refusal forever. + if req.Actor == ActorRegisteredAggregator { + if err := deps.aggregators.ValidateAggregatorPost(ctx, req.AuthorDID, community.DID); err != nil { + return AdmissionDecision{Code: DecisionAggregatorNotAuthorized, Cause: err}, nil + } + } + + // 5. Dedupe, for every actor class. The INSERT is the check: a unique + // violation means an identical submission is already on the ledger for this + // window. It runs ahead of the quota so that a client retrying after a lost + // response is told its post already exists rather than told to slow down. + now := deps.now() + reservation, err := deps.ledger.Reserve(ctx, ReserveSubmissionCommand{ + AuthorDID: req.AuthorDID, + CommunityDID: community.DID, + Fingerprint: req.Fingerprint, + DedupeBucket: dedupeBucket(now, deps.limits.DedupeWindow), + }) + if err != nil { + if errors.Is(err, ErrDuplicateSubmission) { + return AdmissionDecision{Code: DecisionDuplicateSubmission}, nil + } + return undecided(fmt.Errorf("failed to reserve submission: %w", err)) + } + + // 6. The rolling-window quota, for regular users only. Aggregators are + // metered by their own limiter (step 4) or, when trusted, not at all. + // + // The reservation is already on the ledger, so it is counted here — the + // limit is reached when the count EXCEEDS it — and every path out of this + // block that is not an admission has to hand the slot back. + if req.Actor == ActorUser { + count, err := deps.ledger.CountSince(ctx, req.AuthorDID, community.DID, now.Add(-deps.limits.Window)) + if err != nil { + releaseReservation(ctx, deps.ledger, reservation) + return undecided(fmt.Errorf("failed to count recent submissions: %w", err)) + } + if count > deps.limits.MaxPerAuthorPerCommunity { + releaseReservation(ctx, deps.ledger, reservation) + return AdmissionDecision{Code: DecisionRateLimitExceeded}, nil + } + } + + return AdmissionDecision{Community: community, Reservation: &reservation}, nil +} + +// undecided reports that the decision could NOT be made. +// +// The error is returned twice — as the error, and on the decision's Cause — and +// the second copy is the load-bearing one: it is what makes Admitted() false +// for a caller that inspects the value. Leaving the decision zero would have a +// caller who checked the decision before the error read a database outage as +// permission to post. +func undecided(err error) (AdmissionDecision, error) { + return AdmissionDecision{Cause: err}, err +} + +// releaseReservation gives back a slot the decision took and then declined to +// use. The error is logged rather than returned: every caller reaches this +// while already reporting a refusal or a failure, and replacing that answer +// with a second one would hide the reason the submission was actually stopped. +func releaseReservation(ctx context.Context, ledger SubmissionLedger, reservation SubmissionReservation) { + if err := ledger.Release(ctx, reservation); err != nil { + log.Printf("[POST-ADMIT] Warning: failed to release submission reservation %d: %v", reservation.ID, err) + } } // dedupeBucket is the index of the window `now` falls in, so that two // submissions in the same window collide on the ledger's unique key and two // submissions a window apart do not. func dedupeBucket(now time.Time, window time.Duration) int64 { - return 0 + // A non-positive window would divide by zero. config.Validate refuses to + // start a process with one, so reaching this is a wiring bug rather than an + // operator mistake; collapsing to a single bucket keeps it from panicking + // on the write path, and the constant bucket makes the misconfiguration + // loud (every repost is refused) rather than silent. + if window <= 0 { + return 0 + } + return now.UnixNano() / int64(window) } // submissionFingerprint hashes what a moderator would judge about a record: @@ -325,5 +579,21 @@ func dedupeBucket(now time.Time, window time.Duration) int64 { // after a lost response, which is the case dedupe exists to catch. A // fingerprint that included it would never match anything. func submissionFingerprint(record PostRecord) string { - return "" + // The record is taken by value, so clearing the timestamp here cannot + // affect the record the caller goes on to write. + record.CreatedAt = "" + + canonical, err := json.Marshal(record) + if err != nil { + // Unreachable in practice: every field of a PostRecord either has a + // concrete marshalable type or holds a value decoded from JSON. Hashing + // a Go rendering instead of returning an empty string matters anyway — + // a constant fingerprint would collide every submission with every + // other, and the second post the instance ever received would be + // refused as a repeat of the first. + canonical = []byte(fmt.Sprintf("%#v", record)) + } + + sum := sha256.Sum256(canonical) + return hex.EncodeToString(sum[:]) } diff --git a/internal/core/posts/service.go b/internal/core/posts/service.go index 99d0870..20d6b76 100644 --- a/internal/core/posts/service.go +++ b/internal/core/posts/service.go @@ -74,19 +74,24 @@ func NewPostService( for _, opt := range opts { opt(s) } + s.admission = completeAdmissionPolicy(s.admission) return s } // CreatePost creates a new post in a community // Flow: -// 1. Validate input -// 2. Check if author is an aggregator (server-side validation using DID from JWT) -// 3. If aggregator: validate authorization and rate limits, skip membership checks -// 4. If user: resolve community and perform membership/ban validation -// 5. Build post record -// 6. Write to community's PDS repository -// 7. If aggregator: record post for rate limiting -// 8. Return URI/CID (AppView indexes asynchronously via Jetstream) +// 1. Validate input +// 2. Check if author is an aggregator (server-side validation using DID from JWT) +// 3. Admission: one decision over community existence, visibility, ban, +// aggregator authorization, dedupe and the per-author quota (admitPost) +// 4. Build post record +// 5. Write to community's PDS repository +// 6. If aggregator: record post for rate limiting +// 7. Return URI/CID (AppView indexes asynchronously via Jetstream) +// +// Admission runs BEFORE the token refresh, the blob uploads and the PDS write, +// so a refused submission costs a few lookups rather than an upload — and, +// more to the point, leaves no record in a community that refused it. func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*CreatePostResponse, error) { // 1. Validate basic input (before DID checks to give clear validation errors) if err := s.validateCreateRequest(&req); err != nil { @@ -96,8 +101,8 @@ func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*C // 1b. Normalize the fields the lexicon declares as `format: uri` before any // of them reach the record. Runs here, ahead of the community and PDS work, // so an unrecoverable URI fails fast without burning a DB lookup or an - // unfurl fetch. Mutates req in place; the record built in step 9 reads these - // same values, and the unfurl step below then works from the encoded URI, + // unfurl fetch. Mutates req in place; the record built below reads these + // same values, and the unfurl step then works from the encoded URI, // which dereferences identically. if err := normalizeEmbedURIs(req.Embed); err != nil { return nil, err @@ -140,85 +145,128 @@ func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*C // Check if this is a non-trusted aggregator (requires database lookup) var isOtherAggregator bool - var err error if !isTrustedAggregator && s.aggregatorService != nil { - isOtherAggregator, err = s.aggregatorService.IsAggregator(ctx, req.AuthorDID) + aggregator, err := s.aggregatorService.IsAggregator(ctx, req.AuthorDID) if err != nil { log.Printf("[POST-CREATE] Warning: failed to check if DID is aggregator: %v", err) // Don't fail the request - treat as regular user if check fails isOtherAggregator = false + } else { + isOtherAggregator = aggregator } } - // 4. Resolve community at-identifier (handle or DID) to DID - // This accepts both formats per atProto best practices: - // - Handles: !gardening.communities.coves.social - // - DIDs: did:plc:abc123 or did:web:coves.social - communityDID, err := s.communityService.ResolveCommunityIdentifier(ctx, req.Community) + // The classification the admission decision is made against. It is resolved + // HERE, at the call site, rather than inside admitPost: "who is trusted" + // comes out of the process environment, and a decision function that read it + // itself would hide the most consequential input to a security decision from + // the place that makes it. + actor := ActorUser + switch { + case isTrustedAggregator: + log.Printf("[POST-CREATE] Trusted aggregator detected: %s posting to community: %s", req.AuthorDID, req.Community) + actor = ActorTrustedAggregator + case isOtherAggregator: + actor = ActorRegisteredAggregator + } + + // 4. ADMISSION: the single decision over community existence, visibility, + // ban, aggregator authorization, dedupe and the per-author quota (§4.1, §8). + // + // The fingerprint is taken from the record as the CLIENT sent it, before + // unfurl enhancement rewrites the embed: two submissions of the same content + // must hash the same, and an enriched embed varies with whatever the remote + // page served at the time. + decision, err := admitPost(ctx, s.admissionDeps(), AdmissionRequest{ + Actor: actor, + AuthorDID: req.AuthorDID, + Community: req.Community, + Fingerprint: submissionFingerprint(postRecordFor(req, req.Community, "")), + }) if err != nil { - // Handle specific error types appropriately - if communities.IsNotFound(err) { - return nil, ErrCommunityNotFound - } - if communities.IsValidationError(err) { - // Pass through validation errors (invalid format, etc.) - return nil, NewValidationError("community", err.Error()) - } - // Infrastructure failures (DB errors, network issues) should be internal errors - // Don't leak internal details to client (e.g., "pq: connection refused") - return nil, fmt.Errorf("failed to resolve community identifier: %w", err) + return nil, err } - - // 5. AUTHORIZATION: For non-Kagi aggregators, validate authorization and rate limits - // Kagi is exempted from database checks via env var (temporary until XRPC endpoint is ready) - if isOtherAggregator && s.aggregatorService != nil { - if err := s.aggregatorService.ValidateAggregatorPost(ctx, req.AuthorDID, communityDID); err != nil { - log.Printf("[POST-CREATE] Aggregator authorization failed: %s -> %s: %v", req.AuthorDID, communityDID, err) - return nil, fmt.Errorf("aggregator not authorized: %w", err) - } - log.Printf("[POST-CREATE] Aggregator authorized: %s -> %s", req.AuthorDID, communityDID) + if !decision.Admitted() { + log.Printf("[POST-CREATE] Refused: author=%s, community=%s, actor=%s, code=%s", + req.AuthorDID, req.Community, actor, decision.Code) + return nil, refusalError(decision) } - // 6. Fetch community from AppView (includes all metadata) - community, err := s.communityService.GetByDID(ctx, communityDID) - if err != nil { - if communities.IsNotFound(err) { - return nil, ErrCommunityNotFound - } - return nil, fmt.Errorf("failed to fetch community: %w", err) - } - - // 7. Apply validation based on actor type (aggregator vs user) - if isTrustedAggregator { - // TRUSTED AGGREGATOR VALIDATION FLOW - // Trusted aggregators are authorized via TRUSTED_AGGREGATOR_DIDS env var (temporary) - // TODO: Replace with proper XRPC aggregator authorization endpoint - log.Printf("[POST-CREATE] Trusted aggregator detected: %s posting to community: %s", req.AuthorDID, communityDID) - // Aggregators skip membership checks and visibility restrictions - // They are authorized services, not community members - } else if isOtherAggregator { - // OTHER AGGREGATOR VALIDATION FLOW - // Authorization and rate limits already validated above via ValidateAggregatorPost - log.Printf("[POST-CREATE] Authorized aggregator detected: %s posting to community: %s", req.AuthorDID, communityDID) - } else { - // USER VALIDATION FLOW - // Check community visibility (Alpha: public/unlisted only) - // Beta will add membership checks for private communities - if community.Visibility == "private" { - return nil, ErrNotAuthorized + community := decision.Community + communityDID := community.DID + + // From here on the submission holds a ledger row. Every path that fails + // before the record exists has to give it back, or a transient failure + // permanently costs the author a quota slot AND blocks them from retrying + // the same content until the dedupe window rolls. + releaseOnFailure := func() { + if decision.Reservation != nil { + releaseReservation(ctx, s.admission.Ledger, *decision.Reservation) } } - // 8. Ensure community has fresh PDS credentials (token refresh if needed) + // 5. Ensure community has fresh PDS credentials (token refresh if needed) community, err = s.communityService.EnsureFreshToken(ctx, community) if err != nil { + releaseOnFailure() return nil, fmt.Errorf("failed to refresh community credentials: %w", err) } - // 9. Build post record for PDS - postRecord := PostRecord{ - Type: "social.coves.community.post", - Community: communityDID, + // 6. Build post record for PDS + postRecord := postRecordFor(req, communityDID, time.Now().UTC().Format(time.RFC3339)) + + // 7. Validate and enhance external embeds + if err := s.enhanceExternalEmbed(ctx, &postRecord, req, community, actor == ActorTrustedAggregator); err != nil { + releaseOnFailure() + return nil, err + } + + // 8. Write to community's PDS repository + // + // A failure here is the case the reservation was designed around: the row + // went in before the write precisely so two concurrent identical submissions + // would collide on the unique key, and the cost of that ordering is that a + // write which never happened owes the author their slot back. Without it, a + // PDS hiccup would consume a quota slot AND refuse the retry as a duplicate, + // turning a transient outage into a per-author lockout that outlives it. + uri, cid, err := s.createPostOnPDS(ctx, community, postRecord) + if err != nil { + releaseOnFailure() + return nil, fmt.Errorf("failed to write post to PDS: %w", err) + } + + // 9. Record aggregator post for rate limiting (non-Kagi aggregators only) + // Kagi is exempted from rate limiting via env var (temporary) + if isOtherAggregator && s.aggregatorService != nil { + if recordErr := s.aggregatorService.RecordAggregatorPost(ctx, req.AuthorDID, communityDID, uri, cid); recordErr != nil { + // Log but don't fail - post was already created successfully + log.Printf("[POST-CREATE] Warning: failed to record aggregator post for rate limiting: %v", recordErr) + } + } + + // 10. Return response (AppView will index via Jetstream consumer) + log.Printf("[POST-CREATE] Author: %s (trustedKagi=%v, otherAggregator=%v), Community: %s, URI: %s", + req.AuthorDID, isTrustedAggregator, isOtherAggregator, communityDID, uri) + + return &CreatePostResponse{ + URI: uri, + CID: cid, + }, nil +} + +// postRecordFor builds the record a request describes, stamped with the given +// community identifier and creation time. +// +// It is shared by the submission fingerprint and the record actually written, +// so that the thing dedupe hashes and the thing the community's repo receives +// cannot drift into describing different posts. The two callers differ in +// exactly the two arguments: the fingerprint is taken before the community +// identifier has been resolved and with no timestamp at all (createdAt is +// stamped per attempt, so including it would make every retry look new). +func postRecordFor(req CreatePostRequest, community, createdAt string) PostRecord { + return PostRecord{ + Type: postCollection, + Community: community, Author: req.AuthorDID, Title: req.Title, Content: req.Content, @@ -228,22 +276,34 @@ func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*C OriginalAuthor: req.OriginalAuthor, FederatedFrom: req.FederatedFrom, Location: req.Location, - CreatedAt: time.Now().UTC().Format(time.RFC3339), + CreatedAt: createdAt, } +} - // 10. Validate and enhance external embeds +// enhanceExternalEmbed applies the external-embed handling that has to happen +// against a live network: Bluesky URL conversion, client thumb validation, and +// unfurl enrichment with its blob uploads. +// +// It is a method rather than inline steps because every failure inside it now +// happens with a submission reservation already on the ledger, and a caller +// that has one error return to handle can give the reservation back in one +// place instead of at each of the four validation exits. +// +// trusted marks a trusted aggregator, which supplies its own metadata and is +// unfurled only for a thumbnail it did not provide. +func (s *postService) enhanceExternalEmbed(ctx context.Context, postRecord *PostRecord, req CreatePostRequest, community *communities.Community, trusted bool) error { if postRecord.Embed != nil { embedType, typeOk := postRecord.Embed["$type"].(string) if typeOk && embedType == "social.coves.embed.external" { if external, extOk := postRecord.Embed["external"].(map[string]interface{}); extOk { // Check if this is a Bluesky post URL and convert to post embed - if !s.tryConvertBlueskyURLToPostEmbed(ctx, external, &postRecord) { + if !s.tryConvertBlueskyURLToPostEmbed(ctx, external, postRecord) { // Not a Bluesky URL or conversion failed - continue with normal external embed processing // SECURITY: Validate thumb field (must be blob, not URL string) // This validation happens BEFORE unfurl to catch client errors early if existingThumb := external["thumb"]; existingThumb != nil { if thumbStr, isString := existingThumb.(string); isString { - return nil, NewValidationError("thumb", + return NewValidationError("thumb", fmt.Sprintf("thumb must be a blob reference (with $type, ref, mimeType, size), not URL string: %s", thumbStr)) } @@ -251,26 +311,26 @@ func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*C if thumbMap, isMap := existingThumb.(map[string]interface{}); isMap { // Check for $type field if thumbType, ok := thumbMap["$type"].(string); !ok || thumbType != "blob" { - return nil, NewValidationError("thumb", + return NewValidationError("thumb", fmt.Sprintf("thumb must have $type: blob (got: %v)", thumbType)) } // Check for required blob fields if _, hasRef := thumbMap["ref"]; !hasRef { - return nil, NewValidationError("thumb", "thumb blob missing required 'ref' field") + return NewValidationError("thumb", "thumb blob missing required 'ref' field") } if _, hasMimeType := thumbMap["mimeType"]; !hasMimeType { - return nil, NewValidationError("thumb", "thumb blob missing required 'mimeType' field") + return NewValidationError("thumb", "thumb blob missing required 'mimeType' field") } log.Printf("[POST-CREATE] Client provided valid thumbnail blob") } else { - return nil, NewValidationError("thumb", + return NewValidationError("thumb", fmt.Sprintf("thumb must be a blob object, got: %T", existingThumb)) } } // TRUSTED AGGREGATOR: Allow Kagi aggregator to provide thumbnail URLs directly // This bypasses unfurl for more accurate RSS-sourced thumbnails - if req.ThumbnailURL != nil && *req.ThumbnailURL != "" && isTrustedAggregator { + if req.ThumbnailURL != nil && *req.ThumbnailURL != "" && trusted { log.Printf("[AGGREGATOR-THUMB] Trusted aggregator provided thumbnail: %s", *req.ThumbnailURL) if s.blobService != nil { @@ -291,8 +351,8 @@ func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*C // Unfurl enhancement (optional, only if URL is supported) // For trusted aggregators: only unfurl for thumbnail if they didn't provide one // For regular users: full unfurl for all metadata - needsThumbnailUnfurl := isTrustedAggregator && external["thumb"] == nil && (req.ThumbnailURL == nil || *req.ThumbnailURL == "") - needsFullUnfurl := !isTrustedAggregator + needsThumbnailUnfurl := trusted && external["thumb"] == nil && (req.ThumbnailURL == nil || *req.ThumbnailURL == "") + needsFullUnfurl := !trusted if needsThumbnailUnfurl || needsFullUnfurl { if uri, ok := external["uri"].(string); ok && uri != "" { @@ -359,29 +419,7 @@ func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*C } } - // 11. Write to community's PDS repository - uri, cid, err := s.createPostOnPDS(ctx, community, postRecord) - if err != nil { - return nil, fmt.Errorf("failed to write post to PDS: %w", err) - } - - // 12. Record aggregator post for rate limiting (non-Kagi aggregators only) - // Kagi is exempted from rate limiting via env var (temporary) - if isOtherAggregator && s.aggregatorService != nil { - if recordErr := s.aggregatorService.RecordAggregatorPost(ctx, req.AuthorDID, communityDID, uri, cid); recordErr != nil { - // Log but don't fail - post was already created successfully - log.Printf("[POST-CREATE] Warning: failed to record aggregator post for rate limiting: %v", recordErr) - } - } - - // 13. Return response (AppView will index via Jetstream consumer) - log.Printf("[POST-CREATE] Author: %s (trustedKagi=%v, otherAggregator=%v), Community: %s, URI: %s", - req.AuthorDID, isTrustedAggregator, isOtherAggregator, communityDID, uri) - - return &CreatePostResponse{ - URI: uri, - CID: cid, - }, nil + return nil } // validateCreateRequest validates basic input requirements diff --git a/internal/db/migrations/035_create_post_submissions.sql b/internal/db/migrations/035_create_post_submissions.sql new file mode 100644 index 0000000..07d77a9 --- /dev/null +++ b/internal/db/migrations/035_create_post_submissions.sql @@ -0,0 +1,80 @@ +-- +goose Up +-- The submission ledger: the rows that both deduplicate submissions and meter +-- the per-author quota of docs/PRD_AUTHOR_OWNED_POSTS.md §4.1 and §8. +-- +-- WHY A NEW TABLE RATHER THAN COUNTING `posts`. Three independent reasons, any +-- one of which is disqualifying. `posts` is firehose-fed, so its rows appear +-- after ingestion lag — which is to say, after the burst being limited has +-- already landed. Its created_at is author-supplied, so once writes flip to +-- author repos (§4.2) the window a quota is measured over becomes +-- attacker-controlled. And its read indexes exclude soft-deleted rows, so +-- deleting a post would return its quota slot and make delete-to-evade the +-- cheapest way past the limit. This table is written synchronously on the +-- write path, stamped by the server, and never soft-deleted. +-- +-- THE UNIQUE CONSTRAINT IS THE DEDUPE GATE, not an assertion about one. The +-- admission decision does not SELECT and then INSERT: it INSERTs and reads the +-- unique violation as the answer, because the database is the only participant +-- that two racing double-taps both talk to. A read-then-write would pass every +-- sequential test and admit both halves of a concurrent duplicate. +-- +-- WHY THE BUCKET IS PART OF THE KEY. Without it the constraint would say "this +-- author may never post this content into this community again", which is a +-- vastly stronger policy than "do not accept the same thing twice right now". +-- The bucket is the index of the application's dedupe window, derived from the +-- injected clock, so the key expires on its own without a sweeper. +-- +-- WHY NO FOREIGN KEYS. Migration 034 dropped posts.fk_author because a +-- federated author has no `users` row (§5.3) — the AppView only bootstraps +-- authors from trusted bridge PDSs today — and a community named by a +-- submission may be one this AppView has not indexed. An FK to either would +-- turn an ordinary submission into an insert failure, and the refusal would +-- surface as a dead letter rather than as a decision. community_blocks +-- (migration 009) and aggregator_posts' successor rationale set the same +-- precedent for the same reason. +CREATE TABLE post_submissions ( + -- The surrogate key exists so a reservation can be RELEASED by identity. + -- Releasing by the natural key would work too, right up until the row it + -- deleted is not the one this request inserted — which is exactly the + -- concurrent case the reservation exists for. + id BIGSERIAL PRIMARY KEY, + + author_did TEXT NOT NULL, + community_did TEXT NOT NULL, + + -- The hash of the canonical record with createdAt removed (see + -- posts.submissionFingerprint). TEXT rather than BYTEA so it is greppable + -- during an incident and comparable in psql; the column is never + -- interpreted, only equated. + fingerprint TEXT NOT NULL, + + -- The index of the dedupe window this submission falls in, derived from + -- the application's injected clock. An integer rather than a timestamp + -- deliberately: a timestamp here invites comparison against created_at, + -- and the two come from different clocks — one the application's, one the + -- database's. + dedupe_bucket BIGINT NOT NULL, + + -- Server-stamped. The rolling window is measured against this column, so a + -- caller who could set it could set their own quota. + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + + CONSTRAINT uq_post_submissions_dedupe + UNIQUE (author_did, community_did, fingerprint, dedupe_bucket) +); + +-- The rolling-window quota query, run on the write path of every post. +-- Migration 012 built idx_aggregator_posts_rate_limit for the identical shape; +-- without the equivalent here the count degrades into a scan of every +-- submission the instance has ever accepted. +CREATE INDEX idx_post_submissions_rate_limit + ON post_submissions (author_did, community_did, created_at DESC); + +COMMENT ON TABLE post_submissions IS 'Synchronous ledger of admitted post submissions: the dedupe gate and the per-author/per-community quota counter (PRD_AUTHOR_OWNED_POSTS 4.1, 8)'; +COMMENT ON COLUMN post_submissions.fingerprint IS 'Hash of the canonical post record with createdAt removed, so a retry of identical content matches the attempt it repeats'; +COMMENT ON COLUMN post_submissions.dedupe_bucket IS 'Index of the application dedupe window; part of the unique key, which is what makes dedupe expire instead of banning a repost forever'; +COMMENT ON COLUMN post_submissions.created_at IS 'Server-stamped submission time; the rolling quota window is measured against it'; +COMMENT ON INDEX idx_post_submissions_rate_limit IS 'CRITICAL: the per-author, per-community rolling-window count taken on every post write'; + +-- +goose Down +DROP TABLE IF EXISTS post_submissions; diff --git a/internal/db/postgres/submission_ledger_repo.go b/internal/db/postgres/submission_ledger_repo.go index 648d5dd..a7eac24 100644 --- a/internal/db/postgres/submission_ledger_repo.go +++ b/internal/db/postgres/submission_ledger_repo.go @@ -3,6 +3,8 @@ package postgres import ( "context" "database/sql" + "errors" + "fmt" "time" "Coves/internal/core/posts" @@ -26,14 +28,72 @@ func NewSubmissionLedger(db *sql.DB) posts.SubmissionLedger { return &submissionLedger{db: db} } +// Reserve inserts the ledger row, letting the unique key answer the dedupe +// question. +// +// ON CONFLICT DO NOTHING ... RETURNING id is the whole check in one statement: +// a fresh submission returns its new id, and a repeat returns no rows at all, +// which arrives here as sql.ErrNoRows. Two racing identical submissions +// therefore get different answers from the database rather than the same answer +// from two reads — which is the entire reason the insert, and not a preceding +// SELECT, is the gate. +// +// The conflict target names the dedupe key explicitly rather than being left +// bare, so that a unique constraint added to this table later cannot be +// silently absorbed into "that was a duplicate". func (l *submissionLedger) Reserve(ctx context.Context, cmd posts.ReserveSubmissionCommand) (posts.SubmissionReservation, error) { - return posts.SubmissionReservation{}, nil + var id int64 + err := l.db.QueryRowContext(ctx, ` + INSERT INTO post_submissions (author_did, community_did, fingerprint, dedupe_bucket) + VALUES ($1, $2, $3, $4) + ON CONFLICT (author_did, community_did, fingerprint, dedupe_bucket) DO NOTHING + RETURNING id + `, cmd.AuthorDID, cmd.CommunityDID, cmd.Fingerprint, cmd.DedupeBucket).Scan(&id) + + if errors.Is(err, sql.ErrNoRows) { + // The row was refused by the dedupe key, which is a policy answer rather + // than a storage failure. It is reported as the domain sentinel so the + // caller can tell "someone already posted this" from "the database is + // unwell" — and so the wording never reaches posts.IsConflict's + // substring match, which would report a refused submission to its author + // as one that already exists in the index. + return posts.SubmissionReservation{}, posts.ErrDuplicateSubmission + } + if err != nil { + return posts.SubmissionReservation{}, fmt.Errorf("failed to reserve submission: %w", err) + } + + return posts.SubmissionReservation{ID: id}, nil } +// Release removes a reservation whose submission never became a post. +// +// Deleting by the surrogate id rather than by the natural key is what makes +// this safe under concurrency: it can only ever remove the row this request +// inserted, so a release racing another author's submission of identical +// content cannot take theirs instead. +// +// Deleting nothing is a success. The caller is already handling a failure when +// it reaches here, and an error over an absent row would replace the reason the +// submission actually failed with a second, less useful one. func (l *submissionLedger) Release(ctx context.Context, reservation posts.SubmissionReservation) error { + if _, err := l.db.ExecContext(ctx, ` + DELETE FROM post_submissions WHERE id = $1 + `, reservation.ID); err != nil { + return fmt.Errorf("failed to release submission reservation: %w", err) + } return nil } +// CountSince counts one author's submissions to one community inside the +// rolling window — the query idx_post_submissions_rate_limit exists for. func (l *submissionLedger) CountSince(ctx context.Context, authorDID, communityDID string, since time.Time) (int, error) { - return 0, nil + var count int + if err := l.db.QueryRowContext(ctx, ` + SELECT count(*) FROM post_submissions + WHERE author_did = $1 AND community_did = $2 AND created_at >= $3 + `, authorDID, communityDID, since).Scan(&count); err != nil { + return 0, fmt.Errorf("failed to count recent submissions: %w", err) + } + return count, nil } -- 2.51.2 From 01accbe32707ce9a560620b6ee872d884ce0bb8a Mon Sep 17 00:00:00 2001 From: Bretton Date: Fri, 7 Aug 2026 19:45:37 -0700 Subject: [PATCH 3/3] fix(admitpost): apply task-3 second-opinion batch (10 streams) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Red-first: fingerprint normalized (unresolved-identifier hash let handle-vs-DID resubmission bypass dedupe; ThumbnailURL now hashed); reservation release decoupled from the dying request context (WithoutCancel — leaks correlated with exactly the failures that trigger release); admission wiring fail-loud (silent null-object defaults deleted; shame-named test-only allow-all); ActorClass fail-closed; aggregator infra-errors no longer minted as authz refusals; double-tap raced under -race; T2 wire probe pins DualAuth + admission routing + refusal-consumes-no-quota + reserve-release through the production-constructed service. Honesty pass: bucket boundary, leaked reservations, unbounded ledger documented. PRD rev 2.6. Co-Authored-By: Claude Fable 5 --- .env.dev.example | 11 + .env.prod.example | 11 + docs/PRD_AUTHOR_OWNED_POSTS.md | 17 +- internal/api/handlers/post/harness_test.go | 2 + internal/config/submissions_test.go | 33 ++ internal/core/posts/admit.go | 203 ++++++++---- internal/core/posts/admit_matrix_test.go | 290 +++++++++++++++++- internal/core/posts/errors.go | 7 +- internal/core/posts/service.go | 37 ++- internal/core/posts/service_admission_test.go | 216 ++++++++++++- .../core/posts/service_aggregator_test.go | 9 +- .../posts/service_author_posts_query_test.go | 3 +- .../posts/service_create_validation_test.go | 3 +- .../core/posts/service_writeforward_test.go | 6 +- .../unfurl/post_unfurl_integration_test.go | 2 + .../035_create_post_submissions.sql | 23 ++ tests/e2e/post_admission_contract_test.go | 268 ++++++++++++++++ tests/live/post_unfurl_test.go | 1 + 18 files changed, 1051 insertions(+), 91 deletions(-) create mode 100644 tests/e2e/post_admission_contract_test.go diff --git a/.env.dev.example b/.env.dev.example index d36418a..b4e9567 100644 --- a/.env.dev.example +++ b/.env.dev.example @@ -200,4 +200,15 @@ OTEL_ENABLED=false # the other recognises the retry a client sends after a lost response. Raising # it makes reposting the same content take longer to become possible again # (default: 1h). +# +# Two deliberate edges of this window. First, dedupe is bucketed against the +# epoch rather than against each submission, so the effective protection +# ranges from just above zero up to the full window depending on where in the +# bucket a submission lands — content posted just before a bucket edge can be +# reposted right after it. Accepted so the dedupe key expires on its own with +# no cleanup process. Second, a crash between reserving a submission slot and +# the PDS write leaves an orphaned reservation that burns one quota slot and +# refuses identical content as a duplicate until the bucket rolls, then +# clears itself; there is no sweeper, and the damage is bounded by this +# window. # POST_SUBMISSIONS_DEDUPE_WINDOW=1h diff --git a/.env.prod.example b/.env.prod.example index 18b97d5..f986583 100644 --- a/.env.prod.example +++ b/.env.prod.example @@ -394,6 +394,17 @@ OTEL_ENABLED=false # the other recognises the retry a client sends after a lost response. Raising # it makes reposting the same content take longer to become possible again # (default: 1h). +# +# Two deliberate edges of this window. First, dedupe is bucketed against the +# epoch rather than against each submission, so the effective protection +# ranges from just above zero up to the full window depending on where in the +# bucket a submission lands — content posted just before a bucket edge can be +# reposted right after it. Accepted so the dedupe key expires on its own with +# no cleanup process. Second, a crash between reserving a submission slot and +# the PDS write leaves an orphaned reservation that burns one quota slot and +# refuses identical content as a duplicate until the bucket rolls, then +# clears itself; there is no sweeper, and the damage is bounded by this +# window. # POST_SUBMISSIONS_DEDUPE_WINDOW=1h # ============================================================================= diff --git a/docs/PRD_AUTHOR_OWNED_POSTS.md b/docs/PRD_AUTHOR_OWNED_POSTS.md index 0a349a9..df9c759 100644 --- a/docs/PRD_AUTHOR_OWNED_POSTS.md +++ b/docs/PRD_AUTHOR_OWNED_POSTS.md @@ -35,7 +35,10 @@ moderation.ban ingestion exists; no production ban writer yet); rate limits/dedupe get a synchronous post_submissions ledger (migration 035) — the posts table is unusable as a limiter substrate (ingestion lag, author-supplied created_at, delete-to-evade); per-origin-PDS quota -explicitly deferred to Beta.** +explicitly deferred to Beta. +Rev 2.6 (2026-08-08): task-3 second-opinion — fingerprint normalized to +resolved-DID scope, release decoupled from request context, admission wiring +fail-loud, ActorClass fail-closed.** **Supersedes** the write-path architecture in `docs/federation-prd.md`: that document solves cross-instance posting by service-auth-forwarding the write to @@ -335,6 +338,11 @@ fiction. Failure mode: author-repo write succeeds, acceptance write fails → post stays `pending`; the firehose engine (§5.6) retries idempotently (same rkey). Degraded latency, not data loss. Never roll back the author's record. +There is a lost-response asymmetry here: when the PDS write's outcome is +ambiguous (the record may or may not exist) and the submission reservation is +released, a client retry can produce a duplicate post — the remedy, noted for +task 6, is to derive the record rkey deterministically from the submission +fingerprint so retries become idempotent at the PDS layer. `post.delete` likewise flips to an author-session delete. @@ -596,11 +604,14 @@ Anyone can write unlimited posts naming any community; nothing stops the - Per-author, per-community, and per-origin-PDS submission quotas in the acceptance engine (new policy, §4.1), with `rejected` + `rate-limit-exceeded` decision codes, `redrivable = false`. -- Dedupe identical submissions by (author, community, content CID). +- Dedupe identical submissions by (author, community, canonical-record + fingerprint) — the hash of the canonical record with `createdAt` removed + (§4.1, rev 2.5), bucketed by the dedupe window. - Debounce edit re-evaluation per post (a rapid edit storm collapses to the latest CID). - Retention caps on `pending`/`rejected` admission rows for never-accepted - posts. + posts, and on the `post_submissions` ledger (migration 035), whose + confirmed rows are otherwise never deleted and grow one per admitted post. - Notify endpoint: per-caller and per-PDS quotas on top of service-auth. - All outbound fetches (identity bootstrap §5.3, record fetch §5.4/§7) behind SSRF guards, response-size caps, and timeouts. diff --git a/internal/api/handlers/post/harness_test.go b/internal/api/handlers/post/harness_test.go index 11c067f..b1956e4 100644 --- a/internal/api/handlers/post/harness_test.go +++ b/internal/api/handlers/post/harness_test.go @@ -82,6 +82,8 @@ func newCreateStack(t *testing.T, db *sql.DB) createStack { communityService, nil, nil, nil, nil, pdsURL, + // Handler translation is the subject here, not admission policy. + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests()), ) return createStack{ diff --git a/internal/config/submissions_test.go b/internal/config/submissions_test.go index 39b62db..13c3581 100644 --- a/internal/config/submissions_test.go +++ b/internal/config/submissions_test.go @@ -63,6 +63,39 @@ func TestLoad_SubmissionQuotaIsReadFromTheEnvironment(t *testing.T) { } } +// The two tests above prove Load reads the variables and TestValidate below +// proves a zero quota is refused at the struct level. Neither proves the +// COMPOSITION: that a bad value set in the environment actually stops Load() +// itself, the call startup makes. These two close that gap. + +func TestLoad_RejectsAnUnparseableSubmissionWindow(t *testing.T) { + clearEnv(t) + t.Setenv("IS_DEV_ENV", "true") + t.Setenv("POST_SUBMISSIONS_WINDOW", "banana") + + _, err := Load() + if err == nil { + t.Fatal("Load() accepted POST_SUBMISSIONS_WINDOW=banana; an unparseable window must stop startup, not fall back silently") + } + if !strings.Contains(err.Error(), "POST_SUBMISSIONS_WINDOW") { + t.Errorf("error should name POST_SUBMISSIONS_WINDOW so an operator can fix it; got:\n%s", err.Error()) + } +} + +func TestLoad_RejectsAZeroSubmissionQuotaFromTheEnvironment(t *testing.T) { + clearEnv(t) + t.Setenv("IS_DEV_ENV", "true") + t.Setenv("POST_SUBMISSIONS_MAX_PER_COMMUNITY", "0") + + _, err := Load() + if err == nil { + t.Fatal("Load() accepted POST_SUBMISSIONS_MAX_PER_COMMUNITY=0; the process would start with the abuse limit inverted or disabled") + } + if !strings.Contains(err.Error(), "POST_SUBMISSIONS_MAX_PER_COMMUNITY") { + t.Errorf("error should name POST_SUBMISSIONS_MAX_PER_COMMUNITY so an operator can fix it; got:\n%s", err.Error()) + } +} + // A config assembled with the quota left at its zero value must not validate. // This is the assertion that makes "unset means unlimited" unrepresentable // rather than merely discouraged. diff --git a/internal/core/posts/admit.go b/internal/core/posts/admit.go index b03789c..9c84492 100644 --- a/internal/core/posts/admit.go +++ b/internal/core/posts/admit.go @@ -10,6 +10,7 @@ import ( "log" "time" + "Coves/internal/core/aggregators" "Coves/internal/core/communities" ) @@ -76,9 +77,10 @@ type AdmissionRequest struct { Community string // Fingerprint identifies WHAT is being submitted: the hash of the canonical - // record with createdAt removed (see submissionFingerprint). It is the - // dedupe key, and it must exclude the timestamp or every resubmission of - // identical content would look new. + // record with createdAt and the client-typed community identifier removed, + // and the supplied thumbnail URL folded in (see submissionFingerprint). It + // is the dedupe key, and it must exclude the timestamp or every + // resubmission of identical content would look new. Fingerprint string } @@ -259,48 +261,55 @@ type AdmissionPolicy struct { Now Clock } -// WithAdmissionPolicy enables the ban check, dedupe and per-author rate limit -// on CreatePost. +// WithAdmissionPolicy supplies the ban check, dedupe and per-author rate limit +// on CreatePost. It is not optional: NewPostService refuses to construct a +// service without a complete policy (see mustCompleteAdmissionPolicy), because +// a post service whose admission policy silently defaulted to no-ops would be +// one whose ban check and quota do not exist and nothing says so. func WithAdmissionPolicy(policy AdmissionPolicy) PostServiceOption { return func(s *postService) { s.admission = &policy } } -// completeAdmissionPolicy fills in the collaborators a policy did not name, so -// that CreatePost has exactly ONE decision path to run. -// -// The alternative — branching on whether a policy was supplied, and keeping the -// pre-policy checks inline for the other branch — would leave two copies of the -// community/visibility/authorization sequence, and §4.1 of the PRD exists -// because the one copy we had already drifted from what its docstring claimed. -// -// The substitutes are named for what they are. A service constructed without a -// policy enforces exactly what CreatePost enforced before this decision existed: -// community existence, private visibility, and aggregator authorization. It is -// the shape every test fixture that predates §8 uses, and cmd/server always -// supplies the real policy — which is what makes the ban lookup and the quota -// live in production. -func completeAdmissionPolicy(policy *AdmissionPolicy) *AdmissionPolicy { - complete := AdmissionPolicy{} - if policy != nil { - complete = *policy - } - if complete.Ledger == nil { - complete.Ledger = unmeteredLedger{} - } - if complete.Bans == nil { - complete.Bans = unenforcedBans{} - } - if complete.Now == nil { - complete.Now = time.Now +// mustCompleteAdmissionPolicy is NewPostService's guard: a service may not be +// constructed without a complete admission policy. +// +// It panics rather than returning an error, matching how this codebase treats +// every other mandatory collaborator (aggregators.NewAPIKeyService, +// blueskypost.NewService): a missing policy is a wiring bug that must stop the +// process at startup, not a runtime condition to handle. The old alternative — +// silently substituting a no-op ledger and ban lookup — is exactly how the +// pre-§4.1 docstring came to claim "membership/ban validation" that had never +// existed on the write path. +// +// Every limit must be positive for the same reason config.Validate enforces +// it: a quota that silently disappears when a field is left zero is not a +// quota. Tests that are not about admission opt out EXPLICITLY with +// NewAllowAllAdmissionPolicyForTests. +func mustCompleteAdmissionPolicy(policy *AdmissionPolicy) { + switch { + case policy == nil: + panic("posts.NewPostService: an admission policy is required — wire posts.WithAdmissionPolicy " + + "(cmd/server) or posts.NewAllowAllAdmissionPolicyForTests (fixtures that are not about admission)") + case policy.Ledger == nil: + panic("posts.NewPostService: AdmissionPolicy.Ledger cannot be nil") + case policy.Bans == nil: + panic("posts.NewPostService: AdmissionPolicy.Bans cannot be nil") + case policy.Now == nil: + panic("posts.NewPostService: AdmissionPolicy.Now cannot be nil") + case policy.Limits.MaxPerAuthorPerCommunity <= 0: + panic("posts.NewPostService: AdmissionPolicy.Limits.MaxPerAuthorPerCommunity must be positive") + case policy.Limits.Window <= 0: + panic("posts.NewPostService: AdmissionPolicy.Limits.Window must be positive") + case policy.Limits.DedupeWindow <= 0: + panic("posts.NewPostService: AdmissionPolicy.Limits.DedupeWindow must be positive") } - return &complete } -// unmeteredLedger stands in when no submission ledger was wired: it reserves -// nothing, so neither dedupe nor the per-author quota applies. +// unmeteredLedger is the allow-all test policy's ledger: it reserves nothing, +// so neither dedupe nor the per-author quota applies. // // It cannot silently disable a configured limiter — it is only ever reachable -// when AdmissionPolicy.Ledger is nil, which cmd/server never leaves so. +// through NewAllowAllAdmissionPolicyForTests, whose name is the warning. type unmeteredLedger struct{} func (unmeteredLedger) Reserve(context.Context, ReserveSubmissionCommand) (SubmissionReservation, error) { @@ -316,19 +325,44 @@ func (unmeteredLedger) CountSince(context.Context, string, string, time.Time) (i return 0, nil } -// unenforcedBans stands in when no ban lookup was wired, answering the way an -// author with no membership row does. It returns the sentinel rather than a nil -// membership so that it travels the same branch a real absent row does — the -// "no membership means not banned" translation stays in one place. +// unenforcedBans is the allow-all test policy's ban lookup, answering the way +// an author with no membership row does. It returns the sentinel rather than a +// nil membership so that it travels the same branch a real absent row does — +// the "no membership means not banned" translation stays in one place. type unenforcedBans struct{} func (unenforcedBans) GetMembership(context.Context, string, string) (*communities.Membership, error) { return nil, communities.ErrMembershipNotFound } +// NewAllowAllAdmissionPolicyForTests is the explicit opt-out for TEST fixtures +// whose subject is not admission: it admits everything an unconfigured service +// used to — no ban rows to find, no dedupe, no per-author quota — while +// community existence, visibility and aggregator authorization stay enforced. +// +// THE NAME IS THE CONTRACT: this must never be wired in production code. +// cmd/server wires the real policy, and mustCompleteAdmissionPolicy exists +// precisely so that forgetting to do so fails at startup instead of shipping a +// post service whose §8 enforcement quietly does not exist. The limits are +// real (and enormous) only because construction refuses non-positive ones; the +// unmetered ledger never counts against them anyway. +func NewAllowAllAdmissionPolicyForTests() AdmissionPolicy { + return AdmissionPolicy{ + Ledger: unmeteredLedger{}, + Bans: unenforcedBans{}, + Limits: SubmissionLimits{ + MaxPerAuthorPerCommunity: 1 << 30, + Window: time.Hour, + DedupeWindow: time.Hour, + }, + Now: time.Now, + } +} + // admissionDeps assembles the decision's inputs from the service's -// collaborators. s.admission is never nil — NewPostService completes it — so -// this cannot silently hand admitPost a missing ledger or clock. +// collaborators. s.admission is never nil or incomplete — NewPostService +// refuses to construct without a complete policy — so this cannot silently +// hand admitPost a missing ledger or clock. func (s *postService) admissionDeps() admissionDeps { return admissionDeps{ communities: s.communityService, @@ -435,6 +469,17 @@ type admissionDeps struct { // A non-nil error means the decision could NOT be made — a lookup failed — and // is distinct from a refusal, which is a decision. func admitPost(ctx context.Context, deps admissionDeps, req AdmissionRequest) (AdmissionDecision, error) { + // 0. The actor class must be one this decision knows. It gates everything + // below — including the trusted skip of visibility, ban and authorization — + // so an unknown value must fail CLOSED before any lookup runs. Falling + // through would hand the zero value (a caller that forgot to classify) the + // widest privileges in the system. + switch req.Actor { + case ActorUser, ActorRegisteredAggregator, ActorTrustedAggregator: + default: + return undecided(fmt.Errorf("unknown actor class %q: the submission cannot be evaluated", req.Actor)) + } + // 1. Community resolution. Two lookups — the at-identifier to a DID, then // the DID to the indexed row — and either failing to find it is the same // answer to the client. @@ -490,9 +535,18 @@ func admitPost(ctx context.Context, deps admissionDeps, req AdmissionRequest) (A // sentinel that caused it: the boundary tells 403 from 429 by matching on // it, and a bare code would have a well-behaved aggregator retry a // permanent refusal forever. + // + // Only the package's POLICY sentinels are refusals. ValidateAggregatorPost + // also fails when its own lookups do (a wrapped driver error carrying no + // sentinel), and that is an undecided infrastructure failure like any + // other — dressing it as DecisionAggregatorNotAuthorized would mint a + // permanent-sounding 403 out of a Postgres blip. if req.Actor == ActorRegisteredAggregator { if err := deps.aggregators.ValidateAggregatorPost(ctx, req.AuthorDID, community.DID); err != nil { - return AdmissionDecision{Code: DecisionAggregatorNotAuthorized, Cause: err}, nil + if errors.Is(err, aggregators.ErrNotAuthorized) || errors.Is(err, aggregators.ErrRateLimitExceeded) { + return AdmissionDecision{Code: DecisionAggregatorNotAuthorized, Cause: err}, nil + } + return undecided(fmt.Errorf("failed to validate aggregator post: %w", err)) } } @@ -550,8 +604,21 @@ func undecided(err error) (AdmissionDecision, error) { // use. The error is logged rather than returned: every caller reaches this // while already reporting a refusal or a failure, and replacing that answer // with a second one would hide the reason the submission was actually stopped. +// +// The release runs DETACHED from the caller's cancellation (precedent: +// adminreports.raiseAlert), because the most common reason to be here at all +// is that the caller's context is already dead — a client that disconnected +// mid-write is exactly a failed PDS write. A release issued on that context +// would be refused by Postgres as canceled too, and the reservation would +// leak: one quota slot burned and the author's retry refused as a duplicate, +// with nothing but a warning line to say why. Context values (trace IDs) +// survive; only the cancellation signal is dropped, and the fresh timeout +// keeps a wedged database from pinning the goroutine. func releaseReservation(ctx context.Context, ledger SubmissionLedger, reservation SubmissionReservation) { - if err := ledger.Release(ctx, reservation); err != nil { + releaseCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) + defer cancel() + + if err := ledger.Release(releaseCtx, reservation); err != nil { log.Printf("[POST-ADMIT] Warning: failed to release submission reservation %d: %v", reservation.ID, err) } } @@ -559,6 +626,16 @@ func releaseReservation(ctx context.Context, ledger SubmissionLedger, reservatio // dedupeBucket is the index of the window `now` falls in, so that two // submissions in the same window collide on the ledger's unique key and two // submissions a window apart do not. +// +// Buckets are aligned to the epoch, not to the submission, so the effective +// dedupe protection ranges over (0, window] depending on where in the bucket +// a submission lands: content submitted just before a bucket edge can be +// resubmitted the moment the edge passes. That tradeoff is deliberate — the +// epoch-aligned key self-expires without a sweeper, where a per-submission +// window would need a range predicate or a cleanup process to expire. The +// same boundary bounds a leaked reservation: a crash between Reserve and the +// PDS write leaves a row that burns one quota slot and refuses identical +// content as a duplicate until the bucket rolls, then heals on its own. func dedupeBucket(now time.Time, window time.Duration) int64 { // A non-positive window would divide by zero. config.Validate refuses to // start a process with one, so reaching this is a wiring bug rather than an @@ -571,19 +648,40 @@ func dedupeBucket(now time.Time, window time.Duration) int64 { return now.UnixNano() / int64(window) } -// submissionFingerprint hashes what a moderator would judge about a record: -// everything except createdAt. +// submissionFingerprint hashes what a moderator would judge about a +// submission: everything on the record except createdAt and community, plus +// the thumbnail URL that rides alongside the record. // // The timestamp has to go. It is stamped by the server at submission time -// (service.go step 9), so it differs on every attempt — including the retry +// (service.go step 6), so it differs on every attempt — including the retry // after a lost response, which is the case dedupe exists to catch. A // fingerprint that included it would never match anything. -func submissionFingerprint(record PostRecord) string { - // The record is taken by value, so clearing the timestamp here cannot - // affect the record the caller goes on to write. +// +// The community field has to go too, for the opposite failure. It holds the +// at-identifier as the CLIENT typed it — a handle one time, a DID the next — +// while the ledger's unique key already scopes the fingerprint by the +// RESOLVED community DID. Hashing the client-typed identifier would let the +// same submission to the same community bypass dedupe simply by switching +// spelling between attempts; leaving it out cannot collide submissions to +// DIFFERENT communities, because the ledger key keeps them apart. +// +// The thumbnail URL is IN, even though it is not a record field: a trusted +// aggregator supplies it alongside the record (CreatePostRequest.ThumbnailURL) +// and it changes what readers ultimately see. Two submissions differing only +// in their thumbnail are different posts, and excluding it would refuse the +// second as a repeat of the first. +func submissionFingerprint(record PostRecord, thumbnailURL *string) string { + // The record is taken by value, so clearing fields here cannot affect the + // record the caller goes on to write. record.CreatedAt = "" + record.Community = "" + + material := struct { + Record PostRecord `json:"record"` + ThumbnailURL *string `json:"thumbnailUrl,omitempty"` + }{Record: record, ThumbnailURL: thumbnailURL} - canonical, err := json.Marshal(record) + canonical, err := json.Marshal(material) if err != nil { // Unreachable in practice: every field of a PostRecord either has a // concrete marshalable type or holds a value decoded from JSON. Hashing @@ -591,7 +689,8 @@ func submissionFingerprint(record PostRecord) string { // a constant fingerprint would collide every submission with every // other, and the second post the instance ever received would be // refused as a repeat of the first. - canonical = []byte(fmt.Sprintf("%#v", record)) + log.Printf("[POST-ADMIT] Warning: submission fingerprint fell back to a Go rendering, canonical JSON marshal failed: %v", err) + canonical = []byte(fmt.Sprintf("%#v", material)) } sum := sha256.Sum256(canonical) diff --git a/internal/core/posts/admit_matrix_test.go b/internal/core/posts/admit_matrix_test.go index 0f77e17..0cbade7 100644 --- a/internal/core/posts/admit_matrix_test.go +++ b/internal/core/posts/admit_matrix_test.go @@ -556,6 +556,62 @@ func TestAdmitPost_AnAggregatorRefusalKeepsItsSentinel(t *testing.T) { // Failing closed // --------------------------------------------------------------------------- +// An actor class the decision does not recognise must fail CLOSED, before any +// lookup runs. The zero value is the dangerous one: a caller that forgot to +// classify the actor would otherwise sail past every check that switches on +// req.Actor — which is exactly the trusted-aggregator skip path — and a +// database outage would be the least of it. +func TestAdmitPost_AnUnknownActorClassFailsClosed(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + actor ActorClass + }{ + {"the zero value", ActorClass("")}, + {"an unrecognised class", ActorClass("99")}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + + decision, err := h.admit(t, tc.actor, "probe") + require.Error(t, err, + "an unclassifiable actor must fail the request, never fall through to the trusted-skip path") + assert.False(t, decision.Admitted()) + assert.Emptyf(t, decision.Code, + "an unclassifiable actor is a caller bug, not a policy refusal (%q)", decision.Code) + + assert.Zero(t, h.communities.resolveCalls, "no lookup may run for an actor the decision cannot classify") + assert.Zero(t, h.communities.getCalls, "no lookup may run for an actor the decision cannot classify") + assert.Zero(t, h.bans.calls, "no lookup may run for an actor the decision cannot classify") + assert.Zero(t, h.aggregators.calls, "no lookup may run for an actor the decision cannot classify") + assert.Empty(t, h.ledger.reserveCalls, "no reservation may be taken for an actor the decision cannot classify") + }) + } +} + +// A ValidateAggregatorPost failure that is NOT one of the aggregators package's +// policy sentinels is infrastructure, not a refusal. Mapping a database error +// to DecisionAggregatorNotAuthorized would tell a perfectly authorized +// aggregator to stop asking — a 403 minted out of a Postgres blip. +func TestAdmitPost_AnAggregatorLookupFailureIsAnErrorNotARefusal(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + h.aggregators.err = errors.New("driver: bad connection") + + decision, err := h.admit(t, ActorRegisteredAggregator, "item") + require.Error(t, err, + "an authorization check that could not be evaluated must fail the request, not refuse it") + assert.False(t, decision.Admitted()) + assert.Emptyf(t, decision.Code, + "an infrastructure failure must not be dressed up as an authorization refusal (%q)", decision.Code) + assert.Empty(t, h.ledger.reserveCalls, + "a submission we could not evaluate must not reserve quota") +} + // A ban lookup that fails for any reason OTHER than "no such membership" must // fail the request. // @@ -587,6 +643,11 @@ func TestAdmitPost_InfrastructureFailuresAreErrorsNotRefusals(t *testing.T) { for _, tc := range []struct { name string setup func(*admitHarness) + + // wantReserveCalls is how many times the failing path was expected to + // reach the ledger before the failure stopped it — the precondition + // that makes the liveRows assertion below meaningful. + wantReserveCalls int }{ { name: "the community index is unreachable", @@ -597,12 +658,14 @@ func TestAdmitPost_InfrastructureFailuresAreErrorsNotRefusals(t *testing.T) { setup: func(h *admitHarness) { h.communities.getErr = errors.New("connection reset by peer") }, }, { - name: "the ledger insert fails for a reason that is not a duplicate", - setup: func(h *admitHarness) { h.ledger.reserveErr = errors.New("deadlock detected") }, + name: "the ledger insert fails for a reason that is not a duplicate", + setup: func(h *admitHarness) { h.ledger.reserveErr = errors.New("deadlock detected") }, + wantReserveCalls: 1, }, { - name: "the quota count fails", - setup: func(h *admitHarness) { h.ledger.countErr = errors.New("statement timeout") }, + name: "the quota count fails", + setup: func(h *admitHarness) { h.ledger.countErr = errors.New("statement timeout") }, + wantReserveCalls: 1, }, } { t.Run(tc.name, func(t *testing.T) { @@ -611,12 +674,19 @@ func TestAdmitPost_InfrastructureFailuresAreErrorsNotRefusals(t *testing.T) { h := newAdmitHarness() tc.setup(h) + require.Zero(t, h.ledger.liveRows(), "the ledger must start empty for the release assertion to mean anything") + decision, err := h.admit(t, ActorUser, "probe") require.Error(t, err) assert.False(t, decision.Admitted(), "a decision that could not be made must not read as an admission") assert.Emptyf(t, decision.Code, "an infrastructure failure must not be dressed up as a policy code (%q); the client would be told to stop retrying something that will work in a second", decision.Code) + + assert.Len(t, h.ledger.reserveCalls, tc.wantReserveCalls, + "the failure was injected at a different point in the flow than this case describes") + assert.Zero(t, h.ledger.liveRows(), + "an undecided submission left a reservation on the ledger: it burned quota and will refuse the client's retry as a duplicate") }) } } @@ -901,6 +971,117 @@ func TestAdmitPost_QuotaIsScopedToOneCommunity(t *testing.T) { "the quota is per community; being at the limit in one must not silence the author in another") } +// --------------------------------------------------------------------------- +// Construction +// --------------------------------------------------------------------------- + +// A post service without a complete admission policy is not a lighter post +// service — it is one whose ban check, dedupe and quota silently do not exist. +// Construction must therefore fail loudly, the way this codebase treats every +// other mandatory collaborator (aggregators.NewAPIKeyService, blueskypost), +// rather than substituting no-op defaults a production wiring mistake would +// never notice. +func TestNewPostService_RefusesConstructionWithoutACompleteAdmissionPolicy(t *testing.T) { + t.Parallel() + + validLimits := SubmissionLimits{ + MaxPerAuthorPerCommunity: 3, + Window: time.Hour, + DedupeWindow: time.Hour, + } + complete := func() AdmissionPolicy { + return AdmissionPolicy{ + Ledger: &stubLedger{now: time.Now}, + Bans: &stubBans{}, + Limits: validLimits, + Now: time.Now, + } + } + + for _, tc := range []struct { + name string + opts []PostServiceOption + }{ + { + name: "no admission policy at all", + opts: nil, + }, + { + name: "a policy with no ledger", + opts: []PostServiceOption{WithAdmissionPolicy(func() AdmissionPolicy { + p := complete() + p.Ledger = nil + return p + }())}, + }, + { + name: "a policy with no ban lookup", + opts: []PostServiceOption{WithAdmissionPolicy(func() AdmissionPolicy { + p := complete() + p.Bans = nil + return p + }())}, + }, + { + name: "a policy with no clock", + opts: []PostServiceOption{WithAdmissionPolicy(func() AdmissionPolicy { + p := complete() + p.Now = nil + return p + }())}, + }, + { + name: "a policy with an unset quota", + opts: []PostServiceOption{WithAdmissionPolicy(func() AdmissionPolicy { + p := complete() + p.Limits.MaxPerAuthorPerCommunity = 0 + return p + }())}, + }, + { + name: "a policy with an unset window", + opts: []PostServiceOption{WithAdmissionPolicy(func() AdmissionPolicy { + p := complete() + p.Limits.Window = 0 + return p + }())}, + }, + { + name: "a policy with an unset dedupe window", + opts: []PostServiceOption{WithAdmissionPolicy(func() AdmissionPolicy { + p := complete() + p.Limits.DedupeWindow = 0 + return p + }())}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + require.Panics(t, func() { + NewPostService(nil, nil, nil, nil, nil, nil, "", tc.opts...) + }, "a service constructed without a complete admission policy would enforce nothing and say nothing about it") + }) + } + + t.Run("a complete policy constructs", func(t *testing.T) { + t.Parallel() + + require.NotPanics(t, func() { + NewPostService(nil, nil, nil, nil, nil, nil, "", WithAdmissionPolicy(complete())) + }) + }) + + t.Run("the test-only allow-all policy constructs", func(t *testing.T) { + t.Parallel() + + require.NotPanics(t, func() { + NewPostService(nil, nil, nil, nil, nil, nil, "", + WithAdmissionPolicy(NewAllowAllAdmissionPolicyForTests())) + }, "fixtures that are not about admission need an explicit, honestly-named way to opt out") + }) +} + // --------------------------------------------------------------------------- // The sentinel's wording // --------------------------------------------------------------------------- @@ -924,9 +1105,12 @@ func TestErrDuplicateSubmissionIsNotAStorageConflict(t *testing.T) { // The fingerprint is what makes two submissions "identical". createdAt is // stamped per attempt, so including it would make every retry look new and -// dedupe would never fire; everything a moderator would judge must be included, -// or two genuinely different posts would collide and the second would be -// refused as a repeat of the first. +// dedupe would never fire; the community field is the identifier as the CLIENT +// typed it, so including it would let a handle-vs-DID resubmission bypass +// dedupe; everything a moderator would judge must be included — including the +// thumbnail an aggregator supplies alongside the record — or two genuinely +// different posts would collide and the second would be refused as a repeat of +// the first. func TestSubmissionFingerprint(t *testing.T) { t.Parallel() @@ -948,24 +1132,46 @@ func TestSubmissionFingerprint(t *testing.T) { later := base() later.CreatedAt = "2026-08-01T12:00:09Z" - assert.Equal(t, submissionFingerprint(base()), submissionFingerprint(later), + assert.Equal(t, submissionFingerprint(base(), nil), submissionFingerprint(later, nil), "the server stamps createdAt per attempt, so a fingerprint that included it would never match a retry") }) + t.Run("the community identifier is excluded", func(t *testing.T) { + t.Parallel() + + byHandle := base() + byHandle.Community = admitCommunityHandle + + assert.Equal(t, submissionFingerprint(base(), nil), submissionFingerprint(byHandle, nil), + "the community field holds whatever identifier the client typed; hashing it would let the same "+ + "submission dodge dedupe by naming the community by handle once and by DID the next time — "+ + "the ledger's unique key already scopes the fingerprint to the RESOLVED community DID") + }) + t.Run("a non-empty fingerprint", func(t *testing.T) { t.Parallel() - assert.NotEmpty(t, submissionFingerprint(base()), + assert.NotEmpty(t, submissionFingerprint(base(), nil), "an empty fingerprint would make every submission collide with every other") }) + t.Run("a different thumbnail is a different submission", func(t *testing.T) { + t.Parallel() + + one, two := "https://example.com/thumb-1.jpg", "https://example.com/thumb-2.jpg" + assert.NotEqual(t, submissionFingerprint(base(), &one), submissionFingerprint(base(), &two), + "the thumbnail is submission material an aggregator supplies alongside the record; "+ + "excluding it would refuse a post differing only in its thumbnail as a repeat") + assert.NotEqual(t, submissionFingerprint(base(), nil), submissionFingerprint(base(), &one), + "a submission with a thumbnail is not a repeat of the same submission without one") + }) + for _, tc := range []struct { field string mutate func(*PostRecord) }{ {"title", func(r *PostRecord) { title := "A different title"; r.Title = &title }}, {"content", func(r *PostRecord) { content := "Different body text"; r.Content = &content }}, - {"community", func(r *PostRecord) { r.Community = "did:plc:dddddddddddddddddddddddd" }}, {"author", func(r *PostRecord) { r.Author = "did:plc:eeeeeeeeeeeeeeeeeeeeeeee" }}, {"embed", func(r *PostRecord) { r.Embed = map[string]interface{}{"$type": "social.coves.embed.external"} @@ -976,8 +1182,70 @@ func TestSubmissionFingerprint(t *testing.T) { changed := base() tc.mutate(&changed) - assert.NotEqual(t, submissionFingerprint(base()), submissionFingerprint(changed), + assert.NotEqual(t, submissionFingerprint(base(), nil), submissionFingerprint(changed, nil), "two posts differing in %s would collide, and the second would be refused as a repeat of the first", tc.field) }) } } + +// The dedupe gate must recognise a resubmission no matter which at-identifier +// the client used to name the community. The ledger's unique key scopes the +// fingerprint by the RESOLVED community DID, so the fingerprint itself must not +// re-introduce the client-typed identifier — a fingerprint that hashed it would +// admit the same post twice for anyone who typed the handle once and the DID +// the second time. +func TestAdmitPost_ResubmissionByDIDAfterHandleIsADuplicate(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + + title, content := "The same post", "the same body" + record := PostRecord{ + Type: postCollection, + Author: admitAuthorDID, + Title: &title, + Content: &content, + } + + byHandle := record + byHandle.Community = admitCommunityHandle + first, err := h.admit(t, ActorUser, submissionFingerprint(byHandle, nil)) + require.NoError(t, err) + require.True(t, first.Admitted()) + + byDID := record + byDID.Community = admitCommunityDID + second, err := h.admit(t, ActorUser, submissionFingerprint(byDID, nil)) + require.NoError(t, err) + assert.Equal(t, DecisionDuplicateSubmission, second.Code, + "naming the community by DID instead of by handle must not turn a resubmission into a new post") + assert.Equal(t, 1, h.ledger.liveRows()) +} + +// The other direction of the same property: two submissions differing ONLY in +// their thumbnail are different posts, and both must be admitted. +func TestAdmitPost_AThumbnailOnlyDifferenceIsNotADuplicate(t *testing.T) { + t.Parallel() + + h := newAdmitHarness() + + title := "The same link, a different thumbnail" + record := PostRecord{ + Type: postCollection, + Community: admitCommunityHandle, + Author: admitAuthorDID, + Title: &title, + } + + one, two := "https://example.com/thumb-1.jpg", "https://example.com/thumb-2.jpg" + + first, err := h.admit(t, ActorUser, submissionFingerprint(record, &one)) + require.NoError(t, err) + require.True(t, first.Admitted()) + + second, err := h.admit(t, ActorUser, submissionFingerprint(record, &two)) + require.NoError(t, err) + assert.Truef(t, second.Admitted(), + "a thumbnail-only difference is a different post, refused with %q", second.Code) + assert.Equal(t, 2, h.ledger.liveRows()) +} diff --git a/internal/core/posts/errors.go b/internal/core/posts/errors.go index 713d9de..6a7286c 100644 --- a/internal/core/posts/errors.go +++ b/internal/core/posts/errors.go @@ -26,7 +26,12 @@ var ( // ErrNotFound is returned when a post is not found by URI ErrNotFound = errors.New("post not found") - // ErrRateLimitExceeded is returned when an aggregator exceeds rate limits + // ErrRateLimitExceeded is returned when a submission is refused for being + // over quota — primarily the per-author, per-community submission limit of + // PRD_AUTHOR_OWNED_POSTS.md §8 (DecisionRateLimitExceeded). The handler + // maps it to a 429. (An aggregator over its OWN hourly quota is refused + // through the aggregators package's sentinel instead, so the boundary can + // tell the two apart.) ErrRateLimitExceeded = errors.New("rate limit exceeded") // ErrInvalidCursor is returned when a pagination cursor is malformed diff --git a/internal/core/posts/service.go b/internal/core/posts/service.go index 20d6b76..8fe70a7 100644 --- a/internal/core/posts/service.go +++ b/internal/core/posts/service.go @@ -74,24 +74,33 @@ func NewPostService( for _, opt := range opts { opt(s) } - s.admission = completeAdmissionPolicy(s.admission) + // The admission policy is mandatory, and a missing or partial one panics + // here rather than defaulting to no-ops: a post service whose ban check and + // quota silently do not exist is a wiring bug, not a configuration. + mustCompleteAdmissionPolicy(s.admission) return s } // CreatePost creates a new post in a community // Flow: -// 1. Validate input -// 2. Check if author is an aggregator (server-side validation using DID from JWT) -// 3. Admission: one decision over community existence, visibility, ban, +// 1. Validate input (and normalize embed/facet URIs) +// 2. Verify the authenticated DID matches the request's author DID +// 3. Classify the actor: trusted aggregator, registered aggregator, or user +// 4. Admission: one decision over community existence, visibility, ban, // aggregator authorization, dedupe and the per-author quota (admitPost) -// 4. Build post record -// 5. Write to community's PDS repository -// 6. If aggregator: record post for rate limiting -// 7. Return URI/CID (AppView indexes asynchronously via Jetstream) +// 5. Ensure the community has fresh PDS credentials (token refresh) +// 6. Build the post record +// 7. Validate and enhance external embeds (thumb validation, unfurl, blobs) +// 8. Write to community's PDS repository +// 9. If aggregator: record post for rate limiting +// 10. Return URI/CID (AppView indexes asynchronously via Jetstream) // // Admission runs BEFORE the token refresh, the blob uploads and the PDS write, // so a refused submission costs a few lookups rather than an upload — and, -// more to the point, leaves no record in a community that refused it. +// more to the point, leaves no record in a community that refused it. Every +// failure AFTER admission (steps 5-8) must release the ledger reservation the +// admission took, or the failure costs the author a quota slot and refuses +// their retry as a duplicate. func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*CreatePostResponse, error) { // 1. Validate basic input (before DID checks to give clear validation errors) if err := s.validateCreateRequest(&req); err != nil { @@ -146,13 +155,13 @@ func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*C // Check if this is a non-trusted aggregator (requires database lookup) var isOtherAggregator bool if !isTrustedAggregator && s.aggregatorService != nil { - aggregator, err := s.aggregatorService.IsAggregator(ctx, req.AuthorDID) + isAggregator, err := s.aggregatorService.IsAggregator(ctx, req.AuthorDID) if err != nil { log.Printf("[POST-CREATE] Warning: failed to check if DID is aggregator: %v", err) // Don't fail the request - treat as regular user if check fails isOtherAggregator = false } else { - isOtherAggregator = aggregator + isOtherAggregator = isAggregator } } @@ -176,12 +185,14 @@ func (s *postService) CreatePost(ctx context.Context, req CreatePostRequest) (*C // The fingerprint is taken from the record as the CLIENT sent it, before // unfurl enhancement rewrites the embed: two submissions of the same content // must hash the same, and an enriched embed varies with whatever the remote - // page served at the time. + // page served at the time. The thumbnail URL rides along as submitted; the + // client-typed community identifier and the per-attempt timestamp are + // excluded inside submissionFingerprint (see its doc comment). decision, err := admitPost(ctx, s.admissionDeps(), AdmissionRequest{ Actor: actor, AuthorDID: req.AuthorDID, Community: req.Community, - Fingerprint: submissionFingerprint(postRecordFor(req, req.Community, "")), + Fingerprint: submissionFingerprint(postRecordFor(req, req.Community, ""), req.ThumbnailURL), }) if err != nil { return nil, err diff --git a/internal/core/posts/service_admission_test.go b/internal/core/posts/service_admission_test.go index 6b43841..f572c17 100644 --- a/internal/core/posts/service_admission_test.go +++ b/internal/core/posts/service_admission_test.go @@ -5,6 +5,7 @@ package posts_test import ( "context" "database/sql" + "encoding/base64" "fmt" "net/http" "net/http/httptest" @@ -283,20 +284,23 @@ func TestService_TheAuthorQuotaStopsTheNextSubmission(t *testing.T) { assert.NoError(t, err, "the quota is per (author, community); being at the limit in one must not close the others") } -// An identical resubmission is a repeat, not a new post. +// An identical resubmission is a repeat, not a new post — even when the retry +// names the community by DID and the original named it by handle. // // The canonical case is a client that retried after a lost response, and the // answer has to be distinguishable from a quota breach: 409 tells the client its // post already exists, 429 tells it to wait. A submission refused as a duplicate // must also not be billed, or a flaky connection would rate-limit a user who -// posted once. +// posted once. Submitting first by HANDLE and retrying by DID is the identifier +// dodge the fingerprint must not fall for: the ledger scopes dedupe by the +// RESOLVED community DID, so the client-typed spelling must not enter the key. func TestService_AnIdenticalResubmissionIsRefusedAsADuplicate(t *testing.T) { t.Parallel() f := newAdmissionFixture(t) - _, err := f.submit(t, f.base.community.DID, "the very same post") - require.NoError(t, err) + _, err := f.submit(t, f.base.community.Handle, "the very same post") + require.NoError(t, err, "submitting by handle must resolve and admit like submitting by DID") _, err = f.submit(t, f.base.community.DID, "the very same post") require.Error(t, err) @@ -363,6 +367,210 @@ func TestService_AFailedPDSWriteReleasesTheReservation(t *testing.T) { assert.Equal(t, f.base.author.DID, record.Value["author"]) } +// A client that goes away MID-WRITE must still get its reservation back. +// +// The failure path runs on the same context the request came in on, and by the +// time the release runs that context is already dead — the canceled write is +// exactly why the path was taken. A release issued on the caller's context +// would be refused by Postgres as canceled too, and the leak would be +// invisible: the request already failed, the log line is a warning, and the +// author discovers it as a duplicate refusal of a post that does not exist. +// The release must therefore run detached from the caller's cancellation +// (precedent: adminreports raiseAlert), bounded by its own timeout. +func TestService_ACancellationDuringThePDSWriteStillReleasesTheReservation(t *testing.T) { + t.Parallel() + + f := newAdmissionFixture(t) + + // The client's context, canceled by the "PDS" at the exact moment the + // write is in flight — the request-scoped context is dead by the time + // CreatePost's failure path runs, which is the shape of a client + // disconnecting mid-request. + ctx, cancel := context.WithCancel(middleware.SetTestUserDID(context.Background(), f.base.author.DID)) + defer cancel() + + canceling := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // Kill the caller's context while its write is in flight, then refuse + // the write. Whether CreatePost's failure surfaces as the canceled + // context or as the 500 is an interleaving detail; cancel() happens + // before the response is written, so by the time the failure path runs + // the request's context is dead either way. + cancel() + http.Error(w, `{"error":"InternalServerError"}`, http.StatusInternalServerError) + })) + t.Cleanup(canceling.Close) + + healthyURL := communityPDSURL(t, f.base.db, f.base.community.DID) + setCommunityPDSURL(t, f.base.db, f.base.community.DID, canceling.URL) + + const repeatable = "a post whose client disconnects mid-write" + content := "a body that makes this a complete post" + _, err := f.service.CreatePost(ctx, posts.CreatePostRequest{ + Community: f.base.community.DID, + Title: func() *string { s := repeatable; return &s }(), + Content: &content, + AuthorDID: f.base.author.DID, + }) + require.Error(t, err, "the write ran against a dead context and must fail") + + assert.Zerof(t, f.ledgerRows(t, f.base.community.DID), + "the release ran on the caller's canceled context and was refused with it: the reservation leaked, burning a quota slot and blocking the retry as a duplicate") + + setCommunityPDSURL(t, f.base.db, f.base.community.DID, healthyURL) + + // The retry a reconnected client sends: byte-identical content on a live + // context. Admissible only if the canceled attempt released its row. + resp, err := f.submit(t, f.base.community.DID, repeatable) + require.NoError(t, err, "the identical retry was refused, so the canceled attempt leaked its reservation") + require.NotEmpty(t, resp.URI) + assert.Equal(t, 1, f.ledgerRows(t, f.base.community.DID)) +} + +// A token-refresh failure (step 5) happens with the reservation already on the +// ledger, and must give it back for the same reason a failed PDS write must: +// the community's credentials failing is not the author's fault, and must not +// cost them a quota slot or refuse their retry as a duplicate. +func TestService_ATokenRefreshFailureReleasesTheReservation(t *testing.T) { + t.Parallel() + + f := newAdmissionFixture(t) + ctx := context.Background() + + original, err := f.repo.GetByDID(ctx, f.base.community.DID) + require.NoError(t, err) + require.NotEmpty(t, original.PDSAccessToken, "the fixture community must hold real credentials to restore") + + // An expired access token forces EnsureFreshToken down the refresh path, + // and the community's PDS — repointed at a server that 500s everything — + // refuses the refresh. Same seam as the failed-write test: the pds_url and + // credentials are read fresh off the community row on every write. + broken := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, `{"error":"InternalServerError"}`, http.StatusInternalServerError) + })) + t.Cleanup(broken.Close) + + healthyURL := communityPDSURL(t, f.base.db, f.base.community.DID) + setCommunityPDSURL(t, f.base.db, f.base.community.DID, broken.URL) + require.NoError(t, f.repo.UpdateCredentials(ctx, f.base.community.DID, expiredJWT(t), original.PDSRefreshToken)) + + const repeatable = "a post whose community credentials fail to refresh" + _, err = f.submit(t, f.base.community.DID, repeatable) + require.Error(t, err, "the token refresh failed, so CreatePost must report a failure") + + assert.Zerof(t, f.ledgerRows(t, f.base.community.DID), + "the reservation for a submission that failed at token refresh is still on the ledger") + + setCommunityPDSURL(t, f.base.db, f.base.community.DID, healthyURL) + require.NoError(t, f.repo.UpdateCredentials(ctx, f.base.community.DID, original.PDSAccessToken, original.PDSRefreshToken)) + + resp, err := f.submit(t, f.base.community.DID, repeatable) + require.NoError(t, err, "the identical retry after the credentials recovered was refused, so the refresh-failure path leaked its reservation") + require.NotEmpty(t, resp.URI) + assert.Equal(t, 1, f.ledgerRows(t, f.base.community.DID)) +} + +// expiredJWT builds a structurally valid, long-expired JWT: enough for +// communities.NeedsRefresh (which parses the exp claim without verifying the +// signature) to answer "refresh this now". +func expiredJWT(t *testing.T) string { + t.Helper() + + header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"none","typ":"JWT"}`)) + payload := base64.RawURLEncoding.EncodeToString( + []byte(fmt.Sprintf(`{"exp":%d}`, time.Now().Add(-time.Hour).Unix()))) + return header + "." + payload + ".unverified" +} + +// An embed-enhancement failure (step 7) also runs with the reservation held. +// The thumb-must-be-a-blob guard is the reachable failure in that step without +// a network: it refuses the submission after admission, so the refusal must +// hand the slot back or the author's corrected retry meets a quota they never +// spent. +func TestService_AnEmbedEnhancementFailureReleasesTheReservation(t *testing.T) { + t.Parallel() + + f := newAdmissionFixture(t) + + title := "a link post with a malformed thumbnail" + content := "a body that makes this a complete post" + submitWithThumb := func(thumb interface{}) (*posts.CreatePostResponse, error) { + external := map[string]interface{}{ + "uri": "https://example.com/article", + "title": "An article", + "description": "worth reading", + } + if thumb != nil { + external["thumb"] = thumb + } + return f.service.CreatePost( + middleware.SetTestUserDID(context.Background(), f.base.author.DID), + posts.CreatePostRequest{ + Community: f.base.community.DID, + Title: &title, + Content: &content, + AuthorDID: f.base.author.DID, + Embed: map[string]interface{}{ + "$type": "social.coves.embed.external", + "external": external, + }, + }) + } + + // A thumb sent as a URL string passes the lexicon-shape validation of step + // 1 and is refused by the blob guard in step 7 — after admission. + _, err := submitWithThumb("https://example.com/thumb.jpg") + require.Error(t, err) + require.True(t, posts.IsValidationError(err), "the thumb guard reports a validation error, got: %v", err) + + assert.Zerof(t, f.ledgerRows(t, f.base.community.DID), + "the reservation for a submission refused by the embed guard is still on the ledger") + + // The corrected retry — same post, thumb omitted — must be admitted. + resp, err := submitWithThumb(nil) + require.NoError(t, err, "the corrected retry was refused, so the embed-guard path leaked its reservation") + require.NotEmpty(t, resp.URI) + assert.Equal(t, 1, f.ledgerRows(t, f.base.community.DID)) +} + +// The concurrent double-tap the reserve-then-confirm ordering exists to stop: +// two byte-identical submissions racing through CreatePost. The ledger's +// unique key is the only arbiter both goroutines share, so exactly one may be +// admitted; the loser must hear the DUPLICATE sentinel (its post exists — a +// 409), not a generic failure, and must not leave a second row behind. +func TestService_ConcurrentIdenticalSubmissionsAdmitExactlyOne(t *testing.T) { + t.Parallel() + + f := newAdmissionFixture(t) + + const doubleTap = "the same post, submitted twice at once" + var wg sync.WaitGroup + errs := make([]error, 2) + for i := range errs { + wg.Add(1) + go func(slot int) { + defer wg.Done() + _, errs[slot] = f.submit(t, f.base.community.DID, doubleTap) + }(i) + } + wg.Wait() + + winners, losers := 0, 0 + for _, err := range errs { + if err == nil { + winners++ + continue + } + losers++ + assert.ErrorIsf(t, err, posts.ErrDuplicateSubmission, + "the racing loser must hear the duplicate sentinel — its post exists — not %v", err) + } + assert.Equal(t, 1, winners, "exactly one of two identical concurrent submissions may be admitted") + assert.Equal(t, 1, losers) + + assert.Equal(t, 1, f.ledgerRows(t, f.base.community.DID), + "the race must leave exactly the winner's row: the unique key is the arbiter, not a second insert") +} + // communityPDSURL reads a community's stored PDS, so a test that repoints it // can put back what was actually there rather than what it assumed. func communityPDSURL(t *testing.T, db *sql.DB, communityDID string) string { diff --git a/internal/core/posts/service_aggregator_test.go b/internal/core/posts/service_aggregator_test.go index 3080b37..c94e052 100644 --- a/internal/core/posts/service_aggregator_test.go +++ b/internal/core/posts/service_aggregator_test.go @@ -22,8 +22,8 @@ import ( // // An aggregator is a service that writes into communities it does not belong // to, so the membership and visibility rules a human is held to say nothing -// useful about it. CreatePost swaps them for two others (service.go steps 3, 5 -// and 12): the community must have published an authorization record naming +// useful about it. CreatePost swaps them for two others (service.go steps 3, 4 +// and 9): the community must have published an authorization record naming // this aggregator, and the aggregator must be inside its hourly quota. Both are // checked BEFORE anything reaches the community's repository, and a successful // post is then recorded against the aggregator — which is what makes the next @@ -85,7 +85,10 @@ func newAggregatorFixture(t *testing.T) *aggregatorFixture { service: posts.NewPostService( postgres.NewPostRepository(base.db), base.communityService, aggregators.NewAggregatorService(index, base.communityService), - nil, nil, nil, base.pds.URL()), + nil, nil, nil, base.pds.URL(), + // The aggregator's OWN hourly quota is the subject here; the §8 + // per-author policy is opted out of explicitly. + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests())), index: index, aggregatorDID: aggregatorDID, authorizationURI: "at://" + base.community.DID + diff --git a/internal/core/posts/service_author_posts_query_test.go b/internal/core/posts/service_author_posts_query_test.go index 0ac97a4..7309f5c 100644 --- a/internal/core/posts/service_author_posts_query_test.go +++ b/internal/core/posts/service_author_posts_query_test.go @@ -96,7 +96,8 @@ func newAuthorPostsFixture(t *testing.T) *authorPostsFixture { communityRepo, pdsURL, fixtures.InstanceDID(), "", nil, nil, nil) // The optional post collaborators (aggregators, blobs, unfurl, bluesky) are // all write-path concerns and stay nil. - postService := posts.NewPostService(postRepo, communityService, nil, nil, nil, nil, pdsURL) + postService := posts.NewPostService(postRepo, communityService, nil, nil, nil, nil, pdsURL, + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests())) voteService := votes.NewServiceWithPDSFactory(voteRepo, nil, nil, fixtures.PasswordAuthPDSClientFactory()) auth := fixtures.NewOAuthMiddleware() diff --git a/internal/core/posts/service_create_validation_test.go b/internal/core/posts/service_create_validation_test.go index c7ef6c2..20d828f 100644 --- a/internal/core/posts/service_create_validation_test.go +++ b/internal/core/posts/service_create_validation_test.go @@ -61,7 +61,8 @@ func TestService_CreateResolvesTheCommunityAndValidatesTheRequest(t *testing.T) // in the community's handle. communityService := communities.NewCommunityServiceWithPDSFactory( communityRepo, pdsURL, instanceDID, instanceDomain, nil, nil, nil) - postService := posts.NewPostService(postRepo, communityService, nil, nil, nil, nil, pdsURL) + postService := posts.NewPostService(postRepo, communityService, nil, nil, nil, nil, pdsURL, + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests())) authorDID := fixtures.DID("postauthor") _, err := userService.CreateUser(ctx, users.CreateUserRequest{ diff --git a/internal/core/posts/service_writeforward_test.go b/internal/core/posts/service_writeforward_test.go index 01a2880..b30fa03 100644 --- a/internal/core/posts/service_writeforward_test.go +++ b/internal/core/posts/service_writeforward_test.go @@ -39,7 +39,7 @@ import ( // A post record does not live in its author's repo. It lives in the COMMUNITY's // repo, written with the community's own PDS credentials, carrying an `author` // field that names the human who wrote it (internal/core/posts/service.go step -// 9, and the reason the Jetstream consumer's first security check is +// 8, and the reason the Jetstream consumer's first security check is // repoDID == record.community). // // That makes two things testable only from the PDS side. First, that the @@ -125,7 +125,9 @@ func newPostFixture(t *testing.T) *postFixture { return &postFixture{ service: posts.NewPostService( postgres.NewPostRepository(db), communityService, - nil, nil, nil, nil, pdsServer.URL()), + nil, nil, nil, nil, pdsServer.URL(), + // Write-forward is not about admission; the opt-out is explicit. + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests())), pds: pdsServer, db: db, communityService: communityService, diff --git a/internal/core/unfurl/post_unfurl_integration_test.go b/internal/core/unfurl/post_unfurl_integration_test.go index d338cb0..d1b13dd 100644 --- a/internal/core/unfurl/post_unfurl_integration_test.go +++ b/internal/core/unfurl/post_unfurl_integration_test.go @@ -61,6 +61,7 @@ func TestPostUnfurl_UnsupportedURL(t *testing.T) { nil, // unfurlService - intentionally nil to test graceful handling nil, // blueskyService testkit.Endpoints().PDS.BaseURL, + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests()), ) // Create test user @@ -156,6 +157,7 @@ func TestPostUnfurl_MissingEmbedType(t *testing.T) { unfurlService, nil, // blueskyService testkit.Endpoints().PDS.BaseURL, + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests()), ) // Create test user and community diff --git a/internal/db/migrations/035_create_post_submissions.sql b/internal/db/migrations/035_create_post_submissions.sql index 07d77a9..8213073 100644 --- a/internal/db/migrations/035_create_post_submissions.sql +++ b/internal/db/migrations/035_create_post_submissions.sql @@ -24,6 +24,29 @@ -- The bucket is the index of the application's dedupe window, derived from the -- injected clock, so the key expires on its own without a sweeper. -- +-- THE BUCKET BOUNDARY IS A TRADEOFF, taken deliberately. Buckets are aligned +-- to the epoch, not to the submission, so a submission landing just before a +-- bucket edge is protected against an identical resubmission only until that +-- edge: the effective dedupe protection ranges over (0, window] depending on +-- where in the bucket the submission falls. A per-submission window would +-- protect for the full width every time, but expiring it would take a range +-- predicate or a sweeper; the epoch-aligned bucket lets the unique key expire +-- entirely on its own. +-- +-- A LEAKED RESERVATION IS BOUNDED BY THE SAME MECHANISM. If the process dies +-- between reserving the row and completing the PDS write, the orphaned row +-- burns one quota slot until it ages out of the rolling window, and refuses +-- identical content as a duplicate until the bucket rolls. There is no +-- sweeper to reclaim it — deliberate: the damage is bounded and self-healing, +-- and a reaper would be one more process able to disagree with the gate it +-- cleans. +-- +-- THIS TABLE GROWS WITHOUT BOUND. Confirmed rows are never deleted — only +-- releasing a reservation removes a row — so the ledger accumulates one row +-- per admitted post forever. Retention is a known, deliberate deferral: +-- docs/PRD_AUTHOR_OWNED_POSTS.md §8's retention item names post_submissions +-- alongside the pending/rejected admissions rows. +-- -- WHY NO FOREIGN KEYS. Migration 034 dropped posts.fk_author because a -- federated author has no `users` row (§5.3) — the AppView only bootstraps -- authors from trusted bridge PDSs today — and a community named by a diff --git a/tests/e2e/post_admission_contract_test.go b/tests/e2e/post_admission_contract_test.go new file mode 100644 index 0000000..194b850 --- /dev/null +++ b/tests/e2e/post_admission_contract_test.go @@ -0,0 +1,268 @@ +//go:build e2e + +package e2e + +import ( + "context" + "net/http" + "net/url" + "testing" + + "Coves/tests/testkit" + + "github.com/stretchr/testify/require" +) + +// The admission-wiring proof: cmd/server's PRODUCTION-constructed post service +// routes an authenticated social.coves.community.post.create through the §8 +// admission decision (PRD_AUTHOR_OWNED_POSTS, internal/core/posts/admit.go), +// observed end-to-end through the real XRPC surface. +// +// # THE CREDENTIAL, AND WHY IT IS AN AGGREGATOR'S +// +// This contract holds the first real write credential the tier has ever held. +// §3.4b's standing limitation still stands for USERS — nothing but the browser +// OAuth callback mints a sealed session token RequireAuth accepts — but +// post.create is the one Coves route behind DualAuth, and DualAuth's second +// path takes a PDS-signed service JWT from a REGISTERED AGGREGATOR. Every link +// of that chain is mintable inside the hermetic stack: +// +// - the aggregator is a PDS account (provisionAggregatorRepo), whose DID the +// hermetic PLC can resolve to a signing key; +// - it becomes REGISTERED by declaring social.coves.aggregator.service in +// its own repo, which only the firehose can index (aggregator contract's +// opening note) — so holding a working credential at all already proves +// pipeline delivery; +// - the JWT itself comes from the PDS' own com.atproto.server.getServiceAuth, +// signed with the account's repo key, audience'd to the AppView's instance +// DID — exactly what a production bot does. +// +// internal/api/routes/post_aggregator_test.go names this seam as the one it +// cannot reach ("needs the running stack and a token") and injects the +// principal instead. This file is that missing half: the shipped binary's +// DualAuthMiddleware validating a real signature against the hermetic PLC and +// gating on the firehose-fed aggregators table. +// +// # WHAT THIS PROVES ABOUT THE ADMISSION POLICY — AND WHAT IT CANNOT +// +// admitPost classifies this principal ActorRegisteredAggregator, so the checks +// it walks through the wire are community resolution (step 1), aggregator +// authorization (step 4) and the dedupe ledger (step 5). Three properties of +// the NEW decision are pinned here: +// +// - the decision is LIVE: the 403 flips to admitted when the community's +// authorization record arrives over the firehose, with no redeploy; +// - the check ORDER holds: an identical resubmission of a REFUSED submission +// answers the same refusal again, never 409 DuplicateSubmission — with the +// ledger wired, a decision that consulted dedupe ahead of authorization +// would answer 409 the second time (§8: a refusal consumes no quota); +// - reserve-then-release holds: a submission that is ADMITTED but whose PDS +// write then fails must hand its ledger slot back, so retrying it answers +// the write failure again, never 409 — a leaked reservation would turn one +// failed write into a lockout until the dedupe window rolls. +// +// The two USER-classified refusals — 403 Banned (step 3) and the per-author +// 429 RateLimitExceeded (step 6) — are structurally out of this tier's reach: +// they require an ActorUser principal, which requires the sealed-session mint +// that does not exist (§3.4b), and no aggregator credential is ever classified +// ActorUser. They are proven where they can be honestly: the decision matrix +// at T0 (internal/core/posts/admit_matrix_test.go, service_admission_test.go), +// the ledger against real Postgres at T1 (internal/db/postgres), and the +// refusal-to-status mapping at T0 (internal/api/handlers/post/errors_test.go). +// What none of those can see — the production construction in +// cmd/server/wiring.go actually enforcing the decision on the wire — is what +// this file adds. It carries NO ingestion marker: markers are for pipeline +// proofs (§3.4a), and this asserts the client path. +// +// # THE ADMITTED PATH'S KNOWN CEILING, STATED PLAINLY +// +// A community indexed from the firehose carries no PDS credentials in the +// AppView's store — only social.coves.community.create provisions those, and +// it sits behind the OAuth-only middleware this tier cannot satisfy. So an +// ADMITTED submission proceeds past every gate and then fails at the +// community-credential refresh (posts/service.go step 5, EnsureFreshToken on +// an empty token), which the mapper reports as a 500. The assertions below are +// written for the seam under test — refusal vs. admission — and the moment a +// credentialed community becomes reachable at T2, the same test upgrades +// itself to the full dedupe proof (the branch is written out below). +func TestPostAdmissionAPIContract(t *testing.T) { + p := newPipeline(t) + + moderator := p.IndexedAccount(t, "nm") + community := indexedCommunity(t, p, "n", moderator.DID) + aggregator, _ := indexedAggregator(t, p, "na") + + botToken := mintServiceJWT(t, aggregator) + asAggregator := p.AppView.As(botToken) + + // One submission, byte-identical on every attempt: the dedupe fingerprint + // hashes the record as the client sent it, so proving what repeats DON'T + // trigger requires the repeats to be genuine. + title := "admission " + testkit.UniqueID(t) + + t.Run("a service JWT from a DID that is no aggregator stops at the middleware", func(t *testing.T) { + // The security property internal/api/routes/post_aggregator_test.go + // documents as unprovable there: a VALID signature from a real, + // PLC-resolvable identity is still refused when the DID is not in the + // aggregators table. The moderator is exactly that — an indexed USER + // whose PDS mints service JWTs as willingly as anyone's. + // + // The message is asserted as well as the code, and it is load-bearing: + // the middleware answers 401 AuthenticationRequired for a broken + // signature too, and only the message tells "refused by the aggregator + // gate" from "the validator could not resolve the issuer" — the second + // would mean the stack's PLC plumbing is broken, not that the gate held. + err := submitPost(p.AppView.As(mintServiceJWT(t, moderator)), community.DID, title) + refusal := requireXRPCRefusal(t, err, http.StatusUnauthorized, "AuthenticationRequired", + "a non-aggregator's service JWT") + require.Equal(t, "Not a registered aggregator", refusal.XRPCMessage, + "the 401 must come from the aggregator gate, not from signature validation: %v", err) + }) + + t.Run("an unknown community is the decision's first refusal", func(t *testing.T) { + // Answering a POST-mapper refusal at all — not 401 — is the positive + // half of the credential proof: DualAuth validated the aggregator's JWT + // against the hermetic PLC, found the DID in the firehose-fed + // aggregators table, and let the request through to the service, where + // admitPost step 1 refused it. + // + // The DID literal is spelled at 24 base32 characters (a-z, 2-7) for the + // reason TestPostAPIContract gives: UniqueID does not promise that + // alphabet, and a malformed identifier would take the 400 validation + // path instead of the resolution path under test. Nothing indexes this + // DID, on a fresh stack or a kept one. + err := submitPost(asAggregator, "did:plc:aaaaaaaaaanevercommunity", title) + requireXRPCRefusal(t, err, http.StatusNotFound, "CommunityNotFound", + "a submission to a community nobody has indexed") + }) + + t.Run("an unauthorized aggregator is refused, and the refusal consumes nothing", func(t *testing.T) { + // The community exists and is indexed, but has written no authorization + // record for this aggregator — admitPost step 4, carried through the + // mapper as the aggregators-package 403. + err := submitPost(asAggregator, community.DID, title) + requireXRPCRefusal(t, err, http.StatusForbidden, "NotAuthorized", + "a submission from an aggregator the community never authorized") + + // The SAME submission again. §8's check order is observable right here: + // authorization runs AHEAD of dedupe, and a refusal reserves nothing — + // so the identical resubmission meets the identical 403. A decision + // that consulted the ledger first, or leaked a reservation on refusal, + // would answer 409 DuplicateSubmission instead, and this is the only + // tier that can catch the production wiring doing that. + err = submitPost(asAggregator, community.DID, title) + requireXRPCRefusal(t, err, http.StatusForbidden, "NotAuthorized", + "the identical resubmission of a refused submission — a 409 here means a refusal "+ + "consumed a dedupe slot, which §8 forbids") + }) + + // The community lets the aggregator in, the way production does: an + // authorization record in the COMMUNITY's own repo, delivered over the + // firehose. The wait observes the same table ValidateAggregatorPost reads. + community.PutRecord(t, aggregatorAuthorizationCollection, testkit.TID(), + aggregatorAuthorizationRecord(aggregator.DID, community.DID, moderator.DID, true)) + p.Await(t, "the authorization to reach the index the admission decision reads", func() (bool, error) { + enabled, err := p.Authorizations(context.Background(), aggregator.DID, true) + if err != nil { + return false, err + } + return len(enabled) == 1, nil + }) + + t.Run("the authorization's arrival flips the decision without a redeploy", func(t *testing.T) { + // Byte-identical to the submission refused twice above — so everything + // that changed between that 403 and this answer is the firehose-fed + // authorization row, which is the liveness of the decision in one + // assertion. Its two prior refusals reserved nothing, so this attempt's + // own reservation cannot collide with them. + err := submitPost(asAggregator, community.DID, title) + + if err == nil { + // The stack can complete a community-credentialed write — the + // admitted path ran to the PDS and back. The reservation is now + // CONFIRMED on the ledger, so the identical resubmission is the + // full dedupe proof. + err = submitPost(asAggregator, community.DID, title) + requireXRPCRefusal(t, err, http.StatusConflict, "DuplicateSubmission", + "an identical resubmission of an admitted post inside the dedupe window") + return + } + + // Today's ceiling (see the file comment): admission PASSED and the + // write then failed at the community-credential refresh, which no + // firehose-indexed community can satisfy. The mapper reports that + // unclassified failure as exactly one thing, and pinning it keeps this + // branch honest — any 4xx here would mean the admission gate refused, + // which is the regression this contract exists to catch. + requireXRPCRefusal(t, err, http.StatusInternalServerError, "InternalServerError", + "an ADMITTED submission failing at the community-credential refresh — any 4xx here "+ + "means the admission decision refused a submission the community has authorized") + + // And the failed write handed its ledger slot back: the identical + // retry meets the same write failure, never 409. A leaked reservation + // would refuse the retry as a duplicate of a post that does not exist — + // the §8 failure mode where a transient outage becomes a lockout. + err = submitPost(asAggregator, community.DID, title) + requireXRPCRefusal(t, err, http.StatusInternalServerError, "InternalServerError", + "the retry of a failed write — a 409 means the failed write's reservation was never "+ + "released, turning one PDS failure into a lockout until the dedupe window rolls") + }) +} + +// mintServiceJWT asks the stack's PDS to sign a service JWT for account, +// audience'd to the AppView's instance identity — the credential a production +// aggregator bot presents to post.create. +// +// The audience is communityInstanceDID for the reason that constant documents: +// INSTANCE_DID is unset in .env.ci, so the AppView's DualAuth validator was +// built with internal/config's compiled-in default, and a JWT for any other +// audience is refused before the signature is even consulted. lxm is pinned to +// the one route the token is spent on; the AppView validates service JWTs +// endpoint-agnostically (auth.go: lexMethod nil), so this is defence on the +// MINTING side — a leaked test token authorizes nothing else. +func mintServiceJWT(t *testing.T, account *testkit.Account) string { + t.Helper() + + var minted struct { + Token string `json:"token"` + } + err := account.XRPC().Query(context.Background(), "com.atproto.server.getServiceAuth", url.Values{ + "aud": {communityInstanceDID}, + "lxm": {"social.coves.community.post.create"}, + }, &minted) + if err != nil { + t.Fatalf("minting a service JWT for %s via com.atproto.server.getServiceAuth: %v", account.DID, err) + } + if minted.Token == "" { + t.Fatalf("com.atproto.server.getServiceAuth answered 200 with no token for %s", account.DID) + } + return minted.Token +} + +// submitPost drives social.coves.community.post.create as the holder of +// client's credential, with the minimal well-formed body the lexicon requires. +// The transport error is returned rather than asserted: half of this contract +// is about which refusal comes back. +func submitPost(client *testkit.AppView, community, title string) error { + return client.Procedure(context.Background(), "social.coves.community.post.create", map[string]any{ + "community": community, + "title": title, + "content": "submitted through the admission gate", + }, nil) +} + +// requireXRPCRefusal asserts err is an XRPC error envelope with exactly this +// status and error name, and returns it for callers that assert further. The +// name is asserted as well as the status because it is the machine-readable +// half clients switch on: a 403 NotAuthorized tells an aggregator to stop, a +// 403 with any other name tells it nothing. +func requireXRPCRefusal(t *testing.T, err error, status int, code, what string) *testkit.StatusError { + t.Helper() + + var se *testkit.StatusError + require.ErrorAsf(t, err, &se, + "%s must be refused with an XRPC error envelope, got: %v", what, err) + require.Equalf(t, status, se.StatusCode, "%s: answered %v", what, err) + require.Equalf(t, code, se.XRPCError, "%s: answered %v", what, err) + return se +} diff --git a/tests/live/post_unfurl_test.go b/tests/live/post_unfurl_test.go index 7b8ed2f..5cebc3f 100644 --- a/tests/live/post_unfurl_test.go +++ b/tests/live/post_unfurl_test.go @@ -223,6 +223,7 @@ func TestPostUnfurl_UserProvidedMetadata(t *testing.T) { unfurlService, nil, // blueskyService pdsURL, + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests()), ) // Create test user and community