From 04eb1db7f14d5af96d26304b832200b3f22a0650 Mon Sep 17 00:00:00 2001 From: Bretton Date: Sat, 8 Aug 2026 05:51:15 -0700 Subject: [PATCH] =?UTF-8?q?test(posts):=20RED=20cycle=202=20=E2=80=94=20bl?= =?UTF-8?q?ob=20ownership,=20contract=20fixes,=20T2=20probe=20rewrite?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 6 RED cycle 2. Two contract fixes green, eight blob pins red, one T2 arc rewritten around the ceiling the flip moved. CONTRACT FIXES (both green) declaredRoutes gains post.update, declared exactly like post.create beside it — same guard, no limiter of its own — because it is the same kind of write by the same principals, and an edit cheaper to call than the post it edits would be the obvious way to spend a quota the create path meters. The block comment's "two writes" becomes three. AggregatorQuotaStopsTheEleventhPost submits ten DISTINCT titles. Its premise predated content-derived rkeys: ten identical submissions used to produce ten records because each got a fresh clock TID, and now converge on ONE record by design — the second through tenth find their own post standing and are handed back its URI. The quota still reached ten, so the old loop asserted an eleventh refusal while exactly one post existed. BLOB PINS — the two plan-review blockers, red write side (service_blob_test.go, T1): a post whose unfurled link preview produces a thumbnail uploads that blob to the AUTHOR's PDS. Asserted with com.atproto.sync.listBlobs on both repos, because the record only names a CID: a blob in the community's storage produces a record that looks identical and resolves for nobody, can be garbage-collected by a repo that references it nowhere, and is not the author's to release when they delete the post. Today's red is the PDS itself refusing the record — "Could not find blob" — which is the invariant being enforced by the one participant that cannot be fooled. Three guard-survival pins accompany it and pass now and after: the 6MB cap, the image/* allowlist and an unreachable origin each leave the post intact and the author's storage empty. UploadBlobFromURL is a choke point on an attacker-influenced fetch (a client picks the page, the page picks the thumbnail); moving it onto the author's credentials must not lose it. read side (blob_transform_test.go, T0 + one T1): a postv2 record's blobs resolve against the AUTHOR's DID+PDS while deprecated community.post records keep the COMMUNITY owner. One table holds both kinds and one read path serves them, so the owner is chosen per record from the URI's collection; getting it wrong builds a well-formed URL naming a repo that has never held the blob, which is a broken image and a silent server. Fixtures that used to carry no URI now name their collection explicitly — an owner chosen FROM the URI cannot be tested with fixtures that lack one. Fails closed: a postv2 record with no author is left unprojected rather than falling back to the community. The T1 half pins what a pure test cannot — AuthorView.PDSURL is populated on the real read path. The repository has always SELECTed users.pds_url to hydrate the avatar and always dropped it; correct owner selection over a field nobody fills in resolves every postv2 post's media against an empty host. AuthorView.PDSURL added as the rule-7 stub (mirrors CommunityRef). T2 PROBE (tests/e2e/post_admission_contract_test.go) The admitted arc's ceiling moved and the arc moved with it. It used to stop at the community-credential refresh and assert an unclassified 500; a post is written to its author's repo now, so it stops at the author-repo open and asserts the named 503 NoAuthorCredentials. That is a strictly better probe. A 500 was the absence of a classification — a nil-pointer three layers down would have answered it just as readily. The named 503 is produced at exactly one place in the write path, so reaching it proves the request travelled past admission, past classification, past the ledger reservation, to the author-repo open. Any 4xx now means the gate refused something the community authorized; any 500 means something unclassified broke. The never-409 retry property is unchanged but was RE-VERIFIED rather than assumed: the release now happens at a different step, and a reservation released on the old one but not the new one would look identical from every other tier. The other three arcs were checked against the flip and left alone — the 401 aggregator gate, the 404 community resolution and the 403-twice quota-preservation arc all run before the author-repo open, so their wire truth is untouched. FINGERPRINT NOTE A comment at the matrix test's fingerprint fixture recording why it still hashes a PostRecord: the boundary converter keeps the value BYTE-STABLE across the deploy, and retyping it would repartition every live post_submissions row — an author mid-retry when the binary rolls would miss their own reservation and be admitted as a second post, reintroducing the exact duplicate the deterministic rkey exists to close. Task-8 revisit pointer included: re-materialization is the one moment the change is free. go vet clean under all four tag sets; test-audit 0 violations; the full T0+T1 tree green apart from the three blob pins. Co-Authored-By: Claude Fable 5 --- internal/api/routes/registration_test.go | 9 +- internal/core/posts/admit_matrix_test.go | 20 + internal/core/posts/blob_transform_test.go | 169 +++++++- internal/core/posts/post.go | 11 + .../core/posts/service_aggregator_test.go | 15 +- internal/core/posts/service_blob_test.go | 377 ++++++++++++++++++ tests/e2e/post_admission_contract_test.go | 94 +++-- 7 files changed, 658 insertions(+), 37 deletions(-) create mode 100644 internal/core/posts/service_blob_test.go 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") }) } -- 2.51.2