feat: proxy media uploads through native delegate for processing - #357
Conversation
df05265 to
64d64cb
Compare
| return response.text().then( ( body ) => { | ||
| const message = | ||
| response.status === 413 | ||
| ? `The file is too large to upload. Please choose a smaller file.` |
There was a problem hiding this comment.
We should probably print whatever the server sends back – it should be WP_Error-shaped, but some hosts might have messaging like "The max is ${SOME_NUMBER}" or "You've reached your quota".
WDYT?
There was a problem hiding this comment.
Yes, that makes sense.
This was strictly implemented to handle the 250 MB maximum upload restriction of the local server, but I agree it should be made more robust. If we do not add specific handling for 413, the default user-facing message is something like "Unable to get a valid response from the server."
5a78344 to
18f81b7
Compare
|
@jkmassel this now relies upon the GBK HTTP server library. This is ready for another review. |
|
@jkmassel the latest changes tested well for me. I believe this is ready for another review. 🙇🏻♂️ |
8b4b5ab to
6b1d98b
Compare
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/357")Built from e430b9c |
7758c5b to
6b1d98b
Compare
| // Drain oversized body before throwing so the | ||
| // client receives the 413 (RFC 9110 §15.5.14). | ||
| if (parser.state == HTTPRequestParser.State.DRAINING) { | ||
| readUntil(parser, input, buffer, deadlineNanos) { it.isComplete } | ||
| } |
There was a problem hiding this comment.
Relocated to avoid unauthorized requests causing body drains. Similar iOS changes added around ios/Sources/GutenbergKitHTTP/HTTPServer.swift:285.
| /// The default maximum time to wait for the listener to become ready (5 seconds). | ||
| /// Binding to loopback normally completes in milliseconds; the bound exists so a | ||
| /// listener stuck in the `.waiting` state (which emits no further updates) | ||
| /// cannot suspend its caller indefinitely. | ||
| public static let defaultStartTimeout: Duration = .seconds(5) |
There was a problem hiding this comment.
Added in 7da893b to avoid media upload server start delays from indefinitely postponing editor launch. I welcome feedback on the implementation.
| // Deliberately NOT tracked in `dependencyTaskHandle`: `viewDidDisappear` | ||
| // cancels that handle to abort the async dependency *fetch*, but the | ||
| // fast path is cheap local work that must run to completion — a | ||
| // transient disappearance (e.g. a modal presented over the editor) | ||
| // cancelling it mid `startUploadServer()` silently disabled native | ||
| // uploads for the session. `[weak self]` still makes it a no-op once | ||
| // the controller is torn down. | ||
| Task(priority: .userInitiated) { [weak self] in |
There was a problem hiding this comment.
Added in f1ce3cc. Based on my understanding, this seems correct. I welcome help confirming that.
| // Sweep temp files orphaned by a prior crash, off the editor-startup | ||
| // path — the sweep only deletes stale files (>1 hour old), so it cannot | ||
| // race this server's own in-flight uploads and nothing below depends on it. | ||
| let cleanupTask = Task.detached(priority: .utility) { | ||
| cleanOrphanedUploads() | ||
| } |
There was a problem hiding this comment.
Added in 4521918. I believe moving this to a Task is sound for improving editor startup time, but I welcome a second glance to ensure this doesn't cause issue.
| // Drain the oversized body before responding so the (authenticated) | ||
| // client receives the 413 instead of a connection reset | ||
| // (RFC 9110 §15.5.14). | ||
| if (parser.state == HTTPRequestParser.State.DRAINING) { | ||
| readUntil(parser, input, buffer, deadlineNanos) { it.isComplete } | ||
| } |
There was a problem hiding this comment.
Relocated in acea3f5 to avoid unauthorized requests causing body drains. Similar iOS changes added around ios/Sources/GutenbergKitHTTP/HTTPServer.swift:285.
Points GutenbergKit at the XCFramework snapshot for wordpress-mobile/GutenbergKit#357, which adds the native media upload server and MediaUploadDelegate. Swap to a tagged release before merge.
Points GutenbergKit at the XCFramework snapshot for wordpress-mobile/GutenbergKit#357, which adds the native media upload server and MediaUploadDelegate. Swap to a tagged release before merge.
Add MediaUploadServer backed by the HttpServer library, which handles TCP binding, HTTP parsing, bearer token auth, and connection management. The upload server provides a thin handler that routes file uploads through a native delegate pipeline for processing (e.g. image resize, video transcode) before uploading to WordPress. - MediaUploadDelegate interface with processFile and uploadFile hooks - DefaultMediaUploader for WordPress REST API uploads with namespace support - GutenbergView integration with synchronous server lifecycle - GBKitGlobal nativeUploadPort/nativeUploadToken injection - org.json test dependency added Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add DemoMediaUploadDelegate implementations that resize images to a maximum dimension of 2000px before upload. Includes a toggle in the site preparation screen to enable/disable native media upload processing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Integration tests covering server lifecycle, bearer token auth (407 on missing/wrong token), CORS preflight, routing (404 for unknown paths), delegate processing pipeline, and fallback to default uploader. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Integration tests covering server lifecycle, bearer token auth (407 on missing/wrong token), CORS preflight, routing (404 for unknown paths), delegate processing pipeline, fallback to default uploader, DefaultMediaUploader request format, and error handling for bad requests and non-multipart content types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests covering passthrough behavior (missing port, non-POST, non-media paths, sub-paths, non-FormData), upload interception with Relay-Authorization auth, response transformation to WordPress REST API shape, error handling (413 file too large, generic failures), and abort signal forwarding. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add LocalizedError conformance to EditorHTTPClient.ClientError so that WordPress error messages (e.g. "This file is too large. The maximum upload size is 10 KB.") are surfaced to the user instead of a cryptic Swift type description. Remove the dead 413-specific handling from the JS middleware — the HTTP library rejects oversized uploads at the connection level (never producing an HTTP response the browser can read), so the 413 branch was unreachable. All upload errors now go through the generic path which includes the server's error message. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ponses Parse the WordPress JSON error body to extract the message field (e.g. "This file is too large. The maximum upload size is 10 KB.") instead of showing the raw JSON in the upload failure snackbar. Falls back to the raw body for non-JSON error responses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Compose corsPreflightResponse() from the shared corsHeaders constant instead of re-declaring origin and allowed-headers values. Also removes a redundant "what" comment on the multipart parsing call. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ment Guard the mediaUploadDelegate setter with an identity check so that assigning the same delegate instance does not needlessly stop and restart the upload server. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: Avoid "native" term in user-facing errors messages The term "native" is likely unfamiliar to users, provides no tangible value, and may cause confusion. * fix: drain oversized request body before sending 413 response When Content-Length exceeds maxBodySize, the server now reads and discards the full request body before responding with 413. This ensures the client (WebView fetch) receives the error response cleanly instead of a connection reset (RFC 9110 §15.5.14). Adds a `.draining` parser state that tracks consumed bytes without buffering them, keeping memory and disk usage at zero for rejected uploads. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: include CORS headers on server-generated error responses Add an errorResponseHeaders parameter to HTTPServer (iOS) and HttpServer (Android) so that callers can specify headers to include on all server-generated error responses (413, 407, 408, etc.). MediaUploadServer passes its CORS headers through this parameter so the browser does not block error responses due to missing Access-Control-Allow-Origin. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Revert "fix: include CORS headers on server-generated error responses" This reverts commit 0b6c67b. * fix: route 413 response through handler for CORS headers Instead of the HTTP server library building 413 responses directly (which lacked CORS headers), payloadTooLarge is now treated as a non-fatal parse error. parseRequest() returns the parsed headers as a partial request and exposes the error via a new parseError property. The server passes it to the handler via a serverError field on the request, letting MediaUploadServer build the response with CORS headers — consistent with how OPTIONS preflight is handled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * perf(ios): stream multipart upload body from disk instead of memory Replace Data(contentsOf:) + httpBody with a streaming InputStream via httpBodyStream in DefaultMediaUploader. The multipart body (preamble, file content, epilogue) is written through a bound stream pair on a background thread, keeping peak memory at ~65 KB regardless of file size — down from ~2x file size previously. Android already streams via OkHttp's file.asRequestBody() and needs no changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * perf: passthrough upload when delegate does not modify the file When the delegate's processFile returns the original file unchanged (e.g., GIFs, non-images, files already within size limits), the original request body is forwarded directly to WordPress — skipping multipart re-encoding and the extra file read. Detection: after processFile, compare the returned URL/File to the input. If unchanged and uploadFile returns nil, signal passthrough back to handleUpload which streams the original body via passthroughUpload(). Also extracts shared response parsing into performUpload() on both platforms to avoid duplication between upload() and passthroughUpload(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(android): fix Detekt lint violations Extract readUntil() helper from HttpServer.handleRequest() to reduce nesting depth and throw count. Extract performPassthroughUpload() from MediaUploadServer.processAndRespond() to consolidate throw statements. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ios): prevent integer overflow in drain mode byte tracking Cast `bytesWritten` and `offset` to Int64 individually before subtracting, avoiding a potential Int overflow when the difference is computed before the widening cast. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(ios): replace deprecated URL.path with path(percentEncoded:) URL.path is deprecated on iOS 16+. Use path(percentEncoded: false) to get the file system path without percent-encoding. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(ios): localize "file too large" error via EditorLocalization Add a `fileTooLarge` case to `EditorLocalizableString` so host apps can provide translations for the 413 error message. The hardcoded string in MediaUploadServer now reads from the localization system. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Revert "refactor(ios): localize "file too large" error via EditorLocalization" This reverts commit 71440d9. * fix(ios): review adjustments for #419 (#441) * fix(ios): use Int64 for HTTPRequestParser.bytesWritten Change `bytesWritten` from `Int` to `Int64` for consistency with `expectedContentLength` and `maxBodySize`, which are already `Int64`. * test(ios): add end-to-end test for 413 response with CORS headers Expose maxRequestBodySize on MediaUploadServer.start() and add an integration test that sends an oversized request and verifies the response includes both a 413 status and CORS headers. * fix(test): increase maxRequestBodySize for 413 test The multipart overhead (~191 bytes) plus auth headers meant the previous limit of 100 bytes caused the connection to reset before the drain could complete. Use 1024 bytes with a 2048-byte payload so the parser can drain the body and deliver the 413 response. * fix(test): use raw TCP for 413 test to avoid URLSession connection reset URLSession treats a server response during upload as a connection error (NSURLErrorNetworkConnectionLost). Use a raw NWConnection to send the request and read the response directly, which correctly receives the 413 with CORS headers. * fix: complete drain immediately when body arrives with headers When the entire HTTP request (headers + body) arrives in a single read, the parser enters DRAINING but never completes because the body bytes were already counted in bytesWritten. Subsequent reads find no more data, causing a timeout. Check the drain condition immediately when entering the draining state, transitioning to complete if all body bytes have already been received. Fixes both iOS and Android parsers. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Jeremy Massel <1123407+jkmassel@users.noreply.github.com>
… CORS) (#546) * fix: fall back to default upload path when native server is unreachable The native media upload middleware rethrew on any fetch failure, so a local upload server that was never started, restarted on a new port, or torn down turned every upload into a hard failure. Distinguish a connection-level failure (fall back to next()) from a non-ok response (a real server error that must surface), and re-throw AbortError so an explicit cancellation is not retried. Amplifier fix for the iOS/Android upload-server lifecycle issues. * fix(ios): stop upload server on deinit and hold the delegate weakly viewDidDisappear fires whenever another view controller is pushed or presented over the editor. HTTPServer.stop() cancels the NWListener, which is terminal, so uploads stayed broken after the user returned. Move stop() to deinit. Hold the media upload delegate weakly in UploadContext (re-read per request): mediaUploadDelegate is declared weak, and the strong capture both defeated that contract and risked a retain cycle that kept the view controller — and the server — alive so deinit never fired. * fix(android): re-advertise upload server port and token on restart setGlobalJavaScriptVariables only runs from onPageStarted, so when the media upload delegate setter restarts the server after the page has loaded (e.g. a Compose host constructing a new delegate instance per recomposition), the new port and token were never injected and JS kept fetching a dead port. Patch the two fields into window.GBKit after each (re)start; the window.GBKit guard makes it a no-op before the page loads, where onPageStarted still handles the initial injection. * fix(android): use a 60s timeout for media uploads The upload OkHttpClient used the bare defaults, including a 10s read timeout. WordPress generates image sub-sizes synchronously inside POST /wp/v2/media, so >10s responses are routine on shared hosting — and because the attachment row is created before sub-size generation, a client-side timeout orphans the attachment server-side and duplicates it on retry. Match EditorHTTPClient's 60s policy. * fix(android): return CORS headers on all upload error responses processAndRespond only caught MediaUploadException; an IOException from the upload call, JSON parse errors, a throwing delegate, and "no uploader configured" escaped to HttpServer's header-less 500 fallback, so the browser rejected the preflighted cross-origin fetch with an opaque "Failed to fetch" and hid the real error from the editor. Add a catch-all mapping to the CORS-bearing errorResponse (mirroring iOS), rethrowing coroutine cancellation so it is not swallowed. * fix(android): start upload server for cookie-auth hosts with an upload delegate startUploadServer bare-returned when authHeader was empty, but empty authHeader is in-contract for cookie-auth hosts (auth via setCookies), and a delegate implementing uploadFile can own the upload without the default REST uploader. This made an uploadFile-implementing cookie-auth host silently take the unprocessed WebView path, while the same setup worked on iOS. Build the default uploader only when siteApiRoot and authHeader are present (MediaUploadServer already accepts a null default uploader), and start the server whenever a delegate can handle uploads. * fix: relay WordPress's raw upload response instead of a synthesized shape The native upload server parsed WordPress's media response into a 9-field MediaUploadResult (duplicated in Swift, Kotlin, and JS) and the JS middleware re-synthesized an attachment from it. That dropped media_details.sizes — so every native-uploaded image fell back to sizeSlug: full and embedded the full-resolution original — plus the attachment-page link, distinct raw/rendered fields, and _embedded, and flattened WordPress's status to a local 500. Replace the schema with MediaUploadResponse (status + raw body) and relay WordPress's response verbatim. DefaultMediaUploader returns the raw bytes + status and no longer throws on non-2xx (iOS via a new non-throwing EditorHTTPClientProtocol.performRaw; Android reads the OkHttp response directly). The server relays (status, body) with CORS headers, and errorResponse now emits a {code, message} JSON body so its own errors normalize like a relayed WordPress error. The JS middleware returns response.json() unchanged on success, and on a non-2xx rejects with the parsed WordPress error body (like @wordpress/api-fetch) so media-utils surfaces WordPress's real message and code, falling back to invalid_json on a non-JSON body. The uploadFile delegate now returns MediaUploadResponse? (the raw response); nil still selects the default uploader. Forwarding post/additionalData and ?_embed is a follow-up. * fix: forward media upload fields and query through the native server The native media upload middleware rebuilt the request body with only the file field and dropped the URL query, so post (the attachment's post association), any additionalData, and ?_embed never reached WordPress — the attachment was created with no post_parent and _embedded was always absent. Forward the original request body (all fields) and the original query string from JS. On the native side, carry the incoming query onto the WordPress media URL, and — on the re-encode path where processFile changed the file — preserve the non-file parts in the rebuilt multipart body. The passthrough path already forwards them verbatim once the body is relayed unchanged. Completes the finding-3/4/18 upload-fidelity work; the multipart re-encode dedup is tracked separately in #545. * fix: clean up upload temp files on failure and sweep crash orphans When processFile produced a new file but the upload failed, the processed file leaked (the caller only cleaned up the original); partial writes leaked too; and there was no sweep for files orphaned by a crash. Android also staged temp files under java.io.tmpdir instead of the injected cache dir. Clean up the processed file inside processAndUpload (defer/finally) so the throw paths are covered, and delete the original in the write-failure catch. iOS handleUpload now uses defer + straight-line do/catch instead of the Result/mutable-var dance. Android stages temp files under the injected cacheDir (system-temp fallback). Both platforms sweep upload temp files older than an hour at startup, so crash orphans are reclaimed without disturbing in-flight uploads. Findings 13 and 21. * fix: regenerate Package.resolved against the committed manifest Package.resolved pinned wordpress-rs (branch alpha-20260313), which the root Package.swift never declares — so every swift build/test pruned it and dirtied the tree. Regenerate it from the manifest. The Demo-iOS Xcode project resolves wordpress-rs through its own package reference, so this does not affect the demo. Finding 16. * refactor(ios): remove dead MediaUploadError cases and Data.append extension After the raw-relay refactor, MediaUploadError.uploadFailed/unexpectedResponse are unused (the uploader no longer parses or throws on non-2xx), leaving only streamReadFailed — which UploadError already defines. Fold the two streamReadFailed uses into UploadError and delete MediaUploadError, plus the private Data.append(String) extension the multipart builder no longer uses (the test keeps its own copy). Finding 22 (partial). * fix(android): bake EXIF orientation into resized demo images The demo resize decodes with BitmapFactory (which ignores EXIF orientation) and re-encodes via compress() (which writes no EXIF), so a portrait photo — stored with landscape pixels plus an orientation tag — uploaded rotated. Read the tag and rotate the bitmap before compressing. Demo-only, but it is the reference processFile implementation hosts copy. Finding 15. * fix: normalize the media endpoint URL for unslashed root and namespace The upload endpoint URL was built by raw concatenation, so an unslashed siteApiRoot ("...wp-json") or namespace ("sites/123") produced a malformed URL ("...wp-jsonwp/v2/..." or ".../v2/sites/123media") and a 404. Apply the same normalization RESTAPIRepository uses — trim the root's trailing slash and give the namespace a trailing slash. Latent (in-repo producers pass slashed values today). Finding 7. * fix: give processFile an explicit result with corrected metadata processFile returned a bare URL/File, and the framework inferred "did it change?" by path equality — which conflated "unchanged" with "edited in place" and gave the delegate no way to report a new filename or MIME type. So an in-place EXIF/GPS strip was silently discarded (the original body was forwarded instead), and a format-changing transcode (MOV->MP4) uploaded with the original extension and mime_type. Replace the bare return with ProcessedProxyFile (.original / .processed(url, mimeType:, filename:)). Passthrough is now explicit, so an in-place edit is uploaded rather than dropped, and the delegate reports the resulting metadata, which is used verbatim. processFile also gains a filename parameter so the delegate has the original name to echo or rewrite — without it that data was simply lost. Breaking change to the public MediaUploadDelegate; both demos updated. Findings 11 and 12. * feat: add an opt-in permissive CORS policy to the HTTP server The HTTP library generated some responses itself — the read timeout (408) and pre-handler errors — without CORS headers, so the browser blocked the WebView from reading them and a timeout surfaced as an opaque "Failed to fetch" (and, with the JS fallback, a silent re-upload). Every handler also had to remember to add CORS to its own responses. Add an opt-in cors: .permissive policy to HTTPServer (iOS) / HttpServer (Android). When enabled the library answers the OPTIONS preflight itself and stamps permissive CORS headers on every response at the single send choke point — covering handler responses and the library's own. MediaUploadServer opts in and deletes its bespoke corsHeaders / corsPreflightResponse / OPTIONS handling, so CORS lives in one place. This also de-fangs the finding-14 contract change: the library's error responses now carry CORS regardless of whether a handler inspects serverError. Default policy is .none, so existing consumers are unaffected. Findings 10 and 14. * refactor: classify parse-error disposition (fatal vs recoverable) as an enum The parser decided which parse errors abort the connection vs. are surfaced to the handler with a single implicit condition (!= payloadTooLarge), and the fixture runners accepted an expected error via EITHER channel — so a refactor routing a smuggling-relevant error (e.g. conflicting Content-Length) into the recoverable/handler path would have kept every suite green while letting a malformed request reach the handler before auth. Make the classification typed data: HTTPRequestParseError gains a Disposition (fatal / recoverable), declared per case so a new error can't compile without one, and the parser throws on disposition == fatal. A lock test pins that only payloadTooLarge is recoverable, and the fixture runners now assert the channel matches the error's disposition (threw => fatal, pendingParseError => recoverable) on both platforms. Finding 17. * refactor: extract a shared namespaced REST URL builder RESTAPIRepository's site-root/namespace normalization was duplicated by the media uploader — the one endpoint that bypassed the canonical builder, so the two could drift. Extract it to a shared, tested helper (WordPressRESTURL on iOS, RestUrlBuilder on Android) and route RESTAPIRepository through it. No behavior change; pinned by the existing repository tests plus new builder tests. * fix: harden the native media upload server - Relay non-file multipart fields (post, additionalData) as raw bytes rather than round-tripping through String, which silently mangled any non-UTF-8 value. - Build the media endpoint through the shared URL builder, and carry the request query via percentEncodedQuery on iOS so a non-URL-safe value can't drop it. - Sweep crash-orphaned temp files off the caller's thread via an injectable scope and dispatcher (cancelled on stop; tests inject Unconfined to run it inline). Tests: iOS non-2xx relay, processed-file cleanup (both platforms), orphan-sweep age threshold, and the weak-delegate lifetime that keeps deinit teardown working. * fix(android): gate the upload server on reachability and align its timeouts - Only start when a REST uploader can be built (drop the unreachable cookie-auth branch that 500'd), and clear the advertised port on stop so the JS middleware routes to the default path instead of a dead port. - Skip start when cleartext to localhost is blocked, so a misconfigured host degrades gracefully rather than failing every upload. - Drop the total callTimeout for per-operation inactivity timeouts matching URLSession (15s connect, 60s read/write) so a large upload isn't capped. - Post the JS port/token sync to the WebView's UI thread. * fix: drop the media-upload fallback and relay errors faithfully The connection-error fallback re-ran a non-idempotent POST /wp/v2/media, which could duplicate an attachment, and pointed the wrong way under Lockdown Mode. Reachability is now gated natively, so remove it. Detect cancellation via signal.aborted and rethrow signal.reason (catches AbortSignal.timeout, not just AbortError). Normalize a non-JSON 2xx body to invalid_json like the non-ok path. * fix(android): report the re-encoded image type in the demo delegate The resize demo normalizes everything but PNG to JPEG (Bitmap.compress can't round-trip WebP/HEIC), but returned the original mime/filename — so a WebP upload was JPEG bytes labelled image/webp, which WordPress rejects. Report the actual output type and extension. * fix: route the upload server on the path, not the full target (#557) * feat(http): add path and query accessors to parsed requests The request target carries both the path and the query string, so callers that want to route on the path have to split it themselves. Expose `path` and `query` on both platforms' request types instead. A bare trailing "?" yields an empty query on both platforms, so the value can be appended to an upstream URL unconditionally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: route the upload server on the path, not the full target Media uploads fail with a 404. `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, and the middleware now forwards that query on to the native server so it can be relayed to WordPress. The route guard still compared the full request target against "/upload", so a query string made it miss and return 404 before the upload handler ever ran. Match on `path` instead, and take the relayed query from `query` rather than re-deriving it in the handler. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(upload): normalize a bare trailing "?" to an empty query The native `query` accessors treat a bare trailing "?" as carrying no parameters and yield an empty string, so the value can be appended to an upstream URL unconditionally. The middleware that produces the target still derived the query with `indexOf`/`slice`, which keeps the "?" — `/wp/v2/media?` became `POST /upload?`, a target both platforms document as impossible. Extract `requestQuery` so the rule is stated once on this side and named against its native counterparts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(upload): pin which upload branch relays the query Both `upload` and `passthroughUpload` record `lastQuery` on the mock, so asserting the query alone passes whichever branch ran — a regression that collapsed routing onto one path would still go green. Assert the passthrough branch explicitly, matching the sibling tests. Forcing `processFile` to return `.processed` now fails these tests; the query assertion alone did not. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(http): express `path` with prefix(upTo:) The explicit `startIndex..<separator` range says the same thing as `prefix(upTo:)` with more moving parts, and reads further from its Android counterpart (`substringBefore`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: David Calhoun <github@davidcalhoun.me> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two fixes to `nativeMediaUploadMiddleware`, both aligning it with the existing `mediaUploadMiddleware` it short-circuits past. Strip the `post` field when it is the auto-draft sentinel (`-1`). WordPress rejects attaching media to post `-1`, and because this middleware returns the relay fetch without calling `next()`, `mediaUploadMiddleware` (which normally strips the sentinel) never runs — so the value reached WordPress verbatim and broke uploads on the common new-post flow. Require `nativeUploadToken` in the activation guard. The server always requires authentication, so a port-without-token state would send `Bearer undefined` and 407 instead of falling through to the default upload path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`startUploadServer` built `DefaultMediaUploader` whenever a media upload delegate was set, ignoring whether an auth header was present. It uploads to `/wp/v2/media` with the host's `Authorization` header (the WebView has none of its own), so with an empty header every relayed upload would 401 instead of falling back to the default WebView path. Guard on a non-empty auth header before starting the server, matching the Android guard in `GutenbergView.startUploadServer`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`writeStream` force-unwrapped `OutputStream(url:append:)`, which returns nil if the file can't be opened for writing (uploads directory removed after creation, or a permissions/sandbox failure). That trapped and crashed the process instead of returning the clean 500 the caller already builds. Throw `streamWriteFailed` so it flows into the existing do/catch — temp file removed, 500 relayed to the editor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Partially reverts 93aa2d1. api-fetch runs middleware in reverse registration order, so `mediaUploadMiddleware` (registered later) runs first and has already stripped the auto-draft `post: -1` sentinel by the time `nativeMediaUploadMiddleware` runs — the strip was dead code based on an inverted ordering assumption. The `nativeUploadToken` guard from that commit remains. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The oversized-payload path (drain plus serverError handler dispatch) ran before the bearer-token check, so unauthenticated clients could make the server read an arbitrarily large declared body and reach the handler. Reorder the connection flow on both platforms so auth is validated on headers alone first: unauthenticated oversized requests are rejected with an immediate 407, while authenticated ones keep the full drain and CORS-stamped 413 (RFC 9110 §15.5.14). Add regression tests pinning the ordering — an unauthenticated oversized request must return 407, not the handler's 413. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
HTTPServer.start awaited NWListener readiness with no bound and treated .waiting as "keep waiting" — a listener stuck in that state (which emits no further updates) suspended the caller forever. Since the editor awaits the upload server start before loading the WebView, a stuck listener blocked editor startup indefinitely behind the activity spinner. Race the readiness wait against a startTimeout (default 5s; loopback binds normally complete in milliseconds) and throw failedToStart on expiry, cancelling the half-started listener. Callers already degrade gracefully on failedToStart, falling back to the default upload path. The wait is extracted into an internal firstTerminalState helper because a stuck listener cannot be reproduced deterministically with a real NWListener — tests feed it a hand-built state stream instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fast path (dependencies provided at init) became an async Task when loadEditor started awaiting the upload server, and it was tracked in dependencyTaskHandle — which viewDidDisappear cancels. A transient disappearance (e.g. a full-screen modal presented right after the editor) cancelled the load mid startUploadServer(), surfacing as failedToStart and silently disabling native uploads for the session with no retry. Run the fast path untracked: the disappear-cancel exists to abort the async dependency *fetch*, while the fast path is cheap local work that must run to completion — restoring the guarantee it had when it was synchronous. The weak self capture still makes it a no-op after teardown. Leaving the task untracked is safe because the preceding commit bounds the upload server's listener readiness wait (startTimeout) — the only await here that could hang — so the task always terminates. Reverting that commit alone would reintroduce an unbounded, now-uncancellable suspension on this path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both startup sweeps — cleanOrphanedUploads in MediaUploadServer.start and cleanOrphanedTempFiles in HTTPServer.start — ran synchronous directory scans on the editor-load critical path. Detach them to a utility-priority task instead, mirroring Android's cleanupJob; each server exposes the task so tests can await completion. Detaching cleanOrphanedTempFiles required giving it the same one-hour age threshold the upload sweep already uses: it previously deleted every file in the server's temp subdirectory, which a detached sweep could race against a live instance's in-flight temp buffers. The threshold also removes the documented hazard of same-name instances wiping each other's files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4521918 to
bdb0e57
Compare
Rebasing onto trunk pulled in the pinned SwiftLint CI step (#575), which runs `--strict` and promotes these previously-latent warnings to errors: - `MediaUploadServer.swift`: extra trailing newline and a blank line before a closing brace (auto-corrected). - `EditorView.swift` (demo): a blank line before a closing brace (auto-corrected). - `HTTPServer.swift`: `redundant_nil_coalescing` on `group.next() ?? nil`. This is a false positive on a double optional — `TaskGroup.next()` returns `State??` and `?? nil` flattens it to `State?`. Rewrite as `.flatMap { $0 }` to keep the type while dropping the flagged operator. No behavior change. Verified `make lint-swift` is clean and the package compiles for the iOS Simulator SDK.
* fix: bound upload-server body read by idle timeout, not a total read cap The embedded upload server capped the whole request read at a single 30s total-duration timeout while advertising a 4GB body limit, so a legitimate large upload that streamed steadily for longer than 30s was aborted with a 408 on both platforms. Split the read: the pre-body phase (headers + oversized drain — the unauthenticated-reachable portion) keeps the readTimeout cap, while the accepted body is bounded only by the per-read idle timeout plus a generous bodyReadTimeout (10 min for uploads), so a steadily-streamed body is never failed on total duration. Reject auth-exempt OPTIONS that carry a body (Content-Length > 0) before the drain, so idle-only body reads can't be abused by an unauthenticated OPTIONS with an oversized Content-Length to hold a connection slot. iOS splits the timeout task group via a withReadTimeout helper and adds HTTPServerError.unexpectedBody. Android splits deadlineNanos, makes readUntil cancellable (ensureActive) so shutdown reaps an active connection, and rethrows CancellationException. Adds timeout/OPTIONS regression tests on both platforms. * fix(ios): trap when mediaUploadDelegate is set too late or released early Assigning mediaUploadDelegate after the editor had loaded silently did nothing — the delegate is captured once, at load, into the page's initial window.GBKit config, and there was no observer to react. A delegate that a host set but didn't retain (the property is weak) was likewise silently deallocated before load, disabling native uploads with no error. Keep the weak reference — making it strong would reintroduce the deliberately-avoided VC -> server -> ... -> delegate -> VC retain cycle that leaks the server — and instead fail loudly: a precondition in the setter rejects a write after loading has started, and startUploadServer traps if a delegate that was assigned has already been deallocated. Track mediaUploadDelegateWasAssigned so a premature deallocation is distinguished from a deliberate opt-out (never set / explicitly nil). siteApiRoot is a non-optional URL on iOS, so unlike Android there is no empty-root case to guard. * fix(ios): guard temp-file cleanup so concurrent servers don't wipe live buffers The upload server's start-time orphan sweep deleted every file in its temp directory. Two same-name server instances share that directory (two editors open at once, or one being torn down as another starts — the ARC deinit that stops the old server isn't synchronous with the new one starting), so the second's sweep could delete the first's in-flight request-body buffer and fail that upload with a bufferIOError. Mirror Android's activeFiles guard: register a temp file in a process-wide set while it backs a live request (Buffer/TempFileOwner), and skip registered files in cleanOrphanedTempFiles. Files not in the set have no live owner in this process — they're crash orphans and are still reclaimed. Register before creating the file to close the create-vs-sweep window. * fix(ios): tear down the upload body writer thread on every exit Both upload paths feed URLSession a bound stream pair whose background writer blocks on output.write when the buffer is full. If URLSession abandons the stream without draining it (cancel, or a failure that doesn't close it), the writer blocks forever, leaking the thread and its open file handle; repeated stalls exhaust file descriptors. Close the request's httpBodyStream in a defer around performRaw so the bound pair is always broken and the writer unwinds — on success (no-op, already drained), failure, or cancellation. Covers both the multipart re-encode and file-slice passthrough paths. A BoundStreamTeardownTests case verifies that closing the input unblocks a writer blocked on a full buffer. The verification test also documents the CoreFoundation bound-stream behavior the fix relies on. * test(js): make the upload-cancel tests exercise the abort-vs-network race The abort and timeout tests rejected fetch with the same object they set as signal.reason, so throw options.signal.reason and a regression to throw connectionError were indistinguishable — the tests passed either way. Reject fetch with a distinct network TypeError while the signal is aborted so the tests actually assert the middleware rethrows the signal's reason (the canonical cancellation), not the racing fetch rejection. Verified by mutation: with the bug in place the old tests pass but the rewritten ones fail, and the rewritten tests pass on correct code. * fix(android): don't resurrect the upload server on a detached view The uploadServer field was a plain var (its sibling one line up is @volatile), mutated from the mediaUploadDelegate setter, startUploadServer, and onDetachedFromWindow. onDetachedFromWindow stops the server and won't fire again, so a delegate assigned after detach ran startUploadServer and started a server (bound socket + accept-loop coroutine) that nothing ever stopped — a leak reachable even single-threaded. Mark the field @volatile (matching the sibling) and add an isTornDown flag, set in onDetachedFromWindow and reset in onAttachedToWindow, that startUploadServer checks first — so a detached view never starts a server, while a not-yet-attached view (the legitimate set-delegate-during-construction case) still does. A Robolectric test proves the contrast (server starts on a live view, not after detach); mutation-tested by removing the guard and confirming the leak test then fails. * fix(android): cancel the upload server's coroutine scope when it owns it MediaUploadServer.stop() cancelled cleanupJob but not the CoroutineScope, so when no scope was supplied (the default) the internally-created scope was never cancelled. Production passes a lifecycle-scoped coroutineScope so it was unaffected, but the default path (tests, and any caller relying on it) leaked the scope's Job. Default the scope parameter to null and create an owned scope only when the caller supplies none; stop() cancels that owned scope, while a caller-supplied scope is left to the caller's lifecycle. A test proves stop() cancels the owned scope and not a borrowed one; mutation-tested by dropping the cancel. Verified: with the cancel removed the new test fails; restored, all 19 MediaUploadServer tests pass and detekt is clean. * fix(ios): escape multipart header values to prevent injection multipartBodyStream interpolated the client-supplied filename, form-field names, and MIME type straight into Content-Disposition/Content-Type headers. A value containing a quote or CRLF could break the header line or inject an extra multipart part into the request relayed to WordPress (sanitizeFilename only strips path separators for temp-file naming and wasn't applied here). Percent-encode CR, LF, and double-quote in the quoted name/filename parameters (matching WHATWG's form-data serialization) and strip CR/LF from the MIME type. A test crafts CRLF-injecting values for all three and asserts no fake header survives; mutation-tested by removing the escaping. Bounded by the trust model (the token holder already holds the WordPress credential), so this is defense-in-depth against a malformed/crafted filename rather than a privilege escalation. * docs(ios): document the upload body's non-replayable-stream limitation The upload request body is a one-shot bound-pair stream, so URLSession can't resend it. That only bites on a 307/308 redirect that preserves the POST (301/302/303 downgrade to a bodyless GET; a Bearer 401 doesn't resend), which WordPress core never emits for POST /wp/v2/media. If a proxy/misconfig did, the resend sends an empty body that WordPress rejects — a clean failure, not a corrupt attachment. Document that we accept this rather than add needNewBodyStream handling or buffer the body to a replayable file for so rare a case. * fix(js): reject with a canonical AbortError when an aborted signal has no reason nativeMediaUploadMiddleware rethrew options.signal.reason on a cancelled upload, but an engine that marks a signal aborted without populating reason would make it throw undefined — which @wordpress/media-utils surfaces as a spurious upload failure instead of a silent cancel. Fall back to a DOMException('AbortError') when reason is nullish. A test covers the nullish-reason branch; mutation-tested by dropping the fallback (the test then fails). * fix(ios): cancel the upload relay when the client aborts the connection Once a request is fully read, no bytes flow on the connection until the response is sent, so a handler awaiting slow outbound work — the media upload relay awaiting `POST /wp/v2/media` — leaves the connection idle. If the editor WebView aborted the upload during that window nothing noticed: the outbound request ran to completion, creating an orphaned attachment that a retry then duplicated. Race the handler against the connection's peer closing it. A well-behaved HTTP/1.1 client sends nothing between the request and the response, so a receive posted while the handler runs can only complete on EOF/failure — the client going away. If that wins, cancel the handler, which propagates through structured concurrency to cancel the outbound URLSession task, and skip the doomed send. If the handler wins, the watcher is cancelled without touching the connection, so the response is still sent. * fix(android): cancel the upload relay when the client aborts the connection Once a request is fully read, no bytes flow on the connection until the response is sent, so a handler awaiting slow outbound work — the media upload relay awaiting `POST /wp/v2/media` — leaves the connection idle. If the editor WebView aborted the upload during that window nothing noticed: the outbound request ran to completion, creating an orphaned attachment that a retry then duplicated. Two changes are needed because the outbound call was neither cancellable nor raced: - MediaUploadServer.performUpload now enqueues the OkHttp call inside a suspendCancellableCoroutine that cancels it on coroutine cancellation, instead of a blocking execute() that ignored cancellation entirely. - HttpServer races the handler against the peer closing the connection: a read posted while the handler runs can only complete on EOF/failure — the client going away. If that wins, cancel the handler (which now cancels the outbound call) and skip the doomed send. If the handler wins, stop the watcher and shutdownInput() to unblock its read so the scope joins without waiting for the idle timeout; the response is still written. * fix(ios): don't apply the REST request timeout to media uploads The media upload relay went through the editor's shared EditorHTTPClient, so its `requestTimeout` — applied as `URLRequest.timeoutInterval`, an inactivity timer — governed uploads too. A host that sets a short requestTimeout for snappy REST calls would have it fire during the silent window while WordPress synchronously generates image sub-sizes inside `POST /wp/v2/media`, orphaning the attachment server-side and duplicating it on retry. Android already avoids this with a dedicated upload client that has no total-duration cap; iOS was exposed. Add `EditorHTTPClientProtocol.uploadClient()` (default returns self) and override it on EditorHTTPClient to return a sibling client that reuses the same session (preserving custom configuration/pinning) and auth header but drops `requestTimeout`, so uploads use the request's default 60s inactivity timeout — matching Android. The request-observing delegate is intentionally not carried over: sharing a non-Sendable delegate across two actors would be unsound, and Android has no upload observer either. * fix(android): capture the media upload delegate once at load, matching iOS The mediaUploadDelegate setter started/stopped the upload server reactively and could run on any thread, so it raced onDetachedFromWindow: a delegate assigned (on a background thread) between the setter's isTornDown check and its uploadServer assignment could store a live server into an already-detached view, leaking its socket and accept-loop coroutine for the process lifetime. An @volatile flag gave visibility but not the atomicity the compound check-then-act needed. Match iOS instead of guarding the race: the delegate is captured once, when the page begins loading, and the setter throws if written afterward. The server's whole lifecycle now runs on the UI thread — started from onEditorPageStarted (the onPageStarted hook), stopped in onDetachedFromWindow — so there's no cross-thread window to race. startUploadServer no-ops when no delegate was provided, mirroring iOS's `guard mediaUploadDelegate != nil`. This removes the isTornDown guard and the syncUploadServerJavaScriptVariables re-sync path (there's no post-load (re)start to reflect anymore), which also closes the re-attach gap and collapses the two JS-injection paths into one. Hosts must set mediaUploadDelegate before the editor loads (the demo already does, in the AndroidView factory). Rewrote GutenbergViewUploadServerTest for the new contract: the server starts when the page begins loading, a post-load assignment throws, no delegate means no server, and detach stops it. * fix(ios): bound the HTTP server's wait for the listener to become ready HTTPServer.start awaited the NWListener reaching a terminal state (.ready / .failed / .cancelled) with no timeout, treating every other state as `continue`. A listener stuck in a non-terminal state (e.g. .waiting, unable to establish an endpoint) would never resolve, so the await never returned. Because the editor load awaits the upload server's bind (loadEditor → startUploadServer → MediaUploadServer.start → HTTPServer.start), a stuck bind would hang the entire editor on the loading view — not just native uploads. Race the readiness wait against a `startTimeout` (default 5s) via a new `withStartTimeout` helper, throwing HTTPServerError.startTimeout if the listener isn't ready in time, and cancel the listener on any failure path so its socket isn't leaked. On .ready the group returns the server without throwing, so a successfully-started server never has its listener cancelled. startUploadServer already catches a failed MediaUploadServer.start and falls back to the default WebView upload path, so a bind timeout now loads the editor without native uploads instead of hanging it. A loopback bind completes near-instantly, so 5s only bounds the pathological case. Android is unaffected — its ServerSocket bind is synchronous and either succeeds or throws at once. * fix(ios): don't send a truncated multipart when the upload file can't be fully read The streaming multipart writer read the file with `try?`, which collapsed a thrown read error into the same `break` as a clean EOF and then wrote the closing boundary. Because Content-Length was fixed up front from the file's measured size, a mid-stream read failure — or the file shrinking below that size — produced a body shorter than the advertised Content-Length, so WordPress waited for the missing bytes until it timed out (or rejected) while the real read error was silently swallowed. Extract the write loop into `writeMultipartBody`, which distinguishes a read error and a premature EOF from a clean finish: on either failure it logs the cause and returns without writing the closing boundary, so a short body isn't dressed up as a complete multipart. The upload still fails — we can't send bytes we couldn't read — but the cause is now diagnosable. The detached writer can't propagate an error to the URLSession task, so this is the honest limit of what the streaming design allows. Also captures `preamble` immutably to silence a Sendable-capture warning. Tests cover both paths via an in-memory OutputStream: a clean read writes the full body including the closing boundary; a file shorter than its measured size aborts with no closing boundary. * fix(ios): don't write a response to a connection cancelled mid-handler The upload handler is non-throwing, so on cancellation (editor abort / server stop) it caught the CancellationError/URLError.cancelled and returned a 500 — which HTTPServer would then try to write to a connection being torn down, never reaching its outer `catch is CancellationError`. Since the handler can't rethrow, check `Task.checkCancellation()` after it returns, before sending: a cancelled connection task now propagates to the outer cancellation handler, which just closes the connection instead of writing a doomed response. handleUpload also logs a cancelled upload at debug rather than as an "Upload processing failed" error. Test: a handler that blocks until cancelled, then stop() mid-flight; the client sees the connection close rather than an HTTP response. * fix(android): rethrow coroutine cancellation instead of mapping it to a 500 resolveResponse (and the sibling oversized-body handler path) wrapped the handler in a generic `catch (e: Exception)` that also caught CancellationException — Kotlin's cancellation IS an Exception — and returned a 500, undoing MediaUploadServer's deliberate "never swallow cancellation" rethrow and writing that 500 to a connection being torn down by stop()/detach. Rethrow CancellationException before the generic catch in both spots so it propagates to handleConnection's existing cancellation handler and the connection is closed cleanly. Test: a handler that blocks until cancelled, then stop() mid-flight; the client sees the connection close rather than a 500. * fix(js): treat an abort during the response body read as a cancel, not an error The abort re-check lived only in the fetch()-rejection handler, so a cancellation that landed after the headers arrived but before the body finished streaming took the fulfilled path: response.json() rejected with an AbortError, and the json() catch handlers mapped it to `invalid_json` ("The upload server returned an invalid response."). A clean user cancel surfaced a spurious error notice instead of silently cancelling. Re-check `options.signal?.aborted` in both json() catch handlers (2xx and non-2xx) and surface the cancellation. Extracted the reason/canonical-AbortError logic into `uploadAbortError` so all three sites — the two body-read catches and the fetch-rejection handler — stay consistent. Tests cover an abort during both a 2xx and a non-2xx response body read, asserting the middleware rejects with the signal's reason rather than invalid_json. * feat(ios): let the delegate decline a file by metadata to skip the temp copy handleUpload always streamed the uploaded part to a temp file before dispatch, because both processFile and uploadFile take a materialized file URL. When the delegate then declined the file (processFile → .original, uploadFile → nil) the copy was deleted unused — a full disk read+write of, say, a 200 MB video handed to an image-only delegate, purely to be passed through. Add MediaUploadDelegate.handlesFile(ofType:named:), a metadata-only gate (defaulting to true) the server consults before materializing the file. When it returns false, handleUpload forwards the original request body directly with no temp copy. It gates the copy needed by both processFile and uploadFile, so it means "will I process or upload this?"; a true is not a commitment, since processFile can still return .original after inspecting the bytes. Existing delegates are unaffected by the default. Extracted passthroughResponse/relayResponse/uploadErrorResponse so the new gate path and the existing .passthrough path share the forward-and-relay logic. The demo delegate now declines non-image files, exercising the fast path. Test: a delegate that declines by metadata is never asked to process (proving the file wasn't materialized) and the upload is passed through. * feat(android): let the delegate decline a file by metadata to skip the temp copy handleUpload always copied the uploaded part to a temp file before dispatch, because both processFile and uploadFile take a File. When the delegate then declined the file (processFile → Original, uploadFile → null) the copy was deleted unused — a full disk read+write of, say, a 200 MB video handed to an image-only delegate, purely to be passed through. Add MediaUploadDelegate.handlesFile(mimeType, filename), a metadata-only gate (defaulting to true) the server consults before writePartToTempFile. When it returns false, handleUpload forwards the original request body directly with no temp copy. It gates the copy needed by both processFile and uploadFile, so it means "will I process or upload this?"; a true is not a commitment, since processFile can still return Original after inspecting the bytes. Existing delegates are unaffected by the default. Mirrors iOS. Extracted passthroughResponse/relayResponse so the new gate path and the existing Passthrough path share the forward-and-relay logic. The demo delegate now declines non-image files, exercising the fast path. Test: a delegate that declines by metadata is never asked to process (proving the file wasn't materialized) and the upload is passed through. * fix(android): start connection handlers ATOMIC so a shutdown race can't leak the socket The accept loop acquires a semaphore permit and accepts a socket, then launches the per-connection handler with the default start mode. If stop() cancels the scope in the window between tryAcquire() and the child being dispatched, a DEFAULT-started child cancelled before it begins skips its body entirely — so neither the `finally` (release the permit) nor handleConnection's `socket.use` (close the fd) runs, leaking the accepted socket until GC finalization. Launch the handler with CoroutineStart.ATOMIC, which guarantees the body begins even if the scope is already cancelled: it enters `socket.use` and hits readUntil's first `ensureActive()`, which throws and unwinds cleanly through both the socket close and the permit release. This is a dispatch-timing race (stop() landing in a sub-millisecond window), so it isn't practically reproducible in a deterministic test without injecting the server's dispatcher; the fix relies on ATOMIC's documented semantics and is documented inline for future travellers. * fix(js): normalize a native-upload transport failure to api-fetch's error shape On a genuine transport failure the middleware rethrew the raw fetch rejection — a code-less TypeError ("Failed to fetch") with an untranslated message. Because the native middleware short-circuits next() and runs its own fetch(), the default handler's normalization never ran, so a native-upload transport failure reached consumers differently from a direct upload's: media-utils and anything keying off error.code saw code === undefined and an English-only message. Mirror @wordpress/api-fetch's default handler: throw { code: 'offline_error' | 'fetch_error', message } with the same codes and i18n strings (so the existing translations apply). The abort case is already handled earlier via uploadAbortError, and the deliberate no-retry behavior is unchanged. Tests assert the normalized shape on both the online (fetch_error) and offline (offline_error) paths, and that neither retries. * docs(ios): explain the mediaUploadDelegate precondition is a deliberate fail-fast The setter's `precondition(!hasStartedLoading)` enforces the documented "set the delegate before the editor loads" contract: the delegate is captured into the page's initial configuration at load, so a late assignment would silently never take effect, and trapping surfaces that misuse loudly. A review flagged it as a potential production crash on the theory that `hasStartedLoading` flipping inside the async load makes the timing non-deterministic. That's backwards — the flip runs at or after viewDidLoad, so it only widens the safe window; a host that follows the contract can't race it. Document the rationale at the call site so it reads as intentional and isn't re-flagged, and warn against softening it to a silent no-op. * refactor(ios): route recoverable parse errors through an HTTPServerDelegate A recoverable parse error (today only an over-limit body → 413) was surfaced to the main handler as `Request.serverError`, an optional the handler had to remember to check. Forgetting it meant treating a drained, body-less, rejected request as normal: the debug server and the demo's proxy both returned `200 {"status":"ok"}` for an oversized request. Only MediaUploadServer got it right — because someone remembered. Make it structural instead of a documented contract: - Delete `serverError` from `HTTPServer.Request`. The main handler now only ever sees valid, fully-read requests — a rejected request is unrepresentable in a handler, so the false-200 bug can't happen. - Add `HTTPServerDelegate`, an optional, retained, all-defaulted protocol. The library owns the recoverable-error response (fail-safe), and a consumer only overrides `response(forRecoverableParseError:)` to make the body nicer. Future customization points become new defaulted methods, not new `start` parameters. - Expose `HTTPServer.defaultErrorResponse(for:)` and collapse the fatal-error path onto it — one source of truth for the generic error response. - MediaUploadServer supplies a leaf `ServerDelegate` returning its JSON `{code, message}` 413, so the editor still shows "The file is too large to upload in the editor." — behaviour-identical, just relocated out of the hot path. The debug server and demo need no changes and stop lying. The invariant: a consumer can only make a rejected request's error prettier, never make a rejected request look accepted. Tests: a recoverable error is answered by the library and never reaches the handler; a delegate customizes it. * refactor(android): route recoverable parse errors through an HttpServerDelegate Mirror of the iOS change. A recoverable parse error (today only an over-limit body → 413) was surfaced to the main handler as `HttpRequest.serverError`, an optional the handler had to remember to check. Forgetting it meant treating a drained, body-less, rejected request as normal. Make it structural: - Delete `serverError` from `HttpRequest`; the handler only ever sees valid, fully-read requests, so a rejected request can't be mistaken for a normal one. - Add `HttpServerDelegate`, an optional interface with a defaulted `responseForRecoverableParseError`. The library owns the recoverable-error response (fail-safe); a consumer overrides only to make the body nicer. Future customization points are new defaulted methods, not new constructor params. - Add `HttpServer.defaultErrorResponse(error)` and collapse both fatal-error catches onto it — one source of truth. This also deletes the fake body-less HttpRequest construction and its try/catch from the recoverable path. - MediaUploadServer implements the delegate (no cycle concern on the JVM) and returns its JSON `{code, message}` 413, so the editor still shows "The file is too large to upload in the editor." Rewrote the auth test that encoded the old contract to demonstrate the fail-safe default, and added handler-bypass + delegate-customization tests to match iOS. * docs: explain why the upload server's CORS is `*` and not an origin allowlist A reviewer flagged `Access-Control-Allow-Origin: *` as a weak spot. It's a deliberate, safe choice: the server is loopback-only, every non-OPTIONS request is gated by a per-session random bearer token stored only in the editor origin's origin-scoped storage (no cross-origin can read it), so `*` only governs whether a token-holding origin can read the response — and the only token-holder is the editor itself. Echoing a specific origin isn't viable anyway since the editor loads from file:// (Origin null). Document the rationale at the emit site on both platforms so it reads as intentional and isn't re-flagged. * refactor(js): only route a genuine File through the native upload path Tighten the upload middleware's file guard from a truthiness check to `file instanceof File`. `FormData.get('file')` can return a File, a string (a non-file field named `file`), or null; the `instanceof` check covers the missing-field and wrong-type cases at once, so a non-file body takes the default path instead of being routed to the native server (which would only 400 it), and the `file.name` log is always safe. Also document, at the non-2xx throw site, that throwing the parsed body verbatim — even when it isn't the usual WordPress `{ code, message, data }` shape — is a deliberate mirror of @wordpress/api-fetch's `parseAndThrowError`, so a future reader doesn't mistake it for a missing-normalization bug. Test: a FormData whose `file` field is a string passes through untouched. * fix(android): resume the upload coroutine when the response-body read fails performUpload reads the response body inside OkHttp's onResponse callback. OkHttp marks the callback signalled before invoking onResponse, so a throw from there — a truncated/reset body after WordPress sent its 201 headers, or the read timeout firing mid-body — is swallowed (logged, never routed to onFailure). The continuation was never resumed, so the upload coroutine hung forever holding a connection-semaphore permit; five such events exhaust maxConnections and the server stops accepting. Read the body inside a try/catch and resume the continuation from the catch, restoring the prompt failure the pre-enqueue execute().use{} path produced. Regression test drives a real upload through MockWebServer with DISCONNECT_DURING_RESPONSE_BODY and a withTimeout guard; without the fix the coroutine hangs and the test trips the timeout. * fix(ios): carry the request-observing delegate into the upload client uploadClient() built its sibling without the EditorHTTPClientDelegate, so a host observing "all network requests" (as the protocol doc promises) saw zero POST /wp/v2/media uploads or passthroughs — they route through the delegate-less sibling. Constrain EditorHTTPClientDelegate to Sendable so the nonisolated uploadClient() can read and share it, and carry the delegate over; only the REST requestTimeout is still dropped (its whole purpose). The observer now sees uploads too, making the doc accurate. Tightening the protocol to Sendable is source-compatible in-repo (the test spy is already @unchecked Sendable); external non-Sendable conformers would need to add the conformance. * test: pin the racing-close half-close behavior and document it The connection close-watcher races the in-flight handler against the peer closing the connection: a read EOF means the client went away, so it cancels the handler and skips the doomed response — which is what tears down an aborted upload's outbound POST /wp/v2/media before it can orphan an attachment a retry then duplicates. A read EOF can't distinguish a full close from a legal client write-half-close (shutdown(SHUT_WR) after the request, read half kept open for the response), so a half-close is treated the same way — handler cancelled, response dropped. That's deliberate: the sole client is the WebView fetch, which never half-closes and fully closes on abort, and telling the two apart would forfeit the prompt cancellation the watcher exists for (they're only distinguishable by attempting the write, too late to cancel a doomed upload). Add a regression test on both platforms that half-closes mid-handler (NWConnection .finalMessage on iOS, Socket.shutdownOutput() on Android) and asserts the handler is cancelled and no response is sent, so a future change can't "fix" the half-close and silently resurrect the orphan bug. Name the assumption in the runHandler / resolveResponseRacingClose and waitForConnectionClose / awaitPeerClose docs. Comment-only source change; no behavior change. * test(ios): reconcile parent's start/cleanup tests after reparenting onto the rebased base Rebasing this branch onto the rebased parent replayed the hardening commits but dropped the merge commit that carried the earlier conflict reconciliation, so two adaptations had to be re-applied against the parent's now-duplicate fixes: - HTTPServerStartTests: drop the two firstTerminalState unit tests. This branch keeps its own withStartTimeout + HTTPServerError.startTimeout and drops the parent's firstTerminalState helper, so those tests no longer compile; the same behavior is covered by HTTPServerTimeoutTests. The parent's end-to-end port-conflict test is kept. - RFC9112ConformanceTests: adapt the orphan-cleanup test to this branch's ActiveTempFiles registry (register the live file), instead of the parent's age threshold, which this branch replaced. The parent's swiftlint:disable comments elsewhere in the file are left intact. Also strip three stray blank lines the 3-way merge left in MediaUploadServer.swift and EditorView.swift (SwiftLint trailing_newline / vertical_whitespace_closing_braces). Verified: make lint-swift clean; host swift test 959/959; iOS Simulator build-for-testing succeeds.
Points GutenbergKit at the XCFramework snapshot for wordpress-mobile/GutenbergKit#357, which adds the native media upload server and MediaUploadDelegate. Swap to a tagged release before merge.
Moves the pin off the pr-build/357 snapshot now that wordpress-mobile/GutenbergKit#357 has merged. Trunk also carries the follow-up hardening in #561, which adds a defaulted handlesFile(ofType: named:) to MediaUploadDelegate, so GBKMediaUploadProcessor conforms unchanged. No tagged release includes #357 yet — v0.19.0 predates it. Swap to a tagged release before merge.
Points GutenbergKit at the XCFramework snapshot for wordpress-mobile/GutenbergKit#357, which adds the native media upload server and MediaUploadDelegate. Swap to a tagged release before merge.
Moves the pin off the pr-build/357 snapshot now that wordpress-mobile/GutenbergKit#357 has merged. Trunk also carries the follow-up hardening in #561, which adds a defaulted handlesFile(ofType: named:) to MediaUploadDelegate, so GBKMediaUploadProcessor conforms unchanged. No tagged release includes #357 yet — v0.19.0 predates it. Swap to a tagged release before merge.
What?
Adds a native media upload pipeline that routes file uploads through a local HTTP server on iOS and Android, enabling the host app to process files (e.g., resize images, transcode video) before they are uploaded to WordPress.
Why?
Ref CMM-1249.
Gutenberg's built-in upload path sends files directly from the WebView to the WordPress REST API with no opportunity for native processing. Host apps need to resize images, enforce upload size limits, or apply other transformations before upload. This pipeline gives the native layer full control over media processing while keeping the existing Gutenberg upload UX (blob previews, save locking, entity caching) unchanged.
How?
Architecture: A localhost HTTP server runs on each platform, built on the
GutenbergKitHTTPlibrary (iOS) andHttpServer(Android) from #367. The library handles TCP binding, HTTP/1.1 parsing, bearer token authentication (Relay-Authorization), multipart form-data parsing, connection limits, and disk-backed body buffering. The upload server is a thin handler on top.JS layer:
nativeMediaUploadMiddlewareinapi-fetch.jsinterceptsPOST /wp/v2/mediarequests whennativeUploadPortis configured inwindow.GBKit, forwarding the original request body (the file plus every sibling field —post, additionalData) and query string (e.g.?_embed) to the local server. The native server relays WordPress's response verbatim: on success the middleware returns WordPress's attachment object unchanged (preservingmedia_details.sizes,_embedded, distinct raw/rendered fields, etc.), so the existing Gutenberg upload pipeline works unchanged@wordpress/api-fetchand rejects with the parsed WordPress error body ({ code, message, data }) so@wordpress/media-utilssurfaces WordPress's real message and code, falling back to aninvalid_jsonerror on a non-JSON body/wp/v2/mediabut not/wp/v2/media/123or/wp/v2/media-categories) to avoid intercepting non-upload requestsNative layer:
MediaUploadDelegateprotocol/interface withprocessFile(resize/transcode) and optionaluploadFilefor a custom upload, which returns the raw WordPress response (MediaUploadResponse?); returningnilfalls back to the default uploaderDefaultMediaUploaderas fallback, uploading to/wp/v2/mediavia the host's HTTP client, with site API namespace support for namespaced sites. It returns WordPress's raw bytes and status without throwing on non-2xx, so the server can relay the exact responseuploadFilestill starts the server{ code, message }JSON body with CORS headers so they normalize like a relayed WordPress error; oversized bodies are drained before the 413 so the WebView receives a clean response rather than a connection resetContent-DispositionvaluesDemo apps:
Reliability & lifecycle:
POST /wp/v2/mediacould duplicate an attachment the server had already relayed. Reachability is gated proactively upstream instead (the middleware skips the native path when no port is advertised, and the native side only advertises a reachable port). A caller-initiated cancellation propagates as the cancellation (detected viasignal.aborted) rather than being retrieddeinit(notviewDidDisappear, which also fires when another view controller is presented over the editor) and holds the delegate weaklywindow.GBKitafter a restart (e.g. a Compose host constructing a new delegate per recomposition) and uses a 60s upload timeout to matchEditorHTTPClient, since WordPress generates image sub-sizes synchronously insidePOST /wp/v2/mediaKey design decisions
MediaUploadDelegatethat can handle uploads, keeping the default behavior unchangedapi-fetchmiddleware overmediaUploadeditor setting: Ideally, media uploads would be handled via themediaUploadeditor setting (see the Gutenberg Framework guides), but GutenbergKit uses Gutenberg'sEditorProviderwhich overwrites that setting internally. Until GutenbergKit is refactored to useBlockEditorProvider, theapi-fetchmiddleware approach is necessary.Alternatives considered
JS Canvas resize + native inserter resize — Two separate implementations:
createImageBitmap()+OffscreenCanvasin JS for web uploads,CGImageSourceCreateThumbnailAtIndexinMediaFileManager.import()for the native inserter. Ships fastest and lowest complexity, but Canvas resize quality is lower than native, two codepaths to maintain, and a dead-end for video (client-side transcoding in a WebView is impractical).Native upload pipeline via local HTTP server (this PR) — A single
api-fetchmiddleware intercepts allPOST /wp/v2/mediarequests and routes files through a localhost server for native processing. Covers every upload path (file picker, drag-and-drop, paste, programmatic, plugin blocks) with native-quality processing. Scales to video transcoding. More upfront work than option 1.Replace
MediaPlaceholderviawithFiltershook — Useeditor.MediaPlaceholderandeditor.MediaReplaceFlowfilters withhandleUpload={false}to deliver rawFileobjects toonSelect, then route to native. Incomplete coverage: misses block drag-and-drop re-uploads (handleBlocksDropcallsmediaUploaddirectly), directmediaUploadcalls from plugins, and loses blob previews whenhandleUploadis false.instanceof FileListchecks are fragile in WebView contexts.Redirect to native UI on large files — Keep the web upload button, but show a native dialog when files exceed limits. Awkward UX (user already picked a file, now asked to pick again differently). On iOS, the already-selected JS
Filecan't be handed to native for optimization. Two parallel upload paths add complexity.JS resize for images + hide web upload for video blocks — JS Canvas resize for images, hide the "Upload" button on video-accepting blocks via
editor.MediaPlaceholderfilter (forcing users to Media Library for video). Users can't drag-and-drop videos, blocks accepting both image and video (Cover) get complicated, and it's a dead-end architecture.Client-side processing via
@wordpress/upload-media(WASM libvips) — Gutenberg's experimental@wordpress/upload-mediapackage includes a WASM build of libvips for high-quality client-side image resizing, rotation, format transcoding, and thumbnail generation. Quality is comparable to server-side ImageMagick. However, it requiresSharedArrayBufferfor WASM threading, which is only available in cross-origin isolated contexts — WKWebView loads GutenbergKit's HTML locally with no HTTP headers, soSharedArrayBufferis unavailable. WebKit also lackscredentiallessiframe support, meaning cross-origin isolation would break third-party embeds (YouTube, Twitter, etc.). Single-threaded WASM fallback is unvalidated, and the package's memory footprint (50-100MB+ per image) is a concern under iOS jetsam pressure. Not viable today, but worth revisiting if the package decouples its store/queue management from WASM processing (tracked upstream).A key constraint is platform asymmetry: Android can intercept web
<input type="file">viaonShowFileChooser(), but iOS cannot —WKWebViewhandles file selection internally. This rules out purely native interception strategies for web-originated uploads and motivated the localhost server approach, which works identically on both platforms.Testing Instructions
Accessibility Testing Instructions
The toggle follows the same pattern as the existing "Enable Native Inserter" toggle — no new UI beyond that.
Screenshots or screencast
N/A — backend/infrastructure change with no visible UI changes beyond the demo app toggle.