diff --git a/internal/api/routes/registration_test.go b/internal/api/routes/registration_test.go index 1b262f6..113926f 100644 --- a/internal/api/routes/registration_test.go +++ b/internal/api/routes/registration_test.go @@ -175,11 +175,18 @@ var declaredRoutes = []declaredRoute{ {http.MethodPost, "/xrpc/social.coves.community.unblockCommunity", authRequired, 0, false}, // RegisterPostRoutes — social.coves.community.post.* - // The two writes take DualAuthMiddleware in production (OAuth users OR + // The three writes take DualAuthMiddleware in production (OAuth users OR // aggregator service JWTs); the guard is RequireAuth either way, which is // what this table can see. Which principals that RequireAuth accepts is // post_aggregator_test.go's subject. {http.MethodPost, "/xrpc/social.coves.community.post.create", authRequired, 0, false}, + // update arrived with the write-path flip (PRD §4.2): a post lives in its + // author's repo now, so editing one is a write this AppView can actually + // perform. It is declared exactly like create beside it — the same guard, + // no limiter of its own — because it is the same kind of write by the same + // principals, and an edit that was cheaper to call than the post it edits + // would be the obvious way to spend a quota the create path meters. + {http.MethodPost, "/xrpc/social.coves.community.post.update", authRequired, 0, false}, {http.MethodPost, "/xrpc/social.coves.community.post.delete", authRequired, 0, false}, {http.MethodGet, "/xrpc/social.coves.community.post.get", authOptional, 0, false}, // getStatus takes no auth at all, unlike post.get beside it, and the diff --git a/internal/core/posts/admit_matrix_test.go b/internal/core/posts/admit_matrix_test.go index 0cbade7..d3093d3 100644 --- a/internal/core/posts/admit_matrix_test.go +++ b/internal/core/posts/admit_matrix_test.go @@ -1114,6 +1114,26 @@ func TestErrDuplicateSubmissionIsNotAStorageConflict(t *testing.T) { func TestSubmissionFingerprint(t *testing.T) { t.Parallel() + // THE FINGERPRINT STILL HASHES A PostRecord, DELIBERATELY, EVEN THOUGH THE + // RECORD THAT GETS WRITTEN IS A PostV2Record NOW. + // + // The write path converts once at the boundary (postV2From), and admission + // runs on the pre-conversion shape. That ordering is not an oversight left + // over from the flip — it is what keeps the fingerprint BYTE-STABLE across + // the deploy. The value hashed here is the dedupe key stored in + // post_submissions, so retyping it would silently repartition every live + // ledger row: an author mid-retry when the new binary rolls would have their + // retry hash differently, miss its own reservation, and be admitted as a + // second post — the exact duplicate §4.2's deterministic rkey exists to + // close, reintroduced by the migration meant to close it. + // + // The cost is that the two shapes have to be kept in step by hand: a field + // added to PostV2Record and not to PostRecord is a field the fingerprint + // cannot see, so two posts differing only in it would collide and the second + // would be refused as a repeat. TASK 8 REVISIT — it retires the deprecated + // collection and re-materializes the live rows, which is the one moment a + // fingerprint change is free, and the point at which this converter and + // PostRecord itself should both go away. base := func() PostRecord { title, content := "A title", "Some body text" return PostRecord{ diff --git a/internal/core/posts/blob_transform_test.go b/internal/core/posts/blob_transform_test.go index b7636fa..c0e6329 100644 --- a/internal/core/posts/blob_transform_test.go +++ b/internal/core/posts/blob_transform_test.go @@ -13,6 +13,15 @@ const ( embedCommunityDID = "did:plc:testcommunity" embedCommunityPDS = "http://localhost:3001" // coves:allow-host-literal: expected-output fixture for a pure string transform; never dialled embedBlobCID = "bafyreib6tbnql2ux3whnfysbzabthaj2vvck53nimhbi5g5a7jgvgr5eqm" + + embedAuthorDID = "did:plc:testauthor" + embedAuthorPDS = "http://localhost:3002" // coves:allow-host-literal: expected-output fixture for a pure string transform; never dialled + + // The two record URIs the transform has to tell apart. Their COLLECTION is + // the whole signal: it says which repository the record lives in, and + // therefore which repository holds its blobs. + embedLegacyURI = "at://" + embedCommunityDID + "/social.coves.community.post/3lrc77gmww4nc" + embedPostV2URI = "at://" + embedAuthorDID + "/" + PostV2Collection + "/3lrc77gmww4nc" ) // withImageProxy enables the process-wide image proxy for one test. The @@ -38,9 +47,19 @@ func testBlobRef() map[string]interface{} { } } +// externalEmbedPost is a DEPRECATED community-repo post: its record lives in +// the community's repository, so the community owns its blobs. +// +// The URI is set explicitly rather than left empty, which is how these fixtures +// used to read. An owner chosen per record is chosen FROM the URI, so a fixture +// without one would be asserting the transform's fallback while claiming to +// asserting the community case — and would keep passing if the selection broke +// in exactly the direction that matters. func externalEmbedPost() *PostView { return &PostView{ + URI: embedLegacyURI, Community: &CommunityRef{DID: embedCommunityDID, PDSURL: embedCommunityPDS}, + Author: &AuthorView{DID: embedAuthorDID, PDSURL: embedAuthorPDS}, Embed: map[string]interface{}{ "$type": "social.coves.embed.external", "external": map[string]interface{}{ @@ -51,6 +70,14 @@ func externalEmbedPost() *PostView { } } +// authorOwnedEmbedPost is the same post as a postv2 record: it lives in the +// AUTHOR's repository, so the author owns its blobs. +func authorOwnedEmbedPost() *PostView { + post := externalEmbedPost() + post.URI = embedPostV2URI + return post +} + func TestTransformBlobRefsToURLs(t *testing.T) { t.Run("an external thumb is served from the image proxy under the community DID", func(t *testing.T) { withImageProxy(t) @@ -62,9 +89,12 @@ func TestTransformBlobRefsToURLs(t *testing.T) { assert.Equal(t, "social.coves.embed.external#view", embedMap["$type"]) external := embedMap["external"].(map[string]interface{}) - // The AppView signs community posts into the community's repo and - // uploads their blobs there, so the community DID owns the blob no - // matter who authored the post. + // A DEPRECATED community.post record: the AppView signed it into the + // community's repo and uploaded its blobs there, so the community DID + // owns the blob no matter who authored the post. Every such record + // standing in production today still resolves this way, and will until + // task 8 re-materializes them — which is why this case survives the + // flip rather than being replaced by it. assert.Equal(t, "https://img.coves.social/img/embed_thumbnail/plain/"+embedCommunityDID+"/"+embedBlobCID, external["thumb"]) @@ -74,6 +104,7 @@ func TestTransformBlobRefsToURLs(t *testing.T) { withImageProxy(t) post := &PostView{ + URI: embedLegacyURI, Community: &CommunityRef{DID: embedCommunityDID, PDSURL: embedCommunityPDS}, Embed: map[string]interface{}{ "$type": "social.coves.embed.images", @@ -202,3 +233,135 @@ func TestTransformBlobRefsToURLs_ProxyDisabled(t *testing.T) { blobs.HydrateBlobURL(embedCommunityPDS, embedCommunityDID, embedBlobCID), external["thumb"]) } + +// Which repository a post's blobs live in, now that the answer depends on the +// record (docs/PRD_AUTHOR_OWNED_POSTS.md §3.1, §4.2). +// +// Before the write-path flip there was one answer for every post: the AppView +// signed the record into the COMMUNITY's repo and uploaded its blobs there, so +// the community DID owned the media whoever had written it. A postv2 record +// lives in its AUTHOR's repo and its blobs are uploaded under the author's own +// session, so the author DID owns them — and the two kinds of record are indexed +// into ONE table and served by ONE read path. +// +// So the owner has to be chosen PER RECORD, and the only thing that says which +// is the URI's collection. Getting it wrong is not a crash: it builds a +// perfectly well-formed image URL naming a repository that does not hold the +// blob, which renders as a broken image for every reader and looks from the +// server side like everything worked. +// +// The transform is pure, so this is where the choice belongs. What a pure test +// CANNOT prove is that AuthorView.PDSURL is populated at all on the real read +// path — the repository has always selected the column and dropped it — and that +// is pinned at T1 in service_blob_test.go. +func TestTransformBlobRefsToURLs_ChoosesTheOwningRepoPerRecord(t *testing.T) { + t.Run("a postv2 record's blobs are served under the AUTHOR's DID", func(t *testing.T) { + withImageProxy(t) + + post := authorOwnedEmbedPost() + TransformBlobRefsToURLs(post) + + external := post.Embed.(map[string]interface{})["external"].(map[string]interface{}) + assert.Equal(t, + "https://img.coves.social/img/embed_thumbnail/plain/"+embedAuthorDID+"/"+embedBlobCID, + external["thumb"], + "a postv2 post's media was uploaded to the author's PDS under the author's session, so a "+ + "URL built under the community DID names a repo that has never held this blob") + }) + + t.Run("the two collections resolve to different repos for the same blob", func(t *testing.T) { + withImageProxy(t) + + legacy := externalEmbedPost() + authored := authorOwnedEmbedPost() + TransformBlobRefsToURLs(legacy) + TransformBlobRefsToURLs(authored) + + legacyThumb := legacy.Embed.(map[string]interface{})["external"].(map[string]interface{})["thumb"] + authoredThumb := authored.Embed.(map[string]interface{})["external"].(map[string]interface{})["thumb"] + + // Asserted as a PAIR because the failure this catches is a transform + // that reads the URI but answers the same owner either way — which each + // case above would still catch, but only if both are kept. Stating the + // difference directly means the day someone "simplifies" the selection + // back to a constant, this fails with a message that says what was lost. + assert.NotEqual(t, legacyThumb, authoredThumb, + "the same blob CID under the two collections must resolve to different repositories: "+ + "one is in the community's repo and the other is in the author's") + }) + + t.Run("proxy disabled, a postv2 record addresses the AUTHOR's PDS", func(t *testing.T) { + // The self-hosted opt-out builds a direct getBlob URL, which needs the + // HOST as well as the DID — so this is the case where a wrong owner + // stops being a wrong path segment and becomes a request to the wrong + // server entirely. + blobs.ResetImageURLConfigForTesting() + blobs.SetImageURLConfig(blobs.ImageURLConfig{ProxyEnabled: false}) + t.Cleanup(blobs.ResetImageURLConfigForTesting) + + post := authorOwnedEmbedPost() + TransformBlobRefsToURLs(post) + + external := post.Embed.(map[string]interface{})["external"].(map[string]interface{}) + assert.Equal(t, + blobs.HydrateBlobURL(embedAuthorPDS, embedAuthorDID, embedBlobCID), + external["thumb"]) + }) + + t.Run("a postv2 record with no author is left unprojected", func(t *testing.T) { + withImageProxy(t) + + // FAIL CLOSED. Falling back to the community here would be the most + // tempting repair and the worst one: it produces a confident URL under + // a DID that has never held the blob, so the reader gets a broken image + // and the server logs nothing. Leaving the blob ref in place is visibly + // wrong to the client and matches what the transform already does for a + // post with no community (the case above). + post := authorOwnedEmbedPost() + post.Author = nil + + TransformBlobRefsToURLs(post) + + external := post.Embed.(map[string]interface{})["external"].(map[string]interface{}) + thumb, ok := external["thumb"].(map[string]interface{}) + require.True(t, ok, + "with no author there is no owning repo for a postv2 record's blobs, so none may be invented") + assert.Equal(t, "blob", thumb["$type"]) + }) + + t.Run("a postv2 record whose author has no PDS URL still proxies by DID", func(t *testing.T) { + withImageProxy(t) + + // The mirror of the community case beside it: the proxy resolves the DID + // itself, so the host is only needed for the proxy-disabled fallback. An + // author whose pds_url column is empty must not lose their images. + post := authorOwnedEmbedPost() + post.Author.PDSURL = "" + + TransformBlobRefsToURLs(post) + + external := post.Embed.(map[string]interface{})["external"].(map[string]interface{}) + assert.Equal(t, + "https://img.coves.social/img/embed_thumbnail/plain/"+embedAuthorDID+"/"+embedBlobCID, + external["thumb"]) + }) + + t.Run("a view with no URI falls back to the community", func(t *testing.T) { + withImageProxy(t) + + // Not a production shape — every indexed post has a URI — but the + // selection has to answer SOMETHING, and the community is the safe + // answer: it is what every record predating the flip resolves to, so an + // unattributable view degrades to the old behaviour rather than to a + // confident claim about an author's repo. + post := externalEmbedPost() + post.URI = "" + + TransformBlobRefsToURLs(post) + + external := post.Embed.(map[string]interface{})["external"].(map[string]interface{}) + assert.Equal(t, + "https://img.coves.social/img/embed_thumbnail/plain/"+embedCommunityDID+"/"+embedBlobCID, + external["thumb"]) + }) +} diff --git a/internal/core/posts/post.go b/internal/core/posts/post.go index 6e3e78d..352f08a 100644 --- a/internal/core/posts/post.go +++ b/internal/core/posts/post.go @@ -284,6 +284,17 @@ type AuthorView struct { Reputation *int `json:"reputation,omitempty"` DID string `json:"did"` Handle string `json:"handle"` + + // PDSURL is the author's PDS, not exposed to the API — the mirror of + // CommunityRef.PDSURL beside it, and needed for the same reason: a postv2 + // post's blobs live in the AUTHOR's repository, so building their URLs + // means knowing which server holds them. + // + // The repository query has always SELECTed this column (it hydrates the + // author's avatar) and always dropped it here. Carrying it is what lets the + // blob transform pick an owner per record instead of assuming every post's + // media is the community's. + PDSURL string `json:"-"` } // CommunityRef represents minimal community info in post views diff --git a/internal/core/posts/service_aggregator_test.go b/internal/core/posts/service_aggregator_test.go index 30eb9de..129479a 100644 --- a/internal/core/posts/service_aggregator_test.go +++ b/internal/core/posts/service_aggregator_test.go @@ -4,6 +4,7 @@ package posts_test import ( "context" + "fmt" "testing" "time" @@ -227,13 +228,25 @@ func TestService_RevokingAnAuthorizationStopsTheNextPost(t *testing.T) { // worth the cost here because the count they are checked against is the one // CreatePost itself writes: this is the only test in the tree where the // producer and the consumer of that counter are the same code path. +// +// THE TITLES ARE DISTINCT, AND THAT IS NOT COSMETIC. This loop used to submit +// ten copies of one string, which worked only while the record key came from +// the clock: ten identical submissions produced ten records because each got a +// fresh TID. Under the write-path flip the key is derived from the submission's +// own content (§4.2), so ten identical submissions converge on ONE record — the +// second through tenth find their own post already standing and are reported +// back the URI of the first, which is the designed idempotence and not a bug to +// route around. The quota, meanwhile, is metered per ACCEPTED submission, so the +// old loop would still reach ten and then assert the eleventh was refused while +// exactly one post existed. Distinct titles keep the premise honest: ten +// submissions, ten posts, ten metered, and the eleventh refused. func TestService_AggregatorQuotaStopsTheEleventhPost(t *testing.T) { t.Parallel() f := newAggregatorFixture(t) for i := 0; i < aggregators.RateLimitMaxPosts; i++ { - _, err := f.createPost(t, "syndicated item") + _, err := f.createPost(t, fmt.Sprintf("syndicated item %d", i)) require.NoErrorf(t, err, "post %d of %d was refused inside the quota", i+1, aggregators.RateLimitMaxPosts) } require.Equal(t, aggregators.RateLimitMaxPosts, f.postsThisHour(t)) diff --git a/internal/core/posts/service_blob_test.go b/internal/core/posts/service_blob_test.go new file mode 100644 index 0000000..a0d6f0a --- /dev/null +++ b/internal/core/posts/service_blob_test.go @@ -0,0 +1,377 @@ +//go:build integration + +package posts_test + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "Coves/internal/api/middleware" + "Coves/internal/core/blobs" + "Coves/internal/core/posts" + "Coves/internal/core/unfurl" + "Coves/internal/db/postgres" + "Coves/tests/testkit" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Where a post's MEDIA ends up, now that the post itself lives in the author's +// repository (docs/PRD_AUTHOR_OWNED_POSTS.md §4.2 step 2). +// +// # WHY THE BLOB CANNOT BE LEFT BEHIND +// +// The record moved and the blob has to move with it. A postv2 record in the +// author's repo whose thumb blob was uploaded to the COMMUNITY's PDS is a record +// pointing at media in a repository it has no relationship to, and every failure +// that produces is quiet: +// +// - The blob ref in the record names a CID, not a repo. A reader resolves it +// against the repo it believes owns the record — the author's — and gets +// nothing back. A broken image, no error, nothing in a log. +// - The community can garbage-collect a blob no record in ITS repo references. +// atProto blobs are reference-counted per repository, so an orphan upload is +// media on borrowed time in someone else's storage. +// - Deleting the post deletes the record and dereferences nothing, because the +// blob was never the author's to release. An author who deletes a post keeps +// paying for its images in a repo they cannot reach. +// +// # WHAT MUST SURVIVE THE MOVE +// +// UploadBlobFromURL is a CHOKE POINT, not a convenience: it bounds the fetch +// with a timeout, refuses a Content-Type outside the image allowlist, and caps +// the body at 6MB. Every one of those exists because the URL being fetched is +// attacker-influenced — a client picks the page that gets unfurled, and the page +// picks the thumbnail. A new upload path that reached the PDS without them would +// turn a link preview into an unbounded fetch performed by the AppView, with the +// author's own credentials, into the author's own storage quota. +// +// So this file pins two things: that the blob lands in the AUTHOR's repo, and +// that it still has to get past the guard to land anywhere. + +// blobFixture is the write path wired with a real blob service and an unfurl +// service the test scripts. +// +// UNFURL IS THE ROUTE IN because it is the one a regular user takes. The other +// producer of a thumbnail — CreatePostRequest.ThumbnailURL — is only honoured for +// a TRUSTED aggregator, and trust is read from the process environment, which +// t.Setenv cannot be combined with t.Parallel. The unfurl path exercises the +// same UploadBlobFromURL call with no such constraint. +type blobFixture struct { + base *postFixture + service posts.Service + + // origin serves the images the "remote page" offers as its thumbnail. + origin *imageOrigin +} + +// imageOrigin is the remote host a thumbnail is fetched from — the untrusted +// end of the choke point. +type imageOrigin struct { + server *httptest.Server + + // body and contentType are what the next fetch receives. + body []byte + contentType string +} + +func newImageOrigin(t *testing.T) *imageOrigin { + t.Helper() + + origin := &imageOrigin{ + body: testkit.TestPNG(64, 64), + contentType: "image/png", + } + origin.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", origin.contentType) + w.WriteHeader(http.StatusOK) + _, _ = w.Write(origin.body) + })) + t.Cleanup(origin.server.Close) + return origin +} + +func (o *imageOrigin) thumbnailURL() string { return o.server.URL + "/thumb.png" } + +// scriptedUnfurl reports every URL as supported and answers with a thumbnail +// pointing at the origin. The unfurl domain has its own tests; what matters here +// is only that a thumbnail URL reaches the blob service. +type scriptedUnfurl struct { + thumbnailURL string +} + +func (s *scriptedUnfurl) IsSupported(string) bool { return true } + +func (s *scriptedUnfurl) UnfurlURL(context.Context, string) (*unfurl.UnfurlResult, error) { + return &unfurl.UnfurlResult{ + Type: "article", + Title: "An article with a picture", + Description: "unfurled", + ThumbnailURL: s.thumbnailURL, + Provider: "example", + Domain: "example.com", + }, nil +} + +func newBlobFixture(t *testing.T) *blobFixture { + t.Helper() + + base := newPostFixture(t) + origin := newImageOrigin(t) + + return &blobFixture{ + base: base, + origin: origin, + service: posts.NewPostService( + postgres.NewPostRepository(base.db), base.communityService, + nil, // aggregators + blobs.NewBlobService(base.pds.URL()), + &scriptedUnfurl{thumbnailURL: origin.thumbnailURL()}, + nil, // bluesky + base.pds.URL(), + append(base.writePathOptions(), + posts.WithAdmissionPolicy(posts.NewAllowAllAdmissionPolicyForTests()))...), + } +} + +// postWithExternalEmbed submits a post carrying an external embed, which is what +// makes the unfurl-and-upload path run at all. +func (f *blobFixture) postWithExternalEmbed(t *testing.T, title string) (*posts.CreatePostResponse, error) { + t.Helper() + + content := "a post whose link preview has a picture" + return f.service.CreatePost( + middleware.SetTestUserDID(context.Background(), f.base.author.DID), + sessionFor(t, f.base.author, f.base.pds.URL()), + 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": map[string]interface{}{ + "uri": "https://example.com/an-article", + }, + }, + }) +} + +// thumbCIDOf digs the blob CID out of a written record's external embed, or +// reports that the record carries no thumb at all. +func thumbCIDOf(t *testing.T, record testkit.RecordValue) (string, bool) { + t.Helper() + + embed, ok := record.Value["embed"].(map[string]any) + if !ok { + return "", false + } + external, ok := embed["external"].(map[string]any) + if !ok { + return "", false + } + thumb, ok := external["thumb"].(map[string]any) + if !ok { + return "", false + } + ref, ok := thumb["ref"].(map[string]any) + require.Truef(t, ok, "the thumb is not a blob reference: %#v", thumb) + link, ok := ref["$link"].(string) + require.Truef(t, ok, "the blob reference has no $link: %#v", ref) + return link, true +} + +// listBlobs returns the CIDs of every blob held in an account's repository — +// com.atproto.sync.listBlobs, which is the PDS's own answer to "whose storage is +// this in" and cannot be satisfied by a record that merely mentions a CID. +func listBlobs(t *testing.T, account *testkit.Account) []string { + t.Helper() + + var resp struct { + CIDs []string `json:"cids"` + } + require.NoError(t, account.XRPC().Query(context.Background(), "com.atproto.sync.listBlobs", + map[string][]string{ + "did": {account.DID}, + "limit": {"100"}, + }, &resp), + "listing the blobs of %s", account.DID) + return resp.CIDs +} + +func TestService_AThumbnailBlobLandsInTheAuthorsRepo(t *testing.T) { + t.Parallel() + + f := newBlobFixture(t) + resp, err := f.postWithExternalEmbed(t, "a link with a preview image") + require.NoError(t, err) + + record := f.base.author.GetRecord(t, posts.PostV2Collection, rkeyOf(t, resp.URI)) + thumbCID, hasThumb := thumbCIDOf(t, record) + require.Truef(t, hasThumb, + "the unfurled thumbnail never made it onto the record; embed was %#v", record.Value["embed"]) + + // THE BLOB IS IN THE AUTHOR'S STORAGE. Asserted through listBlobs rather + // than by re-reading the record, because the record only names a CID: a blob + // uploaded to the community's PDS produces a record that looks exactly like + // this one and resolves for nobody. + assert.Containsf(t, listBlobs(t, f.base.author), thumbCID, + "the post's thumbnail blob is not in the author's repository; the record that references it "+ + "lives there, so a reader resolving this CID against the author's PDS gets nothing back") + + // AND NOT IN THE COMMUNITY'S. The mirror assertion, for the same reason the + // write-forward test lists both repos: a path that uploaded to both would + // satisfy the check above while still orphaning a blob in storage the author + // cannot release when they delete the post. + assert.NotContainsf(t, listBlobs(t, f.base.communityAccount(t)), thumbCID, + "the thumbnail was uploaded into the community's repository, where no record references it — "+ + "the community can garbage-collect it, and deleting the post will not release it") +} + +func TestService_TheBlobGuardStillRefusesAnOversizeThumbnail(t *testing.T) { + t.Parallel() + + // THE CHOKE POINT HAS TO SURVIVE THE MOVE. The URL being fetched here is + // attacker-influenced twice over — a client chooses the page, the page + // chooses the thumbnail — and the 6MB cap is what stops a link preview + // becoming an unbounded fetch the AppView performs with the author's own + // credentials into the author's own storage quota. + // + // The body is not a real image: the guard rejects on the Content-Type header + // and the byte count, neither of which requires decoding, so 7MB of zeros + // exercises the same branch as 7MB of PNG without the cost of producing one. + f := newBlobFixture(t) + f.origin.body = make([]byte, 7*1024*1024) + + resp, err := f.postWithExternalEmbed(t, "a link whose preview image is enormous") + + // The POST still succeeds. A thumbnail is an enhancement, and it has never + // been able to fail a post — the write path logs the upload failure and + // carries on. That stays true on the author path: an author must not lose a + // post because a remote page served a 7MB image. + require.NoErrorf(t, err, + "a refused thumbnail must not fail the post; the record is the author's and it is complete without it") + + record := f.base.author.GetRecord(t, posts.PostV2Collection, rkeyOf(t, resp.URI)) + _, hasThumb := thumbCIDOf(t, record) + assert.Falsef(t, hasThumb, + "the oversize image was uploaded and attached anyway — the size cap did not survive the move to "+ + "the author's PDS") + + // And nothing reached storage. Checked separately from the record because + // the two failures are different: a record with no thumb but a blob in the + // repo means the upload SUCCEEDED and only the attachment was dropped, which + // is the cap being enforced in the wrong place and paying the full cost of + // not having it. + assert.Emptyf(t, listBlobs(t, f.base.author), + "the oversize image reached the author's storage; the cap must refuse it before the upload, "+ + "not discard the result afterwards") +} + +func TestService_TheBlobGuardStillRefusesANonImageThumbnail(t *testing.T) { + t.Parallel() + + // The other half of the guard. An allowlist that admitted arbitrary + // Content-Types would let a remote page put anything it liked into the + // author's repository — the AppView fetching it, the author's session + // signing for it, and the image proxy later serving it to readers. + f := newBlobFixture(t) + f.origin.contentType = "text/html" + f.origin.body = []byte("not an image at all") + + resp, err := f.postWithExternalEmbed(t, "a link whose preview is not an image") + require.NoError(t, err) + + record := f.base.author.GetRecord(t, posts.PostV2Collection, rkeyOf(t, resp.URI)) + _, hasThumb := thumbCIDOf(t, record) + assert.False(t, hasThumb, "a text/html body was accepted as a thumbnail") + + assert.Empty(t, listBlobs(t, f.base.author), + "a non-image body reached the author's storage") +} + +func TestService_ThePostSurvivesAThumbnailOriginThatIsDown(t *testing.T) { + t.Parallel() + + // The ordinary operational case, pinned because the author path makes it + // newly dangerous to get wrong. A dead thumbnail host used to cost the + // COMMUNITY's write nothing; it must equally cost the AUTHOR's write + // nothing, or a remote page being down becomes a reason a person cannot + // post. + f := newBlobFixture(t) + f.origin.server.Close() + + resp, err := f.postWithExternalEmbed(t, "a link whose preview host is unreachable") + require.NoErrorf(t, err, "an unreachable thumbnail host must not fail the post") + + record := f.base.author.GetRecord(t, posts.PostV2Collection, rkeyOf(t, resp.URI)) + _, hasThumb := thumbCIDOf(t, record) + assert.False(t, hasThumb) + + // The rest of the unfurl still landed: the post keeps the metadata that did + // not depend on the fetch. + embed, ok := record.Value["embed"].(map[string]any) + require.True(t, ok) + external, ok := embed["external"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "An article with a picture", external["title"], + "the thumbnail failed, not the unfurl — the metadata that needed no fetch must survive") +} + +// blobOwnerReadPath is the read-side half of the same question, and the half a +// pure test cannot reach. +// +// blob_transform_test.go pins WHICH owner a record's blobs resolve against. +// What it cannot pin is that the author's PDS URL is populated at all: the +// repository query has always SELECTed users.pds_url — it hydrates the author's +// avatar with it — and has always dropped it on the floor before the view is +// returned. An owner-selection that is perfectly correct over a field nobody +// fills in resolves every postv2 post's media against an empty host. +func TestService_AuthorPDSIsHydratedOntoPostViews(t *testing.T) { + t.Parallel() + + f := newPostFixture(t) + ctx := context.Background() + + // Indexed the way the firehose consumer would, rather than by posting: the + // subject is the READ path, and a real write would leave the row waiting on + // a consumer this fixture does not run. + rkey := testkit.TID() + uri := "at://" + f.author.DID + "/" + posts.PostV2Collection + "/" + rkey + _, err := f.db.ExecContext(ctx, ` + INSERT INTO users (did, handle, pds_url, created_at) + VALUES ($1, $2, $3, NOW()) + ON CONFLICT (did) DO UPDATE SET pds_url = EXCLUDED.pds_url + `, f.author.DID, f.author.DID+".test", f.pds.URL()) + require.NoError(t, err) + + _, err = f.db.ExecContext(ctx, ` + INSERT INTO posts (uri, cid, rkey, author_did, community_did, title, created_at) + VALUES ($1, $2, $3, $4, $5, $6, NOW()) + `, uri, "bafyblobowner", rkey, f.author.DID, f.community.DID, "a post with media") + require.NoError(t, err) + + views, err := postgres.NewPostRepository(f.db).GetViewsByURIs(ctx, []string{uri}) + require.NoError(t, err) + require.Contains(t, views, uri) + + author := views[uri].Author + require.NotNil(t, author) + assert.Equal(t, f.author.DID, author.DID) + assert.Equalf(t, f.pds.URL(), author.PDSURL, + "the author's PDS URL is not hydrated onto the view, so a postv2 post's blob URLs would be "+ + "built against an empty host — the column is already SELECTed for the avatar and only "+ + "needs carrying") + + // The community's stays populated beside it. The two owners coexist in one + // view because one table holds both kinds of record, and a change that + // carried the author's host by overwriting the community's would break every + // pre-flip post's media instead. + require.NotNil(t, views[uri].Community) + assert.NotEmpty(t, views[uri].Community.PDSURL, + "the community's PDS URL must survive: deprecated community.post records still resolve their "+ + "blobs against it") +} diff --git a/tests/e2e/post_admission_contract_test.go b/tests/e2e/post_admission_contract_test.go index 194b850..8da8f61 100644 --- a/tests/e2e/post_admission_contract_test.go +++ b/tests/e2e/post_admission_contract_test.go @@ -56,10 +56,10 @@ import ( // 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. +// - reserve-then-release holds: a submission that is ADMITTED but which then +// fails before the record exists must hand its ledger slot back, so retrying +// it answers the same failure again, never 409 — a leaked reservation would +// turn one transient failure 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: @@ -76,15 +76,33 @@ import ( // // # 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). +// An admitted submission gets past every gate and then stops, and the write-path +// flip MOVED where. It used to stop at the community-credential refresh: a +// community indexed from the firehose carries no PDS credentials in the +// AppView's store, so EnsureFreshToken on an empty token failed and the mapper +// reported an unclassified 500. +// +// A post is written to its AUTHOR's repository now (PRD §4.2 step 3), so the +// credential that has to exist is the AUTHOR's, and this tier cannot mint one +// for the same reason §3.4b gives for everything else. The two ways to hold one +// are a browser OAuth session (which needs the sealed-session mint that does not +// exist here) or an aggregator's STORED tokens from migration 025 (which are +// written by that same OAuth grant, performed once by a human operator). A +// service JWT authenticates the caller to the AppView; it is not a repo +// credential and was never meant to be one. +// +// So the ceiling is now ErrNoAuthorCredentials, which the mapper reports as a +// NAMED 503 — and that is a strictly better probe than the 500 it replaces. A +// 500 was the absence of a classification: it would have been answered just as +// readily by a nil-pointer panic three layers down. A 503 NoAuthorCredentials is +// a specific outcome that can only be produced at one place in the write path, +// so reaching it proves the request travelled past admission, past the actor +// classification, past the ledger reservation, and all the way to the author-repo +// open. Any 4xx means the admission gate refused something it had authorized; +// any 500 now means something unclassified broke. +// +// The moment a credentialed author 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) @@ -178,34 +196,46 @@ func TestPostAdmissionAPIContract(t *testing.T) { 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. + // The stack can complete an author-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") + // Today's ceiling (see the file comment): admission PASSED and the write + // then stopped at the AUTHOR-repo open, because no principal this tier + // can mint holds a repo credential. The status and the NAME are both + // pinned, and the name is what makes this branch worth having — it is + // produced at exactly one place in the write path, so meeting it proves + // the submission travelled past every gate this contract is about. + // + // The two ways this assertion fails are the two regressions it exists to + // catch. A 4xx means the admission decision refused a submission the + // community has authorized. A 500 means the write path broke somewhere + // that has no classification at all — which is what this arc used to + // assert, back when the ceiling was the community-credential refresh, and + // is precisely the vagueness the named sentinel removed. + requireXRPCRefusal(t, err, http.StatusServiceUnavailable, "NoAuthorCredentials", + "an ADMITTED submission stopping at the author-repo open — any 4xx here means the "+ + "admission decision refused a submission the community has authorized, and any 500 "+ + "means it broke somewhere unclassified instead") - // 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. + // And the failed write handed its ledger slot back: the identical retry + // meets the same failure, never 409. This property is UNCHANGED by the + // flip and had to be re-verified rather than assumed — the release now + // happens at a different step (the author-repo open, which sits between + // admission and the community token), and a reservation released on the + // old step but not the new one would look identical from every other + // tier. A leaked one refuses the retry as a duplicate of a post that + // does not exist: the §8 failure mode where a transient outage becomes a + // lockout until the dedupe window rolls. err = submitPost(asAggregator, community.DID, title) - requireXRPCRefusal(t, err, http.StatusInternalServerError, "InternalServerError", + requireXRPCRefusal(t, err, http.StatusServiceUnavailable, "NoAuthorCredentials", "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") + "released, turning one failure into a lockout until the dedupe window rolls") }) }