fix: make external-URL media safe end-to-end (API hosting, X MIME, error sanitization) - #122
Merged
Merged
Conversation
The public REST API accepted inline post media as a free-form array and stored it verbatim, so a client could create/update a post whose media was a bare external URL we never hosted. Publishing then depended on that third-party URL staying alive — when it 404'd (e.g. an image proxy), the post failed across platforms (Facebook 'unsupported media type', X 'HTTP 404', Instagram 'could not fetch media'). Inline media URLs on create/update now go through the same download + MIME- validate + host path as the attach-from-url endpoint (MediaAttacher), so the stored media always points at our own storage. Items already hosted (carrying a path) pass through untouched. If any URL can't be fetched the request is rejected with 422 and nothing is persisted, so a post is never created with broken media. MCP and the web flow were already safe and are unchanged. - MediaAttacher: extract fetchToWorkspace() + add resolveInlineMedia() - Post::allowedMediaTypesFor() so the create flow can compute allowed types without a persisted post - API Store/UpdatePostRequest: media.* item rules (mirroring the web; prevents validated() from stripping hosted-item keys) - PostController store()/update(): host external media before persisting
A post created via the API with bare external-URL media (no mime_type, no path to infer one from) reached XPublisher::getMediaCategory() with a null $mimeType — whose parameter is typed string — throwing a TypeError. The TypeError's message embeds the server file path, which then surfaced verbatim in the user's failure email. X downloads the media bytes anyway, so recover the MIME from the downloaded file when the item carries none, and fail with a clean XPublishException (MediaFormat) rather than a TypeError when it still can't be determined. Also drive the image/video decision off the resolved MIME so sniffed images are still optimized. Audited every publisher: X was the only one whose strict mime path was reachable with null (no isImage() gate + no path to backfill from). Bluesky's uploadBlob is also strictly typed but is guarded by an isImage() check plus the DTO's extension-based MIME backfill; the pull-from-URL publishers classify via nullable helpers and fail cleanly; Pinterest/Mastodon already sniff; YouTube and Facebook video fall back to video/mp4.
The post-failure email renders each platform's stored error_message verbatim. On an unexpected publish error the job's catch-all (and the job-failed hook) stored the raw exception message — for a PHP TypeError that includes the server file path (.../releases/<id>/app/Services/Social/XPublisher.php on line 130), which then reached the customer's inbox. Only our own SocialPublishException carries a vetted, user-facing message. Every other throwable (engine errors like TypeError, or library exceptions such as Intervention's decoder errors which embed temp paths) is now replaced with a generic line; the raw detail stays in the logs. Token-expired and rescheduled paths are unchanged. Note: publishers that still throw plain \Exception for user-facing reasons (some X/Facebook/Threads/LinkedIn cases) will now show the generic message for those; converting them to typed SocialPublishExceptions to restore specific copy is a worthwhile follow-up.
paulocastellano
changed the title
… a clean message After the failure-email sanitization, any non-SocialPublishException is shown to the user as a generic line. The deliberate, user-facing throws in the X, Facebook, Threads, and LinkedIn (+Page) publishers were plain \Exceptions, so they'd have been genericized too — losing actionable messages like 'Unsupported media type for Facebook' or 'X posts require either text or media'. Convert those throws to their platform's SocialPublishException with a vetted userMessage + category, so the publish failure surfaces a specific, safe message (and the catch-all's generic line is reserved for genuinely unexpected errors). Left as plain \Exception on purpose: the download helpers in Bluesky and Mastodon (caught internally, returned as null — they never reach the email), and the shared content-length guard in HasSocialHttpClient (no per-platform exception in the trait, message is path-free, and it's already validated upstream).
…m review Cold-review follow-ups on the PR: - Trim the oversized docblocks/inline comments added across the API controller, MediaAttacher, Post, the publish job, and the X publisher to one line (keeping the @param/@return array-shape annotations). - XPublisher::chunkedUpload now accepts ?string $mediaCategory and only sends media_category when present — getMediaCategory() can return null, so the strict string param was a latent TypeError (unreachable on X today, removed anyway). - Fix MediaAttacher docblocks: the file imports Type as MediaType, so the @param array<Type> annotations didn't resolve — now array<MediaType>. - Tests: cover the failed() job hook genericizing a raw error, and X failing cleanly (XPublishException) when media can't be downloaded.
The media.* rules were duplicated across the web update request and both API requests (and diverged: web requires hosted id+path and tracks source; the API accepts a bare external url it downloads). Pull them into one App\Support\PostMediaRules::rules(hosted:) — same pattern as PostPlatformMetaRules — parameterized by contract, so there's a single place to add a media key and the validated()-strips-unlisted-keys footgun can't drift between entry points. Behavior is unchanged (each ruleset is reproduced exactly). Web store keeps its loose 'media' => array (no item rules) and is left out on purpose — adding strict rules there would change the web create contract.
…ails Final-review follow-ups: - MediaAttacher::resolveInlineMedia now deletes the media it hosted in this call when any item fails, so a mixed [good, bad] batch no longer orphans the good item's Media row + file while the request is correctly rejected with 422. Makes the create/update media resolution truly all-or-nothing. - PostMediaRules: keep source/source_meta on both contracts (the API previously passed them through with no item rules — don't silently drop them) so the media item shape is uniform; only id/path/url differ by contract. - Make MediaAttacher::fetchToWorkspace private (no external callers). - Test the partial-batch rollback (no Media, no post persisted).
- External URL that downloads (200) but isn't a supported media type → 422, nothing persisted (the type-rejection branch, distinct from a download 404). - Mixed batch: an already-hosted item + an external URL both succeed → both kept in order, only the external one creates a Media row.
…tion The download+host+422 orchestration was a private controller method doing IO and throwing — that's an operation, not a controller concern. Move it to App\Actions\Post\HostInlineMedia::execute() (alongside CreatePost/UpdatePost) so the controller stays thin and the logic is reusable/testable.
…led fetch MediaAttacher::download() only caught RuntimeException, but a connection-level failure throws Illuminate\Http\Client\ConnectionException (extends Exception, not RuntimeException) — e.g. a slow/unreachable proxy hitting the 20s timeout. On the API hosting path that propagated as a 500 (not the promised 422), skipped the batch rollback (orphaning an already-hosted item), and leaked the temp file. Catch Throwable so any fetch failure returns null → the caller rejects cleanly with 422 and rolls back. Also hardens the existing MCP/REST attach-from-url paths (a timeout there now reports a failed URL instead of 500).
|
Thanks, I can confirm the fix works. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All of this comes from one incident: a customer's posts (created via the public API with bare external-URL media — an
images.weserv.nlproxy URL stored verbatim, nomime_type/path) failed to publish across Facebook, X, and Instagram, and the X failure leaked a server file path into the failure email.1. API downloads & hosts external media (root cause)
The public REST API accepted inline post
mediaas a free-form array and stored it verbatim, so a post could reference a third-party URL we never hosted. Publishing then depended on that URL staying alive — when the proxy 404'd, every platform failed.Inline media URLs on create/update now go through the same download → MIME-validate → host path as the attach-from-url endpoint (
MediaAttacher). External URLs become hosted media on our storage; already-hosted items (carrying apath) pass through. If any URL can't be fetched the request is rejected with 422 and nothing is persisted. MCP and the web flow were already safe (MCP takes no inline media; web uploads are pre-hosted).2. X recovers a missing MIME (defense)
A bare-URL item has no
mime_typeand nopathto infer one from, soXPublisher::getMediaCategory()(param typedstring) receivednull→ TypeError. X downloads the bytes anyway, so it now recovers the MIME from the downloaded file and fails with a clean exception if it still can't.chunkedUploadalso takes?string $mediaCategorynow (and only sendsmedia_categorywhen present) so the recovered-but-uncategorizable path can't TypeError. Audited every publisher — X was the only one whose strict-mime path was reachable with null.3. Failure emails never leak raw errors (security)
The post-failure email renders each platform's
error_messageverbatim. The TypeError's message embeds the server path (.../releases/<id>/app/Services/Social/XPublisher.php on line 130), which the publish job's catch-all (and the job-failed hook) stored raw → it reached the customer's inbox. Now only our ownSocialPublishException(vetteduserMessage) is surfaced; any other throwable — engine errors likeTypeError, or library exceptions such as Intervention's decoder errors which embed temp paths — is replaced with a generic line, with the raw detail kept in logs. Token-expired and reschedule paths are unchanged.4. Deliberate publish exceptions are typed (UX, follows from #3)
With #3 genericizing anything that isn't a
SocialPublishException, the deliberate user-facing\Exceptionthrows in the X / Facebook / Threads / LinkedIn (+Page) publishers would have been genericized too — losing actionable messages like "Unsupported media type for Facebook". Those are now their platform'sSocialPublishExceptionwith a vetteduserMessage+ category, so failures keep a specific, safe message and the generic line is reserved for genuinely unexpected errors. (Left as plain\Exceptionon purpose: the download helpers in Bluesky/Mastodon — caught internally, never reach the email — and the shared content-length guard inHasSocialHttpClient.)5. Centralized inline-media validation
The
media.*validation rules were duplicated across the web update request and both API requests, and had diverged (web requires hostedid+pathand trackssource; the API accepts a bare externalurl). They're now one source of truth —App\Support\PostMediaRules::rules(hosted:), same pattern asPostPlatformMetaRules— parameterized by contract, so there's a single place to add a media key and thevalidated()-strips-unlisted-keys footgun can't drift between entry points. Behavior is reproduced exactly; the web store's loosemedia => arrayis intentionally left as-is.Scope note
The fix resolves the three errors for newly created/updated posts. An already-stored broken post published via
PUTwith onlystatus(nomedia) skips hosting and still fails — remediation for the customer's existing posts: re-submit the post with itsmedia(re-hosts it), then publish.Tests
XPublishExceptionwhen media can't be downloaded.TypeErrorwith a path) are genericized in both the catch-all and thefailed()hook, and the path never reaches the record; vetted publish-exception messages preserved.PostMediaRules: both contract variants locked (hosted vs external) + shared keys preserved.