fix: harden the native media upload server (review of #357)

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

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,14 +514,19 @@ 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,
         deadlineNanos: Long,
         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")
             }

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
     }
 }
 

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.
      *

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)
+        )
+    }
+}