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/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..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) { @@ -134,6 +143,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) } @@ -145,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() } /** @@ -382,6 +394,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/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) + ) + } +} 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/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 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 } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index a271865ce..74658b676 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) @@ -430,7 +439,30 @@ 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 + // 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. @@ -462,13 +494,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 { @@ -521,6 +557,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/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 2c6ca8d89..915570492 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. /// @@ -576,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/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/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/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) + } +} 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) 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 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" diff --git a/src/utils/api-fetch-upload-middleware.test.js b/src/utils/api-fetch-upload-middleware.test.js index 2bdae9c13..f21011861 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 ) @@ -485,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: