From b0739f10a8a05163f1e8a0d5560a5b7c0a8f6316 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 22 Jul 2026 12:59:05 -0600 Subject: [PATCH 01/10] fix: bound upload-server body read by idle timeout, not a total read cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../org/wordpress/gutenberg/HttpServer.kt | 52 ++++- .../wordpress/gutenberg/MediaUploadServer.kt | 11 ++ .../gutenberg/http/HTTPRequestParser.kt | 6 + .../gutenberg/HttpServerTimeoutTests.kt | 167 ++++++++++++++++ .../Sources/Media/MediaUploadServer.swift | 9 + ios/Sources/GutenbergKitHTTP/HTTPServer.swift | 161 +++++++++------ .../GutenbergKitHTTP/HTTPServerError.swift | 4 + .../HTTPServerTimeoutTests.swift | 187 ++++++++++++++++++ 8 files changed, 534 insertions(+), 63 deletions(-) create mode 100644 android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerTimeoutTests.kt create mode 100644 ios/Tests/GutenbergKitHTTPTests/HTTPServerTimeoutTests.swift diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt index 8b610614f..8185d3270 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt @@ -19,10 +19,13 @@ import java.util.Date import java.util.Locale import java.util.TimeZone import java.util.concurrent.Semaphore +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch /** @@ -208,7 +211,14 @@ class HttpServer( private val requiresAuthentication: Boolean = true, private val maxConnections: Int = DEFAULT_MAX_CONNECTIONS, private val maxBodySize: Long = DEFAULT_MAX_BODY_SIZE, + // Bounds the pre-body phase — receiving headers and draining an oversized + // body — i.e. the unauthenticated-reachable portion of the request. private val readTimeoutMs: Int = DEFAULT_READ_TIMEOUT_MS, + // Total-duration backstop for an accepted (authenticated) body, above the + // per-read idle timeout. Defaults to readTimeoutMs; consumers expecting large + // uploads should pass a generous value so a steadily-streamed body isn't + // aborted mid-transfer. + private val bodyReadTimeoutMs: Int = readTimeoutMs, private val idleTimeoutMs: Int = DEFAULT_IDLE_TIMEOUT_MS, private val cacheDir: File? = null, private val cors: CorsPolicy = CorsPolicy.None, @@ -317,6 +327,9 @@ class HttpServer( } catch (_: Exception) { // Best-effort — socket may already be broken. } + } catch (e: CancellationException) { + // Propagate cancellation (e.g. from stop()) — don't swallow it. + throw e } catch (e: Exception) { Log.w(TAG, "Connection error", e) } @@ -329,17 +342,21 @@ class HttpServer( val parser = HTTPRequestParser(maxBodySize = maxBodySize, cacheDir = cacheDir, tempSubdir = tempSubdir) parser.use { val parseStart = System.nanoTime() + // Bounds the pre-body phase (headers + oversized drain) — the + // unauthenticated-reachable portion of the request. The accepted body gets + // its own, more generous deadline below. + // // Note: the deadline is checked between reads, not during a blocking // read. Since each read can block for up to idleTimeoutMs (soTimeout), // the effective maximum time is readTimeoutMs + idleTimeoutMs. This is // a bounded imprecision — slow-loris protection is still effective // because the attacker must send data to keep the connection alive, // and each time data arrives the loop iterates and checks the deadline. - val deadlineNanos = parseStart + readTimeoutMs * 1_000_000L + val headerDeadlineNanos = parseStart + readTimeoutMs * 1_000_000L val buffer = ByteArray(READ_CHUNK_SIZE) // Phase 1: receive headers only. - readUntil(parser, input, buffer, deadlineNanos) { it.hasHeaders } + readUntil(parser, input, buffer, headerDeadlineNanos) { it.hasHeaders } // Validate headers (triggers full RFC validation). val partial = try { @@ -385,11 +402,23 @@ class HttpServer( } } + // Reject auth-exempt OPTIONS that carry a body. Real CORS preflight + // requests are bodyless; a body on the auth-exempt path would otherwise + // be read/drained without authentication — and the accepted-body read + // below is bounded only by the idle timeout. + if (partial.method.uppercase() == "OPTIONS" && (parser.expectedBodyLength ?: 0L) > 0L) { + sendResponse(socket, HttpResponse( + status = 400, + body = "Unexpected request body".toByteArray() + )) + return + } + // Drain the oversized body before responding so the (authenticated) // client receives the 413 instead of a connection reset - // (RFC 9110 §15.5.14). + // (RFC 9110 §15.5.14). Still bounded by the pre-body deadline. if (parser.state == HTTPRequestParser.State.DRAINING) { - readUntil(parser, input, buffer, deadlineNanos) { it.isComplete } + readUntil(parser, input, buffer, headerDeadlineNanos) { it.isComplete } } // If the parser detected a non-fatal error (e.g., payload too @@ -429,8 +458,12 @@ class HttpServer( return } - // Phase 2: receive body (skipped if already complete). - readUntil(parser, input, buffer, deadlineNanos) { it.isComplete } + // Phase 2 (accepted body): now that the client is authenticated, give the + // body its own generous deadline. A large upload that streams steadily is + // bounded by bodyReadTimeoutMs + the per-read idle timeout, not by the + // pre-body readTimeoutMs — so it isn't aborted mid-transfer. + val bodyDeadlineNanos = System.nanoTime() + bodyReadTimeoutMs * 1_000_000L + readUntil(parser, input, buffer, bodyDeadlineNanos) { it.isComplete } // Final parse with body. val parsed = try { @@ -481,7 +514,7 @@ class HttpServer( } /** Reads data into the parser until [condition] is satisfied or the connection closes. */ - private fun readUntil( + private suspend fun readUntil( parser: HTTPRequestParser, input: BufferedInputStream, buffer: ByteArray, @@ -489,6 +522,11 @@ class HttpServer( condition: (HTTPRequestParser.State) -> Boolean ) { while (!condition(parser.state)) { + // Cooperative cancellation: stop() cancels the connection's coroutine + // scope, but a blocking read isn't interruptible — checking between reads + // lets a steadily-streaming connection be torn down promptly on shutdown + // (an idle connection is already bounded by soTimeout). + currentCoroutineContext().ensureActive() if (System.nanoTime() > deadlineNanos) { throw SocketTimeoutException("Read deadline exceeded") } diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt index c289866a8..5b707f46d 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -134,6 +134,7 @@ internal class MediaUploadServer( name = "media-upload", externallyAccessible = false, requiresAuthentication = true, + bodyReadTimeoutMs = UPLOAD_BODY_READ_TIMEOUT_MS, cacheDir = cacheDir, cors = CorsPolicy.Permissive, handler = { request -> handleRequest(request) } @@ -382,6 +383,16 @@ internal class MediaUploadServer( companion object { private const val TAG = "MediaUploadServer" + + /** + * A generous ceiling for receiving the upload body. The body read is + * primarily bounded by the per-read idle timeout (which reaps a stalled + * connection in seconds); this absolute backstop ensures a slow-but-steady + * client can't hold a connection slot indefinitely. Ten minutes is far + * beyond any realistic media upload over loopback while still bounding a + * wedged one. + */ + private const val UPLOAD_BODY_READ_TIMEOUT_MS: Int = 10 * 60 * 1000 } } diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/http/HTTPRequestParser.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/http/HTTPRequestParser.kt index 004ad53aa..e36b1d280 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/http/HTTPRequestParser.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/http/HTTPRequestParser.kt @@ -82,6 +82,12 @@ class HTTPRequestParser( /** The current buffering state. */ val state: State get() = synchronized(lock) { _state } + /** The expected body length from `Content-Length`, available once headers have been received. */ + val expectedBodyLength: Long? + get() = synchronized(lock) { + if (!_state.hasHeaders) null else expectedContentLength + } + /** * The parse error detected during buffering, if any. * diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerTimeoutTests.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerTimeoutTests.kt new file mode 100644 index 000000000..0776d538e --- /dev/null +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerTimeoutTests.kt @@ -0,0 +1,167 @@ +package org.wordpress.gutenberg + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import java.net.Socket + +/** + * Covers the split read-timeout model: the pre-body phase (headers + drain) is + * bounded by `readTimeoutMs`, while an accepted body is bounded by the generous + * `bodyReadTimeoutMs` plus the per-read idle timeout. Also covers rejecting an + * auth-exempt OPTIONS request that carries a body. + */ +class HttpServerTimeoutTests { + + @Test + fun `body that streams steadily past readTimeout still succeeds`() { + // Pre-body cap is short; the body ceiling and idle timeout are generous. + // A body streamed over a span longer than readTimeoutMs (but with no gap + // longer than idleTimeoutMs) must complete — the pre-body cap must not + // bound the accepted body. + val server = HttpServer( + name = "timeout-steady-body", + externallyAccessible = false, + requiresAuthentication = true, + readTimeoutMs = 500, + bodyReadTimeoutMs = 20_000, + idleTimeoutMs = 5_000, + handler = { HttpResponse(body = "OK\n".toByteArray()) } + ) + server.start() + try { + Socket("127.0.0.1", server.port).use { sock -> + sock.soTimeout = 30_000 + val out = sock.getOutputStream() + // Five 4-byte chunks, 200 ms apart → ~1s of body transfer, well past + // the 500 ms pre-body cap, with each gap far under the 5s idle timeout. + val chunks = List(5) { "data".toByteArray() } + val contentLength = chunks.sumOf { it.size } + val header = "POST /test HTTP/1.1\r\nHost: 127.0.0.1\r\n" + + "Proxy-Authorization: Bearer ${server.token}\r\n" + + "Content-Length: $contentLength\r\n\r\n" + out.write(header.toByteArray()) + out.flush() + for (chunk in chunks) { + Thread.sleep(200) + out.write(chunk) + out.flush() + } + val statusLine = sock.getInputStream().bufferedReader().readLine() + assertEquals("HTTP/1.1 200 OK", statusLine) + } + } finally { + server.stop() + } + } + + @Test + fun `body stalled beyond idleTimeout returns 408 even with a generous ceiling`() { + // readTimeoutMs and bodyReadTimeoutMs are long, so only the idle timeout can + // end this connection. A body that stops mid-transfer must still be reaped + // promptly with a 408 — the idle guard is intact. + val server = HttpServer( + name = "timeout-stalled-body", + externallyAccessible = false, + requiresAuthentication = true, + readTimeoutMs = 10_000, + bodyReadTimeoutMs = 10_000, + idleTimeoutMs = 500, + handler = { HttpResponse(body = "OK\n".toByteArray()) } + ) + server.start() + try { + Socket("127.0.0.1", server.port).use { sock -> + sock.soTimeout = 30_000 + val out = sock.getOutputStream() + // Declare 100 bytes but send only 10, then stop. The server waits one + // idle interval for more body bytes, gets none, and returns 408. + val header = "POST /test HTTP/1.1\r\nHost: 127.0.0.1\r\n" + + "Proxy-Authorization: Bearer ${server.token}\r\n" + + "Content-Length: 100\r\n\r\n" + out.write(header.toByteArray()) + out.write(ByteArray(10)) + out.flush() + val statusLine = sock.getInputStream().bufferedReader().readLine() + assertTrue("expected 408, got: $statusLine", statusLine!!.startsWith("HTTP/1.1 408")) + } + } finally { + server.stop() + } + } + + @Test + fun `auth-exempt OPTIONS carrying a body is rejected with 400`() { + val server = HttpServer( + name = "options-with-body", + externallyAccessible = false, + requiresAuthentication = true, + handler = { HttpResponse(body = "OK\n".toByteArray()) } + ) + server.start() + try { + Socket("127.0.0.1", server.port).use { sock -> + sock.soTimeout = 30_000 + // A real CORS preflight is bodyless; an OPTIONS with a body must not + // be read/drained on the auth-exempt path. + val raw = "OPTIONS /test HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: 5\r\n\r\nhello" + sock.getOutputStream().write(raw.toByteArray()) + sock.getOutputStream().flush() + val statusLine = sock.getInputStream().bufferedReader().readLine() + assertTrue("expected 400, got: $statusLine", statusLine!!.startsWith("HTTP/1.1 400")) + } + } finally { + server.stop() + } + } + + @Test + fun `auth-exempt OPTIONS with an oversized body is rejected with 400, not drained`() { + val server = HttpServer( + name = "options-oversized-body", + externallyAccessible = false, + requiresAuthentication = true, + maxBodySize = 16L, + handler = { HttpResponse(body = "OK\n".toByteArray()) } + ) + server.start() + try { + Socket("127.0.0.1", server.port).use { sock -> + sock.soTimeout = 30_000 + // Content-Length exceeds the max body size, so the parser would + // otherwise enter the drain path — the OPTIONS-with-body guard must + // reject it first. + val raw = "OPTIONS /test HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: 1000\r\n\r\n" + sock.getOutputStream().write(raw.toByteArray()) + sock.getOutputStream().flush() + val statusLine = sock.getInputStream().bufferedReader().readLine() + assertTrue("expected 400, got: $statusLine", statusLine!!.startsWith("HTTP/1.1 400")) + } + } finally { + server.stop() + } + } + + @Test + fun `bodyless OPTIONS preflight still succeeds`() { + val server = HttpServer( + name = "options-bodyless", + externallyAccessible = false, + requiresAuthentication = true, + handler = { HttpResponse(body = "OK\n".toByteArray()) } + ) + server.start() + try { + Socket("127.0.0.1", server.port).use { sock -> + sock.soTimeout = 30_000 + val raw = "OPTIONS /test HTTP/1.1\r\nHost: 127.0.0.1\r\n\r\n" + sock.getOutputStream().write(raw.toByteArray()) + sock.getOutputStream().flush() + val statusLine = sock.getInputStream().bufferedReader().readLine() + assertEquals("HTTP/1.1 200 OK", statusLine) + } + } finally { + server.stop() + } + } +} diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index a271865ce..1a9a82f22 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -39,10 +39,19 @@ final class MediaUploadServer: Sendable { let context = UploadContext(uploadDelegate: uploadDelegate, defaultUploader: defaultUploader) + // A generous ceiling for receiving the upload body. The body read is + // primarily bounded by the per-read idle timeout (which reaps a stalled + // connection in seconds); this absolute backstop ensures a slow-but-steady + // client can't hold a connection slot indefinitely. Ten minutes is far + // beyond any realistic media upload over loopback while still bounding a + // wedged one. + let bodyReadTimeout: Duration = .seconds(600) + let server = try await HTTPServer.start( name: "media-upload", requiresAuthentication: true, maxRequestBodySize: maxRequestBodySize, + bodyReadTimeout: bodyReadTimeout, cors: .permissive, handler: { request in await Self.handleRequest(request, context: context) diff --git a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift index 2c6ca8d89..ac0fc7fc8 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift @@ -140,8 +140,15 @@ public final class HTTPServer: Sendable { /// Requests exceeding this limit receive a 413 response. Defaults to 4 GB. /// - maxConnections: The maximum number of concurrent connections. New connections /// beyond this limit are immediately closed. Defaults to 5. - /// - readTimeout: The maximum time to wait for a complete request before closing - /// the connection. Defaults to 30 seconds. + /// - readTimeout: The maximum time to wait for the pre-body phase of a request — + /// receiving the headers and draining any oversized body — before closing the + /// connection. This bounds the unauthenticated-reachable portion of the request. + /// Defaults to 30 seconds. + /// - bodyReadTimeout: The maximum total time to wait for an accepted (authenticated) + /// request body, as a backstop above the per-read `idleTimeout`. A large body that + /// streams steadily is bounded by this ceiling rather than by `readTimeout`, so it + /// is not aborted mid-transfer. Pass `nil` (the default) to reuse `readTimeout`; + /// consumers expecting large uploads should pass a generous value. /// - idleTimeout: The maximum time to wait between consecutive reads before closing /// the connection. Prevents slow-loris attacks. Defaults to 5 seconds. /// - handler: A closure invoked for each fully-parsed request. Return an ``HTTPResponse`` @@ -156,6 +163,7 @@ public final class HTTPServer: Sendable { maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize, maxConnections: Int = HTTPServer.defaultMaxConnections, readTimeout: Duration = HTTPServer.defaultReadTimeout, + bodyReadTimeout: Duration? = nil, idleTimeout: Duration = HTTPServer.defaultIdleTimeout, cors: CORSPolicy = .none, handler: @escaping @Sendable (HTTPServer.Request) async -> HTTPResponse @@ -187,6 +195,9 @@ public final class HTTPServer: Sendable { let queue = DispatchQueue(label: "com.gutenbergkit.http-server.\(safeName)") let requiresAuth = requiresAuthentication + // Falls back to `readTimeout` so consumers that don't distinguish the two + // keep the prior whole-request behavior. + let resolvedBodyReadTimeout = bodyReadTimeout ?? readTimeout listener.newConnectionHandler = { connection in guard connectionCounter.tryIncrement() else { Logger.httpServer.warning("Connection limit reached, rejecting connection") @@ -197,6 +208,7 @@ public final class HTTPServer: Sendable { connection, queue: queue, token: token, requiresAuthentication: requiresAuth, maxRequestBodySize: maxRequestBodySize, readTimeout: readTimeout, + bodyReadTimeout: resolvedBodyReadTimeout, idleTimeout: idleTimeout, cors: cors, tempDirectory: tempDirectory, connectionCounter: connectionCounter, connectionTasks: connectionTasks, handler: handler ) @@ -258,6 +270,7 @@ public final class HTTPServer: Sendable { requiresAuthentication: Bool, maxRequestBodySize: Int64, readTimeout: Duration, + bodyReadTimeout: Duration, idleTimeout: Duration, cors: CORSPolicy, tempDirectory: URL, @@ -277,69 +290,80 @@ public final class HTTPServer: Sendable { let parser = HTTPRequestParser(maxBodySize: maxRequestBodySize, tempDirectory: tempDirectory) var request: ParsedHTTPRequest! let duration = try await ContinuousClock().measure { - request = try await withThrowingTaskGroup(of: ParsedHTTPRequest.self) { group in - group.addTask { - // Phase 1: receive headers only. - try await Self.receiveUntil(\.hasHeaders, parser: parser, on: connection, idleTimeout: idleTimeout) - - // Validate headers (triggers full RFC validation). - guard let partial = try parser.parseRequest() else { - throw HTTPServerError.connectionClosed - } + // Phase 1 (pre-body): receive and validate headers, authenticate, + // and drain any oversized body — all bounded by `readTimeout`. This is + // the unauthenticated-reachable portion of the request, so it keeps a + // strict total-duration cap. + let partial = try await Self.withReadTimeout(readTimeout) { () -> ParsedHTTPRequest in + // Receive headers only. + try await Self.receiveUntil(\.hasHeaders, parser: parser, on: connection, idleTimeout: idleTimeout) + + // Validate headers (triggers full RFC validation). + guard let partial = try parser.parseRequest() else { + throw HTTPServerError.connectionClosed + } - // Check auth on headers alone, before draining or - // consuming any body bytes — an unauthenticated client - // must not be able to make the server read (and - // discard) an arbitrarily large body, and the handler - // must never see an unauthenticated request. - // OPTIONS is exempt because CORS preflight requests - // never include credentials (Fetch spec §3.3.5). - if requiresAuthentication && partial.method.uppercased() != "OPTIONS" { - guard authenticate(partial, token: token) else { - throw HTTPServerError.authenticationFailed - } + // Check auth on headers alone, before draining or consuming any + // body bytes — an unauthenticated client must not be able to make + // the server read (and discard) an arbitrarily large body, and the + // handler must never see an unauthenticated request. OPTIONS is + // exempt because CORS preflight requests never include credentials + // (Fetch spec §3.3.5). + if requiresAuthentication && partial.method.uppercased() != "OPTIONS" { + guard authenticate(partial, token: token) else { + throw HTTPServerError.authenticationFailed } + } - // 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 == .draining { - try await Self.receiveUntil(\.isComplete, parser: parser, on: connection, idleTimeout: idleTimeout) - } + // Reject auth-exempt OPTIONS that carry a body. Real CORS preflight + // requests are bodyless; a body on the auth-exempt path would + // otherwise be read/drained without authentication — and the + // accepted-body read below is bounded only by the idle timeout. + if partial.method.uppercased() == "OPTIONS", (parser.expectedBodyLength ?? 0) > 0 { + throw HTTPServerError.unexpectedBody + } - // If the parser detected a non-fatal error (e.g., - // payload too large after drain), return the partial - // request so the handler can build the response. - if parser.parseError != nil { - return partial - } + // Drain the oversized body before responding so the (authenticated) + // client receives the 413 instead of a connection reset + // (RFC 9110 §15.5.14). Still bounded by `readTimeout`. + if parser.state == .draining { + try await Self.receiveUntil(\.isComplete, parser: parser, on: connection, idleTimeout: idleTimeout) + } - // Reject body-bearing methods without Content-Length. - // We don't support Transfer-Encoding: chunked, so - // Content-Length is the only way to determine body size. - let upperMethod = partial.method.uppercased() - if ["POST", "PUT", "PATCH"].contains(upperMethod) && partial.header("Content-Length") == nil { - throw HTTPServerError.lengthRequired - } + return partial + } - // Phase 2: receive body (skipped if already complete). - if !parser.state.isComplete { - try await Self.receiveUntil(\.isComplete, parser: parser, on: connection, idleTimeout: idleTimeout) - } + // If the parser detected a non-fatal error (e.g., payload too large + // after drain), hand the partial request to the handler so it can + // build the response. + if parser.parseError != nil { + request = partial + return + } - guard let complete = try parser.parseRequest(), complete.isComplete else { - throw HTTPServerError.connectionClosed - } - return complete - } - group.addTask { - try await Task.sleep(for: readTimeout) - throw HTTPServerError.readTimeout + // Reject body-bearing methods without Content-Length. We don't support + // Transfer-Encoding: chunked, so Content-Length is the only way to + // determine body size. + let upperMethod = partial.method.uppercased() + if ["POST", "PUT", "PATCH"].contains(upperMethod) && partial.header("Content-Length") == nil { + throw HTTPServerError.lengthRequired + } + + // Phase 2 (accepted body): the client is authenticated, so read the body + // bounded by `bodyReadTimeout` (a generous backstop) plus the per-read + // `idleTimeout`. A large upload that streams steadily is never failed on + // total duration — only a genuine stall (idle) or the generous ceiling + // ends it. + if !parser.state.isComplete { + try await Self.withReadTimeout(bodyReadTimeout) { + try await Self.receiveUntil(\.isComplete, parser: parser, on: connection, idleTimeout: idleTimeout) } - let result = try await group.next()! - group.cancelAll() - return result } + + guard let complete = try parser.parseRequest(), complete.isComplete else { + throw HTTPServerError.connectionClosed + } + request = complete } // Under a permissive CORS policy the library answers the OPTIONS @@ -358,6 +382,9 @@ public final class HTTPServer: Sendable { await send(HTTPResponse(status: 407, headers: [("Content-Type", "text/plain"), ("Proxy-Authenticate", "Bearer")]), on: connection, cors: cors) } catch HTTPServerError.lengthRequired { await send(HTTPResponse(status: 411, statusText: "Length Required", body: Data("Length Required".utf8)), on: connection, cors: cors) + } catch HTTPServerError.unexpectedBody { + Logger.httpServer.warning("Rejected auth-exempt request carrying a body") + await send(HTTPResponse(status: 400, statusText: "Bad Request", body: Data("Unexpected request body".utf8)), on: connection, cors: cors) } catch is CancellationError { Logger.httpServer.debug("Connection cancelled during shutdown") connection.cancel() @@ -381,6 +408,28 @@ public final class HTTPServer: Sendable { connectionTasks.track(taskID, task) } + /// Runs `operation` under a total-duration timeout, racing it against a sleep + /// task. Used to bound one phase of the request read (pre-body vs. accepted + /// body). The per-read `idleTimeout` inside `operation` still applies + /// independently, and cancellation of the enclosing task cancels both children. + private static func withReadTimeout( + _ timeout: Duration, + _ operation: @escaping @Sendable () async throws -> T + ) async throws -> T { + try await withThrowingTaskGroup(of: T.self) { group in + group.addTask { + try await operation() + } + group.addTask { + try await Task.sleep(for: timeout) + throw HTTPServerError.readTimeout + } + let result = try await group.next()! + group.cancelAll() + return result + } + } + /// Feeds data from the connection into the parser until the given state /// predicate is satisfied or the connection closes. /// diff --git a/ios/Sources/GutenbergKitHTTP/HTTPServerError.swift b/ios/Sources/GutenbergKitHTTP/HTTPServerError.swift index 6c054329d..5b461ec47 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPServerError.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPServerError.swift @@ -15,6 +15,9 @@ public enum HTTPServerError: Error, LocalizedError, Sendable { case authenticationFailed /// The request method requires a Content-Length header but none was provided. case lengthRequired + /// An auth-exempt request (OPTIONS) carried a body. CORS preflights are + /// bodyless, so a body on the auth-exempt path is rejected rather than read. + case unexpectedBody /// A network-level error occurred on the connection. case networkError(NWError) @@ -25,6 +28,7 @@ public enum HTTPServerError: Error, LocalizedError, Sendable { case .readTimeout: "Read timeout expired before request was complete" case .authenticationFailed: "Request failed authentication" case .lengthRequired: "Content-Length header is required for this method" + case .unexpectedBody: "Request method must not carry a body" case .networkError(let error): "Network error: \(error.localizedDescription)" } } diff --git a/ios/Tests/GutenbergKitHTTPTests/HTTPServerTimeoutTests.swift b/ios/Tests/GutenbergKitHTTPTests/HTTPServerTimeoutTests.swift new file mode 100644 index 000000000..b48e58b8f --- /dev/null +++ b/ios/Tests/GutenbergKitHTTPTests/HTTPServerTimeoutTests.swift @@ -0,0 +1,187 @@ +#if canImport(Network) + +import Foundation +import Network +import Testing +@testable import GutenbergKitHTTP + +/// Covers the split read-timeout model: the pre-body phase (headers + drain) is +/// bounded by `readTimeout`, while an accepted body is bounded by the generous +/// `bodyReadTimeout` plus the per-read `idleTimeout`. Also covers rejecting an +/// auth-exempt `OPTIONS` request that carries a body. +@Suite("HTTPServer Timeouts") +struct HTTPServerTimeoutTests { + + @Test("body that streams steadily past readTimeout still succeeds") + func steadyBodyPastReadTimeoutSucceeds() async throws { + // Pre-body cap is short; the body ceiling and idle timeout are generous. + // A body streamed over a span longer than `readTimeout` (but with no gap + // longer than `idleTimeout`) must complete — the pre-body cap must not + // bound the accepted body. + let server = try await HTTPServer.start( + name: "timeout-steady-body", + requiresAuthentication: true, + readTimeout: .milliseconds(500), + bodyReadTimeout: .seconds(20), + idleTimeout: .seconds(5) + ) { _ in + HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + defer { server.stop() } + + // Five 4-byte chunks, 200 ms apart → ~1s of body transfer, well past the + // 500 ms pre-body cap, with each gap far under the 5s idle timeout. + let chunks = Array(repeating: Data("data".utf8), count: 5) + let contentLength = chunks.reduce(0) { $0 + $1.count } + let header = "POST /test HTTP/1.1\r\nHost: 127.0.0.1\r\nProxy-Authorization: Bearer \(server.token)\r\nContent-Length: \(contentLength)\r\n\r\n" + + let response = try await sendChunked(header, chunks: chunks, gap: .milliseconds(200), toPort: server.port) + #expect(response.hasPrefix("HTTP/1.1 200")) + } + + @Test("body stalled beyond idleTimeout is reaped promptly despite a generous ceiling") + func stalledBodyIsReaped() async throws { + // `readTimeout` and `bodyReadTimeout` are long, so only the idle timeout + // can end this connection. A body that stops mid-transfer must still be + // reaped promptly by the idle guard rather than held for the full ceiling. + // + // iOS closes the connection on read timeout rather than delivering a 408 + // (see RFC9110ConformanceTests.serverSends408OnReadTimeout, disabled for + // the same reason: "HTTPServer does not yet send 408 on idle timeout"). + // The property under test is the reaping, observed as a prompt close. + let server = try await HTTPServer.start( + name: "timeout-stalled-body", + requiresAuthentication: true, + readTimeout: .seconds(10), + bodyReadTimeout: .seconds(10), + idleTimeout: .milliseconds(500) + ) { _ in + HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + defer { server.stop() } + + // Declare 100 bytes but send only 10, then stop. The server waits one idle + // interval for more body bytes, gets none, and closes the connection. + let header = "POST /test HTTP/1.1\r\nHost: 127.0.0.1\r\nProxy-Authorization: Bearer \(server.token)\r\nContent-Length: 100\r\n\r\n" + let clock = ContinuousClock() + let start = clock.now + let response = try await sendChunked(header, chunks: [Data(repeating: 0x61, count: 10)], gap: .zero, toPort: server.port) + let elapsed = clock.now - start + + #expect(!response.contains("HTTP/1.1 200")) // the stalled body is not accepted + #expect(elapsed < .seconds(3)) // reaped by the 500ms idle timeout, not the 10s ceiling + } + + @Test("auth-exempt OPTIONS carrying a body is rejected with 400") + func optionsWithBodyReturns400() async throws { + let server = try await HTTPServer.start( + name: "options-with-body", + requiresAuthentication: true + ) { _ in + HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + defer { server.stop() } + + // A real CORS preflight is bodyless; an OPTIONS with a body must not be + // read/drained on the auth-exempt path. + let raw = "OPTIONS /test HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: 5\r\n\r\nhello" + let response = try await sendRaw(raw, toPort: server.port) + #expect(response.hasPrefix("HTTP/1.1 400")) + } + + @Test("auth-exempt OPTIONS with an oversized body is rejected with 400, not drained") + func optionsWithOversizedBodyReturns400() async throws { + let server = try await HTTPServer.start( + name: "options-oversized-body", + requiresAuthentication: true, + maxRequestBodySize: 16 + ) { _ in + HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + defer { server.stop() } + + // Content-Length exceeds the max body size, so the parser would otherwise + // enter the drain path — the OPTIONS-with-body guard must reject it first. + let raw = "OPTIONS /test HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: 1000\r\n\r\n" + let response = try await sendRaw(raw, toPort: server.port) + #expect(response.hasPrefix("HTTP/1.1 400")) + } + + @Test("bodyless OPTIONS preflight still succeeds") + func bodylessOptionsSucceeds() async throws { + let server = try await HTTPServer.start( + name: "options-bodyless", + requiresAuthentication: true + ) { _ in + HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + defer { server.stop() } + + let raw = "OPTIONS /test HTTP/1.1\r\nHost: 127.0.0.1\r\n\r\n" + let response = try await sendRaw(raw, toPort: server.port) + #expect(response.hasPrefix("HTTP/1.1 200")) + } + + // MARK: - Helpers + + /// Sends `header` then each element of `chunks`, pausing `gap` before every + /// chunk, and returns the first response chunk. Used to simulate a body that + /// arrives incrementally over time. + private func sendChunked(_ header: String, chunks: [Data], gap: Duration, toPort port: UInt16) async throws -> String { + let connection = NWConnection( + host: .ipv4(.loopback), + port: NWEndpoint.Port(rawValue: port)!, + using: .tcp + ) + defer { connection.cancel() } + + try await withCheckedThrowingContinuation { (cont: CheckedContinuation) in + connection.stateUpdateHandler = { state in + switch state { + case .ready: + connection.stateUpdateHandler = nil + cont.resume() + case .failed(let error): + connection.stateUpdateHandler = nil + cont.resume(throwing: error) + default: + break + } + } + connection.start(queue: .global()) + } + + try await send(Data(header.utf8), on: connection) + for chunk in chunks { + if gap != .zero { + try await Task.sleep(for: gap) + } + try await send(chunk, on: connection) + } + + return try await withCheckedThrowingContinuation { cont in + connection.receive(minimumIncompleteLength: 1, maximumLength: 8192) { data, _, _, error in + if let error { + cont.resume(throwing: error) + } else { + cont.resume(returning: String(data: data ?? Data(), encoding: .utf8) ?? "") + } + } + } + } + + private func send(_ data: Data, on connection: NWConnection) async throws { + try await withCheckedThrowingContinuation { (cont: CheckedContinuation) in + connection.send(content: data, completion: .contentProcessed { error in + if let error { cont.resume(throwing: error) } else { cont.resume() } + }) + } + } + + /// Sends a raw HTTP request over TCP and returns the response string. + private func sendRaw(_ request: String, toPort port: UInt16) async throws -> String { + try await sendChunked(request, chunks: [], gap: .zero, toPort: port) + } +} + +#endif // canImport(Network) From cdb1683b42bb5d6f23564d4cb796097e3fd41475 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:00:08 -0600 Subject: [PATCH 02/10] fix(ios): trap when mediaUploadDelegate is set too late or released early MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Sources/EditorViewController.swift | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index a200c512e..f8c816c72 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -104,8 +104,40 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// Used by `EditorViewController.warmup()` to reduce first-render latency. private let isWarmupMode: Bool + /// Set once the editor has begun loading and captured its configuration + /// (including ``mediaUploadDelegate``). After this, that delegate can no longer + /// take effect, so its setter traps if written. + private var hasStartedLoading = false + + /// Whether a non-nil ``mediaUploadDelegate`` was ever assigned. Lets the load + /// path tell "the delegate was released before load" (a retention mistake to + /// trap) apart from "no delegate was configured" (a valid opt-out). + private var mediaUploadDelegateWasAssigned = false + /// Delegate for customizing media file processing and upload behavior. - public weak var mediaUploadDelegate: (any MediaUploadDelegate)? + /// + /// Provide this **before the editor loads** — typically right after `init`, the + /// same way the rest of the editor configuration is supplied. It is captured + /// once, when the editor begins loading, and injected into the page's initial + /// configuration; setting it afterward has no effect, so the setter traps. + /// + /// - Important: This is a `weak` reference — you must hold a strong reference to + /// your delegate until the editor has loaded, or native uploads are silently + /// disabled. To surface that mistake, the editor traps at load time if a + /// delegate that was assigned here has already been deallocated. + public weak var mediaUploadDelegate: (any MediaUploadDelegate)? { + didSet { + // Record whether a delegate was provided so the load path can tell a + // premature deallocation apart from a deliberate opt-out (see + // `startUploadServer`). + mediaUploadDelegateWasAssigned = mediaUploadDelegate != nil + precondition( + !hasStartedLoading, + "mediaUploadDelegate must be set before the editor loads (e.g. right after init). " + + "It is captured into the editor configuration at load; setting it afterward has no effect." + ) + } + } // MARK: - Private Properties (Services) private let editorService: EditorService @@ -325,6 +357,10 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// @MainActor private func loadEditor(dependencies: EditorDependencies) async throws { + // From here on the editor configuration — including `mediaUploadDelegate` — + // is captured, so the delegate setter traps if written after this point. + self.hasStartedLoading = true + self.displayActivityView() // Set asset bundle for the URL scheme handler to serve cached plugin/theme assets @@ -388,6 +424,14 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// falls back to Gutenberg's default upload behavior (the JS override won't activate /// because `nativeUploadPort` will be nil in GBKit). private func startUploadServer() async { + // A delegate that was provided but is already nil here was deallocated before + // the editor finished loading — the host didn't hold a strong reference to it. + // That silently disables native uploads, so trap loudly instead. + precondition( + !(mediaUploadDelegateWasAssigned && mediaUploadDelegate == nil), + "mediaUploadDelegate was released before the editor loaded — hold a strong reference to it." + ) + guard mediaUploadDelegate != nil else { return } From 8a08a0843e70cd08135b71d44c70137d8bdba82e Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:54:49 -0600 Subject: [PATCH 03/10] fix(ios): guard temp-file cleanup so concurrent servers don't wipe live buffers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../GutenbergKitHTTP/HTTPRequestParser.swift | 6 +++ ios/Sources/GutenbergKitHTTP/HTTPServer.swift | 23 ++++---- .../GutenbergKitHTTP/RequestBody.swift | 35 ++++++++++-- .../TempFileCleanupTests.swift | 53 +++++++++++++++++++ 4 files changed, 100 insertions(+), 17 deletions(-) create mode 100644 ios/Tests/GutenbergKitHTTPTests/TempFileCleanupTests.swift diff --git a/ios/Sources/GutenbergKitHTTP/HTTPRequestParser.swift b/ios/Sources/GutenbergKitHTTP/HTTPRequestParser.swift index ddfc46c2c..f57b24529 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPRequestParser.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPRequestParser.swift @@ -368,6 +368,10 @@ private final class Buffer { let dir = directory ?? FileManager.default.temporaryDirectory let url = dir.appendingPathComponent("GutenbergKitHTTP-\(UUID().uuidString)") + // Mark the file active before creating it, so a concurrent server's orphan + // sweep can't delete it in the window between creation and first use. + ActiveTempFiles.register(url.lastPathComponent) + if FileManager.default.createFile(atPath: url.path, contents: nil), let handle = FileHandle(forUpdatingAtPath: url.path) { self.fileURL = url @@ -375,6 +379,7 @@ private final class Buffer { self.memoryBuffer = nil } else { // Temp file unavailable — buffer in memory instead. + ActiveTempFiles.unregister(url.lastPathComponent) self.fileURL = nil self.fileHandle = nil self.memoryBuffer = Data() @@ -388,6 +393,7 @@ private final class Buffer { try? fileHandle.close() } if let fileURL, !fileOwnershipTransferred { + ActiveTempFiles.unregister(fileURL.lastPathComponent) try? FileManager.default.removeItem(at: fileURL) } } diff --git a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift index ac0fc7fc8..915570492 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift @@ -625,25 +625,20 @@ public final class HTTPServer: Sendable { /// /// The parser creates temp files in a server-specific subdirectory under the /// system temp directory (e.g., `GutenbergKitHTTP-media-proxy/`). Under normal - /// operation, `TempFileOwner.deinit` deletes them via ARC. After a crash, these + /// operation, `Buffer`/`TempFileOwner` delete them via ARC. After a crash these /// files survive — this method cleans them up on the next server start. /// - /// Because each server `name` maps to its own subdirectory, cleanup is scoped - /// to a single server instance and will not affect files belonging to other - /// servers running concurrently. - /// - /// **Important:** Two server instances with the same `name` must not run - /// concurrently. On startup, this method deletes **all** files in the - /// server's temp subdirectory. If another instance with the same name is - /// still handling requests, its in-flight temp files will be removed, - /// causing `bufferIOError` failures. Callers must ensure each running - /// server uses a unique name, or that the previous instance is fully - /// stopped before starting a new one. - private static func cleanOrphanedTempFiles(in directory: URL) { + /// Files currently backing an in-flight request are registered in + /// ``ActiveTempFiles`` and skipped, so a server instance that shares a + /// directory with a concurrently-running instance of the same name (e.g. two + /// editors open at once, or one being torn down as another starts) does not + /// delete the other's live buffers. Files not in the registry have no live + /// owner in this process — they are crash orphans and are removed. + static func cleanOrphanedTempFiles(in directory: URL) { guard let contents = try? FileManager.default.contentsOfDirectory( at: directory, includingPropertiesForKeys: nil ) else { return } - for url in contents { + for url in contents where !ActiveTempFiles.contains(url.lastPathComponent) { try? FileManager.default.removeItem(at: url) } } diff --git a/ios/Sources/GutenbergKitHTTP/RequestBody.swift b/ios/Sources/GutenbergKitHTTP/RequestBody.swift index 80d7a2d72..dc22f9ef1 100644 --- a/ios/Sources/GutenbergKitHTTP/RequestBody.swift +++ b/ios/Sources/GutenbergKitHTTP/RequestBody.swift @@ -1,15 +1,44 @@ import Foundation +/// Process-wide registry of temp files currently backing an in-flight request. +/// +/// ``HTTPServer/cleanOrphanedTempFiles(in:)`` runs a delete-all sweep of a +/// server's temp directory on start to reclaim files orphaned by a crash. Two +/// server instances that share a name share that directory (e.g. two editors +/// open at once, or one being torn down as another starts), so the sweep would +/// otherwise delete the other instance's live buffers. Registering a file here +/// while it is in use makes the sweep skip it; files not registered are crash +/// orphans (no live owner in this process) and are removed. +/// +/// Keyed by file name (a unique UUID), which is stable however the directory is +/// later enumerated. +enum ActiveTempFiles { + private static let lock = NSLock() + // Guarded by `lock` on every access. + nonisolated(unsafe) private static var names = Set() + + static func register(_ name: String) { lock.withLock { _ = names.insert(name) } } + static func unregister(_ name: String) { lock.withLock { _ = names.remove(name) } } + static func contains(_ name: String) -> Bool { lock.withLock { names.contains(name) } } +} + /// Reference-counted owner for a temporary file. /// /// The file is deleted when the last reference is released. This allows /// ``RequestBody`` (a value type) to share ownership of a temp file across /// copies — including multipart part bodies that reference byte ranges within -/// the same file. +/// the same file. While owned, the file is registered in ``ActiveTempFiles`` so +/// a concurrent server's orphan sweep won't delete it. final class TempFileOwner: Sendable { let url: URL - init(url: URL) { self.url = url } - deinit { try? FileManager.default.removeItem(at: url) } + init(url: URL) { + self.url = url + ActiveTempFiles.register(url.lastPathComponent) + } + deinit { + ActiveTempFiles.unregister(url.lastPathComponent) + try? FileManager.default.removeItem(at: url) + } } /// An HTTP request body with stream semantics. diff --git a/ios/Tests/GutenbergKitHTTPTests/TempFileCleanupTests.swift b/ios/Tests/GutenbergKitHTTPTests/TempFileCleanupTests.swift new file mode 100644 index 000000000..0ba71fafe --- /dev/null +++ b/ios/Tests/GutenbergKitHTTPTests/TempFileCleanupTests.swift @@ -0,0 +1,53 @@ +#if canImport(Network) + +import Foundation +import Testing +@testable import GutenbergKitHTTP + +@Suite("Temp File Cleanup") +struct TempFileCleanupTests { + + @Test("orphan cleanup skips registered (in-flight) files and removes orphans") + func cleanupSkipsActiveFiles() throws { + let dir = FileManager.default.temporaryDirectory + .appendingPathComponent("GutenbergKitHTTP-cleanup-test-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + + let active = dir.appendingPathComponent("GutenbergKitHTTP-\(UUID().uuidString)") + let orphan = dir.appendingPathComponent("GutenbergKitHTTP-\(UUID().uuidString)") + #expect(FileManager.default.createFile(atPath: active.path, contents: Data("a".utf8))) + #expect(FileManager.default.createFile(atPath: orphan.path, contents: Data("b".utf8))) + + // Mark `active` as backing an in-flight request, as a concurrently-running + // server instance sharing this directory would. + ActiveTempFiles.register(active.lastPathComponent) + defer { ActiveTempFiles.unregister(active.lastPathComponent) } + + HTTPServer.cleanOrphanedTempFiles(in: dir) + + #expect(FileManager.default.fileExists(atPath: active.path), "registered (live) file must be preserved") + #expect(!FileManager.default.fileExists(atPath: orphan.path), "unregistered orphan must be removed") + } + + @Test("cleanup removes everything when nothing is registered (crash recovery)") + func cleanupRemovesAllOrphansOnFreshProcess() throws { + let dir = FileManager.default.temporaryDirectory + .appendingPathComponent("GutenbergKitHTTP-cleanup-test-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + + let orphans = (0..<3).map { _ in dir.appendingPathComponent("GutenbergKitHTTP-\(UUID().uuidString)") } + for url in orphans { + #expect(FileManager.default.createFile(atPath: url.path, contents: Data("x".utf8))) + } + + HTTPServer.cleanOrphanedTempFiles(in: dir) + + for url in orphans { + #expect(!FileManager.default.fileExists(atPath: url.path)) + } + } +} + +#endif From 63d046a1fc5a3f2d8f44ab5f0e207e10afe098a2 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:54:57 -0600 Subject: [PATCH 04/10] fix(ios): tear down the upload body writer thread on every exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Sources/Media/MediaUploadServer.swift | 10 ++++ .../BoundStreamTeardownTests.swift | 51 +++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 ios/Tests/GutenbergKitHTTPTests/BoundStreamTeardownTests.swift diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 1a9a82f22..a6aa4ec4e 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -440,6 +440,16 @@ class DefaultMediaUploader: @unchecked Sendable { } private func performUpload(_ request: URLRequest) async throws -> MediaUploadResponse { + // The body may be fed by a background writer thread via a bound stream pair + // (multipartBodyStream, or RequestBody.makeInputStream for file slices). If + // the request is cancelled or fails, URLSession may abandon the stream + // without draining it, leaving that writer blocked forever on a full buffer + // — leaking the thread and its open file handle. Closing the input stream on + // every exit breaks the pair so the writer's write() fails and it unwinds. + // (For in-memory/whole-file bodies there is no writer thread and this is a + // harmless no-op.) + defer { request.httpBodyStream?.close() } + // Relay WordPress's response verbatim — including non-2xx statuses — so // the editor sees WordPress's real status and error body, exactly as a // direct upload would. `performRaw` does not throw on non-2xx. diff --git a/ios/Tests/GutenbergKitHTTPTests/BoundStreamTeardownTests.swift b/ios/Tests/GutenbergKitHTTPTests/BoundStreamTeardownTests.swift new file mode 100644 index 000000000..9450722e3 --- /dev/null +++ b/ios/Tests/GutenbergKitHTTPTests/BoundStreamTeardownTests.swift @@ -0,0 +1,51 @@ +import Foundation +import Testing + +/// Verifies the assumption behind `MediaUploadServer.performUpload`'s +/// `defer { request.httpBodyStream?.close() }`: closing the input side of a bound +/// stream pair unblocks a writer that is blocked on a full output buffer. Without +/// that, a consumer (URLSession) that abandons the body stream on cancel/failure +/// without draining it would leave the background writer thread blocked forever, +/// leaking the thread and its open file handle. +@Suite("Bound Stream Teardown") +struct BoundStreamTeardownTests { + + @Test("closing the input stream unblocks a blocked bound-pair writer") + func closingInputUnblocksBlockedWriter() throws { + var readStream: InputStream? + var writeStream: OutputStream? + Stream.getBoundStreams(withBufferSize: 1024, inputStream: &readStream, outputStream: &writeStream) + let input = try #require(readStream) + let output = try #require(writeStream) + input.open() + output.open() + + let exited = DispatchSemaphore(value: 0) + // OutputStream is not Sendable; only the writer thread touches it after this. + nonisolated(unsafe) let out = output + Thread.detachNewThread { + // Write far more than the 1 KB buffer with nobody reading the input — + // once the buffer fills, `write` blocks (backpressure). + let chunk = [UInt8](repeating: 0, count: 256 * 1024) + chunk.withUnsafeBufferPointer { buffer in + guard let base = buffer.baseAddress else { return } + var written = 0 + while written < chunk.count { + let result = out.write(base + written, maxLength: chunk.count - written) + if result <= 0 { break } + written += result + } + } + out.close() + exited.signal() + } + + // The writer should be blocked on the full buffer (nothing is reading). + #expect(exited.wait(timeout: .now() + .milliseconds(300)) == .timedOut) + + // Closing the input breaks the pair; the blocked `write` should fail and the + // writer thread should unwind and exit. + input.close() + #expect(exited.wait(timeout: .now() + .seconds(3)) == .success) + } +} From 4aa967cab309d29d5d50d3d3fd79523020ff56fe Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:05:19 -0600 Subject: [PATCH 05/10] test(js): make the upload-cancel tests exercise the abort-vs-network race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/utils/api-fetch-upload-middleware.test.js | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/utils/api-fetch-upload-middleware.test.js b/src/utils/api-fetch-upload-middleware.test.js index 2bdae9c13..2d2d7399e 100644 --- a/src/utils/api-fetch-upload-middleware.test.js +++ b/src/utils/api-fetch-upload-middleware.test.js @@ -438,16 +438,19 @@ describe( 'nativeMediaUploadMiddleware', () => { } ); const next = makeNext(); - // A real aborted signal — `fetch` rejects with the signal's reason. + // The race the middleware guards against: the signal is aborted, but + // `fetch` rejects with a *distinct* network error (a TypeError can win the + // race with the abort). The middleware must rethrow the signal's canonical + // reason, NOT the fetch rejection — otherwise a cancelled upload surfaces a + // spurious transport-failure notice. const controller = new AbortController(); controller.abort(); const options = { ...makePostMediaOptions( makeFile() ), signal: controller.signal, }; - global.fetch = vi.fn( () => - Promise.reject( controller.signal.reason ) - ); + const networkError = new TypeError( 'Failed to fetch' ); + global.fetch = vi.fn( () => Promise.reject( networkError ) ); // The middleware rethrows the signal's canonical reason (not the fetch // rejection) and does not retry. @@ -466,16 +469,19 @@ describe( 'nativeMediaUploadMiddleware', () => { } ); const next = makeNext(); - // `AbortSignal.timeout()` aborts its signal and rejects with a - // TimeoutError (not an AbortError). A `name === 'AbortError'` check would - // miss it and wrongly fall back; keying off `signal.aborted` catches it. + // `AbortSignal.timeout()` aborts its signal and rejects with a TimeoutError + // (not an AbortError). A `name === 'AbortError'` check would miss it and + // wrongly fall back; keying off `signal.aborted` catches it. As with a + // plain abort, `fetch` may reject with a distinct network error that races + // the timeout, so the middleware must still rethrow the signal's reason. const timeoutError = new Error( 'The operation timed out.' ); timeoutError.name = 'TimeoutError'; const options = { ...makePostMediaOptions( makeFile() ), signal: { aborted: true, reason: timeoutError }, }; - global.fetch = vi.fn( () => Promise.reject( timeoutError ) ); + const networkError = new TypeError( 'Failed to fetch' ); + global.fetch = vi.fn( () => Promise.reject( networkError ) ); await expect( nativeMediaUploadMiddleware( options, next ) From d7cddecb0447bc1a488f8fc01466351d4594374c Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:19:55 -0600 Subject: [PATCH 06/10] fix(android): don't resurrect the upload server on a detached view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../org/wordpress/gutenberg/GutenbergView.kt | 19 ++++- .../GutenbergViewUploadServerTest.kt | 85 +++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt index 47b1fe28a..eb7b80587 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -134,7 +134,16 @@ class GutenbergView : FrameLayout { syncUploadServerJavaScriptVariables() } - private var uploadServer: MediaUploadServer? = null + @Volatile private var uploadServer: MediaUploadServer? = null + + /** + * True once the view has been detached from its window. [onDetachedFromWindow] + * stops the upload server and won't fire again, so [startUploadServer] must not + * resurrect one (e.g. from a delegate set after detach) — that would leak its + * socket and accept-loop coroutine. Reset on re-attach. + */ + @Volatile private var isTornDown = false + private val uploadHttpClient: okhttp3.OkHttpClient by lazy { // The read/write inactivity timeouts mirror URLSession's 60s // timeoutIntervalForRequest default — an inactivity timer that resets on @@ -678,6 +687,12 @@ class GutenbergView : FrameLayout { } private fun startUploadServer() { + // Don't (re)start on a detached view: onDetachedFromWindow has already torn + // the server down and won't fire again, so a server started here (e.g. from a + // delegate assigned after detach) would leak its socket and accept-loop + // coroutine with nothing left to stop it. + if (isTornDown) return + // The native upload server relays through DefaultMediaUploader, which needs a // site root and an auth header (every host provides one — the editor injects // it because the WebView has no auth cookies). Without both there is nothing @@ -1143,11 +1158,13 @@ class GutenbergView : FrameLayout { override fun onAttachedToWindow() { super.onAttachedToWindow() + isTornDown = false startNetworkMonitoring() } override fun onDetachedFromWindow() { super.onDetachedFromWindow() + isTornDown = true stopNetworkMonitoring() uploadServer?.stop() uploadServer = null diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt new file mode 100644 index 000000000..550e48997 --- /dev/null +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt @@ -0,0 +1,85 @@ +package org.wordpress.gutenberg + +import android.os.Looper +import android.view.View +import kotlinx.coroutines.test.TestScope +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mockito.mock +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config +import org.wordpress.gutenberg.model.EditorConfiguration +import org.wordpress.gutenberg.model.EditorDependencies + +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [28], manifest = Config.NONE) +class GutenbergViewUploadServerTest { + + private val testScope = TestScope() + + private fun makeView(): GutenbergView { + val config = EditorConfiguration + .builder("https://example.com", "https://example.com/wp-json/") + .setAuthHeader("Bearer test") + .build() + return GutenbergView( + config, + EditorDependencies.empty, + testScope, + RuntimeEnvironment.getApplication() + ) + } + + private fun uploadServerOf(view: GutenbergView): Any? { + val field = GutenbergView::class.java.getDeclaredField("uploadServer") + field.isAccessible = true + return field.get(view) + } + + /** Invokes the protected `onDetachedFromWindow` lifecycle callback. */ + private fun detach(view: GutenbergView) { + val method = View::class.java.getDeclaredMethod("onDetachedFromWindow") + method.isAccessible = true + method.invoke(view) + } + + private fun idle() = shadowOf(Looper.getMainLooper()).idle() + + @Test + fun `setting the delegate on a live view starts the upload server (baseline)`() { + val view = makeView() + try { + view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + idle() + assertNotNull( + "a live view with a valid config should start the upload server", + uploadServerOf(view) + ) + } finally { + // Release the bound socket. + view.mediaUploadDelegate = null + } + } + + @Test + fun `setting the delegate after detach does not start a leaked server`() { + val view = makeView() + + // onDetachedFromWindow stops any server and won't fire again. + detach(view) + + // A delegate assigned after detach must not resurrect a server that nothing + // would ever stop (bound socket + accept-loop coroutine). + view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + idle() + + assertNull( + "no upload server should be started once the view is detached", + uploadServerOf(view) + ) + } +} From 70497ea3899241a63fa4aa627c96bcd4907c81ba Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:24:34 -0600 Subject: [PATCH 07/10] 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. --- .../wordpress/gutenberg/MediaUploadServer.kt | 15 ++++++-- .../gutenberg/MediaUploadServerTest.kt | 36 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt index 5b707f46d..0216fdc65 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -5,6 +5,7 @@ import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job +import kotlinx.coroutines.cancel import kotlinx.coroutines.launch import org.wordpress.gutenberg.http.HeaderValue import org.wordpress.gutenberg.http.MultipartPart @@ -97,7 +98,7 @@ internal class MediaUploadServer( private val uploadDelegate: MediaUploadDelegate?, private val defaultUploader: DefaultMediaUploader?, cacheDir: File? = null, - scope: CoroutineScope = CoroutineScope(Dispatchers.IO), + scope: CoroutineScope? = null, ioDispatcher: CoroutineDispatcher = Dispatchers.IO ) { /** The port the server is listening on. */ @@ -115,13 +116,21 @@ internal class MediaUploadServer( private val uploadsTempDir: File = File(cacheDir ?: File(System.getProperty("java.io.tmpdir")), "gutenbergkit-uploads") + /** + * The scope MediaUploadServer created itself because the caller supplied none. + * It is cancelled in [stop]; a caller-supplied scope is left to the caller's + * lifecycle (cancelling it here would tear down state the caller still owns). + */ + private val ownedScope: CoroutineScope? = + if (scope == null) CoroutineScope(Dispatchers.IO) else null + /** * Sweeps crash-orphaned temp files off the caller's thread. Exposed so tests * can await it; injecting `Dispatchers.Unconfined` for [ioDispatcher] runs the * sweep synchronously. */ @Suppress("TooGenericExceptionCaught") - val cleanupJob: Job = scope.launch(ioDispatcher) { + val cleanupJob: Job = (scope ?: ownedScope!!).launch(ioDispatcher) { try { cleanOrphanedUploads() } catch (e: Exception) { @@ -146,6 +155,8 @@ internal class MediaUploadServer( fun stop() { cleanupJob.cancel() server.stop() + // Cancel the scope only if we created it; a caller-supplied scope is theirs. + ownedScope?.cancel() } /** diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt index 7ce925f0f..6b4098de3 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -1,13 +1,18 @@ package org.wordpress.gutenberg import com.google.gson.JsonParser +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.cancel +import kotlinx.coroutines.isActive import kotlinx.coroutines.runBlocking import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Rule @@ -41,6 +46,37 @@ class MediaUploadServerTest { assertTrue(server.token.isNotEmpty()) } + @Test + fun `stop cancels an internally-created scope but leaves a caller-supplied one alone`() { + // No scope supplied → the server owns one, which stop() must cancel. + val owningServer = + MediaUploadServer(uploadDelegate = null, defaultUploader = null, cacheDir = tempFolder.root) + val ownedScope = ownedScopeOf(owningServer) + assertNotNull("server should own a scope when none is supplied", ownedScope) + assertTrue(ownedScope!!.isActive) + owningServer.stop() + assertFalse("stop() must cancel the scope it created", ownedScope.isActive) + + // A caller-supplied scope belongs to the caller — stop() must not cancel it. + val callerScope = CoroutineScope(Dispatchers.IO) + val borrowingServer = MediaUploadServer( + uploadDelegate = null, + defaultUploader = null, + cacheDir = tempFolder.root, + scope = callerScope + ) + assertNull("server must not own a caller-supplied scope", ownedScopeOf(borrowingServer)) + borrowingServer.stop() + assertTrue("stop() must not cancel a caller-supplied scope", callerScope.isActive) + callerScope.cancel() + } + + private fun ownedScopeOf(uploadServer: MediaUploadServer): CoroutineScope? { + val field = MediaUploadServer::class.java.getDeclaredField("ownedScope") + field.isAccessible = true + return field.get(uploadServer) as CoroutineScope? + } + // MARK: - Auth validation @Test From ebc9e802bdda33d7d840f1eece3258714d1b82c0 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:31:40 -0600 Subject: [PATCH 08/10] 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. --- .../Sources/Media/MediaUploadServer.swift | 21 ++++++++++++++--- .../Media/MediaUploadServerTests.swift | 23 +++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index a6aa4ec4e..f0465d050 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -481,13 +481,17 @@ class DefaultMediaUploader: @unchecked Sendable { var preamble = Data() for field in extraFields { preamble.append(Data("--\(boundary)\r\n".utf8)) - preamble.append(Data("Content-Disposition: form-data; name=\"\(field.name)\"\r\n\r\n".utf8)) + preamble.append(Data("Content-Disposition: form-data; name=\"\(escapeQuotedParameter(field.name))\"\r\n\r\n".utf8)) preamble.append(field.value) preamble.append(Data("\r\n".utf8)) } preamble.append(Data("--\(boundary)\r\n".utf8)) - preamble.append(Data("Content-Disposition: form-data; name=\"file\"; filename=\"\(filename)\"\r\n".utf8)) - preamble.append(Data("Content-Type: \(mimeType)\r\n\r\n".utf8)) + preamble.append(Data("Content-Disposition: form-data; name=\"file\"; filename=\"\(escapeQuotedParameter(filename))\"\r\n".utf8)) + // `mimeType` is a client-supplied Content-Type value; strip CR/LF so a + // crafted value can't inject additional headers. (Quotes are legal in + // Content-Type parameters, so they're left intact.) + let safeMimeType = mimeType.replacingOccurrences(of: "\r", with: "").replacingOccurrences(of: "\n", with: "") + preamble.append(Data("Content-Type: \(safeMimeType)\r\n\r\n".utf8)) let epilogue = Data("\r\n--\(boundary)--\r\n".utf8) guard let fileSize = try FileManager.default.attributesOfItem(atPath: fileURL.path(percentEncoded: false))[.size] as? Int else { @@ -540,6 +544,17 @@ class DefaultMediaUploader: @unchecked Sendable { return (inputStream, contentLength) } + /// Escapes a client-supplied value for a quoted `Content-Disposition` + /// parameter (`name`/`filename`). Percent-encodes CR, LF, and `"` so a crafted + /// filename or field name can't break the header line or inject an extra + /// multipart part — matching WHATWG's `multipart/form-data` field serialization. + private static func escapeQuotedParameter(_ value: String) -> String { + value + .replacingOccurrences(of: "\r", with: "%0D") + .replacingOccurrences(of: "\n", with: "%0A") + .replacingOccurrences(of: "\"", with: "%22") + } + /// Writes all bytes of `data` to the output stream, handling partial writes. private static func writeAll(_ data: Data, to output: OutputStream) -> Bool { data.withUnsafeBytes { buffer in diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index be3c6cbf3..ca1351704 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -393,6 +393,29 @@ struct MultipartBodyStreamTests { #expect(result == expected) } + @Test("escapes CR/LF and quotes so a crafted filename can't inject headers or parts") + func escapesHeaderInjection() throws { + let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent("stream-test-\(UUID().uuidString)") + try Data("file-bytes".utf8).write(to: tempFile) + defer { try? FileManager.default.removeItem(at: tempFile) } + + // Craft a filename, field name, and MIME type that each try to smuggle a CRLF + // and a fake header into the body relayed to WordPress. + let (stream, _) = try DefaultMediaUploader.multipartBodyStream( + fileURL: tempFile, + boundary: "boundary", + filename: "evil\"\r\nX-Injected-File: 1.jpg", + mimeType: "image/jpeg\r\nX-Injected-Type: 1", + extraFields: [("field\"\r\nX-Injected-Name: 1", Data("v".utf8))] + ) + let text = String(decoding: readAllFromStream(stream), as: UTF8.self) + + // None of the crafted CRLF sequences may survive as a real header break. + #expect(!text.contains("\r\nX-Injected-File:")) + #expect(!text.contains("\r\nX-Injected-Type:")) + #expect(!text.contains("\r\nX-Injected-Name:")) + } + @Test("includes non-file parts (e.g. post) ahead of the file") func multipartBodyIncludesExtraParts() throws { let boundary = "boundary" From 55867b64063b02d80053c250f1b0305a861036d4 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:38:12 -0600 Subject: [PATCH 09/10] docs(ios): document the upload body's non-replayable-stream limitation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Sources/Media/MediaUploadServer.swift | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index f0465d050..74658b676 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -439,6 +439,19 @@ class DefaultMediaUploader: @unchecked Sendable { return try await performUpload(request) } + /// Sends the assembled upload request to WordPress and relays the response. + /// + /// The request body is a **one-shot** stream (a bound-pair pipe for the + /// multipart re-encode and file-slice paths), so it can't be replayed. That + /// only matters if URLSession has to resend the body — i.e. a `307`/`308` + /// redirect that preserves the `POST`. `301`/`302`/`303` downgrade to a + /// bodyless GET, and a Bearer-token `401` doesn't trigger a resend, so those + /// never replay the stream. WordPress core never redirects `POST /wp/v2/media`; + /// if a proxy or misconfiguration did, the resend would read the now-exhausted + /// stream and send an empty body, which WordPress rejects — a clean failure, + /// not a truncated attachment (the stream is consumed, never rewound). We + /// intentionally don't implement `needNewBodyStream`, or buffer the body to a + /// replayable file, for that rare case. private func performUpload(_ request: URLRequest) async throws -> MediaUploadResponse { // The body may be fed by a background writer thread via a bound stream pair // (multipartBodyStream, or RequestBody.makeInputStream for file slices). If From b7b39a5e8659b4ae774b5e9e2f606686550b3ffd Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:46:32 -0600 Subject: [PATCH 10/10] fix(js): reject with a canonical AbortError when an aborted signal has no reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- src/utils/api-fetch-upload-middleware.test.js | 26 +++++++++++++++++++ src/utils/api-fetch.js | 8 +++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/utils/api-fetch-upload-middleware.test.js b/src/utils/api-fetch-upload-middleware.test.js index 2d2d7399e..f21011861 100644 --- a/src/utils/api-fetch-upload-middleware.test.js +++ b/src/utils/api-fetch-upload-middleware.test.js @@ -491,6 +491,32 @@ describe( 'nativeMediaUploadMiddleware', () => { expect( next ).not.toHaveBeenCalled(); } ); + it( 'throws a canonical AbortError when an aborted signal has no reason', async () => { + getGBKit.mockReturnValue( { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + } ); + const next = makeNext(); + + // Some engines mark the signal aborted without populating `reason`. The + // middleware must still reject with a real AbortError, not a thrown + // `undefined` that upstream would surface as a spurious failure. + const options = { + ...makePostMediaOptions( makeFile() ), + signal: { aborted: true, reason: undefined }, + }; + global.fetch = vi.fn( () => + Promise.reject( new TypeError( 'Failed to fetch' ) ) + ); + + const error = await nativeMediaUploadMiddleware( options, next ).catch( + ( thrown ) => thrown + ); + expect( error ).not.toBeUndefined(); + expect( error?.name ).toBe( 'AbortError' ); + expect( next ).not.toHaveBeenCalled(); + } ); + // MARK: - Signal forwarding it( 'forwards abort signal to fetch', async () => { diff --git a/src/utils/api-fetch.js b/src/utils/api-fetch.js index 3a5e4d079..754a7d54c 100644 --- a/src/utils/api-fetch.js +++ b/src/utils/api-fetch.js @@ -258,7 +258,13 @@ export function nativeMediaUploadMiddleware( options, next ) { // make upstream treat a cancelled upload as a real failure — surfacing // a spurious error notice instead of a silent cancel. if ( options.signal?.aborted ) { - throw options.signal.reason; + // Some engines abort without populating `reason`; fall back to a + // canonical AbortError so upstream recognizes the cancellation + // rather than a thrown `undefined`. + throw ( + options.signal.reason ?? + new DOMException( 'The upload was aborted.', 'AbortError' ) + ); } // Otherwise the loopback upload server is unreachable at the transport // layer. We deliberately do NOT fall back to a direct re-upload: