fix: native media upload review follow-ups (relay, delegate contract, CORS)#546
Open
jkmassel wants to merge 21 commits into
Open
fix: native media upload review follow-ups (relay, delegate contract, CORS)#546jkmassel wants to merge 21 commits into
jkmassel wants to merge 21 commits into
Conversation
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.
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.
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.
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.
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.
…d 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.
…hape
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.
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.
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.
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.
…ension 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).
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.
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.
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.
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.
…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.
github-actions
Bot
added
the
[Type] Bug
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/546")Built from 0e88279 |
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.
- 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.
…meouts - 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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups from reviewing the native media upload pipeline in #357, plus a code-review hardening pass on those follow-ups. Stacked on #357; no new feature surface.
The load-bearing fix: the editor now gets WordPress's upload response verbatim. #357 reshaped that response into a hand-built subset, which dropped
media_details.sizes— so every native-uploaded image fell back to full resolution in the editor. The proxy now changes only the bytes that get uploaded, not the response contract, so every consumer — image sub-size selection, gallery links, error notices — behaves exactly as a non-proxied upload.Response fidelity
post, any additional form fields, and the?_embedquery — rather than the file alone. Non-file fields are relayed as raw bytes through the re-encode, so a non-UTF-8 value can't be silently coerced.Delegate contract
processFilereturns aProcessedProxyFile(.original, or.processed(url, mimeType:, filename:)) instead of a bare path. Passthrough vs. processed was previously inferred by path equality, which conflated "unchanged" with "edited in place" and left the delegate no way to report a corrected type or name — so a format change (HEIC→JPEG, MOV→MP4) uploaded under the original extension. The transformed file's type and name are now explicit.Server lifecycle & reachability
deinitrather thanviewDidDisappear, which was tearing it down whenever the editor was merely backgrounded or covered; hold the delegate weakly to break a retain cycle.localhostis blocked (NetworkSecurityPolicy) so a host that hasn't allow-listed it degrades gracefully instead of failing every upload. Sweep crash-orphaned temp files off the caller's thread, and post the port/token sync to the WebView's UI thread.POST /wp/v2/media(risking a duplicate attachment) and pointed the wrong way under Lockdown Mode, where the loopback proxy is the path that works. A user-initiated cancel is still propagated — keyed offsignal.aborted(soAbortSignal.timeout()is honored, not justAbortError), rethrowing the signal's canonical reason.Timeouts
callTimeout. A large upload that keeps making progress is never capped by total duration; only a genuine 60s stall in the active direction fails.CORS
permissivepolicy. The library answersOPTIONSand stamps CORS headers on every response — including the ones it generates itself (read timeout, parse error) that never reach the upload handler and previously surfaced in the editor as an opaqueFailed to fetch. Each server opts in instead of hand-maintaining CORS headers per response.Parse-error classification
fatalorrecoverablethrough an explicitDisposition, pinned by a test: only an over-limit payload (413) is recoverable, so a smuggling-relevant malformed request can't silently be reclassified to reach the handler ahead of auth.Cleanup
WordPressRESTURLon iOS,RestUrlBuilderon Android) so the media endpoint stops being the one path that bypassesRESTAPIRepository's canonical builder and can't drift from it. On iOS the request query is now merged viaURLComponents.percentEncodedQuery, so a value that isn't URL-safe can't drop?_embed.image/jpeg/.jpgrather than JPEG bytes mislabeledimage/webp.MediaUploadErrorcases and an unusedData.appendextension; regeneratePackage.resolvedagainst the committed manifest.Test plan
Automated (all green):
api-fetch-upload-middleware.test.js: interception, verbatim relay, WordPress-error surfacing,invalid_jsonnormalization on a non-JSON body (both 2xx and non-2xx), abort/timeout propagation without fallback, and body + query forwarding.swift test:GutenbergKitHTTP(request-parsing fixtures + the fatal/recoverable disposition lock),WordPressRESTURL, andMediaUploadServer(verbatim relay incl. a real non-2xx path, processed-metadata forwarding, processed-file cleanup, the orphan-sweep age threshold, the weak-delegate lifetime that keepsdeinitteardown working, and the 413 + CORS path)../gradlew :Gutenberg:testDebugUnitTestanddetektgreen:RestUrlBuilder,MediaUploadServer(relay / metadata / URL normalization / non-UTF-8 field bytes / orphan-sweep), the shared HTTP fixtures, and the disposition lock.Manual:
largesize (not full resolution), attaches to the post, and logs the resize.deinitchange keeps the server alive across backgrounding).