fix: native media upload review follow-ups (relay, delegate contract, CORS)

android/Gutenberg/src/androidTest/java/org/wordpress/gutenberg/http/InstrumentedFixtureTests.kt

@@ -160,6 +160,11 @@ class InstrumentedFixtureTests {
                         pendingError.errorId,
                         "$description: expected $expectedError but got ${pendingError.errorId}"
                     )
+                    assertEquals(
+                        HTTPRequestParseError.Disposition.RECOVERABLE,
+                        pendingError.disposition,
+                        "$description: ${pendingError.errorId} surfaced via pendingParseError but is not RECOVERABLE"
+                    )
                 } else {
                     fail("$description: expected error $expectedError but parsing succeeded")
                 }
@@ -169,6 +174,11 @@ class InstrumentedFixtureTests {
                     e.error.errorId,
                     "$description: expected $expectedError but got ${e.error.errorId}"
                 )
+                assertEquals(
+                    HTTPRequestParseError.Disposition.FATAL,
+                    e.error.disposition,
+                    "$description: ${e.error.errorId} was thrown but is not FATAL"
+                )
             }
         }
     }

android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt

@@ -12,6 +12,7 @@ import android.net.Uri
 import android.os.Bundle
 import android.os.Handler
 import android.os.Looper
+import android.security.NetworkSecurityPolicy
 import android.util.Log
 import android.view.Gravity
 import android.view.inputmethod.InputMethodManager
@@ -119,16 +120,49 @@ class GutenbergView : FrameLayout {
             // Stop any previously running server before starting a new one.
             uploadServer?.stop()
             uploadServer = null
-            // (Re)start the upload server so it captures the delegate.
-            // This handles the common case where the delegate is set after
+            // (Re)start the upload server so it captures the delegate. This
+            // handles the common case where the delegate is set after
             // construction but before the editor finishes loading.
             if (value != null) {
                 startUploadServer()
             }
+            // Reflect the resulting server state in the page: advertise a freshly
+            // (re)started server, or clear the port when the server did NOT
+            // (re)start (delegate cleared, auth missing, or cleartext-to-localhost
+            // blocked) so the JS upload middleware routes to the default path
+            // instead of fetching a now-dead port.
+            syncUploadServerJavaScriptVariables()
         }
 
     private var uploadServer: MediaUploadServer? = null
-    private val uploadHttpClient: okhttp3.OkHttpClient by lazy { okhttp3.OkHttpClient() }
+    private val uploadHttpClient: okhttp3.OkHttpClient by lazy {
+        // The read/write inactivity timeouts mirror URLSession's 60s
+        // timeoutIntervalForRequest default — an inactivity timer that resets on
+        // progress — so the iOS and Android upload clients behave the same. Like
+        // URLSession, which governs uploads with that inactivity timer rather than a
+        // total-duration cap, there is deliberately NO callTimeout here (unlike the
+        // sibling EditorHTTPClient, which caps total call duration — fine for small
+        // REST payloads, wrong for uploads): because each timeout resets on progress,
+        // a large upload is never failed on total duration, only on a genuine 60s
+        // stall in the active direction.
+        //
+        //  - writeTimeout (upload): a transient loss of connectivity during the upload
+        //    fails within ~60s so it surfaces and can be retried, rather than hanging.
+        //  - readTimeout (download): gives WordPress time to generate image sub-sizes
+        //    synchronously inside POST /wp/v2/media, during which it sends no response
+        //    bytes. The bare OkHttpClient() 10s default fired mid-resize — and since the
+        //    attachment row exists before resizing finishes, that orphaned it
+        //    server-side and duplicated it on retry.
+        //  - connectTimeout: a much shorter 15s — establishing a socket should be
+        //    quick, so this fails fast on an unreachable host instead of making the
+        //    user wait out the full window. (URLSession has no separate connect dial;
+        //    it folds connection setup into the same 60s request timer.)
+        okhttp3.OkHttpClient.Builder()
+            .connectTimeout(CONNECT_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS)
+            .readTimeout(UPLOAD_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS)
+            .writeTimeout(UPLOAD_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS)
+            .build()
+    }
 
     private var onFileChooserRequested: ((Intent, Int) -> Unit)? = null
     private var contentChangeListener: ContentChangeListener? = null
@@ -613,10 +647,61 @@ class GutenbergView : FrameLayout {
         webView.evaluateJavascript(gbKitConfig, null)
     }
 
+    /**
+     * Syncs the current upload server's port and token into the already-loaded
+     * page.
+     *
+     * Advertises a running server so JS uploads route through it, and clears them
+     * (to `null`) when the server is stopped or was never started — so the JS
+     * upload middleware routes to the default path instead of fetching a now-dead
+     * port. The initial injection is handled by [setGlobalJavaScriptVariables]
+     * from `onPageStarted`; this keeps JS in sync when the server (re)starts or
+     * stops *after* the page has loaded (e.g. when [mediaUploadDelegate] is
+     * assigned, replaced, or cleared).
+     *
+     * The `window.GBKit` guard makes this a no-op before the page has loaded, so
+     * it is safe to call on the initial start too.
+     */
+    private fun syncUploadServerJavaScriptVariables() {
+        val portJs = uploadServer?.port?.toString() ?: "null"
+        val tokenJs = uploadServer?.token?.let { JSONObject.quote(it) } ?: "null"
+        val js = """
+            if (window.GBKit) {
+                window.GBKit.nativeUploadPort = $portJs;
+                window.GBKit.nativeUploadToken = $tokenJs;
+                localStorage.setItem('GBKit', JSON.stringify(window.GBKit));
+            }
+        """.trimIndent()
+        // evaluateJavascript must run on the WebView's (UI) thread; post it so a
+        // delegate set from a background thread doesn't throw thread-affinity.
+        webView.post { webView.evaluateJavascript(js, null) }
+    }
 
     private fun startUploadServer() {
+        // 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
+        // to upload through, so leave the server down and let uploads fall to the
+        // default WebView path rather than start a server that could only fail.
         if (configuration.siteApiRoot.isEmpty() || configuration.authHeader.isEmpty()) return
 
+        // The editor reaches the loopback server over cleartext http://localhost. If
+        // the host app's network-security config doesn't permit cleartext to
+        // localhost, the WebView blocks every upload fetch (ERR_CLEARTEXT_NOT_PERMITTED)
+        // before it leaves the page. Detect that here and don't start the server, so
+        // the JS middleware routes uploads down the default path instead of a server
+        // it can never reach. Hosts that want native media processing must permit
+        // cleartext to localhost (see the demo's res/xml/network_security_config.xml).
+        if (!NetworkSecurityPolicy.getInstance().isCleartextTrafficPermitted(LOOPBACK_HOST)) {
+            Log.w(
+                TAG,
+                "Cleartext to $LOOPBACK_HOST is not permitted, so the native media upload " +
+                    "server can't be reached from the WebView. Permit cleartext to $LOOPBACK_HOST " +
+                    "in the app's network security config to enable native media processing."
+            )
+            return
+        }
+
         try {
             val defaultUploader = DefaultMediaUploader(
                 httpClient = uploadHttpClient,
@@ -627,8 +712,10 @@ class GutenbergView : FrameLayout {
             uploadServer = MediaUploadServer(
                 uploadDelegate = mediaUploadDelegate,
                 defaultUploader = defaultUploader,
-                cacheDir = context.cacheDir
+                cacheDir = context.cacheDir,
+                scope = coroutineScope
             )
+            // JS is synced by the mediaUploadDelegate setter after this returns.
         } catch (e: Exception) {
             Log.w(TAG, "Failed to start upload server", e)
         }
@@ -1144,6 +1231,18 @@ class GutenbergView : FrameLayout {
 
         private const val ASSET_LOADING_TIMEOUT_MS = 5000L
 
+        /**
+         * Read/write inactivity timeout for media uploads, matching URLSession's
+         * 60s `timeoutIntervalForRequest` default. See [uploadHttpClient].
+         */
+        private const val UPLOAD_TIMEOUT_SECONDS = 60L
+
+        /** Connection-setup timeout for media uploads — short, to fail fast on an unreachable host. */
+        private const val CONNECT_TIMEOUT_SECONDS = 15L
+
+        /** Host the WebView uses to reach the loopback upload server (must match the JS fetch host). */
+        private const val LOOPBACK_HOST = "localhost"
+
         // Warmup state management
         private var warmupHandler: Handler? = null
         private var warmupRunnable: Runnable? = null

android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt

@@ -45,6 +45,23 @@ data class HttpRequest(
      *  for building an appropriate error response. */
     val serverError: org.wordpress.gutenberg.http.HTTPRequestParseError? = null
 ) {
+    /**
+     * The path portion of [target], without the query component
+     * (e.g., "/wp/v2/posts" for "/wp/v2/posts?per_page=10").
+     *
+     * Use this for routing — matching against [target] fails as soon as a
+     * client appends a query string.
+     */
+    val path: String
+        get() = target.substringBefore('?')
+
+    /**
+     * The query component of [target], including the leading "?"
+     * (e.g., "?per_page=10"), or an empty string when there is no query.
+     */
+    val query: String
+        get() = target.substringAfter('?', "").let { if (it.isEmpty()) "" else "?$it" }
+
     /**
      * Returns the value of the first header matching the given name (case-insensitive).
      */
@@ -71,6 +88,45 @@ data class HttpResponse(
     val body: ByteArray = ByteArray(0)
 )
 
+/** CORS behavior for an [HttpServer]. */
+enum class CorsPolicy {
+    /** No CORS headers are added (the default). */
+    None,
+
+    /**
+     * Permissive CORS for a loopback-only server serving a WebView: allows any
+     * origin and the methods/headers this library's clients use. The server
+     * answers OPTIONS preflight requests itself and stamps these headers on every
+     * response — including ones it generates internally (timeouts, parse errors)
+     * that never reach the handler.
+     */
+    Permissive;
+
+    /** Headers added to every response under this policy. */
+    val responseHeaders: Map<String, String>
+        get() = when (this) {
+            None -> emptyMap()
+            Permissive -> mapOf(
+                "Access-Control-Allow-Origin" to "*",
+                "Access-Control-Allow-Methods" to "GET, POST, PUT, DELETE, OPTIONS",
+                "Access-Control-Allow-Headers" to "Authorization, Relay-Authorization, Content-Type",
+                "Access-Control-Max-Age" to "86400"
+            )
+        }
+}
+
+/**
+ * Returns a copy with [newHeaders] added, skipping any whose name
+ * (case-insensitive) is already present.
+ */
+private fun HttpResponse.addingHeadersIfAbsent(newHeaders: Map<String, String>): HttpResponse {
+    if (newHeaders.isEmpty()) return this
+    val existing = headers.keys.map { it.lowercase() }.toSet()
+    val toAdd = newHeaders.filterKeys { it.lowercase() !in existing }
+    if (toAdd.isEmpty()) return this
+    return copy(headers = headers + toAdd)
+}
+
 /**
  * A lightweight local HTTP/1.1 server.
  *
@@ -144,6 +200,7 @@ data class HttpResponse(
  * server.stop()
  * ```
  */
+@Suppress("LongParameterList")
 class HttpServer(
     val name: String,
     private val requestedPort: Int = 0,
@@ -154,6 +211,7 @@ class HttpServer(
     private val readTimeoutMs: Int = DEFAULT_READ_TIMEOUT_MS,
     private val idleTimeoutMs: Int = DEFAULT_IDLE_TIMEOUT_MS,
     private val cacheDir: File? = null,
+    private val cors: CorsPolicy = CorsPolicy.None,
     private val handler: suspend (HttpRequest) -> HttpResponse
 ) {
     @Volatile
@@ -412,15 +470,7 @@ class HttpServer(
                     body = parsed.body,
                     parseDurationMs = parseDurationMs
                 )
-                val response = try {
-                    handler(request)
-                } catch (e: Exception) {
-                    Log.e(TAG, "Handler threw", e)
-                    HttpResponse(
-                        status = 500,
-                        body = "Internal Server Error".toByteArray()
-                    )
-                }
+                val response = resolveResponse(request)
                 sendResponse(socket, response)
                 Log.d(TAG, "${parsed.method} ${parsed.target} → ${response.status} (${"%.1f".format(parseDurationMs)}ms)")
             }
@@ -445,9 +495,27 @@ class HttpServer(
         }
     }
 
+    /**
+     * Resolves the response for a request: the CORS preflight (under a permissive
+     * policy) or the handler's response. Kept separate from [handleRequest] so
+     * that already-complex function doesn't grow.
+     */
+    private suspend fun resolveResponse(request: HttpRequest): HttpResponse {
+        if (cors == CorsPolicy.Permissive && request.method.uppercase() == "OPTIONS") {
+            return HttpResponse(status = 204, body = ByteArray(0))
+        }
+        return try {
+            handler(request)
+        } catch (e: Exception) {
+            Log.e(TAG, "Handler threw", e)
+            HttpResponse(status = 500, body = "Internal Server Error".toByteArray())
+        }
+    }
+
     private fun sendResponse(socket: Socket, response: HttpResponse) {
+        val decorated = response.addingHeadersIfAbsent(cors.responseHeaders)
         val output = socket.getOutputStream()
-        output.write(serializeResponse(response))
+        output.write(serializeResponse(decorated))
         output.flush()
     }