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..0989879ac 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -112,29 +112,34 @@ class GutenbergView : FrameLayout { var requestInterceptor: GutenbergRequestInterceptor = DefaultGutenbergRequestInterceptor() - /** Optional delegate for customizing media upload behavior (resize, transcode, custom upload). */ + /** + * Optional delegate for customizing media upload behavior (resize, transcode, + * custom upload). + * + * Provide this **before the editor loads** — typically right after + * construction (e.g. in the `AndroidView` factory). It is captured once, when + * the page begins loading, and advertised to the page then; setting it + * afterward has no effect, so the setter throws to surface the mistake. + */ var mediaUploadDelegate: MediaUploadDelegate? = null set(value) { - if (field === value) return - field = value - // 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 - // construction but before the editor finishes loading. - if (value != null) { - startUploadServer() + check(!hasStartedLoading) { + "mediaUploadDelegate must be set before the editor loads (e.g. right " + + "after construction). It is captured when the page begins loading; " + + "setting it afterward has no effect." } - // 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() + field = value } - private var uploadServer: MediaUploadServer? = null + @Volatile private var uploadServer: MediaUploadServer? = null + + /** + * True once the editor page has begun loading and the upload server's + * configuration has been captured. After this the [mediaUploadDelegate] can no + * longer take effect, so its setter throws. + */ + @Volatile private var hasStartedLoading = 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 @@ -405,7 +410,7 @@ class GutenbergView : FrameLayout { override fun onPageStarted(view: WebView?, url: String?, favicon: Bitmap?) { super.onPageStarted(view, url, favicon) - setGlobalJavaScriptVariables() + onEditorPageStarted() } override fun shouldInterceptRequest( @@ -631,6 +636,24 @@ class GutenbergView : FrameLayout { } } + /** + * Invoked when the editor page begins loading. Starts the upload server once — + * capturing the [mediaUploadDelegate] provided before load — then advertises + * the editor globals (including the server's port and token) to the page. + * + * Starting the server here, on the UI thread, rather than from the + * [mediaUploadDelegate] setter keeps its whole lifecycle — start here, stop in + * [onDetachedFromWindow] — on the UI thread, so it can't race a + * background-thread delegate assignment. + */ + private fun onEditorPageStarted() { + if (!hasStartedLoading) { + hasStartedLoading = true + startUploadServer() + } + setGlobalJavaScriptVariables() + } + private fun setGlobalJavaScriptVariables() { val gbKit = GBKitGlobal.fromConfiguration( configuration, @@ -647,37 +670,12 @@ 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() { + // No delegate means nothing wants to customize uploads, so there's no reason + // to route them through the native server — leave it down and let uploads + // fall to the default WebView path. (Matches iOS.) + if (mediaUploadDelegate == null) 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 @@ -715,7 +713,8 @@ class GutenbergView : FrameLayout { cacheDir = context.cacheDir, scope = coroutineScope ) - // JS is synced by the mediaUploadDelegate setter after this returns. + // The page globals (including the server's port/token) are injected by + // onEditorPageStarted after this returns. } catch (e: Exception) { Log.w(TAG, "Failed to start upload server", e) } 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..fefd16a3e 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt @@ -1,6 +1,7 @@ package org.wordpress.gutenberg import android.util.Log +import org.wordpress.gutenberg.http.HTTPRequestParseError import org.wordpress.gutenberg.http.HTTPRequestParser import org.wordpress.gutenberg.http.HTTPRequestParseException import org.wordpress.gutenberg.http.TempFileOwner @@ -19,11 +20,18 @@ 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.CoroutineStart import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.async import kotlinx.coroutines.cancel +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch +import kotlinx.coroutines.selects.select /** * A received HTTP request. @@ -39,11 +47,7 @@ data class HttpRequest( val target: String, val headers: Map, val body: org.wordpress.gutenberg.http.RequestBody? = null, - val parseDurationMs: Double = 0.0, - /** A server-detected error that occurred after headers were parsed - * (e.g., payload too large). When set, the handler is responsible - * for building an appropriate error response. */ - val serverError: org.wordpress.gutenberg.http.HTTPRequestParseError? = null + val parseDurationMs: Double = 0.0 ) { /** * The path portion of [target], without the query component @@ -107,6 +111,17 @@ enum class CorsPolicy { get() = when (this) { None -> emptyMap() Permissive -> mapOf( + // `*` (any origin) rather than echoing a specific origin is + // deliberate, and safe here — not an oversight to tighten. The + // server is loopback-only, and every non-OPTIONS request is gated + // by a per-session random bearer token stored only in the editor + // origin's localStorage/window.GBKit, which is origin-scoped and + // unreadable by any other origin — so no cross-origin can obtain + // it. `*` only governs whether a *token-holding* origin may read + // the response, and the sole token-holder is the editor itself, the + // legitimate client. Echoing the origin isn't viable anyway: the + // editor loads from file:// (Origin null), which can't be cleanly + // allowlisted. "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", @@ -208,10 +223,18 @@ 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, + private val delegate: HttpServerDelegate? = null, private val handler: suspend (HttpRequest) -> HttpResponse ) { @Volatile @@ -263,7 +286,16 @@ class HttpServer( socket.close() continue } - launch { + // Start ATOMIC, not DEFAULT: the permit has been acquired and + // the socket accepted, but stop() can cancel this scope in the + // window before the child is dispatched. With DEFAULT, a child + // cancelled before it starts skips its body entirely — so + // neither `finally` (release the permit) nor handleConnection's + // `socket.use` (close the fd) would run, leaking the accepted + // socket. ATOMIC guarantees the body begins: it enters + // `socket.use` and hits readUntil's first `ensureActive()`, + // which throws and unwinds cleanly through both. + launch(start = CoroutineStart.ATOMIC) { try { handleConnection(socket) } finally { @@ -317,6 +349,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,27 +364,29 @@ 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 { parser.parseRequest() } catch (e: HTTPRequestParseException) { - val statusText = STATUS_TEXT[e.error.httpStatus] ?: "Bad Request" - sendResponse(socket, HttpResponse( - status = e.error.httpStatus, - body = statusText.toByteArray() - )) + // Fatal parse error (malformed framing, smuggling-relevant, etc.): + // always answered by the library, never routed to the delegate. + sendResponse(socket, defaultErrorResponse(e.error)) return } catch (_: java.io.IOException) { sendResponse(socket, HttpResponse( @@ -385,33 +422,33 @@ 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 - // large after drain), let the handler build the response. + // A recoverable parse error (payload too large, drained above): the + // request was never fully read, so it must not reach the handler. The + // library owns the response — the delegate customizes the body if it + // wants, otherwise a correct generic error. sendResponse stamps CORS. parser.pendingParseError?.let { error -> val parseDurationMs = (System.nanoTime() - parseStart) / 1_000_000.0 - val request = HttpRequest( - method = partial.method, - target = partial.target, - headers = partial.headers, - parseDurationMs = parseDurationMs, - serverError = error - ) - val response = try { - handler(request) - } catch (e: Exception) { - Log.e(TAG, "Handler threw", e) - HttpResponse( - status = error.httpStatus, - body = (STATUS_TEXT[error.httpStatus] ?: "Error").toByteArray() - ) - } + val response = delegate?.responseForRecoverableParseError(error) + ?: defaultErrorResponse(error) sendResponse(socket, response) Log.d(TAG, "${partial.method} ${partial.target} → ${response.status} (${"%.1f".format(parseDurationMs)}ms)") return @@ -429,18 +466,20 @@ 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 { parser.parseRequest() } catch (e: HTTPRequestParseException) { - val statusText = STATUS_TEXT[e.error.httpStatus] ?: "Bad Request" - sendResponse(socket, HttpResponse( - status = e.error.httpStatus, - body = statusText.toByteArray() - )) + // Fatal parse error (malformed framing, smuggling-relevant, etc.): + // always answered by the library, never routed to the delegate. + sendResponse(socket, defaultErrorResponse(e.error)) return } catch (_: java.io.IOException) { sendResponse(socket, HttpResponse( @@ -473,15 +512,19 @@ class HttpServer( body = parsed.body, parseDurationMs = parseDurationMs ) - val response = resolveResponse(request) - sendResponse(socket, response) - Log.d(TAG, "${parsed.method} ${parsed.target} → ${response.status} (${"%.1f".format(parseDurationMs)}ms)") + val response = resolveResponseRacingClose(request, socket, input) + if (response != null) { + sendResponse(socket, response) + Log.d(TAG, "${parsed.method} ${parsed.target} → ${response.status} (${"%.1f".format(parseDurationMs)}ms)") + } else { + Log.d(TAG, "${parsed.method} ${parsed.target} → client disconnected before response; cancelled in-flight handler") + } } } } /** 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 +532,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") } @@ -509,12 +557,95 @@ class HttpServer( } return try { handler(request) + } catch (e: CancellationException) { + // Never swallow cooperative cancellation (e.g. from stop()/detach): + // rethrow so handleConnection unwinds cleanly instead of writing a 500 + // to a connection that's being torn down. + throw e } catch (e: Exception) { Log.e(TAG, "Handler threw", e) HttpResponse(status = 500, body = "Internal Server Error".toByteArray()) } } + /** + * Runs [resolveResponse], racing it against the connection's peer closing. + * + * Once the request has been fully read no bytes flow on the connection until + * the response is sent, so a read posted now can only return EOF (the client + * closed) or fail — i.e. the client went away, which is what the editor + * WebView does when it aborts an upload. A media-upload handler awaits a slow + * outbound `POST /wp/v2/media`; if the client aborts during that window, + * cancelling the handler cancels the outbound call instead of letting it run + * to completion and orphan an attachment that a retry then duplicates. + * + * A read EOF can't distinguish a full close from a client write-half-close + * (`shutdownOutput()` after the request, read half kept open for the + * response), so both are deliberately treated as an abort. That's safe here + * because the only client is the editor WebView's `fetch`, which never + * half-closes and fully closes on abort; serving a half-closer instead would + * forfeit the prompt cancellation this exists for — the two are only + * distinguishable by attempting the write, by which point an aborted upload + * has already run. A regression test pins this. + * + * Returns null if the peer closed before the handler produced a response, in + * which case the caller skips the (doomed) send. + */ + private suspend fun resolveResponseRacingClose( + request: HttpRequest, + socket: Socket, + input: BufferedInputStream + ): HttpResponse? = coroutineScope { + val handlerJob = async { resolveResponse(request) } + val watcherJob = async { awaitPeerClose(input) } + + val response = select { + handlerJob.onAwait { it } + watcherJob.onAwait { null } + } + + if (response != null) { + // The handler won. Stop watching and unblock the watcher's pending + // blocking read (soTimeout would otherwise hold it for a full idle + // interval) so this scope can join it promptly. shutdownInput closes + // only the receive half — the response can still be written. + watcherJob.cancel() + try { + socket.shutdownInput() + } catch (_: Exception) { + // Best-effort — the socket may already be closed. + } + } else { + // The peer closed first. Cancel the in-flight handler, which cancels + // the outbound relay call via its continuation's cancellation. + handlerJob.cancel() + } + response + } + + /** + * Suspends until the connection's peer closes its send half (EOF) — a full + * close or a write-half-close alike — or it fails. A + * well-behaved client sends nothing before the response, so the read blocks + * until the peer closes; the per-read idle timeout ([Socket.setSoTimeout]) + * just makes it loop. Any unexpected pre-response bytes are discarded — this + * never feeds the parser. + */ + private suspend fun awaitPeerClose(input: BufferedInputStream) { + while (true) { + currentCoroutineContext().ensureActive() + val byte = try { + input.read() + } catch (_: SocketTimeoutException) { + continue // idle window elapsed; the connection is still open + } catch (_: java.io.IOException) { + return // reset/closed (incl. shutdownInput on the handler-win path) + } + if (byte == -1) return // clean EOF — the peer closed + // Otherwise an unexpected pre-response byte; discard and keep watching. + } + } + private fun sendResponse(socket: Socket, response: HttpResponse) { val decorated = response.addingHeadersIfAbsent(cors.responseHeaders) val output = socket.getOutputStream() @@ -523,6 +654,18 @@ class HttpServer( } companion object { + /** + * The library's default response for a parse error: the mapped status code + * with a plain-text body echoing the reason phrase (e.g. 413 "Content Too + * Large"). This is what fatal errors always use, what a recoverable error + * uses when no delegate customizes it, and what an [HttpServerDelegate] can + * delegate back to for cases it doesn't handle. + */ + fun defaultErrorResponse(error: HTTPRequestParseError): HttpResponse { + val statusText = STATUS_TEXT[error.httpStatus] ?: "Error" + return HttpResponse(status = error.httpStatus, body = statusText.toByteArray()) + } + /** Default maximum number of concurrent connections. */ const val DEFAULT_MAX_CONNECTIONS: Int = 5 diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServerDelegate.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServerDelegate.kt new file mode 100644 index 000000000..a3bc7c46b --- /dev/null +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServerDelegate.kt @@ -0,0 +1,32 @@ +package org.wordpress.gutenberg + +import org.wordpress.gutenberg.http.HTTPRequestParseError + +/** + * Customization points for an [HttpServer], beyond its main request handler. + * + * Every method has a default implementation, so a conformer overrides only the + * behavior it wants to change. A server started without a delegate — or whose + * delegate leaves a method defaulted — uses the library's built-in behavior. New + * customization points are added here as new defaulted methods, so the + * [HttpServer] constructor never grows another parameter for them. + */ +interface HttpServerDelegate { + /** + * The response to send for a *recoverable* parse error — one where the request + * line and headers are well-formed but the request can't be accepted in full + * (today only an over-limit body, HTTP 413). The body was drained and is + * unavailable, and the main request handler is intentionally **not** invoked, + * so a handler can never mistake a rejected request for a normal one. + * + * The default returns a generic status + reason-phrase response + * ([HttpServer.defaultErrorResponse]). Override to supply a consumer-specific + * body — e.g. a JSON error the client can parse. The server still stamps CORS + * headers on whatever you return. + * + * Fatal parse errors (malformed framing, header smuggling, etc.) are always + * answered by the library and never routed here. + */ + fun responseForRecoverableParseError(error: HTTPRequestParseError): HttpResponse = + HttpServer.defaultErrorResponse(error) +} 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..0fcfa5e90 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -5,7 +5,11 @@ 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 kotlinx.coroutines.suspendCancellableCoroutine +import kotlin.coroutines.resume +import kotlin.coroutines.resumeWithException import org.wordpress.gutenberg.http.HeaderValue import org.wordpress.gutenberg.http.MultipartPart import org.wordpress.gutenberg.http.HTTPRequestParseError @@ -60,6 +64,24 @@ sealed class ProcessedProxyFile { * transcode video, or use its own upload service. */ interface MediaUploadDelegate { + /** + * Whether this delegate might handle a file with the given metadata — either + * processing it ([processFile]) or uploading it itself ([uploadFile]). + * + * A cheap, metadata-only gate the server consults *before* materializing the + * upload to a temp file. Return false to decline a file by type — e.g. an + * image-only delegate returning false for a video — so the server forwards + * the original upload to WordPress without first copying a file the delegate + * won't touch. Because it gates the temp-file copy needed by *both* + * [processFile] and [uploadFile], return true for any file the delegate will + * either process or upload itself. + * + * Defaults to true: every file is materialized and the full pipeline runs. A + * true here is not a commitment — [processFile] may still return + * [ProcessedProxyFile.Original] after inspecting the file's contents. + */ + fun handlesFile(mimeType: String, filename: String): Boolean = true + /** * Process a file before upload (e.g., resize image, transcode video). * @@ -97,9 +119,9 @@ 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 -) { +) : HttpServerDelegate { /** The port the server is listening on. */ val port: Int get() = server.port @@ -115,13 +137,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,8 +164,10 @@ internal class MediaUploadServer( name = "media-upload", externallyAccessible = false, requiresAuthentication = true, + bodyReadTimeoutMs = UPLOAD_BODY_READ_TIMEOUT_MS, cacheDir = cacheDir, cors = CorsPolicy.Permissive, + delegate = this, handler = { request -> handleRequest(request) } ) server.start() @@ -145,6 +177,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() } /** @@ -163,17 +197,21 @@ internal class MediaUploadServer( // MARK: - Request Handling - private suspend fun handleRequest(request: HttpRequest): HttpResponse { - // Server-detected error (e.g., payload too large) — build the - // error response here so it includes CORS headers. - request.serverError?.let { error -> - val message = when (error) { - HTTPRequestParseError.PAYLOAD_TOO_LARGE -> "The file is too large to upload in the editor." - else -> error.errorId - } - return errorResponse(error.httpStatus, message) + /** + * Answers the server's recoverable parse errors (e.g. an over-limit body) with + * the same JSON `{code, message}` shape the editor expects, so the middleware + * surfaces a real message ("The file is too large…") instead of a generic + * parse-failure. See [HttpServerDelegate]. + */ + override fun responseForRecoverableParseError(error: HTTPRequestParseError): HttpResponse { + val message = when (error) { + HTTPRequestParseError.PAYLOAD_TOO_LARGE -> "The file is too large to upload in the editor." + else -> error.errorId } + return errorResponse(error.httpStatus, message) + } + private suspend fun handleRequest(request: HttpRequest): HttpResponse { // Route: only POST /upload is handled. (OPTIONS preflight is answered by // the HTTP library under its permissive CORS policy.) Match on the path // alone — the target carries a query string (e.g. `?_embed`) that the @@ -195,6 +233,16 @@ internal class MediaUploadServer( // (e.g. ?_embed) must reach WordPress too — relay them alongside the file. val extraParts = parts.filter { it.filename == null } val query = request.query + val mimeType = filePart.contentType + val filename = filePart.filename ?: "upload" + + // Ask the delegate — from metadata alone — whether it will touch a file + // like this. If not, forward the original upload to WordPress directly, + // skipping a full temp-file copy of a file the delegate won't process or + // upload (e.g. a video handed to an image-only delegate). + if (uploadDelegate?.handlesFile(mimeType, filename) != true) { + return passthroughResponse(request, query) + } val tempFile = writePartToTempFile(filePart) ?: return errorResponse(500, "Failed to save file") @@ -202,6 +250,31 @@ internal class MediaUploadServer( return processAndRespond(request, tempFile, filePart, extraParts, query) } + @Suppress("TooGenericExceptionCaught") + private suspend fun passthroughResponse(request: HttpRequest, query: String): HttpResponse { + return try { + Log.d(TAG, "Passthrough: forwarding original request body to WordPress") + relayResponse(performPassthroughUpload(request, query)) + } catch (e: kotlin.coroutines.cancellation.CancellationException) { + throw e // Never swallow coroutine cancellation. + } catch (e: Exception) { + Log.e(TAG, "Passthrough upload failed", e) + errorResponse(500, e.message ?: "Upload failed") + } + } + + /** + * Relays WordPress's exact status and body to the editor so it sees the same + * attachment object (or error) as a direct upload. + */ + private fun relayResponse(response: MediaUploadResponse): HttpResponse { + return HttpResponse( + status = response.statusCode, + headers = mapOf("Content-Type" to "application/json"), + body = response.body + ) + } + private fun parseParts(request: HttpRequest): List? { val contentType = request.header("Content-Type") ?: return null val boundary = HeaderValue.extractParameter("boundary", contentType) ?: return null @@ -264,13 +337,7 @@ internal class MediaUploadServer( performPassthroughUpload(request, query) } } - // Relay WordPress's exact status and body to the editor so it sees - // the same attachment object (or error) as a direct upload. - return HttpResponse( - status = response.statusCode, - headers = mapOf("Content-Type" to "application/json"), - body = response.body - ) + return relayResponse(response) } catch (e: MediaUploadException) { Log.e(TAG, "Upload processing failed", e) return errorResponse(500, e.message ?: "Upload failed") @@ -382,6 +449,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 } } @@ -457,12 +534,47 @@ internal open class DefaultMediaUploader( return performUpload(request) } - private fun performUpload(request: okhttp3.Request): MediaUploadResponse { + private suspend fun performUpload(request: okhttp3.Request): MediaUploadResponse { // 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. - return httpClient.newCall(request).execute().use { response -> - MediaUploadResponse(response.code, response.body?.bytes() ?: ByteArray(0)) + // + // Enqueue rather than execute() so coroutine cancellation can tear down + // the outbound call: when the editor aborts the upload the server cancels + // this handler, and the in-flight POST /wp/v2/media must be cancelled + // rather than run to completion and orphan an attachment that a retry + // then duplicates. + val call = httpClient.newCall(request) + return suspendCancellableCoroutine { continuation -> + continuation.invokeOnCancellation { call.cancel() } + call.enqueue(object : okhttp3.Callback { + override fun onResponse(call: okhttp3.Call, response: okhttp3.Response) { + // Read the body inside a try/catch and resume the continuation + // ourselves on failure. OkHttp marks the callback as signalled + // before invoking onResponse, so a throw while reading the body + // — a truncated/reset stream, or the read timeout firing mid-body + // after WordPress already sent its 201 headers — is swallowed + // rather than routed to onFailure. Without this the continuation + // would never resume and the upload coroutine would hang forever, + // holding a connection permit. + val result = try { + response.use { + MediaUploadResponse(it.code, it.body?.bytes() ?: ByteArray(0)) + } + } catch (e: IOException) { + if (!continuation.isCancelled) continuation.resumeWithException(e) + return + } + continuation.resume(result) + } + + override fun onFailure(call: okhttp3.Call, e: IOException) { + // A cancelled call also surfaces here; the continuation is + // already resumed via cancellation, so don't resume again. + if (continuation.isCancelled) return + continuation.resumeWithException(e) + } + }) } } } 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..a83cfe5f5 --- /dev/null +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt @@ -0,0 +1,128 @@ +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.Assert.assertThrows +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 private `onEditorPageStarted` hook (fired from the WebViewClient's + * `onPageStarted`) to simulate the editor page beginning to load — the point at + * which the delegate is captured and the upload server starts. + */ + private fun startLoading(view: GutenbergView) { + val method = GutenbergView::class.java.getDeclaredMethod("onEditorPageStarted") + method.isAccessible = true + method.invoke(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 `the upload server starts when the page begins loading, capturing the delegate`() { + val view = makeView() + try { + // A delegate provided before load is captured when the page starts. + view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + startLoading(view) + idle() + assertNotNull( + "a delegate provided before load should bring up the upload server", + uploadServerOf(view) + ) + } finally { + detach(view) // stops the server, releasing the bound socket + } + } + + @Test + fun `no delegate means no upload server`() { + val view = makeView() + try { + // No delegate provided — uploads should use the default WebView path. + startLoading(view) + idle() + assertNull( + "with no delegate, no upload server should be started", + uploadServerOf(view) + ) + } finally { + detach(view) + } + } + + @Test + fun `setting the delegate after the page has started loading throws`() { + val view = makeView() + try { + startLoading(view) + idle() + // The delegate is captured at load; a later assignment is a programmer + // error and must surface loudly rather than silently do nothing. + assertThrows(IllegalStateException::class.java) { + view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + } + } finally { + detach(view) + } + } + + @Test + fun `detaching the view stops and clears the upload server`() { + val view = makeView() + view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + startLoading(view) + idle() + assertNotNull(uploadServerOf(view)) + + detach(view) + + assertNull( + "detaching the view should stop and clear the upload server", + uploadServerOf(view) + ) + } +} diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerAuthenticationTests.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerAuthenticationTests.kt index c7d882dc1..04b7c1933 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerAuthenticationTests.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerAuthenticationTests.kt @@ -265,7 +265,7 @@ class HttpServerAuthenticationTests { try { // Auth is checked on headers alone, before the oversized body is // drained or the handler runs — so the request is rejected with - // 407, not answered with the handler's 413. An unauthenticated + // 407, not answered with the library's 413. An unauthenticated // client must not be able to make the server read (and discard) // an arbitrarily large body. assertEquals(407, conn.responseCode) @@ -278,13 +278,16 @@ class HttpServerAuthenticationTests { } @Test - fun `oversized request with valid token reaches handler as serverError 413`() { + fun `oversized request with valid token is answered 413 by the library, bypassing the handler`() { val smallServer = oversizedTestServer() try { val conn = oversizedPost(smallServer) { it.setRequestProperty("Proxy-Authorization", "Bearer ${smallServer.token}") } try { + // The library answers a recoverable parse error itself; the handler + // (which would return 200 "OK") is never invoked for a rejected + // request. assertEquals(413, conn.responseCode) } finally { conn.disconnect() @@ -294,18 +297,16 @@ class HttpServerAuthenticationTests { } } - /** A server whose 1 KB body limit lets a 2 KB POST exercise the drain path. */ + /** A server whose 1 KB body limit lets a 2 KB POST exercise the drain path. Its + * handler only ever answers valid requests — a rejected (oversized) request is + * answered by the library, not here. */ private fun oversizedTestServer(): HttpServer { val smallServer = HttpServer( name = "auth-drain-test", externallyAccessible = false, requiresAuthentication = true, maxBodySize = 1024L, - handler = { request -> - request.serverError?.let { error -> - HttpResponse(status = error.httpStatus, body = "too large".toByteArray()) - } ?: HttpResponse(body = "OK\n".toByteArray()) - } + handler = { HttpResponse(body = "OK\n".toByteArray()) } ) smallServer.start() return smallServer diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerCancellationTests.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerCancellationTests.kt new file mode 100644 index 000000000..e6564c931 --- /dev/null +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerCancellationTests.kt @@ -0,0 +1,216 @@ +package org.wordpress.gutenberg + +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.delay +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import java.net.Socket +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean + +/** + * Covers the "connection close cancels the in-flight handler" behaviour. Once a + * request has been fully read, no bytes flow on the connection until the + * response is sent, so a handler awaiting slow outbound work (the media-upload + * relay awaiting `POST /wp/v2/media`) leaves the connection idle. If the peer + * closes it during that window — what the editor WebView does when it aborts an + * upload — the handler's coroutine must be cancelled so the outbound work is torn + * down instead of running to completion and orphaning an attachment. + */ +class HttpServerCancellationTests { + + @Test + fun `peer closing the connection cancels an in-flight handler`() { + val handlerStarted = CountDownLatch(1) + val cancelled = AtomicBoolean(false) + val finishedNormally = AtomicBoolean(false) + + val server = HttpServer( + name = "cancel-on-close", + externallyAccessible = false, + requiresAuthentication = true, + handler = { + handlerStarted.countDown() + try { + // Stands in for slow outbound work. A cancelled coroutine + // throws here promptly, well before the delay would elapse. + delay(10_000) + finishedNormally.set(true) + HttpResponse(body = "OK\n".toByteArray()) + } catch (e: CancellationException) { + cancelled.set(true) + throw e + } + } + ) + server.start() + try { + val sock = Socket("127.0.0.1", server.port) + sock.soTimeout = 30_000 + val request = "POST /upload HTTP/1.1\r\nHost: 127.0.0.1\r\n" + + "Proxy-Authorization: Bearer ${server.token}\r\n" + + "Content-Length: 0\r\n\r\n" + sock.getOutputStream().write(request.toByteArray()) + sock.getOutputStream().flush() + + // Once the handler is running, abort by closing the client side of the + // connection — exactly what an aborted fetch does. + assertTrue("handler never started", handlerStarted.await(5, TimeUnit.SECONDS)) + sock.close() + + // The handler must observe cancellation promptly, not run its 10s delay. + val deadline = System.currentTimeMillis() + 3_000 + while (System.currentTimeMillis() < deadline && !cancelled.get()) { + Thread.sleep(20) + } + assertTrue("handler should have been cancelled", cancelled.get()) + assertFalse("handler should not have completed normally", finishedNormally.get()) + } finally { + server.stop() + } + } + + @Test + fun `peer half-closing its write side is treated as an abort and drops the response`() { + val handlerStarted = CountDownLatch(1) + val cancelled = AtomicBoolean(false) + val finishedNormally = AtomicBoolean(false) + + val server = HttpServer( + name = "half-close-abort", + externallyAccessible = false, + requiresAuthentication = true, + handler = { + handlerStarted.countDown() + try { + delay(10_000) + finishedNormally.set(true) + HttpResponse(body = "OK\n".toByteArray()) + } catch (e: CancellationException) { + cancelled.set(true) + throw e + } + } + ) + server.start() + try { + val sock = Socket("127.0.0.1", server.port) + sock.soTimeout = 30_000 + val request = "POST /upload HTTP/1.1\r\nHost: 127.0.0.1\r\n" + + "Proxy-Authorization: Bearer ${server.token}\r\n" + + "Content-Length: 0\r\n\r\n" + sock.getOutputStream().write(request.toByteArray()) + sock.getOutputStream().flush() + + // Once the handler is running, half-close: shut down only the write half + // (a legal HTTP/1.1 "done sending" signal) while keeping the read half + // open for the response. The server sees the same EOF a full close + // produces and can't tell the two apart, so it treats it as an abort — + // see awaitPeerClose. + assertTrue("handler never started", handlerStarted.await(5, TimeUnit.SECONDS)) + sock.shutdownOutput() + + // Deliberate, documented behavior: the in-flight handler is cancelled and + // no response is written, even though the request was fully read. Pins + // the tradeoff so a future change doesn't start running aborted uploads + // to completion. + val deadline = System.currentTimeMillis() + 3_000 + while (System.currentTimeMillis() < deadline && !cancelled.get()) { + Thread.sleep(20) + } + assertTrue("handler should have been cancelled on a half-close", cancelled.get()) + assertFalse("handler should not have completed normally", finishedNormally.get()) + + // The still-open read half sees the connection close (EOF/reset), not an + // HTTP response. + val statusLine = try { + sock.getInputStream().bufferedReader().readLine() + } catch (_: java.io.IOException) { + null + } + assertTrue( + "expected no HTTP response, got: $statusLine", + statusLine == null || !statusLine.startsWith("HTTP/") + ) + sock.close() + } finally { + server.stop() + } + } + + @Test + fun `stopping the server mid-handler closes the connection without sending a response`() { + val handlerStarted = CountDownLatch(1) + + val server = HttpServer( + name = "cancel-on-stop", + externallyAccessible = false, + requiresAuthentication = true, + handler = { + handlerStarted.countDown() + delay(10_000) // cancelled by stop(); throws CancellationException + HttpResponse(body = "OK\n".toByteArray()) + } + ) + server.start() + try { + val sock = Socket("127.0.0.1", server.port) + sock.soTimeout = 30_000 + val request = "POST /upload HTTP/1.1\r\nHost: 127.0.0.1\r\n" + + "Proxy-Authorization: Bearer ${server.token}\r\n" + + "Content-Length: 0\r\n\r\n" + sock.getOutputStream().write(request.toByteArray()) + sock.getOutputStream().flush() + + // Once the handler is running, stop the server — this cancels the + // in-flight connection coroutine. + assertTrue("handler never started", handlerStarted.await(5, TimeUnit.SECONDS)) + server.stop() + + // The client must see the connection close (EOF/reset), not a 500 + // written to a connection being torn down. + val statusLine = try { + sock.getInputStream().bufferedReader().readLine() + } catch (_: java.io.IOException) { + null + } + assertTrue( + "expected no HTTP response, got: $statusLine", + statusLine == null || !statusLine.startsWith("HTTP/") + ) + } finally { + server.stop() + } + } + + @Test + fun `handler that finishes first still sends its response despite the watcher`() { + // The close watcher must not interfere with the normal path: a handler + // that completes before any close still produces a response on the live + // connection. + val server = HttpServer( + name = "no-close-normal-response", + 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 request = "POST /upload HTTP/1.1\r\nHost: 127.0.0.1\r\n" + + "Proxy-Authorization: Bearer ${server.token}\r\n" + + "Content-Length: 0\r\n\r\n" + sock.getOutputStream().write(request.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/HttpServerTimeoutTests.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerTimeoutTests.kt new file mode 100644 index 000000000..4fc867e35 --- /dev/null +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpServerTimeoutTests.kt @@ -0,0 +1,233 @@ +package org.wordpress.gutenberg + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import java.net.Socket +import java.util.concurrent.atomic.AtomicBoolean +import org.wordpress.gutenberg.http.HTTPRequestParseError + +/** + * 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 `a recoverable parse error is answered by the library and never reaches the handler`() { + val handlerCalled = AtomicBoolean(false) + val server = HttpServer( + name = "recoverable-default", + externallyAccessible = false, + requiresAuthentication = false, + maxBodySize = 16L, + handler = { + handlerCalled.set(true) + HttpResponse(body = "OK\n".toByteArray()) + } + ) + server.start() + try { + Socket("127.0.0.1", server.port).use { sock -> + sock.soTimeout = 30_000 + // A 100-byte body far exceeds the 16-byte limit → payloadTooLarge, a + // recoverable error. With no delegate, the library answers 413. + val header = "POST /test HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: 100\r\n\r\n" + sock.getOutputStream().write(header.toByteArray()) + sock.getOutputStream().write(ByteArray(100) { 0x61 }) + sock.getOutputStream().flush() + val statusLine = sock.getInputStream().bufferedReader().readLine() + assertTrue("expected 413, got: $statusLine", statusLine!!.startsWith("HTTP/1.1 413")) + } + assertFalse("a rejected request must never reach the handler", handlerCalled.get()) + } finally { + server.stop() + } + } + + @Test + fun `a delegate customizes the recoverable-error response`() { + val delegate = object : HttpServerDelegate { + override fun responseForRecoverableParseError(error: HTTPRequestParseError): HttpResponse = + HttpResponse(status = error.httpStatus, body = "custom-error-body".toByteArray()) + } + val server = HttpServer( + name = "recoverable-delegate", + externallyAccessible = false, + requiresAuthentication = false, + maxBodySize = 16L, + delegate = delegate, + handler = { HttpResponse(body = "OK\n".toByteArray()) } + ) + server.start() + try { + Socket("127.0.0.1", server.port).use { sock -> + sock.soTimeout = 30_000 + val header = "POST /test HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: 100\r\n\r\n" + sock.getOutputStream().write(header.toByteArray()) + sock.getOutputStream().write(ByteArray(100) { 0x61 }) + sock.getOutputStream().flush() + val response = sock.getInputStream().bufferedReader().readText() + assertTrue("expected 413, got: $response", response.startsWith("HTTP/1.1 413")) + assertTrue("expected the delegate's body, got: $response", response.contains("custom-error-body")) + } + } 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..e87290434 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -1,19 +1,27 @@ 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 kotlinx.coroutines.withTimeout import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer +import okhttp3.mockwebserver.SocketPolicy 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 import org.junit.Test import org.junit.rules.TemporaryFolder import java.io.File +import java.io.IOException import java.net.Socket class MediaUploadServerTest { @@ -41,6 +49,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 @@ -280,6 +319,35 @@ class MediaUploadServerTest { assertEquals(99, json.get("id").asInt) } + @Test + fun `skips processing and the temp copy when the delegate declines by metadata`() { + val delegate = DeclineByMetadataDelegate() + val mockUploader = MockDefaultUploader() + + server.stop() + server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + + val boundary = "test-boundary-decline" + val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "fake movie".toByteArray()) + + val response = sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) + // Declined by metadata → the delegate is never asked to process (so the + // file was never materialized), and the upload is passed through directly. + assertFalse(delegate.processFileCalled) + assertTrue(mockUploader.passthroughUploadCalled) + assertFalse(mockUploader.uploadCalled) + } + // MARK: - DefaultMediaUploader @Test @@ -367,6 +435,49 @@ class MediaUploadServerTest { mockWpServer.shutdown() } + @Test + fun `upload surfaces an error instead of hanging when the response body is truncated after headers`() { + // Regression test: WordPress sends the 201 status line + headers, then the + // body is truncated mid-transfer. OkHttp delivers onResponse for the 201 and + // the body read throws there — a throw OkHttp swallows rather than routing to + // onFailure. The upload must surface that as an error, not suspend forever + // holding a connection permit. + val mockWpServer = MockWebServer() + mockWpServer.enqueue( + MockResponse() + .setResponseCode(201) + .setBody("x".repeat(2048)) // large enough that DISCONNECT truncates it mid-body + .setSocketPolicy(SocketPolicy.DISCONNECT_DURING_RESPONSE_BODY) + ) + mockWpServer.start() + + val uploader = DefaultMediaUploader( + httpClient = okhttp3.OkHttpClient(), + siteApiRoot = mockWpServer.url("/wp-json/").toString(), + authHeader = "Bearer test-token" + ) + val file = tempFolder.newFile("image.jpg") + file.writeBytes("fake image".toByteArray()) + + // withTimeout is the regression guard: without the fix the coroutine never + // resumes, so this fails with a TimeoutCancellationException instead of the + // expected IOException. + val error = runCatching { + runBlocking { + withTimeout(5_000) { + uploader.upload(file, "image/jpeg", "image.jpg", emptyList(), "") + } + } + }.exceptionOrNull() + + assertTrue( + "Expected an IOException from the truncated body, got: $error", + error is IOException + ) + + mockWpServer.shutdown() + } + @Test fun `DefaultMediaUploader re-encode preserves extra parts and query`() { val mockWpServer = MockWebServer() @@ -576,6 +687,21 @@ class MediaUploadServerTest { } } + /** + * Declines every file by metadata via [handlesFile], so the server must pass + * through without materializing the file or calling [processFile]. + */ + private class DeclineByMetadataDelegate : MediaUploadDelegate { + @Volatile var processFileCalled = false + + override fun handlesFile(mimeType: String, filename: String): Boolean = false + + override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { + processFileCalled = true + return ProcessedProxyFile.Original + } + } + /** A delegate that produces a new file with changed metadata (e.g. a transcode). */ private class TranscodingDelegate : MediaUploadDelegate { /** The processed file this delegate wrote, for cleanup assertions. */ diff --git a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt index 9524bb278..572836e4c 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt @@ -20,6 +20,13 @@ class DemoMediaUploadDelegate : MediaUploadDelegate { private const val TAG = "DemoMediaUploadDelegate" } + // Only non-GIF images are ever resized (see processFile), so decline + // everything else by metadata — the server then skips copying a file this + // delegate would only pass through. + override fun handlesFile(mimeType: String, filename: String): Boolean { + return mimeType.startsWith("image/") && mimeType != "image/gif" + } + override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { if (!mimeType.startsWith("image/") || mimeType == "image/gif") { return ProcessedProxyFile.Original diff --git a/ios/Demo-iOS/Sources/Views/EditorView.swift b/ios/Demo-iOS/Sources/Views/EditorView.swift index 6d549623d..0f9b56ca4 100644 --- a/ios/Demo-iOS/Sources/Views/EditorView.swift +++ b/ios/Demo-iOS/Sources/Views/EditorView.swift @@ -297,6 +297,13 @@ private struct _EditorView: UIViewControllerRepresentable { // MARK: - MediaUploadDelegate + /// Only non-GIF images are ever resized (see `processFile`), so decline + /// everything else by metadata — the server then skips copying a file + /// this delegate would only pass through. + nonisolated func handlesFile(ofType mimeType: String, named _: String) -> Bool { + mimeType.hasPrefix("image/") && mimeType != "image/gif" + } + /// Resizes images to a maximum dimension of 2000px before upload. nonisolated func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { guard mimeType.hasPrefix("image/"), mimeType != "image/gif" else { diff --git a/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift b/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift index a13cbf550..e27d66f8a 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift @@ -12,6 +12,13 @@ public protocol EditorHTTPClientProtocol: Sendable { func performRaw(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) func download(_ urlRequest: URLRequest) async throws -> (URL, HTTPURLResponse) + + /// Returns a client tuned for large media uploads. The default returns the + /// client unchanged; ``EditorHTTPClient`` overrides it to drop the REST + /// request timeout so a silent server-side window — WordPress synchronously + /// generating image sub-sizes inside `POST /wp/v2/media` — can't trip an + /// inactivity timeout and orphan the attachment. + func uploadClient() -> any EditorHTTPClientProtocol } public extension EditorHTTPClientProtocol { @@ -20,12 +27,18 @@ public extension EditorHTTPClientProtocol { func performRaw(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { try await perform(urlRequest) } + + /// Default implementation returns the client unchanged. + func uploadClient() -> any EditorHTTPClientProtocol { self } } /// A delegate for observing HTTP requests made by the editor. /// -/// Implement this protocol to inspect or log all network requests. -public protocol EditorHTTPClientDelegate { +/// Implement this protocol to inspect or log all network requests — including the +/// media uploads and passthroughs routed through +/// ``EditorHTTPClientProtocol/uploadClient()``. Conformers are invoked from an +/// actor, so the protocol requires `Sendable` (implementations must be thread-safe). +public protocol EditorHTTPClientDelegate: Sendable { func didPerformRequest(_ request: URLRequest, response: URLResponse, data: EditorResponseData) } @@ -147,6 +160,24 @@ public actor EditorHTTPClient: EditorHTTPClientProtocol { return (url, response as! HTTPURLResponse) } + /// A sibling client tuned for large media uploads: it reuses this client's + /// session (preserving any custom configuration or pinning) and auth header, + /// but drops the REST `requestTimeout`. That timeout is an inactivity timer + /// (`URLRequest.timeoutInterval`); a short value set for snappy REST calls + /// would also fire during the silent window while WordPress synchronously + /// generates image sub-sizes inside `POST /wp/v2/media`, orphaning the + /// attachment server-side and duplicating it on retry. Uploads instead use + /// the request's default 60s inactivity timeout, mirroring Android's + /// dedicated upload client (no total-duration cap). + /// + /// The request-observing `delegate` is carried over, so a host that installs + /// one observes media uploads and passthroughs like every other request; only + /// the REST `requestTimeout` is dropped. Sharing the observer across both + /// clients is sound because `EditorHTTPClientDelegate` is `Sendable`. + public nonisolated func uploadClient() -> any EditorHTTPClientProtocol { + EditorHTTPClient(urlSession: urlSession, authHeader: authHeader, delegate: delegate) + } + private func configureRequest(_ request: URLRequest) -> URLRequest { var mutableRequest = request mutableRequest.addValue(self.authHeader, forHTTPHeaderField: "Authorization") diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 722bea35c..da4c1fefe 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -104,8 +104,52 @@ 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 + // Deliberate fail-fast, not a defensive check. The delegate is captured + // into the page's initial configuration when the editor begins loading, + // so a delegate assigned afterward would silently never take effect; + // trapping surfaces that misuse loudly instead of failing quietly. + // + // `hasStartedLoading` flips at the start of the async load (see + // `loadEditor`), which runs at or after `viewDidLoad` — so this only + // *widens* the safe window versus a synchronous flip. A host that + // follows the documented contract (set right after `init`, before + // presenting) can never race it; the trap fires only on a genuinely + // late assignment. Do not soften this to a no-op or a log — silently + // dropping the delegate is exactly the failure this is here to catch. + 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 @@ -340,6 +384,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 @@ -403,6 +451,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 } @@ -417,7 +473,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro } let defaultUploader = DefaultMediaUploader( - httpClient: httpClient, + httpClient: httpClient.uploadClient(), siteApiRoot: configuration.siteApiRoot, siteApiNamespace: configuration.siteApiNamespace ) diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index ab4c809b9..56b1cf2b6 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -40,6 +40,23 @@ public enum ProcessedProxyFile: Sendable { /// transcode video, or use its own upload service. Default implementations /// pass files through unchanged and upload via the WordPress REST API. public protocol MediaUploadDelegate: AnyObject, Sendable { + /// Whether this delegate might handle a file with the given metadata — either + /// processing it (``processFile(at:mimeType:filename:)``) or uploading it + /// itself (``uploadFile(at:mimeType:filename:)``). + /// + /// A cheap, metadata-only gate the server consults *before* materializing the + /// upload to a temp file. Return `false` to decline a file by type — e.g. an + /// image-only delegate returning `false` for a video — so the server forwards + /// the original upload to WordPress without first copying a file the delegate + /// won't touch. Because it gates the temp-file copy needed by *both* + /// `processFile` and `uploadFile`, return `true` for any file the delegate + /// will either process or upload itself. + /// + /// Defaults to `true`: every file is materialized and the full pipeline runs. + /// A `true` here is not a commitment — `processFile` may still return + /// `.original` after inspecting the file's contents. + func handlesFile(ofType mimeType: String, named filename: String) -> Bool + /// Process a file before upload (e.g., resize image, transcode video). /// /// Return ``ProcessedProxyFile/original`` to upload the file unchanged, or @@ -59,6 +76,10 @@ public protocol MediaUploadDelegate: AnyObject, Sendable { /// Default implementations. extension MediaUploadDelegate { + public func handlesFile(ofType mimeType: String, named filename: String) -> Bool { + true + } + public func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { .original } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 11b4450b1..cbab7d742 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -47,11 +47,21 @@ 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, + delegate: ServerDelegate(), handler: { request in await Self.handleRequest(request, context: context) } @@ -77,16 +87,6 @@ final class MediaUploadServer: Sendable { private static func handleRequest(_ request: HTTPServer.Request, context: UploadContext) async -> HTTPResponse { let parsed = request.parsed - // Server-detected error (e.g., payload too large) — build the - // error response here so it includes CORS headers. - if let serverError = request.serverError { - let message: String = switch serverError { - case .payloadTooLarge: "The file is too large to upload in the editor." - default: "\(serverError.httpStatusText)" - } - return errorResponse(status: serverError.httpStatus, message: message) - } - // Route: only POST /upload is handled. (OPTIONS preflight is answered by // the HTTP library under its permissive CORS policy.) Match on the path // alone — the target carries a query string (e.g. `?_embed`) that the @@ -117,18 +117,29 @@ final class MediaUploadServer: Sendable { let extraParts = parts.filter { $0.filename == nil } let query = request.parsed.query - // Write part body to a dedicated temp file for the delegate. - // - // The library's RequestBody may be a byte-range slice of a larger temp - // file whose lifecycle is tied to ARC. The delegate needs a standalone - // file that outlives the handler return, so we stream to our own file. - let filename = sanitizeFilename(filePart.filename ?? "upload") + let filename = filePart.filename ?? "upload" let mimeType = filePart.contentType + // Ask the delegate — from metadata alone — whether it will touch a file + // like this. If not, forward the original upload to WordPress directly, + // skipping a full temp-file copy of a file the delegate won't process or + // upload (e.g. a video handed to an image-only delegate). + guard context.uploadDelegate?.handlesFile(ofType: mimeType, named: filename) ?? false else { + do { + return try await passthroughResponse(request, query: query, context: context) + } catch { + return uploadErrorResponse(error) + } + } + + // The delegate wants the file. Stream the part body to a dedicated temp + // file for it — the library's RequestBody may be a byte-range slice of a + // larger temp file whose lifecycle is tied to ARC, so the delegate needs a + // standalone file that outlives the handler return. let tempDir = uploadsTempDirectory try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) - let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(filename)") + let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(sanitizeFilename(filename))") do { let inputStream = try filePart.body.makeInputStream() try writeStream(inputStream, to: fileURL) @@ -145,36 +156,62 @@ final class MediaUploadServer: Sendable { do { let uploadResult = try await processAndUpload( - fileURL: fileURL, mimeType: mimeType, filename: filePart.filename ?? "upload", + fileURL: fileURL, mimeType: mimeType, filename: filename, extraParts: extraParts, query: query, context: context ) - let response: MediaUploadResponse switch uploadResult { case .uploaded(let uploaded): Logger.uploadServer.debug("Uploaded file to WordPress") - response = uploaded + return relayResponse(uploaded) case .passthrough: - // Delegate didn't modify the file — forward the original - // request body to WordPress without re-encoding. - Logger.uploadServer.debug("Passthrough: forwarding original request body to WordPress") - guard let body = request.parsed.body, - let contentType = request.parsed.header("Content-Type"), - let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) - } - response = try await defaultUploader.passthroughUpload(body: body, contentType: contentType, query: query) + // Delegate didn't modify the file — forward the original request + // body to WordPress without re-encoding. + return try await passthroughResponse(request, query: query, context: context) } - // Relay WordPress's exact status and body to the editor so it sees - // the same attachment object (or error) as a direct upload. - return HTTPResponse( - status: response.statusCode, - headers: [("Content-Type", "application/json")], - body: response.body - ) } catch { + return uploadErrorResponse(error) + } + } + + /// Forwards the original request body to WordPress unchanged (no multipart + /// re-encoding) and relays the response. Used when the delegate won't touch + /// the file — it declined by metadata (`handlesFile` returned false) or + /// `processFile` returned `.original`. + private static func passthroughResponse( + _ request: HTTPServer.Request, query: String, context: UploadContext + ) async throws -> HTTPResponse { + Logger.uploadServer.debug("Passthrough: forwarding original request body to WordPress") + guard let body = request.parsed.body, + let contentType = request.parsed.header("Content-Type"), + let defaultUploader = context.defaultUploader else { + return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + } + let response = try await defaultUploader.passthroughUpload(body: body, contentType: contentType, query: query) + return relayResponse(response) + } + + /// Relays WordPress's exact status and body to the editor so it sees the same + /// attachment object (or error) as a direct upload. + private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { + HTTPResponse( + status: response.statusCode, + headers: [("Content-Type", "application/json")], + body: response.body + ) + } + + /// Builds the 500 response for a failed upload. A cancelled connection task + /// (editor abort / server stop) surfaces here too — as CancellationError or + /// URLError.cancelled — but isn't a failure and the server closes the + /// connection without sending this response (see HTTPServer's cancellation + /// check), so log that quietly. + private static func uploadErrorResponse(_ error: any Error) -> HTTPResponse { + if Task.isCancelled { + Logger.uploadServer.debug("Upload cancelled") + } else { Logger.uploadServer.error("Upload processing failed: \(error)") - return errorResponse(status: 500, message: error.localizedDescription) } + return errorResponse(status: 500, message: error.localizedDescription) } // MARK: - Delegate Pipeline @@ -257,6 +294,20 @@ final class MediaUploadServer: Sendable { ) } + /// Answers the server's recoverable parse errors (e.g. an over-limit body) + /// with the same JSON `{code, message}` shape the editor expects, so the + /// middleware surfaces a real message ("The file is too large…") instead of a + /// generic parse-failure. A leaf object — the HTTP server retains it. + private final class ServerDelegate: HTTPServerDelegate { + func response(forRecoverableParseError error: HTTPRequestParseError) -> HTTPResponse { + let message: String = switch error { + case .payloadTooLarge: "The file is too large to upload in the editor." + default: "\(error.httpStatusText)" + } + return MediaUploadServer.errorResponse(status: error.httpStatus, message: message) + } + } + // MARK: - Helpers /// Directory for staging uploaded files, under the system temp dir. @@ -438,7 +489,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. @@ -470,13 +544,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 { @@ -501,32 +579,73 @@ class DefaultMediaUploader: @unchecked Sendable { // writer thread — only the thread accesses it after this point. nonisolated(unsafe) let output = outputStream - Thread.detachNewThread { + Thread.detachNewThread { [preamble] in defer { output.close() try? fileHandle.close() } + _ = Self.writeMultipartBody( + fileHandle: fileHandle, fileSize: fileSize, + preamble: preamble, epilogue: epilogue, to: output + ) + } - // Write preamble (multipart headers). - guard Self.writeAll(preamble, to: output) else { return } + return (inputStream, contentLength) + } - // Stream file content in chunks. - var remaining = fileSize - while remaining > 0 { - let chunkSize = min(65_536, remaining) - guard let chunk = try? fileHandle.read(upToCount: chunkSize), - !chunk.isEmpty else { - break - } - guard Self.writeAll(chunk, to: output) else { return } - remaining -= chunk.count + /// Writes the multipart body — preamble, then the file's bytes, then the + /// closing boundary — to `output`, returning `true` only if all of it was + /// written. + /// + /// Returns `false` **without** writing the closing boundary if the file can't + /// be fully read: a mid-stream read error, or the file ending short of the + /// `fileSize` the caller measured (it shrank since). The request's + /// Content-Length reflects that measured size, so a short body can't be + /// dressed up as a complete multipart — it fails the upload rather than + /// silently corrupting it, and the real cause is logged instead of swallowed. + /// (`false` is also returned on a write failure — e.g. the consumer closing + /// the stream — matching the preamble/chunk write checks.) + static func writeMultipartBody( + fileHandle: FileHandle, + fileSize: Int, + preamble: Data, + epilogue: Data, + to output: OutputStream + ) -> Bool { + guard writeAll(preamble, to: output) else { return false } + + var remaining = fileSize + while remaining > 0 { + let chunkSize = min(65_536, remaining) + let chunk: Data + do { + chunk = try fileHandle.read(upToCount: chunkSize) ?? Data() + } catch { + Logger.uploadServer.error("Reading the upload file failed mid-stream: \(error)") + return false } - - // Write epilogue (closing boundary). - _ = Self.writeAll(epilogue, to: output) + guard !chunk.isEmpty else { + // The file ended before `fileSize` bytes — it shrank since we + // measured it. Abort rather than emit a truncated multipart. + Logger.uploadServer.error("Upload file ended \(remaining) bytes short of its measured size") + return false + } + guard writeAll(chunk, to: output) else { return false } + remaining -= chunk.count } - return (inputStream, contentLength) + return writeAll(epilogue, to: output) + } + + /// 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. diff --git a/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift b/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift index db48652cf..26524d11e 100644 --- a/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift +++ b/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift @@ -19,6 +19,17 @@ public enum CORSPolicy: Sendable { [] case .permissive: [ + // `*` (any origin) rather than echoing a specific origin is + // deliberate, and safe here — not an oversight to tighten. The + // server is loopback-only, and every non-OPTIONS request is gated + // by a per-session random bearer token stored only in the editor + // origin's `localStorage`/`window.GBKit`, which is origin-scoped + // and unreadable by any other origin — so no cross-origin can + // obtain it. `*` only governs whether a *token-holding* origin may + // read the response, and the sole token-holder is the editor + // itself, the legitimate client. Echoing the origin isn't viable + // anyway: the editor loads from `file://` (Origin `null`), which + // can't be cleanly allowlisted. ("Access-Control-Allow-Origin", "*"), ("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS"), ("Access-Control-Allow-Headers", "Authorization, Relay-Authorization, Content-Type"), 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 4352c75a5..ac05fbb01 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift @@ -75,15 +75,10 @@ public final class HTTPServer: Sendable { public let parsed: ParsedHTTPRequest /// Time spent receiving and parsing the request. public let parseDuration: Duration - /// A server-detected error that occurred after headers were parsed - /// (e.g., payload too large). When set, the handler is responsible - /// for building an appropriate error response. - public let serverError: HTTPRequestParseError? - init(parsed: ParsedHTTPRequest, parseDuration: Duration, serverError: HTTPRequestParseError? = nil) { + init(parsed: ParsedHTTPRequest, parseDuration: Duration) { self.parsed = parsed self.parseDuration = parseDuration - self.serverError = serverError } } @@ -125,10 +120,9 @@ public final class HTTPServer: Sendable { /// If no data arrives within this interval, the connection is closed with a 408 response. public static let defaultIdleTimeout: Duration = .seconds(5) - /// The default maximum time to wait for the listener to become ready (5 seconds). - /// Binding to loopback normally completes in milliseconds; the bound exists so a - /// listener stuck in the `.waiting` state (which emits no further updates) - /// cannot suspend its caller indefinitely. + /// The default ceiling on waiting for the listener to become ready (5 seconds). + /// A loopback bind completes near-instantly; this only bounds a pathological + /// listener stuck in a non-terminal state so the caller isn't hung forever. public static let defaultStartTimeout: Duration = .seconds(5) /// The maximum number of bytes to read from the network in a single receive call. @@ -151,8 +145,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. /// - startTimeout: The maximum time to wait for the listener to become ready @@ -171,9 +172,11 @@ public final class HTTPServer: Sendable { maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize, maxConnections: Int = HTTPServer.defaultMaxConnections, readTimeout: Duration = HTTPServer.defaultReadTimeout, + bodyReadTimeout: Duration? = nil, idleTimeout: Duration = HTTPServer.defaultIdleTimeout, startTimeout: Duration = HTTPServer.defaultStartTimeout, cors: CORSPolicy = .none, + delegate: HTTPServerDelegate? = nil, handler: @escaping @Sendable (HTTPServer.Request) async -> HTTPResponse ) async throws -> HTTPServer { // Sanitize to prevent path traversal — only allow safe filename characters. @@ -207,6 +210,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") @@ -217,8 +223,10 @@ 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 + connectionCounter: connectionCounter, connectionTasks: connectionTasks, + delegate: delegate, handler: handler ) } @@ -230,70 +238,64 @@ public final class HTTPServer: Sendable { } listener.start(queue: queue) - guard let terminalState = await firstTerminalState(in: states, timeout: startTimeout) else { - // No terminal state within the timeout: the listener is stuck in - // `.setup` or `.waiting` (which emit no further state updates), or - // the surrounding task was cancelled. Cancel the half-started - // listener so it doesn't leak. - listener.stateUpdateHandler = nil - listener.cancel() - Logger.httpServer.error("Listener not ready within \(startTimeout); giving up") - throw HTTPServerError.failedToStart - } - - listener.stateUpdateHandler = nil - switch terminalState { - case .ready: - guard let p = listener.port else { - listener.cancel() + // Bound the wait for readiness. The listener can sit in a non-terminal + // state (e.g. `.waiting` when it can't establish an endpoint) indefinitely; + // since callers await this — the editor load awaits the upload server's + // bind — an unbounded wait would hang the caller, not just fail the server. + // Race the readiness wait against a timeout, and tear the listener down on + // any failure path so its socket isn't leaked. On `.ready` the group + // returns the server without throwing, so a successfully-started server + // never has its listener cancelled out from under it. + do { + return try await withStartTimeout(startTimeout) { + for await state in states { + switch state { + case .ready: + listener.stateUpdateHandler = nil + guard let p = listener.port else { + throw HTTPServerError.failedToStart + } + let server = HTTPServer(listener: listener, port: p.rawValue, queue: queue, token: token, connectionTasks: connectionTasks, cleanupTask: cleanupTask) + Logger.httpServer.info("HTTP server started on port \(p.rawValue)") + return server + case .failed(let error): + Logger.httpServer.error("Listener failed: \(error)") + throw HTTPServerError.failedToStart + case .cancelled: + throw HTTPServerError.failedToStart + default: + continue + } + } throw HTTPServerError.failedToStart } - let server = HTTPServer(listener: listener, port: p.rawValue, queue: queue, token: token, connectionTasks: connectionTasks, cleanupTask: cleanupTask) - Logger.httpServer.info("HTTP server started on port \(p.rawValue)") - return server - case .failed(let error): - Logger.httpServer.error("Listener failed: \(error)") + } catch { + // Failure or timeout: the listener may still be started, so cancel it + // to release the socket. (On success the returned server owns it.) listener.cancel() - throw HTTPServerError.failedToStart - default: // .cancelled - throw HTTPServerError.failedToStart + throw error } } - /// Awaits the first terminal listener state (`.ready`, `.failed`, or - /// `.cancelled`) on `states`, skipping non-terminal states (`.setup`, - /// `.waiting`), or returns nil once `timeout` elapses without one. - /// - /// Internal for testability: a listener stuck in `.waiting` cannot be - /// reproduced deterministically with a real `NWListener`, but the wait's - /// behavior can be verified by feeding this a hand-built state stream. - static func firstTerminalState( - in states: AsyncStream, - timeout: Duration - ) async -> NWListener.State? { - await withTaskGroup(of: NWListener.State?.self) { group in + /// Races `operation` against `timeout`, throwing ``HTTPServerError/startTimeout`` + /// if the timeout wins. Used to bound the wait for the listener to become ready + /// so a caller — such as the editor load awaiting the upload server's bind — + /// isn't hung on a listener stuck in a non-terminal state. + static func withStartTimeout( + _ timeout: Duration, + _ operation: @escaping @Sendable () async throws -> T + ) async throws -> T { + try await withThrowingTaskGroup(of: T.self) { group in group.addTask { - for await state in states { - switch state { - case .ready, .failed, .cancelled: - return state - default: - continue - } - } - // The stream finished (or the task was cancelled) without a - // terminal state. - return nil + try await operation() } group.addTask { - try? await Task.sleep(for: timeout) - return nil + try await Task.sleep(for: timeout) + throw HTTPServerError.startTimeout } - // First child to finish wins: a terminal state, or nil on timeout. - // `next()` yields a double optional (`State??`); flatten it to `State?`. - let first = (await group.next()).flatMap { $0 } + let result = try await group.next()! group.cancelAll() - return first + return result } } @@ -312,6 +314,16 @@ public final class HTTPServer: Sendable { connectionTasks.cancelAll() } + /// The library's default response for a parse error: the mapped status code + /// with a plain-text body echoing the RFC reason phrase (e.g. 413 "Content Too + /// Large"). This is what fatal errors always use, what a recoverable error uses + /// when no delegate customizes it, and what an ``HTTPServerDelegate`` can + /// delegate back to for cases it doesn't handle. + public static func defaultErrorResponse(for error: HTTPRequestParseError) -> HTTPResponse { + let statusText = String(error.httpStatusText) + return HTTPResponse(status: error.httpStatus, statusText: statusText, body: Data(statusText.utf8)) + } + // MARK: - Connection Handling private static func handleConnection( @@ -321,11 +333,13 @@ public final class HTTPServer: Sendable { requiresAuthentication: Bool, maxRequestBodySize: Int64, readTimeout: Duration, + bodyReadTimeout: Duration, idleTimeout: Duration, cors: CORSPolicy, tempDirectory: URL, connectionCounter: ConnectionCounter, connectionTasks: ConnectionTasks, + delegate: HTTPServerDelegate?, handler: @escaping @Sendable (HTTPServer.Request) async -> HTTPResponse ) { connection.start(queue: queue) @@ -340,79 +354,126 @@ 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 recoverable error (e.g. payload too + // large, drained above), stop reading and let the post-measure + // branch answer it via the delegate. `request` is the body-less + // partial; the main handler is never invoked for it. + 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 - // preflight itself; the send layer stamps the CORS headers. + // A recoverable parse error (payload too large, drained above): the + // request was never fully read, so it must not reach the handler. + // The library owns the response — the delegate customizes the body if + // it wants, otherwise a correct generic error. `send` stamps CORS. let response: HTTPResponse - if cors == .permissive, request.method.uppercased() == "OPTIONS" { + if let parseError = parser.parseError { + response = delegate?.response(forRecoverableParseError: parseError) + ?? Self.defaultErrorResponse(for: parseError) + } else if cors == .permissive, request.method.uppercased() == "OPTIONS" { + // Under a permissive CORS policy the library answers the OPTIONS + // preflight itself; the send layer stamps the CORS headers. response = HTTPResponse(status: 204) } else { - response = await handler(Request(parsed: request, parseDuration: duration, serverError: parser.parseError)) + // Run the handler, but race it against the peer closing the + // connection. Once the request has been fully read, no bytes + // flow on this connection until the response is sent, so a + // handler that awaits slow outbound work — the media-upload + // relay awaiting `POST /wp/v2/media` — leaves the connection + // idle. If the client (the editor WebView) aborts the upload + // during that window, nothing here would otherwise notice, and + // the outbound request would run to completion, creating an + // orphaned attachment that a retry then duplicates. Watching for + // the close and cancelling the handler propagates cancellation + // through structured concurrency to the outbound URLSession + // task, so a cancelled upload is actually cancelled. + switch await Self.runHandler( + handler, + Request(parsed: request, parseDuration: duration), + racingCloseOf: connection + ) { + case .completed(let handlerResponse): + response = handlerResponse + case .clientDisconnected: + Logger.httpServer.debug("\(request.method) \(request.target) → client disconnected before response; cancelled in-flight handler") + connection.cancel() + return + } } + // The handler type is non-throwing and maps cancellation to a 500, + // so if the connection task was cancelled while it ran (server stop / + // editor teardown), honor that here rather than writing a doomed + // response: propagate so the outer handler just closes the connection. + try Task.checkCancellation() await send(response, on: connection, cors: cors) let (sec, atto) = duration.components let ms = Double(sec) * 1000.0 + Double(atto) / 1_000_000_000_000_000.0 @@ -421,6 +482,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() @@ -428,14 +492,10 @@ public final class HTTPServer: Sendable { Logger.httpServer.warning("Read timeout, closing connection") await send(HTTPResponse(status: 408, statusText: "Request Timeout", body: Data("Request Timeout".utf8)), on: connection, cors: cors) } catch let error as HTTPRequestParseError { + // Fatal parse error (malformed framing, smuggling-relevant, etc.): + // always answered by the library, never routed to the delegate. Logger.httpServer.error("Parse error: \(error)") - let statusText = String(error.httpStatusText) - let response = HTTPResponse( - status: error.httpStatus, - statusText: statusText, - body: Data(statusText.utf8) - ) - await send(response, on: connection, cors: cors) + await send(Self.defaultErrorResponse(for: error), on: connection, cors: cors) } catch { Logger.httpServer.error("Unexpected error: \(error)") await send(HTTPResponse(status: 400, statusText: "Bad Request", body: Data("Malformed HTTP request".utf8)), on: connection, cors: cors) @@ -444,6 +504,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. /// @@ -552,6 +634,133 @@ public final class HTTPServer: Sendable { } } + /// The result of racing a request handler against the connection's peer + /// closing it. See ``runHandler(_:_:racingCloseOf:)``. + private enum HandlerOutcome: Sendable { + case completed(HTTPResponse) + case clientDisconnected + } + + /// Runs `handler`, racing it against the connection's peer closing it. + /// + /// Between the end of the request and the start of the response a + /// well-behaved HTTP/1.1 client sends nothing, so a receive posted now can + /// only complete when the peer closes the connection (EOF) or it fails — + /// i.e. the client went away (an aborted `fetch`). If that wins the race, the + /// handler task is cancelled, which propagates through structured concurrency + /// to any outbound work the handler is awaiting, and the caller skips the + /// (doomed) send. If the handler wins, the watcher is cancelled *without* + /// cancelling the connection, so the response can still be sent. + /// + /// A read EOF can't distinguish a full close from a client *write*-half-close + /// (`shutdown(SHUT_WR)` after the request, read half kept open for the + /// response), so both are deliberately treated as an abort. That's safe here + /// because the only client is the editor WebView's `fetch`, which never + /// half-closes and fully closes on abort; serving a half-closer instead would + /// forfeit the prompt cancellation this exists for — the two are only + /// distinguishable by attempting the write, by which point an aborted upload + /// has already run. A regression test pins this. + private static func runHandler( + _ handler: @escaping @Sendable (HTTPServer.Request) async -> HTTPResponse, + _ request: HTTPServer.Request, + racingCloseOf connection: NWConnection + ) async -> HandlerOutcome { + await withTaskGroup(of: HandlerOutcome.self) { group in + group.addTask { + .completed(await handler(request)) + } + group.addTask { + await waitForConnectionClose(on: connection) + return .clientDisconnected + } + let outcome = await group.next()! + group.cancelAll() + return outcome + } + } + + /// Suspends until the connection's peer closes its send half (EOF) — a full + /// close or a write-half-close alike — or it fails, by posting a receive + /// whose bytes are discarded (it never feeds the parser). + /// A well-behaved client sends nothing before the response, so in the common + /// case the receive simply stays pending until the peer closes. + /// + /// If the surrounding task is cancelled first (the handler finished), this + /// returns *without* cancelling the connection, so the caller can still use + /// it to send the response. + private static func waitForConnectionClose(on connection: NWConnection) async { + let watcher = ConnectionCloseWatcher() + await withTaskCancellationHandler { + await withCheckedContinuation { (continuation: CheckedContinuation) in + guard watcher.store(continuation) else { return } + watchForClose(on: connection, watcher: watcher) + } + } onCancel: { + watcher.cancel() + } + } + + /// Posts a receive that wakes the watcher when the peer closes the + /// connection. Re-issues on a spurious wake or on unexpected pre-response + /// bytes (both are discarded); a normal idle connection never invokes the + /// callback until the peer actually closes. + private static func watchForClose(on connection: NWConnection, watcher: ConnectionCloseWatcher) { + connection.receive(minimumIncompleteLength: 1, maximumLength: readChunkSize) { _, _, isComplete, error in + if error != nil || isComplete { + watcher.resume() + } else { + watchForClose(on: connection, watcher: watcher) + } + } + } + + /// Resumes ``waitForConnectionClose``'s continuation exactly once, whether + /// the wake comes from the peer closing the connection or from the + /// surrounding task being cancelled. Cancellation never touches the + /// connection itself. + private final class ConnectionCloseWatcher: @unchecked Sendable { + private let lock = NSLock() + private var continuation: CheckedContinuation? + private var resumed = false + private var cancelled = false + + /// Stores the continuation. Returns `false` — and resumes immediately — + /// if cancellation already arrived, so the caller skips posting a receive. + func store(_ continuation: CheckedContinuation) -> Bool { + lock.lock() + if cancelled { + resumed = true + lock.unlock() + continuation.resume() + return false + } + self.continuation = continuation + lock.unlock() + return true + } + + /// The peer closed the connection. + func resume() { + lock.lock() + guard !resumed, let continuation else { lock.unlock(); return } + resumed = true + self.continuation = nil + lock.unlock() + continuation.resume() + } + + /// The surrounding task was cancelled (the handler finished first). + func cancel() { + lock.lock() + cancelled = true + guard !resumed, let continuation else { lock.unlock(); return } + resumed = true + self.continuation = nil + lock.unlock() + continuation.resume() + } + } + /// Sends a response on the connection and then closes it. private static func send(_ response: HTTPResponse, on connection: NWConnection, cors: CORSPolicy) async { let decorated = response.addingHeadersIfAbsent(cors.responseHeaders) @@ -639,31 +848,23 @@ 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. - /// - /// Only files older than one hour are deleted. Fresh files are preserved so - /// the sweep — which runs detached from `start()` — cannot race in-flight - /// temp files, whether they belong to this instance or another live server - /// sharing the same `name`. - private static func cleanOrphanedTempFiles(in directory: URL) { - // Only delete files past the age threshold. Fresh files may belong to a - // live server instance — the sweep runs detached from start(), so - // without the threshold it could race and delete an in-flight - // request's temp buffer. - let cutoff = Date(timeIntervalSinceNow: -3600) // 1 hour ago + /// 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. The sweep + /// runs detached from `start()` (see `cleanupTask`), off the caller's startup + /// path; the registry keeps it safe regardless of when it runs. + static func cleanOrphanedTempFiles(in directory: URL) { guard let contents = try? FileManager.default.contentsOfDirectory( - at: directory, includingPropertiesForKeys: [.contentModificationDateKey] + at: directory, includingPropertiesForKeys: nil ) else { return } - for url in contents { - let modified = (try? url.resourceValues(forKeys: [.contentModificationDateKey]))?.contentModificationDate - if let modified, modified < cutoff { - try? FileManager.default.removeItem(at: url) - } + for url in contents where !ActiveTempFiles.contains(url.lastPathComponent) { + try? FileManager.default.removeItem(at: url) } } } diff --git a/ios/Sources/GutenbergKitHTTP/HTTPServerDelegate.swift b/ios/Sources/GutenbergKitHTTP/HTTPServerDelegate.swift new file mode 100644 index 000000000..281533746 --- /dev/null +++ b/ios/Sources/GutenbergKitHTTP/HTTPServerDelegate.swift @@ -0,0 +1,41 @@ +#if canImport(Network) + +import Foundation + +/// Customization points for an ``HTTPServer``, beyond its main request handler. +/// +/// Every method has a default implementation, so a conformer implements only the +/// behavior it wants to change. A server started without a delegate — or whose +/// delegate leaves a method defaulted — uses the library's built-in behavior. +/// New customization points are added here as new defaulted methods, so +/// ``HTTPServer/start(name:port:listenOnAllInterfaces:requiresAuthentication:maxRequestBodySize:maxConnections:readTimeout:bodyReadTimeout:idleTimeout:startTimeout:cors:delegate:handler:)`` +/// never grows another parameter for them. +/// +/// The server **retains** its delegate for its lifetime. Because the delegate is +/// injected at `start(...)` rather than assigned as a back-reference, this does +/// not create a reference cycle unless the delegate itself strongly holds the +/// server — keep the delegate a leaf, or break the cycle yourself. +public protocol HTTPServerDelegate: AnyObject, Sendable { + /// The response to send for a *recoverable* parse error — one where the + /// request line and headers are well-formed but the request can't be accepted + /// in full (today only an over-limit body, HTTP 413). The body was drained and + /// is unavailable, and the main request handler is intentionally **not** + /// invoked, so a handler can never mistake a rejected request for a normal one. + /// + /// The default returns a generic status + reason-phrase response + /// (``HTTPServer/defaultErrorResponse(for:)``). Override to supply a + /// consumer-specific body — e.g. a JSON error the client can parse. The server + /// still stamps CORS headers on whatever you return. + /// + /// Fatal parse errors (malformed framing, header smuggling, etc.) are always + /// answered by the library and never routed here. + func response(forRecoverableParseError error: HTTPRequestParseError) -> HTTPResponse +} + +public extension HTTPServerDelegate { + func response(forRecoverableParseError error: HTTPRequestParseError) -> HTTPResponse { + HTTPServer.defaultErrorResponse(for: error) + } +} + +#endif // canImport(Network) diff --git a/ios/Sources/GutenbergKitHTTP/HTTPServerError.swift b/ios/Sources/GutenbergKitHTTP/HTTPServerError.swift index 6c054329d..1b24009a5 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPServerError.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPServerError.swift @@ -7,6 +7,10 @@ import Network public enum HTTPServerError: Error, LocalizedError, Sendable { /// The server failed to bind to the requested port. case failedToStart + /// The listener did not become ready within the start timeout (e.g. it was + /// stuck in `.waiting`). Bounds the bind wait so a caller — such as the + /// editor load — isn't hung indefinitely on a listener that never binds. + case startTimeout /// The connection closed before a complete request was received. case connectionClosed /// The read timeout expired before a complete request was received. @@ -15,16 +19,21 @@ 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) public var errorDescription: String? { switch self { case .failedToStart: "Failed to start HTTP server" + case .startTimeout: "HTTP server listener did not become ready within the start timeout" case .connectionClosed: "Connection closed before request was complete" 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/HTTPServerCancellationTests.swift b/ios/Tests/GutenbergKitHTTPTests/HTTPServerCancellationTests.swift new file mode 100644 index 000000000..817abdc97 --- /dev/null +++ b/ios/Tests/GutenbergKitHTTPTests/HTTPServerCancellationTests.swift @@ -0,0 +1,292 @@ +#if canImport(Network) + +import Foundation +import Network +import Testing +@testable import GutenbergKitHTTP + +/// Covers the "connection close cancels the in-flight handler" behaviour. Once a +/// request has been fully read, no bytes flow on the connection until the +/// response is sent, so a handler awaiting slow outbound work (the media-upload +/// relay awaiting `POST /wp/v2/media`) leaves the connection idle. If the peer +/// closes it during that window — what the editor WebView does when it aborts an +/// upload — the handler's task must be cancelled so the outbound work is torn +/// down instead of running to completion and orphaning an attachment. +@Suite("HTTPServer Cancellation") +struct HTTPServerCancellationTests { + + @Test("peer closing the connection cancels an in-flight handler") + func peerCloseCancelsHandler() async throws { + let signals = HandlerSignals() + + let server = try await HTTPServer.start( + name: "cancel-on-close", + requiresAuthentication: true + ) { _ in + signals.markStarted() + do { + // Stands in for slow outbound work. A cancelled task throws here + // promptly, well before the sleep would otherwise complete. + try await Task.sleep(for: .seconds(10)) + signals.markFinishedNormally() + } catch { + signals.markCancelled() + } + return HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + defer { server.stop() } + + // Open a raw connection and send a complete request so the handler runs. + let connection = NWConnection( + host: .ipv4(.loopback), + port: NWEndpoint.Port(rawValue: server.port)!, + using: .tcp + ) + try await waitUntilReady(connection) + let request = "POST /upload HTTP/1.1\r\nHost: 127.0.0.1\r\nProxy-Authorization: Bearer \(server.token)\r\nContent-Length: 0\r\n\r\n" + try await send(Data(request.utf8), on: connection) + + // Once the handler is actually running, abort by closing the client side + // of the connection — exactly what an aborted `fetch` does. + try await signals.waitUntilStarted() + connection.cancel() + + // The handler must observe cancellation promptly, not run its 10s sleep + // to completion. + let cancelled = await signals.waitUntilCancelled(timeout: .seconds(3)) + #expect(cancelled) + #expect(!signals.didFinishNormally) + } + + @Test("a client write-half-close mid-handler is treated as an abort, and the response is dropped") + func halfCloseIsTreatedAsAbort() async throws { + // A client MAY legally half-close its write half (`shutdown(SHUT_WR)`) + // after a complete request, keeping its read half open for the response. + // The server sees the same read EOF a full close produces and can't tell + // the two apart, so — by design — it treats the half-close as an abort: it + // cancels the in-flight handler and sends no response. This pins that + // deliberate tradeoff (prompt cancellation of the outbound POST /wp/v2/media + // so an aborted upload can't orphan an attachment) against a future change + // that "fixes" the half-close and, with it, silently resurrects the orphan + // bug. Safe in practice: the only client is the editor WebView's `fetch`, + // which never half-closes and fully closes on abort. + let signals = HandlerSignals() + + let server = try await HTTPServer.start( + name: "half-close-abort", + requiresAuthentication: true + ) { _ in + signals.markStarted() + do { + try await Task.sleep(for: .seconds(10)) + signals.markFinishedNormally() + } catch { + signals.markCancelled() + } + return HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + defer { server.stop() } + + let connection = NWConnection( + host: .ipv4(.loopback), + port: NWEndpoint.Port(rawValue: server.port)!, + using: .tcp + ) + try await waitUntilReady(connection) + let request = "POST /upload HTTP/1.1\r\nHost: 127.0.0.1\r\nProxy-Authorization: Bearer \(server.token)\r\nContent-Length: 0\r\n\r\n" + try await send(Data(request.utf8), on: connection) + + // Once the handler is running, half-close the write half only — the read + // half stays open to receive the (never-sent) response. + try await signals.waitUntilStarted() + try await halfCloseWrite(connection) + + // Treated exactly like a full close: handler cancelled, no response. + let cancelled = await signals.waitUntilCancelled(timeout: .seconds(3)) + #expect(cancelled) + #expect(!signals.didFinishNormally) + + let received = (try? await receiveResponse(connection)) ?? "" + #expect(!received.hasPrefix("HTTP/1.1")) + connection.cancel() + } + + @Test("stopping the server mid-handler closes the connection without sending a response") + func serverStopMidHandlerSendsNoResponse() async throws { + let signals = HandlerSignals() + + let server = try await HTTPServer.start( + name: "cancel-on-stop", + requiresAuthentication: true + ) { _ in + signals.markStarted() + do { + try await Task.sleep(for: .seconds(10)) + } catch { + signals.markCancelled() + } + // The handler is non-throwing, so it still returns a response after + // being cancelled — the server must NOT write it to the dying + // connection. + return HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + + let connection = NWConnection( + host: .ipv4(.loopback), + port: NWEndpoint.Port(rawValue: server.port)!, + using: .tcp + ) + try await waitUntilReady(connection) + let request = "POST /upload HTTP/1.1\r\nHost: 127.0.0.1\r\nProxy-Authorization: Bearer \(server.token)\r\nContent-Length: 0\r\n\r\n" + try await send(Data(request.utf8), on: connection) + + // Once the handler is running, stop the server — this cancels the + // in-flight connection task. + try await signals.waitUntilStarted() + server.stop() + + // The client must see the connection close (EOF/reset), not an HTTP + // response written to a connection being torn down. + let received = (try? await receiveResponse(connection)) ?? "" + #expect(!received.hasPrefix("HTTP/1.1")) + connection.cancel() + } + + @Test("handler that finishes first still sends its response despite the watcher") + func handlerFinishesFirstStillResponds() async throws { + // The close watcher must not interfere with the normal path: a handler + // that completes before any close still produces a response on the live + // connection. + let server = try await HTTPServer.start( + name: "no-close-normal-response", + requiresAuthentication: true + ) { _ in + HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + defer { server.stop() } + + let request = "POST /upload HTTP/1.1\r\nHost: 127.0.0.1\r\nProxy-Authorization: Bearer \(server.token)\r\nContent-Length: 0\r\n\r\n" + let response = try await sendRaw(request, toPort: server.port) + #expect(response.hasPrefix("HTTP/1.1 200")) + } + + // MARK: - Helpers + + private func waitUntilReady(_ connection: NWConnection) async throws { + 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()) + } + } + + 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() } + }) + } + } + + /// Half-closes the connection's send half (a FIN via an empty final message) + /// while leaving the receive half open — Network.framework's `shutdown(SHUT_WR)`. + private func halfCloseWrite(_ connection: NWConnection) async throws { + try await withCheckedThrowingContinuation { (cont: CheckedContinuation) in + connection.send( + content: nil, + contentContext: .finalMessage, + isComplete: true, + completion: .contentProcessed { error in + if let error { cont.resume(throwing: error) } else { cont.resume() } + } + ) + } + } + + /// Reads one chunk from the connection, returning it as a string (empty on a + /// clean EOF). Throws if the connection errors (e.g. reset). + private func receiveResponse(_ connection: NWConnection) async throws -> String { + try await withCheckedThrowingContinuation { (cont: CheckedContinuation) 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 sendRaw(_ request: String, toPort port: UInt16) async throws -> String { + let connection = NWConnection( + host: .ipv4(.loopback), + port: NWEndpoint.Port(rawValue: port)!, + using: .tcp + ) + defer { connection.cancel() } + try await waitUntilReady(connection) + try await send(Data(request.utf8), 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) ?? "") + } + } + } + } +} + +/// Thread-safe coordinator letting a test observe when the server's handler +/// starts and whether it observed cancellation. +private final class HandlerSignals: @unchecked Sendable { + private let lock = NSLock() + private var started = false + private var cancelled = false + private var finishedNormally = false + + func markStarted() { lock.lock(); started = true; lock.unlock() } + func markCancelled() { lock.lock(); cancelled = true; lock.unlock() } + func markFinishedNormally() { lock.lock(); finishedNormally = true; lock.unlock() } + + private var isStarted: Bool { lock.lock(); defer { lock.unlock() }; return started } + private var isCancelled: Bool { lock.lock(); defer { lock.unlock() }; return cancelled } + var didFinishNormally: Bool { lock.lock(); defer { lock.unlock() }; return finishedNormally } + + func waitUntilStarted(timeout: Duration = .seconds(3)) async throws { + let clock = ContinuousClock() + let deadline = clock.now + timeout + while clock.now < deadline { + if isStarted { return } + try await Task.sleep(for: .milliseconds(20)) + } + throw HandlerSignalTimeout.handlerNeverStarted + } + + func waitUntilCancelled(timeout: Duration) async -> Bool { + let clock = ContinuousClock() + let deadline = clock.now + timeout + while clock.now < deadline { + if isCancelled { return true } + try? await Task.sleep(for: .milliseconds(20)) + } + return isCancelled + } +} + +private enum HandlerSignalTimeout: Error { + case handlerNeverStarted +} + +#endif // canImport(Network) diff --git a/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift b/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift index 7c90f0ed0..17be7d603 100644 --- a/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift +++ b/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift @@ -8,42 +8,10 @@ import Testing @Suite("HTTPServer Start") struct HTTPServerStartTests { - @Test("readiness wait returns nil after the timeout when only non-terminal states arrive") - func readinessWaitTimesOut() async { - // A listener stuck in `.setup`/`.waiting` emits no further state - // updates. Without the bound, the wait — and whatever startup path - // awaits `HTTPServer.start` — would suspend forever. - let (states, continuation) = AsyncStream.makeStream(of: NWListener.State.self) - continuation.yield(.setup) - continuation.yield(.waiting(.posix(.EADDRINUSE))) - - let clock = ContinuousClock() - let started = clock.now - let result = await HTTPServer.firstTerminalState(in: states, timeout: .milliseconds(200)) - let elapsed = clock.now - started - continuation.finish() - - #expect(result == nil) - // Waited out the timeout, then returned promptly instead of hanging. - #expect(elapsed >= .milliseconds(150)) - #expect(elapsed < .seconds(5)) - } - - @Test("readiness wait skips non-terminal states and returns the first terminal one") - func readinessWaitReturnsTerminalState() async { - let (states, continuation) = AsyncStream.makeStream(of: NWListener.State.self) - continuation.yield(.setup) - continuation.yield(.waiting(.posix(.EADDRINUSE))) - continuation.yield(.ready) - - let result = await HTTPServer.firstTerminalState(in: states, timeout: .seconds(5)) - continuation.finish() - - guard case .ready = result else { - Issue.record("Expected .ready, got \(String(describing: result))") - return - } - } + // The unit-level readiness-wait timeout behavior (a listener stuck in a + // non-terminal state must not hang the caller) is covered by + // `HTTPServerTimeoutTests` against `HTTPServer.withStartTimeout`. This suite + // keeps the end-to-end port-conflict check below. @Test("start fails promptly when the port is already taken (no hang)") func startFailsPromptlyOnPortConflict() async throws { diff --git a/ios/Tests/GutenbergKitHTTPTests/HTTPServerTimeoutTests.swift b/ios/Tests/GutenbergKitHTTPTests/HTTPServerTimeoutTests.swift new file mode 100644 index 000000000..4fc444480 --- /dev/null +++ b/ios/Tests/GutenbergKitHTTPTests/HTTPServerTimeoutTests.swift @@ -0,0 +1,277 @@ +#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: - Start timeout + + @Test("the start-readiness wait fails with .startTimeout instead of hanging") + func startTimeoutFiresOnStuckBind() async { + // `withStartTimeout` bounds `HTTPServer.start`'s wait for the listener to + // become ready. If the readiness wait never completes (a listener stuck in + // a non-terminal state), it must fail near the deadline — not hang the + // caller (the editor load awaits this) for the whole operation. + let clock = ContinuousClock() + let start = clock.now + do { + _ = try await HTTPServer.withStartTimeout(.milliseconds(100)) { + try await Task.sleep(for: .seconds(60)) // stands in for a stuck bind + return 0 + } + Issue.record("expected withStartTimeout to throw .startTimeout") + } catch HTTPServerError.startTimeout { + // Expected: the timeout won the race. + } catch { + Issue.record("expected .startTimeout, got \(error)") + } + #expect(clock.now - start < .seconds(2)) + } + + @Test("the start-readiness wait returns the bound server when it's ready in time") + func startTimeoutPassesValueThroughWhenReady() async throws { + // The common case: readiness completes well within the deadline, so the + // timeout is inert and the operation's value flows through unchanged. + let value = try await HTTPServer.withStartTimeout(.seconds(5)) { 42 } + #expect(value == 42) + } + + // MARK: - Recoverable parse errors + + @Test("a recoverable parse error is answered by the library and never reaches the handler") + func recoverableErrorBypassesHandler() async throws { + let handlerCalled = CallFlag() + let server = try await HTTPServer.start( + name: "recoverable-default", + requiresAuthentication: false, + maxRequestBodySize: 16 + ) { _ in + handlerCalled.mark() + return HTTPResponse(status: 200, body: Data("OK".utf8)) + } + defer { server.stop() } + + // A 100-byte body far exceeds the 16-byte limit → payloadTooLarge, a + // recoverable error. With no delegate, the library answers a generic 413. + let header = "POST /test HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: 100\r\n\r\n" + let response = try await sendChunked(header, chunks: [Data(repeating: 0x61, count: 100)], gap: .zero, toPort: server.port) + + #expect(response.hasPrefix("HTTP/1.1 413")) + #expect(!handlerCalled.wasCalled) // a rejected request must never reach the handler + } + + @Test("a delegate customizes the recoverable-error response") + func delegateCustomizesRecoverableError() async throws { + let server = try await HTTPServer.start( + name: "recoverable-delegate", + requiresAuthentication: false, + maxRequestBodySize: 16, + delegate: CustomErrorDelegate() + ) { _ in + HTTPResponse(status: 200, body: Data("OK".utf8)) + } + defer { server.stop() } + + let header = "POST /test HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: 100\r\n\r\n" + let response = try await sendChunked(header, chunks: [Data(repeating: 0x61, count: 100)], gap: .zero, toPort: server.port) + + #expect(response.hasPrefix("HTTP/1.1 413")) + #expect(response.contains("custom-error-body")) // the delegate's body, not the generic one + } + + // 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) + } +} + +/// A thread-safe one-way flag for asserting whether a handler ran. +private final class CallFlag: @unchecked Sendable { + private let lock = NSLock() + private var value = false + func mark() { lock.lock(); value = true; lock.unlock() } + var wasCalled: Bool { lock.lock(); defer { lock.unlock() }; return value } +} + +/// A delegate that returns a recognizable body for a recoverable parse error. +private final class CustomErrorDelegate: HTTPServerDelegate { + func response(forRecoverableParseError error: HTTPRequestParseError) -> HTTPResponse { + HTTPResponse(status: error.httpStatus, body: Data("custom-error-body".utf8)) + } +} + +#endif // canImport(Network) diff --git a/ios/Tests/GutenbergKitHTTPTests/RFC9112ConformanceTests.swift b/ios/Tests/GutenbergKitHTTPTests/RFC9112ConformanceTests.swift index 7bb63c330..c47836dba 100644 --- a/ios/Tests/GutenbergKitHTTPTests/RFC9112ConformanceTests.swift +++ b/ios/Tests/GutenbergKitHTTPTests/RFC9112ConformanceTests.swift @@ -572,7 +572,7 @@ struct RFC9112ConformanceTests { // MARK: - L5: Orphaned temp file cleanup - @Test("cleanOrphanedTempFiles removes stale files in the server-specific temp directory") + @Test("cleanOrphanedTempFiles removes unregistered orphans but preserves live registered buffers") func orphanedTempFilesCleanedOnStart() async throws { let serverName = "orphan-cleanup-test" let serverTempDir = FileManager.default.temporaryDirectory @@ -581,37 +581,34 @@ struct RFC9112ConformanceTests { let orphan1 = serverTempDir.appendingPathComponent("GutenbergKitHTTP-\(UUID().uuidString)") let orphan2 = serverTempDir.appendingPathComponent("GutenbergKitHTTP-\(UUID().uuidString)") - let fresh = serverTempDir.appendingPathComponent("GutenbergKitHTTP-\(UUID().uuidString)") + // A file backing an in-flight request of a concurrent server sharing this + // dir: registered in `ActiveTempFiles`, so the sweep must preserve it + // regardless of age. The orphans have no owner in this process. + let live = serverTempDir.appendingPathComponent("GutenbergKitHTTP-\(UUID().uuidString)") let unrelated = FileManager.default.temporaryDirectory .appendingPathComponent("SomeOtherFile-\(UUID().uuidString)") FileManager.default.createFile(atPath: orphan1.path, contents: Data("test".utf8)) FileManager.default.createFile(atPath: orphan2.path, contents: Data("test".utf8)) - FileManager.default.createFile(atPath: fresh.path, contents: Data("test".utf8)) + FileManager.default.createFile(atPath: live.path, contents: Data("test".utf8)) FileManager.default.createFile(atPath: unrelated.path, contents: Data("test".utf8)) + ActiveTempFiles.register(live.lastPathComponent) defer { - try? FileManager.default.removeItem(at: fresh) + ActiveTempFiles.unregister(live.lastPathComponent) + try? FileManager.default.removeItem(at: live) try? FileManager.default.removeItem(at: unrelated) } - // Backdate the orphans well past the 1-hour cutoff. The fresh file - // stays recent — the sweep runs detached from start(), so it must - // preserve files that could belong to a live server instance. - for orphan in [orphan1, orphan2] { - try FileManager.default.setAttributes( - [.modificationDate: Date(timeIntervalSinceNow: -7200)], - ofItemAtPath: orphan.path - ) - } - // start() kicks off cleanOrphanedTempFiles() off the startup path; - // await it before asserting. + // start() kicks off cleanOrphanedTempFiles() off the startup path; await it + // before asserting. The sweep removes every file in the server's temp dir + // that isn't registered as live, and never touches files outside that dir. let server = try await HTTPServer.start(name: serverName, handler: { _ in HTTPResponse(status: 200) }) await server.cleanupTask.value server.stop() #expect(!FileManager.default.fileExists(atPath: orphan1.path)) #expect(!FileManager.default.fileExists(atPath: orphan2.path)) - #expect(FileManager.default.fileExists(atPath: fresh.path)) + #expect(FileManager.default.fileExists(atPath: live.path)) #expect(FileManager.default.fileExists(atPath: unrelated.path)) } } 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/EditorHTTPClientTests.swift b/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift index 7676888d3..0d49d9bfe 100644 --- a/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift +++ b/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift @@ -167,6 +167,74 @@ struct EditorHTTPClientTests { #expect(capturedRequest.timeoutInterval == 45) } + @Test("uploadClient() drops the REST request timeout so uploads aren't cut off mid-resize") + func uploadClientDropsRESTTimeout() async throws { + let spySession = SpyURLSession() + let authHeader = "Bearer upload-token" + // A short REST timeout a host might set for snappy REST calls. + let restTimeout: TimeInterval = 15 + let client = EditorHTTPClient( + urlSession: spySession, + authHeader: authHeader, + requestTimeout: restTimeout + ) + + let uploadClient = client.uploadClient() + let request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/media")!) + _ = try await uploadClient.performRaw(request) + + let captured = try #require(spySession.lastCapturedRequest) + // The short REST timeout must not bleed into uploads — the request keeps + // its own (default) inactivity timeout instead. + #expect(captured.timeoutInterval != restTimeout) + #expect(captured.timeoutInterval == request.timeoutInterval) + // Auth and the shared session are still used. + #expect(captured.value(forHTTPHeaderField: "Authorization") == authHeader) + } + + @Test("uploadClient() preserves an explicit upload request timeout instead of clobbering it") + func uploadClientPreservesExplicitTimeout() async throws { + let spySession = SpyURLSession() + let client = EditorHTTPClient( + urlSession: spySession, + authHeader: "Bearer token", + requestTimeout: 15 + ) + + let uploadClient = client.uploadClient() + var request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/media")!) + request.timeoutInterval = 120 + + _ = try await uploadClient.performRaw(request) + + // On the REST client the requestTimeout (15) would clobber this to 15; + // the upload client leaves it alone. + let captured = try #require(spySession.lastCapturedRequest) + #expect(captured.timeoutInterval == 120) + } + + @Test("uploadClient() carries the request-observing delegate so uploads are observed too") + func uploadClientCarriesDelegate() async throws { + let spySession = SpyURLSession() + let spyDelegate = SpyHTTPClientDelegate() + let client = EditorHTTPClient( + urlSession: spySession, + authHeader: "Bearer token", + delegate: spyDelegate + ) + + let uploadClient = client.uploadClient() + let request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/media")!) + _ = try await uploadClient.performRaw(request) + + // The delegate installed on the REST client must also observe requests + // made through the upload sibling — otherwise a host logging "all network + // requests" silently misses every media upload. + #expect(spyDelegate.callCount == 1) + let observed = try #require(spyDelegate.lastCall) + #expect(observed.request.url == request.url) + } + // MARK: - Cookie Handling Tests @Test("perform() disables cookie handling") diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 3f8ffeb79..97a300733 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -198,6 +198,34 @@ struct MediaUploadServerTests { #expect(json["id"] as? Int == 99) } + @Test("skips processing and the temp copy when the delegate declines by metadata") + func delegateDeclinesByMetadata() async throws { + let delegate = DeclineByMetadataDelegate() + let mockUploader = MockDefaultUploader() + let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", data: Data("movie".utf8)) + + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + let (_, response) = try await URLSession.shared.data(for: request) + let httpResponse = try #require(response as? HTTPURLResponse) + #expect(httpResponse.statusCode == 201) + + // Declined by metadata → the delegate is never asked to process (so the file + // was never materialized), and the upload is passed through directly. + #expect(!delegate.processFileCalled) + #expect(mockUploader.passthroughUploadCalled) + #expect(!mockUploader.uploadCalled) + } + @Test("forwards the delegate's processed metadata to the uploader") func processedMetadataForwarded() async throws { let delegate = ResizingDelegate() @@ -394,6 +422,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" @@ -469,6 +520,61 @@ struct MultipartBodyStreamTests { let result = readAllFromStream(stream) #expect(result.count == contentLength) } + + @Test("writeMultipartBody streams the full body and closing boundary when the file reads cleanly") + func writeMultipartBodyWritesFullBody() throws { + let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent("wmb-\(UUID().uuidString)") + let fileContent = Data("the file bytes".utf8) + try fileContent.write(to: tempFile) + defer { try? FileManager.default.removeItem(at: tempFile) } + + let fileHandle = try FileHandle(forReadingFrom: tempFile) + defer { try? fileHandle.close() } + + let output = OutputStream.toMemory() + output.open() + defer { output.close() } + + let preamble = Data("PREAMBLE".utf8) + let epilogue = Data("EPILOGUE".utf8) + let ok = DefaultMediaUploader.writeMultipartBody( + fileHandle: fileHandle, fileSize: fileContent.count, + preamble: preamble, epilogue: epilogue, to: output + ) + + #expect(ok) + let written = output.property(forKey: .dataWrittenToMemoryStreamKey) as? Data + #expect(written == preamble + fileContent + epilogue) + } + + @Test("writeMultipartBody aborts without the closing boundary when the file is shorter than measured") + func writeMultipartBodyAbortsOnShortFile() throws { + let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent("wmb-short-\(UUID().uuidString)") + let fileContent = Data("only ten!!".utf8) // 10 bytes + try fileContent.write(to: tempFile) + defer { try? FileManager.default.removeItem(at: tempFile) } + + let fileHandle = try FileHandle(forReadingFrom: tempFile) + defer { try? fileHandle.close() } + + let output = OutputStream.toMemory() + output.open() + defer { output.close() } + + let preamble = Data("PREAMBLE".utf8) + let epilogue = Data("EPILOGUE".utf8) + // Claim the file is larger than it is, as if it shrank after being measured. + let ok = DefaultMediaUploader.writeMultipartBody( + fileHandle: fileHandle, fileSize: fileContent.count + 100, + preamble: preamble, epilogue: epilogue, to: output + ) + + #expect(!ok) + // The preamble and the real file bytes were written, but NOT the closing + // boundary — a short body must not masquerade as a complete multipart. + let written = (output.property(forKey: .dataWrittenToMemoryStreamKey) as? Data) ?? Data() + #expect(written == preamble + fileContent) + } } // MARK: - DefaultMediaUploader Relay Tests @@ -634,6 +740,23 @@ private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendabl } } +/// A delegate that declines every file by metadata via `handlesFile`, so the +/// server must pass through without ever materializing the file or calling +/// `processFile`. +private final class DeclineByMetadataDelegate: MediaUploadDelegate, @unchecked Sendable { + private let lock = NSLock() + private var _processFileCalled = false + + var processFileCalled: Bool { lock.withLock { _processFileCalled } } + + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } + + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + lock.withLock { _processFileCalled = true } + return .original + } +} + /// A delegate that produces a new file with changed metadata (e.g. a transcode). private final class ResizingDelegate: MediaUploadDelegate, @unchecked Sendable { private let lock = NSLock() diff --git a/src/utils/api-fetch-upload-middleware.test.js b/src/utils/api-fetch-upload-middleware.test.js index 2bdae9c13..21a75a14e 100644 --- a/src/utils/api-fetch-upload-middleware.test.js +++ b/src/utils/api-fetch-upload-middleware.test.js @@ -160,6 +160,24 @@ describe( 'nativeMediaUploadMiddleware', () => { expect( global.fetch ).not.toHaveBeenCalled(); } ); + it( 'passes through when the file field is not a File (e.g. a string)', () => { + getGBKit.mockReturnValue( { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + } ); + const next = makeNext(); + const body = new FormData(); + body.append( 'file', 'not-a-file' ); + + nativeMediaUploadMiddleware( + { method: 'POST', path: '/wp/v2/media', body }, + next + ); + + expect( next ).toHaveBeenCalled(); + expect( global.fetch ).not.toHaveBeenCalled(); + } ); + // MARK: - Interception it( 'intercepts POST /wp/v2/media with file and fetches to local server', async () => { @@ -406,7 +424,64 @@ describe( 'nativeMediaUploadMiddleware', () => { ).rejects.toMatchObject( { code: 'invalid_json' } ); } ); - it( 'surfaces a transport failure instead of retrying (no silent duplicate)', async () => { + it( 'propagates an abort during the 2xx response body read instead of invalid_json', async () => { + getGBKit.mockReturnValue( { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + } ); + const next = makeNext(); + + // Headers arrive (fetch resolves), but the user aborts before the body + // finishes streaming — json() rejects while the signal is now aborted. The + // middleware must surface the cancellation, not an "invalid response". + const abortError = new DOMException( 'Aborted', 'AbortError' ); + const signal = { aborted: false, reason: undefined }; + const options = { ...makePostMediaOptions( makeFile() ), signal }; + global.fetch = vi.fn( () => + Promise.resolve( { + ok: true, + json: () => { + signal.aborted = true; + signal.reason = abortError; + return Promise.reject( abortError ); + }, + } ) + ); + + await expect( + nativeMediaUploadMiddleware( options, next ) + ).rejects.toBe( abortError ); + expect( next ).not.toHaveBeenCalled(); + } ); + + it( 'propagates an abort during a non-2xx response body read instead of invalid_json', async () => { + getGBKit.mockReturnValue( { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + } ); + const next = makeNext(); + + const abortError = new DOMException( 'Aborted', 'AbortError' ); + const signal = { aborted: false, reason: undefined }; + const options = { ...makePostMediaOptions( makeFile() ), signal }; + global.fetch = vi.fn( () => + Promise.resolve( { + ok: false, + json: () => { + signal.aborted = true; + signal.reason = abortError; + return Promise.reject( abortError ); + }, + } ) + ); + + await expect( + nativeMediaUploadMiddleware( options, next ) + ).rejects.toBe( abortError ); + expect( next ).not.toHaveBeenCalled(); + } ); + + it( 'normalizes a transport failure to api-fetch’s error shape without retrying', async () => { getGBKit.mockReturnValue( { nativeUploadPort: 8080, nativeUploadToken: 'token', @@ -419,18 +494,51 @@ describe( 'nativeMediaUploadMiddleware', () => { const connectionError = new TypeError( 'Failed to fetch' ); global.fetch = vi.fn( () => Promise.reject( connectionError ) ); - await expect( - nativeMediaUploadMiddleware( - makePostMediaOptions( makeFile() ), - next - ) - ).rejects.toBe( connectionError ); + const error = await nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + next + ).catch( ( e ) => e ); + + // Normalized to api-fetch's { code, message } shape (online → fetch_error), + // not surfaced as the raw code-less TypeError, so consumers keying off + // error.code behave the same as for a direct upload. + expect( error ).not.toBe( connectionError ); + expect( error.code ).toBe( 'fetch_error' ); + expect( typeof error.message ).toBe( 'string' ); + expect( error.message.length ).toBeGreaterThan( 0 ); // No silent fallback to a direct re-upload — retrying a non-idempotent // POST could duplicate the attachment. expect( next ).not.toHaveBeenCalled(); } ); + it( 'normalizes an offline transport failure to offline_error', async () => { + getGBKit.mockReturnValue( { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + } ); + const next = makeNext(); + + const onLineSpy = vi + .spyOn( globalThis.navigator, 'onLine', 'get' ) + .mockReturnValue( false ); + try { + global.fetch = vi.fn( () => + Promise.reject( new TypeError( 'Failed to fetch' ) ) + ); + + const error = await nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + next + ).catch( ( e ) => e ); + + expect( error.code ).toBe( 'offline_error' ); + expect( next ).not.toHaveBeenCalled(); + } finally { + onLineSpy.mockRestore(); + } + } ); + it( 'propagates an abort instead of falling back', async () => { getGBKit.mockReturnValue( { nativeUploadPort: 8080, @@ -438,16 +546,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 +577,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 +599,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..5f8885e4d 100644 --- a/src/utils/api-fetch.js +++ b/src/utils/api-fetch.js @@ -3,6 +3,7 @@ */ import apiFetch from '@wordpress/api-fetch'; import { getQueryArg } from '@wordpress/url'; +import { __ } from '@wordpress/i18n'; /** * Internal dependencies @@ -194,8 +195,13 @@ export function nativeMediaUploadMiddleware( options, next ) { return next( options ); } + // Only intercept a genuine file upload. `FormData.get('file')` returns a + // `File`, a string (a non-file field that happens to be named `file`), or + // `null` (no such field). The `instanceof File` check covers all the + // non-file cases at once — a missing field and a wrong-typed value both fall + // through to the default path — and guarantees `file.name` below is safe. const file = options.body.get( 'file' ); - if ( ! file ) { + if ( ! ( file instanceof File ) ) { return next( options ); } @@ -230,9 +236,22 @@ export function nativeMediaUploadMiddleware( options, next ) { if ( ! response.ok ) { return response .json() - .catch( invalidUploadResponseError ) + .catch( () => { + // An abort during the body read rejects json() too; surface + // the cancellation, not an "invalid response" error. + if ( options.signal?.aborted ) { + throw uploadAbortError( options.signal ); + } + return invalidUploadResponseError(); + } ) .then( ( body ) => { logError( 'Native upload failed', body ); + // Throw the parsed body verbatim, even if it isn't the usual + // WordPress `{ code, message, data }` shape. This is + // deliberate: it mirrors `@wordpress/api-fetch`'s + // `parseAndThrowError`, so a native-relayed error reaches + // consumers identically to a direct upload's. We intentionally + // don't reshape or second-guess a non-standard error body. throw body; } ); } @@ -240,6 +259,11 @@ export function nativeMediaUploadMiddleware( options, next ) { // intermediary) rejects json(); normalize it the same way as the // non-ok path rather than surfacing a raw SyntaxError. return response.json().catch( () => { + // An abort during the body read rejects json(); surface the + // cancellation rather than an "invalid response" error notice. + if ( options.signal?.aborted ) { + throw uploadAbortError( options.signal ); + } const error = invalidUploadResponseError(); logError( 'Native upload returned an invalid response', error ); throw error; @@ -258,7 +282,7 @@ 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; + throw uploadAbortError( options.signal ); } // Otherwise the loopback upload server is unreachable at the transport // layer. We deliberately do NOT fall back to a direct re-upload: @@ -273,7 +297,27 @@ export function nativeMediaUploadMiddleware( options, next ) { 'Native upload failed at the transport layer', connectionError ); - throw connectionError; + // Normalize to the same `{ code, message }` shape + // `@wordpress/api-fetch`'s default handler produces for a failed fetch, + // so a native-upload transport failure surfaces to consumers (which key + // off `error.code` and show `error.message`) exactly like a direct + // upload's would — not as a raw, code-less TypeError with an + // untranslated message. Same codes and strings as api-fetch, so the + // existing translations apply. + if ( ! globalThis.navigator.onLine ) { + throw { + code: 'offline_error', + message: __( + 'Unable to connect. Please check your Internet connection.' + ), + }; + } + throw { + code: 'fetch_error', + message: __( + 'Could not get a valid response from the server.' + ), + }; } ); } @@ -314,6 +358,26 @@ function invalidUploadResponseError() { }; } +/** + * The error to surface for a cancelled upload. + * + * Returns the signal's `reason` (the canonical abort error), falling back to a + * canonical `AbortError` for engines that abort without populating `reason`. + * Callers gate this behind `signal.aborted` (the cancellation *state*) rather + * than an error's `name`, so a body-read rejection or a network error that + * races the abort still surfaces as a silent cancel — not a spurious failure + * notice. + * + * @param {AbortSignal} signal The aborted signal. + * @return {Error} The error representing the cancellation. + */ +function uploadAbortError( signal ) { + return ( + signal.reason ?? + new DOMException( 'The upload was aborted.', 'AbortError' ) + ); +} + /** * Middleware to modify media upload requests. *