fix: harden the native media upload server (review of #357) - #561
Merged
jkmassel merged 32 commits intoAug 14, 2026
Merged
Conversation
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/561")Built from 3634a02 |
jkmassel
marked this pull request as draft
July 24, 2026 04:58
jkmassel
force-pushed
the
feat/leverage-host-media-processing-v1
branch
from
August 12, 2026 23:45
2dd181c to
8a5430f
Compare
jkmassel
marked this pull request as ready for review
August 13, 2026 17:59
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.
…arly 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.
…ve 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.
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.
…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.
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.
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.
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.
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.
…s 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).
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.
…ection 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.
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.
…g 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.
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.
… 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.
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.
… 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.
…t 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.
…mp 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.
…e 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.
…'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.
…rror 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.
…te 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.
…legate
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.
…erDelegate
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.
…llowlist 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.
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.
… 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.
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.
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.
…nto 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.
jkmassel
force-pushed
the
feat/leverage-host-media-processing-v1
branch
from
August 13, 2026 18:05
8a5430f to
3634a02
Compare
dcalhoun
approved these changes
Aug 14, 2026
dcalhoun
left a comment
dcalhoun
left a comment
Member
There was a problem hiding this comment.
Thanks for the effort put into this! 🙇🏻♂️
I stepped through each commit, focusing on review of the logic rather than tests. All the changes look good to me. I left a few inline suggestions to consider. None are blocking.
I tested this branch in the Demo app on both iOS and Android. I tested media uploads for Simple, Atomic, and self-hosted sites. I did not encounter issues while testing. Images were correctly resized when necessary as configured by the Demo app, others were passthrough to the WordPress server.
| @@ -444,6 +504,28 @@ public final class HTTPServer: Sendable { | |||
| connectionTasks.track(taskID, task) | |||
Member
There was a problem hiding this comment.
Capturing a note from Claude worth considering:
The task is created on line 348 but only registered here. If
stop()runs in that window,cancelAll()iterates a map that doesn't yet contain this task and then clears it —track()then inserts an uncancelled task nobody will ever cancel.This is an unstructured
Task, so nothing else propagates the cancellation. Past thetry Task.checkCancellation()on line 476, the handler runs the relay to completion andPOST /wp/v2/medialands — the orphaned attachment the close-racing work exists to prevent, via a different door.Android isn't exposed to this:
launchmakes the handler a child ofscope, soscope.cancel()instop()covers children that haven't dispatched yet. (CoroutineStart.ATOMICthere solves a different problem — a pre-dispatch cancel skipping the body and leaking the fd + permit.) The iOS registry has no parent/child relationship, so it needs the latch explicitly.Suggest latching a stopped flag in
ConnectionTasks(line 882, outside this diff):final class ConnectionTasks: @unchecked Sendable { private let lock = NSLock() private var tasks: [UUID: Task<Void, Never>] = [:] private var isStopped = false func track(_ id: UUID, _ task: Task<Void, Never>) { let cancelImmediately: Bool = lock.withLock { guard !isStopped else { return true } tasks[id] = task return false } // Registered after cancelAll() — the server is already stopping, so this // task would otherwise never be cancelled. if cancelImmediately { task.cancel() } } func cancelAll() { lock.withLock { isStopped = true for task in tasks.values { task.cancel() } tasks.removeAll() } } }
| @@ -417,7 +473,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro | |||
| } | |||
Member
There was a problem hiding this comment.
Capturing a note from Claude worth considering:
(Re: the
authHeaderguard just above — line 471 isn't in the diff, so anchoring here.)This guard omits
isOfflineModeEnabled.GBKitGlobalnilssiteURL/siteApiRootin offline mode ("No site exists; skip all networking entirely"), butDefaultMediaUploaderis built fromconfiguration.siteApiRootdirectly — so an offline host that still supplies anauthHeadergets a live server that would relay to the real site.Low impact in practice:
EditorService.prepare()short-circuits to empty dependencies in offline mode, so there's little to upload against. Filing it for contract consistency rather than an observed failure.guard !configuration.isOfflineModeEnabled, !configuration.authHeader.isEmpty else { return }
Comment on lines
+122
to
+124
| // legitimate client. Echoing the origin isn't viable anyway: the | ||
| // editor loads from file:// (Origin null), which can't be cleanly | ||
| // allowlisted. |
Member
There was a problem hiding this comment.
The last clause is true on iOS but not here. Android sets a custom or default asset domain. Local files, but served from the site's own origin, not file://. So echoing the origin is viable on Android; it's just unnecessary, since the token already gates every non-OPTIONS request.
The rest of the rationale holds unchanged. Worth correcting, though.
Suggested change
| // legitimate client. Echoing the origin isn't viable anyway: the | |
| // editor loads from file:// (Origin null), which can't be cleanly | |
| // allowlisted. | |
| // legitimate client. An origin allowlist would be viable here | |
| // (the editor document is served from the site's own origin — | |
| // see `assetDomain` in `GutenbergView`), but it would add | |
| // nothing the token gate doesn't already provide. |
9 tasks
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
3 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.
Adversarial review of the native media upload server introduced in #357, with the findings fixed. Twelve changes across iOS, Android, and the JS middleware — timeout, lifecycle, concurrency, resource-leak, and injection bugs — each verified, and the behavior-changing ones mutation-tested (remove the guard → the new test fails → restore). The last two (
8d836f42,dfa7f855) came from a second prosecutor/defender validation pass over the already-hardened code.Stacks on #357: the base is
feat/leverage-host-media-processing, so this merges into that PR's branch rather than trunk.Summary
Correctness
b0739f10, both platforms) — the read timeout was a single 30s total-duration cap over the whole request while the body limit is 4 GB, so a legitimate large upload that streamed steadily for more than 30s got a 408. Split the read: the pre-body phase (headers + oversized drain — the unauthenticated-reachable part) keeps the total cap; the accepted body is bounded by the per-read idle timeout plus a generous ceiling. Coupled fix: reject auth-exemptOPTIONSthat carry a body, so an unauthenticatedOPTIONSwith an oversizedContent-Lengthcan't abuse the now idle-only body read.mediaUploadDelegateset after load silently did nothing (cdb1683b) — it's captured once into the page config at load. Kept theweakreference (making it strong reintroduces a deliberately-avoided retain cycle that would leak the server) and made both misuse paths trap: set-after-load, and a delegate deallocated before load.8a08a084) — the start-time temp-file sweep deleted every file in a shared directory, and two editors (iPad multi-window, or one tearing down as another starts) share it. Added Android'sactiveFilesguard so the sweep skips live buffers while still reclaiming crash orphans.d7cddecb) — a delegate assigned afteronDetachedFromWindowstarted a server that nothing would ever stop.@Volatilethe field (matching its sibling) plus a torn-down guard.8d836f42) — theexecute()→enqueuerefactor moved the body read inside OkHttp'sonResponse, which OkHttp marks signalled before invoking, so a throw there (a truncated/reset body after WordPress's201, or the read timeout firing mid-body) is swallowed — logged, never routed toonFailure. The continuation never resumed, so the coroutine hung holding a connection-semaphore permit; five exhaustmaxConnectionsand the server stops accepting. Resume from acatch (IOException), restoring the prompt failure the synchronousexecute().use{}produced.dfa7f855) —uploadClient()built its sibling without theEditorHTTPClientDelegate, so a host wiring one to observe all requests (as the protocol doc promises) saw zeroPOST /wp/v2/mediauploads or passthroughs. Constrained the protocol toSendableand carried the delegate over; only the REST timeout is still dropped. Source-compatible in-repo (the test spy is already@unchecked Sendable); external non-Sendableconformers would need the conformance.Resource leaks
63d046a1) — the bound-pair writer blocks forever if URLSession abandons the body stream; close the input on every exit so it unwinds.stop()didn't cancel an internally-created coroutine scope (70497ea3).Security hardening
ebc9e802) — client-supplied filename, field names, and MIME type were interpolated intoContent-Disposition/Content-Typeunescaped, so a"or CRLF could inject headers or an extra part into the request relayed to WordPress. Escape at the serialization boundary. Bounded by the trust model (the token holder already holds the WordPress credential), so this is defense-in-depth rather than privilege escalation.JS middleware
throw undefined(b7b39a5e) when the abort signal carried noreason; fall back to a canonicalAbortError.4aa967ca) — they rejectedfetchwith the same object they asserted on, so a regression that rethrew the network error would have passed. Rewritten to exercise the abort-vs-network race.Documented, not fixed
55867b64) — a307/308redirect on the mediaPOST(which WordPress core never emits) would resend an exhausted, empty body → a clean failure, not a corrupt attachment. Documented the limitation rather than addingneedNewBodyStreamplumbing for so rare a case.Determined non-issues
siteApiRootguard — the finding assumed Android's empty-Stringcheck applies, but iOS'ssiteApiRootis a non-optionalURL, so there is no empty state to guard against.NetworkSecurityPolicycheck. Android-specific by nature.Test plan
GutenbergKitHTTPsuite (386 tests) plus new timeout / OPTIONS-with-body / temp-file-cleanup / bound-stream-teardown testsGutenbergKitupload and multipart suites, including a new header-injection testEditorViewControlleris#if canImport(UIKit), so hostswift buildcompiles it empty; verified withxcodebuild -destination 'generic/platform=iOS Simulator':gutenbergunit tests anddetekt, including new Robolectric detached-view-leak and owned-scope-cancel testseslintAbortErrorfallback, scope cancel, cancel-test rewrite) — removed each guard, confirmed the new test fails, restoredMediaUploadServertruncated-response regression test — mutation-tested (MockWebServerDISCONNECT_DURING_RESPONSE_BODY+ awithTimeoutguard; without the fix the coroutine hangs and the test trips the timeout)EditorHTTPClienttest assertinguploadClient()carries the delegate through to uploadsOut of scope
authHeaderpersisted to WebViewlocalStoragepredates this feature (it's already inorigin/trunk); this PR only adds a short-lived loopback token to that already-persisted blob, so it isn't addressed here.