From 285ccc82d541a352682fc7b5aad903e695bf4610 Mon Sep 17 00:00:00 2001 From: Bretton Date: Sat, 8 Aug 2026 06:09:21 -0700 Subject: [PATCH] =?UTF-8?q?feat(posts):=20GREEN=20cycle=202=20=E2=80=94=20?= =?UTF-8?q?a=20post's=20media=20follows=20the=20post?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 6 GREEN cycle 2. Implementation only; no test file touched. All eight blob pins green, and `make ci` passes at 5093 tests with 0 skips. WRITE SIDE — the blob goes where the record went blobs.Service.FetchImageForURL UploadBlobFromURL's first half on its own: the bounded fetch, the image/* allowlist and the 6MB cap, touching no PDS. UploadBlobFromURL now calls it, so the community path is unchanged and the guard has exactly one implementation. SPLIT RATHER THAN COPIED because the uploader changed, not the guard. An author authenticates with a DPoP-signed OAuth session, which cannot be expressed as the bearer token blobs.BlobOwner carries, so the caller does the upload leg itself — and a second copy of a guard on an attacker-influenced fetch is a guard that will drift. AuthorRepo.UploadBlob the author's own storage, satisfied by pds.Client unchanged. A blob ref names a CID and not a repository, so a thumbnail left behind in the community's storage produces a record that looks identical to a correct one and resolves for nobody, is garbage-collectable by a repo referencing it nowhere, and is not the author's to release when they delete the post. postService.uploadThumbnail fetch-and-validate, THEN upload, and only if the first passed. Discarding a bad blob after uploading it would pay the entire cost of not having a cap and merely decline to show the result. One 15s budget covers both legs, as the single call it replaces did. Both producers move together: the unfurled thumbnail and the trusted aggregator's supplied ThumbnailURL. READ SIDE — the owner is a property of the record, not of the deployment blobOwnerOf chooses the repository per view from the URI's COLLECTION, which is the only honest signal: it says which repo the record is in, which is exactly the question. postv2 resolves against the author, deprecated community.post against the community, and one table holding both kinds is why the choice cannot be a constant. Getting it wrong does not crash — it builds a well-formed URL naming a repo that has never held the blob, which is a broken image for every reader and a silent success on the server. FAILS CLOSED: a postv2 view with no author is left unprojected rather than falling back to the community, because the fallback is a confident claim about a repo that has never held the blob while an unprojected ref is visibly wrong to the client. AuthorView.PDSURL carried out of the repository scan. The column has always been SELECTed — it hydrates the author's avatar — and always dropped here, which would have left correct owner selection addressing every author-owned post's media to an empty host. NOT DONE, AND WHY The community-token refresh stays on the create path. It genuinely has no consumer left — both writes that used it have moved, and the refreshed value is not read below — but service_admission_test.go's token-refresh case still requires a failed refresh to fail the submission, and that test is not mine to retire. Verified by deletion: the test fails on exactly that assertion. The call site now says so, including the cost of leaving it — a community whose stored refresh token has rotted blocks its authors from posting for a reason no longer present in the code. Co-Authored-By: Claude Fable 5 --- internal/core/blobs/service.go | 51 ++++++++++++--- internal/core/posts/blob_transform.go | 52 +++++++++++++-- internal/core/posts/postv2.go | 17 +++++ internal/core/posts/service.go | 94 ++++++++++++++++++--------- internal/db/postgres/post_repo.go | 6 ++ 5 files changed, 174 insertions(+), 46 deletions(-) diff --git a/internal/core/blobs/service.go b/internal/core/blobs/service.go index d746c53..b7d8bae 100644 --- a/internal/core/blobs/service.go +++ b/internal/core/blobs/service.go @@ -28,6 +28,26 @@ type Service interface { // UploadBlob uploads binary data to the owner's PDS UploadBlob(ctx context.Context, owner BlobOwner, data []byte, mimeType string) (*BlobRef, error) + + // FetchImageForURL is UploadBlobFromURL's FIRST half on its own: fetch the + // remote image under a timeout, refuse a Content-Type outside the image + // allowlist, and cap the body at 6MB. It touches no PDS. + // + // IT IS SPLIT OUT BECAUSE THE UPLOADER CHANGED, NOT THE GUARD. A post's + // media now goes into the AUTHOR's repository, under the author's own OAuth + // session — which is DPoP-signed, so it cannot travel through this + // package's BlobOwner (a bearer token and a URL). The caller therefore does + // the upload itself, through the author's PDS client, and this is what it + // calls first so that the choke point is reached by both paths rather than + // reimplemented beside one of them. + // + // The guard is the reason the split is a split and not a copy. The URL + // being fetched is attacker-influenced twice over — a client picks the page + // that gets unfurled, and the page picks the thumbnail — so a second + // implementation that drifted would turn a link preview into an unbounded + // fetch performed by the AppView with a user's credentials into that user's + // own storage quota. + FetchImageForURL(ctx context.Context, imageURL string) (data []byte, mimeType string, err error) } type blobService struct { @@ -48,9 +68,21 @@ func NewBlobService(pdsURL string) Service { // 3. Validate MIME type (image/jpeg, image/png, image/webp) // 4. Call UploadBlob to upload to PDS func (s *blobService) UploadBlobFromURL(ctx context.Context, owner BlobOwner, imageURL string) (*BlobRef, error) { + data, mimeType, err := s.FetchImageForURL(ctx, imageURL) + if err != nil { + return nil, err + } + + // Upload to PDS + return s.UploadBlob(ctx, owner, data, mimeType) +} + +// FetchImageForURL fetches and validates a remote image without uploading it. +// See Service.FetchImageForURL for why this half stands on its own. +func (s *blobService) FetchImageForURL(ctx context.Context, imageURL string) ([]byte, string, error) { // Input validation if imageURL == "" { - return nil, fmt.Errorf("image URL cannot be empty") + return nil, "", fmt.Errorf("image URL cannot be empty") } // Create HTTP client with timeout (30s to handle slow CDNs and large images) @@ -61,7 +93,7 @@ func (s *blobService) UploadBlobFromURL(ctx context.Context, owner BlobOwner, im // Fetch image from URL req, err := http.NewRequestWithContext(ctx, "GET", imageURL, nil) if err != nil { - return nil, fmt.Errorf("failed to create request for image URL: %w", err) + return nil, "", fmt.Errorf("failed to create request for image URL: %w", err) } // Set User-Agent to avoid being blocked by CDNs that filter bot traffic @@ -69,7 +101,7 @@ func (s *blobService) UploadBlobFromURL(ctx context.Context, owner BlobOwner, im resp, err := client.Do(req) if err != nil { - return nil, fmt.Errorf("failed to fetch image from URL: %w", err) + return nil, "", fmt.Errorf("failed to fetch image from URL: %w", err) } defer func() { if closeErr := resp.Body.Close(); closeErr != nil { @@ -79,13 +111,13 @@ func (s *blobService) UploadBlobFromURL(ctx context.Context, owner BlobOwner, im // Check HTTP status if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("failed to fetch image: HTTP %d", resp.StatusCode) + return nil, "", fmt.Errorf("failed to fetch image: HTTP %d", resp.StatusCode) } // Get MIME type from Content-Type header mimeType := resp.Header.Get("Content-Type") if mimeType == "" { - return nil, fmt.Errorf("image URL response missing Content-Type header") + return nil, "", fmt.Errorf("image URL response missing Content-Type header") } // Normalize MIME type (e.g., image/jpg → image/jpeg) @@ -93,23 +125,22 @@ func (s *blobService) UploadBlobFromURL(ctx context.Context, owner BlobOwner, im // Validate MIME type before reading data if !isValidMimeType(mimeType) { - return nil, fmt.Errorf("unsupported MIME type: %s (allowed: image/jpeg, image/png, image/webp)", mimeType) + return nil, "", fmt.Errorf("unsupported MIME type: %s (allowed: image/jpeg, image/png, image/webp)", mimeType) } // Read image data data, err := io.ReadAll(resp.Body) if err != nil { - return nil, fmt.Errorf("failed to read image data: %w", err) + return nil, "", fmt.Errorf("failed to read image data: %w", err) } // Validate size (6MB = 6291456 bytes) const maxSize = 6291456 if len(data) > maxSize { - return nil, fmt.Errorf("image size %d bytes exceeds maximum of %d bytes (6MB)", len(data), maxSize) + return nil, "", fmt.Errorf("image size %d bytes exceeds maximum of %d bytes (6MB)", len(data), maxSize) } - // Upload to PDS - return s.UploadBlob(ctx, owner, data, mimeType) + return data, mimeType, nil } // UploadBlob uploads binary data to the owner's PDS diff --git a/internal/core/posts/blob_transform.go b/internal/core/posts/blob_transform.go index 3e8e161..6a136c9 100644 --- a/internal/core/posts/blob_transform.go +++ b/internal/core/posts/blob_transform.go @@ -14,17 +14,16 @@ import ( // the #view shape served to clients, replacing blob references with fetchable // image-proxy URLs. It modifies the Embed field in place. // -// Post embeds resolve against the community's repository: the AppView signs -// community post records into the community's PDS and uploads their blobs -// there, so the community DID owns every blob in the embed regardless of who -// authored the post. +// WHICH REPOSITORY THE BLOBS LIVE IN DEPENDS ON THE RECORD (see blobOwnerOf). +// One table holds both kinds of post and one read path serves them, so the +// owner is chosen per view rather than assumed. // // Must run before the response is written. A post whose embed still carries // blob references forces the client to build its own blob URLs, which routes // media around the image proxy and therefore around CSAM scanning — see // internal/core/embeds. func TransformBlobRefsToURLs(postView *PostView) { - if postView == nil || postView.Embed == nil || postView.Community == nil { + if postView == nil || postView.Embed == nil { return } @@ -33,7 +32,48 @@ func TransformBlobRefsToURLs(postView *PostView) { return } - embeds.HydrateView(embedMap, postView.Community.DID, postView.Community.PDSURL) + ownerDID, ownerPDSURL, ok := blobOwnerOf(postView) + if !ok { + return + } + + embeds.HydrateView(embedMap, ownerDID, ownerPDSURL) +} + +// blobOwnerOf reports the repository a post's blobs live in, and false when +// there is no answer. +// +// A postv2 record lives in its AUTHOR's repo (§3.1) and its media was uploaded +// under the author's own session, so the author owns it. A deprecated +// social.coves.community.post record was signed into the COMMUNITY's repo by the +// AppView, with its blobs uploaded there, so the community owns it — and every +// such record standing in production today still resolves that way, until task 8 +// re-materializes them. +// +// THE COLLECTION IN THE URI IS THE ONLY HONEST SIGNAL. It says which repository +// the record is in, which is exactly the question being asked. Getting this +// wrong does not crash: it builds a perfectly well-formed URL naming a repo that +// has never held the blob, which is a broken image for every reader and looks +// from the server side like everything worked. +// +// IT FAILS CLOSED. A postv2 view with no author is left unprojected rather than +// falling back to the community — the fallback is the tempting repair and the +// worse one, because it produces a confident URL under a DID that has never held +// the blob, where an unprojected blob ref is visibly wrong to the client. A view +// with no URI at all is not a production shape, and degrades to the community: +// that is what every record predating the flip resolves to. +func blobOwnerOf(postView *PostView) (did, pdsURL string, ok bool) { + if CollectionOfPostURI(postView.URI) == PostV2Collection { + if postView.Author == nil || postView.Author.DID == "" { + return "", "", false + } + return postView.Author.DID, postView.Author.PDSURL, true + } + + if postView.Community == nil { + return "", "", false + } + return postView.Community.DID, postView.Community.PDSURL, true } // TransformPostEmbeds enriches post embeds with resolved Bluesky post data diff --git a/internal/core/posts/postv2.go b/internal/core/posts/postv2.go index 492eb53..05dcf75 100644 --- a/internal/core/posts/postv2.go +++ b/internal/core/posts/postv2.go @@ -11,6 +11,7 @@ import ( "github.com/bluesky-social/indigo/atproto/syntax" "Coves/internal/atproto/pds" + "Coves/internal/core/blobs" ) // The author-repo half of the write path (docs/PRD_AUTHOR_OWNED_POSTS.md §3.1, @@ -197,6 +198,22 @@ type AuthorRepo interface { // DeleteRecord removes a record from the author's repo. DeleteRecord(ctx context.Context, collection, rkey string) error + // UploadBlob puts a post's media into the author's own storage. + // + // THE BLOB HAS TO TRAVEL WITH THE RECORD. A blob ref names a CID and not a + // repository, so a reader resolves it against the repo it believes owns the + // record — the author's. A thumbnail left in the community's storage + // therefore produces a record that looks identical to a correct one and + // resolves for nobody, is garbage-collectable by a repo that references it + // nowhere, and is not the author's to release when they delete the post. + // + // It is on THIS interface rather than reached through blobs.Service because + // an author authenticates with a DPoP-signed OAuth session, and that cannot + // be expressed as the bearer token blobs.BlobOwner carries. The fetch and + // the size/MIME guard still come from blobs.Service.FetchImageForURL — only + // the upload leg moved. + UploadBlob(ctx context.Context, data []byte, mimeType string) (*blobs.BlobRef, error) + // DID is the repo being written — the author's own identity, which is the // authority half of every post URI this path produces. DID() string diff --git a/internal/core/posts/service.go b/internal/core/posts/service.go index 9c6c035..fcf3de1 100644 --- a/internal/core/posts/service.go +++ b/internal/core/posts/service.go @@ -93,8 +93,8 @@ func NewPostService( // 4. Admission: one decision over community existence, visibility, ban, // aggregator authorization, dedupe and the per-author quota (admitPost) // 5. Open the AUTHOR's repository under the author's own credentials -// 6. Ensure the community has fresh PDS credentials (the blob uploads in -// step 8 still land in the community's repo until task 7 moves them) +// 6. Ensure the community has fresh PDS credentials — a step with no consumer +// left on this path; see the note at the call site // 7. Build the postv2 record // 8. Validate and enhance external embeds (thumb validation, unfurl, blobs) // 9. Create-only write at the deterministic rkey @@ -232,8 +232,19 @@ func (s *postService) CreatePost(ctx context.Context, session *oauth.ClientSessi } // 6. Ensure community has fresh PDS credentials (token refresh if needed). - // Still needed because the thumbnail blobs an external embed uploads are - // still written to the COMMUNITY's repo; task 7 moves them to the author's. + // + // THIS STEP NOW HAS NO CONSUMER ON THIS PATH, and it should go. It existed + // for the two writes that used the community's token — the post record and + // its thumbnail blob — and both have moved to the author's repository. The + // refreshed community is not read again below; only communityDID is, and + // that was captured before the call. + // + // It survives because service_admission_test.go's token-refresh case still + // requires a failed refresh to fail the submission, and that test is not + // mine to retire. Removing it is a four-line deletion the moment it is. + // Leaving it is not free: a community whose stored refresh token has rotted + // currently blocks its authors from posting for no reason any longer + // present in the code. community, err = s.communityService.EnsureFreshToken(ctx, community) if err != nil { releaseOnFailure() @@ -244,7 +255,7 @@ func (s *postService) CreatePost(ctx context.Context, session *oauth.ClientSessi postRecord := postRecordFor(req, communityDID, time.Now().UTC().Format(time.RFC3339)) // 8. Validate and enhance external embeds - if err := s.enhanceExternalEmbed(ctx, &postRecord, req, community, actor == ActorTrustedAggregator); err != nil { + if err := s.enhanceExternalEmbed(ctx, &postRecord, req, authorRepo, actor == ActorTrustedAggregator); err != nil { releaseOnFailure() return nil, err } @@ -639,7 +650,7 @@ func postV2From(record PostRecord) PostV2Record { // // 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 { +func (s *postService) enhanceExternalEmbed(ctx context.Context, postRecord *PostRecord, req CreatePostRequest, authorRepo AuthorRepo, trusted bool) error { if postRecord.Embed != nil { embedType, typeOk := postRecord.Embed["$type"].(string) if typeOk && embedType == "social.coves.embed.external" { @@ -655,18 +666,13 @@ func (s *postService) enhanceExternalEmbed(ctx context.Context, postRecord *Post if req.ThumbnailURL != nil && *req.ThumbnailURL != "" && trusted { log.Printf("[AGGREGATOR-THUMB] Trusted aggregator provided thumbnail: %s", *req.ThumbnailURL) - if s.blobService != nil { - blobCtx, blobCancel := context.WithTimeout(ctx, 15*time.Second) - defer blobCancel() - - blob, blobErr := s.blobService.UploadBlobFromURL(blobCtx, community, *req.ThumbnailURL) - if blobErr != nil { - log.Printf("[AGGREGATOR-THUMB] Failed to upload thumbnail: %v", blobErr) - // No fallback - aggregators only use RSS feed thumbnails - } else { - external["thumb"] = blob - log.Printf("[AGGREGATOR-THUMB] Successfully uploaded thumbnail from trusted aggregator") - } + blob, blobErr := s.uploadThumbnail(ctx, authorRepo, *req.ThumbnailURL) + if blobErr != nil { + log.Printf("[AGGREGATOR-THUMB] Failed to upload thumbnail: %v", blobErr) + // No fallback - aggregators only use RSS feed thumbnails + } else if blob != nil { + external["thumb"] = blob + log.Printf("[AGGREGATOR-THUMB] Successfully uploaded thumbnail from trusted aggregator") } } @@ -711,18 +717,13 @@ func (s *postService) enhanceExternalEmbed(ctx context.Context, postRecord *Post // Upload thumbnail from unfurl if client didn't provide one // (Thumb validation already happened above) - if external["thumb"] == nil { - if result.ThumbnailURL != "" && s.blobService != nil { - blobCtx, blobCancel := context.WithTimeout(ctx, 15*time.Second) - defer blobCancel() - - blob, blobErr := s.blobService.UploadBlobFromURL(blobCtx, community, result.ThumbnailURL) - if blobErr != nil { - log.Printf("[POST-CREATE] Warning: Failed to upload thumbnail for %s: %v", uri, blobErr) - } else { - external["thumb"] = blob - log.Printf("[POST-CREATE] Uploaded thumbnail blob for %s", uri) - } + if external["thumb"] == nil && result.ThumbnailURL != "" { + blob, blobErr := s.uploadThumbnail(ctx, authorRepo, result.ThumbnailURL) + if blobErr != nil { + log.Printf("[POST-CREATE] Warning: Failed to upload thumbnail for %s: %v", uri, blobErr) + } else if blob != nil { + external["thumb"] = blob + log.Printf("[POST-CREATE] Uploaded thumbnail blob for %s", uri) } } @@ -744,6 +745,39 @@ func (s *postService) enhanceExternalEmbed(ctx context.Context, postRecord *Post return nil } +// uploadThumbnail fetches a remote thumbnail through the blob service's guard +// and puts it into the AUTHOR's own repository. +// +// THE GUARD RUNS FIRST AND THE UPLOAD ONLY HAPPENS IF IT PASSES. FetchImageForURL +// bounds the fetch with a timeout, refuses a Content-Type outside the image +// allowlist and caps the body at 6MB; a refusal returns before the author's PDS +// has been touched at all. Discarding a bad blob after uploading it would be the +// easy mistake and the wrong one — it pays the whole cost of not having a cap +// (the fetch, the transfer, the storage write, the author's quota) and only +// declines to show the result. +// +// A nil blob with a nil error means there was nothing to do: no blob service is +// wired, or no author repo is available. Both are wiring states a post survives +// — a thumbnail is an enhancement, and it has never been able to fail a post. +func (s *postService) uploadThumbnail(ctx context.Context, authorRepo AuthorRepo, imageURL string) (*blobs.BlobRef, error) { + if s.blobService == nil || authorRepo == nil || imageURL == "" { + return nil, nil + } + + // One budget for the fetch and the upload together, as the single + // UploadBlobFromURL call used to have: the pair is one enhancement, and + // letting each half have its own would double the worst case a post waits. + blobCtx, cancel := context.WithTimeout(ctx, 15*time.Second) + defer cancel() + + data, mimeType, err := s.blobService.FetchImageForURL(blobCtx, imageURL) + if err != nil { + return nil, err + } + + return authorRepo.UploadBlob(blobCtx, data, mimeType) +} + // validateCreateRequest validates basic input requirements func (s *postService) validateCreateRequest(req *CreatePostRequest) error { // Global content limits (from lexicon) diff --git a/internal/db/postgres/post_repo.go b/internal/db/postgres/post_repo.go index f880bb2..a41444e 100644 --- a/internal/db/postgres/post_repo.go +++ b/internal/db/postgres/post_repo.go @@ -436,6 +436,12 @@ func scanPostView(rows *sql.Rows, extraDest ...interface{}) (*posts.PostView, er if avatarURL := blobs.HydrateImageURL(blobs.GetImageURLConfig(), authorPDSURL.String, authorView.DID, authorAvatar.String, "avatar_small"); avatarURL != "" { authorView.Avatar = &avatarURL } + // CARRIED, not just used for the avatar above. A postv2 post's media lives + // in the AUTHOR's repository, so the blob transform needs to know which + // server holds it; this column has always been selected and always dropped + // here, which would leave every author-owned post's images addressed to an + // empty host. + authorView.PDSURL = authorPDSURL.String postView.Author = &authorView // Build community ref -- 2.51.2