Conversation
The local `mediaUploadMiddleware` shadowed the same-named export from `@wordpress/api-fetch`, so the file read as though core's post-process retry behavior was registered when only the draft post ID stripping was. Rename it to `stripDraftPostIdMiddleware` to describe what it does. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
When `wp_generate_attachment_metadata()` fails server-side (commonly a PHP memory_limit or max_execution_time fatal on large images), WordPress returns a 5xx carrying an `x-wp-upload-attachment-id` header. Core's `mediaUploadMiddleware` recovers from this by retrying `POST /wp/v2/media/<id>/post-process` up to five times, then deleting the orphaned attachment if every attempt fails. That middleware was never registered, so these uploads surfaced as failures that left an orphaned gray attachment behind and duplicated the attachment on retry. `apiFetch.use` unshifts, so registration order is the reverse of execution order. Core's middleware is registered before the native one so that it runs after it, and below auth, namespacing, and the root URL so the `post-process` requests it issues through `next` stay authenticated and correctly addressed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
`new RegExp('(' + [].join('|') + ')')` is `/()/`, which matches every
string, so `alreadyHasSiteNamespace` was unconditionally true whenever a
site configured no namespace — as self-hosted sites do.
That was load-bearing rather than merely benign: it suppressed a rewrite
that would otherwise interpolate `siteApiNamespace[0]` — `undefined` for an
empty namespace — into every path, producing `/wp/v2/undefinedposts`. Gate
the rewrite on a configured namespace so the guard no longer has to, and
escape the namespaces so each is matched literally.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
The native upload path circumvented core's post-process retry entirely, so a metadata fatal on a delegate-handled upload stayed a permanent failure with an orphaned attachment left behind. Two things blocked it: - `relayResponse` rebuilt the response with a hardcoded Content-Type, dropping `x-wp-upload-attachment-id` — the header core's middleware needs to identify the attachment to retry. Relay it (via an allowlist, since the body is re-sent with a recomputed length) and expose it through CORS, without which the WebView cannot read it cross-origin regardless. - `nativeMediaUploadMiddleware` always parsed the body, so it never yielded the `Response` that core's middleware inspects. Honor `parse: false` by resolving or rejecting with the `Response` itself. `MediaUploadResponse` gains a `headers` property on both platforms, defaulted so existing host callers are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
The retry depends on reading `x-wp-upload-attachment-id`, which is only possible same-origin or where the server exposes it via CORS. Record which combinations recover, and why there is no client-side fallback, so the iOS direct-upload case is not mistaken for a bug in this registration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
Reproduces the server-side image processing fatal the post-process retry recovers from, so the middleware can be exercised locally without a large image or a resource-starved host. Adapted from the approach in WordPress/gutenberg#17858, with the random failure rate replaced by an explicit mode (`recover`/`always`/`off`) so both the recovery and the exhaust-and-delete paths are reproducible. The mode is an option rather than per-request state, since the upload and each retry are separate requests and a native-server upload carries no browser cookie. The plugin sets the 500 itself: a real fatal under FPM surfaces as a 500, but the Playground runtime returns 200, which the retry would ignore. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
Reading or flipping the mode meant a curl invocation with an inline credentials lookup, which is easy to get wrong mid-debug and silently no-ops if it fails — leaving a passing upload that looks like the retry never fired. Wrap it in `make wp-env-media-failure [MODE=off|recover|always]`, matching the existing VAR=value convention and the thin-target-plus-bin-script pattern. Each precondition reports the fix that applies to it: missing credentials, a rejected 401 (stale after a Playground restart), an unreachable server, and an unregistered endpoint. Also document the orphaned-server and 401 cases in troubleshooting; both came up repeatedly while testing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
A bundled Android build serves the editor from the site's host without a port (`http://10.0.2.2`), which was not in the allowlist, so its REST requests were rejected before reaching WordPress. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
Testing a bundled Android build against wp-env disproved the claim that Android direct uploads are same-origin and therefore recover. `GutenbergView` derives the asset domain from the site's host, and `host` drops the port — so the editor at `http://10.0.2.2` is cross-origin with a site at `http://10.0.2.2:8888`, and the attachment ID header stays unreadable. The claim holds only when the site runs on the scheme's default port, as production sites do. Say that, rather than implying every Android site recovers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
`always` mode fatalled on every sub-size pass, and core's `force=true` delete path runs sub-size handling too — so the editor's orphan cleanup fatalled as well, leaving the orphan behind. The retry logic was correct; the simulator refused the cleanup it had correctly requested. Exempt deletes, including the `POST` + `X-Http-Method-Override: DELETE` form api-fetch sends, so the bare request method alone is not enough to identify one. Also document that a simulated fatal aborts the request before WordPress adds CORS headers, so these responses surface as CORS errors rather than readable 500s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
When every post-process retry fails, core's middleware deletes the orphaned attachment. A cross-origin editor cannot make that request: api-fetch tunnels DELETE as a POST carrying `X-HTTP-Method-Override`, and core's `rest_allowed_cors_headers` omits that header, so the browser blocks it at preflight and the orphan survives. Route media deletions through the loopback upload server instead, which sets its own CORS policy and already permits DELETE. The middleware intercepts before api-fetch's `httpV1` adds the override header, so what reaches the native server is a plain DELETE. Both servers gain a single narrow route — `DELETE /media/<id>` with a numeric ID — rather than a general proxy, matching the existing `POST /upload`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
`handleDelete` went straight to the default uploader, unlike `handleUpload` which offers the work to the delegate first. A host whose `uploadFile` uploads to its own media service holds an ID only it can resolve, so deleting through the default uploader would address the wrong site. Add `deleteFile(attachmentId:)` to `MediaUploadDelegate` on both platforms, defaulted to nil so existing hosts are unaffected, and try it before falling back. Rename the handler to `handleMediaDelete`, since it deletes an attachment rather than an upload and no longer mirrors `handleUpload`'s signature. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
Drop the two notes explaining how the simulator produces its 500 and why a fatal response reads as a CORS error — implementation detail that belongs in the plugin, not the guide. Drop the orphaned-server and stale-credential troubleshooting entries; those are environment problems to address on their own. Also drop a comment restating what the adjacent condition already says, and reword the make target's help text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
`nativeMediaUploadMiddleware` mixed dispatch with the whole upload implementation, so adding the deletion path left the two handled asymmetrically — one extracted, one inline. Extract `nativeMediaUpload` alongside `nativeMediaDelete`, both returning null when a request is not theirs, leaving the middleware as a short dispatcher. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
`handleMediaDelete` caught only `IOException`, so a delegate's `deleteFile` throwing anything else — `IllegalStateException`, a JSON error — escaped to `HttpServer.resolveResponse` and returned a plain-text 500. The editor's `nativeMediaDelete` then failed on `response.json()` and reported `invalid_json` rather than the delegate's actual failure. Catch `Exception` and rethrow `CancellationException`, matching `passthroughResponse` and iOS's untyped `catch`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr
`relayResponse` prepended `Content-Type: application/json` to an array of the response's own headers, and `HTTPResponse` serializes every entry it is given. A delegate returning its own `Content-Type` therefore put the header on the wire twice, which URLSession surfaces as "application/json, text/plain". Android's map merge already overrode instead, so the two platforms disagreed on the same public API. Skip the default when the response already carries the name, matching Android. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr
`deleteFile` is called for every deletion, including attachments the delegate declined at upload time — an attachment ID carries no MIME type or filename, so there is no `handlesFile` gate to apply. A delegate answering for one of those leaves the real WordPress attachment undeleted, which is the orphan the cleanup exists to remove. Returning nil already falls through to the default uploader; document that as the signal for an unrecognized ID. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr
The credentials path was interpolated into the `node -e` source as a single-quoted JS string literal, so a checkout under a path containing a quote or backslash produced a SyntaxError stack trace instead of the intended "could not read authHeader" message. Pass it through `process.argv` and single-quote the script body so the shell does not expand it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr
`relayResponse` merged the JSON default with the response's own headers via Kotlin's map merge, which only overrides on an exact key match. A delegate returning `content-type` therefore produced a two-entry map, and `serializeResponse` writes every entry, putting the header on the wire twice — the WebView sees "application/json, text/plain". This is the same defect `0bf40ad3` fixed on iOS, which the map merge was believed to already handle. Skip the default when the response carries the name under any casing, matching iOS and the case-insensitive lookups `HttpServer` already uses. Add the Android counterpart to the iOS `delegateContentTypeWins` test. It asserts on the raw header lines rather than the parsed map, which lowercases keys into a map and would collapse the duplicate — hiding the very bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVdVBubbCDp7mRXSWkcHHm
The native upload middleware runs below core's mediaUploadMiddleware, which forwards every upload as `parse: false` and reads `x-wp-upload-attachment-id` off a rejected Response to retry post-process. Logging the initial 5xx at error level reported a failure before recovery ran, so every upload that silently recovered still emitted an error. Reject without logging, matching the nativeMediaDelete sibling — the initial 5xx is a handoff to core's retry, not an outcome.
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/621")Built from 2bbc65d |
jkmassel
force-pushed
the
refactor/media-processor-uploader
branch
4 times, most recently
from
September 3, 2026 21:50
9fe481e to
9f00177
Compare
Make performing a media upload -- and retrying it -- a single, all-or-nothing responsibility: either GutenbergKit performs the upload and owns its retries, or the host does (say, to run it through its own networking so it can log every request). Both go to the same configured site; the only difference is who executes the requests. There is no in-between where the host performs the upload but GutenbergKit retries it. When an upload fatals in server-side post-processing it has to be retried: core retries POST .../post-process up to 5x, and cleans up the orphan if that fails. The old `MediaUploadDelegate.uploadFile` let a host perform the upload itself by returning the raw response it received -- which split one upload's HTTP across two owners: the host performed the POST /wp/v2/media, then core, reading that raw response, drove the post-process retries (and the orphan cleanup) behind it. A host that took over uploads to run them through its own stack still didn't own the retries; those went out through the browser, not the host. Delivery and its retries were owned by different parties. Make the upload and its retries one unit with one owner: - `MediaProcessor` (handlesFile, processFile) only transforms the file. It never performs the upload, so GutenbergKit performs it and owns the retries -- the extension point almost every host wants. - `MediaUploader` (upload) performs the upload on the host's own stack (its networking, logging, retry policy, a background session) and owns the whole lifecycle. `upload` returns the finished attachment or throws: there's no raw response for core to retry behind it, so the host drives its own post-process recovery and force-deletes its own orphan on terminal failure. All-or-nothing: the host performs the upload and its retries, or GutenbergKit does -- never a split. An uploader and GutenbergKit's built-in default both target the same configured site; the choice is only who executes the requests. Media deletes always relay to the default uploader (the configured site): every attachment lives there, even one a host uploader delivered, so there is no per-host delete path. The relay is left unscoped -- core issues its cleanup DELETE there, but the relay can't tell it from any other DELETE the WebView sends, so a client-side-compromised editor holding the loopback token could force-delete media on the site. Accepted -- such a script already has broad write access, and a server-side compromise deletes media directly without the editor. An earlier revision carried a per-session ledger to scope the relay; dropped as not worth the cost for a client-side-only threat. `EditorViewController`/`GutenbergView` expose `mediaProcessor` + `mediaUploader` in place of `mediaUploadDelegate`; the server starts if either is set and builds a default uploader whenever site credentials are present (it delivers GutenbergKit's own uploads and relays every media delete). `MediaUploadResponse` drops to internal -- it is no longer on any public API. Both demos and all tests move to the new protocols. Breaking change: hosts must migrate `mediaUploadDelegate` (WordPress-iOS/Android, Jetpack). iOS and Android suites green; SwiftLint and Detekt clean.
MediaUpload.fields was a last-wins map, so a repeated field name (e.g. a `field[]` array) collapsed to its final value before a host MediaUploader saw it — diverging from GutenbergKit's own upload path, which preserves repeats. Pass an ordered list of (name, value) pairs on both platforms.
UploadContext held the processor and uploader `weak`, and each was read twice per request — once at the admission gate, once at delivery. Those reads are separated by a synchronous disk copy and an unbounded `processFile`, so a host that released its handler in that window (the user closing the editor mid-transcode) changed the answer between them: a file admitted for processing was forwarded unprocessed, and an upload gated on a host uploader was delivered by GutenbergKit itself — creating an attachment on the configured site that the host never learns about, behind an uploader documented as keeping GutenbergKit "out of the network entirely". Hold all three strongly, as Android already does. The `weak` was load-bearing when `EditorViewController.mediaUploadDelegate` was itself `weak` and this was the only strong path; 4a03bcc made those properties strong and left it behind. It no longer prevents a cycle — a host object retaining the view controller already forms `EditorViewController -> mediaUploader -> EditorViewController` through the view controller's own strong property, which this container can neither create nor prevent. Immutable strong references also make the two reads agree by construction, so `handlesFile` admission and delivery can't disagree. UploadContext becomes a struct and drops its `@unchecked Sendable` opt-out: both protocols are `Sendable` and InternalMediaClient is `@unchecked Sendable`, so it is implicitly Sendable. `doesNotStronglyRetainProcessor` pinned the vestigial invariant, so it is replaced by `retainsProcessorForServerLifetime`, asserting both halves — the server owns its processor while it runs, and releases it when the server goes away. `uploaderReleasedMidRequestStillDelivers` covers the bug directly; against the previous commit it fails with the real symptom, the host uploader bypassed and passthroughUpload called.
Both delivery paths could put bytes on the wire after the editor was gone. `EditorViewController.deinit` calls `stop()`, which cancels the in-flight connection tasks, but Swift cancellation is cooperative: `writeStream` is an uninterruptible read loop and a host's `processFile` need not check at all, so a handler can reach delivery well after teardown. Whether the request then actually reached WordPress rested entirely on URLSession noticing the cancellation. That is not a guarantee the server can rely on. `URLSessionProtocol` is public and documented for dependency injection, and the obvious conformance for a host wrapping a callback-based stack — `withCheckedThrowingContinuation` around a completion handler — has no cancellation awareness at all. Such a host would upload deterministically after teardown, and the response is discarded either way, leaving an attachment on the site that nothing cleans up. Check cancellation explicitly before delivery in `processAndUpload` and before `passthroughUpload`, so the guarantee comes from this file rather than from the HTTP client's behaviour. CancellationError is already handled quietly by `uploadErrorResponse`, and HTTPServer drops the response for a cancelled task.
`passthroughResponse` and `handleMediaDelete` took the whole UploadContext and touched only `internalClient`. Pass that directly. On the delete path this is more than tidiness. Every attachment lives on the configured site — even one a host uploader delivered — so a deletion always relays through the internal client, never the uploader. That was a convention the signature let you break; now it is a fact the compiler enforces. `handleUpload` and `processAndUpload` keep the context: they genuinely need all three, and spelling them out would push processAndUpload to nine parameters.
The closure form of `start` can't capture the object that owns the server: the closure has to exist before the server does, and retrofitting `self` would form `owner -> HTTPServer -> handler -> owner`, so the owner's deinit — and its `stop()` — would never run. A consumer with dependencies to hold therefore ends up with static functions threading a context parameter through every call, which is how MediaUploadServer is written today. Add an `HTTPRequestHandler` protocol and a `start` overload that takes one. The dependencies become stored properties and the request logic becomes instance methods. The protocol is deliberately not `AnyObject`-constrained: a struct conformer cannot participate in a reference cycle at all, so the ownership question doesn't arise. A final class works too, under the same leaf discipline HTTPServerDelegate already documents. The closure overload is unchanged and forwards to the same code path, so this is purely additive — no existing caller, test, or the debug server is affected. Request handling is mandatory, so it can't be a defaulted HTTPServerDelegate method the way optional customization points are; hence an overload rather than a new delegate requirement.
Every request function was `private static` taking an UploadContext, for one reason: the handler closure has to exist before MediaUploadServer does, so it couldn't capture `self`, and capturing it later would form `MediaUploadServer -> HTTPServer -> handler -> MediaUploadServer` and stop `deinit` from ever running `stop()`. Move them onto a `Handler` struct conforming to the HTTPRequestHandler protocol added in the previous commit. The dependencies become stored properties, so the five request functions become instance methods and drop their context parameter; UploadContext is deleted, since the handler now *is* the context. A struct can't participate in a reference cycle, so the constraint that forced the statics is gone rather than worked around. The statics that remain — errorResponse, relayResponse, attachmentId, formFields, sanitizeFilename, writeStream, cleanOrphanedUploads — are pure functions of their arguments. `static` there is not a workaround; it is the honest signal that they depend on nothing, which is now a meaningful distinction rather than an artifact of the closure. No behaviour change and no test changes: the only entry point is `MediaUploadServer.start`, whose signature is untouched. Reviewing with `--color-moved` will help — most of the diff is the request block moving into the struct and gaining a level of indentation.
MediaUpload.fields was `[(name: String, value: String)]` on iOS. Tuples are not nominal types, so a tuple-typed stored property permanently blocks synthesized Equatable, Hashable and Codable on MediaUpload — inside GutenbergKit as well as for hosts, and retroactively, so no later conformance can recover it. That matters for the offline queue the MediaUploader docs advertise as a motivating use case: a host that wants to persist a pending upload's fields has to hand-roll a mirror type. Unlike the missing public memberwise init on MediaUpload — which stays internal, matching how this library treats outbound types, and which could be added later without breaking anyone — this one is not fixable additively. Changing `fields` after release is a source break for every host, so it happens now or not at all. Introduce MediaUploadField (Sendable, Hashable, Codable, public init) and use it on both platforms. Android had no equivalent defect — Kotlin's Pair is nominal — but the same change lands there for parity, and `field.name`/`field.value` reads better than `first`/`second` in a host's upload code. Kotlin data class destructuring means the multipart writers are unchanged. MediaUpload itself is deliberately left non-Codable: it carries a `fileURL` pointing at a GutenbergKit temp file that will not exist after a relaunch, so a serialized MediaUpload would be a trap. A host queueing an upload should copy the bytes and persist the fields, which this type now supports.
Android refused a mediaUploader when either `siteApiRoot` or `authHeader` was missing; iOS checked only `authHeader`. So a host with valid credentials but no site root crashed on Android and started a server on iOS — the same configuration, opposite outcomes, on a pair of fields that are both required for the internal media client to reach the configured site at all. Gate iOS on both. The types differ — `siteApiRoot` is a `URL` on iOS and a `String` on Android — so the equivalent of Android's `isEmpty()` is "not absolute": a URL with no scheme or host cannot address the site, and every media request built from it fails at the URLSession layer. Also covers the arm nothing tested. `GutenbergViewUploadServerTest` only exercised the missing-authHeader case; add its siteApiRoot sibling. There is no iOS equivalent because `precondition` takes the test process down, where Kotlin's `check` throws catchably. The iOS comment claimed every delete "would 500" without credentials. That is right for a missing site root, where okhttp/URLSession reject the schemeless URL, but a missing auth header relays WordPress's 401 instead. Say "would fail", which is true of both.
The previous commit claimed this policy was untestable on iOS because
`precondition` takes the test process down. That was wrong: Swift Testing
has exit tests, which run the body in a child process.
The real obstacle was narrower. Exit tests are unavailable on iOS and the
simulator ("Exit tests are not available on this platform"), and the
policy lived in EditorViewController, which is `#if canImport(UIKit)` and
therefore absent from the macOS host — so the one platform that can run
exit tests couldn't see the code. The intersection was empty because of
where the code sat, not because of the tool.
Move the decision into MediaServerCredentials, outside the UIKit gate,
and have startUploadServer call it. The predicate and the fail-fast are
now both reachable from the host suite: six tests pin the predicate
(including the two arms of the site-root check that a `URL` makes
different from Android's `String`), and two exit tests pin the trap
itself. Neutering the precondition fails both, so they are not vacuous.
This also gives the crash policy a named home. It diverged silently
between iOS and Android once already; a host-testable predicate is
harder to let drift again.
`formFields` decodes each non-file form value as UTF-8, which substitutes U+FFFD on malformed input. That is lossless today, but only because of an invariant nothing in the code states or enforces: the sole client is the editor's browser FormData. The server binds to loopback behind a per-session token; a FormData string value is a USVString, already well-formed at append time; and its only way to carry arbitrary bytes is a Blob, which always gets a filename and is filtered out of `extraParts`. Write that down on both platforms, including the part that makes it matter — if it stops holding, the two platforms are lossy *differently* (for ED A0 80, Swift's maximal-subpart rule yields three replacement characters where Java's decoder yields one), so there is no single behaviour that could be documented instead. Also reword the raw-bytes comment on the re-encode path. "So a non-UTF-8 value is forwarded verbatim" read as though malformed values were expected, which made the two delivery paths look contradictory. The actual hazard is the failable `String(data:encoding:)` returning nil and an obvious `?? ""` dropping the whole value; the reason to keep bytes is that the re-encode should stay byte-identical to the passthrough it stands in for. Cover the partition rather than the decode, since the partition is what makes the invariant true: a request carrying a second, Blob-shaped part whose bytes are not valid UTF-8 must not surface that part in `fields`. Both tests fail when the filename filter is relaxed, so neither is vacuous. Neither asserts what becomes of that second part — it is currently dropped rather than relayed, which is a separate open question.
jkmassel
force-pushed
the
fix/register-core-media-upload-middleware
branch
from
September 5, 2026 02:52
910fc77 to
b6d05cb
Compare
This was referenced Sep 5, 2026
jkmassel
force-pushed
the
fix/register-core-media-upload-middleware
branch
from
September 8, 2026 16:11
b6d05cb to
09d33ef
Compare
This was referenced Sep 17, 2026
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
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.
Stacked on #594. Makes performing a media upload — and retrying it — a single, all-or-nothing responsibility: either GutenbergKit performs the upload and owns its retries, or the host does (say, to run it through its own networking so it can log every request). Both go to the same configured site; the only difference is who executes the requests. There's no in-between where the host performs the upload but GutenbergKit retries it.
Summary
Two protocols replace the single
MediaUploadDelegate:MediaProcessor— transform the file before upload; GutenbergKit performs the upload and owns its retries. The common, safe extension point.MediaUploader— perform the upload yourself (your own networking, logging, retry policy, a background session) and own its whole lifecycle: retries, recovery, and cleanup. Receives aMediaUploadvalue carrying the file, its metadata, the editor's non-file form fields (post, additionalData), and the request query (?_embed).EditorViewController/GutenbergViewown whichever handler you set — their properties are strong on both platforms, so you can assign one and drop your own reference (just don't strongly retain the editor back).Breaking: hosts migrate
mediaUploadDelegate→mediaProcessor/mediaUploader.The problem with the old design
When an upload fatals in server-side post-processing, it has to be retried: core retries
POST …/post-processup to 5×, and cleans up the orphan if that fails.MediaUploadDelegate.uploadFilelet a host perform the upload itself by returning the raw response it received. But that split one upload's HTTP across two owners: the host performed thePOST /wp/v2/media, then core — reading that raw response — drove thepost-processretries (and the orphan cleanup) behind it. A host that took over uploads to run them through its own stack still didn't own the retries; those went out through the browser, not the host. Delivery and its retries were owned by different parties.The fix
Make the upload and its retries one unit with one owner:
MediaProcessor(handlesFile,processFile) only transforms the file. It never performs the upload, so GutenbergKit performs it and owns the retries. The extension point almost every host wants.MediaUploader(upload(_:)) performs the upload on the host's own stack and owns the whole lifecycle.uploadreturns the finished attachment or throws — there's no raw response for core to retry behind it, so the host drives its ownpost-processrecovery and force-deletes its own orphan on terminal failure. TheMediaUploadit receives carries everything needed to reproduce a native request — the file, the editor'spost/ additionalData fields, and the?_embedquery — so a host upload attaches to its post instead of landing as an unattached orphan.So it's all-or-nothing: the host performs the upload and its retries, or GutenbergKit does — never a split. An uploader and GutenbergKit's built-in default both target the same configured site; the choice is only who executes the requests.
Media deletes always relay to the default uploader (the configured site) — every attachment lives there, even one a host uploader delivered, so there's no per-host delete path. The server starts if either handler is set and builds a default uploader whenever site credentials are present (it delivers GutenbergKit's own uploads and relays every delete). Because deletes need it, a
mediaUploaderset without site credentials is a configuration error and traps at startup rather than starting a server whose every delete would 500.MediaUploadResponseis nowinternal— it's no longer on any public API.Accepted Risk / Out of Scope
DELETEto the configured site, but the relay can't distinguish it from any otherDELETEthe WebView sends — a client-side-compromised editor (a supply-chain-tampered JS bundle, or editor XSS) holding the loopback token could force-delete arbitrary media there. We accept this: such a script already has broad write access via allowed methods, and a server-side compromise (a malicious plugin) deletes media directly without the editor. An earlier revision carried a per-session ledger to scope the relay; it's dropped as not worth the cost for a client-side-only threat.Test Plan
MediaUploadServer,GutenbergViewxcodebuild, Xcode 26.4.1); Android demo builds (Detekt compiles it)mediaUploadDelegatein WordPress-iOS / WordPress-Android / JetpackRelated