fix: harden the native media upload server (review of #357) - #561
Draft
jkmassel wants to merge 10 commits into
Draft
fix: harden the native media upload server (review of #357)#561jkmassel wants to merge 10 commits into
jkmassel wants to merge 10 commits into
Conversation
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).
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 b7b39a5 |
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.
Adversarial review of the native media upload server introduced in #357, with the findings fixed. Ten 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).
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.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, restoredOut 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.