diff --git a/Package.resolved b/Package.resolved index aacd18e8c..2ff636af3 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "6db3023106dfc39818a2a045dfbd8be56ad662c039842cf91e1f21a9bd7ce81f", + "originHash" : "c6b2e9abe520d144490a97766c0d11f9249bd0d35e0e6f16609262f5e91acea0", "pins" : [ { "identity" : "svgview", @@ -18,15 +18,6 @@ "revision" : "aa85ee96017a730031bafe411cde24a08a17a9c9", "version" : "2.8.8" } - }, - { - "identity" : "wordpress-rs", - "kind" : "remoteSourceControl", - "location" : "https://github.com/Automattic/wordpress-rs", - "state" : { - "branch" : "alpha-20260313", - "revision" : "cde2fda82257f4ac7b81543d5b831bb267d4e52c" - } } ], "version" : 3 diff --git a/android/Gutenberg/src/androidTest/java/org/wordpress/gutenberg/http/InstrumentedFixtureTests.kt b/android/Gutenberg/src/androidTest/java/org/wordpress/gutenberg/http/InstrumentedFixtureTests.kt index b3a558bda..ae739cedc 100644 --- a/android/Gutenberg/src/androidTest/java/org/wordpress/gutenberg/http/InstrumentedFixtureTests.kt +++ b/android/Gutenberg/src/androidTest/java/org/wordpress/gutenberg/http/InstrumentedFixtureTests.kt @@ -160,6 +160,11 @@ class InstrumentedFixtureTests { pendingError.errorId, "$description: expected $expectedError but got ${pendingError.errorId}" ) + assertEquals( + HTTPRequestParseError.Disposition.RECOVERABLE, + pendingError.disposition, + "$description: ${pendingError.errorId} surfaced via pendingParseError but is not RECOVERABLE" + ) } else { fail("$description: expected error $expectedError but parsing succeeded") } @@ -169,6 +174,11 @@ class InstrumentedFixtureTests { e.error.errorId, "$description: expected $expectedError but got ${e.error.errorId}" ) + assertEquals( + HTTPRequestParseError.Disposition.FATAL, + e.error.disposition, + "$description: ${e.error.errorId} was thrown but is not FATAL" + ) } } } 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 9756c943f..47b1fe28a 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -12,6 +12,7 @@ import android.net.Uri import android.os.Bundle import android.os.Handler import android.os.Looper +import android.security.NetworkSecurityPolicy import android.util.Log import android.view.Gravity import android.view.inputmethod.InputMethodManager @@ -119,16 +120,49 @@ class GutenbergView : FrameLayout { // Stop any previously running server before starting a new one. uploadServer?.stop() uploadServer = null - // (Re)start the upload server so it captures the delegate. - // This handles the common case where the delegate is set after + // (Re)start the upload server so it captures the delegate. This + // handles the common case where the delegate is set after // construction but before the editor finishes loading. if (value != null) { startUploadServer() } + // Reflect the resulting server state in the page: advertise a freshly + // (re)started server, or clear the port when the server did NOT + // (re)start (delegate cleared, auth missing, or cleartext-to-localhost + // blocked) so the JS upload middleware routes to the default path + // instead of fetching a now-dead port. + syncUploadServerJavaScriptVariables() } private var uploadServer: MediaUploadServer? = null - private val uploadHttpClient: okhttp3.OkHttpClient by lazy { okhttp3.OkHttpClient() } + private val uploadHttpClient: okhttp3.OkHttpClient by lazy { + // The read/write inactivity timeouts mirror URLSession's 60s + // timeoutIntervalForRequest default — an inactivity timer that resets on + // progress — so the iOS and Android upload clients behave the same. Like + // URLSession, which governs uploads with that inactivity timer rather than a + // total-duration cap, there is deliberately NO callTimeout here (unlike the + // sibling EditorHTTPClient, which caps total call duration — fine for small + // REST payloads, wrong for uploads): because each timeout resets on progress, + // a large upload is never failed on total duration, only on a genuine 60s + // stall in the active direction. + // + // - writeTimeout (upload): a transient loss of connectivity during the upload + // fails within ~60s so it surfaces and can be retried, rather than hanging. + // - readTimeout (download): gives WordPress time to generate image sub-sizes + // synchronously inside POST /wp/v2/media, during which it sends no response + // bytes. The bare OkHttpClient() 10s default fired mid-resize — and since the + // attachment row exists before resizing finishes, that orphaned it + // server-side and duplicated it on retry. + // - connectTimeout: a much shorter 15s — establishing a socket should be + // quick, so this fails fast on an unreachable host instead of making the + // user wait out the full window. (URLSession has no separate connect dial; + // it folds connection setup into the same 60s request timer.) + okhttp3.OkHttpClient.Builder() + .connectTimeout(CONNECT_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS) + .readTimeout(UPLOAD_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS) + .writeTimeout(UPLOAD_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS) + .build() + } private var onFileChooserRequested: ((Intent, Int) -> Unit)? = null private var contentChangeListener: ContentChangeListener? = null @@ -613,10 +647,61 @@ class GutenbergView : FrameLayout { webView.evaluateJavascript(gbKitConfig, null) } + /** + * Syncs the current upload server's port and token into the already-loaded + * page. + * + * Advertises a running server so JS uploads route through it, and clears them + * (to `null`) when the server is stopped or was never started — so the JS + * upload middleware routes to the default path instead of fetching a now-dead + * port. The initial injection is handled by [setGlobalJavaScriptVariables] + * from `onPageStarted`; this keeps JS in sync when the server (re)starts or + * stops *after* the page has loaded (e.g. when [mediaUploadDelegate] is + * assigned, replaced, or cleared). + * + * The `window.GBKit` guard makes this a no-op before the page has loaded, so + * it is safe to call on the initial start too. + */ + private fun syncUploadServerJavaScriptVariables() { + val portJs = uploadServer?.port?.toString() ?: "null" + val tokenJs = uploadServer?.token?.let { JSONObject.quote(it) } ?: "null" + val js = """ + if (window.GBKit) { + window.GBKit.nativeUploadPort = $portJs; + window.GBKit.nativeUploadToken = $tokenJs; + localStorage.setItem('GBKit', JSON.stringify(window.GBKit)); + } + """.trimIndent() + // evaluateJavascript must run on the WebView's (UI) thread; post it so a + // delegate set from a background thread doesn't throw thread-affinity. + webView.post { webView.evaluateJavascript(js, null) } + } private fun startUploadServer() { + // The native upload server relays through DefaultMediaUploader, which needs a + // site root and an auth header (every host provides one — the editor injects + // it because the WebView has no auth cookies). Without both there is nothing + // to upload through, so leave the server down and let uploads fall to the + // default WebView path rather than start a server that could only fail. if (configuration.siteApiRoot.isEmpty() || configuration.authHeader.isEmpty()) return + // The editor reaches the loopback server over cleartext http://localhost. If + // the host app's network-security config doesn't permit cleartext to + // localhost, the WebView blocks every upload fetch (ERR_CLEARTEXT_NOT_PERMITTED) + // before it leaves the page. Detect that here and don't start the server, so + // the JS middleware routes uploads down the default path instead of a server + // it can never reach. Hosts that want native media processing must permit + // cleartext to localhost (see the demo's res/xml/network_security_config.xml). + if (!NetworkSecurityPolicy.getInstance().isCleartextTrafficPermitted(LOOPBACK_HOST)) { + Log.w( + TAG, + "Cleartext to $LOOPBACK_HOST is not permitted, so the native media upload " + + "server can't be reached from the WebView. Permit cleartext to $LOOPBACK_HOST " + + "in the app's network security config to enable native media processing." + ) + return + } + try { val defaultUploader = DefaultMediaUploader( httpClient = uploadHttpClient, @@ -627,8 +712,10 @@ class GutenbergView : FrameLayout { uploadServer = MediaUploadServer( uploadDelegate = mediaUploadDelegate, defaultUploader = defaultUploader, - cacheDir = context.cacheDir + cacheDir = context.cacheDir, + scope = coroutineScope ) + // JS is synced by the mediaUploadDelegate setter after this returns. } catch (e: Exception) { Log.w(TAG, "Failed to start upload server", e) } @@ -1144,6 +1231,18 @@ class GutenbergView : FrameLayout { private const val ASSET_LOADING_TIMEOUT_MS = 5000L + /** + * Read/write inactivity timeout for media uploads, matching URLSession's + * 60s `timeoutIntervalForRequest` default. See [uploadHttpClient]. + */ + private const val UPLOAD_TIMEOUT_SECONDS = 60L + + /** Connection-setup timeout for media uploads — short, to fail fast on an unreachable host. */ + private const val CONNECT_TIMEOUT_SECONDS = 15L + + /** Host the WebView uses to reach the loopback upload server (must match the JS fetch host). */ + private const val LOOPBACK_HOST = "localhost" + // Warmup state management private var warmupHandler: Handler? = null private var warmupRunnable: Runnable? = null 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 190f4cde7..8d0885539 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt @@ -45,6 +45,23 @@ data class HttpRequest( * for building an appropriate error response. */ val serverError: org.wordpress.gutenberg.http.HTTPRequestParseError? = null ) { + /** + * The path portion of [target], without the query component + * (e.g., "/wp/v2/posts" for "/wp/v2/posts?per_page=10"). + * + * Use this for routing — matching against [target] fails as soon as a + * client appends a query string. + */ + val path: String + get() = target.substringBefore('?') + + /** + * The query component of [target], including the leading "?" + * (e.g., "?per_page=10"), or an empty string when there is no query. + */ + val query: String + get() = target.substringAfter('?', "").let { if (it.isEmpty()) "" else "?$it" } + /** * Returns the value of the first header matching the given name (case-insensitive). */ @@ -71,6 +88,45 @@ data class HttpResponse( val body: ByteArray = ByteArray(0) ) +/** CORS behavior for an [HttpServer]. */ +enum class CorsPolicy { + /** No CORS headers are added (the default). */ + None, + + /** + * Permissive CORS for a loopback-only server serving a WebView: allows any + * origin and the methods/headers this library's clients use. The server + * answers OPTIONS preflight requests itself and stamps these headers on every + * response — including ones it generates internally (timeouts, parse errors) + * that never reach the handler. + */ + Permissive; + + /** Headers added to every response under this policy. */ + val responseHeaders: Map + get() = when (this) { + None -> emptyMap() + Permissive -> mapOf( + "Access-Control-Allow-Origin" to "*", + "Access-Control-Allow-Methods" to "GET, POST, PUT, DELETE, OPTIONS", + "Access-Control-Allow-Headers" to "Authorization, Relay-Authorization, Content-Type", + "Access-Control-Max-Age" to "86400" + ) + } +} + +/** + * Returns a copy with [newHeaders] added, skipping any whose name + * (case-insensitive) is already present. + */ +private fun HttpResponse.addingHeadersIfAbsent(newHeaders: Map): HttpResponse { + if (newHeaders.isEmpty()) return this + val existing = headers.keys.map { it.lowercase() }.toSet() + val toAdd = newHeaders.filterKeys { it.lowercase() !in existing } + if (toAdd.isEmpty()) return this + return copy(headers = headers + toAdd) +} + /** * A lightweight local HTTP/1.1 server. * @@ -144,6 +200,7 @@ data class HttpResponse( * server.stop() * ``` */ +@Suppress("LongParameterList") class HttpServer( val name: String, private val requestedPort: Int = 0, @@ -154,6 +211,7 @@ class HttpServer( private val readTimeoutMs: Int = DEFAULT_READ_TIMEOUT_MS, private val idleTimeoutMs: Int = DEFAULT_IDLE_TIMEOUT_MS, private val cacheDir: File? = null, + private val cors: CorsPolicy = CorsPolicy.None, private val handler: suspend (HttpRequest) -> HttpResponse ) { @Volatile @@ -412,15 +470,7 @@ class HttpServer( body = parsed.body, parseDurationMs = parseDurationMs ) - val response = try { - handler(request) - } catch (e: Exception) { - Log.e(TAG, "Handler threw", e) - HttpResponse( - status = 500, - body = "Internal Server Error".toByteArray() - ) - } + val response = resolveResponse(request) sendResponse(socket, response) Log.d(TAG, "${parsed.method} ${parsed.target} → ${response.status} (${"%.1f".format(parseDurationMs)}ms)") } @@ -445,9 +495,27 @@ class HttpServer( } } + /** + * Resolves the response for a request: the CORS preflight (under a permissive + * policy) or the handler's response. Kept separate from [handleRequest] so + * that already-complex function doesn't grow. + */ + private suspend fun resolveResponse(request: HttpRequest): HttpResponse { + if (cors == CorsPolicy.Permissive && request.method.uppercase() == "OPTIONS") { + return HttpResponse(status = 204, body = ByteArray(0)) + } + return try { + handler(request) + } catch (e: Exception) { + Log.e(TAG, "Handler threw", e) + HttpResponse(status = 500, body = "Internal Server Error".toByteArray()) + } + } + private fun sendResponse(socket: Socket, response: HttpResponse) { + val decorated = response.addingHeadersIfAbsent(cors.responseHeaders) val output = socket.getOutputStream() - output.write(serializeResponse(response)) + output.write(serializeResponse(decorated)) output.flush() } 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 c264644bc..c289866a8 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -1,6 +1,11 @@ package org.wordpress.gutenberg import android.util.Log +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job +import kotlinx.coroutines.launch import org.wordpress.gutenberg.http.HeaderValue import org.wordpress.gutenberg.http.MultipartPart import org.wordpress.gutenberg.http.HTTPRequestParseError @@ -10,25 +15,44 @@ import java.io.IOException import java.util.UUID import okhttp3.MediaType.Companion.toMediaType import okhttp3.RequestBody.Companion.asRequestBody +import okhttp3.RequestBody.Companion.toRequestBody import okio.source /** - * Result of a successful media upload to the remote WordPress server. + * A raw response from the WordPress REST API media endpoint. * - * Matches the format expected by Gutenberg's `onFileChange` callback. + * GutenbergKit relays this to the editor verbatim — it does not interpret the + * body. The editor receives the exact attachment object (on success) or + * WordPress REST error object (on failure) it would get from a direct upload, + * so every consumer — image sub-sizes, attachment links, error notices — + * behaves identically to a non-native upload. */ -data class MediaUploadResult( - val id: Int, - val url: String, - val alt: String = "", - val caption: String = "", - val title: String, - val mime: String, - val type: String, - val width: Int? = null, - val height: Int? = null +class MediaUploadResponse( + /** The HTTP status code WordPress (or the host's upload service) returned. */ + val statusCode: Int, + /** + * The raw response body — a WordPress REST attachment on success, or a + * WordPress REST error object (`{ "code", "message", "data" }`) on failure. + */ + val body: ByteArray ) +/** + * The result of a delegate's [MediaUploadDelegate.processFile]. + */ +sealed class ProcessedProxyFile { + /** The delegate did not modify the file; the original upload is forwarded unchanged. */ + data object Original : ProcessedProxyFile() + + /** + * The delegate produced a file to upload, along with its MIME type and + * filename. Both are used verbatim, so a format change (e.g. transcoding MOV + * to MP4, or an in-place EXIF strip) must report the resulting type and + * filename for WordPress to store the file correctly. + */ + data class Processed(val file: File, val mimeType: String, val filename: String) : ProcessedProxyFile() +} + /** * Interface for customizing media upload behavior. * @@ -38,15 +62,23 @@ data class MediaUploadResult( interface MediaUploadDelegate { /** * Process a file before upload (e.g., resize image, transcode video). - * Return the path of the processed file, or the original path for passthrough. + * + * Return [ProcessedProxyFile.Original] to upload the file unchanged, or + * [ProcessedProxyFile.Processed] with the processed file and its metadata. + * When the format changes, report the new mimeType and filename so WordPress + * stores it with the correct extension and type. */ - suspend fun processFile(file: File, mimeType: String): File = file + suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile = ProcessedProxyFile.Original /** * Upload a processed file to the remote WordPress site. - * Return the Gutenberg-compatible media result, or null to use the default uploader. + * + * Return the raw WordPress response (status code + body), which GutenbergKit + * relays to the editor unchanged, or null to use the default uploader. A host + * that uploads to WordPress should return the exact response it received so + * the editor sees a complete attachment object. */ - suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResult? = null + suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null } /** @@ -64,7 +96,9 @@ interface MediaUploadDelegate { internal class MediaUploadServer( private val uploadDelegate: MediaUploadDelegate?, private val defaultUploader: DefaultMediaUploader?, - cacheDir: File? = null + cacheDir: File? = null, + scope: CoroutineScope = CoroutineScope(Dispatchers.IO), + ioDispatcher: CoroutineDispatcher = Dispatchers.IO ) { /** The port the server is listening on. */ val port: Int get() = server.port @@ -74,12 +108,34 @@ internal class MediaUploadServer( private val server: HttpServer + /** + * Directory for staging uploaded files, under the injected cache dir (with a + * system-temp fallback) so orphans share the app's managed cache lifecycle. + */ + private val uploadsTempDir: File = + File(cacheDir ?: File(System.getProperty("java.io.tmpdir")), "gutenbergkit-uploads") + + /** + * 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) { + try { + cleanOrphanedUploads() + } catch (e: Exception) { + Log.w(TAG, "Failed to sweep orphaned uploads", e) + } + } + init { server = HttpServer( name = "media-upload", externallyAccessible = false, requiresAuthentication = true, cacheDir = cacheDir, + cors = CorsPolicy.Permissive, handler = { request -> handleRequest(request) } ) server.start() @@ -87,9 +143,24 @@ internal class MediaUploadServer( /** Stops the server and releases resources. */ fun stop() { + cleanupJob.cancel() server.stop() } + /** + * Deletes upload temp files left behind by a prior crash. Files still in + * flight (only seconds old) are preserved by the age threshold, so this is + * safe even if another editor instance is mid-upload. + */ + private fun cleanOrphanedUploads() { + val cutoff = System.currentTimeMillis() - 60 * 60 * 1000L // 1 hour + uploadsTempDir.listFiles()?.forEach { file -> + if (file.lastModified() < cutoff) { + file.delete() + } + } + } + // MARK: - Request Handling private suspend fun handleRequest(request: HttpRequest): HttpResponse { @@ -103,14 +174,11 @@ internal class MediaUploadServer( return errorResponse(error.httpStatus, message) } - // CORS preflight — the library exempts OPTIONS from auth, so this is - // reached without a token. - if (request.method.uppercase() == "OPTIONS") { - return corsPreflightResponse() - } - - // Route: only POST /upload is handled. - if (request.method.uppercase() != "POST" || request.target != "/upload") { + // 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 + // upload handler relays on to WordPress. + if (request.method.uppercase() != "POST" || request.path != "/upload") { return errorResponse(404, "Not found") } @@ -118,21 +186,28 @@ internal class MediaUploadServer( } private suspend fun handleUpload(request: HttpRequest): HttpResponse { - val filePart = parseFilePart(request) + val parts = parseParts(request) ?: return errorResponse(400, "Expected multipart/form-data with a file") + val filePart = parts.firstOrNull { it.filename != null } + ?: return errorResponse(400, "Expected multipart/form-data with a file") + + // The non-file parts (post, additionalData) and the original query + // (e.g. ?_embed) must reach WordPress too — relay them alongside the file. + val extraParts = parts.filter { it.filename == null } + val query = request.query val tempFile = writePartToTempFile(filePart) ?: return errorResponse(500, "Failed to save file") - return processAndRespond(request, tempFile, filePart) + return processAndRespond(request, tempFile, filePart, extraParts, query) } - private fun parseFilePart(request: HttpRequest): MultipartPart? { + private fun parseParts(request: HttpRequest): List? { val contentType = request.header("Content-Type") ?: return null val boundary = HeaderValue.extractParameter("boundary", contentType) ?: return null val body = request.body ?: return null - val parts = try { + return try { val inMemory = body.inMemoryData if (inMemory != null) { MultipartPart.parse(body, inMemory, 0L, boundary) @@ -145,16 +220,14 @@ internal class MediaUploadServer( } } catch (e: MultipartParseException) { Log.e(TAG, "Multipart parse failed", e) - return null + null } - - return parts.firstOrNull { it.filename != null } } private fun writePartToTempFile(filePart: MultipartPart): File? { val filename = sanitizeFilename(filePart.filename ?: "upload") - val tempDir = File(System.getProperty("java.io.tmpdir"), "gutenbergkit-uploads").apply { mkdirs() } - val tempFile = File(tempDir, "${UUID.randomUUID()}-$filename") + uploadsTempDir.mkdirs() + val tempFile = File(uploadsTempDir, "${UUID.randomUUID()}-$filename") return try { filePart.body.inputStream().use { input -> @@ -164,122 +237,141 @@ internal class MediaUploadServer( } tempFile } catch (e: IOException) { + tempFile.delete() Log.e(TAG, "Failed to write upload to disk", e) null } } + @Suppress("TooGenericExceptionCaught") private suspend fun processAndRespond( - request: HttpRequest, tempFile: File, filePart: MultipartPart + request: HttpRequest, tempFile: File, filePart: MultipartPart, + extraParts: List, query: String ): HttpResponse { - var processedFile: File? = null try { val uploadResult = processAndUpload( - tempFile, filePart.contentType, filePart.filename ?: "upload" + tempFile, filePart.contentType, filePart.filename ?: "upload", extraParts, query ) - val media = when (uploadResult) { + val response = when (uploadResult) { is UploadResult.Uploaded -> { - processedFile = uploadResult.processedFile - Log.d(TAG, "Uploading processed file to WordPress") - uploadResult.result + Log.d(TAG, "Uploaded file to WordPress") + uploadResult.response } is UploadResult.Passthrough -> { // Delegate didn't modify the file — forward the original // request body to WordPress without re-encoding. Log.d(TAG, "Passthrough: forwarding original request body to WordPress") - performPassthroughUpload(request) + performPassthroughUpload(request, query) } } - return successResponse(media) + // 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 + ) } catch (e: MediaUploadException) { Log.e(TAG, "Upload processing failed", e) return errorResponse(500, e.message ?: "Upload failed") + } catch (e: kotlin.coroutines.cancellation.CancellationException) { + throw e // Never swallow coroutine cancellation. + } catch (e: Exception) { + // Any other failure — IOException from the upload call, JSON parse + // errors, a throwing host delegate, or "no uploader configured" — + // must still be answered WITH CORS headers. Otherwise it escapes to + // HttpServer's header-less 500 fallback and the browser rejects the + // preflighted cross-origin fetch with an opaque "Failed to fetch", + // hiding the real error from the editor (mirrors the iOS catch-all). + Log.e(TAG, "Upload failed", e) + return errorResponse(500, e.message ?: "Upload failed") } finally { tempFile.delete() - processedFile?.let { if (it != tempFile) it.delete() } } } // MARK: - Delegate Pipeline private sealed class UploadResult { - data class Uploaded(val result: MediaUploadResult, val processedFile: File) : UploadResult() + data class Uploaded(val response: MediaUploadResponse) : UploadResult() data object Passthrough : UploadResult() } - private suspend fun performPassthroughUpload(request: HttpRequest): MediaUploadResult { + private suspend fun performPassthroughUpload(request: HttpRequest, query: String): MediaUploadResponse { val body = request.body val contentType = request.header("Content-Type") val uploader = defaultUploader if (body == null || contentType == null || uploader == null) { throw MediaUploadException("Passthrough upload requires a request body, Content-Type, and default uploader") } - return uploader.passthroughUpload(body, contentType) + return uploader.passthroughUpload(body, contentType, query) } private suspend fun processAndUpload( - file: File, mimeType: String, filename: String + file: File, mimeType: String, filename: String, + extraParts: List, query: String ): UploadResult { - val processedFile = uploadDelegate?.processFile(file, mimeType) ?: file - - // If the delegate provided its own upload, use that. - uploadDelegate?.uploadFile(processedFile, mimeType, filename)?.let { - return UploadResult.Uploaded(it, processedFile) + val processed = uploadDelegate?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original + + // Resolve the file to upload and its metadata. Processed uses the + // delegate's values verbatim, so a format change is reported to WordPress. + val targetFile: File + val targetMimeType: String + val targetFilename: String + when (processed) { + is ProcessedProxyFile.Original -> { + targetFile = file + targetMimeType = mimeType + targetFilename = filename + } + is ProcessedProxyFile.Processed -> { + targetFile = processed.file + targetMimeType = processed.mimeType + targetFilename = processed.filename + } } - // If the delegate didn't modify the file, the original request - // body can be forwarded directly — skip multipart re-encoding. - if (processedFile == file) { - return UploadResult.Passthrough - } + try { + // If the delegate provided its own upload, use that. + uploadDelegate?.uploadFile(targetFile, targetMimeType, targetFilename)?.let { + return UploadResult.Uploaded(it) + } - val result = defaultUploader?.upload(processedFile, mimeType, filename) - ?: error("No upload delegate or default uploader configured") - return UploadResult.Uploaded(result, processedFile) + // Unmodified — forward the original request body directly, skipping + // multipart re-encoding. + if (processed is ProcessedProxyFile.Original) { + return UploadResult.Passthrough + } + + val result = defaultUploader?.upload(targetFile, targetMimeType, targetFilename, extraParts, query) + ?: error("No upload delegate or default uploader configured") + return UploadResult.Uploaded(result) + } finally { + // The processed file (if the delegate produced a new one) is ours to + // clean up — covers the success and throw paths alike. + if (targetFile != file) { + targetFile.delete() + } + } } // MARK: - Response Building - private val corsHeaders: Map = mapOf( - "Access-Control-Allow-Origin" to "*", - "Access-Control-Allow-Headers" to "Relay-Authorization, Content-Type" - ) - - private fun corsPreflightResponse(): HttpResponse = HttpResponse( - status = 204, - headers = corsHeaders + mapOf( - "Access-Control-Allow-Methods" to "POST, OPTIONS", - "Access-Control-Max-Age" to "86400" - ), - body = ByteArray(0) - ) - - private fun successResponse(media: MediaUploadResult): HttpResponse { - val json = org.json.JSONObject().apply { - put("id", media.id) - put("url", media.url) - put("alt", media.alt) - put("caption", media.caption) - put("title", media.title) - put("mime", media.mime) - put("type", media.type) - media.width?.let { put("width", it) } - media.height?.let { put("height", it) } - }.toString() - + private fun errorResponse(status: Int, message: String): HttpResponse { + // Emit a WordPress-REST-style error object so the JS middleware normalizes + // it (and surfaces `message`) the same way it does a relayed WordPress + // error — the local server's own errors need no special-casing. + val json = org.json.JSONObject() + .put("code", "upload_error") + .put("message", message) + .toString() return HttpResponse( - status = 200, - headers = corsHeaders + mapOf("Content-Type" to "application/json"), + status = status, + headers = mapOf("Content-Type" to "application/json"), body = json.toByteArray() ) } - private fun errorResponse(status: Int, body: String): HttpResponse = HttpResponse( - status = status, - headers = corsHeaders + mapOf("Content-Type" to "text/plain"), - body = body.toByteArray() - ) - // MARK: - Helpers /** Sanitizes a filename to prevent path traversal. */ @@ -305,24 +397,33 @@ internal open class DefaultMediaUploader( private val authHeader: String, private val siteApiNamespace: List = emptyList() ) { - /** The WordPress media endpoint URL, accounting for site API namespaces. */ - private val mediaEndpointUrl: String - get() { - val namespace = siteApiNamespace.firstOrNull() ?: "" - return "${siteApiRoot}wp/v2/${namespace}media" - } + /** + * The WordPress media endpoint URL, built through the shared [RestUrlBuilder] + * namespacing (so it matches every other REST URL) and carrying the original + * request query (e.g. `?_embed`) through to WordPress. + */ + private fun mediaEndpointUrl(query: String): String = + RestUrlBuilder.namespaced(siteApiRoot, siteApiNamespace.firstOrNull(), "/wp/v2/media") + query - open suspend fun upload(file: File, mimeType: String, filename: String): MediaUploadResult { + open suspend fun upload( + file: File, mimeType: String, filename: String, + extraParts: List, query: String + ): MediaUploadResponse { val mediaType = mimeType.toMediaType() - val requestBody = okhttp3.MultipartBody.Builder() - .setType(okhttp3.MultipartBody.FORM) - .addFormDataPart("file", filename, file.asRequestBody(mediaType)) - .build() + val builder = okhttp3.MultipartBody.Builder().setType(okhttp3.MultipartBody.FORM) + // Preserve the non-file parts (post, additionalData) through the re-encode. + // Append each field's raw bytes (not via String) so a non-UTF-8 value is + // forwarded verbatim rather than coerced. filename=null makes it a plain + // field, matching okhttp's String overload byte-for-byte. + for (part in extraParts) { + builder.addFormDataPart(part.name, null, part.body.readBytes().toRequestBody()) + } + builder.addFormDataPart("file", filename, file.asRequestBody(mediaType)) val request = okhttp3.Request.Builder() - .url(mediaEndpointUrl) + .url(mediaEndpointUrl(query)) .addHeader("Authorization", authHeader) - .post(requestBody) + .post(builder.build()) .build() return performUpload(request) @@ -336,8 +437,9 @@ internal open class DefaultMediaUploader( */ open suspend fun passthroughUpload( body: org.wordpress.gutenberg.http.RequestBody, - contentType: String - ): MediaUploadResult { + contentType: String, + query: String + ): MediaUploadResponse { val streamBody = object : okhttp3.RequestBody() { override fun contentType() = contentType.toMediaType() override fun contentLength() = body.size @@ -347,7 +449,7 @@ internal open class DefaultMediaUploader( } val request = okhttp3.Request.Builder() - .url(mediaEndpointUrl) + .url(mediaEndpointUrl(query)) .addHeader("Authorization", authHeader) .post(streamBody) .build() @@ -355,44 +457,12 @@ internal open class DefaultMediaUploader( return performUpload(request) } - private fun performUpload(request: okhttp3.Request): MediaUploadResult { - val response = httpClient.newCall(request).execute() - val body = response.body?.string() - - if (!response.isSuccessful) { - // Try to extract the human-readable message from a WordPress error - // response ({"code":"...","message":"..."}) before falling back to - // the raw body. - val errorMessage = body?.let { - try { org.json.JSONObject(it).optString("message", null) } catch (_: org.json.JSONException) { null } - } ?: body ?: response.message - throw MediaUploadException(errorMessage) + private 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)) } - - if (body == null) { - throw MediaUploadException("Empty response body from server") - } - - return parseMediaResponse(body) - } - - private fun parseMediaResponse(body: String): MediaUploadResult { - val json = try { - org.json.JSONObject(body) - } catch (e: org.json.JSONException) { - throw MediaUploadException("Unexpected response: ${body.take(500)}", e) - } - val mediaDetails = json.optJSONObject("media_details") - return MediaUploadResult( - id = json.getInt("id"), - url = json.getString("source_url"), - alt = json.optString("alt_text", ""), - caption = json.optJSONObject("caption")?.optString("rendered", "") ?: "", - title = json.getJSONObject("title").getString("rendered"), - mime = json.getString("mime_type"), - type = json.getString("media_type"), - width = mediaDetails?.optInt("width"), - height = mediaDetails?.optInt("height") - ) } } diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/RESTAPIRepository.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/RESTAPIRepository.kt index 692ca79ca..14375f39b 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/RESTAPIRepository.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/RESTAPIRepository.kt @@ -23,10 +23,6 @@ class RESTAPIRepository( ) { private val json = Json { ignoreUnknownKeys = true } - private val apiRoot = configuration.siteApiRoot.trimEnd('/') - private val namespace = configuration.siteApiNamespace.firstOrNull()?.let { - it.trimEnd('/') + "/" - } private val editorSettingsUrl = buildNamespacedUrl(EDITOR_SETTINGS_PATH) private val activeThemeUrl = buildNamespacedUrl(ACTIVE_THEME_PATH) private val siteSettingsUrl = buildNamespacedUrl(SITE_SETTINGS_PATH) @@ -217,25 +213,13 @@ class RESTAPIRepository( return urlResponse } - /** - * Builds a URL from the API root and path, inserting the site API namespace - * after the version segment if one is configured. - * - * For example, with namespace `sites/123/` and path `/wp/v2/types`: - * the result is `$apiRoot/wp/v2/sites/123/types`. - */ - private fun buildNamespacedUrl(path: String): String { - if (namespace == null) { - return "$apiRoot$path" - } - - val parts = path.removePrefix("/").split("/", limit = 3) - if (parts.size < 3) { - return "$apiRoot$path" - } - - return "$apiRoot/${parts[0]}/${parts[1]}/$namespace${parts[2]}" - } + /** Builds a namespaced REST URL via the shared [RestUrlBuilder]. */ + private fun buildNamespacedUrl(path: String): String = + RestUrlBuilder.namespaced( + configuration.siteApiRoot, + configuration.siteApiNamespace.firstOrNull(), + path + ) companion object { private const val EDITOR_SETTINGS_PATH = "/wp-block-editor/v1/settings" diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/RestUrlBuilder.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/RestUrlBuilder.kt new file mode 100644 index 000000000..068d60141 --- /dev/null +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/RestUrlBuilder.kt @@ -0,0 +1,30 @@ +package org.wordpress.gutenberg + +/** + * Single source of truth for building namespaced WordPress REST API URLs, so the + * media endpoint and every [RESTAPIRepository] endpoint normalize the site API + * root and namespace identically (no drift). + */ +internal object RestUrlBuilder { + /** + * Builds a URL from [siteApiRoot] and [path], inserting [siteApiNamespace] + * after the version segment if one is configured. A `null` namespace appends + * the path unchanged. + * + * Trailing slashes on the root and namespace are normalized, so an unslashed + * root or namespace still joins cleanly. For example, with namespace `sites/123` + * and path `/wp/v2/types`, the result is `$root/wp/v2/sites/123/types`. + */ + fun namespaced(siteApiRoot: String, siteApiNamespace: String?, path: String): String { + val root = siteApiRoot.trimEnd('/') + val namespace = siteApiNamespace?.let { it.trimEnd('/') + "/" } + ?: return "$root$path" + + val parts = path.removePrefix("/").split("/", limit = 3) + if (parts.size < 3) { + return "$root$path" + } + + return "$root/${parts[0]}/${parts[1]}/$namespace${parts[2]}" + } +} 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 0157386e4..004ad53aa 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 @@ -229,11 +229,13 @@ class HTTPRequestParser( fun parseRequest(): ParsedHTTPRequest? = synchronized(lock) { if (!_state.hasHeaders) return null - // Payload-too-large means "valid headers, rejected body" — let - // the caller access the parsed headers so the handler can build - // a response (e.g., with CORS headers). Other parse errors - // indicate genuinely malformed requests and are still thrown. - parseError?.let { if (it != HTTPRequestParseError.PAYLOAD_TOO_LARGE) throw HTTPRequestParseException(it) } + // Recoverable errors (e.g. payloadTooLarge — valid headers, rejected + // body) are surfaced to the caller so the handler can build a response. + // Fatal errors indicate genuinely malformed requests and are thrown, + // closing the connection before the handler runs. + parseError?.let { + if (it.disposition == HTTPRequestParseError.Disposition.FATAL) throw HTTPRequestParseException(it) + } if (parsedHeaders == null) { val headerData = buffer.read(0, minOf(bytesWritten, MAX_HEADER_SIZE.toLong()).toInt()) diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/http/HTTPRequestSerializer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/http/HTTPRequestSerializer.kt index 5a56493ea..be00ffba2 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/http/HTTPRequestSerializer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/http/HTTPRequestSerializer.kt @@ -7,25 +7,43 @@ enum class HTTPRequestParseError( /** The HTTP status code that should be sent for this error. */ val httpStatus: Int, /** A camelCase identifier matching the Swift error case names and JSON fixture keys. */ - val errorId: String + val errorId: String, + /** + * Whether this error aborts the connection ([Disposition.FATAL], thrown so the + * handler never runs) or is surfaced to the handler via `pendingParseError` + * ([Disposition.RECOVERABLE]) so it can build a response. Only genuinely + * recoverable errors — request line and headers well-formed — may be + * RECOVERABLE; anything smuggling-relevant (framing, Content-Length) must stay + * FATAL so the request never reaches the handler. + */ + val disposition: Disposition ) { - EMPTY_HEADER_SECTION(400, "emptyHeaderSection"), - MALFORMED_REQUEST_LINE(400, "malformedRequestLine"), - OBS_FOLD_DETECTED(400, "obsFoldDetected"), - WHITESPACE_BEFORE_COLON(400, "whitespaceBeforeColon"), - INVALID_CONTENT_LENGTH(400, "invalidContentLength"), - CONFLICTING_CONTENT_LENGTH(400, "conflictingContentLength"), - UNSUPPORTED_TRANSFER_ENCODING(400, "unsupportedTransferEncoding"), - INVALID_HTTP_VERSION(400, "invalidHTTPVersion"), - INVALID_FIELD_NAME(400, "invalidFieldName"), - INVALID_FIELD_VALUE(400, "invalidFieldValue"), - MISSING_HOST_HEADER(400, "missingHostHeader"), - MULTIPLE_HOST_HEADERS(400, "multipleHostHeaders"), - PAYLOAD_TOO_LARGE(413, "payloadTooLarge"), - HEADERS_TOO_LARGE(431, "headersTooLarge"), - TOO_MANY_HEADERS(431, "tooManyHeaders"), - INVALID_ENCODING(400, "invalidEncoding"), - BUFFER_IO_ERROR(500, "bufferIOError"); + EMPTY_HEADER_SECTION(400, "emptyHeaderSection", Disposition.FATAL), + MALFORMED_REQUEST_LINE(400, "malformedRequestLine", Disposition.FATAL), + OBS_FOLD_DETECTED(400, "obsFoldDetected", Disposition.FATAL), + WHITESPACE_BEFORE_COLON(400, "whitespaceBeforeColon", Disposition.FATAL), + INVALID_CONTENT_LENGTH(400, "invalidContentLength", Disposition.FATAL), + CONFLICTING_CONTENT_LENGTH(400, "conflictingContentLength", Disposition.FATAL), + UNSUPPORTED_TRANSFER_ENCODING(400, "unsupportedTransferEncoding", Disposition.FATAL), + INVALID_HTTP_VERSION(400, "invalidHTTPVersion", Disposition.FATAL), + INVALID_FIELD_NAME(400, "invalidFieldName", Disposition.FATAL), + INVALID_FIELD_VALUE(400, "invalidFieldValue", Disposition.FATAL), + MISSING_HOST_HEADER(400, "missingHostHeader", Disposition.FATAL), + MULTIPLE_HOST_HEADERS(400, "multipleHostHeaders", Disposition.FATAL), + PAYLOAD_TOO_LARGE(413, "payloadTooLarge", Disposition.RECOVERABLE), + HEADERS_TOO_LARGE(431, "headersTooLarge", Disposition.FATAL), + TOO_MANY_HEADERS(431, "tooManyHeaders", Disposition.FATAL), + INVALID_ENCODING(400, "invalidEncoding", Disposition.FATAL), + BUFFER_IO_ERROR(500, "bufferIOError", Disposition.FATAL); + + /** How the parser disposes of a parse error. */ + enum class Disposition { + /** Abort the connection; the malformed request never reaches the handler. */ + FATAL, + + /** Surface to the handler via `pendingParseError` so it can build a response. */ + RECOVERABLE + } } /** diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpRequestTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpRequestTest.kt new file mode 100644 index 000000000..6aae33ebd --- /dev/null +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpRequestTest.kt @@ -0,0 +1,47 @@ +package org.wordpress.gutenberg + +import org.junit.Assert.assertEquals +import org.junit.Test + +class HttpRequestTest { + + private fun request(target: String) = HttpRequest( + method = "POST", + target = target, + headers = emptyMap() + ) + + @Test + fun `path is the whole target when there is no query`() { + assertEquals("/upload", request("/upload").path) + assertEquals("", request("/upload").query) + } + + @Test + fun `path and query split on the first question mark`() { + val parsed = request("/upload?_embed=wp:featuredmedia") + assertEquals("/upload", parsed.path) + assertEquals("?_embed=wp:featuredmedia", parsed.query) + } + + @Test + fun `a trailing question mark yields an empty query`() { + val parsed = request("/upload?") + assertEquals("/upload", parsed.path) + assertEquals("", parsed.query) + } + + @Test + fun `later question marks belong to the query`() { + val parsed = request("/search?q=a?b") + assertEquals("/search", parsed.path) + assertEquals("?q=a?b", parsed.query) + } + + @Test + fun `multiple query parameters are preserved`() { + val parsed = request("/wp/v2/posts?per_page=10&page=2") + assertEquals("/wp/v2/posts", parsed.path) + assertEquals("?per_page=10&page=2", parsed.query) + } +} 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 86ad0c51d..7ce925f0f 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -1,6 +1,7 @@ package org.wordpress.gutenberg import com.google.gson.JsonParser +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.runBlocking import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer @@ -100,6 +101,38 @@ class MediaUploadServerTest { assertTrue(response.statusLine.contains("404")) } + @Test + fun `routes upload with a query string and relays the query`() { + val delegate = ProcessOnlyDelegate() + val mockUploader = MockDefaultUploader() + server.stop() + server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + + // `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, + // so the middleware forwards that query on to the native server. Routing must + // match on the path alone, and the query must reach WordPress unchanged. + val boundary = "test-boundary-query" + val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) + + val response = sendRawRequest( + method = "POST", + path = "/upload?_embed=wp:featuredmedia", + 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")) + // The delegate returns Original, so this is the passthrough branch. + // Pin which branch ran — `lastQuery` is recorded by both, so without this + // the query assertion would pass even if routing collapsed onto one path. + assertTrue(mockUploader.passthroughUploadCalled) + assertFalse(mockUploader.uploadCalled) + assertEquals("?_embed=wp:featuredmedia", mockUploader.lastQuery) + } + // MARK: - Upload with delegate @Test @@ -121,16 +154,97 @@ class MediaUploadServerTest { body = body ) - assertTrue("Expected 200 but got: ${response.statusLine}", response.statusLine.contains("200")) + assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) assertTrue(delegate.processFileCalled) assertTrue(delegate.uploadFileCalled) assertEquals("image/jpeg", delegate.lastMimeType) assertEquals("photo.jpg", delegate.lastFilename) + // The server relays WordPress's raw response body verbatim. val json = JsonParser.parseString(response.body).asJsonObject assertEquals(42, json.get("id").asInt) - assertEquals("https://example.com/photo.jpg", json.get("url").asString) - assertEquals("image", json.get("type").asString) + assertEquals("https://example.com/photo.jpg", json.get("source_url").asString) + assertEquals("image", json.get("media_type").asString) + } + + @Test + fun `forwards the delegate's processed metadata to the uploader`() { + val delegate = TranscodingDelegate() + val mockUploader = MockDefaultUploader() + server.stop() + server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + + val boundary = "test-boundary-meta" + val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) + + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + // The delegate changed the format, so the uploader must receive the new + // metadata — not the original video/quicktime + clip.mov. + assertTrue(mockUploader.uploadCalled) + assertEquals("video/mp4", mockUploader.lastUploadMimeType) + assertEquals("clip.mp4", mockUploader.lastUploadFilename) + } + + @Test + fun `deletes the delegate's processed file after upload`() { + val delegate = TranscodingDelegate() + val mockUploader = MockDefaultUploader() + server.stop() + server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + + val boundary = "test-boundary-cleanup" + val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) + + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + // The server owns the file the delegate produced and must delete it once the + // upload finishes — the finally in processAndUpload covers success and throw + // paths alike. A leaked processed file is a full-size temp per upload. + val processed = requireNotNull(delegate.producedFile) { "processFile was not called" } + assertFalse("Processed temp file should be deleted after upload", processed.exists()) + } + + @Test + fun `startup sweep deletes stale upload temps but preserves fresh ones`() { + val uploadsDir = File(tempFolder.root, "gutenbergkit-uploads").apply { mkdirs() } + val stale = File(uploadsDir, "stale.tmp").apply { writeText("x") } + val fresh = File(uploadsDir, "fresh.tmp").apply { writeText("y") } + // Backdate the stale file well past the 1-hour cutoff. + assertTrue( + "Could not backdate the stale file", + stale.setLastModified(System.currentTimeMillis() - 2 * 60 * 60 * 1000L) + ) + + // The sweep runs via the injected dispatcher; Unconfined runs it synchronously + // so we can assert immediately. It must delete the aged file and keep the fresh + // one — a flipped comparison would do the opposite and wipe an in-flight upload. + server.stop() + server = MediaUploadServer( + uploadDelegate = null, + defaultUploader = null, + cacheDir = tempFolder.root, + ioDispatcher = Dispatchers.Unconfined + ) + + assertFalse("Stale temp should have been swept", stale.exists()) + assertTrue("Fresh temp should be preserved", fresh.exists()) } // MARK: - Fallback to default uploader @@ -156,7 +270,7 @@ class MediaUploadServerTest { body = body ) - assertTrue("Expected 200 but got: ${response.statusLine}", response.statusLine.contains("200")) + assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) assertTrue(delegate.processFileCalled) // Passthrough: original body forwarded directly, not re-encoded. assertTrue(mockUploader.passthroughUploadCalled) @@ -169,20 +283,15 @@ class MediaUploadServerTest { // MARK: - DefaultMediaUploader @Test - fun `DefaultMediaUploader sends correct request to WP REST API`() { + fun `DefaultMediaUploader relays the WordPress response`() { val mockWpServer = MockWebServer() - // DefaultMediaUploader uses org.json.JSONObject internally which is - // stubbed in JVM unit tests — so we only verify the outgoing request - // format, not the response parsing. + val wpBody = + """{"id":1,"source_url":"https://example.com/u.jpg","media_type":"image"}""" mockWpServer.enqueue( MockResponse() - .setResponseCode(200) + .setResponseCode(201) .setHeader("Content-Type", "application/json") - .setBody( - """{"id":1,"source_url":"u","alt_text":"",""" + - """"caption":{"rendered":""},"title":{"rendered":"t"},""" + - """"mime_type":"image/jpeg","media_type":"image"}""" - ) + .setBody(wpBody) ) mockWpServer.start() @@ -196,13 +305,11 @@ class MediaUploadServerTest { val file = tempFolder.newFile("image.jpg") file.writeBytes("fake image".toByteArray()) - // The upload call will fail at org.json parsing in JVM tests, but we - // can still verify the request was sent correctly. - try { - runBlocking { uploader.upload(file, "image/jpeg", "image.jpg") } - } catch (_: Exception) { - // Expected — org.json stubs return defaults in JVM tests - } + val response = runBlocking { uploader.upload(file, "image/jpeg", "image.jpg", emptyList(), "") } + + // The uploader relays WordPress's exact status and body — no parsing. + assertEquals(201, response.statusCode) + assertEquals(wpBody, String(response.body)) val request = mockWpServer.takeRequest() assertEquals("POST", request.method) @@ -214,7 +321,7 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader throws on server error`() { + fun `DefaultMediaUploader relays a WordPress error response instead of throwing`() { val mockWpServer = MockWebServer() mockWpServer.enqueue(MockResponse().setResponseCode(500).setBody("Internal error")) mockWpServer.start() @@ -229,13 +336,104 @@ class MediaUploadServerTest { val file = tempFolder.newFile("fail.jpg") file.writeBytes("data".toByteArray()) - try { - runBlocking { uploader.upload(file, "image/jpeg", "fail.jpg") } - throw AssertionError("Expected exception") - } catch (e: MediaUploadException) { - assertTrue(e.message!!.contains("Internal error")) + // WordPress's error status + body flow through to the editor, which + // surfaces the real message — the uploader does not throw. + val response = runBlocking { uploader.upload(file, "image/jpeg", "fail.jpg", emptyList(), "") } + assertEquals(500, response.statusCode) + assertEquals("Internal error", String(response.body)) + + mockWpServer.shutdown() + } + + @Test + fun `DefaultMediaUploader normalizes an unslashed root and namespace`() { + val mockWpServer = MockWebServer() + mockWpServer.enqueue(MockResponse().setResponseCode(201).setBody("{}")) + mockWpServer.start() + + val uploader = DefaultMediaUploader( + httpClient = okhttp3.OkHttpClient(), + siteApiRoot = mockWpServer.url("/wp-json").toString(), // no trailing slash + authHeader = "Bearer test-token", + siteApiNamespace = listOf("sites/123") // no trailing slash + ) + val file = tempFolder.newFile("image.jpg") + file.writeBytes("x".toByteArray()) + + runBlocking { uploader.upload(file, "image/jpeg", "image.jpg", emptyList(), "") } + + assertEquals("/wp-json/wp/v2/sites/123/media", mockWpServer.takeRequest().path) + + mockWpServer.shutdown() + } + + @Test + fun `DefaultMediaUploader re-encode preserves extra parts and query`() { + val mockWpServer = MockWebServer() + mockWpServer.enqueue(MockResponse().setResponseCode(201).setBody("{}")) + 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()) + + val postPart = org.wordpress.gutenberg.http.MultipartPart( + name = "post", + filename = null, + contentType = "text/plain", + body = org.wordpress.gutenberg.http.RequestBody.InMemory("123".toByteArray()) + ) + + runBlocking { + uploader.upload(file, "image/jpeg", "image.jpg", listOf(postPart), "?_embed=wp:featuredmedia") + } + + val request = mockWpServer.takeRequest() + // The query and the non-file part must both reach WordPress. + assertTrue(request.path!!.contains("_embed")) + val bodyText = request.body.readUtf8() + assertTrue("Expected post field in multipart body", bodyText.contains("name=\"post\"")) + assertTrue(bodyText.contains("123")) + + mockWpServer.shutdown() + } + + @Test + fun `re-encode forwards a non-UTF-8 field value verbatim`() { + val mockWpServer = MockWebServer() + mockWpServer.enqueue(MockResponse().setResponseCode(201).setBody("{}")) + 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()) + + // A value that is not valid UTF-8 (a lone 0xFF byte between two ASCII bytes). + val binaryValue = byteArrayOf(0x61, 0xFF.toByte(), 0x62) + val blobPart = org.wordpress.gutenberg.http.MultipartPart( + name = "blob", + filename = null, + contentType = "application/octet-stream", + body = org.wordpress.gutenberg.http.RequestBody.InMemory(binaryValue) + ) + + runBlocking { + uploader.upload(file, "image/jpeg", "image.jpg", listOf(blobPart), "") } + // The raw 0xFF byte survives verbatim — not coerced to a replacement char. + val bodyBytes = mockWpServer.takeRequest().body.readByteArray() + val found = bodyBytes.toList().windowed(binaryValue.size).any { it == binaryValue.toList() } + assertTrue("Non-UTF-8 field value should pass through verbatim", found) + mockWpServer.shutdown() } @@ -355,31 +553,39 @@ class MediaUploadServerTest { @Volatile var lastMimeType: String? = null @Volatile var lastFilename: String? = null - override suspend fun processFile(file: File, mimeType: String): File { + override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { processFileCalled = true lastMimeType = mimeType - return file + return ProcessedProxyFile.Original } - override suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResult? { + override suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? { uploadFileCalled = true lastFilename = filename - return MediaUploadResult( - id = 42, - url = "https://example.com/photo.jpg", - title = "photo", - mime = "image/jpeg", - type = "image" - ) + val json = """{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}""" + return MediaUploadResponse(201, json.toByteArray()) } } private class ProcessOnlyDelegate : MediaUploadDelegate { @Volatile var processFileCalled = false - override suspend fun processFile(file: File, mimeType: String): File { + override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { processFileCalled = true - return file + 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. */ + @Volatile var producedFile: File? = null + + override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { + val newFile = File(file.parentFile, "processed-${file.name}") + newFile.writeBytes("processed".toByteArray()) + producedFile = newFile + return ProcessedProxyFile.Processed(newFile, "video/mp4", "clip.mp4") } } @@ -390,26 +596,34 @@ class MediaUploadServerTest { ) { @Volatile var uploadCalled = false @Volatile var passthroughUploadCalled = false - - override suspend fun upload(file: File, mimeType: String, filename: String): MediaUploadResult { + @Volatile var lastUploadMimeType: String? = null + @Volatile var lastUploadFilename: String? = null + @Volatile var lastQuery: String? = null + + override suspend fun upload( + file: File, mimeType: String, filename: String, + extraParts: List, query: String + ): MediaUploadResponse { uploadCalled = true - return mockResult() + lastUploadMimeType = mimeType + lastUploadFilename = filename + lastQuery = query + return mockResponse() } override suspend fun passthroughUpload( body: org.wordpress.gutenberg.http.RequestBody, - contentType: String - ): MediaUploadResult { + contentType: String, + query: String + ): MediaUploadResponse { passthroughUploadCalled = true - return mockResult() + lastQuery = query + return mockResponse() } - private fun mockResult() = MediaUploadResult( - id = 99, - url = "https://example.com/doc.pdf", - title = "doc", - mime = "application/pdf", - type = "file" + private fun mockResponse() = MediaUploadResponse( + 201, + """{"id":99,"source_url":"https://example.com/doc.pdf","media_type":"file"}""".toByteArray() ) } diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/RestUrlBuilderTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/RestUrlBuilderTest.kt new file mode 100644 index 000000000..73065c5fb --- /dev/null +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/RestUrlBuilderTest.kt @@ -0,0 +1,47 @@ +package org.wordpress.gutenberg + +import org.junit.Assert.assertEquals +import org.junit.Test + +class RestUrlBuilderTest { + + @Test + fun `appends the path when no namespace is configured`() { + assertEquals( + "https://example.com/wp-json/wp/v2/media", + RestUrlBuilder.namespaced("https://example.com/wp-json", null, "/wp/v2/media") + ) + } + + @Test + fun `inserts the namespace after the version segment`() { + assertEquals( + "https://example.com/wp-json/wp/v2/sites/123/media", + RestUrlBuilder.namespaced("https://example.com/wp-json", "sites/123/", "/wp/v2/media") + ) + } + + @Test + fun `normalizes an unslashed root and namespace`() { + assertEquals( + "https://example.com/wp-json/wp/v2/sites/123/media", + RestUrlBuilder.namespaced("https://example.com/wp-json", "sites/123", "/wp/v2/media") + ) + } + + @Test + fun `does not double the slash when the root already ends in one`() { + assertEquals( + "https://example.com/wp-json/wp/v2/sites/123/media", + RestUrlBuilder.namespaced("https://example.com/wp-json/", "sites/123", "/wp/v2/media") + ) + } + + @Test + fun `inserts the namespace after a non-wp-v2 version segment`() { + assertEquals( + "https://example.com/wp-json/wp-block-editor/v1/sites/123/settings", + RestUrlBuilder.namespaced("https://example.com/wp-json", "sites/123", "/wp-block-editor/v1/settings") + ) + } +} diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/http/FixtureTests.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/http/FixtureTests.kt index 012ba42a3..d7459e684 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/http/FixtureTests.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/http/FixtureTests.kt @@ -158,6 +158,11 @@ class FixtureTests { pendingError.errorId, "$description: expected $expectedError but got ${pendingError.errorId}" ) + assertEquals( + HTTPRequestParseError.Disposition.RECOVERABLE, + pendingError.disposition, + "$description: ${pendingError.errorId} surfaced via pendingParseError but is not RECOVERABLE" + ) } else { fail("$description: expected error $expectedError but parsing succeeded") } @@ -167,6 +172,11 @@ class FixtureTests { e.error.errorId, "$description: expected $expectedError but got ${e.error.errorId}" ) + assertEquals( + HTTPRequestParseError.Disposition.FATAL, + e.error.disposition, + "$description: ${e.error.errorId} was thrown but is not FATAL" + ) } } } diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/http/HTTPRequestParserTests.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/http/HTTPRequestParserTests.kt index abdc9c2e1..060a211e5 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/http/HTTPRequestParserTests.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/http/HTTPRequestParserTests.kt @@ -15,6 +15,21 @@ import org.junit.Test */ class HTTPRequestParserTests { + // MARK: - Error Disposition + + /** + * Locks the fatal/recoverable classification so a refactor can't silently + * make a smuggling-relevant error recoverable — which would let a malformed + * request reach the handler before auth. + */ + @Test + fun `only payloadTooLarge is recoverable`() { + val recoverable = HTTPRequestParseError.entries.filter { + it.disposition == HTTPRequestParseError.Disposition.RECOVERABLE + } + assertEquals(listOf(HTTPRequestParseError.PAYLOAD_TOO_LARGE), recoverable) + } + // MARK: - Duplicate Header Key Casing (Internal Dict Representation) @Test 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 7f390546a..9524bb278 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt @@ -2,9 +2,13 @@ package com.example.gutenbergkit import android.graphics.Bitmap import android.graphics.BitmapFactory +import android.graphics.Matrix +import android.media.ExifInterface import android.util.Log import org.wordpress.gutenberg.MediaUploadDelegate +import org.wordpress.gutenberg.ProcessedProxyFile import java.io.File +import java.io.IOException /** * Demo media upload delegate that resizes images to a maximum dimension of 2000px. @@ -16,9 +20,9 @@ class DemoMediaUploadDelegate : MediaUploadDelegate { private const val TAG = "DemoMediaUploadDelegate" } - override suspend fun processFile(file: File, mimeType: String): File { + override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { if (!mimeType.startsWith("image/") || mimeType == "image/gif") { - return file + return ProcessedProxyFile.Original } val maxDimension = 2000 @@ -30,17 +34,17 @@ class DemoMediaUploadDelegate : MediaUploadDelegate { val width = options.outWidth val height = options.outHeight - if (width <= 0 || height <= 0) return file + if (width <= 0 || height <= 0) return ProcessedProxyFile.Original val longestSide = maxOf(width, height) - if (longestSide <= maxDimension) return file + if (longestSide <= maxDimension) return ProcessedProxyFile.Original // Calculate sample size for memory-efficient decoding val sampleSize = Integer.highestOneBit(longestSide / maxDimension) val decodeOptions = BitmapFactory.Options().apply { inSampleSize = sampleSize } - val sampled = BitmapFactory.decodeFile(file.absolutePath, decodeOptions) ?: return file + val sampled = BitmapFactory.decodeFile(file.absolutePath, decodeOptions) ?: return ProcessedProxyFile.Original // Scale to exact target dimensions val scale = maxDimension.toFloat() / longestSide.toFloat() @@ -49,16 +53,57 @@ class DemoMediaUploadDelegate : MediaUploadDelegate { val scaled = Bitmap.createScaledBitmap(sampled, targetWidth, targetHeight, true) if (scaled !== sampled) sampled.recycle() - val outputFile = File(file.parent, "resized-${file.name}") - val format = if (mimeType == "image/png") Bitmap.CompressFormat.PNG - else Bitmap.CompressFormat.JPEG + // Bake the EXIF orientation into the pixels. Re-encoding via compress() + // writes no EXIF, so without this a portrait photo (stored landscape plus + // an orientation tag) would upload rotated. + val oriented = applyExifOrientation(scaled, file) + + // Re-encoding normalizes everything but PNG to JPEG (Bitmap.compress can't + // round-trip WebP/HEIC/etc.), so report the ACTUAL output type and extension. + // Otherwise a WebP/HEIC upload would be JPEG bytes labeled image/webp, and + // WordPress would reject the content/extension mismatch. + val (format, outputMimeType, outputExtension) = + if (mimeType == "image/png") { + Triple(Bitmap.CompressFormat.PNG, "image/png", "png") + } else { + Triple(Bitmap.CompressFormat.JPEG, "image/jpeg", "jpg") + } + val outputFile = File(file.parent, "resized-${file.name}") outputFile.outputStream().use { out -> - scaled.compress(format, 85, out) + oriented.compress(format, 85, out) } - scaled.recycle() + oriented.recycle() + val outputFilename = filename.substringBeforeLast('.', filename) + ".$outputExtension" Log.d(TAG, "Resized image from ${width}×${height} to ${targetWidth}×${targetHeight}") - return outputFile + return ProcessedProxyFile.Processed(outputFile, outputMimeType, outputFilename) + } + + private fun applyExifOrientation(bitmap: Bitmap, sourceFile: File): Bitmap { + val orientation = try { + ExifInterface(sourceFile.absolutePath).getAttributeInt( + ExifInterface.TAG_ORIENTATION, ExifInterface.ORIENTATION_NORMAL + ) + } catch (e: IOException) { + Log.w(TAG, "Failed to read EXIF orientation", e) + ExifInterface.ORIENTATION_NORMAL + } + + val matrix = Matrix() + when (orientation) { + ExifInterface.ORIENTATION_ROTATE_90 -> matrix.postRotate(90f) + ExifInterface.ORIENTATION_ROTATE_180 -> matrix.postRotate(180f) + ExifInterface.ORIENTATION_ROTATE_270 -> matrix.postRotate(270f) + ExifInterface.ORIENTATION_FLIP_HORIZONTAL -> matrix.postScale(-1f, 1f) + ExifInterface.ORIENTATION_FLIP_VERTICAL -> matrix.postScale(1f, -1f) + ExifInterface.ORIENTATION_TRANSPOSE -> matrix.apply { postRotate(90f); postScale(-1f, 1f) } + ExifInterface.ORIENTATION_TRANSVERSE -> matrix.apply { postRotate(270f); postScale(-1f, 1f) } + else -> return bitmap + } + + val rotated = Bitmap.createBitmap(bitmap, 0, 0, bitmap.width, bitmap.height, matrix, true) + if (rotated !== bitmap) bitmap.recycle() + return rotated } } diff --git a/ios/Demo-iOS/Sources/Views/EditorView.swift b/ios/Demo-iOS/Sources/Views/EditorView.swift index d40fb1aaa..47dd0755f 100644 --- a/ios/Demo-iOS/Sources/Views/EditorView.swift +++ b/ios/Demo-iOS/Sources/Views/EditorView.swift @@ -298,9 +298,9 @@ private struct _EditorView: UIViewControllerRepresentable { // MARK: - MediaUploadDelegate /// Resizes images to a maximum dimension of 2000px before upload. - nonisolated func processFile(at url: URL, mimeType: String) async throws -> URL { + nonisolated func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { guard mimeType.hasPrefix("image/"), mimeType != "image/gif" else { - return url + return .original } let maxDimension: CGFloat = 2000 @@ -309,12 +309,12 @@ private struct _EditorView: UIViewControllerRepresentable { let properties = CGImageSourceCopyPropertiesAtIndex(source, 0, nil) as? [CFString: Any], let width = properties[kCGImagePropertyPixelWidth] as? CGFloat, let height = properties[kCGImagePropertyPixelHeight] as? CGFloat else { - return url + return .original } let longestSide = max(width, height) guard longestSide > maxDimension else { - return url + return .original } let options: [CFString: Any] = [ @@ -324,7 +324,7 @@ private struct _EditorView: UIViewControllerRepresentable { ] guard let thumbnail = CGImageSourceCreateThumbnailAtIndex(source, 0, options as CFDictionary) else { - return url + return .original } let outputURL = url.deletingLastPathComponent() @@ -337,16 +337,17 @@ private struct _EditorView: UIViewControllerRepresentable { 1, nil ) else { - return url + return .original } CGImageDestinationAddImage(destination, thumbnail, nil) guard CGImageDestinationFinalize(destination) else { - return url + return .original } Logger.demo.info("Resized image from \(Int(width))x\(Int(height)) to fit \(Int(maxDimension))px") - return outputURL + // Same format, so the original mimeType/filename carry over. + return .processed(outputURL, mimeType: mimeType, filename: filename) } } diff --git a/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift b/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift index eba61c3cb..ee3c384b4 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift @@ -4,9 +4,24 @@ import OSLog /// A protocol for making authenticated HTTP requests to the WordPress REST API. public protocol EditorHTTPClientProtocol: Sendable { func perform(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) + + /// Like ``perform(_:)`` but does **not** throw on a non-2xx status — returns + /// the raw response so the caller can relay WordPress's exact status and body. + /// Used by the media upload server, which forwards WordPress's response (and + /// its errors) to the editor unchanged. + func performRaw(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) + func download(_ urlRequest: URLRequest) async throws -> (URL, HTTPURLResponse) } +public extension EditorHTTPClientProtocol { + /// Default implementation validates the status like ``perform(_:)``. Only + /// clients that need to relay non-2xx responses override this. + func performRaw(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { + try await perform(urlRequest) + } +} + /// A delegate for observing HTTP requests made by the editor. /// /// Implement this protocol to inspect or log all network requests. @@ -99,6 +114,13 @@ public actor EditorHTTPClient: EditorHTTPClientProtocol { return (data, httpResponse) } + public func performRaw(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { + let configuredRequest = self.configureRequest(urlRequest) + let (data, response) = try await self.urlSession.data(for: configuredRequest) + self.delegate?.didPerformRequest(configuredRequest, response: response, data: .bytes(data)) + return (data, response as! HTTPURLResponse) + } + public func download(_ urlRequest: URLRequest) async throws -> (URL, HTTPURLResponse) { let configuredRequest = self.configureRequest(urlRequest) diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 5b4c20544..eb8eea996 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -272,7 +272,17 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro public override func viewDidDisappear(_ animated: Bool) { super.viewDidDisappear(animated) self.dependencyTaskHandle?.cancel() - self.uploadServer?.stop() + } + + deinit { + // Stop the upload server when the editor is permanently torn down. + // + // This deliberately does NOT happen in `viewDidDisappear`, which also + // fires when another view controller is merely pushed or presented over + // the editor. `HTTPServer.stop()` cancels the `NWListener`, which is + // terminal and has no restart path — stopping on disappear left uploads + // permanently broken once the user returned to the editor. + uploadServer?.stop() } /// Fetches all required dependencies and then loads the editor. diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index 08947ca22..ab4c809b9 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -1,32 +1,39 @@ import Foundation -/// Result of a successful media upload to the remote WordPress server. +/// A raw response from the WordPress REST API media endpoint. /// -/// This structure matches the format expected by Gutenberg's `onFileChange` callback. -public struct MediaUploadResult: Codable, Sendable { - public let id: Int - public let url: String - public let alt: String - public let caption: String - public let title: String - public let mime: String - public let type: String - public let width: Int? - public let height: Int? - - public init(id: Int, url: String, alt: String = "", caption: String = "", title: String, mime: String, type: String, width: Int? = nil, height: Int? = nil) { - self.id = id - self.url = url - self.alt = alt - self.caption = caption - self.title = title - self.mime = mime - self.type = type - self.width = width - self.height = height +/// GutenbergKit relays this to the editor verbatim — it does not interpret the +/// body. The editor therefore receives the exact attachment object (on success) +/// or WordPress REST error object (on failure) it would get from a direct +/// upload, so every consumer — image sub-sizes, attachment links, error notices — +/// behaves identically to a non-native upload. +public struct MediaUploadResponse: Sendable { + /// The HTTP status code WordPress (or the host's upload service) returned. + public let statusCode: Int + + /// The raw response body — a WordPress REST attachment on success, or a + /// WordPress REST error object (`{ "code", "message", "data" }`) on failure. + public let body: Data + + public init(statusCode: Int, body: Data) { + self.statusCode = statusCode + self.body = body } } +/// The result of a delegate's ``MediaUploadDelegate/processFile(at:mimeType:filename:)``. +public enum ProcessedProxyFile: Sendable { + /// The delegate did not modify the file; the original upload is forwarded + /// to WordPress unchanged. + case original + + /// The delegate produced a file to upload, along with its MIME type and + /// filename. Both are used verbatim, so a format change (e.g. transcoding + /// MOV to MP4, or an in-place EXIF strip) must report the resulting type and + /// filename for WordPress to store the file correctly. + case processed(URL, mimeType: String, filename: String) +} + /// Protocol for customizing media upload behavior. /// /// The native host app can provide an implementation to resize images, @@ -34,21 +41,29 @@ public struct MediaUploadResult: Codable, Sendable { /// pass files through unchanged and upload via the WordPress REST API. public protocol MediaUploadDelegate: AnyObject, Sendable { /// Process a file before upload (e.g., resize image, transcode video). - /// Return the URL of the processed file, or the original URL for passthrough. - func processFile(at url: URL, mimeType: String) async throws -> URL + /// + /// Return ``ProcessedProxyFile/original`` to upload the file unchanged, or + /// ``ProcessedProxyFile/processed(_:mimeType:filename:)`` with the processed + /// file and its metadata. When the format changes, report the new mimeType + /// and filename so WordPress stores it with the correct extension and type. + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile /// Upload a processed file to the remote WordPress site. - /// Return the Gutenberg-compatible media result, or `nil` to use the default uploader. - func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResult? + /// + /// Return the raw WordPress response (status code + body), which GutenbergKit + /// relays to the editor unchanged, or `nil` to use the default uploader. A + /// host that uploads to WordPress should return the exact response it + /// received so the editor sees a complete attachment object. + func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? } /// Default implementations. extension MediaUploadDelegate { - public func processFile(at url: URL, mimeType: String) async throws -> URL { - url + public func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + .original } - public func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResult? { + public func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { nil } } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index c06a0fa79..53795d79f 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -34,12 +34,16 @@ final class MediaUploadServer: Sendable { defaultUploader: DefaultMediaUploader? = nil, maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize ) async throws -> MediaUploadServer { + // Sweep temp files orphaned by a prior crash before starting. + cleanOrphanedUploads() + let context = UploadContext(uploadDelegate: uploadDelegate, defaultUploader: defaultUploader) let server = try await HTTPServer.start( name: "media-upload", requiresAuthentication: true, maxRequestBodySize: maxRequestBodySize, + cors: .permissive, handler: { request in await Self.handleRequest(request, context: context) } @@ -71,18 +75,15 @@ final class MediaUploadServer: Sendable { case .payloadTooLarge: "The file is too large to upload in the editor." default: "\(serverError.httpStatusText)" } - return errorResponse(status: serverError.httpStatus, body: message) - } - - // CORS preflight — the library exempts OPTIONS from auth, so this is - // reached without a token. - if parsed.method.uppercased() == "OPTIONS" { - return corsPreflightResponse() + return errorResponse(status: serverError.httpStatus, message: message) } - // Route: only POST /upload is handled. - guard parsed.method.uppercased() == "POST", parsed.target == "/upload" else { - return errorResponse(status: 404, body: "Not found") + // 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 + // upload handler relays on to WordPress. + guard parsed.method.uppercased() == "POST", parsed.path == "/upload" else { + return errorResponse(status: 404, message: "Not found") } return await handleUpload(request, context: context) @@ -94,14 +95,19 @@ final class MediaUploadServer: Sendable { parts = try request.parsed.multipartParts() } catch { Logger.uploadServer.error("Multipart parse failed: \(error)") - return errorResponse(status: 400, body: "Expected multipart/form-data") + return errorResponse(status: 400, message: "Expected multipart/form-data") } // Find the file part (the first part with a filename). guard let filePart = parts.first(where: { $0.filename != nil }) else { - return errorResponse(status: 400, body: "No file found in request") + return errorResponse(status: 400, message: "No file found in request") } + // The non-file parts (post, additionalData) and the original query + // (e.g. ?_embed) must reach WordPress too — relay them alongside the file. + 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 @@ -110,8 +116,7 @@ final class MediaUploadServer: Sendable { let filename = sanitizeFilename(filePart.filename ?? "upload") let mimeType = filePart.contentType - let tempDir = FileManager.default.temporaryDirectory - .appending(component: "GutenbergKit-uploads", directoryHint: .isDirectory) + let tempDir = uploadsTempDirectory try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(filename)") @@ -119,22 +124,26 @@ final class MediaUploadServer: Sendable { let inputStream = try filePart.body.makeInputStream() try writeStream(inputStream, to: fileURL) } catch { + try? FileManager.default.removeItem(at: fileURL) Logger.uploadServer.error("Failed to write upload to disk: \(error)") - return errorResponse(status: 500, body: "Failed to save file") + return errorResponse(status: 500, message: "Failed to save file") } - // Process and upload through the delegate pipeline. - let result: Result - var processedURL: URL? + // From here on always clean up the original temp file. The processed + // file (if the delegate produced a new one) is cleaned up inside + // processAndUpload so its throw paths are covered too. + defer { try? FileManager.default.removeItem(at: fileURL) } + do { let uploadResult = try await processAndUpload( - fileURL: fileURL, mimeType: mimeType, filename: filePart.filename ?? "upload", context: context + fileURL: fileURL, mimeType: mimeType, filename: filePart.filename ?? "upload", + extraParts: extraParts, query: query, context: context ) + let response: MediaUploadResponse switch uploadResult { - case .uploaded(let media, let processed): - processedURL = processed - Logger.uploadServer.debug("Uploading processed file to WordPress") - result = .success(media) + case .uploaded(let uploaded): + Logger.uploadServer.debug("Uploaded file to WordPress") + response = uploaded case .passthrough: // Delegate didn't modify the file — forward the original // request body to WordPress without re-encoding. @@ -142,37 +151,20 @@ final class MediaUploadServer: Sendable { guard let body = request.parsed.body, let contentType = request.parsed.header("Content-Type"), let defaultUploader = context.defaultUploader else { - result = .failure(UploadError.noUploader) - break + return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) } - let media = try await defaultUploader.passthroughUpload(body: body, contentType: contentType) - result = .success(media) + response = try await defaultUploader.passthroughUpload(body: body, contentType: contentType, query: 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: [("Content-Type", "application/json")], + body: response.body + ) } catch { - result = .failure(error) - } - - // Clean up temp files (success or failure). - try? FileManager.default.removeItem(at: fileURL) - if let processedURL, processedURL != fileURL { - try? FileManager.default.removeItem(at: processedURL) - } - - switch result { - case .success(let media): - do { - let json = try JSONEncoder().encode(media) - return HTTPResponse( - status: 200, - headers: corsHeaders + [("Content-Type", "application/json")], - body: json - ) - } catch { - return errorResponse(status: 500, body: "Failed to encode response") - } - case .failure(let error): Logger.uploadServer.error("Upload processing failed: \(error)") - return errorResponse(status: 500, body: error.localizedDescription) + return errorResponse(status: 500, message: error.localizedDescription) } } @@ -180,69 +172,107 @@ final class MediaUploadServer: Sendable { /// Result of the delegate processing + upload pipeline. private enum UploadResult { - /// The delegate (or default uploader) completed the upload. - case uploaded(MediaUploadResult, processedURL: URL) + /// The delegate (or default uploader) completed the upload; carries the + /// raw WordPress response to relay. + case uploaded(MediaUploadResponse) /// The delegate didn't modify the file and `uploadFile` returned nil. /// The caller should forward the original request body to WordPress. case passthrough } private static func processAndUpload( - fileURL: URL, mimeType: String, filename: String, context: UploadContext + fileURL: URL, mimeType: String, filename: String, + extraParts: [MultipartPart], query: String, context: UploadContext ) async throws -> UploadResult { // Step 1: Process (resize, transcode, etc.) - let processedURL: URL + let processed: ProcessedProxyFile if let delegate = context.uploadDelegate { - processedURL = try await delegate.processFile(at: fileURL, mimeType: mimeType) + processed = try await delegate.processFile(at: fileURL, mimeType: mimeType, filename: filename) } else { - processedURL = fileURL + processed = .original + } + + // Resolve the file to upload and its metadata. `.processed` uses the + // delegate's values verbatim, so a format change is reported to WordPress. + let uploadURL: URL + let uploadMimeType: String + let uploadFilename: String + switch processed { + case .original: + uploadURL = fileURL + uploadMimeType = mimeType + uploadFilename = filename + case let .processed(url, processedMimeType, processedFilename): + uploadURL = url + uploadMimeType = processedMimeType + uploadFilename = processedFilename + } + + // The processed file (if the delegate produced a new one) is ours to + // clean up — on success it has been uploaded, on failure it is abandoned. + // Cleaning up here rather than in the caller covers the throw paths too. + defer { + if uploadURL != fileURL { + try? FileManager.default.removeItem(at: uploadURL) + } } // Step 2: Upload to remote WordPress if let delegate = context.uploadDelegate, - let result = try await delegate.uploadFile(at: processedURL, mimeType: mimeType, filename: filename) { - return .uploaded(result, processedURL: processedURL) + let result = try await delegate.uploadFile(at: uploadURL, mimeType: uploadMimeType, filename: uploadFilename) { + return .uploaded(result) } else if let defaultUploader = context.defaultUploader { - // If the delegate didn't modify the file, the original request - // body can be forwarded directly — skip multipart re-encoding. - if processedURL == fileURL { + // Unmodified — forward the original request body directly, skipping + // multipart re-encoding. + if case .original = processed { return .passthrough } - let result = try await defaultUploader.upload(fileURL: processedURL, mimeType: mimeType, filename: filename) - return .uploaded(result, processedURL: processedURL) + let result = try await defaultUploader.upload(fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, extraParts: extraParts, query: query) + return .uploaded(result) } else { throw UploadError.noUploader } } - // MARK: - CORS - - private static let corsHeaders: [(String, String)] = [ - ("Access-Control-Allow-Origin", "*"), - ("Access-Control-Allow-Headers", "Relay-Authorization, Content-Type"), - ] - - private static func corsPreflightResponse() -> HTTPResponse { - HTTPResponse( - status: 204, - headers: corsHeaders + [ - ("Access-Control-Allow-Methods", "POST, OPTIONS"), - ("Access-Control-Max-Age", "86400"), - ], - body: Data() - ) - } - - private static func errorResponse(status: Int, body: String) -> HTTPResponse { - HTTPResponse( + private static func errorResponse(status: Int, message: String) -> HTTPResponse { + // Emit a WordPress-REST-style error object so the JS middleware normalizes + // it (and surfaces `message`) the same way it does a relayed WordPress + // error — the local server's own errors need no special-casing. + let payload = ["code": "upload_error", "message": message] + let body = (try? JSONSerialization.data(withJSONObject: payload)) + ?? Data(#"{"code":"upload_error","message":"Upload failed"}"#.utf8) + return HTTPResponse( status: status, - headers: corsHeaders + [("Content-Type", "text/plain")], - body: Data(body.utf8) + headers: [("Content-Type", "application/json")], + body: body ) } // MARK: - Helpers + /// Directory for staging uploaded files, under the system temp dir. + private static var uploadsTempDirectory: URL { + FileManager.default.temporaryDirectory + .appending(component: "GutenbergKit-uploads", directoryHint: .isDirectory) + } + + /// Deletes upload temp files left behind by a prior crash. Files still in + /// flight (only seconds old) are preserved by the age threshold, so this is + /// safe even if another editor instance is mid-upload. + private static func cleanOrphanedUploads() { + let cutoff = Date(timeIntervalSinceNow: -3600) // 1 hour ago + guard let files = try? FileManager.default.contentsOfDirectory( + at: uploadsTempDirectory, + includingPropertiesForKeys: [.contentModificationDateKey] + ) else { return } + for file in files { + let modified = (try? file.resourceValues(forKeys: [.contentModificationDateKey]))?.contentModificationDate + if let modified, modified < cutoff { + try? FileManager.default.removeItem(at: file) + } + } + } + /// Sanitizes a filename to prevent path traversal. private static func sanitizeFilename(_ name: String) -> String { let safe = (name as NSString).lastPathComponent @@ -286,30 +316,47 @@ final class MediaUploadServer: Sendable { } } - // MARK: - Errors +} - enum UploadError: Error, LocalizedError { - case noUploader - case streamReadFailed - case streamWriteFailed +// MARK: - Errors - var errorDescription: String? { - switch self { - case .noUploader: "No upload delegate or default uploader configured" - case .streamReadFailed: "Failed to read upload stream" - case .streamWriteFailed: "Failed to write upload to disk" - } +/// Errors from the native media upload pipeline. +enum UploadError: Error, LocalizedError { + case noUploader + case streamReadFailed + case streamWriteFailed + + var errorDescription: String? { + switch self { + case .noUploader: "No upload delegate or default uploader configured" + case .streamReadFailed: "Failed to read upload stream" + case .streamWriteFailed: "Failed to write upload to disk" } } } // MARK: - Upload Context -/// Thread-safe container for the upload delegate and default uploader, -/// captured by the HTTPServer handler closure. -private struct UploadContext: Sendable { - let uploadDelegate: (any MediaUploadDelegate)? +/// Container for the upload delegate and default uploader, captured by the +/// HTTPServer handler closure and re-read on each request. +/// +/// The delegate is held **weakly**. `EditorViewController.mediaUploadDelegate` is +/// declared `weak` — the host owns the delegate's lifetime. Capturing it strongly +/// here would silently defeat that contract and, worse, risk a retain cycle +/// (`EditorViewController → uploadServer → HTTPServer → handler → UploadContext → +/// delegate → EditorViewController`) that would keep the view controller — and +/// therefore the server — alive forever, so `deinit` would never stop it. +/// +/// `@unchecked Sendable`: `uploadDelegate` is assigned once at init and only read +/// afterwards; weak-reference reads are thread-safe at runtime. +private final class UploadContext: @unchecked Sendable { + weak var uploadDelegate: (any MediaUploadDelegate)? let defaultUploader: DefaultMediaUploader? + + init(uploadDelegate: (any MediaUploadDelegate)?, defaultUploader: DefaultMediaUploader?) { + self.uploadDelegate = uploadDelegate + self.defaultUploader = defaultUploader + } } // MARK: - Default Media Uploader @@ -326,24 +373,35 @@ class DefaultMediaUploader: @unchecked Sendable { self.siteApiNamespace = siteApiNamespace.first } - /// The WordPress media endpoint URL, accounting for site API namespaces. - private var mediaEndpointURL: URL { - let mediaPath = if let siteApiNamespace { - "wp/v2/\(siteApiNamespace)media" - } else { - "wp/v2/media" - } - return siteApiRoot.appending(path: mediaPath) + /// The WordPress media endpoint URL, built through the shared + /// ``WordPressRESTURL`` namespacing (so it matches every other REST URL) and + /// carrying the original request query (e.g. `?_embed`) through to WordPress. + private func mediaEndpointURL(query: String) -> URL { + let base = WordPressRESTURL.namespaced(apiRoot: siteApiRoot, path: "/wp/v2/media", namespace: siteApiNamespace) + guard !query.isEmpty else { return base } + // `query` is the raw request query in wire form (leading "?"). Set it via + // `percentEncodedQuery` so a value that isn't URL-safe can't make + // `URL(string:)` return nil and silently drop the query. + var components = URLComponents(url: base, resolvingAgainstBaseURL: false) + components?.percentEncodedQuery = String(query.dropFirst()) + return components?.url ?? base } - func upload(fileURL: URL, mimeType: String, filename: String) async throws -> MediaUploadResult { + func upload(fileURL: URL, mimeType: String, filename: String, extraParts: [MultipartPart], query: String) async throws -> MediaUploadResponse { let boundary = UUID().uuidString + // Read the (small, text) non-file parts up front so the body builder + // stays synchronous — the file itself is still streamed from disk. + var extraFields: [(name: String, value: Data)] = [] + for part in extraParts { + extraFields.append((part.name, try await part.body.data)) + } + let (bodyStream, contentLength) = try Self.multipartBodyStream( - fileURL: fileURL, boundary: boundary, filename: filename, mimeType: mimeType + fileURL: fileURL, boundary: boundary, filename: filename, mimeType: mimeType, extraFields: extraFields ) - var request = URLRequest(url: mediaEndpointURL) + var request = URLRequest(url: mediaEndpointURL(query: query)) request.httpMethod = "POST" request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") request.setValue("\(contentLength)", forHTTPHeaderField: "Content-Length") @@ -356,8 +414,8 @@ class DefaultMediaUploader: @unchecked Sendable { /// /// Used when the delegate's `processFile` returned the file unchanged — /// the incoming multipart body is already valid for WordPress. - func passthroughUpload(body: RequestBody, contentType: String) async throws -> MediaUploadResult { - var request = URLRequest(url: mediaEndpointURL) + func passthroughUpload(body: RequestBody, contentType: String, query: String) async throws -> MediaUploadResponse { + var request = URLRequest(url: mediaEndpointURL(query: query)) request.httpMethod = "POST" request.setValue(contentType, forHTTPHeaderField: "Content-Type") request.setValue("\(body.count)", forHTTPHeaderField: "Content-Length") @@ -366,34 +424,12 @@ class DefaultMediaUploader: @unchecked Sendable { return try await performUpload(request) } - private func performUpload(_ request: URLRequest) async throws -> MediaUploadResult { - let (data, response) = try await httpClient.perform(request) - - guard (200...299).contains(response.statusCode) else { - let preview = String(data: data.prefix(500), encoding: .utf8) ?? "" - throw MediaUploadError.uploadFailed(statusCode: response.statusCode, preview: preview) - } - - // Parse the WordPress media response into our result type - let wpMedia: WPMediaResponse - do { - wpMedia = try JSONDecoder().decode(WPMediaResponse.self, from: data) - } catch { - let preview = String(data: data.prefix(500), encoding: .utf8) ?? "" - throw MediaUploadError.unexpectedResponse(preview: preview, underlyingError: error) - } - - return MediaUploadResult( - id: wpMedia.id, - url: wpMedia.source_url, - alt: wpMedia.alt_text ?? "", - caption: wpMedia.caption?.rendered ?? "", - title: wpMedia.title.rendered, - mime: wpMedia.mime_type, - type: wpMedia.media_type, - width: wpMedia.media_details?.width, - height: wpMedia.media_details?.height - ) + private func performUpload(_ request: URLRequest) async throws -> 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. `performRaw` does not throw on non-2xx. + let (data, response) = try await httpClient.performRaw(request) + return MediaUploadResponse(statusCode: response.statusCode, body: data) } // MARK: - Streaming Multipart Body @@ -409,17 +445,28 @@ class DefaultMediaUploader: @unchecked Sendable { fileURL: URL, boundary: String, filename: String, - mimeType: String + mimeType: String, + extraFields: [(name: String, value: Data)] ) throws -> (InputStream, Int) { - let preamble = Data( - ("--\(boundary)\r\n" - + "Content-Disposition: form-data; name=\"file\"; filename=\"\(filename)\"\r\n" - + "Content-Type: \(mimeType)\r\n\r\n").utf8 - ) + // Serialize the non-file parts (post, additionalData) into the preamble + // ahead of the streamed file. They are small, so keeping them in memory is + // fine; `contentLength` counts them via `preamble.count`. Field values are + // appended as raw bytes (not through String) so a non-UTF-8 value is + // forwarded verbatim rather than coerced to empty. + 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(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)) 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 { - throw MediaUploadError.streamReadFailed + throw UploadError.streamReadFailed } let contentLength = preamble.count + fileSize + epilogue.count @@ -431,7 +478,7 @@ class DefaultMediaUploader: @unchecked Sendable { guard let inputStream = readStream, let outputStream = writeStream else { try? fileHandle.close() - throw MediaUploadError.streamReadFailed + throw UploadError.streamReadFailed } outputStream.open() @@ -483,54 +530,3 @@ class DefaultMediaUploader: @unchecked Sendable { } } -/// WordPress REST API media response (subset of fields). -private struct WPMediaResponse: Decodable { - let id: Int - let source_url: String - let alt_text: String? - let caption: RenderedField? - let title: RenderedField - let mime_type: String - let media_type: String - let media_details: MediaDetails? - - struct RenderedField: Decodable { - let rendered: String - } - - struct MediaDetails: Decodable { - let width: Int? - let height: Int? - } -} - -/// Errors specific to the native media upload pipeline. -enum MediaUploadError: Error, LocalizedError { - /// The WordPress REST API returned a non-success HTTP status code. - case uploadFailed(statusCode: Int, preview: String) - - /// The WordPress REST API returned a non-JSON response (e.g. HTML error page). - case unexpectedResponse(preview: String, underlyingError: Error) - - /// Failed to read the file for streaming upload. - case streamReadFailed - - var errorDescription: String? { - switch self { - case .uploadFailed(let statusCode, let preview): - return "Upload failed (\(statusCode)): \(preview)" - case .unexpectedResponse(let preview, _): - return "WordPress returned an unexpected response: \(preview)" - case .streamReadFailed: - return "Failed to read file for upload" - } - } -} - -// MARK: - Helpers - -private extension Data { - mutating func append(_ string: String) { - append(Data(string.utf8)) - } -} diff --git a/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift b/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift index 91b53fd56..616c5053d 100644 --- a/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift +++ b/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift @@ -34,62 +34,30 @@ public struct RESTAPIRepository: Sendable { if let customEndpoint = configuration.editorSettingsEndpoint { self.editorSettingsUrl = customEndpoint } else { - self.editorSettingsUrl = Self.buildNamespacedURL( + self.editorSettingsUrl = WordPressRESTURL.namespaced( apiRoot: apiRoot, path: Constants.API.editorSettingsPath, namespace: configuration.siteApiNamespace.first ) } - self.activeThemeUrl = Self.buildNamespacedURL( + self.activeThemeUrl = WordPressRESTURL.namespaced( apiRoot: apiRoot, path: Constants.API.activeThemePath, namespace: configuration.siteApiNamespace.first ) - self.siteSettingsUrl = Self.buildNamespacedURL( + self.siteSettingsUrl = WordPressRESTURL.namespaced( apiRoot: apiRoot, path: Constants.API.siteSettingsPath, namespace: configuration.siteApiNamespace.first ) - self.postTypesUrl = Self.buildNamespacedURL( + self.postTypesUrl = WordPressRESTURL.namespaced( apiRoot: apiRoot, path: Constants.API.postTypesPath, namespace: configuration.siteApiNamespace.first ) } - /// Builds a URL by inserting the namespace after the version segment of the path. - /// For example: `/wp/v2/posts` with namespace `sites/123/` becomes `/wp/v2/sites/123/posts` - private static func buildNamespacedURL(apiRoot: URL, path: String, namespace: String?) -> URL { - guard let rawNamespace = namespace else { - return apiRoot.appending(rawPath: path) - } - - let namespace = rawNamespace.hasSuffix("/") ? rawNamespace : rawNamespace + "/" - - // Parse the path to find where to insert the namespace - // Path format is typically: /prefix/version/endpoint (e.g., /wp/v2/posts or /wp-block-editor/v1/settings) - let components = path.split(separator: "/", omittingEmptySubsequences: true) - guard components.count >= 2 else { - return apiRoot.appending(rawPath: path) - } - - // Insert namespace after the version segment (second component) - // e.g., /wp-block-editor/v1/settings -> /wp-block-editor/v1/sites/123/settings - let prefix = components[0] - let version = components[1] - let remainder = components.dropFirst(2).joined(separator: "/") - - let namespacedPath: String - if remainder.isEmpty { - namespacedPath = "/\(prefix)/\(version)/\(namespace)" - } else { - namespacedPath = "/\(prefix)/\(version)/\(namespace)\(remainder)" - } - - return apiRoot.appending(rawPath: namespacedPath) - } - /// Clears all cached API responses. public func purge() throws { try self.cache.clear() @@ -110,7 +78,7 @@ public struct RESTAPIRepository: Sendable { private func buildPostUrl(id: Int) -> URL { let restNamespace = configuration.postType.restNamespace let restBase = configuration.postType.restBase - return Self.buildNamespacedURL( + return WordPressRESTURL.namespaced( apiRoot: configuration.siteApiRoot, path: "/\(restNamespace)/\(restBase)/\(id)", namespace: configuration.siteApiNamespace.first @@ -155,7 +123,7 @@ public struct RESTAPIRepository: Sendable { } private func buildPostTypeUrl(type: String) -> URL { - Self.buildNamespacedURL( + WordPressRESTURL.namespaced( apiRoot: configuration.siteApiRoot, path: "/wp/v2/types/\(type)", namespace: configuration.siteApiNamespace.first diff --git a/ios/Sources/GutenbergKit/Sources/WordPressRESTURL.swift b/ios/Sources/GutenbergKit/Sources/WordPressRESTURL.swift new file mode 100644 index 000000000..60b55a998 --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/WordPressRESTURL.swift @@ -0,0 +1,38 @@ +import Foundation + +/// Single source of truth for building namespaced WordPress REST API URLs, so the +/// media endpoint and every ``RESTAPIRepository`` endpoint normalize the site API +/// root and namespace identically (no drift). +enum WordPressRESTURL { + /// Builds a URL by inserting the site API namespace after the version segment + /// of the path. For example, `/wp/v2/posts` with namespace `sites/123` becomes + /// `/wp/v2/sites/123/posts`. A `nil` namespace appends the path unchanged. + /// + /// Trailing slashes on the root and namespace are normalized, so an unslashed + /// `apiRoot` or `namespace` still joins cleanly. + static func namespaced(apiRoot: URL, path: String, namespace: String?) -> URL { + guard let rawNamespace = namespace else { + return apiRoot.appending(rawPath: path) + } + + let namespace = rawNamespace.hasSuffix("/") ? rawNamespace : rawNamespace + "/" + + // Path format is typically /prefix/version/endpoint + // (e.g. /wp/v2/posts or /wp-block-editor/v1/settings). + let components = path.split(separator: "/", omittingEmptySubsequences: true) + guard components.count >= 2 else { + return apiRoot.appending(rawPath: path) + } + + // Insert the namespace after the version segment (second component). + let prefix = components[0] + let version = components[1] + let remainder = components.dropFirst(2).joined(separator: "/") + + let namespacedPath = remainder.isEmpty + ? "/\(prefix)/\(version)/\(namespace)" + : "/\(prefix)/\(version)/\(namespace)\(remainder)" + + return apiRoot.appending(rawPath: namespacedPath) + } +} diff --git a/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift b/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift new file mode 100644 index 000000000..db48652cf --- /dev/null +++ b/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift @@ -0,0 +1,41 @@ +import Foundation + +/// CORS behavior for an ``HTTPServer``. +public enum CORSPolicy: Sendable { + /// No CORS headers are added (the default). + case none + + /// Permissive CORS for a loopback-only server serving a WebView: allows any + /// origin and the methods/headers this library's clients use. The server + /// answers OPTIONS preflight requests itself and stamps these headers on + /// every response — including ones it generates internally (timeouts, parse + /// errors) that never reach the handler. + case permissive + + /// Headers added to every response under this policy. + var responseHeaders: [(String, String)] { + switch self { + case .none: + [] + case .permissive: + [ + ("Access-Control-Allow-Origin", "*"), + ("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS"), + ("Access-Control-Allow-Headers", "Authorization, Relay-Authorization, Content-Type"), + ("Access-Control-Max-Age", "86400"), + ] + } + } +} + +extension HTTPResponse { + /// Returns a copy with `newHeaders` appended, skipping any whose name + /// (case-insensitive) is already present. + func addingHeadersIfAbsent(_ newHeaders: [(String, String)]) -> HTTPResponse { + guard !newHeaders.isEmpty else { return self } + let existing = Set(headers.map { $0.0.lowercased() }) + let toAdd = newHeaders.filter { !existing.contains($0.0.lowercased()) } + guard !toAdd.isEmpty else { return self } + return HTTPResponse(status: status, statusText: statusText, headers: headers + toAdd, body: body) + } +} diff --git a/ios/Sources/GutenbergKitHTTP/HTTPRequestParser.swift b/ios/Sources/GutenbergKitHTTP/HTTPRequestParser.swift index 07f188ea3..ddfc46c2c 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPRequestParser.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPRequestParser.swift @@ -137,11 +137,11 @@ public final class HTTPRequestParser: @unchecked Sendable { try lock.withLock { guard _state.hasHeaders else { return nil } - // Payload-too-large means "valid headers, rejected body" — let - // the caller access the parsed headers so the handler can build - // a response (e.g., with CORS headers). Other parse errors - // indicate genuinely malformed requests and are still thrown. - if let error = _parseError, error != .payloadTooLarge { + // Recoverable errors (e.g. payloadTooLarge — valid headers, rejected + // body) are surfaced to the caller so the handler can build a + // response. Fatal errors indicate genuinely malformed requests and + // are thrown, closing the connection before the handler runs. + if let error = _parseError, error.disposition == .fatal { throw error } diff --git a/ios/Sources/GutenbergKitHTTP/HTTPRequestSerializer.swift b/ios/Sources/GutenbergKitHTTP/HTTPRequestSerializer.swift index 2e90b7c89..086b89f98 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPRequestSerializer.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPRequestSerializer.swift @@ -1,7 +1,7 @@ import Foundation /// Errors thrown when parsing an HTTP/1.1 request fails due to RFC 7230/9112 violations. -public enum HTTPRequestParseError: Error, Sendable, Equatable, LocalizedError { +public enum HTTPRequestParseError: Error, Sendable, Equatable, LocalizedError, CaseIterable { /// The header section before `\r\n\r\n` is empty (e.g., `\r\n\r\n` with no request line). case emptyHeaderSection /// The request line does not contain at least a method and target (RFC 9112 §3). @@ -37,6 +37,34 @@ public enum HTTPRequestParseError: Error, Sendable, Equatable, LocalizedError { /// An I/O error occurred while buffering the request (e.g. disk full). case bufferIOError + /// How the parser disposes of a parse error. + public enum Disposition: Sendable { + /// Abort the connection; the malformed request never reaches the handler. + case fatal + /// Surface to the handler via ``HTTPRequestParser/parseError`` so it can + /// build a response. + case recoverable + } + + /// Whether this error aborts the connection (``Disposition/fatal``) or is + /// surfaced to the handler (``Disposition/recoverable``). + /// + /// Only genuinely recoverable errors — where the request line and headers are + /// well-formed — may be recoverable; anything smuggling-relevant (framing, + /// Content-Length) must stay fatal so the request never reaches the handler. + public var disposition: Disposition { + switch self { + case .payloadTooLarge: + return .recoverable + case .emptyHeaderSection, .malformedRequestLine, .obsFoldDetected, + .whitespaceBeforeColon, .invalidContentLength, .conflictingContentLength, + .unsupportedTransferEncoding, .invalidHTTPVersion, .invalidFieldName, + .invalidFieldValue, .missingHostHeader, .multipleHostHeaders, + .headersTooLarge, .tooManyHeaders, .invalidEncoding, .bufferIOError: + return .fatal + } + } + /// The HTTP status code that should be sent for this error. public var httpStatus: Int { switch self { diff --git a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift index 83bf016a2..2a85781c2 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift @@ -157,6 +157,7 @@ public final class HTTPServer: Sendable { maxConnections: Int = HTTPServer.defaultMaxConnections, readTimeout: Duration = HTTPServer.defaultReadTimeout, idleTimeout: Duration = HTTPServer.defaultIdleTimeout, + cors: CORSPolicy = .none, handler: @escaping @Sendable (HTTPServer.Request) async -> HTTPResponse ) async throws -> HTTPServer { // Sanitize to prevent path traversal — only allow safe filename characters. @@ -196,7 +197,7 @@ public final class HTTPServer: Sendable { connection, queue: queue, token: token, requiresAuthentication: requiresAuth, maxRequestBodySize: maxRequestBodySize, readTimeout: readTimeout, - idleTimeout: idleTimeout, tempDirectory: tempDirectory, + idleTimeout: idleTimeout, cors: cors, tempDirectory: tempDirectory, connectionCounter: connectionCounter, connectionTasks: connectionTasks, handler: handler ) } @@ -258,6 +259,7 @@ public final class HTTPServer: Sendable { maxRequestBodySize: Int64, readTimeout: Duration, idleTimeout: Duration, + cors: CORSPolicy, tempDirectory: URL, connectionCounter: ConnectionCounter, connectionTasks: ConnectionTasks, @@ -336,21 +338,28 @@ public final class HTTPServer: Sendable { } } - let response = await handler(Request(parsed: request, parseDuration: duration, serverError: parser.parseError)) - await send(response, on: connection) + // Under a permissive CORS policy the library answers the OPTIONS + // preflight itself; the send layer stamps the CORS headers. + let response: HTTPResponse + if cors == .permissive, request.method.uppercased() == "OPTIONS" { + response = HTTPResponse(status: 204) + } else { + response = await handler(Request(parsed: request, parseDuration: duration, serverError: parser.parseError)) + } + 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 Logger.httpServer.debug("\(request.method) \(request.target) → \(response.status) (\(String(format: "%.1f", ms))ms)") } catch HTTPServerError.authenticationFailed { - await send(HTTPResponse(status: 407, headers: [("Content-Type", "text/plain"), ("Proxy-Authenticate", "Bearer")]), on: connection) + 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) + await send(HTTPResponse(status: 411, statusText: "Length Required", body: Data("Length Required".utf8)), on: connection, cors: cors) } catch is CancellationError { Logger.httpServer.debug("Connection cancelled during shutdown") connection.cancel() } catch HTTPServerError.readTimeout { Logger.httpServer.warning("Read timeout, closing connection") - await send(HTTPResponse(status: 408, statusText: "Request Timeout", body: Data("Request Timeout".utf8)), on: connection) + await send(HTTPResponse(status: 408, statusText: "Request Timeout", body: Data("Request Timeout".utf8)), on: connection, cors: cors) } catch let error as HTTPRequestParseError { Logger.httpServer.error("Parse error: \(error)") let statusText = String(error.httpStatusText) @@ -359,10 +368,10 @@ public final class HTTPServer: Sendable { statusText: statusText, body: Data(statusText.utf8) ) - await send(response, on: connection) + await send(response, 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) + await send(HTTPResponse(status: 400, statusText: "Bad Request", body: Data("Malformed HTTP request".utf8)), on: connection, cors: cors) } } connectionTasks.track(taskID, task) @@ -477,9 +486,10 @@ public final class HTTPServer: Sendable { } /// Sends a response on the connection and then closes it. - private static func send(_ response: HTTPResponse, on connection: NWConnection) async { + private static func send(_ response: HTTPResponse, on connection: NWConnection, cors: CORSPolicy) async { + let decorated = response.addingHeadersIfAbsent(cors.responseHeaders) await withCheckedContinuation { (continuation: CheckedContinuation) in - connection.send(content: response.serialized(), completion: .contentProcessed { _ in + connection.send(content: decorated.serialized(), completion: .contentProcessed { _ in connection.cancel() continuation.resume() }) diff --git a/ios/Sources/GutenbergKitHTTP/ParsedHTTPRequest.swift b/ios/Sources/GutenbergKitHTTP/ParsedHTTPRequest.swift index d68774921..95222d1c0 100644 --- a/ios/Sources/GutenbergKitHTTP/ParsedHTTPRequest.swift +++ b/ios/Sources/GutenbergKitHTTP/ParsedHTTPRequest.swift @@ -32,6 +32,29 @@ extension ParsedHTTPRequest { } } + /// The path portion of ``target``, without the query component + /// (e.g., "/wp/v2/posts" for "/wp/v2/posts?per_page=10"). + /// + /// Use this for routing — matching against ``target`` fails as soon as a + /// client appends a query string. + public var path: String { + let target = target + guard let separator = target.firstIndex(of: "?") else { return target } + return String(target.prefix(upTo: separator)) + } + + /// The query component of ``target``, including the leading "?" + /// (e.g., "?per_page=10"), or an empty string when there is no query. + /// + /// A bare trailing "?" carries no parameters and yields an empty string, so + /// the value can be appended to an upstream URL unconditionally. + public var query: String { + let target = target + guard let separator = target.firstIndex(of: "?") else { return "" } + let value = String(target[target.index(after: separator)...]) + return value.isEmpty ? "" : "?\(value)" + } + /// The HTTP-version from the request line (e.g., "HTTP/1.1"), per RFC 9112 §2.3. public var httpVersion: String { switch self { diff --git a/ios/Tests/GutenbergKitHTTPTests/FixtureTests.swift b/ios/Tests/GutenbergKitHTTPTests/FixtureTests.swift index 22feadbd0..0acc1726c 100644 --- a/ios/Tests/GutenbergKitHTTPTests/FixtureTests.swift +++ b/ios/Tests/GutenbergKitHTTPTests/FixtureTests.swift @@ -255,17 +255,18 @@ struct RequestParsingFixtureTests { let expectedError = testCase.expected.error do { _ = try parser.parseRequest() - // Non-fatal errors (e.g., payloadTooLarge) are exposed via - // parseError instead of being thrown. + // A recoverable error is surfaced via `parseError` instead of thrown. if let parseError = parser.parseError { let errorName = String(describing: parseError) #expect(errorName == expectedError, "\(testCase.description): expected \(expectedError) but got \(errorName)") + #expect(parseError.disposition == .recoverable, "\(testCase.description): \(errorName) surfaced via parseError but is not recoverable") } else { Issue.record("Expected error \(expectedError) but parsing succeeded — \(testCase.description)") } } catch { let errorName = String(describing: error) #expect(errorName == expectedError, "\(testCase.description): expected \(expectedError) but got \(errorName)") + #expect((error as? HTTPRequestParseError)?.disposition == .fatal, "\(testCase.description): \(errorName) was thrown but is not fatal") } } diff --git a/ios/Tests/GutenbergKitHTTPTests/HTTPRequestParserTests.swift b/ios/Tests/GutenbergKitHTTPTests/HTTPRequestParserTests.swift index c4dc5366c..43b255352 100644 --- a/ios/Tests/GutenbergKitHTTPTests/HTTPRequestParserTests.swift +++ b/ios/Tests/GutenbergKitHTTPTests/HTTPRequestParserTests.swift @@ -5,6 +5,17 @@ import Testing @Suite("HTTPRequestParser") struct HTTPRequestParserTests { + // MARK: - Error Disposition + + /// Locks the fatal/recoverable classification so a refactor can't silently + /// make a smuggling-relevant error recoverable — which would let a malformed + /// request reach the handler before auth. + @Test("only payloadTooLarge is recoverable") + func onlyPayloadTooLargeIsRecoverable() { + let recoverable = HTTPRequestParseError.allCases.filter { $0.disposition == .recoverable } + #expect(recoverable == [.payloadTooLarge]) + } + // MARK: - Basic Request Parsing @Test("parses a simple GET request") diff --git a/ios/Tests/GutenbergKitHTTPTests/ParsedHTTPRequestTests.swift b/ios/Tests/GutenbergKitHTTPTests/ParsedHTTPRequestTests.swift index 2d37a9abe..82e17b2da 100644 --- a/ios/Tests/GutenbergKitHTTPTests/ParsedHTTPRequestTests.swift +++ b/ios/Tests/GutenbergKitHTTPTests/ParsedHTTPRequestTests.swift @@ -5,6 +5,47 @@ import Testing @Suite("ParsedHTTPRequest") struct ParsedHTTPRequestTests { + // MARK: - path / query + + @Test( + "path and query split the target", + arguments: [ + ("/upload", "/upload", ""), + ("/upload?_embed=wp:featuredmedia", "/upload", "?_embed=wp:featuredmedia"), + // A bare "?" carries no parameters, so the query is empty. + ("/upload?", "/upload", ""), + ("/wp/v2/posts?per_page=10&page=2", "/wp/v2/posts", "?per_page=10&page=2"), + // Only the first "?" separates path from query; later ones belong to it. + ("/search?q=a?b", "/search", "?q=a?b"), + ("/", "/", ""), + ] + ) + func pathAndQuery(target: String, expectedPath: String, expectedQuery: String) { + let request = ParsedHTTPRequest.complete( + method: "POST", + target: target, + httpVersion: "HTTP/1.1", + headers: [:], + body: nil + ) + + #expect(request.path == expectedPath) + #expect(request.query == expectedQuery) + } + + @Test("path and query are available on a partial request") + func pathAndQueryOnPartial() { + let request = ParsedHTTPRequest.partial( + method: "POST", + target: "/upload?_embed=wp:featuredmedia", + httpVersion: "HTTP/1.1", + headers: [:] + ) + + #expect(request.path == "/upload") + #expect(request.query == "?_embed=wp:featuredmedia") + } + // MARK: - urlRequest(relativeTo:) @Test("urlRequest resolves path against base URL") diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 02f2f93e7..1d29e4ddc 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -100,6 +100,37 @@ struct MediaUploadServerTests { #expect(httpResponse.statusCode == 404) } + @Test("routes /upload with a query string and relays the query") + func uploadWithQueryString() async throws { + let delegate = ProcessOnlyDelegate() + let mockUploader = MockDefaultUploader() + let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + defer { server.stop() } + + // `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, + // so the middleware forwards that query on to the native server. Routing must + // match on the path alone, and the query must reach WordPress unchanged. + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8)) + + let url = URL(string: "http://127.0.0.1:\(server.port)/upload?_embed=wp:featuredmedia")! + 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) + // The delegate returns `.original`, so this is the passthrough branch. + // Pin which branch ran — `lastQuery` is recorded by both, so without this + // the query assertion would pass even if routing collapsed onto one path. + #expect(mockUploader.passthroughUploadCalled) + #expect(!mockUploader.uploadCalled) + #expect(mockUploader.lastQuery == "?_embed=wp:featuredmedia") + } + @Test("calls delegate and returns upload result") func delegateProcessAndUpload() async throws { let delegate = MockUploadDelegate() @@ -119,17 +150,19 @@ struct MediaUploadServerTests { let (data, response) = try await URLSession.shared.data(for: request) let httpResponse = try #require(response as? HTTPURLResponse) - #expect(httpResponse.statusCode == 200) + #expect(httpResponse.statusCode == 201) #expect(delegate.processFileCalled) #expect(delegate.uploadFileCalled) #expect(delegate.lastMimeType == "image/jpeg") #expect(delegate.lastFilename == "photo.jpg") - let result = try JSONDecoder().decode(MediaUploadResult.self, from: data) - #expect(result.id == 42) - #expect(result.url == "https://example.com/photo.jpg") - #expect(result.type == "image") + // The server relays WordPress's raw response body verbatim. + let object = try JSONSerialization.jsonObject(with: data) + let json = try #require(object as? [String: Any]) + #expect(json["id"] as? Int == 42) + #expect(json["source_url"] as? String == "https://example.com/photo.jpg") + #expect(json["media_type"] as? String == "image") } @Test("uses passthrough when delegate does not modify file") @@ -152,15 +185,69 @@ struct MediaUploadServerTests { let (data, response) = try await URLSession.shared.data(for: request) let httpResponse = try #require(response as? HTTPURLResponse) - #expect(httpResponse.statusCode == 200) + #expect(httpResponse.statusCode == 201) #expect(delegate.processFileCalled) // Passthrough: original body forwarded directly, not re-encoded. #expect(mockUploader.passthroughUploadCalled) #expect(!mockUploader.uploadCalled) - let result = try JSONDecoder().decode(MediaUploadResult.self, from: data) - #expect(result.id == 99) + // The server relays WordPress's raw response body verbatim. + let object = try JSONSerialization.jsonObject(with: data) + let json = try #require(object as? [String: Any]) + #expect(json["id"] as? Int == 99) + } + + @Test("forwards the delegate's processed metadata to the uploader") + func processedMetadataForwarded() async throws { + let delegate = ResizingDelegate() + 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 + + _ = try await URLSession.shared.data(for: request) + + // The delegate changed the format, so the uploader must receive the new + // metadata — not the original video/quicktime + clip.mov. + #expect(mockUploader.uploadCalled) + #expect(mockUploader.lastUploadMimeType == "video/mp4") + #expect(mockUploader.lastUploadFilename == "clip.mp4") + } + + @Test("deletes the delegate's processed file after upload") + func deletesProcessedFile() async throws { + let delegate = ResizingDelegate() + 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 + + _ = try await URLSession.shared.data(for: request) + + // The server owns the file the delegate produced and must delete it once the + // upload finishes — the defer in processAndUpload covers the success and throw + // paths alike. A leaked processed file is a full-size temp per upload. + let processedURL = try #require(delegate.producedURL) + #expect(!FileManager.default.fileExists(atPath: processedURL.path(percentEncoded: false))) } @Test("returns 413 with CORS headers when request body exceeds max size") @@ -188,6 +275,54 @@ struct MediaUploadServerTests { #expect(responseBody.contains("too large")) } + @Test("startup sweep deletes stale upload temps but preserves fresh ones") + func cleanOrphanedUploadsAgeThreshold() async throws { + let dir = FileManager.default.temporaryDirectory + .appending(component: "GutenbergKit-uploads", directoryHint: .isDirectory) + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + + let stale = dir.appending(component: "stale-\(UUID().uuidString)") + let fresh = dir.appending(component: "fresh-\(UUID().uuidString)") + try Data("x".utf8).write(to: stale) + try Data("y".utf8).write(to: fresh) + defer { + try? FileManager.default.removeItem(at: stale) + try? FileManager.default.removeItem(at: fresh) + } + // Backdate the stale file well past the 1-hour cutoff. + try FileManager.default.setAttributes( + [.modificationDate: Date(timeIntervalSinceNow: -7200)], + ofItemAtPath: stale.path(percentEncoded: false) + ) + + // start() runs cleanOrphanedUploads(). The sweep must delete the aged file and + // keep the fresh one — a flipped comparison would do the opposite and wipe an + // in-flight upload. + let server = try await MediaUploadServer.start() + server.stop() + + #expect(!FileManager.default.fileExists(atPath: stale.path(percentEncoded: false))) + #expect(FileManager.default.fileExists(atPath: fresh.path(percentEncoded: false))) + } + + @Test("does not strongly retain the upload delegate (weak — preserves deinit teardown)") + func doesNotStronglyRetainDelegate() async throws { + weak var weakDelegate: MockUploadDelegate? + let server: MediaUploadServer + do { + let delegate = MockUploadDelegate() + weakDelegate = delegate + server = try await MediaUploadServer.start(uploadDelegate: delegate) + } + defer { server.stop() } + + // UploadContext holds the delegate weakly, so releasing the host's strong + // reference deallocates it. A strong reference here would reintroduce the + // EditorViewController → uploadServer → … → delegate → EditorViewController + // cycle, so deinit would never fire and the server would never stop. + #expect(weakDelegate == nil) + } + private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data) -> Data { var body = Data() body.append("--\(boundary)\r\n") @@ -225,7 +360,7 @@ struct MultipartBodyStreamTests { // Build streaming output. let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( - fileURL: tempFile, boundary: boundary, filename: filename, mimeType: mimeType + fileURL: tempFile, boundary: boundary, filename: filename, mimeType: mimeType, extraFields: [] ) #expect(contentLength == expected.count) @@ -233,6 +368,67 @@ struct MultipartBodyStreamTests { #expect(result == expected) } + @Test("includes non-file parts (e.g. post) ahead of the file") + func multipartBodyIncludesExtraParts() throws { + let boundary = "boundary" + let filename = "photo.jpg" + let mimeType = "image/jpeg" + let fileContent = Data("image bytes".utf8) + let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent("stream-extra-\(UUID().uuidString)") + try fileContent.write(to: tempFile) + defer { try? FileManager.default.removeItem(at: tempFile) } + + var expected = Data() + expected.append(Data("--\(boundary)\r\n".utf8)) + expected.append(Data("Content-Disposition: form-data; name=\"post\"\r\n\r\n".utf8)) + expected.append(Data("123\r\n".utf8)) + expected.append(Data("--\(boundary)\r\n".utf8)) + expected.append(Data("Content-Disposition: form-data; name=\"file\"; filename=\"\(filename)\"\r\n".utf8)) + expected.append(Data("Content-Type: \(mimeType)\r\n\r\n".utf8)) + expected.append(fileContent) + expected.append(Data("\r\n--\(boundary)--\r\n".utf8)) + + let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( + fileURL: tempFile, boundary: boundary, filename: filename, mimeType: mimeType, + extraFields: [("post", Data("123".utf8))] + ) + #expect(contentLength == expected.count) + #expect(readAllFromStream(stream) == expected) + } + + @Test("forwards a non-UTF-8 field value verbatim") + func multipartBodyPreservesNonUTF8FieldValue() throws { + let boundary = "boundary" + let filename = "photo.jpg" + let mimeType = "image/jpeg" + let fileContent = Data("image bytes".utf8) + let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent("stream-binary-\(UUID().uuidString)") + try fileContent.write(to: tempFile) + defer { try? FileManager.default.removeItem(at: tempFile) } + + // A field value that is not valid UTF-8 (a lone 0xFF byte between ASCII bytes). + let binaryValue = Data([0x61, 0xFF, 0x62]) + + var expected = Data() + expected.append(Data("--\(boundary)\r\n".utf8)) + expected.append(Data("Content-Disposition: form-data; name=\"blob\"\r\n\r\n".utf8)) + expected.append(binaryValue) + expected.append(Data("\r\n".utf8)) + expected.append(Data("--\(boundary)\r\n".utf8)) + expected.append(Data("Content-Disposition: form-data; name=\"file\"; filename=\"\(filename)\"\r\n".utf8)) + expected.append(Data("Content-Type: \(mimeType)\r\n\r\n".utf8)) + expected.append(fileContent) + expected.append(Data("\r\n--\(boundary)--\r\n".utf8)) + + let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( + fileURL: tempFile, boundary: boundary, filename: filename, mimeType: mimeType, + extraFields: [("blob", binaryValue)] + ) + #expect(contentLength == expected.count) + // The raw 0xFF byte survives — it was not coerced through String. + #expect(readAllFromStream(stream) == expected) + } + @Test("content length matches actual stream output for larger files") func contentLengthAccurate() throws { let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent("stream-test-\(UUID().uuidString)") @@ -241,7 +437,7 @@ struct MultipartBodyStreamTests { defer { try? FileManager.default.removeItem(at: tempFile) } let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( - fileURL: tempFile, boundary: "boundary", filename: "big.bin", mimeType: "application/octet-stream" + fileURL: tempFile, boundary: "boundary", filename: "big.bin", mimeType: "application/octet-stream", extraFields: [] ) let result = readAllFromStream(stream) @@ -249,6 +445,104 @@ struct MultipartBodyStreamTests { } } +// MARK: - DefaultMediaUploader Relay Tests + +@Suite("DefaultMediaUploader relay") +struct DefaultMediaUploaderRelayTests { + + @Test("relays a non-2xx WordPress response instead of throwing") + func relaysErrorResponseVerbatim() async throws { + // A WordPress REST error body, returned with a non-2xx status. + let errorBody = Data(#"{"code":"rest_cannot_create","message":"Sorry, you are not allowed to upload this file type."}"#.utf8) + let client = RelayStubHTTPClient(statusCode: 403, body: errorBody) + let uploader = DefaultMediaUploader(httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json/")!) + + let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent("relay-\(UUID().uuidString).jpg") + try Data("fake image".utf8).write(to: tempFile) + defer { try? FileManager.default.removeItem(at: tempFile) } + + // performUpload must route through performRaw, which does NOT validate status, + // so WordPress's 403 + body flow through verbatim. A revert to perform() would + // throw on the non-2xx (RelayStubHTTPClient.perform mirrors that), failing here. + let response = try await uploader.upload( + fileURL: tempFile, mimeType: "image/jpeg", filename: "photo.jpg", extraParts: [], query: "" + ) + + #expect(response.statusCode == 403) + #expect(response.body == errorBody) + } + + @Test("carries the namespace and request query through to the media endpoint") + func forwardsNamespaceAndQuery() async throws { + let client = URLCapturingHTTPClient() + let uploader = DefaultMediaUploader( + httpClient: client, + siteApiRoot: URL(string: "https://example.com/wp-json")!, + siteApiNamespace: ["sites/123"] + ) + let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent("query-\(UUID().uuidString).jpg") + try Data("img".utf8).write(to: tempFile) + defer { try? FileManager.default.removeItem(at: tempFile) } + + _ = try await uploader.upload( + fileURL: tempFile, mimeType: "image/jpeg", filename: "photo.jpg", + extraParts: [], query: "?_embed=wp:featuredmedia" + ) + + // Namespace inserted via the shared builder, and the query preserved verbatim — + // including the `:`, which would make URL(string:) return nil and drop it (#6). + let url = try #require(client.lastURL) + #expect(url.absoluteString == "https://example.com/wp-json/wp/v2/sites/123/media?_embed=wp:featuredmedia") + } +} + +/// An HTTP client whose `performRaw` relays a canned response without validating +/// status, while `perform` throws on a non-2xx — mirroring the real +/// `EditorHTTPClient`. Lets a test prove `DefaultMediaUploader` routes uploads +/// through `performRaw` (relay) rather than `perform` (throw). +private struct RelayStubHTTPClient: EditorHTTPClientProtocol { + let statusCode: Int + let body: Data + + func perform(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { + let response = HTTPURLResponse(url: urlRequest.url!, statusCode: statusCode, httpVersion: nil, headerFields: nil)! + guard (200...299).contains(statusCode) else { + throw NSError(domain: "RelayStubHTTPClient", code: statusCode) + } + return (body, response) + } + + func performRaw(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { + let response = HTTPURLResponse(url: urlRequest.url!, statusCode: statusCode, httpVersion: nil, headerFields: nil)! + return (body, response) + } + + func download(_ urlRequest: URLRequest) async throws -> (URL, HTTPURLResponse) { + let response = HTTPURLResponse(url: urlRequest.url!, statusCode: statusCode, httpVersion: nil, headerFields: nil)! + return (FileManager.default.temporaryDirectory, response) + } +} + +/// Captures the URL of the last request so a test can assert the media endpoint +/// URL (namespace + query) the uploader built. +private final class URLCapturingHTTPClient: EditorHTTPClientProtocol, @unchecked Sendable { + private let lock = NSLock() + private var _lastURL: URL? + var lastURL: URL? { lock.withLock { _lastURL } } + + private func ok(_ urlRequest: URLRequest) -> (Data, HTTPURLResponse) { + lock.withLock { _lastURL = urlRequest.url } + return (Data("{}".utf8), HTTPURLResponse(url: urlRequest.url!, statusCode: 201, httpVersion: nil, headerFields: nil)!) + } + + func perform(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { ok(urlRequest) } + func performRaw(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { ok(urlRequest) } + func download(_ urlRequest: URLRequest) async throws -> (URL, HTTPURLResponse) { + let (_, response) = ok(urlRequest) + return (FileManager.default.temporaryDirectory, response) + } +} + // MARK: - Helpers /// Reads all bytes from an InputStream using `read()` return value as @@ -284,26 +578,21 @@ private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable var lastMimeType: String? { lock.withLock { _lastMimeType } } var lastFilename: String? { lock.withLock { _lastFilename } } - func processFile(at url: URL, mimeType: String) async throws -> URL { + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { lock.withLock { _processFileCalled = true _lastMimeType = mimeType } - return url + return .original } - func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResult? { + func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { lock.withLock { _uploadFileCalled = true _lastFilename = filename } - return MediaUploadResult( - id: 42, - url: "https://example.com/photo.jpg", - title: "photo", - mime: "image/jpeg", - type: "image" - ) + let json = #"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"# + return MediaUploadResponse(statusCode: 201, body: Data(json.utf8)) } } @@ -313,9 +602,25 @@ private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendabl var processFileCalled: Bool { lock.withLock { _processFileCalled } } - func processFile(at url: URL, mimeType: String) async throws -> URL { + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { lock.withLock { _processFileCalled = true } - return url + 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() + private var _producedURL: URL? + + /// The URL of the processed file this delegate wrote, for cleanup assertions. + var producedURL: URL? { lock.withLock { _producedURL } } + + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + let newURL = url.deletingLastPathComponent().appending(component: "processed-\(UUID().uuidString)") + try Data("processed".utf8).write(to: newURL) + lock.withLock { _producedURL = newURL } + return .processed(newURL, mimeType: "video/mp4", filename: "clip.mp4") } } @@ -323,32 +628,41 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab private let lock = NSLock() private var _uploadCalled = false private var _passthroughUploadCalled = false + private var _lastUploadMimeType: String? + private var _lastUploadFilename: String? + private var _lastQuery: String? var uploadCalled: Bool { lock.withLock { _uploadCalled } } var passthroughUploadCalled: Bool { lock.withLock { _passthroughUploadCalled } } + var lastUploadMimeType: String? { lock.withLock { _lastUploadMimeType } } + var lastUploadFilename: String? { lock.withLock { _lastUploadFilename } } + var lastQuery: String? { lock.withLock { _lastQuery } } init() { super.init(httpClient: MockHTTPClient(), siteApiRoot: URL(string: "https://example.com/wp-json/")!) } - override func upload(fileURL: URL, mimeType: String, filename: String) async throws -> MediaUploadResult { - lock.withLock { _uploadCalled = true } - return mockResult() + override func upload(fileURL: URL, mimeType: String, filename: String, extraParts: [MultipartPart], query: String) async throws -> MediaUploadResponse { + lock.withLock { + _uploadCalled = true + _lastUploadMimeType = mimeType + _lastUploadFilename = filename + _lastQuery = query + } + return mockResponse() } - override func passthroughUpload(body: RequestBody, contentType: String) async throws -> MediaUploadResult { - lock.withLock { _passthroughUploadCalled = true } - return mockResult() + override func passthroughUpload(body: RequestBody, contentType: String, query: String) async throws -> MediaUploadResponse { + lock.withLock { + _passthroughUploadCalled = true + _lastQuery = query + } + return mockResponse() } - private func mockResult() -> MediaUploadResult { - MediaUploadResult( - id: 99, - url: "https://example.com/doc.pdf", - title: "doc", - mime: "application/pdf", - type: "file" - ) + private func mockResponse() -> MediaUploadResponse { + let json = #"{"id":99,"source_url":"https://example.com/doc.pdf","media_type":"file"}"# + return MediaUploadResponse(statusCode: 201, body: Data(json.utf8)) } } diff --git a/ios/Tests/GutenbergKitTests/WordPressRESTURLTests.swift b/ios/Tests/GutenbergKitTests/WordPressRESTURLTests.swift new file mode 100644 index 000000000..290d90a8a --- /dev/null +++ b/ios/Tests/GutenbergKitTests/WordPressRESTURLTests.swift @@ -0,0 +1,57 @@ +import Foundation +import Testing +@testable import GutenbergKit + +@Suite("WordPressRESTURL") +struct WordPressRESTURLTests { + + @Test("appends the path when no namespace is configured") + func noNamespace() { + let url = WordPressRESTURL.namespaced( + apiRoot: URL(string: "https://example.com/wp-json")!, + path: "/wp/v2/media", + namespace: nil + ) + #expect(url.absoluteString == "https://example.com/wp-json/wp/v2/media") + } + + @Test("inserts the namespace after the version segment") + func insertsNamespace() { + let url = WordPressRESTURL.namespaced( + apiRoot: URL(string: "https://example.com/wp-json")!, + path: "/wp/v2/media", + namespace: "sites/123/" + ) + #expect(url.absoluteString == "https://example.com/wp-json/wp/v2/sites/123/media") + } + + @Test("normalizes an unslashed root and namespace") + func normalizesUnslashed() { + let url = WordPressRESTURL.namespaced( + apiRoot: URL(string: "https://example.com/wp-json")!, // no trailing slash + path: "/wp/v2/media", + namespace: "sites/123" // no trailing slash + ) + #expect(url.absoluteString == "https://example.com/wp-json/wp/v2/sites/123/media") + } + + @Test("does not double the slash when the root already ends in one") + func trailingSlashRoot() { + let url = WordPressRESTURL.namespaced( + apiRoot: URL(string: "https://example.com/wp-json/")!, // trailing slash + path: "/wp/v2/media", + namespace: "sites/123" + ) + #expect(url.absoluteString == "https://example.com/wp-json/wp/v2/sites/123/media") + } + + @Test("inserts the namespace after a non-wp/v2 version segment") + func otherVersionSegment() { + let url = WordPressRESTURL.namespaced( + apiRoot: URL(string: "https://example.com/wp-json")!, + path: "/wp-block-editor/v1/settings", + namespace: "sites/123" + ) + #expect(url.absoluteString == "https://example.com/wp-json/wp-block-editor/v1/sites/123/settings") + } +} diff --git a/src/utils/api-fetch-upload-middleware.test.js b/src/utils/api-fetch-upload-middleware.test.js index 952a5baa8..07bac0062 100644 --- a/src/utils/api-fetch-upload-middleware.test.js +++ b/src/utils/api-fetch-upload-middleware.test.js @@ -202,89 +202,138 @@ describe( 'nativeMediaUploadMiddleware', () => { expect( options.body ).toBeInstanceOf( FormData ); } ); - it( 'transforms native response to WordPress REST API shape', async () => { + it( 'forwards the original body and query to the native server', async () => { getGBKit.mockReturnValue( { - nativeUploadPort: 8080, - nativeUploadToken: 'token', + nativeUploadPort: 12345, + nativeUploadToken: 'test-token', } ); global.fetch = vi.fn( () => Promise.resolve( { ok: true, - json: () => - Promise.resolve( { - id: 77, - url: 'https://example.com/image.jpg', - alt: 'alt text', - caption: 'a caption', - title: 'image', - mime: 'image/jpeg', - type: 'image', - } ), + json: () => Promise.resolve( { id: 1 } ), } ) ); - const result = await nativeMediaUploadMiddleware( - makePostMediaOptions( makeFile() ), + const body = new FormData(); + body.append( 'file', makeFile(), 'photo.jpg' ); + body.append( 'post', '123' ); + + await nativeMediaUploadMiddleware( + { + method: 'POST', + path: '/wp/v2/media?_embed=wp:featuredmedia', + body, + }, makeNext() ); - expect( result ).toEqual( { - id: 77, - source_url: 'https://example.com/image.jpg', - alt_text: 'alt text', - caption: { raw: 'a caption', rendered: 'a caption' }, - title: { raw: 'image', rendered: 'image' }, - mime_type: 'image/jpeg', - media_type: 'image', - media_details: { width: 0, height: 0 }, - link: 'https://example.com/image.jpg', + const [ url, fetchOptions ] = global.fetch.mock.calls[ 0 ]; + // The original query is appended so ?_embed reaches WordPress. + expect( url ).toBe( + 'http://localhost:12345/upload?_embed=wp:featuredmedia' + ); + // The original body is forwarded verbatim, so `post` survives. + expect( fetchOptions.body ).toBe( body ); + expect( fetchOptions.body.get( 'post' ) ).toBe( '123' ); + } ); + + it.each( [ + // A bare trailing `?` carries no parameters. The native `query` + // accessors normalize it away, so this side must too — otherwise the + // upload server receives a `?` the two platforms agree cannot exist. + [ '/wp/v2/media?', 'http://localhost:12345/upload' ], + [ '/wp/v2/media', 'http://localhost:12345/upload' ], + ] )( 'normalizes the query of %s to %s', async ( path, expectedUrl ) => { + getGBKit.mockReturnValue( { + nativeUploadPort: 12345, + nativeUploadToken: 'test-token', } ); + + global.fetch = vi.fn( () => + Promise.resolve( { + ok: true, + json: () => Promise.resolve( { id: 1 } ), + } ) + ); + + const body = new FormData(); + body.append( 'file', makeFile(), 'photo.jpg' ); + + await nativeMediaUploadMiddleware( + { method: 'POST', path, body }, + makeNext() + ); + + const [ url ] = global.fetch.mock.calls[ 0 ]; + expect( url ).toBe( expectedUrl ); } ); - it( 'handles missing optional fields in native response', async () => { + it( 'returns the relayed WordPress attachment unchanged', async () => { getGBKit.mockReturnValue( { nativeUploadPort: 8080, nativeUploadToken: 'token', } ); + // The native server relays WordPress's raw attachment response; the + // middleware must return it verbatim, not reshape it — so consumers get + // the real media_details.sizes, link, and raw/rendered fields. + const attachment = { + id: 77, + source_url: 'https://example.com/image.jpg', + alt_text: 'alt text', + caption: { raw: 'a caption', rendered: 'a caption' }, + title: { raw: 'image', rendered: 'image' }, + mime_type: 'image/jpeg', + media_type: 'image', + media_details: { + width: 4032, + height: 3024, + sizes: { + large: { + source_url: 'https://example.com/image-1024x768.jpg', + }, + }, + }, + link: 'https://example.com/image/', + }; + global.fetch = vi.fn( () => Promise.resolve( { ok: true, - json: () => - Promise.resolve( { - id: 1, - url: 'https://example.com/file.pdf', - title: 'file', - mime: 'application/pdf', - type: 'application', - } ), + json: () => Promise.resolve( attachment ), } ) ); const result = await nativeMediaUploadMiddleware( - makePostMediaOptions( makeFile( 'file.pdf', 'application/pdf' ) ), + makePostMediaOptions( makeFile() ), makeNext() ); - expect( result.alt_text ).toBe( '' ); - expect( result.caption ).toEqual( { raw: '', rendered: '' } ); + expect( result ).toEqual( attachment ); } ); // MARK: - Error handling - it( 'throws on non-ok response from local server', async () => { + it( 'rejects with the WordPress error body on a non-ok response', async () => { getGBKit.mockReturnValue( { nativeUploadPort: 8080, nativeUploadToken: 'token', } ); + // The native server relays WordPress's error status + JSON body; the + // middleware rejects with that body as-is (like @wordpress/api-fetch) so + // media-utils surfaces WordPress's real message. global.fetch = vi.fn( () => Promise.resolve( { ok: false, - status: 500, - statusText: 'Internal Server Error', - text: () => Promise.resolve( 'Server crashed' ), + status: 403, + json: () => + Promise.resolve( { + code: 'rest_cannot_create', + message: + 'Sorry, you are not allowed to upload this file type.', + } ), } ) ); @@ -294,19 +343,24 @@ describe( 'nativeMediaUploadMiddleware', () => { makeNext() ) ).rejects.toMatchObject( { - code: 'upload_failed', - message: expect.stringContaining( '500' ), + code: 'rest_cannot_create', + message: expect.stringContaining( 'not allowed' ), } ); } ); - it( 'throws on fetch network error', async () => { + it( 'rejects with invalid_json when the error body is not JSON', async () => { getGBKit.mockReturnValue( { nativeUploadPort: 8080, nativeUploadToken: 'token', } ); global.fetch = vi.fn( () => - Promise.reject( new Error( 'Failed to fetch' ) ) + Promise.resolve( { + ok: false, + status: 502, + json: () => + Promise.reject( new SyntaxError( 'Unexpected token' ) ), + } ) ); await expect( @@ -314,7 +368,111 @@ describe( 'nativeMediaUploadMiddleware', () => { makePostMediaOptions( makeFile() ), makeNext() ) - ).rejects.toBeDefined(); + ).rejects.toMatchObject( { code: 'invalid_json' } ); + } ); + + it( 'rejects with invalid_json when a 2xx response body is not JSON', async () => { + getGBKit.mockReturnValue( { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + } ); + + // A successful status but a non-JSON body (e.g. an HTML error page from + // an intermediary). json() rejects; the middleware must normalize it, not + // surface a raw SyntaxError. + global.fetch = vi.fn( () => + Promise.resolve( { + ok: true, + json: () => + Promise.reject( new SyntaxError( 'Unexpected token' ) ), + } ) + ); + + await expect( + nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + makeNext() + ) + ).rejects.toMatchObject( { code: 'invalid_json' } ); + } ); + + it( 'surfaces a transport failure instead of retrying (no silent duplicate)', async () => { + getGBKit.mockReturnValue( { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + } ); + const next = makeNext(); + + // A connection-level failure — the loopback server died out-of-band after + // a valid start (reachability is otherwise gated proactively upstream, so + // an unreachable server never advertises a port to fetch). + const connectionError = new TypeError( 'Failed to fetch' ); + global.fetch = vi.fn( () => Promise.reject( connectionError ) ); + + await expect( + nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + next + ) + ).rejects.toBe( connectionError ); + + // No silent fallback to a direct re-upload — retrying a non-idempotent + // POST could duplicate the attachment. + expect( next ).not.toHaveBeenCalled(); + } ); + + it( 'propagates an abort instead of falling back', async () => { + getGBKit.mockReturnValue( { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + } ); + const next = makeNext(); + + // A real aborted signal — `fetch` rejects with the signal's reason. + const controller = new AbortController(); + controller.abort(); + const options = { + ...makePostMediaOptions( makeFile() ), + signal: controller.signal, + }; + global.fetch = vi.fn( () => + Promise.reject( controller.signal.reason ) + ); + + // The middleware rethrows the signal's canonical reason (not the fetch + // rejection) and does not retry. + await expect( + nativeMediaUploadMiddleware( options, next ) + ).rejects.toBe( controller.signal.reason ); + + // An explicit cancellation must not be retried via the default path. + expect( next ).not.toHaveBeenCalled(); + } ); + + it( 'propagates a timeout cancellation (aborted signal, non-AbortError) instead of falling back', async () => { + getGBKit.mockReturnValue( { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + } ); + 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. + 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 ) ); + + await expect( + nativeMediaUploadMiddleware( options, next ) + ).rejects.toBe( timeoutError ); + + // A timeout is a cancellation, not an unreachable server — do not retry. + expect( next ).not.toHaveBeenCalled(); } ); // MARK: - Signal forwarding diff --git a/src/utils/api-fetch.js b/src/utils/api-fetch.js index 395bb9541..e08d03220 100644 --- a/src/utils/api-fetch.js +++ b/src/utils/api-fetch.js @@ -202,59 +202,115 @@ export function nativeMediaUploadMiddleware( options, next ) { `Routing upload of ${ file.name } through native server on port ${ nativeUploadPort }` ); - const formData = new FormData(); - formData.append( 'file', file, file.name ); - - return fetch( `http://localhost:${ nativeUploadPort }/upload`, { + // Forward the original request body — the file plus every sibling field + // (`post`, additionalData) — and the original query string (e.g. `?_embed`) + // so the native server can relay them to WordPress unchanged. Rebuilding the + // body with only `file` would drop the post association and additionalData. + const query = requestQuery( options.path ); + + // Use the two-argument form of `.then()` so the rejection handler catches + // *only* a connection-level failure of the `fetch()` itself — not errors + // thrown while handling a response (those must surface as real failures). + return fetch( `http://localhost:${ nativeUploadPort }/upload${ query }`, { method: 'POST', headers: { 'Relay-Authorization': `Bearer ${ nativeUploadToken }`, }, - body: formData, + body: options.body, signal: options.signal, - } ) - .then( ( response ) => { + } ).then( + ( response ) => { + // The native server relays WordPress's response verbatim. On a + // non-2xx, mirror @wordpress/api-fetch: reject with the parsed WP + // error body ({ code, message, data }) so @wordpress/media-utils + // surfaces WordPress's real message. On success, return WordPress's + // attachment object unchanged so every consumer behaves exactly as + // it would for a non-native upload. if ( ! response.ok ) { - return response.text().then( ( body ) => { - const error = new Error( - `Upload failed (${ response.status }): ${ - body || response.statusText - }` - ); - error.code = 'upload_failed'; - throw error; - } ); + return response + .json() + .catch( invalidUploadResponseError ) + .then( ( body ) => { + logError( 'Native upload failed', body ); + throw body; + } ); } - return response.json(); - } ) - .then( ( result ) => { - // Transform native server response into WordPress REST API - // attachment shape expected by @wordpress/media-utils. - return { - id: result.id, - source_url: result.url, - alt_text: result.alt || '', - caption: { - raw: result.caption || '', - rendered: result.caption || '', - }, - title: { - raw: result.title || '', - rendered: result.title || '', - }, - mime_type: result.mime, - media_type: result.type, - media_details: { - width: result.width || 0, - height: result.height || 0, - }, - link: result.url, - }; - } ) - .catch( ( err ) => { - logError( 'Native upload failed', err ); - throw err; - } ); + // A 2xx with a non-JSON body (e.g. an HTML error page injected by an + // intermediary) rejects json(); normalize it the same way as the + // non-ok path rather than surfacing a raw SyntaxError. + return response.json().catch( () => { + const error = invalidUploadResponseError(); + logError( 'Native upload returned an invalid response', error ); + throw error; + } ); + }, + ( connectionError ) => { + // A caller-initiated cancellation must propagate as the cancellation, + // never be retried. Detect it via `signal.aborted` — the cancellation + // *state* — rather than `connectionError.name === 'AbortError'`: the + // state check also catches `AbortSignal.timeout()` (which rejects with + // a TimeoutError, not an AbortError) and custom abort reasons, which a + // name match would miss and wrongly fall back on. Rethrow the signal's + // `reason` (the canonical abort error), not `connectionError`: if a + // network failure and the abort race, `fetch` can reject with a network + // TypeError even though the signal aborted, and rethrowing that would + // 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; + } + // Otherwise the loopback upload server is unreachable at the transport + // layer. We deliberately do NOT fall back to a direct re-upload: + // reachability is gated proactively upstream — this middleware's guard + // skips the native path when no port is advertised, and the native side + // only advertises a port the WebView can actually reach (server running + // + cleartext-to-localhost permitted, cleared on stop). So reaching here + // means the server died out-of-band after a valid start; retrying a + // non-idempotent POST /wp/v2/media could duplicate the attachment if the + // native server had already relayed it to WordPress. + logError( + 'Native upload failed at the transport layer', + connectionError + ); + throw connectionError; + } + ); +} + +/** + * The query component of a request path, including the leading `?`, or an empty + * string when there is no query. + * + * Mirrors the `query` accessors on the native request types (`HttpRequest` on + * Android, `ParsedHTTPRequest` on iOS): the split is on the first `?`, and a + * bare trailing `?` carries no parameters so it yields an empty string. Keeping + * the three in agreement means the value can be appended to an upstream URL + * unconditionally, whichever side derived it. + * + * @param {string} path The request path, e.g. `/wp/v2/media?_embed`. + * @return {string} The query, e.g. `?_embed`, or `''`. + */ +function requestQuery( path ) { + const separator = path.indexOf( '?' ); + if ( separator === -1 ) { + return ''; + } + const value = path.slice( separator + 1 ); + return value ? `?${ value }` : ''; +} + +/** + * The error rejected when the upload server's response body can't be parsed as + * JSON. Shaped like a WordPress REST error so `@wordpress/media-utils` surfaces + * it the same way as a real one, on both the non-2xx and 2xx paths. + * + * @return {{ code: string, message: string }} The normalized error. + */ +function invalidUploadResponseError() { + return { + code: 'invalid_json', + message: 'The upload server returned an invalid response.', + }; } /**