From f591f1fbd8d4a730e28b027a20da6aa0a46302a6 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Fri, 21 Aug 2026 09:12:09 -0400 Subject: [PATCH 01/21] fix: let a delegate handle its own media deletions `handleDelete` went straight to the default uploader, unlike `handleUpload` which offers the work to the delegate first. A host whose `uploadFile` uploads to its own media service holds an ID only it can resolve, so deleting through the default uploader would address the wrong site. Add `deleteFile(attachmentId:)` to `MediaUploadDelegate` on both platforms, defaulted to nil so existing hosts are unaffected, and try it before falling back. Rename the handler to `handleMediaDelete`, since it deletes an attachment rather than an upload and no longer mirrors `handleUpload`'s signature. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF --- .../wordpress/gutenberg/MediaUploadServer.kt | 27 +++++++++++--- .../gutenberg/MediaUploadServerTest.kt | 34 ++++++++++++++++++ .../Sources/Media/MediaUploadDelegate.swift | 17 +++++++++ .../Sources/Media/MediaUploadServer.swift | 20 +++++++---- .../Media/MediaUploadServerTests.swift | 36 +++++++++++++++++++ 5 files changed, 124 insertions(+), 10 deletions(-) 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 66cb94791..6bc3d2e6f 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -111,6 +111,20 @@ interface MediaUploadDelegate { * the editor sees a complete attachment object. */ suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null + + /** + * Delete a previously uploaded attachment. + * + * The editor deletes the attachment when an upload's server-side + * post-processing fails past recovery, so it does not leave an orphan + * behind. A delegate that uploaded the attachment itself via [uploadFile] + * owns an ID only it can resolve, so it must delete the attachment itself + * too — the default uploader would address the wrong site. + * + * Return the raw response (status code + body), which GutenbergKit relays + * to the editor unchanged, or null to use the default uploader. + */ + suspend fun deleteFile(attachmentId: String): MediaUploadResponse? = null } /** @@ -234,7 +248,7 @@ internal class MediaUploadServer( if (method == "DELETE") { attachmentIdFromPath(request.path)?.let { attachmentId -> - return handleDelete(attachmentId, request.query) + return handleMediaDelete(attachmentId, request.query) } } @@ -255,17 +269,22 @@ internal class MediaUploadServer( } /** - * Relays the editor's orphan cleanup to WordPress. + * Relays the editor's orphan cleanup. * * Core's media upload middleware deletes the attachment when every * `post-process` retry fails. A cross-origin editor cannot issue that * request directly — api-fetch tunnels `DELETE` as a `POST` carrying * `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the * browser blocks it at preflight. Relaying it here lets the cleanup run. + * + * Offers the deletion to the delegate first, as [handleUpload] does, so a + * host that uploaded the attachment itself deletes it from the same place. */ - private suspend fun handleDelete(attachmentId: String, query: String): HttpResponse { - val uploader = defaultUploader ?: return errorResponse(500, "No uploader configured") + private suspend fun handleMediaDelete(attachmentId: String, query: String): HttpResponse { return try { + uploadDelegate?.deleteFile(attachmentId)?.let { return relayResponse(it) } + + val uploader = defaultUploader ?: return errorResponse(500, "No uploader configured") relayResponse(uploader.deleteMedia(attachmentId, query)) } catch (e: IOException) { Log.e(TAG, "Media deletion failed", e) 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 1d1f5ed6a..f4da7f671 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -140,6 +140,27 @@ class MediaUploadServerTest { assertTrue(response.statusLine.contains("404")) } + @Test + fun `routes a deletion to the delegate when it handles one`() { + // A host that uploaded the attachment itself owns an ID only it can + // resolve, so the default uploader must not be asked to delete it. No + // default uploader is configured, so a 200 here can only come from the + // delegate — the fallback path would fail with "no uploader". + val delegate = DeletingDelegate() + server.stop() + server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + + val response = sendRawRequest( + method = "DELETE", + path = "/media/42?force=true", + headers = mapOf("Relay-Authorization" to "Bearer ${server.token}"), + body = ByteArray(0) + ) + + assertTrue("Expected 200 but got: ${response.statusLine}", response.statusLine.contains("200")) + assertEquals("42", delegate.deletedAttachmentId) + } + @Test fun `routes upload with a query string and relays the query`() { val delegate = ProcessOnlyDelegate() @@ -734,6 +755,19 @@ class MediaUploadServerTest { } } + /** + * A delegate that handles deletions itself, as a host uploading to its own + * media service would. + */ + private class DeletingDelegate : MediaUploadDelegate { + @Volatile var deletedAttachmentId: String? = null + + override suspend fun deleteFile(attachmentId: String): MediaUploadResponse? { + deletedAttachmentId = attachmentId + return MediaUploadResponse(200, """{"deleted":true}""".toByteArray()) + } + } + private class ProcessOnlyDelegate : MediaUploadDelegate { @Volatile var processFileCalled = false diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index 73752166b..d74743f97 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -82,6 +82,19 @@ public protocol MediaUploadDelegate: AnyObject, Sendable { /// 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? + + /// Delete a previously uploaded attachment. + /// + /// The editor deletes the attachment when an upload's server-side + /// post-processing fails past recovery, so it does not leave an orphan + /// behind. A delegate that uploaded the attachment itself via + /// ``uploadFile(at:mimeType:filename:)`` owns an ID only it can resolve, so + /// it must delete the attachment itself too — the default uploader would + /// address the wrong site. + /// + /// Return the raw response (status code + body), which GutenbergKit relays + /// to the editor unchanged, or `nil` to use the default uploader. + func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? } /// Default implementations. @@ -97,4 +110,8 @@ extension MediaUploadDelegate { public func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { nil } + + public func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { + nil + } } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index a2159d762..13b7047e6 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -98,7 +98,7 @@ final class MediaUploadServer: Sendable { } if method == "DELETE", let attachmentId = attachmentId(fromPath: parsed.path) { - return await handleDelete(attachmentId, query: parsed.query, context: context) + return await handleMediaDelete(attachmentId, query: parsed.query, context: context) } return errorResponse(status: 404, message: "Not found") @@ -208,20 +208,28 @@ final class MediaUploadServer: Sendable { return id } - /// Relays the editor's orphan cleanup to WordPress. + /// Relays the editor's orphan cleanup. /// /// Core's media upload middleware deletes the attachment when every /// `post-process` retry fails. A cross-origin editor cannot issue that /// request directly — api-fetch tunnels `DELETE` as a `POST` carrying /// `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the /// browser blocks it at preflight. Relaying it here lets the cleanup run. - private static func handleDelete( + /// + /// Offers the deletion to the delegate first, as ``handleUpload(_:context:)`` + /// does, so a host that uploaded the attachment itself deletes it from the + /// same place. + private static func handleMediaDelete( _ attachmentId: String, query: String, context: UploadContext ) async -> HTTPResponse { - guard let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) - } do { + if let response = try await context.uploadDelegate?.deleteFile(attachmentId: attachmentId) { + return relayResponse(response) + } + + guard let defaultUploader = context.defaultUploader else { + return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + } let response = try await defaultUploader.deleteMedia(attachmentId: attachmentId, query: query) return relayResponse(response) } catch { diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index fa87bbc4f..598067463 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -131,6 +131,28 @@ struct MediaUploadServerTests { #expect(mockUploader.lastQuery == "?_embed=wp:featuredmedia") } + @Test("routes a deletion to the delegate when it handles one") + func delegateHandlesDeletion() async throws { + // A host that uploaded the attachment itself owns an ID only it can + // resolve, so the default uploader must not be asked to delete it. + // No default uploader is configured, so a 200 here can only come from the + // delegate — the fallback path would fail with "no uploader". + let delegate = DeletingDelegate() + let server = try await MediaUploadServer.start(uploadDelegate: delegate) + defer { server.stop() } + + let url = URL(string: "http://127.0.0.1:\(server.port)/media/42?force=true")! + var request = URLRequest(url: url) + request.httpMethod = "DELETE" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + + let (_, response) = try await URLSession.shared.data(for: request) + let httpResponse = try #require(response as? HTTPURLResponse) + + #expect(httpResponse.statusCode == 200) + #expect(delegate.deletedAttachmentId == "42") + } + @Test("calls delegate and returns upload result") func delegateProcessAndUpload() async throws { let delegate = MockUploadDelegate() @@ -795,6 +817,20 @@ private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable } } +/// A delegate that handles deletions itself, as a host uploading to its own +/// media service would. +private final class DeletingDelegate: MediaUploadDelegate, @unchecked Sendable { + private let lock = NSLock() + private var _deletedAttachmentId: String? + + var deletedAttachmentId: String? { lock.withLock { _deletedAttachmentId } } + + func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { + lock.withLock { _deletedAttachmentId = attachmentId } + return MediaUploadResponse(statusCode: 200, body: Data(#"{"deleted":true}"#.utf8)) + } +} + private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false From 9732e1e1077cb08523762368bacaaa993f520755 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Fri, 21 Aug 2026 09:12:19 -0400 Subject: [PATCH 02/21] docs: trim implementation detail from the wp-env media failure guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the two notes explaining how the simulator produces its 500 and why a fatal response reads as a CORS error — implementation detail that belongs in the plugin, not the guide. Drop the orphaned-server and stale-credential troubleshooting entries; those are environment problems to address on their own. Also drop a comment restating what the adjacent condition already says, and reword the make target's help text. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF --- Makefile | 2 +- docs/code/local-wordpress.md | 21 --------------------- src/utils/api-fetch.js | 4 ---- 3 files changed, 1 insertion(+), 26 deletions(-) diff --git a/Makefile b/Makefile index 283f2d6a3..75fa08f9c 100644 --- a/Makefile +++ b/Makefile @@ -219,7 +219,7 @@ wp-env-android-reset: ## Remove the Android emulator URL remap and restart @RESET=1 $(MAKE) wp-env-start .PHONY: wp-env-media-failure -wp-env-media-failure: ## Report the media upload failure simulation mode (MODE=off|recover|always to set it) +wp-env-media-failure: ## Report the media upload failure simulation mode (set via MODE=off|recover|always) @MODE=$(MODE) bash bin/wp-env-media-failure.sh ################################################################################ diff --git a/docs/code/local-wordpress.md b/docs/code/local-wordpress.md index 051568c07..a8fac7eea 100644 --- a/docs/code/local-wordpress.md +++ b/docs/code/local-wordpress.md @@ -129,12 +129,6 @@ The mode is stored server-side, so it persists across uploads and retries until Then upload an image from a demo app and watch the network requests. In `recover` mode the upload 500s and the following `post-process` call succeeds, leaving a complete attachment; in `always` mode you should see five `post-process` attempts followed by a `DELETE`. -The mode is stored as an option rather than per-request state, because the upload and each retry are separate requests — and an upload routed through the native upload server is relayed by URLSession/OkHttp, which carries no browser cookie. - -**Note:** the plugin sets the 500 status itself. A real PHP fatal under FPM surfaces as a 500, but the Playground runtime wp-env uses returns 200, which the editor's `status >= 500` check would ignore. - -**Note:** a simulated fatal aborts the request before WordPress adds CORS headers, so a cross-origin editor reports these responses as CORS errors with provisional request headers rather than as a readable 500. That is expected for the upload and `post-process` responses — the editor only needs their status and the attachment ID header. It is why the plugin never fails a `DELETE`, including the `POST` + `X-Http-Method-Override: DELETE` form api-fetch actually sends: failing the orphan cleanup would make a correctly working retry look broken. - **Only the native upload server path recovers locally.** Reading `X-WP-Upload-Attachment-ID` cross-origin requires the site to list it in `Access-Control-Expose-Headers`, and WordPress core's `rest_send_cors_headers()` does not. Uploads routed through the native upload server recover on both platforms, since that server exposes the header itself. A **direct** upload (native media upload disabled) never recovers on iOS, which loads the editor from `file://`. It does not recover against wp-env on Android either: `GutenbergView` derives the asset domain from the site's _host_, which drops the port, so the editor at `http://10.0.2.2` is cross-origin with the site at `http://10.0.2.2:8888`. Direct uploads are only same-origin — and therefore only recover — when the site runs on the scheme's default port, as production sites do. @@ -162,21 +156,6 @@ Another service is using port 8888. Stop the conflicting service or change the w } ``` -Under the Playground runtime the culprit is often a previous wp-env server that outlived `make wp-env-stop`, which then makes every subsequent start fail with `EADDRINUSE`: - -```bash -lsof -ti:8888 # confirm what holds the port -pkill -f "wp-playground.js" -``` - -### Credentials rejected with HTTP 401 - -The Playground runtime starts from a fresh database on each start, so an existing `.wp-env.credentials.json` no longer matches the site's application password. Regenerate it: - -```bash -make wp-env-start RESET=1 -``` - ### Resetting the environment To start fresh, destroy the environment and recreate it: diff --git a/src/utils/api-fetch.js b/src/utils/api-fetch.js index 2c0edce44..e53a8dba4 100644 --- a/src/utils/api-fetch.js +++ b/src/utils/api-fetch.js @@ -94,10 +94,6 @@ function corsMiddleware( options, next ) { */ function apiPathModifierMiddleware( options, next ) { const { siteApiNamespace, namespaceExcludedPaths } = getGBKit(); - // Self-hosted sites configure no namespace, so there is nothing to insert. - // This has to gate the rewrite explicitly: the namespace match below cannot - // stand in for it, and an empty namespace would otherwise interpolate - // `undefined` into the path. const isEligiblePath = options.path && siteApiNamespace.length > 0 && From ebb67a5a10d925e9be1e24f876f3d7abd3bdecb3 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Fri, 21 Aug 2026 09:18:10 -0400 Subject: [PATCH 03/21] refactor: extract the native media upload helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `nativeMediaUploadMiddleware` mixed dispatch with the whole upload implementation, so adding the deletion path left the two handled asymmetrically — one extracted, one inline. Extract `nativeMediaUpload` alongside `nativeMediaDelete`, both returning null when a request is not theirs, leaving the middleware as a short dispatcher. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF --- src/utils/api-fetch.js | 60 +++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/src/utils/api-fetch.js b/src/utils/api-fetch.js index e53a8dba4..590a44780 100644 --- a/src/utils/api-fetch.js +++ b/src/utils/api-fetch.js @@ -193,17 +193,12 @@ function filterEndpointsMiddleware( options, next ) { } /** - * Middleware that routes media uploads through the native host's local HTTP - * server for processing (e.g. image resizing) before uploading to WordPress. + * Middleware that routes media requests through the native host's local HTTP + * server: uploads for processing (e.g. image resizing) before they reach + * WordPress, and attachment deletions for the editor's orphan cleanup. * * Exported for testing only. * - * When `nativeUploadPort` is configured in GBKit, this middleware intercepts - * `POST /wp/v2/media` requests, forwards the file to the native server, and - * returns the response in WordPress REST API attachment format so the existing - * Gutenberg upload pipeline (blob previews, save locking, entity caching) - * works unchanged. - * * When the native server is not configured, requests pass through unmodified. * * Note: Ideally, media uploads would be handled via the `mediaUpload` editor @@ -223,27 +218,44 @@ function filterEndpointsMiddleware( options, next ) { export function nativeMediaUploadMiddleware( options, next ) { const { nativeUploadPort, nativeUploadToken } = getGBKit(); - if ( nativeUploadPort && nativeUploadToken ) { - const deletion = nativeMediaDelete( - options, - nativeUploadPort, - nativeUploadToken - ); - if ( deletion ) { - return deletion; - } + if ( ! nativeUploadPort || ! nativeUploadToken ) { + return next( options ); } + // Each helper returns `null` when the request is not its concern, so an + // unhandled request falls through to the default path. + return ( + nativeMediaDelete( options, nativeUploadPort, nativeUploadToken ) ?? + nativeMediaUpload( options, nativeUploadPort, nativeUploadToken ) ?? + next( options ) + ); +} + +/** + * Routes a media upload through the native upload server. + * + * Returns `null` when the request is not a media upload, so the caller falls + * through to its normal handling. + * + * Intercepts `POST /wp/v2/media`, forwards the file to the native server, and + * returns the response in WordPress REST API attachment format so the existing + * Gutenberg upload pipeline (blob previews, save locking, entity caching) works + * unchanged. + * + * @param {Object} options The api-fetch options. + * @param {number} port The native upload server port. + * @param {string} token The native upload server bearer token. + * @return {?Promise} The relayed upload, or `null` if not applicable. + */ +function nativeMediaUpload( options, port, token ) { if ( - ! nativeUploadPort || - ! nativeUploadToken || ! options.method || options.method.toUpperCase() !== 'POST' || ! options.path || ! MEDIA_UPLOAD_PATH.test( options.path ) || ! ( options.body instanceof FormData ) ) { - return next( options ); + return null; } // Only intercept a genuine file upload. `FormData.get('file')` returns a @@ -253,11 +265,11 @@ export function nativeMediaUploadMiddleware( options, next ) { // through to the default path — and guarantees `file.name` below is safe. const file = options.body.get( 'file' ); if ( ! ( file instanceof File ) ) { - return next( options ); + return null; } info( - `Routing upload of ${ file.name } through native server on port ${ nativeUploadPort }` + `Routing upload of ${ file.name } through native server on port ${ port }` ); // Forward the original request body — the file plus every sibling field @@ -269,10 +281,10 @@ export function nativeMediaUploadMiddleware( options, next ) { // 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 }`, { + return fetch( `http://localhost:${ port }/upload${ query }`, { method: 'POST', headers: { - 'Relay-Authorization': `Bearer ${ nativeUploadToken }`, + 'Relay-Authorization': `Bearer ${ token }`, }, body: options.body, signal: options.signal, From c31573acc7166b6ec67d4231f418af2022a2b321 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Fri, 21 Aug 2026 14:20:51 -0400 Subject: [PATCH 04/21] fix: surface a delegate's real media deletion failure on Android MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `handleMediaDelete` caught only `IOException`, so a delegate's `deleteFile` throwing anything else — `IllegalStateException`, a JSON error — escaped to `HttpServer.resolveResponse` and returned a plain-text 500. The editor's `nativeMediaDelete` then failed on `response.json()` and reported `invalid_json` rather than the delegate's actual failure. Catch `Exception` and rethrow `CancellationException`, matching `passthroughResponse` and iOS's untyped `catch`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr --- .../main/java/org/wordpress/gutenberg/MediaUploadServer.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 6bc3d2e6f..4cbdf1442 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -280,13 +280,16 @@ internal class MediaUploadServer( * Offers the deletion to the delegate first, as [handleUpload] does, so a * host that uploaded the attachment itself deletes it from the same place. */ + @Suppress("TooGenericExceptionCaught") private suspend fun handleMediaDelete(attachmentId: String, query: String): HttpResponse { return try { uploadDelegate?.deleteFile(attachmentId)?.let { return relayResponse(it) } val uploader = defaultUploader ?: return errorResponse(500, "No uploader configured") relayResponse(uploader.deleteMedia(attachmentId, query)) - } catch (e: IOException) { + } catch (e: kotlin.coroutines.cancellation.CancellationException) { + throw e // Never swallow coroutine cancellation. + } catch (e: Exception) { Log.e(TAG, "Media deletion failed", e) errorResponse(500, e.message ?: "Deletion failed") } From 0bf40ad39cdbadfd8f9d1cbe736199ca47d1a6c6 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Fri, 21 Aug 2026 14:25:33 -0400 Subject: [PATCH 05/21] fix: let a delegate's Content-Type win over the JSON default on iOS `relayResponse` prepended `Content-Type: application/json` to an array of the response's own headers, and `HTTPResponse` serializes every entry it is given. A delegate returning its own `Content-Type` therefore put the header on the wire twice, which URLSession surfaces as "application/json, text/plain". Android's map merge already overrode instead, so the two platforms disagreed on the same public API. Skip the default when the response already carries the name, matching Android. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr --- .../Sources/Media/MediaUploadServer.swift | 9 +++-- .../Media/MediaUploadServerTests.swift | 34 +++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 13b7047e6..0c03d8de2 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -244,10 +244,15 @@ final class MediaUploadServer: Sendable { /// the editor retry `post-process` for an upload whose metadata generation /// fataled server-side, rather than surfacing a permanent failure and /// leaving an orphaned attachment behind. + /// + /// The response's own `Content-Type` wins over the JSON default. `HTTPResponse` + /// serializes every header it is given, so appending the default unconditionally + /// would emit the name twice for a delegate that sets it. private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { - HTTPResponse( + let hasContentType = response.headers.keys.contains { $0.lowercased() == "content-type" } + return HTTPResponse( status: response.statusCode, - headers: [("Content-Type", "application/json")] + headers: (hasContentType ? [] : [("Content-Type", "application/json")]) + response.headers.map { ($0.key, $0.value) }, body: response.body ) diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 598067463..910f49db4 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -153,6 +153,28 @@ struct MediaUploadServerTests { #expect(delegate.deletedAttachmentId == "42") } + @Test("relays the delegate's own Content-Type instead of emitting it twice") + func delegateContentTypeWins() async throws { + // `HTTPResponse` serializes every header it is given, so appending the JSON + // default unconditionally would put `Content-Type` on the wire twice. + // URLSession joins repeated headers with a comma, which is what a + // regression would look like here. + let delegate = ContentTypeDeletingDelegate() + let server = try await MediaUploadServer.start(uploadDelegate: delegate) + defer { server.stop() } + + let url = URL(string: "http://127.0.0.1:\(server.port)/media/42?force=true")! + var request = URLRequest(url: url) + request.httpMethod = "DELETE" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + + let (_, response) = try await URLSession.shared.data(for: request) + let httpResponse = try #require(response as? HTTPURLResponse) + let contentType = httpResponse.value(forHTTPHeaderField: "Content-Type") + + #expect(contentType == "text/plain") + } + @Test("calls delegate and returns upload result") func delegateProcessAndUpload() async throws { let delegate = MockUploadDelegate() @@ -831,6 +853,18 @@ private final class DeletingDelegate: MediaUploadDelegate, @unchecked Sendable { } } +/// A delegate that sets its own `Content-Type`, so the relay must not also +/// append the JSON default. +private final class ContentTypeDeletingDelegate: MediaUploadDelegate, @unchecked Sendable { + func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { + MediaUploadResponse( + statusCode: 200, + body: Data("deleted".utf8), + headers: ["Content-Type": "text/plain"] + ) + } +} + private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false From ed031c3560b81675b935c92539a6cd10aabba2ea Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Fri, 21 Aug 2026 14:26:10 -0400 Subject: [PATCH 06/21] docs: tell a delegate to decline media deletions it does not own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `deleteFile` is called for every deletion, including attachments the delegate declined at upload time — an attachment ID carries no MIME type or filename, so there is no `handlesFile` gate to apply. A delegate answering for one of those leaves the real WordPress attachment undeleted, which is the orphan the cleanup exists to remove. Returning nil already falls through to the default uploader; document that as the signal for an unrecognized ID. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr --- .../java/org/wordpress/gutenberg/MediaUploadServer.kt | 9 +++++++++ .../Sources/Media/MediaUploadDelegate.swift | 10 ++++++++++ 2 files changed, 19 insertions(+) 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 4cbdf1442..d9bf10d5f 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -123,6 +123,15 @@ interface MediaUploadDelegate { * * Return the raw response (status code + body), which GutenbergKit relays * to the editor unchanged, or null to use the default uploader. + * + * Return null for any ID the delegate does not recognize. Unlike the upload + * path, there is no [handlesFile] gate here — an attachment ID carries no + * MIME type or filename — so this method is called for *every* deletion, + * including attachments the delegate declined at upload time and WordPress + * therefore created itself. Returning a response for one of those (an error + * from the host's own media service, say) leaves the real WordPress + * attachment undeleted — precisely the orphan this cleanup exists to remove. + * null hands it to the default uploader, which addresses the right site. */ suspend fun deleteFile(attachmentId: String): MediaUploadResponse? = null } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index d74743f97..01b0be7d8 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -94,6 +94,16 @@ public protocol MediaUploadDelegate: AnyObject, Sendable { /// /// Return the raw response (status code + body), which GutenbergKit relays /// to the editor unchanged, or `nil` to use the default uploader. + /// + /// Return `nil` for any ID the delegate does not recognize. Unlike the upload + /// path, there is no ``handlesFile(ofType:named:)`` gate here — an attachment + /// ID carries no MIME type or filename — so this method is called for *every* + /// deletion, including attachments the delegate declined at upload time and + /// WordPress therefore created itself. Returning a response for one of those + /// (an error from the host's own media service, say) leaves the real + /// WordPress attachment undeleted — precisely the orphan this cleanup exists + /// to remove. `nil` hands it to the default uploader, which addresses the + /// right site. func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? } From 02ee5305e602eb66f247218db9bba9504773c5ec Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Fri, 21 Aug 2026 14:32:06 -0400 Subject: [PATCH 07/21] fix: read the wp-env credentials path as an argument, not as source The credentials path was interpolated into the `node -e` source as a single-quoted JS string literal, so a checkout under a path containing a quote or backslash produced a SyntaxError stack trace instead of the intended "could not read authHeader" message. Pass it through `process.argv` and single-quote the script body so the shell does not expand it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr --- bin/wp-env-media-failure.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/bin/wp-env-media-failure.sh b/bin/wp-env-media-failure.sh index 8c5963824..b4254cd61 100755 --- a/bin/wp-env-media-failure.sh +++ b/bin/wp-env-media-failure.sh @@ -51,16 +51,19 @@ if [ ! -f "$CREDENTIALS_FILE" ]; then exit 1 fi -AUTH_HEADER=$(node -e " - const fs = require('fs'); +# The path arrives as an argument rather than interpolated into the source: a +# checkout under a path containing a quote or backslash would otherwise produce +# a syntax error instead of the failure message below. +AUTH_HEADER=$(node -e ' + const fs = require("fs"); try { - const c = JSON.parse(fs.readFileSync('$CREDENTIALS_FILE', 'utf8')); + const c = JSON.parse(fs.readFileSync(process.argv[1], "utf8")); if (!c.authHeader) process.exit(1); process.stdout.write(c.authHeader); } catch { process.exit(1); } -") || { +' "$CREDENTIALS_FILE") || { echo "Error: could not read authHeader from $CREDENTIALS_FILE" >&2 echo 'The file may be malformed. Run "make wp-env-start RESET=1" to regenerate it.' >&2 exit 1 From 6140749fca710d5eb04f25eac1b84682431601a5 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Fri, 21 Aug 2026 15:50:42 -0400 Subject: [PATCH 08/21] fix: match a delegate's Content-Type case-insensitively on Android MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `relayResponse` merged the JSON default with the response's own headers via Kotlin's map merge, which only overrides on an exact key match. A delegate returning `content-type` therefore produced a two-entry map, and `serializeResponse` writes every entry, putting the header on the wire twice — the WebView sees "application/json, text/plain". This is the same defect `0bf40ad3` fixed on iOS, which the map merge was believed to already handle. Skip the default when the response carries the name under any casing, matching iOS and the case-insensitive lookups `HttpServer` already uses. Add the Android counterpart to the iOS `delegateContentTypeWins` test. It asserts on the raw header lines rather than the parsed map, which lowercases keys into a map and would collapse the duplicate — hiding the very bug. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LVdVBubbCDp7mRXSWkcHHm --- .../wordpress/gutenberg/MediaUploadServer.kt | 9 ++- .../gutenberg/MediaUploadServerTest.kt | 58 ++++++++++++++++++- 2 files changed, 63 insertions(+), 4 deletions(-) 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 d9bf10d5f..a65ceb301 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -352,11 +352,18 @@ internal class MediaUploadServer( * the editor retry `post-process` for an upload whose metadata generation * fataled server-side, rather than surfacing a permanent failure and leaving * an orphaned attachment behind. + * + * The response's own `Content-Type` wins over the JSON default, matched + * case-insensitively — HTTP header names are case-insensitive, and + * [HttpResponse] serializes every entry it is given, so a plain map merge + * would emit the name twice for a delegate that spells it `content-type`. */ private fun relayResponse(response: MediaUploadResponse): HttpResponse { + val hasContentType = response.headers.keys.any { it.lowercase() == "content-type" } + val defaults = if (hasContentType) emptyMap() else mapOf("Content-Type" to "application/json") return HttpResponse( status = response.statusCode, - headers = mapOf("Content-Type" to "application/json") + response.headers, + headers = defaults + response.headers, body = response.body ) } 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 f4da7f671..6cdc04da7 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -161,6 +161,30 @@ class MediaUploadServerTest { assertEquals("42", delegate.deletedAttachmentId) } + @Test + fun `relays a delegate's own Content-Type instead of emitting it twice`() { + // HTTP header names are case-insensitive, so a delegate spelling it + // `content-type` must still override the JSON default rather than merge + // alongside it — HttpResponse serializes every entry it is given, which + // would put the name on the wire twice (mirrors the iOS behavior). + val delegate = ContentTypeDeletingDelegate() + server.stop() + server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + + val response = sendRawRequest( + method = "DELETE", + path = "/media/42?force=true", + headers = mapOf("Relay-Authorization" to "Bearer ${server.token}"), + body = ByteArray(0) + ) + + assertTrue("Expected 200 but got: ${response.statusLine}", response.statusLine.contains("200")) + // Assert on the raw header lines, not the parsed map: the parser + // lowercases keys into a map, so a duplicated header would silently + // collapse and this test would pass against the very bug it covers. + assertEquals(listOf("text/plain"), response.rawHeaderValues("content-type")) + } + @Test fun `routes upload with a query string and relays the query`() { val delegate = ProcessOnlyDelegate() @@ -659,8 +683,22 @@ class MediaUploadServerTest { private data class RawHttpResponse( val statusLine: String, val headers: Map, - val body: String - ) + val body: String, + /** The header lines exactly as received, before collapsing into [headers]. */ + val rawHeaderLines: List = emptyList() + ) { + /** + * Every value sent for [name], in order. Unlike [headers], this preserves + * repeats — the only way to catch a header emitted twice. + */ + fun rawHeaderValues(name: String): List = + rawHeaderLines.mapNotNull { line -> + val colonIndex = line.indexOf(':') + if (colonIndex <= 0) return@mapNotNull null + if (!line.substring(0, colonIndex).trim().equals(name, ignoreCase = true)) return@mapNotNull null + line.substring(colonIndex + 1).trim() + } + } private fun sendRawRequest( method: String, @@ -715,7 +753,7 @@ class MediaUploadServerTest { } } - return RawHttpResponse(statusLine, responseHeaders, responseBody) + return RawHttpResponse(statusLine, responseHeaders, responseBody, lines.drop(1)) } private fun buildMultipartBody( @@ -768,6 +806,20 @@ class MediaUploadServerTest { } } + /** + * A delegate that sets its own `Content-Type`, lowercased, so the relay must + * override the JSON default rather than emit the header twice. + */ + private class ContentTypeDeletingDelegate : MediaUploadDelegate { + override suspend fun deleteFile(attachmentId: String): MediaUploadResponse? { + return MediaUploadResponse( + 200, + "deleted".toByteArray(), + mapOf("content-type" to "text/plain") + ) + } + } + private class ProcessOnlyDelegate : MediaUploadDelegate { @Volatile var processFileCalled = false From 910fc77149bc99811ed2e15e78b8886afd511c46 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:50:24 -0600 Subject: [PATCH 09/21] fix: don't log a recoverable media upload 5xx as a failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The native upload middleware runs below core's mediaUploadMiddleware, which forwards every upload as `parse: false` and reads `x-wp-upload-attachment-id` off a rejected Response to retry post-process. Logging the initial 5xx at error level reported a failure before recovery ran, so every upload that silently recovered still emitted an error. Reject without logging, matching the nativeMediaDelete sibling — the initial 5xx is a handoff to core's retry, not an outcome. --- src/utils/api-fetch-post-process.test.js | 27 ++++++++++++++++++++++++ src/utils/api-fetch.js | 7 +++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/utils/api-fetch-post-process.test.js b/src/utils/api-fetch-post-process.test.js index 6ab220d87..2311f9fa7 100644 --- a/src/utils/api-fetch-post-process.test.js +++ b/src/utils/api-fetch-post-process.test.js @@ -21,6 +21,7 @@ import apiFetch from '@wordpress/api-fetch'; */ import { configureApiFetch } from './api-fetch'; import * as bridge from './bridge'; +import { error } from './logger'; vi.mock( './bridge', async ( importOriginal ) => { const actual = await importOriginal(); @@ -258,4 +259,30 @@ describe( "core's media upload post-process middleware", () => { expect( global.fetch ).toHaveBeenCalledTimes( 1 ); } ); + + it( 'does not log an error for an upload that recovers', async () => { + // The initial 5xx is a handoff to core's post-process retry, not a + // failure. A silently-recovered upload must surface no error to the host. + bridge.getGBKit.mockReturnValue( { + siteApiRoot: SITE_API_ROOT, + authHeader: 'Bearer test-token', + siteApiNamespace: [], + namespaceExcludedPaths: [], + nativeUploadPort: 8080, + nativeUploadToken: 'relay-token', + } ); + + global.fetch = vi.fn( ( url ) => { + if ( String( url ).includes( 'post-process' ) ) { + return Promise.resolve( makeResponse( 200, null, { id: 42 } ) ); + } + return Promise.resolve( makeResponse( 500, '42' ) ); + } ); + + await expect( apiFetch( uploadOptions() ) ).resolves.toEqual( { + id: 42, + } ); + + expect( error ).not.toHaveBeenCalled(); + } ); } ); diff --git a/src/utils/api-fetch.js b/src/utils/api-fetch.js index 590a44780..43c7907e5 100644 --- a/src/utils/api-fetch.js +++ b/src/utils/api-fetch.js @@ -300,9 +300,10 @@ function nativeMediaUpload( options, port, token ) { // failure. if ( options.parse === false ) { if ( ! response.ok ) { - logError( - `Native upload failed with status ${ response.status }` - ); + // A handoff to core's post-process retry, not an outcome — + // core reads `x-wp-upload-attachment-id` off this response and + // may still recover. Stay silent (as `nativeMediaDelete` does) + // rather than reporting a failure that hasn't happened yet. return Promise.reject( response ); } return response; From 1247d09f3969a4f2e0424c5f95f62b6c7b8b2a9e Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:18:26 -0600 Subject: [PATCH 10/21] refactor: split the media delegate into MediaProcessor and MediaUploader Make performing a media upload -- and retrying it -- a single, all-or-nothing responsibility: either GutenbergKit performs the upload and owns its retries, or the host does (say, to run it through its own networking so it can log every request). Both go to the same configured site; the only difference is who executes the requests. There is no in-between where the host performs the upload but GutenbergKit retries it. When an upload fatals in server-side post-processing it has to be retried: core retries POST .../post-process up to 5x, and cleans up the orphan if that fails. The old `MediaUploadDelegate.uploadFile` let a host perform the upload itself by returning the raw response it received -- which split one upload's HTTP across two owners: the host performed the POST /wp/v2/media, then core, reading that raw response, drove the post-process retries (and the orphan cleanup) behind it. A host that took over uploads to run them through its own stack still didn't own the retries; those went out through the browser, not the host. Delivery and its retries were owned by different parties. Make the upload and its retries one unit with one owner: - `MediaProcessor` (handlesFile, processFile) only transforms the file. It never performs the upload, so GutenbergKit performs it and owns the retries -- the extension point almost every host wants. - `MediaUploader` (upload) performs the upload on the host's own stack (its networking, logging, retry policy, a background session) and owns the whole lifecycle. `upload` returns the finished attachment or throws: there's no raw response for core to retry behind it, so the host drives its own post-process recovery and force-deletes its own orphan on terminal failure. All-or-nothing: the host performs the upload and its retries, or GutenbergKit does -- never a split. An uploader and GutenbergKit's built-in default both target the same configured site; the choice is only who executes the requests. Media deletes always relay to the default uploader (the configured site): every attachment lives there, even one a host uploader delivered, so there is no per-host delete path. The relay is left unscoped -- core issues its cleanup DELETE there, but the relay can't tell it from any other DELETE the WebView sends, so a client-side-compromised editor holding the loopback token could force-delete media on the site. Accepted -- such a script already has broad write access, and a server-side compromise deletes media directly without the editor. An earlier revision carried a per-session ledger to scope the relay; dropped as not worth the cost for a client-side-only threat. `EditorViewController`/`GutenbergView` expose `mediaProcessor` + `mediaUploader` in place of `mediaUploadDelegate`; the server starts if either is set and builds a default uploader whenever site credentials are present (it delivers GutenbergKit's own uploads and relays every media delete). `MediaUploadResponse` drops to internal -- it is no longer on any public API. Both demos and all tests move to the new protocols. Breaking change: hosts must migrate `mediaUploadDelegate` (WordPress-iOS/Android, Jetpack). iOS and Android suites green; SwiftLint and Detekt clean. --- android/Gutenberg/detekt-baseline.xml | 1 + .../org/wordpress/gutenberg/GutenbergView.kt | 99 +++++--- .../wordpress/gutenberg/MediaUploadServer.kt | 180 +++++++------- .../GutenbergViewUploadServerTest.kt | 22 +- .../gutenberg/MediaUploadServerTest.kt | 215 ++++++++--------- ...ploadDelegate.kt => DemoMediaProcessor.kt} | 11 +- .../example/gutenbergkit/EditorActivity.kt | 2 +- ios/Demo-iOS/Sources/Views/EditorView.swift | 8 +- .../Sources/EditorViewController.swift | 137 ++++++----- .../Sources/Media/MediaUploadDelegate.swift | 118 +++++---- .../Sources/Media/MediaUploadServer.swift | 118 +++++---- .../Media/MediaUploadServerTests.swift | 224 +++++++++--------- 12 files changed, 601 insertions(+), 534 deletions(-) rename android/app/src/main/java/com/example/gutenbergkit/{DemoMediaUploadDelegate.kt => DemoMediaProcessor.kt} (93%) diff --git a/android/Gutenberg/detekt-baseline.xml b/android/Gutenberg/detekt-baseline.xml index 4f6c96915..ce3b4481a 100644 --- a/android/Gutenberg/detekt-baseline.xml +++ b/android/Gutenberg/detekt-baseline.xml @@ -11,6 +11,7 @@ ExplicitItLambdaParameter:EditorAssetsLibrary.kt$EditorAssetsLibrary${ str, it -> str + "%02x".format(it) } FunctionNaming:EditorURLCache.kt$EditorURLCache$private fun __store( response: EditorURLResponse, url: String, httpMethod: EditorHttpMethod, currentDate: Date ) LargeClass:GutenbergView.kt$GutenbergView : FrameLayout + LargeClass:MediaUploadServerTest.kt$MediaUploadServerTest LongMethod:FixtureTests.kt$FixtureTests$@Test fun `request parsing - all basic cases pass`() LongMethod:FixtureTests.kt$FixtureTests$@Test fun `request parsing - all incremental cases pass`() LongMethod:HTTPRequestParser.kt$HTTPRequestParser$fun append(data: ByteArray): Unit 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 0989879ac..ae421c251 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -113,30 +113,44 @@ class GutenbergView : FrameLayout { var requestInterceptor: GutenbergRequestInterceptor = DefaultGutenbergRequestInterceptor() /** - * Optional delegate for customizing media upload behavior (resize, transcode, - * custom upload). + * Transforms media (resize, transcode, …) before GutenbergKit delivers it to + * the configured site. The safe, common extension point — a processor never + * performs the upload itself, so it cannot deliver media to the wrong place. * * Provide this **before the editor loads** — typically right after * construction (e.g. in the `AndroidView` factory). It is captured once, when - * the page begins loading, and advertised to the page then; setting it - * afterward has no effect, so the setter throws to surface the mistake. + * the page begins loading; setting it afterward has no effect, so the setter + * throws to surface the mistake. */ - var mediaUploadDelegate: MediaUploadDelegate? = null + var mediaProcessor: MediaProcessor? = null set(value) { - check(!hasStartedLoading) { - "mediaUploadDelegate must be set before the editor loads (e.g. right " + - "after construction). It is captured when the page begins loading; " + - "setting it afterward has no effect." - } + check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaProcessor") } field = value } + /** + * Takes over media upload on the host's own stack (background service, offline + * queue, resumable transport). Setting it makes the host own every upload and + * its whole lifecycle; GutenbergKit stays out of the network entirely for media. + * + * Same lifecycle rules as [mediaProcessor]: set it before the editor loads. + */ + var mediaUploader: MediaUploader? = null + set(value) { + check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaUploader") } + field = value + } + + private fun lateMediaAssignmentMessage(name: String) = + "$name must be set before the editor loads (e.g. right after construction). " + + "It is captured when the page begins loading; setting it afterward has no effect." + @Volatile private var uploadServer: MediaUploadServer? = null /** * True once the editor page has begun loading and the upload server's - * configuration has been captured. After this the [mediaUploadDelegate] can no - * longer take effect, so its setter throws. + * configuration has been captured. After this the [mediaProcessor]/[mediaUploader] + * can no longer take effect, so their setters throw. */ @Volatile private var hasStartedLoading = false @@ -638,13 +652,13 @@ class GutenbergView : FrameLayout { /** * Invoked when the editor page begins loading. Starts the upload server once — - * capturing the [mediaUploadDelegate] provided before load — then advertises - * the editor globals (including the server's port and token) to the page. + * capturing the [mediaProcessor]/[mediaUploader] provided before load — then + * advertises the editor globals (including the server's port and token) to the page. * * Starting the server here, on the UI thread, rather than from the - * [mediaUploadDelegate] setter keeps its whole lifecycle — start here, stop in - * [onDetachedFromWindow] — on the UI thread, so it can't race a - * background-thread delegate assignment. + * [mediaProcessor]/[mediaUploader] setters keeps its whole lifecycle — start + * here, stop in [onDetachedFromWindow] — on the UI thread, so it can't race a + * background-thread assignment. */ private fun onEditorPageStarted() { if (!hasStartedLoading) { @@ -671,17 +685,22 @@ class GutenbergView : FrameLayout { } private fun startUploadServer() { - // No delegate means nothing wants to customize uploads, so there's no reason - // to route them through the native server — leave it down and let uploads - // fall to the default WebView path. (Matches iOS.) - if (mediaUploadDelegate == null) return - - // The native upload server relays through DefaultMediaUploader, which needs a - // site root and an auth header (every host provides one — the editor injects - // it because the WebView has no auth cookies). Without both there is nothing - // 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 + // Nothing to route through the native server unless the host provided a + // processor or an uploader. (Matches iOS.) + if (mediaProcessor == null && mediaUploader == null) return + + // A DefaultMediaUploader delivers GutenbergKit-owned uploads (when no uploader + // is set) and relays the editor's media DELETEs to the configured site — every + // attachment lives there, even one a host uploader delivered. It needs a site + // root and an auth header (every host provides one — the editor injects it + // because the WebView has no auth cookies). If GutenbergKit would have to + // deliver uploads itself but lacks those, there's nothing to upload through, so + // leave the server down and let uploads fall to the default WebView path. + if (mediaUploader == null && + (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 @@ -701,14 +720,24 @@ class GutenbergView : FrameLayout { } try { - val defaultUploader = DefaultMediaUploader( - httpClient = uploadHttpClient, - siteApiRoot = configuration.siteApiRoot, - authHeader = configuration.authHeader, - siteApiNamespace = configuration.siteApiNamespace.toList() - ) + // Build a DefaultMediaUploader whenever there are credentials to reach the + // site: it delivers GutenbergKit-owned uploads and relays the editor's + // DELETEs there. null only when a host owns uploads and no creds exist. + val defaultUploader = if ( + configuration.siteApiRoot.isNotEmpty() && configuration.authHeader.isNotEmpty() + ) { + DefaultMediaUploader( + httpClient = uploadHttpClient, + siteApiRoot = configuration.siteApiRoot, + authHeader = configuration.authHeader, + siteApiNamespace = configuration.siteApiNamespace.toList() + ) + } else { + null + } uploadServer = MediaUploadServer( - uploadDelegate = mediaUploadDelegate, + processor = mediaProcessor, + uploader = mediaUploader, defaultUploader = defaultUploader, cacheDir = context.cacheDir, scope = coroutineScope 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 a65ceb301..a2b944912 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -31,8 +31,8 @@ import okio.source * so every consumer — image sub-sizes, attachment links, error notices — * behaves identically to a non-native upload. */ -class MediaUploadResponse( - /** The HTTP status code WordPress (or the host's upload service) returned. */ +internal class MediaUploadResponse( + /** The HTTP status code WordPress returned. */ val statusCode: Int, /** * The raw response body — a WordPress REST attachment on success, or a @@ -52,7 +52,7 @@ class MediaUploadResponse( ) /** - * The result of a delegate's [MediaUploadDelegate.processFile]. + * The result of a [MediaProcessor.processFile]. */ sealed class ProcessedProxyFile { /** The delegate did not modify the file; the original upload is forwarded unchanged. */ @@ -68,72 +68,70 @@ sealed class ProcessedProxyFile { } /** - * Interface for customizing media upload behavior. + * Transforms media before GutenbergKit delivers it. * - * The native host app can provide an implementation to resize images, - * transcode video, or use its own upload service. + * A processor only changes *bytes* — GutenbergKit still uploads the result to the + * configured site and owns the whole lifecycle (retries, cleanup). Because it + * never performs the upload itself, a processor cannot deliver media to the wrong + * place. Set [GutenbergView.mediaProcessor] to resize images, transcode video, + * strip EXIF, etc. This is the safe, common extension point: most hosts want only + * this. */ -interface MediaUploadDelegate { +interface MediaProcessor { /** - * Whether this delegate might handle a file with the given metadata — either - * processing it ([processFile]) or uploading it itself ([uploadFile]). - * - * A cheap, metadata-only gate the server consults *before* materializing the - * upload to a temp file. Return false to decline a file by type — e.g. an - * image-only delegate returning false for a video — so the server forwards - * the original upload to WordPress without first copying a file the delegate - * won't touch. Because it gates the temp-file copy needed by *both* - * [processFile] and [uploadFile], return true for any file the delegate will - * either process or upload itself. - * - * Defaults to true: every file is materialized and the full pipeline runs. A - * true here is not a commitment — [processFile] may still return - * [ProcessedProxyFile.Original] after inspecting the file's contents. + * Whether this processor might transform a file with the given metadata. A + * cheap, metadata-only gate consulted *before* the upload is materialized to a + * temp file; return false to pass a file straight through untouched — e.g. an + * image-only processor returning false for a video. Defaults to true; not a + * commitment, since [processFile] may still return [ProcessedProxyFile.Original] + * after inspecting the file's contents. */ fun handlesFile(mimeType: String, filename: String): Boolean = true /** - * Process a file before upload (e.g., resize image, transcode video). - * - * 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. + * Transform a file before upload (e.g., resize image, transcode video). Return + * [ProcessedProxyFile.Original] to upload it unchanged, or + * [ProcessedProxyFile.Processed] with the new file and its metadata (report the + * new mimeType and filename when the format changes, so WordPress stores it + * with the correct extension and type). */ suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile = ProcessedProxyFile.Original +} +/** + * Takes over performing a media upload — on the host's own stack: its own + * networking (say, to log every request), a background service, an offline queue, + * a resumable transport, its own retry policy. + * + * This is a choice of who executes the requests, not where they go: an uploader + * and GutenbergKit's built-in default both target the same configured site. + * Setting [GutenbergView.mediaUploader] makes the host own that upload end-to-end — + * the request, its own retries, and its recovery and cleanup — with GutenbergKit + * out of the network entirely. Because the host does the retries itself, there's no + * raw response left for core to retry behind it. The attachment you return lives on + * that same configured site, where the editor reads and updates it by ID. + */ +interface MediaUploader { /** - * Upload a processed file to the remote WordPress site. - * - * 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): MediaUploadResponse? = null - - /** - * Delete a previously uploaded attachment. + * Upload a (possibly processed) file and return the finished WordPress + * attachment JSON the editor inserts — the same object a direct + * `POST /wp/v2/media` returns. Return only once the upload is genuinely done, or + * throw on terminal failure: a returned value is taken as a completed attachment, + * and there is no GutenbergKit recovery behind you. * - * The editor deletes the attachment when an upload's server-side - * post-processing fails past recovery, so it does not leave an orphan - * behind. A delegate that uploaded the attachment itself via [uploadFile] - * owns an ID only it can resolve, so it must delete the attachment itself - * too — the default uploader would address the wrong site. + * That recovery is yours to run. When `POST /wp/v2/media` fatals in server-side + * post-processing it returns a 5xx carrying the attachment's ID in + * `x-wp-upload-attachment-id` — the attachment exists but is unfinished. Don't + * re-upload; drive `POST /wp/v2/media//post-process` to completion, the way + * core recovers its own uploads (up to 5 attempts), then return the finished + * attachment. * - * Return the raw response (status code + body), which GutenbergKit relays - * to the editor unchanged, or null to use the default uploader. - * - * Return null for any ID the delegate does not recognize. Unlike the upload - * path, there is no [handlesFile] gate here — an attachment ID carries no - * MIME type or filename — so this method is called for *every* deletion, - * including attachments the delegate declined at upload time and WordPress - * therefore created itself. Returning a response for one of those (an error - * from the host's own media service, say) leaves the real WordPress - * attachment undeleted — precisely the orphan this cleanup exists to remove. - * null hands it to the default uploader, which addresses the right site. + * Owning the upload means owning cleanup on the server too: if post-process + * can't be recovered, force-delete the orphan (`DELETE /wp/v2/media/?force=true`) + * before you throw, or it stays on the site — neither GutenbergKit nor core + * cleans up behind you. */ - suspend fun deleteFile(attachmentId: String): MediaUploadResponse? = null + suspend fun upload(file: File, mimeType: String, filename: String): ByteArray } /** @@ -149,7 +147,8 @@ interface MediaUploadDelegate { * stop on detach. */ internal class MediaUploadServer( - private val uploadDelegate: MediaUploadDelegate?, + private val processor: MediaProcessor?, + private val uploader: MediaUploader?, private val defaultUploader: DefaultMediaUploader?, cacheDir: File? = null, scope: CoroutineScope? = null, @@ -278,24 +277,32 @@ internal class MediaUploadServer( } /** - * Relays the editor's orphan cleanup. + * Relays a media deletion. * - * Core's media upload middleware deletes the attachment when every - * `post-process` retry fails. A cross-origin editor cannot issue that - * request directly — api-fetch tunnels `DELETE` as a `POST` carrying - * `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the - * browser blocks it at preflight. Relaying it here lets the cleanup run. + * The editor deletes an attachment when the user removes it, and core deletes + * an upload's orphan when every `post-process` retry fails. A cross-origin + * editor cannot issue `DELETE` directly — api-fetch tunnels it as a `POST` + * carrying `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the + * browser blocks it at preflight; relaying it here lets the deletion run. * - * Offers the deletion to the delegate first, as [handleUpload] does, so a - * host that uploaded the attachment itself deletes it from the same place. + * Every attachment lives on the configured site — even one a host uploader + * delivered — so its deletion is relayed to the default uploader there. See + * the accepted-risk note in the body. */ @Suppress("TooGenericExceptionCaught") private suspend fun handleMediaDelete(attachmentId: String, query: String): HttpResponse { return try { - uploadDelegate?.deleteFile(attachmentId)?.let { return relayResponse(it) } - - val uploader = defaultUploader ?: return errorResponse(500, "No uploader configured") - relayResponse(uploader.deleteMedia(attachmentId, query)) + // Relay to the default uploader (the configured site) — every attachment + // lives there, even one a host uploader delivered. Core issues this only + // as orphan cleanup after failed recovery, but the relay can't tell that + // from any other DELETE the WebView sends: a compromised editor script + // holding the loopback token could force-delete arbitrary media on the + // configured site. Accepted risk — such a script already has broad write + // access, and a server-side compromise (a malicious plugin) deletes media + // directly without the editor, so scoping this with a per-session ledger + // buys little for the cost. + val defaultUploader = defaultUploader ?: return errorResponse(500, "No uploader configured") + relayResponse(defaultUploader.deleteMedia(attachmentId, query)) } catch (e: kotlin.coroutines.cancellation.CancellationException) { throw e // Never swallow coroutine cancellation. } catch (e: Exception) { @@ -317,11 +324,13 @@ internal class MediaUploadServer( val mimeType = filePart.contentType val filename = filePart.filename ?: "upload" - // Ask the delegate — from metadata alone — whether it will touch a file - // like this. If not, forward the original upload to WordPress directly, - // skipping a full temp-file copy of a file the delegate won't process or - // upload (e.g. a video handed to an image-only delegate). - if (uploadDelegate?.handlesFile(mimeType, filename) != true) { + // Materialize a temp file only if someone will touch it: a processor that + // claims this file, or an uploader (which always delivers the file itself). + // If GutenbergKit will deliver (no uploader) and no processor wants the + // file, forward the original request body directly, skipping a temp copy of + // a file nobody will process (e.g. a video handed to an image-only processor). + val processorWantsFile = processor?.handlesFile(mimeType, filename) == true + if (uploader == null && !processorWantsFile) { return passthroughResponse(request, query) } @@ -424,8 +433,8 @@ internal class MediaUploadServer( uploadResult.response } is UploadResult.Passthrough -> { - // Delegate didn't modify the file — forward the original - // request body to WordPress without re-encoding. + // No uploader is set and the processor left the file unmodified — + // forward the original request body without re-encoding. Log.d(TAG, "Passthrough: forwarding original request body to WordPress") performPassthroughUpload(request, query) } @@ -450,7 +459,7 @@ internal class MediaUploadServer( } } - // MARK: - Delegate Pipeline + // MARK: - Process + Deliver Pipeline private sealed class UploadResult { data class Uploaded(val response: MediaUploadResponse) : UploadResult() @@ -471,10 +480,15 @@ internal class MediaUploadServer( file: File, mimeType: String, filename: String, extraParts: List, query: String ): UploadResult { - val processed = uploadDelegate?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original + // Transform (resize, transcode, …) if a processor claims the file. + val processed = if (processor?.handlesFile(mimeType, filename) == true) { + processor.processFile(file, mimeType, filename) + } else { + 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. + // processor's values verbatim, so a format change is reported to WordPress. val targetFile: File val targetMimeType: String val targetFilename: String @@ -492,9 +506,11 @@ internal class MediaUploadServer( } try { - // If the delegate provided its own upload, use that. - uploadDelegate?.uploadFile(targetFile, targetMimeType, targetFilename)?.let { - return UploadResult.Uploaded(it) + // An uploader owns delivery on the host's own stack and returns the + // finished attachment JSON (or throws); GutenbergKit relays that as a + // success and never runs its own recovery behind it. + uploader?.let { + return UploadResult.Uploaded(MediaUploadResponse(201, it.upload(targetFile, targetMimeType, targetFilename))) } // Unmodified — forward the original request body directly, skipping @@ -504,7 +520,7 @@ internal class MediaUploadServer( } val result = defaultUploader?.upload(targetFile, targetMimeType, targetFilename, extraParts, query) - ?: error("No upload delegate or default uploader configured") + ?: error("No uploader or default uploader configured") return UploadResult.Uploaded(result) } finally { // The processed file (if the delegate produced a new one) is ours to diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt index a83cfe5f5..7ea179401 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt @@ -62,15 +62,15 @@ class GutenbergViewUploadServerTest { private fun idle() = shadowOf(Looper.getMainLooper()).idle() @Test - fun `the upload server starts when the page begins loading, capturing the delegate`() { + fun `the upload server starts when the page begins loading, capturing the processor`() { val view = makeView() try { - // A delegate provided before load is captured when the page starts. - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + // A processor provided before load is captured when the page starts. + view.mediaProcessor = mock(MediaProcessor::class.java) startLoading(view) idle() assertNotNull( - "a delegate provided before load should bring up the upload server", + "a processor provided before load should bring up the upload server", uploadServerOf(view) ) } finally { @@ -79,14 +79,14 @@ class GutenbergViewUploadServerTest { } @Test - fun `no delegate means no upload server`() { + fun `no processor or uploader means no upload server`() { val view = makeView() try { - // No delegate provided — uploads should use the default WebView path. + // Nothing provided — uploads should use the default WebView path. startLoading(view) idle() assertNull( - "with no delegate, no upload server should be started", + "with no processor or uploader, no upload server should be started", uploadServerOf(view) ) } finally { @@ -95,15 +95,15 @@ class GutenbergViewUploadServerTest { } @Test - fun `setting the delegate after the page has started loading throws`() { + fun `setting a media handler after the page has started loading throws`() { val view = makeView() try { startLoading(view) idle() - // The delegate is captured at load; a later assignment is a programmer + // The processor is captured at load; a later assignment is a programmer // error and must surface loudly rather than silently do nothing. assertThrows(IllegalStateException::class.java) { - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + view.mediaProcessor = mock(MediaProcessor::class.java) } } finally { detach(view) @@ -113,7 +113,7 @@ class GutenbergViewUploadServerTest { @Test fun `detaching the view stops and clears the upload server`() { val view = makeView() - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + view.mediaProcessor = mock(MediaProcessor::class.java) startLoading(view) idle() assertNotNull(uploadServerOf(view)) 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 6cdc04da7..78ab70bd0 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -33,7 +33,7 @@ class MediaUploadServerTest { @Before fun setUp() { - server = MediaUploadServer(uploadDelegate = null, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = null, uploader = null, defaultUploader = null, cacheDir = tempFolder.root) } @After @@ -53,7 +53,7 @@ class MediaUploadServerTest { fun `stop cancels an internally-created scope but leaves a caller-supplied one alone`() { // No scope supplied → the server owns one, which stop() must cancel. val owningServer = - MediaUploadServer(uploadDelegate = null, defaultUploader = null, cacheDir = tempFolder.root) + MediaUploadServer(processor = null, uploader = null, defaultUploader = null, cacheDir = tempFolder.root) val ownedScope = ownedScopeOf(owningServer) assertNotNull("server should own a scope when none is supplied", ownedScope) assertTrue(ownedScope!!.isActive) @@ -63,7 +63,8 @@ class MediaUploadServerTest { // A caller-supplied scope belongs to the caller — stop() must not cancel it. val callerScope = CoroutineScope(Dispatchers.IO) val borrowingServer = MediaUploadServer( - uploadDelegate = null, + processor = null, + uploader = null, defaultUploader = null, cacheDir = tempFolder.root, scope = callerScope @@ -141,14 +142,19 @@ class MediaUploadServerTest { } @Test - fun `routes a deletion to the delegate when it handles one`() { - // A host that uploaded the attachment itself owns an ID only it can - // resolve, so the default uploader must not be asked to delete it. No - // default uploader is configured, so a 200 here can only come from the - // delegate — the fallback path would fail with "no uploader". - val delegate = DeletingDelegate() + fun `relays a deletion to the default uploader even when an uploader owns uploads`() { + // An attachment lives on the configured site even when a host uploader + // delivered it, so its deletion goes to the default uploader — the host + // uploader owns uploads, not deletes. + val uploader = MockUploader() + val defaultUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer( + processor = null, + uploader = uploader, + defaultUploader = defaultUploader, + cacheDir = tempFolder.root + ) val response = sendRawRequest( method = "DELETE", @@ -158,39 +164,43 @@ class MediaUploadServerTest { ) assertTrue("Expected 200 but got: ${response.statusLine}", response.statusLine.contains("200")) - assertEquals("42", delegate.deletedAttachmentId) + assertTrue(defaultUploader.deleteMediaCalled) + assertEquals("42", defaultUploader.deletedAttachmentId) } + // MARK: - Media deletion + @Test - fun `relays a delegate's own Content-Type instead of emitting it twice`() { - // HTTP header names are case-insensitive, so a delegate spelling it - // `content-type` must still override the JSON default rather than merge - // alongside it — HttpResponse serializes every entry it is given, which - // would put the name on the wire twice (mirrors the iOS behavior). - val delegate = ContentTypeDeletingDelegate() + fun `relays a deletion to the default uploader (configured site)`() { + // With no uploader set, GutenbergKit owns deletes: core's orphan cleanup + // DELETE is relayed to the default uploader (the configured site). + val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer( + processor = null, + uploader = null, + defaultUploader = mockUploader, + cacheDir = tempFolder.root + ) val response = sendRawRequest( method = "DELETE", - path = "/media/42?force=true", + path = "/media/512?force=true", headers = mapOf("Relay-Authorization" to "Bearer ${server.token}"), body = ByteArray(0) ) assertTrue("Expected 200 but got: ${response.statusLine}", response.statusLine.contains("200")) - // Assert on the raw header lines, not the parsed map: the parser - // lowercases keys into a map, so a duplicated header would silently - // collapse and this test would pass against the very bug it covers. - assertEquals(listOf("text/plain"), response.rawHeaderValues("content-type")) + assertTrue(mockUploader.deleteMediaCalled) + assertEquals("512", mockUploader.deletedAttachmentId) } @Test fun `routes upload with a query string and relays the query`() { - val delegate = ProcessOnlyDelegate() + val processor = PassthroughProcessor() val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, 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 @@ -209,7 +219,7 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - // The delegate returns Original, so this is the passthrough branch. + // The processor 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) @@ -217,13 +227,21 @@ class MediaUploadServerTest { assertEquals("?_embed=wp:featuredmedia", mockUploader.lastQuery) } - // MARK: - Upload with delegate + // MARK: - Upload with a processor or uploader @Test - fun `calls delegate processFile and uploadFile`() { - val delegate = MockUploadDelegate() + fun `routes an upload to the uploader and relays its attachment`() { + // With an uploader set, GutenbergKit hands it the file and relays the finished + // attachment it returns — the default uploader (configured site) is never used. + val uploader = MockUploader() + val defaultUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer( + processor = null, + uploader = uploader, + defaultUploader = defaultUploader, + cacheDir = tempFolder.root + ) val boundary = "test-boundary-123" val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) @@ -239,12 +257,14 @@ class MediaUploadServerTest { ) 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. + assertTrue(uploader.uploadCalled) + assertEquals("image/jpeg", uploader.lastMimeType) + assertEquals("photo.jpg", uploader.lastFilename) + // The host owns delivery — GutenbergKit must not upload to the configured site. + assertFalse(defaultUploader.uploadCalled) + assertFalse(defaultUploader.passthroughUploadCalled) + + // The server relays the exact attachment JSON the uploader returned. val json = JsonParser.parseString(response.body).asJsonObject assertEquals(42, json.get("id").asInt) assertEquals("https://example.com/photo.jpg", json.get("source_url").asString) @@ -252,11 +272,11 @@ class MediaUploadServerTest { } @Test - fun `forwards the delegate's processed metadata to the uploader`() { - val delegate = TranscodingDelegate() + fun `forwards the processor's processed metadata to the uploader`() { + val processor = TranscodingProcessor() val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-meta" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -271,7 +291,7 @@ class MediaUploadServerTest { body = body ) - // The delegate changed the format, so the uploader must receive the new + // The processor 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) @@ -279,11 +299,11 @@ class MediaUploadServerTest { } @Test - fun `deletes the delegate's processed file after upload`() { - val delegate = TranscodingDelegate() + fun `deletes the processor's processed file after upload`() { + val processor = TranscodingProcessor() val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-cleanup" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -298,10 +318,10 @@ class MediaUploadServerTest { body = body ) - // The server owns the file the delegate produced and must delete it once the + // The server owns the file the processor 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" } + val processed = requireNotNull(processor.producedFile) { "processFile was not called" } assertFalse("Processed temp file should be deleted after upload", processed.exists()) } @@ -321,7 +341,8 @@ class MediaUploadServerTest { // one — a flipped comparison would do the opposite and wipe an in-flight upload. server.stop() server = MediaUploadServer( - uploadDelegate = null, + processor = null, + uploader = null, defaultUploader = null, cacheDir = tempFolder.root, ioDispatcher = Dispatchers.Unconfined @@ -334,12 +355,12 @@ class MediaUploadServerTest { // MARK: - Fallback to default uploader @Test - fun `uses passthrough when delegate does not modify file`() { - val delegate = ProcessOnlyDelegate() + fun `uses passthrough when the processor does not modify the file`() { + val processor = PassthroughProcessor() val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-456" val body = buildMultipartBody(boundary, "doc.pdf", "application/pdf", "fake pdf data".toByteArray()) @@ -355,7 +376,7 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - assertTrue(delegate.processFileCalled) + assertTrue(processor.processFileCalled) // Passthrough: original body forwarded directly, not re-encoded. assertTrue(mockUploader.passthroughUploadCalled) assertFalse(mockUploader.uploadCalled) @@ -365,12 +386,12 @@ class MediaUploadServerTest { } @Test - fun `skips processing and the temp copy when the delegate declines by metadata`() { - val delegate = DeclineByMetadataDelegate() + fun `skips processing and the temp copy when the processor declines by metadata`() { + val processor = DecliningProcessor() val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-decline" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "fake movie".toByteArray()) @@ -386,9 +407,9 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - // Declined by metadata → the delegate is never asked to process (so the + // Declined by metadata → the processor is never asked to process (so the // file was never materialized), and the upload is passed through directly. - assertFalse(delegate.processFileCalled) + assertFalse(processor.processFileCalled) assertTrue(mockUploader.passthroughUploadCalled) assertFalse(mockUploader.uploadCalled) } @@ -773,54 +794,27 @@ class MediaUploadServerTest { // MARK: - Mocks - private class MockUploadDelegate : MediaUploadDelegate { - @Volatile var processFileCalled = false - @Volatile var uploadFileCalled = false + /** + * A host uploader: it performs the upload on its own stack. `upload` returns the + * finished attachment JSON (or throws). + */ + private class MockUploader( + private val uploadBody: ByteArray = + """{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}""".toByteArray() + ) : MediaUploader { + @Volatile var uploadCalled = false @Volatile var lastMimeType: String? = null @Volatile var lastFilename: String? = null - override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { - processFileCalled = true + override suspend fun upload(file: File, mimeType: String, filename: String): ByteArray { + uploadCalled = true lastMimeType = mimeType - return ProcessedProxyFile.Original - } - - override suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? { - uploadFileCalled = true lastFilename = filename - val json = """{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}""" - return MediaUploadResponse(201, json.toByteArray()) - } - } - - /** - * A delegate that handles deletions itself, as a host uploading to its own - * media service would. - */ - private class DeletingDelegate : MediaUploadDelegate { - @Volatile var deletedAttachmentId: String? = null - - override suspend fun deleteFile(attachmentId: String): MediaUploadResponse? { - deletedAttachmentId = attachmentId - return MediaUploadResponse(200, """{"deleted":true}""".toByteArray()) - } - } - - /** - * A delegate that sets its own `Content-Type`, lowercased, so the relay must - * override the JSON default rather than emit the header twice. - */ - private class ContentTypeDeletingDelegate : MediaUploadDelegate { - override suspend fun deleteFile(attachmentId: String): MediaUploadResponse? { - return MediaUploadResponse( - 200, - "deleted".toByteArray(), - mapOf("content-type" to "text/plain") - ) + return uploadBody } } - private class ProcessOnlyDelegate : MediaUploadDelegate { + private class PassthroughProcessor : MediaProcessor { @Volatile var processFileCalled = false override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { @@ -833,7 +827,7 @@ class MediaUploadServerTest { * Declines every file by metadata via [handlesFile], so the server must pass * through without materializing the file or calling [processFile]. */ - private class DeclineByMetadataDelegate : MediaUploadDelegate { + private class DecliningProcessor : MediaProcessor { @Volatile var processFileCalled = false override fun handlesFile(mimeType: String, filename: String): Boolean = false @@ -844,9 +838,9 @@ class MediaUploadServerTest { } } - /** 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. */ + /** A processor that produces a new file with changed metadata (e.g. a transcode). */ + private class TranscodingProcessor : MediaProcessor { + /** The processed file this processor wrote, for cleanup assertions. */ @Volatile var producedFile: File? = null override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { @@ -857,7 +851,13 @@ class MediaUploadServerTest { } } - private class MockDefaultUploader : DefaultMediaUploader( + private class MockDefaultUploader( + /** The response `upload`/`passthroughUpload` return. Defaults to a 201 success. */ + private val uploadResponse: MediaUploadResponse = MediaUploadResponse( + 201, + """{"id":99,"source_url":"https://example.com/doc.pdf","media_type":"file"}""".toByteArray() + ) + ) : DefaultMediaUploader( httpClient = okhttp3.OkHttpClient(), siteApiRoot = "https://example.com/wp-json/", authHeader = "Bearer mock" @@ -867,6 +867,8 @@ class MediaUploadServerTest { @Volatile var lastUploadMimeType: String? = null @Volatile var lastUploadFilename: String? = null @Volatile var lastQuery: String? = null + @Volatile var deleteMediaCalled = false + @Volatile var deletedAttachmentId: String? = null override suspend fun upload( file: File, mimeType: String, filename: String, @@ -876,7 +878,7 @@ class MediaUploadServerTest { lastUploadMimeType = mimeType lastUploadFilename = filename lastQuery = query - return mockResponse() + return uploadResponse } override suspend fun passthroughUpload( @@ -886,13 +888,14 @@ class MediaUploadServerTest { ): MediaUploadResponse { passthroughUploadCalled = true lastQuery = query - return mockResponse() + return uploadResponse } - private fun mockResponse() = MediaUploadResponse( - 201, - """{"id":99,"source_url":"https://example.com/doc.pdf","media_type":"file"}""".toByteArray() - ) + override suspend fun deleteMedia(attachmentId: String, query: String): MediaUploadResponse { + deleteMediaCalled = true + deletedAttachmentId = attachmentId + return MediaUploadResponse(200, """{"deleted":true}""".toByteArray()) + } } } diff --git a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt similarity index 93% rename from android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt rename to android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt index 572836e4c..ea7de6ca4 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt @@ -5,19 +5,18 @@ 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.MediaProcessor 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. - * - * Only overrides [processFile] — [uploadFile] returns null so the default uploader is used. + * Demo media processor that resizes images to a maximum dimension of 2000px, then + * lets GutenbergKit deliver the result to the configured site. */ -class DemoMediaUploadDelegate : MediaUploadDelegate { +class DemoMediaProcessor : MediaProcessor { companion object { - private const val TAG = "DemoMediaUploadDelegate" + private const val TAG = "DemoMediaProcessor" } // Only non-GIF images are ever resized (see processFile), so decline diff --git a/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt b/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt index 20f3e84b2..c2a48ac0f 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt @@ -338,7 +338,7 @@ fun EditorScreen( } }) if (enableNativeMediaUpload) { - mediaUploadDelegate = DemoMediaUploadDelegate() + mediaProcessor = DemoMediaProcessor() } onGutenbergViewCreated(this) } diff --git a/ios/Demo-iOS/Sources/Views/EditorView.swift b/ios/Demo-iOS/Sources/Views/EditorView.swift index 0f9b56ca4..c358db526 100644 --- a/ios/Demo-iOS/Sources/Views/EditorView.swift +++ b/ios/Demo-iOS/Sources/Views/EditorView.swift @@ -136,7 +136,7 @@ private struct _EditorView: UIViewControllerRepresentable { let viewController = EditorViewController(configuration: configuration, dependencies: dependencies) viewController.delegate = context.coordinator if enableNativeMediaUpload { - viewController.mediaUploadDelegate = context.coordinator + viewController.mediaProcessor = context.coordinator } viewController.webView.isInspectable = true @@ -189,7 +189,7 @@ private struct _EditorView: UIViewControllerRepresentable { } @MainActor - class Coordinator: NSObject, EditorViewControllerDelegate, MediaUploadDelegate { + class Coordinator: NSObject, EditorViewControllerDelegate, MediaProcessor { let viewModel: EditorViewModel init(viewModel: EditorViewModel) { @@ -295,11 +295,11 @@ private struct _EditorView: UIViewControllerRepresentable { return nil } - // MARK: - MediaUploadDelegate + // MARK: - MediaProcessor /// Only non-GIF images are ever resized (see `processFile`), so decline /// everything else by metadata — the server then skips copying a file - /// this delegate would only pass through. + /// this processor would only pass through. nonisolated func handlesFile(ofType mimeType: String, named _: String) -> Bool { mimeType.hasPrefix("image/") && mimeType != "image/gif" } diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index da4c1fefe..86de817ae 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -105,52 +105,62 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro private let isWarmupMode: Bool /// Set once the editor has begun loading and captured its configuration - /// (including ``mediaUploadDelegate``). After this, that delegate can no longer - /// take effect, so its setter traps if written. + /// (including ``mediaProcessor`` and ``mediaUploader``). After this, they can no + /// longer take effect, so their setters trap if written. private var hasStartedLoading = false - /// Whether a non-nil ``mediaUploadDelegate`` was ever assigned. Lets the load - /// path tell "the delegate was released before load" (a retention mistake to - /// trap) apart from "no delegate was configured" (a valid opt-out). - private var mediaUploadDelegateWasAssigned = false + /// Whether a non-nil ``mediaProcessor``/``mediaUploader`` was ever assigned. Lets + /// the load path tell "the host object was released before load" (a retention + /// mistake to trap) apart from "none was configured" (a valid opt-out). + private var mediaProcessorWasAssigned = false + private var mediaUploaderWasAssigned = false - /// Delegate for customizing media file processing and upload behavior. + /// Transforms media (resize, transcode, …) before GutenbergKit delivers it to + /// the configured site. The safe, common extension point — a processor never + /// performs the upload itself, so it cannot deliver media to the wrong place. /// - /// Provide this **before the editor loads** — typically right after `init`, the - /// same way the rest of the editor configuration is supplied. It is captured - /// once, when the editor begins loading, and injected into the page's initial - /// configuration; setting it afterward has no effect, so the setter traps. + /// Provide this **before the editor loads** — typically right after `init`. It + /// is captured once, when the editor begins loading; setting it afterward has no + /// effect, so the setter traps. /// - /// - Important: This is a `weak` reference — you must hold a strong reference to - /// your delegate until the editor has loaded, or native uploads are silently - /// disabled. To surface that mistake, the editor traps at load time if a - /// delegate that was assigned here has already been deallocated. - public weak var mediaUploadDelegate: (any MediaUploadDelegate)? { + /// - Important: This is a `weak` reference — hold a strong reference to your + /// processor until the editor has loaded, or native media handling is silently + /// disabled. The editor traps at load time if a processor assigned here has + /// already been deallocated. + public weak var mediaProcessor: (any MediaProcessor)? { didSet { - // Record whether a delegate was provided so the load path can tell a - // premature deallocation apart from a deliberate opt-out (see - // `startUploadServer`). - mediaUploadDelegateWasAssigned = mediaUploadDelegate != nil - // Deliberate fail-fast, not a defensive check. The delegate is captured - // into the page's initial configuration when the editor begins loading, - // so a delegate assigned afterward would silently never take effect; - // trapping surfaces that misuse loudly instead of failing quietly. - // - // `hasStartedLoading` flips at the start of the async load (see - // `loadEditor`), which runs at or after `viewDidLoad` — so this only - // *widens* the safe window versus a synchronous flip. A host that - // follows the documented contract (set right after `init`, before - // presenting) can never race it; the trap fires only on a genuinely - // late assignment. Do not soften this to a no-op or a log — silently - // dropping the delegate is exactly the failure this is here to catch. - precondition( - !hasStartedLoading, - "mediaUploadDelegate must be set before the editor loads (e.g. right after init). " - + "It is captured into the editor configuration at load; setting it afterward has no effect." - ) + mediaProcessorWasAssigned = mediaProcessor != nil + precondition(!hasStartedLoading, Self.lateMediaAssignmentMessage("mediaProcessor")) } } + /// Takes over media upload on the host's own stack (background session, offline + /// queue, resumable transport). Setting it makes the host own every upload and + /// its whole lifecycle; GutenbergKit stays out of the network entirely for media. + /// + /// Same lifecycle rules as ``mediaProcessor``: set it before the editor loads, + /// and hold a strong reference until then. + public weak var mediaUploader: (any MediaUploader)? { + didSet { + mediaUploaderWasAssigned = mediaUploader != nil + precondition(!hasStartedLoading, Self.lateMediaAssignmentMessage("mediaUploader")) + } + } + + /// Message for the fail-fast when media handling is assigned too late. + /// + /// Deliberate fail-fast, not a defensive check: the processor/uploader is + /// captured into the page's initial configuration when the editor begins + /// loading, so one assigned afterward would silently never take effect. + /// `hasStartedLoading` flips at the start of the async load, which runs at or + /// after `viewDidLoad`, so a host that follows the contract (set right after + /// `init`) can never race it. Do not soften this to a no-op or a log — silently + /// dropping the host's media handling is exactly the failure this catches. + private static func lateMediaAssignmentMessage(_ name: String) -> String { + "\(name) must be set before the editor loads (e.g. right after init). " + + "It is captured into the editor configuration at load; setting it afterward has no effect." + } + // MARK: - Private Properties (Services) private let editorService: EditorService private let httpClient: any EditorHTTPClientProtocol @@ -451,36 +461,51 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// falls back to Gutenberg's default upload behavior (the JS override won't activate /// because `nativeUploadPort` will be nil in GBKit). private func startUploadServer() async { - // A delegate that was provided but is already nil here was deallocated before - // the editor finished loading — the host didn't hold a strong reference to it. - // That silently disables native uploads, so trap loudly instead. + // A processor/uploader that was provided but is already nil here was + // deallocated before the editor finished loading — the host didn't hold a + // strong reference. That silently disables native media handling, so trap. + precondition( + !(mediaProcessorWasAssigned && mediaProcessor == nil), + "mediaProcessor was released before the editor loaded — hold a strong reference to it." + ) precondition( - !(mediaUploadDelegateWasAssigned && mediaUploadDelegate == nil), - "mediaUploadDelegate was released before the editor loaded — hold a strong reference to it." + !(mediaUploaderWasAssigned && mediaUploader == nil), + "mediaUploader was released before the editor loaded — hold a strong reference to it." ) - guard mediaUploadDelegate != nil else { + // Nothing to route through the native server unless the host provided a + // processor or an uploader. + guard mediaProcessor != nil || mediaUploader != nil else { return } - // The native upload server relays through DefaultMediaUploader, which needs a - // site root and an auth header (every host provides one — the editor injects - // it because the WebView has no auth cookies). Without both there is nothing - // 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. - guard !configuration.authHeader.isEmpty else { + // A DefaultMediaUploader does two jobs: it delivers GutenbergKit-owned + // uploads (when no `mediaUploader` is set), and it relays the editor's media + // DELETEs to the configured site — every attachment lives there, even one a + // host uploader delivered, so that's where its deletion goes. It needs a site + // root and an auth header (every host provides one — the editor injects it + // because the WebView has no auth cookies). + // + // If GutenbergKit would have to deliver uploads itself but has no auth + // header, there's nothing to upload through: leave the server down and let + // uploads fall to the default WebView path rather than start a server that + // could only fail. + if mediaUploader == nil && configuration.authHeader.isEmpty { return } - - let defaultUploader = DefaultMediaUploader( - httpClient: httpClient.uploadClient(), - siteApiRoot: configuration.siteApiRoot, - siteApiNamespace: configuration.siteApiNamespace - ) + var defaultUploader: DefaultMediaUploader? + if !configuration.authHeader.isEmpty { + defaultUploader = DefaultMediaUploader( + httpClient: httpClient.uploadClient(), + siteApiRoot: configuration.siteApiRoot, + siteApiNamespace: configuration.siteApiNamespace + ) + } do { self.uploadServer = try await MediaUploadServer.start( - uploadDelegate: mediaUploadDelegate, + processor: mediaProcessor, + uploader: mediaUploader, defaultUploader: defaultUploader ) } catch { diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index 01b0be7d8..bfd9c1b07 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -7,13 +7,13 @@ import Foundation /// 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 +struct MediaUploadResponse: Sendable { + /// The HTTP status code WordPress returned. + 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 + let body: Data /// The response headers to relay to the editor. /// @@ -22,16 +22,16 @@ public struct MediaUploadResponse: Sendable { /// metadata generation fataled, and the editor's api-fetch middleware reads /// it to retry `post-process` and clean up the orphan. Dropping it turns a /// recoverable upload into a permanent failure. - public let headers: [String: String] + let headers: [String: String] - public init(statusCode: Int, body: Data, headers: [String: String] = [:]) { + init(statusCode: Int, body: Data, headers: [String: String] = [:]) { self.statusCode = statusCode self.body = body self.headers = headers } } -/// The result of a delegate's ``MediaUploadDelegate/processFile(at:mimeType:filename:)``. +/// The result of a ``MediaProcessor/processFile(at:mimeType:filename:)``. public enum ProcessedProxyFile: Sendable { /// The delegate did not modify the file; the original upload is forwarded /// to WordPress unchanged. @@ -44,71 +44,71 @@ public enum ProcessedProxyFile: Sendable { case processed(URL, mimeType: String, filename: String) } -/// Protocol for customizing media upload behavior. +/// Transforms media before GutenbergKit delivers it. /// -/// The native host app can provide an implementation to resize images, -/// transcode video, or use its own upload service. Default implementations -/// pass files through unchanged and upload via the WordPress REST API. -public protocol MediaUploadDelegate: AnyObject, Sendable { - /// Whether this delegate might handle a file with the given metadata — either - /// processing it (``processFile(at:mimeType:filename:)``) or uploading it - /// itself (``uploadFile(at:mimeType:filename:)``). +/// A processor only changes *bytes* — GutenbergKit still uploads the result to +/// the configured site and owns the whole lifecycle (retries, cleanup). Because +/// it never performs the upload itself, a processor cannot deliver media to the +/// wrong place. Set ``EditorViewController/mediaProcessor`` to resize images, +/// transcode video, strip EXIF, etc. +/// +/// This is the safe, common extension point: most hosts want only this. +public protocol MediaProcessor: AnyObject, Sendable { + /// Whether this processor might transform a file with the given metadata. /// /// A cheap, metadata-only gate the server consults *before* materializing the - /// upload to a temp file. Return `false` to decline a file by type — e.g. an - /// image-only delegate returning `false` for a video — so the server forwards - /// the original upload to WordPress without first copying a file the delegate - /// won't touch. Because it gates the temp-file copy needed by *both* - /// `processFile` and `uploadFile`, return `true` for any file the delegate - /// will either process or upload itself. + /// upload to a temp file. Return `false` to pass a file straight through + /// untouched — e.g. an image-only processor returning `false` for a video — + /// so the server never copies a file the processor won't touch. /// - /// Defaults to `true`: every file is materialized and the full pipeline runs. - /// A `true` here is not a commitment — `processFile` may still return - /// `.original` after inspecting the file's contents. + /// Defaults to `true`. A `true` here is not a commitment — `processFile` may + /// still return `.original` after inspecting the file's contents. func handlesFile(ofType mimeType: String, named filename: String) -> Bool - /// Process a file before upload (e.g., resize image, transcode video). + /// Transform a file before upload (e.g., resize image, transcode video). /// /// 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 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? - - /// Delete a previously uploaded attachment. - /// - /// The editor deletes the attachment when an upload's server-side - /// post-processing fails past recovery, so it does not leave an orphan - /// behind. A delegate that uploaded the attachment itself via - /// ``uploadFile(at:mimeType:filename:)`` owns an ID only it can resolve, so - /// it must delete the attachment itself too — the default uploader would - /// address the wrong site. +/// Takes over *performing* a media upload — on the host's own stack: its own +/// networking (say, to log every request), a background session, an offline queue, +/// a resumable transport, its own retry policy. +/// +/// This is a choice of *who executes the requests*, not where they go: an uploader +/// and GutenbergKit's built-in default both target the same configured site. +/// Setting ``EditorViewController/mediaUploader`` makes the host own that upload +/// end-to-end — the request, its own retries, and its recovery and cleanup — with +/// GutenbergKit out of the network entirely. Because the host does the retries +/// itself, there's no raw response left for core to retry behind it. The attachment +/// you return lives on that same configured site, where the editor reads and +/// updates it by ID. +public protocol MediaUploader: AnyObject, Sendable { + /// Upload a (possibly processed) file and return the finished WordPress + /// attachment JSON the editor inserts — the same object a direct + /// `POST /wp/v2/media` returns. Return only once the upload is genuinely done, + /// or `throw` on terminal failure: a returned value is taken as a completed + /// attachment, and there is no GutenbergKit recovery behind you. /// - /// Return the raw response (status code + body), which GutenbergKit relays - /// to the editor unchanged, or `nil` to use the default uploader. + /// That recovery is yours to run. When `POST /wp/v2/media` fatals in server-side + /// post-processing it returns a 5xx carrying the attachment's ID in + /// `x-wp-upload-attachment-id` — the attachment exists but is unfinished. Don't + /// re-upload; drive `POST /wp/v2/media//post-process` to completion, the way + /// core recovers its own uploads (up to 5 attempts), then return the finished + /// attachment. /// - /// Return `nil` for any ID the delegate does not recognize. Unlike the upload - /// path, there is no ``handlesFile(ofType:named:)`` gate here — an attachment - /// ID carries no MIME type or filename — so this method is called for *every* - /// deletion, including attachments the delegate declined at upload time and - /// WordPress therefore created itself. Returning a response for one of those - /// (an error from the host's own media service, say) leaves the real - /// WordPress attachment undeleted — precisely the orphan this cleanup exists - /// to remove. `nil` hands it to the default uploader, which addresses the - /// right site. - func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? + /// Owning the upload means owning cleanup on the server too: if post-process + /// can't be recovered, force-delete the orphan + /// (`DELETE /wp/v2/media/?force=true`) before you `throw`, or it stays on the + /// site — neither GutenbergKit nor core cleans up behind you. + func upload(fileAt url: URL, mimeType: String, filename: String) async throws -> Data } -/// Default implementations. -extension MediaUploadDelegate { +/// Default implementations for the optional ``MediaProcessor`` methods. +extension MediaProcessor { public func handlesFile(ofType mimeType: String, named filename: String) -> Bool { true } @@ -116,12 +116,4 @@ extension MediaUploadDelegate { 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 -> MediaUploadResponse? { - nil - } - - public func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { - nil - } } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 0c03d8de2..cee6839f6 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -29,12 +29,14 @@ final class MediaUploadServer: Sendable { /// Creates and starts a new upload server. /// /// - Parameters: - /// - uploadDelegate: Optional delegate for customizing file processing and upload. - /// - defaultUploader: Fallback uploader used when no delegate provides `uploadFile`. + /// - processor: Optional processor for transforming files before upload. + /// - uploader: Optional uploader that takes over delivery on the host's own stack. + /// - defaultUploader: Delivers to the configured site when no uploader is set. /// - maxRequestBodySize: The maximum allowed request body size in bytes. /// Requests exceeding this limit receive a 413 response. Defaults to 4 GB. static func start( - uploadDelegate: (any MediaUploadDelegate)? = nil, + processor: (any MediaProcessor)? = nil, + uploader: (any MediaUploader)? = nil, defaultUploader: DefaultMediaUploader? = nil, maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize ) async throws -> MediaUploadServer { @@ -45,7 +47,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(uploadDelegate: uploadDelegate, defaultUploader: defaultUploader) + let context = UploadContext(processor: processor, uploader: uploader, defaultUploader: defaultUploader) // A generous ceiling for receiving the upload body. The body read is // primarily bounded by the per-read idle timeout (which reaps a stalled @@ -126,11 +128,13 @@ final class MediaUploadServer: Sendable { let filename = filePart.filename ?? "upload" let mimeType = filePart.contentType - // Ask the delegate — from metadata alone — whether it will touch a file - // like this. If not, forward the original upload to WordPress directly, - // skipping a full temp-file copy of a file the delegate won't process or - // upload (e.g. a video handed to an image-only delegate). - guard context.uploadDelegate?.handlesFile(ofType: mimeType, named: filename) ?? false else { + // Materialize a temp file only if someone will touch it: a processor that + // claims this file, or an uploader (which always delivers the file itself). + // If GutenbergKit will deliver (no uploader) and no processor wants the + // file, forward the original request body directly, skipping a temp copy of + // a file nobody will process (e.g. a video handed to an image-only processor). + let processorWantsFile = context.processor?.handlesFile(ofType: mimeType, named: filename) ?? false + if context.uploader == nil, !processorWantsFile { do { return try await passthroughResponse(request, query: query, context: context) } catch { @@ -180,9 +184,9 @@ final class MediaUploadServer: Sendable { } /// Forwards the original request body to WordPress unchanged (no multipart - /// re-encoding) and relays the response. Used when the delegate won't touch - /// the file — it declined by metadata (`handlesFile` returned false) or - /// `processFile` returned `.original`. + /// re-encoding) and relays the response. Used on the no-uploader path when the + /// processor won't touch the file — it declined by metadata (`handlesFile` + /// returned false) or `processFile` returned `.original`. private static func passthroughResponse( _ request: HTTPServer.Request, query: String, context: UploadContext ) async throws -> HTTPResponse { @@ -208,25 +212,30 @@ final class MediaUploadServer: Sendable { return id } - /// Relays the editor's orphan cleanup. + /// Relays a media deletion. /// - /// Core's media upload middleware deletes the attachment when every - /// `post-process` retry fails. A cross-origin editor cannot issue that - /// request directly — api-fetch tunnels `DELETE` as a `POST` carrying - /// `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the - /// browser blocks it at preflight. Relaying it here lets the cleanup run. + /// The editor deletes an attachment when the user removes it, and core deletes + /// an upload's orphan when every `post-process` retry fails. A cross-origin + /// editor cannot issue `DELETE` directly — api-fetch tunnels it as a `POST` + /// carrying `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the + /// browser blocks it at preflight; relaying it here lets the deletion run. /// - /// Offers the deletion to the delegate first, as ``handleUpload(_:context:)`` - /// does, so a host that uploaded the attachment itself deletes it from the - /// same place. + /// Every attachment lives on the configured site — even one a host uploader + /// delivered — so its deletion is relayed to the default uploader there. See + /// the accepted-risk note in the body. private static func handleMediaDelete( _ attachmentId: String, query: String, context: UploadContext ) async -> HTTPResponse { do { - if let response = try await context.uploadDelegate?.deleteFile(attachmentId: attachmentId) { - return relayResponse(response) - } - + // Relay to the default uploader (the configured site) — every attachment + // lives there, even one a host uploader delivered. Core issues this only + // as orphan cleanup after failed recovery, but the relay can't tell that + // from any other DELETE the WebView sends: a compromised editor script + // holding the loopback token could force-delete arbitrary media on the + // configured site. Accepted risk — such a script already has broad write + // access, and a server-side compromise (a malicious plugin) deletes media + // directly without the editor, so scoping this with a per-session ledger + // buys little for the cost. guard let defaultUploader = context.defaultUploader else { return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) } @@ -274,13 +283,13 @@ final class MediaUploadServer: Sendable { // MARK: - Delegate Pipeline - /// Result of the delegate processing + upload pipeline. + /// Result of the process + deliver pipeline. private enum UploadResult { - /// The delegate (or default uploader) completed the upload; carries the - /// raw WordPress response to relay. + /// The uploader or default uploader completed the upload; carries the + /// 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. + /// No uploader is set and the processor left the file unmodified, so the + /// caller should forward the original request body to the configured site. case passthrough } @@ -288,16 +297,16 @@ final class MediaUploadServer: Sendable { fileURL: URL, mimeType: String, filename: String, extraParts: [MultipartPart], query: String, context: UploadContext ) async throws -> UploadResult { - // Step 1: Process (resize, transcode, etc.) + // Step 1: transform (resize, transcode, …) if a processor claims the file. let processed: ProcessedProxyFile - if let delegate = context.uploadDelegate { - processed = try await delegate.processFile(at: fileURL, mimeType: mimeType, filename: filename) + if let processor = context.processor, processor.handlesFile(ofType: mimeType, named: filename) { + processed = try await processor.processFile(at: fileURL, mimeType: mimeType, filename: filename) } else { 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. + // processor's values verbatim, so a format change is reported to WordPress. let uploadURL: URL let uploadMimeType: String let uploadFilename: String @@ -312,7 +321,7 @@ final class MediaUploadServer: Sendable { uploadFilename = processedFilename } - // The processed file (if the delegate produced a new one) is ours to + // The processed file (if the processor 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 { @@ -321,10 +330,13 @@ final class MediaUploadServer: Sendable { } } - // Step 2: Upload to remote WordPress - if let delegate = context.uploadDelegate, - let result = try await delegate.uploadFile(at: uploadURL, mimeType: uploadMimeType, filename: uploadFilename) { - return .uploaded(result) + // Step 2: deliver. An uploader owns delivery on the host's own stack and + // returns the finished attachment JSON (or throws); GutenbergKit relays that + // as a success and never runs its own recovery behind it. Otherwise the + // default uploader delivers to the configured site. + if let uploader = context.uploader { + let body = try await uploader.upload(fileAt: uploadURL, mimeType: uploadMimeType, filename: uploadFilename) + return .uploaded(MediaUploadResponse(statusCode: 201, body: body)) } else if let defaultUploader = context.defaultUploader { // Unmodified — forward the original request body directly, skipping // multipart re-encoding. @@ -460,24 +472,26 @@ enum UploadError: Error, LocalizedError { // MARK: - Upload Context -/// Container for the upload delegate and default uploader, captured by the -/// HTTPServer handler closure and re-read on each request. +/// Container for the media processor, uploader, 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. +/// The processor and uploader are held **weakly** — the host owns their lifetime +/// (`EditorViewController.mediaProcessor` / `.mediaUploader` are `weak`). Capturing +/// them strongly here would risk a retain cycle (`EditorViewController → +/// uploadServer → HTTPServer → handler → UploadContext → host object → +/// 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. +/// `@unchecked Sendable`: `processor`/`uploader` are 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)? + weak var processor: (any MediaProcessor)? + weak var uploader: (any MediaUploader)? let defaultUploader: DefaultMediaUploader? - init(uploadDelegate: (any MediaUploadDelegate)?, defaultUploader: DefaultMediaUploader?) { - self.uploadDelegate = uploadDelegate + init(processor: (any MediaProcessor)?, uploader: (any MediaUploader)?, defaultUploader: DefaultMediaUploader?) { + self.processor = processor + self.uploader = uploader self.defaultUploader = defaultUploader } } diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 910f49db4..91c7a8da8 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -102,9 +102,9 @@ struct MediaUploadServerTests { @Test("routes /upload with a query string and relays the query") func uploadWithQueryString() async throws { - let delegate = ProcessOnlyDelegate() + let processor = PassthroughProcessor() let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) defer { server.stop() } // `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, @@ -123,7 +123,7 @@ struct MediaUploadServerTests { 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. + // The processor 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) @@ -131,14 +131,14 @@ struct MediaUploadServerTests { #expect(mockUploader.lastQuery == "?_embed=wp:featuredmedia") } - @Test("routes a deletion to the delegate when it handles one") - func delegateHandlesDeletion() async throws { - // A host that uploaded the attachment itself owns an ID only it can - // resolve, so the default uploader must not be asked to delete it. - // No default uploader is configured, so a 200 here can only come from the - // delegate — the fallback path would fail with "no uploader". - let delegate = DeletingDelegate() - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + @Test("relays a deletion to the default uploader even when an uploader owns uploads") + func deletesGoToDefaultUploaderNotUploader() async throws { + // An attachment lives on the configured site even when a host uploader delivered + // it, so its deletion goes to the default uploader — the host uploader owns + // uploads, not deletes. Held strongly: UploadContext keeps the uploader weakly. + let uploader = MockUploader() + let defaultUploader = MockDefaultUploader() + let server = try await MediaUploadServer.start(uploader: uploader, defaultUploader: defaultUploader) defer { server.stop() } let url = URL(string: "http://127.0.0.1:\(server.port)/media/42?force=true")! @@ -150,40 +150,40 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 200) - #expect(delegate.deletedAttachmentId == "42") + #expect(defaultUploader.deleteMediaCalled) + #expect(defaultUploader.deletedAttachmentId == "42") } - @Test("relays the delegate's own Content-Type instead of emitting it twice") - func delegateContentTypeWins() async throws { - // `HTTPResponse` serializes every header it is given, so appending the JSON - // default unconditionally would put `Content-Type` on the wire twice. - // URLSession joins repeated headers with a comma, which is what a - // regression would look like here. - let delegate = ContentTypeDeletingDelegate() - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + @Test("relays a deletion to the default uploader (configured site)") + func relaysDeleteToDefaultUploader() async throws { + // With no uploader set, GutenbergKit owns deletes: core's orphan cleanup DELETE + // is relayed to the default uploader (the configured site). + let mockUploader = MockDefaultUploader() + let server = try await MediaUploadServer.start(defaultUploader: mockUploader) defer { server.stop() } - let url = URL(string: "http://127.0.0.1:\(server.port)/media/42?force=true")! + let url = URL(string: "http://127.0.0.1:\(server.port)/media/512?force=true")! var request = URLRequest(url: url) request.httpMethod = "DELETE" request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") - let (_, response) = try await URLSession.shared.data(for: request) - let httpResponse = try #require(response as? HTTPURLResponse) - let contentType = httpResponse.value(forHTTPHeaderField: "Content-Type") - #expect(contentType == "text/plain") + #expect((response as? HTTPURLResponse)?.statusCode == 200) + #expect(mockUploader.deleteMediaCalled) + #expect(mockUploader.deletedAttachmentId == "512") } - @Test("calls delegate and returns upload result") - func delegateProcessAndUpload() async throws { - let delegate = MockUploadDelegate() - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + @Test("routes an upload to the uploader and relays its attachment") + func uploaderDeliversAttachment() async throws { + // With an uploader set, GutenbergKit hands it the file and relays the finished + // attachment it returns — the default uploader (configured site) is never used. + let uploader = MockUploader() + let defaultUploader = MockDefaultUploader() + let server = try await MediaUploadServer.start(uploader: uploader, defaultUploader: defaultUploader) defer { server.stop() } let boundary = UUID().uuidString - let fileData = "fake image data".data(using: .utf8)! - let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: fileData) + 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")! var request = URLRequest(url: url) @@ -196,12 +196,14 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - #expect(delegate.processFileCalled) - #expect(delegate.uploadFileCalled) - #expect(delegate.lastMimeType == "image/jpeg") - #expect(delegate.lastFilename == "photo.jpg") + #expect(uploader.uploadCalled) + #expect(uploader.lastMimeType == "image/jpeg") + #expect(uploader.lastFilename == "photo.jpg") + // The host owns delivery — GutenbergKit must not upload to the configured site. + #expect(!defaultUploader.uploadCalled) + #expect(!defaultUploader.passthroughUploadCalled) - // The server relays WordPress's raw response body verbatim. + // The server relays the exact attachment JSON the uploader returned. let object = try JSONSerialization.jsonObject(with: data) let json = try #require(object as? [String: Any]) #expect(json["id"] as? Int == 42) @@ -209,11 +211,11 @@ struct MediaUploadServerTests { #expect(json["media_type"] as? String == "image") } - @Test("uses passthrough when delegate does not modify file") - func delegatePassthrough() async throws { - let delegate = ProcessOnlyDelegate() + @Test("uses passthrough when the processor does not modify the file") + func processorPassthrough() async throws { + let processor = PassthroughProcessor() let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -231,7 +233,7 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - #expect(delegate.processFileCalled) + #expect(processor.processFileCalled) // Passthrough: original body forwarded directly, not re-encoded. #expect(mockUploader.passthroughUploadCalled) #expect(!mockUploader.uploadCalled) @@ -242,11 +244,11 @@ struct MediaUploadServerTests { #expect(json["id"] as? Int == 99) } - @Test("skips processing and the temp copy when the delegate declines by metadata") - func delegateDeclinesByMetadata() async throws { - let delegate = DeclineByMetadataDelegate() + @Test("skips processing and the temp copy when the processor declines by metadata") + func processorDeclinesByMetadata() async throws { + let processor = DecliningProcessor() let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -263,18 +265,18 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - // Declined by metadata → the delegate is never asked to process (so the file + // Declined by metadata → the processor is never asked to process (so the file // was never materialized), and the upload is passed through directly. - #expect(!delegate.processFileCalled) + #expect(!processor.processFileCalled) #expect(mockUploader.passthroughUploadCalled) #expect(!mockUploader.uploadCalled) } - @Test("forwards the delegate's processed metadata to the uploader") + @Test("forwards the processor's processed metadata to the uploader") func processedMetadataForwarded() async throws { - let delegate = ResizingDelegate() + let processor = ResizingProcessor() let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -289,18 +291,18 @@ struct MediaUploadServerTests { _ = try await URLSession.shared.data(for: request) - // The delegate changed the format, so the uploader must receive the new + // The processor 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") + @Test("deletes the processor's processed file after upload") func deletesProcessedFile() async throws { - let delegate = ResizingDelegate() + let processor = ResizingProcessor() let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -315,10 +317,10 @@ struct MediaUploadServerTests { _ = try await URLSession.shared.data(for: request) - // The server owns the file the delegate produced and must delete it once the + // The server owns the file the processor 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) + let processedURL = try #require(processor.producedURL) #expect(!FileManager.default.fileExists(atPath: processedURL.path(percentEncoded: false))) } @@ -403,22 +405,22 @@ struct MediaUploadServerTests { #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? + @Test("does not strongly retain the processor (weak — preserves deinit teardown)") + func doesNotStronglyRetainProcessor() async throws { + weak var weakProcessor: PassthroughProcessor? let server: MediaUploadServer do { - let delegate = MockUploadDelegate() - weakDelegate = delegate - server = try await MediaUploadServer.start(uploadDelegate: delegate) + let processor = PassthroughProcessor() + weakProcessor = processor + server = try await MediaUploadServer.start(processor: processor) } defer { server.stop() } - // UploadContext holds the delegate weakly, so releasing the host's strong + // UploadContext holds the processor weakly, so releasing the host's strong // reference deallocates it. A strong reference here would reintroduce the - // EditorViewController → uploadServer → … → delegate → EditorViewController + // EditorViewController → uploadServer → … → processor → EditorViewController // cycle, so deinit would never fire and the server would never stop. - #expect(weakDelegate == nil) + #expect(weakProcessor == nil) } private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data) -> Data { @@ -809,63 +811,34 @@ private func readAllFromStream(_ stream: InputStream) -> Data { // MARK: - Mocks -private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable { +/// A host uploader: it performs the upload on its own stack. `upload` returns the +/// finished attachment JSON (or throws). +private final class MockUploader: MediaUploader, @unchecked Sendable { private let lock = NSLock() - private var _processFileCalled = false - private var _uploadFileCalled = false + private var _uploadCalled = false private var _lastMimeType: String? private var _lastFilename: String? + private let uploadBody: Data - var processFileCalled: Bool { lock.withLock { _processFileCalled } } - var uploadFileCalled: Bool { lock.withLock { _uploadFileCalled } } + var uploadCalled: Bool { lock.withLock { _uploadCalled } } var lastMimeType: String? { lock.withLock { _lastMimeType } } var lastFilename: String? { lock.withLock { _lastFilename } } - func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { - lock.withLock { - _processFileCalled = true - _lastMimeType = mimeType - } - return .original + init(uploadBody: Data = Data(#"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"#.utf8)) { + self.uploadBody = uploadBody } - func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { + func upload(fileAt url: URL, mimeType: String, filename: String) async throws -> Data { lock.withLock { - _uploadFileCalled = true + _uploadCalled = true + _lastMimeType = mimeType _lastFilename = filename } - let json = #"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"# - return MediaUploadResponse(statusCode: 201, body: Data(json.utf8)) + return uploadBody } } -/// A delegate that handles deletions itself, as a host uploading to its own -/// media service would. -private final class DeletingDelegate: MediaUploadDelegate, @unchecked Sendable { - private let lock = NSLock() - private var _deletedAttachmentId: String? - - var deletedAttachmentId: String? { lock.withLock { _deletedAttachmentId } } - - func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { - lock.withLock { _deletedAttachmentId = attachmentId } - return MediaUploadResponse(statusCode: 200, body: Data(#"{"deleted":true}"#.utf8)) - } -} - -/// A delegate that sets its own `Content-Type`, so the relay must not also -/// append the JSON default. -private final class ContentTypeDeletingDelegate: MediaUploadDelegate, @unchecked Sendable { - func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { - MediaUploadResponse( - statusCode: 200, - body: Data("deleted".utf8), - headers: ["Content-Type": "text/plain"] - ) - } -} - -private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class PassthroughProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -877,10 +850,10 @@ private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendabl } } -/// A delegate that declines every file by metadata via `handlesFile`, so the +/// A processor that declines every file by metadata via `handlesFile`, so the /// server must pass through without ever materializing the file or calling /// `processFile`. -private final class DeclineByMetadataDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class DecliningProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -894,12 +867,12 @@ private final class DeclineByMetadataDelegate: MediaUploadDelegate, @unchecked S } } -/// A delegate that produces a new file with changed metadata (e.g. a transcode). -private final class ResizingDelegate: MediaUploadDelegate, @unchecked Sendable { +/// A processor that produces a new file with changed metadata (e.g. a transcode). +private final class ResizingProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _producedURL: URL? - /// The URL of the processed file this delegate wrote, for cleanup assertions. + /// The URL of the processed file this processor wrote, for cleanup assertions. var producedURL: URL? { lock.withLock { _producedURL } } func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { @@ -917,14 +890,22 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab private var _lastUploadMimeType: String? private var _lastUploadFilename: String? private var _lastQuery: String? + private var _deleteMediaCalled = false + private var _deletedAttachmentId: String? + + /// The response `upload`/`passthroughUpload` return. `nil` uses a 201 default. + private let uploadResponse: MediaUploadResponse? 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 } } + var deleteMediaCalled: Bool { lock.withLock { _deleteMediaCalled } } + var deletedAttachmentId: String? { lock.withLock { _deletedAttachmentId } } - init() { + init(uploadResponse: MediaUploadResponse? = nil) { + self.uploadResponse = uploadResponse super.init(httpClient: MockHTTPClient(), siteApiRoot: URL(string: "https://example.com/wp-json/")!) } @@ -935,7 +916,7 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab _lastUploadFilename = filename _lastQuery = query } - return mockResponse() + return uploadResponse ?? Self.defaultResponse } override func passthroughUpload(body: RequestBody, contentType: String, query: String) async throws -> MediaUploadResponse { @@ -943,13 +924,20 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab _passthroughUploadCalled = true _lastQuery = query } - return mockResponse() + return uploadResponse ?? Self.defaultResponse } - 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)) + override func deleteMedia(attachmentId: String, query: String) async throws -> MediaUploadResponse { + lock.withLock { + _deleteMediaCalled = true + _deletedAttachmentId = attachmentId + } + return MediaUploadResponse(statusCode: 200, body: Data(#"{"deleted":true}"#.utf8)) } + + private static let defaultResponse = MediaUploadResponse( + statusCode: 201, + body: Data(#"{"id":99,"source_url":"https://example.com/doc.pdf","media_type":"file"}"#.utf8)) } private struct MockHTTPClient: EditorHTTPClientProtocol { From 4a03bcc5d304aa0a1ea1c0e2422e1f0299dbe339 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 11:15:32 -0600 Subject: [PATCH 11/21] refactor: tighten the MediaProcessor/MediaUploader contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups from review of the delegate split: - Ownership: `EditorViewController` holds `mediaProcessor`/`mediaUploader` strongly now, not `weak`, matching Android — a host can assign one and drop its own reference without native media silently stopping mid-session. The load-time `wasAssigned` traps that only guarded the old release-before-load footgun are gone; a new `EditorMediaHandlerOwnershipTests` pins the retention. - Credentials: setting a `mediaUploader` without an auth header now traps at startup. Media deletes always relay to the configured site, so an uploader without credentials could upload but every delete would 500 — a configuration error, surfaced like the late-assignment trap rather than a half-working server. - Upload context: `MediaUploader.upload` takes a `MediaUpload` value type carrying the file plus the editor's non-file form fields (`post`, additionalData) and the request query (`?_embed`). The old signature dropped them, so a host upload created an unattached orphan and lost the query — the default path already forwarded them. - `handlesFile` is consulted once per upload, not twice: the admission gate's decision is threaded into the pipeline instead of recomputed, so the two steps can't disagree. - Rename the internal configured-site client `DefaultMediaUploader` → `InternalMediaClient` (field `defaultUploader` → `internalClient`). It's GutenbergKit's own client for the configured site — it delivers GutenbergKit-owned uploads, relays every delete, and does passthrough — not a "default" that a host `mediaUploader` overrides; the host takes a different path entirely. Internal-only; no public-API change. - Cleanup: drop the now-dead `Content-Type` dedup in `relayResponse` (relayed bodies are always WordPress REST JSON and relayed headers are a content-type-free allowlist, so the JSON default always applies); drop the stale `delegate` vocabulary (error string, doc comment); rename `MediaUploadDelegate.swift` to `MediaHandlers.swift`. `MediaUploadServer` now requires a non-null `cacheDir` and `internalClient`: the server never starts without a cache dir or site credentials, so its staging directory is correct-by-construction (no `java.io.tmpdir` fallback), and its delete / passthrough / upload paths drop the dead "no uploader configured" guards. - Tests: the host-`mediaUploader` suite now asserts the uploader receives the actual file bytes (not just metadata), that a processor's processed file and new metadata reach the uploader, and that a throwing uploader surfaces as a relayed 500. iOS and Android suites green; SwiftLint and Detekt clean. --- .../org/wordpress/gutenberg/GutenbergView.kt | 64 ++++-- .../wordpress/gutenberg/MediaUploadServer.kt | 95 +++++--- .../GutenbergViewUploadServerTest.kt | 37 +++ .../gutenberg/MediaUploadServerTest.kt | 204 +++++++++++----- .../Sources/EditorViewController.swift | 96 ++++---- ...loadDelegate.swift => MediaHandlers.swift} | 28 ++- .../Sources/Media/MediaUploadServer.swift | 100 ++++---- .../EditorMediaHandlerOwnershipTests.swift | 61 +++++ .../Media/MediaUploadServerTests.swift | 217 ++++++++++++------ 9 files changed, 633 insertions(+), 269 deletions(-) rename ios/Sources/GutenbergKit/Sources/Media/{MediaUploadDelegate.swift => MediaHandlers.swift} (83%) create mode 100644 ios/Tests/GutenbergKitTests/Media/EditorMediaHandlerOwnershipTests.swift 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 ae421c251..96f999dfe 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -121,6 +121,10 @@ class GutenbergView : FrameLayout { * construction (e.g. in the `AndroidView` factory). It is captured once, when * the page begins loading; setting it afterward has no effect, so the setter * throws to surface the mistake. + * + * This view owns the processor for its lifetime, so you don't need to keep a + * reference after assigning it — and avoid strongly retaining this [GutenbergView] + * from your processor in return, so the two don't form a reference cycle. */ var mediaProcessor: MediaProcessor? = null set(value) { @@ -133,7 +137,13 @@ class GutenbergView : FrameLayout { * queue, resumable transport). Setting it makes the host own every upload and * its whole lifecycle; GutenbergKit stays out of the network entirely for media. * - * Same lifecycle rules as [mediaProcessor]: set it before the editor loads. + * Same lifecycle rules as [mediaProcessor]: set it before the editor loads, and + * this view owns it for its lifetime — so you needn't retain it yourself, just + * don't strongly retain this [GutenbergView] from your uploader. + * + * Requires site credentials in the editor configuration: media deletes always + * relay to the configured site, so an uploader set without a site root and auth + * header is a configuration error and throws at load. */ var mediaUploader: MediaUploader? = null set(value) { @@ -689,16 +699,26 @@ class GutenbergView : FrameLayout { // processor or an uploader. (Matches iOS.) if (mediaProcessor == null && mediaUploader == null) return - // A DefaultMediaUploader delivers GutenbergKit-owned uploads (when no uploader + // An InternalMediaClient delivers GutenbergKit-owned uploads (when no uploader // is set) and relays the editor's media DELETEs to the configured site — every // attachment lives there, even one a host uploader delivered. It needs a site - // root and an auth header (every host provides one — the editor injects it - // because the WebView has no auth cookies). If GutenbergKit would have to - // deliver uploads itself but lacks those, there's nothing to upload through, so - // leave the server down and let uploads fall to the default WebView path. - if (mediaUploader == null && - (configuration.siteApiRoot.isEmpty() || configuration.authHeader.isEmpty()) - ) { + // root and an auth header (the editor injects it because the WebView has no + // auth cookies). Without them the behavior forks by intent: + // + // - A mediaProcessor only enhances GutenbergKit-owned uploads; with no + // credentials there's nothing to deliver through, so nothing to process — + // leave the server down and let uploads fall to the default WebView path. + // + // - A mediaUploader means the host is taking over uploads. Falling back would + // silently drop it, and its media deletes still need the internal media client to + // reach the configured site. A host that sets an uploader must provide + // credentials too; omitting them is a configuration error, so fail fast. + if (configuration.siteApiRoot.isEmpty() || configuration.authHeader.isEmpty()) { + check(mediaUploader == null) { + "A mediaUploader needs site credentials so GutenbergKit can relay the " + + "editor's media deletes to the configured site. Set siteApiRoot and " + + "the auth header in the editor configuration." + } return } @@ -720,25 +740,19 @@ class GutenbergView : FrameLayout { } try { - // Build a DefaultMediaUploader whenever there are credentials to reach the - // site: it delivers GutenbergKit-owned uploads and relays the editor's - // DELETEs there. null only when a host owns uploads and no creds exist. - val defaultUploader = if ( - configuration.siteApiRoot.isNotEmpty() && configuration.authHeader.isNotEmpty() - ) { - DefaultMediaUploader( - httpClient = uploadHttpClient, - siteApiRoot = configuration.siteApiRoot, - authHeader = configuration.authHeader, - siteApiNamespace = configuration.siteApiNamespace.toList() - ) - } else { - null - } + // Credentials are present (checked above), so always build a default + // uploader: it delivers GutenbergKit-owned uploads and relays the editor's + // media DELETEs to the configured site. + val internalClient = InternalMediaClient( + httpClient = uploadHttpClient, + siteApiRoot = configuration.siteApiRoot, + authHeader = configuration.authHeader, + siteApiNamespace = configuration.siteApiNamespace.toList() + ) uploadServer = MediaUploadServer( processor = mediaProcessor, uploader = mediaUploader, - defaultUploader = defaultUploader, + internalClient = internalClient, cacheDir = context.cacheDir, scope = coroutineScope ) 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 a2b944912..9eda0953f 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -98,6 +98,27 @@ interface MediaProcessor { suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile = ProcessedProxyFile.Original } +/** + * Everything a [MediaUploader] needs to reproduce a native upload: the file to send, + * its metadata, the editor's non-file form fields, and the request's query. + * + * @property file The file to upload — already processed, if a [MediaProcessor] ran. + * @property mimeType The file's MIME type. + * @property filename The file's name. + * @property fields The editor's non-file form fields, decoded as UTF-8 — most + * importantly `post`, the parent post's ID, without which the attachment is created + * unattached. Send each as a form part on your `POST /wp/v2/media`. + * @property query The request's query string (leading `?`, e.g. `?_embed=...`), or + * empty. Carry it on your request so the editor gets the response it expects. + */ +data class MediaUpload( + val file: File, + val mimeType: String, + val filename: String, + val fields: Map, + val query: String +) + /** * Takes over performing a media upload — on the host's own stack: its own * networking (say, to log every request), a background service, an offline queue, @@ -119,6 +140,10 @@ interface MediaUploader { * throw on terminal failure: a returned value is taken as a completed attachment, * and there is no GutenbergKit recovery behind you. * + * The [MediaUpload] carries the file plus the editor's form fields (e.g. `post`) + * and query — send them all so the created attachment matches a native upload + * rather than landing as an unattached orphan. + * * That recovery is yours to run. When `POST /wp/v2/media` fatals in server-side * post-processing it returns a 5xx carrying the attachment's ID in * `x-wp-upload-attachment-id` — the attachment exists but is unfinished. Don't @@ -131,7 +156,7 @@ interface MediaUploader { * before you throw, or it stays on the site — neither GutenbergKit nor core * cleans up behind you. */ - suspend fun upload(file: File, mimeType: String, filename: String): ByteArray + suspend fun upload(upload: MediaUpload): ByteArray } /** @@ -149,8 +174,8 @@ interface MediaUploader { internal class MediaUploadServer( private val processor: MediaProcessor?, private val uploader: MediaUploader?, - private val defaultUploader: DefaultMediaUploader?, - cacheDir: File? = null, + private val internalClient: InternalMediaClient, + cacheDir: File, scope: CoroutineScope? = null, ioDispatcher: CoroutineDispatcher = Dispatchers.IO ) : HttpServerDelegate { @@ -163,11 +188,10 @@ 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. + * Directory for staging uploaded files, under the injected cache dir so orphans + * share the app's managed cache lifecycle. */ - private val uploadsTempDir: File = - File(cacheDir ?: File(System.getProperty("java.io.tmpdir")), "gutenbergkit-uploads") + private val uploadsTempDir: File = File(cacheDir, "gutenbergkit-uploads") /** * The scope MediaUploadServer created itself because the caller supplied none. @@ -286,13 +310,13 @@ internal class MediaUploadServer( * browser blocks it at preflight; relaying it here lets the deletion run. * * Every attachment lives on the configured site — even one a host uploader - * delivered — so its deletion is relayed to the default uploader there. See + * delivered — so its deletion is relayed to the internal media client there. See * the accepted-risk note in the body. */ @Suppress("TooGenericExceptionCaught") private suspend fun handleMediaDelete(attachmentId: String, query: String): HttpResponse { return try { - // Relay to the default uploader (the configured site) — every attachment + // Relay to the internal media client (the configured site) — every attachment // lives there, even one a host uploader delivered. Core issues this only // as orphan cleanup after failed recovery, but the relay can't tell that // from any other DELETE the WebView sends: a compromised editor script @@ -301,8 +325,7 @@ internal class MediaUploadServer( // access, and a server-side compromise (a malicious plugin) deletes media // directly without the editor, so scoping this with a per-session ledger // buys little for the cost. - val defaultUploader = defaultUploader ?: return errorResponse(500, "No uploader configured") - relayResponse(defaultUploader.deleteMedia(attachmentId, query)) + relayResponse(internalClient.deleteMedia(attachmentId, query)) } catch (e: kotlin.coroutines.cancellation.CancellationException) { throw e // Never swallow coroutine cancellation. } catch (e: Exception) { @@ -337,7 +360,7 @@ internal class MediaUploadServer( val tempFile = writePartToTempFile(filePart) ?: return errorResponse(500, "Failed to save file") - return processAndRespond(request, tempFile, filePart, extraParts, query) + return processAndRespond(request, tempFile, filePart, extraParts, query, processorWantsFile) } @Suppress("TooGenericExceptionCaught") @@ -362,17 +385,15 @@ internal class MediaUploadServer( * fataled server-side, rather than surfacing a permanent failure and leaving * an orphaned attachment behind. * - * The response's own `Content-Type` wins over the JSON default, matched - * case-insensitively — HTTP header names are case-insensitive, and - * [HttpResponse] serializes every entry it is given, so a plain map merge - * would emit the name twice for a delegate that spells it `content-type`. + * The relayed body is always WordPress REST JSON — an attachment, or a + * `{code, message, data}` error — so the response is always `application/json`. + * The relayed headers are a content-type-free allowlist (`RELAYABLE_HEADER_NAMES`), + * so prepending the JSON default never collides with them. */ private fun relayResponse(response: MediaUploadResponse): HttpResponse { - val hasContentType = response.headers.keys.any { it.lowercase() == "content-type" } - val defaults = if (hasContentType) emptyMap() else mapOf("Content-Type" to "application/json") return HttpResponse( status = response.statusCode, - headers = defaults + response.headers, + headers = mapOf("Content-Type" to "application/json") + response.headers, body = response.body ) } @@ -421,11 +442,12 @@ internal class MediaUploadServer( @Suppress("TooGenericExceptionCaught") private suspend fun processAndRespond( request: HttpRequest, tempFile: File, filePart: MultipartPart, - extraParts: List, query: String + extraParts: List, query: String, processorWantsFile: Boolean ): HttpResponse { try { val uploadResult = processAndUpload( - tempFile, filePart.contentType, filePart.filename ?: "upload", extraParts, query + tempFile, filePart.contentType, filePart.filename ?: "upload", + extraParts, query, processorWantsFile ) val response = when (uploadResult) { is UploadResult.Uploaded -> { @@ -469,19 +491,20 @@ internal class MediaUploadServer( 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") + if (body == null || contentType == null) { + throw MediaUploadException("Passthrough upload requires a request body and Content-Type") } - return uploader.passthroughUpload(body, contentType, query) + return internalClient.passthroughUpload(body, contentType, query) } private suspend fun processAndUpload( file: File, mimeType: String, filename: String, - extraParts: List, query: String + extraParts: List, query: String, processorWantsFile: Boolean ): UploadResult { - // Transform (resize, transcode, …) if a processor claims the file. - val processed = if (processor?.handlesFile(mimeType, filename) == true) { + // Transform (resize, transcode, …) if a processor claims the file. Reuse the + // gate's handlesFile decision from handleUpload rather than asking again — one + // metadata call per upload, and the admit and transform steps can't disagree. + val processed = if (processorWantsFile && processor != null) { processor.processFile(file, mimeType, filename) } else { ProcessedProxyFile.Original @@ -509,8 +532,15 @@ internal class MediaUploadServer( // An uploader owns delivery on the host's own stack and returns the // finished attachment JSON (or throws); GutenbergKit relays that as a // success and never runs its own recovery behind it. - uploader?.let { - return UploadResult.Uploaded(MediaUploadResponse(201, it.upload(targetFile, targetMimeType, targetFilename))) + uploader?.let { up -> + // Hand the host the editor's non-file fields (e.g. `post`) and query + // too, so its own POST can reproduce a native upload — otherwise the + // attachment is created unattached and `?_embed` is lost. + val fields = extraParts.associate { part -> + part.name to String(part.body.readBytes(), Charsets.UTF_8) + } + val upload = MediaUpload(targetFile, targetMimeType, targetFilename, fields, query) + return UploadResult.Uploaded(MediaUploadResponse(201, up.upload(upload))) } // Unmodified — forward the original request body directly, skipping @@ -519,8 +549,7 @@ internal class MediaUploadServer( return UploadResult.Passthrough } - val result = defaultUploader?.upload(targetFile, targetMimeType, targetFilename, extraParts, query) - ?: error("No uploader or default uploader configured") + val result = internalClient.upload(targetFile, targetMimeType, targetFilename, extraParts, query) return UploadResult.Uploaded(result) } finally { // The processed file (if the delegate produced a new one) is ours to @@ -577,7 +606,7 @@ internal class MediaUploadException(message: String, cause: Throwable? = null) : /** * Uploads files to the WordPress REST API using OkHttp. */ -internal open class DefaultMediaUploader( +internal open class InternalMediaClient( private val httpClient: okhttp3.OkHttpClient, private val siteApiRoot: String, private val authHeader: String, diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt index 7ea179401..8f673b592 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt @@ -2,10 +2,12 @@ package org.wordpress.gutenberg import android.os.Looper import android.view.View +import java.lang.reflect.InvocationTargetException import kotlinx.coroutines.test.TestScope import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mockito.mock @@ -110,6 +112,41 @@ class GutenbergViewUploadServerTest { } } + @Test + fun `an uploader without site credentials is a configuration error`() { + // A mediaUploader owns uploads, but media deletes still relay to the configured + // site — which needs credentials to reach. Setting an uploader without them is + // a programmer error, surfaced loudly rather than starting a server whose every + // delete would fail. (A processor without credentials is fine — it just falls + // back to the default WebView path, covered above.) + val config = EditorConfiguration + .builder("https://example.com", "https://example.com/wp-json/") + .build() // deliberately no auth header + val view = GutenbergView( + config, + EditorDependencies.empty, + testScope, + RuntimeEnvironment.getApplication() + ) + try { + view.mediaUploader = mock(MediaUploader::class.java) + // startUploadServer runs inside onEditorPageStarted, so reflection wraps its throw. + val error = assertThrows(InvocationTargetException::class.java) { + startLoading(view) + } + assertTrue( + "an uploader without credentials should fail with IllegalStateException", + error.cause is IllegalStateException + ) + assertNull( + "no server should be left running after the configuration error", + uploadServerOf(view) + ) + } finally { + detach(view) + } + } + @Test fun `detaching the view stops and clears the upload server`() { val view = makeView() 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 78ab70bd0..ab7282387 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -33,7 +33,12 @@ class MediaUploadServerTest { @Before fun setUp() { - server = MediaUploadServer(processor = null, uploader = null, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer( + processor = null, + uploader = null, + internalClient = MockInternalMediaClient(), + cacheDir = tempFolder.root + ) } @After @@ -53,7 +58,12 @@ class MediaUploadServerTest { fun `stop cancels an internally-created scope but leaves a caller-supplied one alone`() { // No scope supplied → the server owns one, which stop() must cancel. val owningServer = - MediaUploadServer(processor = null, uploader = null, defaultUploader = null, cacheDir = tempFolder.root) + MediaUploadServer( + processor = null, + uploader = null, + internalClient = MockInternalMediaClient(), + cacheDir = tempFolder.root + ) val ownedScope = ownedScopeOf(owningServer) assertNotNull("server should own a scope when none is supplied", ownedScope) assertTrue(ownedScope!!.isActive) @@ -65,7 +75,7 @@ class MediaUploadServerTest { val borrowingServer = MediaUploadServer( processor = null, uploader = null, - defaultUploader = null, + internalClient = MockInternalMediaClient(), cacheDir = tempFolder.root, scope = callerScope ) @@ -142,17 +152,17 @@ class MediaUploadServerTest { } @Test - fun `relays a deletion to the default uploader even when an uploader owns uploads`() { + fun `relays a deletion to the internal media client even when an uploader owns uploads`() { // An attachment lives on the configured site even when a host uploader - // delivered it, so its deletion goes to the default uploader — the host + // delivered it, so its deletion goes to the internal media client — the host // uploader owns uploads, not deletes. val uploader = MockUploader() - val defaultUploader = MockDefaultUploader() + val internalClient = MockInternalMediaClient() server.stop() server = MediaUploadServer( processor = null, uploader = uploader, - defaultUploader = defaultUploader, + internalClient = internalClient, cacheDir = tempFolder.root ) @@ -164,22 +174,22 @@ class MediaUploadServerTest { ) assertTrue("Expected 200 but got: ${response.statusLine}", response.statusLine.contains("200")) - assertTrue(defaultUploader.deleteMediaCalled) - assertEquals("42", defaultUploader.deletedAttachmentId) + assertTrue(internalClient.deleteMediaCalled) + assertEquals("42", internalClient.deletedAttachmentId) } // MARK: - Media deletion @Test - fun `relays a deletion to the default uploader (configured site)`() { + fun `relays a deletion to the internal media client (configured site)`() { // With no uploader set, GutenbergKit owns deletes: core's orphan cleanup - // DELETE is relayed to the default uploader (the configured site). - val mockUploader = MockDefaultUploader() + // DELETE is relayed to the internal media client (the configured site). + val mockUploader = MockInternalMediaClient() server.stop() server = MediaUploadServer( processor = null, uploader = null, - defaultUploader = mockUploader, + internalClient = mockUploader, cacheDir = tempFolder.root ) @@ -198,9 +208,9 @@ class MediaUploadServerTest { @Test fun `routes upload with a query string and relays the query`() { val processor = PassthroughProcessor() - val mockUploader = MockDefaultUploader() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, internalClient = 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 @@ -232,23 +242,26 @@ class MediaUploadServerTest { @Test fun `routes an upload to the uploader and relays its attachment`() { // With an uploader set, GutenbergKit hands it the file and relays the finished - // attachment it returns — the default uploader (configured site) is never used. + // attachment it returns — the internal media client (configured site) is never used. val uploader = MockUploader() - val defaultUploader = MockDefaultUploader() + val internalClient = MockInternalMediaClient() server.stop() server = MediaUploadServer( processor = null, uploader = uploader, - defaultUploader = defaultUploader, + internalClient = internalClient, cacheDir = tempFolder.root ) val boundary = "test-boundary-123" - val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) + val body = buildMultipartBody( + boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray(), + fields = mapOf("post" to "123") + ) val response = sendRawRequest( method = "POST", - path = "/upload", + path = "/upload?_embed=wp:featuredmedia", headers = mapOf( "Relay-Authorization" to "Bearer ${server.token}", "Content-Type" to "multipart/form-data; boundary=$boundary" @@ -260,9 +273,15 @@ class MediaUploadServerTest { assertTrue(uploader.uploadCalled) assertEquals("image/jpeg", uploader.lastMimeType) assertEquals("photo.jpg", uploader.lastFilename) + // The editor's post association and query must reach the host uploader, so it + // can reproduce a native upload (attach to the post, honor ?_embed). + assertEquals("123", uploader.lastFields["post"]) + assertEquals("?_embed=wp:featuredmedia", uploader.lastQuery) + // …and the actual file bytes the editor sent — the host uploads them itself. + assertEquals("fake image data", uploader.lastFileBytes?.decodeToString()) // The host owns delivery — GutenbergKit must not upload to the configured site. - assertFalse(defaultUploader.uploadCalled) - assertFalse(defaultUploader.passthroughUploadCalled) + assertFalse(internalClient.uploadCalled) + assertFalse(internalClient.passthroughUploadCalled) // The server relays the exact attachment JSON the uploader returned. val json = JsonParser.parseString(response.body).asJsonObject @@ -271,12 +290,74 @@ class MediaUploadServerTest { assertEquals("image", json.get("media_type").asString) } + @Test + fun `hands the processed file and its new metadata to the uploader`() { + // A processor transcodes the file; the host uploader must receive the processed + // bytes and the new metadata, not the original clip.mov. + val processor = TranscodingProcessor() + val uploader = MockUploader() + server.stop() + server = MediaUploadServer( + processor = processor, + uploader = uploader, + internalClient = MockInternalMediaClient(), + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-proc" + val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) + + val response = sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) + assertTrue(uploader.uploadCalled) + assertEquals("processed", uploader.lastFileBytes?.decodeToString()) + assertEquals("video/mp4", uploader.lastMimeType) + assertEquals("clip.mp4", uploader.lastFilename) + } + + @Test + fun `relays a 500 when the host uploader throws`() { + val uploader = MockUploader(error = RuntimeException("upload failed")) + server.stop() + server = MediaUploadServer( + processor = null, + uploader = uploader, + internalClient = MockInternalMediaClient(), + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-err" + val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) + + val response = sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertTrue("Expected 500 but got: ${response.statusLine}", response.statusLine.contains("500")) + assertTrue(uploader.uploadCalled) + } + @Test fun `forwards the processor's processed metadata to the uploader`() { val processor = TranscodingProcessor() - val mockUploader = MockDefaultUploader() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-meta" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -301,9 +382,9 @@ class MediaUploadServerTest { @Test fun `deletes the processor's processed file after upload`() { val processor = TranscodingProcessor() - val mockUploader = MockDefaultUploader() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-cleanup" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -343,7 +424,7 @@ class MediaUploadServerTest { server = MediaUploadServer( processor = null, uploader = null, - defaultUploader = null, + internalClient = MockInternalMediaClient(), cacheDir = tempFolder.root, ioDispatcher = Dispatchers.Unconfined ) @@ -352,15 +433,15 @@ class MediaUploadServerTest { assertTrue("Fresh temp should be preserved", fresh.exists()) } - // MARK: - Fallback to default uploader + // MARK: - Fallback to internal media client @Test fun `uses passthrough when the processor does not modify the file`() { val processor = PassthroughProcessor() - val mockUploader = MockDefaultUploader() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-456" val body = buildMultipartBody(boundary, "doc.pdf", "application/pdf", "fake pdf data".toByteArray()) @@ -388,10 +469,10 @@ class MediaUploadServerTest { @Test fun `skips processing and the temp copy when the processor declines by metadata`() { val processor = DecliningProcessor() - val mockUploader = MockDefaultUploader() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-decline" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "fake movie".toByteArray()) @@ -414,10 +495,10 @@ class MediaUploadServerTest { assertFalse(mockUploader.uploadCalled) } - // MARK: - DefaultMediaUploader + // MARK: - InternalMediaClient @Test - fun `DefaultMediaUploader relays the WordPress response`() { + fun `InternalMediaClient relays the WordPress response`() { val mockWpServer = MockWebServer() val wpBody = """{"id":1,"source_url":"https://example.com/u.jpg","media_type":"image"}""" @@ -430,7 +511,7 @@ class MediaUploadServerTest { mockWpServer.start() val wpBaseUrl = mockWpServer.url("/wp-json/").toString() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = wpBaseUrl, authHeader = "Bearer test-token" @@ -455,13 +536,13 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader relays a WordPress error response instead of throwing`() { + fun `InternalMediaClient relays a WordPress error response instead of throwing`() { val mockWpServer = MockWebServer() mockWpServer.enqueue(MockResponse().setResponseCode(500).setBody("Internal error")) mockWpServer.start() val wpBaseUrl = mockWpServer.url("/wp-json/").toString() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = wpBaseUrl, authHeader = "Bearer test-token" @@ -480,7 +561,7 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader relays the upload attachment ID header`() { + fun `InternalMediaClient relays the upload attachment ID header`() { // WordPress sets this header on an upload whose attachment row was // created before metadata generation fataled. The editor reads it to // retry post-process and clean up the orphan, so it must survive the @@ -495,7 +576,7 @@ class MediaUploadServerTest { ) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -513,12 +594,12 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader deletes an attachment carrying namespace and force query`() { + fun `InternalMediaClient deletes an attachment carrying namespace and force query`() { val mockWpServer = MockWebServer() mockWpServer.enqueue(MockResponse().setResponseCode(200).setBody("""{"deleted":true}""")) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token", @@ -536,12 +617,12 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader normalizes an unslashed root and namespace`() { + fun `InternalMediaClient normalizes an unslashed root and namespace`() { val mockWpServer = MockWebServer() mockWpServer.enqueue(MockResponse().setResponseCode(201).setBody("{}")) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json").toString(), // no trailing slash authHeader = "Bearer test-token", @@ -573,7 +654,7 @@ class MediaUploadServerTest { ) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -601,12 +682,12 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader re-encode preserves extra parts and query`() { + fun `InternalMediaClient re-encode preserves extra parts and query`() { val mockWpServer = MockWebServer() mockWpServer.enqueue(MockResponse().setResponseCode(201).setBody("{}")) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -641,7 +722,7 @@ class MediaUploadServerTest { mockWpServer.enqueue(MockResponse().setResponseCode(201).setBody("{}")) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -781,9 +862,16 @@ class MediaUploadServerTest { boundary: String, filename: String, mimeType: String, - data: ByteArray + data: ByteArray, + fields: Map = emptyMap() ): ByteArray { val out = java.io.ByteArrayOutputStream() + for ((name, value) in fields) { + out.write("--$boundary\r\n".toByteArray()) + out.write("Content-Disposition: form-data; name=\"$name\"\r\n\r\n".toByteArray()) + out.write(value.toByteArray()) + out.write("\r\n".toByteArray()) + } out.write("--$boundary\r\n".toByteArray()) out.write("Content-Disposition: form-data; name=\"file\"; filename=\"$filename\"\r\n".toByteArray()) out.write("Content-Type: $mimeType\r\n\r\n".toByteArray()) @@ -800,16 +888,24 @@ class MediaUploadServerTest { */ private class MockUploader( private val uploadBody: ByteArray = - """{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}""".toByteArray() + """{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}""".toByteArray(), + private val error: Exception? = null ) : MediaUploader { @Volatile var uploadCalled = false @Volatile var lastMimeType: String? = null @Volatile var lastFilename: String? = null + @Volatile var lastFields: Map = emptyMap() + @Volatile var lastQuery: String? = null + @Volatile var lastFileBytes: ByteArray? = null - override suspend fun upload(file: File, mimeType: String, filename: String): ByteArray { + override suspend fun upload(upload: MediaUpload): ByteArray { uploadCalled = true - lastMimeType = mimeType - lastFilename = filename + lastMimeType = upload.mimeType + lastFilename = upload.filename + lastFields = upload.fields + lastQuery = upload.query + lastFileBytes = upload.file.readBytes() + error?.let { throw it } return uploadBody } } @@ -851,13 +947,13 @@ class MediaUploadServerTest { } } - private class MockDefaultUploader( + private class MockInternalMediaClient( /** The response `upload`/`passthroughUpload` return. Defaults to a 201 success. */ private val uploadResponse: MediaUploadResponse = MediaUploadResponse( 201, """{"id":99,"source_url":"https://example.com/doc.pdf","media_type":"file"}""".toByteArray() ) - ) : DefaultMediaUploader( + ) : InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = "https://example.com/wp-json/", authHeader = "Bearer mock" diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 86de817ae..7fe9af377 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -109,12 +109,6 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// longer take effect, so their setters trap if written. private var hasStartedLoading = false - /// Whether a non-nil ``mediaProcessor``/``mediaUploader`` was ever assigned. Lets - /// the load path tell "the host object was released before load" (a retention - /// mistake to trap) apart from "none was configured" (a valid opt-out). - private var mediaProcessorWasAssigned = false - private var mediaUploaderWasAssigned = false - /// Transforms media (resize, transcode, …) before GutenbergKit delivers it to /// the configured site. The safe, common extension point — a processor never /// performs the upload itself, so it cannot deliver media to the wrong place. @@ -123,13 +117,12 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// is captured once, when the editor begins loading; setting it afterward has no /// effect, so the setter traps. /// - /// - Important: This is a `weak` reference — hold a strong reference to your - /// processor until the editor has loaded, or native media handling is silently - /// disabled. The editor traps at load time if a processor assigned here has - /// already been deallocated. - public weak var mediaProcessor: (any MediaProcessor)? { + /// The editor **owns** this for its lifetime and releases it on `deinit`, so you + /// don't need to keep a reference after assigning it. The one rule: your processor + /// must not strongly retain this `EditorViewController` in return, or the two form + /// a retain cycle and neither is freed. + public var mediaProcessor: (any MediaProcessor)? { didSet { - mediaProcessorWasAssigned = mediaProcessor != nil precondition(!hasStartedLoading, Self.lateMediaAssignmentMessage("mediaProcessor")) } } @@ -138,11 +131,16 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// queue, resumable transport). Setting it makes the host own every upload and /// its whole lifecycle; GutenbergKit stays out of the network entirely for media. /// - /// Same lifecycle rules as ``mediaProcessor``: set it before the editor loads, - /// and hold a strong reference until then. - public weak var mediaUploader: (any MediaUploader)? { + /// Same lifecycle rules as ``mediaProcessor``: set it before the editor loads. + /// The editor owns it for its lifetime (releasing it on `deinit`), so you needn't + /// retain it yourself — just don't strongly retain this `EditorViewController` + /// from your uploader. + /// + /// Requires site credentials in the editor configuration: media deletes always + /// relay to the configured site, so an uploader set without an auth header is a + /// configuration error and traps at load. + public var mediaUploader: (any MediaUploader)? { didSet { - mediaUploaderWasAssigned = mediaUploader != nil precondition(!hasStartedLoading, Self.lateMediaAssignmentMessage("mediaUploader")) } } @@ -394,8 +392,8 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// @MainActor private func loadEditor(dependencies: EditorDependencies) async throws { - // From here on the editor configuration — including `mediaUploadDelegate` — - // is captured, so the delegate setter traps if written after this point. + // From here on the editor configuration — including `mediaProcessor` and + // `mediaUploader` — is captured, so their setters trap if written after this point. self.hasStartedLoading = true self.displayActivityView() @@ -461,52 +459,52 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// falls back to Gutenberg's default upload behavior (the JS override won't activate /// because `nativeUploadPort` will be nil in GBKit). private func startUploadServer() async { - // A processor/uploader that was provided but is already nil here was - // deallocated before the editor finished loading — the host didn't hold a - // strong reference. That silently disables native media handling, so trap. - precondition( - !(mediaProcessorWasAssigned && mediaProcessor == nil), - "mediaProcessor was released before the editor loaded — hold a strong reference to it." - ) - precondition( - !(mediaUploaderWasAssigned && mediaUploader == nil), - "mediaUploader was released before the editor loaded — hold a strong reference to it." - ) - // Nothing to route through the native server unless the host provided a - // processor or an uploader. + // processor or an uploader. The editor owns whichever it was given — its + // `mediaProcessor`/`mediaUploader` are strong — so there's no + // released-before-load case to guard against; they live as long as it does. guard mediaProcessor != nil || mediaUploader != nil else { return } - // A DefaultMediaUploader does two jobs: it delivers GutenbergKit-owned + // An InternalMediaClient does two jobs: it delivers GutenbergKit-owned // uploads (when no `mediaUploader` is set), and it relays the editor's media // DELETEs to the configured site — every attachment lives there, even one a - // host uploader delivered, so that's where its deletion goes. It needs a site - // root and an auth header (every host provides one — the editor injects it - // because the WebView has no auth cookies). + // host uploader delivered, so that's where its deletion goes. It needs an auth + // header (the editor injects it because the WebView has no auth cookies). // - // If GutenbergKit would have to deliver uploads itself but has no auth - // header, there's nothing to upload through: leave the server down and let - // uploads fall to the default WebView path rather than start a server that - // could only fail. - if mediaUploader == nil && configuration.authHeader.isEmpty { - return - } - var defaultUploader: DefaultMediaUploader? - if !configuration.authHeader.isEmpty { - defaultUploader = DefaultMediaUploader( - httpClient: httpClient.uploadClient(), - siteApiRoot: configuration.siteApiRoot, - siteApiNamespace: configuration.siteApiNamespace + // Without one there's no internal media client, so the behavior forks by intent: + // + // - A `mediaProcessor` only enhances GutenbergKit-owned uploads. With no + // credentials there's nothing to deliver through, so nothing to process — + // leave the server down and let uploads fall to the default WebView path. + // + // - A `mediaUploader` means the host is *taking over* uploads. Falling back + // would silently drop it, and its media deletes still need the default + // uploader to reach the configured site. A host that sets an uploader must + // provide credentials too; omitting them is a configuration error, so trap + // rather than start a server whose every delete would 500. + if configuration.authHeader.isEmpty { + precondition( + mediaUploader == nil, + "A mediaUploader needs site credentials so GutenbergKit can relay the " + + "editor's media deletes to the configured site. Set the auth header " + + "in the editor configuration." ) + return } + let internalClient = InternalMediaClient( + httpClient: httpClient.uploadClient(), + siteApiRoot: configuration.siteApiRoot, + siteApiNamespace: configuration.siteApiNamespace + ) + do { self.uploadServer = try await MediaUploadServer.start( processor: mediaProcessor, uploader: mediaUploader, - defaultUploader: defaultUploader + internalClient: internalClient ) } catch { Logger.uploadServer.error("Failed to start upload server: \(error). Falling back to default upload behavior.") diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift similarity index 83% rename from ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift rename to ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift index bfd9c1b07..1eb1a42d7 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift @@ -74,6 +74,28 @@ public protocol MediaProcessor: AnyObject, Sendable { func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile } +/// Everything a ``MediaUploader`` needs to reproduce a native upload: the file to +/// send, its metadata, the editor's non-file form fields, and the request's query. +public struct MediaUpload: Sendable { + /// The file to upload — already processed, if a ``MediaProcessor`` ran. + public let fileURL: URL + + /// The file's MIME type. + public let mimeType: String + + /// The file's name. + public let filename: String + + /// The editor's non-file form fields, decoded as UTF-8 — most importantly `post`, + /// the parent post's ID, without which the attachment is created unattached. Send + /// each as a form part on your `POST /wp/v2/media`. + public let fields: [String: String] + + /// The request's query string (leading `?`, e.g. `?_embed=wp:featuredmedia`), or + /// empty. Carry it on your request so the editor gets the response it expects. + public let query: String +} + /// Takes over *performing* a media upload — on the host's own stack: its own /// networking (say, to log every request), a background session, an offline queue, /// a resumable transport, its own retry policy. @@ -93,6 +115,10 @@ public protocol MediaUploader: AnyObject, Sendable { /// or `throw` on terminal failure: a returned value is taken as a completed /// attachment, and there is no GutenbergKit recovery behind you. /// + /// The ``MediaUpload`` carries the file plus the editor's form fields (e.g. + /// `post`) and query — send them all so the created attachment matches a native + /// upload rather than landing as an unattached orphan. + /// /// That recovery is yours to run. When `POST /wp/v2/media` fatals in server-side /// post-processing it returns a 5xx carrying the attachment's ID in /// `x-wp-upload-attachment-id` — the attachment exists but is unfinished. Don't @@ -104,7 +130,7 @@ public protocol MediaUploader: AnyObject, Sendable { /// can't be recovered, force-delete the orphan /// (`DELETE /wp/v2/media/?force=true`) before you `throw`, or it stays on the /// site — neither GutenbergKit nor core cleans up behind you. - func upload(fileAt url: URL, mimeType: String, filename: String) async throws -> Data + func upload(_ upload: MediaUpload) async throws -> Data } /// Default implementations for the optional ``MediaProcessor`` methods. diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index cee6839f6..72fd0bcd6 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -31,13 +31,14 @@ final class MediaUploadServer: Sendable { /// - Parameters: /// - processor: Optional processor for transforming files before upload. /// - uploader: Optional uploader that takes over delivery on the host's own stack. - /// - defaultUploader: Delivers to the configured site when no uploader is set. + /// - internalClient: GutenbergKit's client for the configured site — delivers + /// GutenbergKit's own uploads (when no uploader is set) and relays every delete. /// - maxRequestBodySize: The maximum allowed request body size in bytes. /// Requests exceeding this limit receive a 413 response. Defaults to 4 GB. static func start( processor: (any MediaProcessor)? = nil, uploader: (any MediaUploader)? = nil, - defaultUploader: DefaultMediaUploader? = nil, + internalClient: InternalMediaClient, maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize ) async throws -> MediaUploadServer { // Sweep temp files orphaned by a prior crash, off the editor-startup @@ -47,7 +48,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(processor: processor, uploader: uploader, defaultUploader: defaultUploader) + let context = UploadContext(processor: processor, uploader: uploader, internalClient: internalClient) // A generous ceiling for receiving the upload body. The body read is // primarily bounded by the per-read idle timeout (which reaps a stalled @@ -167,7 +168,8 @@ final class MediaUploadServer: Sendable { do { let uploadResult = try await processAndUpload( fileURL: fileURL, mimeType: mimeType, filename: filename, - extraParts: extraParts, query: query, context: context + extraParts: extraParts, query: query, + processorWantsFile: processorWantsFile, context: context ) switch uploadResult { case .uploaded(let uploaded): @@ -192,11 +194,10 @@ final class MediaUploadServer: Sendable { ) async throws -> HTTPResponse { Logger.uploadServer.debug("Passthrough: forwarding original request body to WordPress") guard let body = request.parsed.body, - let contentType = request.parsed.header("Content-Type"), - let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + let contentType = request.parsed.header("Content-Type") else { + return errorResponse(status: 500, message: "Passthrough upload requires a request body and Content-Type") } - let response = try await defaultUploader.passthroughUpload(body: body, contentType: contentType, query: query) + let response = try await context.internalClient.passthroughUpload(body: body, contentType: contentType, query: query) return relayResponse(response) } @@ -221,13 +222,13 @@ final class MediaUploadServer: Sendable { /// browser blocks it at preflight; relaying it here lets the deletion run. /// /// Every attachment lives on the configured site — even one a host uploader - /// delivered — so its deletion is relayed to the default uploader there. See + /// delivered — so its deletion is relayed to the internal media client there. See /// the accepted-risk note in the body. private static func handleMediaDelete( _ attachmentId: String, query: String, context: UploadContext ) async -> HTTPResponse { do { - // Relay to the default uploader (the configured site) — every attachment + // Relay to the internal media client (the configured site) — every attachment // lives there, even one a host uploader delivered. Core issues this only // as orphan cleanup after failed recovery, but the relay can't tell that // from any other DELETE the WebView sends: a compromised editor script @@ -236,10 +237,7 @@ final class MediaUploadServer: Sendable { // access, and a server-side compromise (a malicious plugin) deletes media // directly without the editor, so scoping this with a per-session ledger // buys little for the cost. - guard let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) - } - let response = try await defaultUploader.deleteMedia(attachmentId: attachmentId, query: query) + let response = try await context.internalClient.deleteMedia(attachmentId: attachmentId, query: query) return relayResponse(response) } catch { return uploadErrorResponse(error) @@ -254,15 +252,14 @@ final class MediaUploadServer: Sendable { /// fataled server-side, rather than surfacing a permanent failure and /// leaving an orphaned attachment behind. /// - /// The response's own `Content-Type` wins over the JSON default. `HTTPResponse` - /// serializes every header it is given, so appending the default unconditionally - /// would emit the name twice for a delegate that sets it. + /// The relayed body is always WordPress REST JSON — an attachment, or a + /// `{code, message, data}` error — so the response is always `application/json`. + /// The relayed headers are a content-type-free allowlist (`relayableHeaderNames`), + /// so prepending the JSON default never collides with them. private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { - let hasContentType = response.headers.keys.contains { $0.lowercased() == "content-type" } return HTTPResponse( status: response.statusCode, - headers: (hasContentType ? [] : [("Content-Type", "application/json")]) - + response.headers.map { ($0.key, $0.value) }, + headers: [("Content-Type", "application/json")] + response.headers.map { ($0.key, $0.value) }, body: response.body ) } @@ -285,7 +282,7 @@ final class MediaUploadServer: Sendable { /// Result of the process + deliver pipeline. private enum UploadResult { - /// The uploader or default uploader completed the upload; carries the + /// The uploader or internal media client completed the upload; carries the /// response to relay. case uploaded(MediaUploadResponse) /// No uploader is set and the processor left the file unmodified, so the @@ -295,11 +292,15 @@ final class MediaUploadServer: Sendable { private static func processAndUpload( fileURL: URL, mimeType: String, filename: String, - extraParts: [MultipartPart], query: String, context: UploadContext + extraParts: [MultipartPart], query: String, + processorWantsFile: Bool, context: UploadContext ) async throws -> UploadResult { // Step 1: transform (resize, transcode, …) if a processor claims the file. + // Reuse the gate's `handlesFile` decision from `handleUpload` rather than + // asking again — one metadata call per upload, and the admit and transform + // steps can't disagree. let processed: ProcessedProxyFile - if let processor = context.processor, processor.handlesFile(ofType: mimeType, named: filename) { + if processorWantsFile, let processor = context.processor { processed = try await processor.processFile(at: fileURL, mimeType: mimeType, filename: filename) } else { processed = .original @@ -333,23 +334,40 @@ final class MediaUploadServer: Sendable { // Step 2: deliver. An uploader owns delivery on the host's own stack and // returns the finished attachment JSON (or throws); GutenbergKit relays that // as a success and never runs its own recovery behind it. Otherwise the - // default uploader delivers to the configured site. + // internal media client delivers to the configured site. if let uploader = context.uploader { - let body = try await uploader.upload(fileAt: uploadURL, mimeType: uploadMimeType, filename: uploadFilename) + // Hand the host the editor's non-file fields (e.g. `post`) and query too, + // so its own POST can reproduce a native upload — otherwise the attachment + // is created unattached and `?_embed` is lost. + let fields = try await formFields(from: extraParts) + let upload = MediaUpload( + fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, + fields: fields, query: query + ) + let body = try await uploader.upload(upload) return .uploaded(MediaUploadResponse(statusCode: 201, body: body)) - } else if let defaultUploader = context.defaultUploader { + } else { // Unmodified — forward the original request body directly, skipping // multipart re-encoding. if case .original = processed { return .passthrough } - let result = try await defaultUploader.upload(fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, extraParts: extraParts, query: query) + let result = try await context.internalClient.upload(fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, extraParts: extraParts, query: query) return .uploaded(result) - } else { - throw UploadError.noUploader } } + /// Decodes the editor's non-file form parts (e.g. `post`, additionalData) into + /// name→value pairs for a host uploader, so it can send them on its own + /// `POST /wp/v2/media`. Values are WordPress form fields — UTF-8 text. + private static func formFields(from parts: [MultipartPart]) async throws -> [String: String] { + var fields: [String: String] = [:] + for part in parts { + fields[part.name] = String(decoding: try await part.body.data, as: UTF8.self) + } + return fields + } + 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 @@ -457,13 +475,11 @@ final class MediaUploadServer: Sendable { /// 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" } @@ -472,34 +488,36 @@ enum UploadError: Error, LocalizedError { // MARK: - Upload Context -/// Container for the media processor, uploader, and default uploader, captured by +/// Container for the media processor, uploader, and internal media client, captured by /// the HTTPServer handler closure and re-read on each request. /// -/// The processor and uploader are held **weakly** — the host owns their lifetime -/// (`EditorViewController.mediaProcessor` / `.mediaUploader` are `weak`). Capturing -/// them strongly here would risk a retain cycle (`EditorViewController → +/// The processor and uploader are held **weakly**. `EditorViewController` owns them +/// for its lifetime — its `mediaProcessor` / `.mediaUploader` are strong — and owns +/// this server too, so they stay alive for every request. The weak reference here is +/// solely to break the cycle the server would otherwise close (`EditorViewController → /// uploadServer → HTTPServer → handler → UploadContext → host object → -/// EditorViewController`) that would keep the view controller — and therefore the -/// server — alive forever, so `deinit` would never stop it. +/// EditorViewController`) whenever the host object retains the view controller: +/// capturing strongly would keep the view controller — and therefore the server — +/// alive forever, so `deinit` would never stop it. /// /// `@unchecked Sendable`: `processor`/`uploader` are assigned once at init and only /// read afterwards; weak-reference reads are thread-safe at runtime. private final class UploadContext: @unchecked Sendable { weak var processor: (any MediaProcessor)? weak var uploader: (any MediaUploader)? - let defaultUploader: DefaultMediaUploader? + let internalClient: InternalMediaClient - init(processor: (any MediaProcessor)?, uploader: (any MediaUploader)?, defaultUploader: DefaultMediaUploader?) { + init(processor: (any MediaProcessor)?, uploader: (any MediaUploader)?, internalClient: InternalMediaClient) { self.processor = processor self.uploader = uploader - self.defaultUploader = defaultUploader + self.internalClient = internalClient } } // MARK: - Default Media Uploader /// Uploads files to the WordPress REST API using site credentials from EditorConfiguration. -class DefaultMediaUploader: @unchecked Sendable { +class InternalMediaClient: @unchecked Sendable { private let httpClient: EditorHTTPClientProtocol private let siteApiRoot: URL private let siteApiNamespace: String? diff --git a/ios/Tests/GutenbergKitTests/Media/EditorMediaHandlerOwnershipTests.swift b/ios/Tests/GutenbergKitTests/Media/EditorMediaHandlerOwnershipTests.swift new file mode 100644 index 000000000..0c685e85a --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Media/EditorMediaHandlerOwnershipTests.swift @@ -0,0 +1,61 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +#if canImport(UIKit) + +/// The editor takes **strong** ownership of the media handlers it is given, so a host +/// can assign one and immediately drop its own reference. These pin that ownership: if +/// the properties regressed to `weak`, the handler would deallocate the moment the host +/// released it and the expectations below would fail. +/// +/// The mirror invariant — that the upload *server* holds them **weakly**, so it can't +/// form a retain cycle back through the view controller — lives in +/// `MediaUploadServerTests.doesNotStronglyRetainProcessor`. +@Suite("Editor media-handler ownership") +struct EditorMediaHandlerOwnershipTests: MakesTestFixtures { + static let testSiteURL = URL(string: "https://test.example.com")! + static let testApiRoot = URL(string: "https://test.example.com/wp-json/wp/v2")! + + @MainActor + @Test("the editor retains its mediaProcessor after the host releases it") + func editorRetainsMediaProcessor() { + let editor = EditorViewController(configuration: makeConfiguration()) + weak var weakProcessor: OwnershipTestProcessor? + do { + let processor = OwnershipTestProcessor() + weakProcessor = processor + editor.mediaProcessor = processor + } + withExtendedLifetime(editor) { + #expect(weakProcessor != nil, "the editor must own its mediaProcessor for its lifetime") + } + } + + @MainActor + @Test("the editor retains its mediaUploader after the host releases it") + func editorRetainsMediaUploader() { + let editor = EditorViewController(configuration: makeConfiguration()) + weak var weakUploader: OwnershipTestUploader? + do { + let uploader = OwnershipTestUploader() + weakUploader = uploader + editor.mediaUploader = uploader + } + withExtendedLifetime(editor) { + #expect(weakUploader != nil, "the editor must own its mediaUploader for its lifetime") + } + } +} + +private final class OwnershipTestProcessor: MediaProcessor, @unchecked Sendable { + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { .original } +} + +private final class OwnershipTestUploader: MediaUploader, @unchecked Sendable { + func upload(_ upload: MediaUpload) async throws -> Data { Data() } +} + +#endif diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 91c7a8da8..d32835664 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -9,7 +9,7 @@ private let _canStartUploadServer: Bool = { let semaphore = DispatchSemaphore(value: 0) Task { do { - let server = try await MediaUploadServer.start() + let server = try await MediaUploadServer.start(internalClient: MockInternalMediaClient()) server.stop() result.value = true } catch { @@ -34,7 +34,7 @@ struct MediaUploadServerTests { @Test("starts and provides a port and token") func startAndStop() async throws { - let server = try await MediaUploadServer.start() + let server = try await MediaUploadServer.start(internalClient: MockInternalMediaClient()) #expect(server.port > 0) #expect(!server.token.isEmpty) server.stop() @@ -42,7 +42,7 @@ struct MediaUploadServerTests { @Test("rejects requests without auth token") func rejectsUnauthenticated() async throws { - let server = try await MediaUploadServer.start() + let server = try await MediaUploadServer.start(internalClient: MockInternalMediaClient()) defer { server.stop() } let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! @@ -56,7 +56,7 @@ struct MediaUploadServerTests { @Test("rejects requests with wrong token") func rejectsWrongToken() async throws { - let server = try await MediaUploadServer.start() + let server = try await MediaUploadServer.start(internalClient: MockInternalMediaClient()) defer { server.stop() } let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! @@ -71,7 +71,7 @@ struct MediaUploadServerTests { @Test("responds to OPTIONS preflight with CORS headers") func corsPreflightResponse() async throws { - let server = try await MediaUploadServer.start() + let server = try await MediaUploadServer.start(internalClient: MockInternalMediaClient()) defer { server.stop() } let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! @@ -87,7 +87,7 @@ struct MediaUploadServerTests { @Test("returns 404 for unknown paths") func unknownPath() async throws { - let server = try await MediaUploadServer.start() + let server = try await MediaUploadServer.start(internalClient: MockInternalMediaClient()) defer { server.stop() } let url = URL(string: "http://127.0.0.1:\(server.port)/unknown")! @@ -103,8 +103,8 @@ struct MediaUploadServerTests { @Test("routes /upload with a query string and relays the query") func uploadWithQueryString() async throws { let processor = PassthroughProcessor() - let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } // `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, @@ -131,14 +131,14 @@ struct MediaUploadServerTests { #expect(mockUploader.lastQuery == "?_embed=wp:featuredmedia") } - @Test("relays a deletion to the default uploader even when an uploader owns uploads") - func deletesGoToDefaultUploaderNotUploader() async throws { + @Test("relays a deletion to the internal media client even when an uploader owns uploads") + func deletesGoToInternalClientNotUploader() async throws { // An attachment lives on the configured site even when a host uploader delivered - // it, so its deletion goes to the default uploader — the host uploader owns + // it, so its deletion goes to the internal media client — the host uploader owns // uploads, not deletes. Held strongly: UploadContext keeps the uploader weakly. let uploader = MockUploader() - let defaultUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploader: uploader, defaultUploader: defaultUploader) + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: internalClient) defer { server.stop() } let url = URL(string: "http://127.0.0.1:\(server.port)/media/42?force=true")! @@ -150,16 +150,16 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 200) - #expect(defaultUploader.deleteMediaCalled) - #expect(defaultUploader.deletedAttachmentId == "42") + #expect(internalClient.deleteMediaCalled) + #expect(internalClient.deletedAttachmentId == "42") } - @Test("relays a deletion to the default uploader (configured site)") - func relaysDeleteToDefaultUploader() async throws { + @Test("relays a deletion to the internal media client (configured site)") + func relaysDeleteToInternalClient() async throws { // With no uploader set, GutenbergKit owns deletes: core's orphan cleanup DELETE - // is relayed to the default uploader (the configured site). - let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(defaultUploader: mockUploader) + // is relayed to the internal media client (the configured site). + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(internalClient: mockUploader) defer { server.stop() } let url = URL(string: "http://127.0.0.1:\(server.port)/media/512?force=true")! @@ -176,16 +176,19 @@ struct MediaUploadServerTests { @Test("routes an upload to the uploader and relays its attachment") func uploaderDeliversAttachment() async throws { // With an uploader set, GutenbergKit hands it the file and relays the finished - // attachment it returns — the default uploader (configured site) is never used. + // attachment it returns — the internal media client (configured site) is never used. let uploader = MockUploader() - let defaultUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploader: uploader, defaultUploader: defaultUploader) + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: internalClient) defer { server.stop() } let boundary = UUID().uuidString - let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8)) + let body = buildMultipartBody( + boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", + data: Data("fake image data".utf8), fields: ["post": "123"] + ) - let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + 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") @@ -199,9 +202,15 @@ struct MediaUploadServerTests { #expect(uploader.uploadCalled) #expect(uploader.lastMimeType == "image/jpeg") #expect(uploader.lastFilename == "photo.jpg") + // The editor's post association and query must reach the host uploader, so it can + // reproduce a native upload (attach to the post, honor ?_embed). + #expect(uploader.lastFields["post"] == "123") + #expect(uploader.lastQuery == "?_embed=wp:featuredmedia") + // …and the actual file bytes the editor sent — the host uploads them itself. + #expect(uploader.lastFileData == Data("fake image data".utf8)) // The host owns delivery — GutenbergKit must not upload to the configured site. - #expect(!defaultUploader.uploadCalled) - #expect(!defaultUploader.passthroughUploadCalled) + #expect(!internalClient.uploadCalled) + #expect(!internalClient.passthroughUploadCalled) // The server relays the exact attachment JSON the uploader returned. let object = try JSONSerialization.jsonObject(with: data) @@ -211,11 +220,62 @@ struct MediaUploadServerTests { #expect(json["media_type"] as? String == "image") } + @Test("hands the processed file and its new metadata to the uploader") + func uploaderReceivesProcessedFile() async throws { + // A processor transcodes the file; the host uploader must receive the processed + // bytes and the new metadata, not the original clip.mov. + let processor = ResizingProcessor() + let uploader = MockUploader() + let server = try await MediaUploadServer.start( + processor: processor, uploader: uploader, internalClient: MockInternalMediaClient() + ) + 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) + + #expect(uploader.uploadCalled) + #expect(uploader.lastFileData == Data("processed".utf8)) + #expect(uploader.lastMimeType == "video/mp4") + #expect(uploader.lastFilename == "clip.mp4") + } + + @Test("relays a 500 when the host uploader throws") + func uploaderErrorRelayedAs500() async throws { + struct UploaderFailure: Error {} + let uploader = MockUploader(error: UploaderFailure()) + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + 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")! + 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) + #expect((response as? HTTPURLResponse)?.statusCode == 500) + #expect(uploader.uploadCalled) + } + @Test("uses passthrough when the processor does not modify the file") func processorPassthrough() async throws { let processor = PassthroughProcessor() - let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -247,8 +307,8 @@ struct MediaUploadServerTests { @Test("skips processing and the temp copy when the processor declines by metadata") func processorDeclinesByMetadata() async throws { let processor = DecliningProcessor() - let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -275,8 +335,8 @@ struct MediaUploadServerTests { @Test("forwards the processor's processed metadata to the uploader") func processedMetadataForwarded() async throws { let processor = ResizingProcessor() - let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -301,8 +361,8 @@ struct MediaUploadServerTests { @Test("deletes the processor's processed file after upload") func deletesProcessedFile() async throws { let processor = ResizingProcessor() - let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -326,7 +386,7 @@ struct MediaUploadServerTests { @Test("returns 413 with CORS headers when request body exceeds max size") func oversizedUploadReturns413WithCORSHeaders() async throws { - let server = try await MediaUploadServer.start(maxRequestBodySize: 1024) + let server = try await MediaUploadServer.start(internalClient: MockInternalMediaClient(), maxRequestBodySize: 1024) defer { server.stop() } let boundary = UUID().uuidString @@ -351,7 +411,7 @@ struct MediaUploadServerTests { @Test("unauthenticated oversized request returns 407, not 413 (auth precedes drain)") func oversizedUploadWithoutTokenReturns407() async throws { - let server = try await MediaUploadServer.start(maxRequestBodySize: 1024) + let server = try await MediaUploadServer.start(internalClient: MockInternalMediaClient(), maxRequestBodySize: 1024) defer { server.stop() } let boundary = UUID().uuidString @@ -397,7 +457,7 @@ struct MediaUploadServerTests { // start() kicks off cleanOrphanedUploads() off the editor-startup path. // 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() + let server = try await MediaUploadServer.start(internalClient: MockInternalMediaClient()) await server.cleanupTask.value server.stop() @@ -412,19 +472,27 @@ struct MediaUploadServerTests { do { let processor = PassthroughProcessor() weakProcessor = processor - server = try await MediaUploadServer.start(processor: processor) + server = try await MediaUploadServer.start(processor: processor, internalClient: MockInternalMediaClient()) } defer { server.stop() } - // UploadContext holds the processor weakly, so releasing the host's strong - // reference deallocates it. A strong reference here would reintroduce the - // EditorViewController → uploadServer → … → processor → EditorViewController - // cycle, so deinit would never fire and the server would never stop. + // The server (via UploadContext) holds the processor *weakly*, so once the only + // strong reference is released it deallocates. This is deliberately the opposite + // of the editor, which owns the processor *strongly* for its lifetime (see + // `EditorMediaHandlerOwnershipTests`): a strong capture here would reintroduce the + // EditorViewController → uploadServer → … → processor → EditorViewController cycle, + // so deinit would never fire and the server would never stop. #expect(weakProcessor == nil) } - private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data) -> Data { + private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data, fields: [String: String] = [:]) -> Data { var body = Data() + for (name, value) in fields { + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"\(name)\"\r\n\r\n") + body.append(value) + body.append("\r\n") + } body.append("--\(boundary)\r\n") body.append("Content-Disposition: form-data; name=\"file\"; filename=\"\(filename)\"\r\n") body.append("Content-Type: \(mimeType)\r\n\r\n") @@ -436,7 +504,7 @@ struct MediaUploadServerTests { // MARK: - Streaming Multipart Body Tests -@Suite("DefaultMediaUploader streaming multipart body") +@Suite("InternalMediaClient streaming multipart body") struct MultipartBodyStreamTests { @Test("streaming output matches in-memory multipart format") @@ -459,7 +527,7 @@ struct MultipartBodyStreamTests { expected.append(Data("\r\n--\(boundary)--\r\n".utf8)) // Build streaming output. - let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( + let (stream, contentLength) = try InternalMediaClient.multipartBodyStream( fileURL: tempFile, boundary: boundary, filename: filename, mimeType: mimeType, extraFields: [] ) #expect(contentLength == expected.count) @@ -476,7 +544,7 @@ struct MultipartBodyStreamTests { // Craft a filename, field name, and MIME type that each try to smuggle a CRLF // and a fake header into the body relayed to WordPress. - let (stream, _) = try DefaultMediaUploader.multipartBodyStream( + let (stream, _) = try InternalMediaClient.multipartBodyStream( fileURL: tempFile, boundary: "boundary", filename: "evil\"\r\nX-Injected-File: 1.jpg", @@ -511,7 +579,7 @@ struct MultipartBodyStreamTests { expected.append(fileContent) expected.append(Data("\r\n--\(boundary)--\r\n".utf8)) - let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( + let (stream, contentLength) = try InternalMediaClient.multipartBodyStream( fileURL: tempFile, boundary: boundary, filename: filename, mimeType: mimeType, extraFields: [("post", Data("123".utf8))] ) @@ -543,7 +611,7 @@ struct MultipartBodyStreamTests { expected.append(fileContent) expected.append(Data("\r\n--\(boundary)--\r\n".utf8)) - let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( + let (stream, contentLength) = try InternalMediaClient.multipartBodyStream( fileURL: tempFile, boundary: boundary, filename: filename, mimeType: mimeType, extraFields: [("blob", binaryValue)] ) @@ -559,7 +627,7 @@ struct MultipartBodyStreamTests { try fileContent.write(to: tempFile) defer { try? FileManager.default.removeItem(at: tempFile) } - let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( + let (stream, contentLength) = try InternalMediaClient.multipartBodyStream( fileURL: tempFile, boundary: "boundary", filename: "big.bin", mimeType: "application/octet-stream", extraFields: [] ) @@ -583,7 +651,7 @@ struct MultipartBodyStreamTests { let preamble = Data("PREAMBLE".utf8) let epilogue = Data("EPILOGUE".utf8) - let ok = DefaultMediaUploader.writeMultipartBody( + let ok = InternalMediaClient.writeMultipartBody( fileHandle: fileHandle, fileSize: fileContent.count, preamble: preamble, epilogue: epilogue, to: output ) @@ -610,7 +678,7 @@ struct MultipartBodyStreamTests { let preamble = Data("PREAMBLE".utf8) let epilogue = Data("EPILOGUE".utf8) // Claim the file is larger than it is, as if it shrank after being measured. - let ok = DefaultMediaUploader.writeMultipartBody( + let ok = InternalMediaClient.writeMultipartBody( fileHandle: fileHandle, fileSize: fileContent.count + 100, preamble: preamble, epilogue: epilogue, to: output ) @@ -623,17 +691,17 @@ struct MultipartBodyStreamTests { } } -// MARK: - DefaultMediaUploader Relay Tests +// MARK: - InternalMediaClient Relay Tests -@Suite("DefaultMediaUploader relay") -struct DefaultMediaUploaderRelayTests { +@Suite("InternalMediaClient relay") +struct InternalMediaClientRelayTests { @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 uploader = InternalMediaClient(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) @@ -660,7 +728,7 @@ struct DefaultMediaUploaderRelayTests { body: Data(#"{"code":"rest_upload_error"}"#.utf8), headerFields: ["x-wp-upload-attachment-id": "4242"] ) - let uploader = DefaultMediaUploader( + let uploader = InternalMediaClient( httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json/")!) let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent( @@ -683,7 +751,7 @@ struct DefaultMediaUploaderRelayTests { body: Data("{}".utf8), headerFields: ["X-Powered-By": "PHP/8.2", "Set-Cookie": "session=secret"] ) - let uploader = DefaultMediaUploader( + let uploader = InternalMediaClient( httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json/")!) let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent( @@ -701,7 +769,7 @@ struct DefaultMediaUploaderRelayTests { @Test("deletes an attachment, carrying the namespace and force query") func deletesAttachment() async throws { let client = URLCapturingHTTPClient() - let uploader = DefaultMediaUploader( + let uploader = InternalMediaClient( httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json")!, siteApiNamespace: ["sites/123"] @@ -717,7 +785,7 @@ struct DefaultMediaUploaderRelayTests { @Test("carries the namespace and request query through to the media endpoint") func forwardsNamespaceAndQuery() async throws { let client = URLCapturingHTTPClient() - let uploader = DefaultMediaUploader( + let uploader = InternalMediaClient( httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json")!, siteApiNamespace: ["sites/123"] @@ -740,7 +808,7 @@ struct DefaultMediaUploaderRelayTests { /// 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 +/// `EditorHTTPClient`. Lets a test prove `InternalMediaClient` routes uploads /// through `performRaw` (relay) rather than `perform` (throw). private struct RelayStubHTTPClient: EditorHTTPClientProtocol { let statusCode: Int @@ -818,22 +886,39 @@ private final class MockUploader: MediaUploader, @unchecked Sendable { private var _uploadCalled = false private var _lastMimeType: String? private var _lastFilename: String? + private var _lastFields: [String: String] = [:] + private var _lastQuery: String? + private var _lastFileData: Data? private let uploadBody: Data + private let error: (any Error)? var uploadCalled: Bool { lock.withLock { _uploadCalled } } var lastMimeType: String? { lock.withLock { _lastMimeType } } var lastFilename: String? { lock.withLock { _lastFilename } } + var lastFields: [String: String] { lock.withLock { _lastFields } } + var lastQuery: String? { lock.withLock { _lastQuery } } + var lastFileData: Data? { lock.withLock { _lastFileData } } - init(uploadBody: Data = Data(#"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"#.utf8)) { + init( + uploadBody: Data = Data(#"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"#.utf8), + error: (any Error)? = nil + ) { self.uploadBody = uploadBody + self.error = error } - func upload(fileAt url: URL, mimeType: String, filename: String) async throws -> Data { + func upload(_ upload: MediaUpload) async throws -> Data { + // Read the file the server handed us so a test can assert its contents. + let fileData = try? Data(contentsOf: upload.fileURL) lock.withLock { _uploadCalled = true - _lastMimeType = mimeType - _lastFilename = filename + _lastMimeType = upload.mimeType + _lastFilename = upload.filename + _lastFields = upload.fields + _lastQuery = upload.query + _lastFileData = fileData } + if let error { throw error } return uploadBody } } @@ -883,7 +968,7 @@ private final class ResizingProcessor: MediaProcessor, @unchecked Sendable { } } -private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendable { +private final class MockInternalMediaClient: InternalMediaClient, @unchecked Sendable { private let lock = NSLock() private var _uploadCalled = false private var _passthroughUploadCalled = false From be5678aeb4f217b1b5bb42d21460dbaa80a07c9f Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 14:49:10 -0600 Subject: [PATCH 12/21] fix: hand a media uploader its form fields as an ordered list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MediaUpload.fields was a last-wins map, so a repeated field name (e.g. a `field[]` array) collapsed to its final value before a host MediaUploader saw it — diverging from GutenbergKit's own upload path, which preserves repeats. Pass an ordered list of (name, value) pairs on both platforms. --- .../wordpress/gutenberg/MediaUploadServer.kt | 12 +++-- .../gutenberg/MediaUploadServerTest.kt | 49 +++++++++++++++++-- .../Sources/Media/MediaHandlers.swift | 10 ++-- .../Sources/Media/MediaUploadServer.swift | 13 ++--- .../Media/MediaUploadServerTests.swift | 47 ++++++++++++++++-- 5 files changed, 107 insertions(+), 24 deletions(-) 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 9eda0953f..f02296fb1 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -105,9 +105,11 @@ interface MediaProcessor { * @property file The file to upload — already processed, if a [MediaProcessor] ran. * @property mimeType The file's MIME type. * @property filename The file's name. - * @property fields The editor's non-file form fields, decoded as UTF-8 — most - * importantly `post`, the parent post's ID, without which the attachment is created - * unattached. Send each as a form part on your `POST /wp/v2/media`. + * @property fields The editor's non-file form fields, in order, each decoded as UTF-8 — + * most importantly `post`, the parent post's ID, without which the attachment is + * created unattached. A list of `(name, value)` pairs, not a map, so repeated field + * names (e.g. a `field[]` array) survive verbatim. Send each as a form part on your + * `POST /wp/v2/media`, in the given order. * @property query The request's query string (leading `?`, e.g. `?_embed=...`), or * empty. Carry it on your request so the editor gets the response it expects. */ @@ -115,7 +117,7 @@ data class MediaUpload( val file: File, val mimeType: String, val filename: String, - val fields: Map, + val fields: List>, val query: String ) @@ -536,7 +538,7 @@ internal class MediaUploadServer( // Hand the host the editor's non-file fields (e.g. `post`) and query // too, so its own POST can reproduce a native upload — otherwise the // attachment is created unattached and `?_embed` is lost. - val fields = extraParts.associate { part -> + val fields = extraParts.map { part -> part.name to String(part.body.readBytes(), Charsets.UTF_8) } val upload = MediaUpload(targetFile, targetMimeType, targetFilename, fields, 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 ab7282387..4ccc7e970 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -256,7 +256,7 @@ class MediaUploadServerTest { val boundary = "test-boundary-123" val body = buildMultipartBody( boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray(), - fields = mapOf("post" to "123") + fields = listOf("post" to "123") ) val response = sendRawRequest( @@ -275,7 +275,7 @@ class MediaUploadServerTest { assertEquals("photo.jpg", uploader.lastFilename) // The editor's post association and query must reach the host uploader, so it // can reproduce a native upload (attach to the post, honor ?_embed). - assertEquals("123", uploader.lastFields["post"]) + assertEquals("123", uploader.lastFields.first { it.first == "post" }.second) assertEquals("?_embed=wp:featuredmedia", uploader.lastQuery) // …and the actual file bytes the editor sent — the host uploads them itself. assertEquals("fake image data", uploader.lastFileBytes?.decodeToString()) @@ -290,6 +290,47 @@ class MediaUploadServerTest { assertEquals("image", json.get("media_type").asString) } + @Test + fun `hands a host uploader repeated form field names in order, not collapsed`() { + // A `field[]`-style repeated name (e.g. a custom attachment taxonomy): WordPress + // builds an array from these, so both values must reach the host uploader in + // order. A map would drop the first — the ordered-list contract must not. + val uploader = MockUploader() + val internalClient = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + processor = null, + uploader = uploader, + internalClient = internalClient, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-123" + val body = buildMultipartBody( + boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray(), + fields = listOf("post" to "123", "media_folder[]" to "12", "media_folder[]" to "45") + ) + + val response = sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) + assertTrue(uploader.uploadCalled) + // Both repeated values survive, in order — not collapsed to the last. + assertEquals( + listOf("12", "45"), + uploader.lastFields.filter { it.first == "media_folder[]" }.map { it.second } + ) + assertEquals("123", uploader.lastFields.first { it.first == "post" }.second) + } + @Test fun `hands the processed file and its new metadata to the uploader`() { // A processor transcodes the file; the host uploader must receive the processed @@ -863,7 +904,7 @@ class MediaUploadServerTest { filename: String, mimeType: String, data: ByteArray, - fields: Map = emptyMap() + fields: List> = emptyList() ): ByteArray { val out = java.io.ByteArrayOutputStream() for ((name, value) in fields) { @@ -894,7 +935,7 @@ class MediaUploadServerTest { @Volatile var uploadCalled = false @Volatile var lastMimeType: String? = null @Volatile var lastFilename: String? = null - @Volatile var lastFields: Map = emptyMap() + @Volatile var lastFields: List> = emptyList() @Volatile var lastQuery: String? = null @Volatile var lastFileBytes: ByteArray? = null diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift index 1eb1a42d7..c9aacc9b0 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift @@ -86,10 +86,12 @@ public struct MediaUpload: Sendable { /// The file's name. public let filename: String - /// The editor's non-file form fields, decoded as UTF-8 — most importantly `post`, - /// the parent post's ID, without which the attachment is created unattached. Send - /// each as a form part on your `POST /wp/v2/media`. - public let fields: [String: String] + /// The editor's non-file form fields, in order, each decoded as UTF-8 — most + /// importantly `post`, the parent post's ID, without which the attachment is created + /// unattached. A list of `(name, value)` pairs, not a dictionary, so repeated field + /// names (e.g. a `field[]` array) survive verbatim. Send each as a form part on your + /// `POST /wp/v2/media`, in the given order. + public let fields: [(name: String, value: String)] /// The request's query string (leading `?`, e.g. `?_embed=wp:featuredmedia`), or /// empty. Carry it on your request so the editor gets the response it expects. diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 72fd0bcd6..2daa9ce1f 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -357,13 +357,14 @@ final class MediaUploadServer: Sendable { } } - /// Decodes the editor's non-file form parts (e.g. `post`, additionalData) into - /// name→value pairs for a host uploader, so it can send them on its own - /// `POST /wp/v2/media`. Values are WordPress form fields — UTF-8 text. - private static func formFields(from parts: [MultipartPart]) async throws -> [String: String] { - var fields: [String: String] = [:] + /// Decodes the editor's non-file form parts (e.g. `post`, additionalData) into an + /// ordered list of name/value pairs for a host uploader, so it can send them on its + /// own `POST /wp/v2/media`. A list, not a dictionary, so repeated field names survive + /// in order. Values are WordPress form fields — UTF-8 text. + private static func formFields(from parts: [MultipartPart]) async throws -> [(name: String, value: String)] { + var fields: [(name: String, value: String)] = [] for part in parts { - fields[part.name] = String(decoding: try await part.body.data, as: UTF8.self) + fields.append((name: part.name, value: String(decoding: try await part.body.data, as: UTF8.self))) } return fields } diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index d32835664..72346489a 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -185,7 +185,7 @@ struct MediaUploadServerTests { let boundary = UUID().uuidString let body = buildMultipartBody( boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", - data: Data("fake image data".utf8), fields: ["post": "123"] + data: Data("fake image data".utf8), fields: [(name: "post", value: "123")] ) let url = URL(string: "http://127.0.0.1:\(server.port)/upload?_embed=wp:featuredmedia")! @@ -204,7 +204,7 @@ struct MediaUploadServerTests { #expect(uploader.lastFilename == "photo.jpg") // The editor's post association and query must reach the host uploader, so it can // reproduce a native upload (attach to the post, honor ?_embed). - #expect(uploader.lastFields["post"] == "123") + #expect(uploader.lastFields.first { $0.name == "post" }?.value == "123") #expect(uploader.lastQuery == "?_embed=wp:featuredmedia") // …and the actual file bytes the editor sent — the host uploads them itself. #expect(uploader.lastFileData == Data("fake image data".utf8)) @@ -220,6 +220,43 @@ struct MediaUploadServerTests { #expect(json["media_type"] as? String == "image") } + @Test("hands a host uploader repeated form field names in order, not collapsed") + func uploaderReceivesRepeatedFieldNames() async throws { + // A `field[]`-style repeated name (e.g. a custom attachment taxonomy): WordPress + // builds an array from these, so both values must reach the host uploader in order. + // A dictionary would drop the first — the ordered-list contract must not. + let uploader = MockUploader() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: internalClient) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody( + boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", + data: Data("fake image data".utf8), + fields: [ + (name: "post", value: "123"), + (name: "media_folder[]", value: "12"), + (name: "media_folder[]", value: "45"), + ] + ) + + 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) + + #expect(uploader.uploadCalled) + // Both repeated values survive, in order — not collapsed to the last. + let folderValues = uploader.lastFields.filter { $0.name == "media_folder[]" }.map(\.value) + #expect(folderValues == ["12", "45"]) + #expect(uploader.lastFields.first { $0.name == "post" }?.value == "123") + } + @Test("hands the processed file and its new metadata to the uploader") func uploaderReceivesProcessedFile() async throws { // A processor transcodes the file; the host uploader must receive the processed @@ -485,7 +522,7 @@ struct MediaUploadServerTests { #expect(weakProcessor == nil) } - private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data, fields: [String: String] = [:]) -> Data { + private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data, fields: [(name: String, value: String)] = []) -> Data { var body = Data() for (name, value) in fields { body.append("--\(boundary)\r\n") @@ -886,7 +923,7 @@ private final class MockUploader: MediaUploader, @unchecked Sendable { private var _uploadCalled = false private var _lastMimeType: String? private var _lastFilename: String? - private var _lastFields: [String: String] = [:] + private var _lastFields: [(name: String, value: String)] = [] private var _lastQuery: String? private var _lastFileData: Data? private let uploadBody: Data @@ -895,7 +932,7 @@ private final class MockUploader: MediaUploader, @unchecked Sendable { var uploadCalled: Bool { lock.withLock { _uploadCalled } } var lastMimeType: String? { lock.withLock { _lastMimeType } } var lastFilename: String? { lock.withLock { _lastFilename } } - var lastFields: [String: String] { lock.withLock { _lastFields } } + var lastFields: [(name: String, value: String)] { lock.withLock { _lastFields } } var lastQuery: String? { lock.withLock { _lastQuery } } var lastFileData: Data? { lock.withLock { _lastFileData } } From 23f919d90f2a276feace4619fe58335a08d4f012 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 18:50:29 -0600 Subject: [PATCH 13/21] fix: keep a media handler alive for the whole upload request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UploadContext held the processor and uploader `weak`, and each was read twice per request — once at the admission gate, once at delivery. Those reads are separated by a synchronous disk copy and an unbounded `processFile`, so a host that released its handler in that window (the user closing the editor mid-transcode) changed the answer between them: a file admitted for processing was forwarded unprocessed, and an upload gated on a host uploader was delivered by GutenbergKit itself — creating an attachment on the configured site that the host never learns about, behind an uploader documented as keeping GutenbergKit "out of the network entirely". Hold all three strongly, as Android already does. The `weak` was load-bearing when `EditorViewController.mediaUploadDelegate` was itself `weak` and this was the only strong path; 4a03bcc5 made those properties strong and left it behind. It no longer prevents a cycle — a host object retaining the view controller already forms `EditorViewController -> mediaUploader -> EditorViewController` through the view controller's own strong property, which this container can neither create nor prevent. Immutable strong references also make the two reads agree by construction, so `handlesFile` admission and delivery can't disagree. UploadContext becomes a struct and drops its `@unchecked Sendable` opt-out: both protocols are `Sendable` and InternalMediaClient is `@unchecked Sendable`, so it is implicitly Sendable. `doesNotStronglyRetainProcessor` pinned the vestigial invariant, so it is replaced by `retainsProcessorForServerLifetime`, asserting both halves — the server owns its processor while it runs, and releases it when the server goes away. `uploaderReleasedMidRequestStillDelivers` covers the bug directly; against the previous commit it fails with the real symptom, the host uploader bypassed and passthroughUpload called. --- .../Sources/Media/MediaUploadServer.swift | 39 +++--- .../Media/MediaUploadServerTests.swift | 130 ++++++++++++++++-- 2 files changed, 139 insertions(+), 30 deletions(-) diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 2daa9ce1f..22ff10167 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -490,29 +490,28 @@ enum UploadError: Error, LocalizedError { // MARK: - Upload Context /// Container for the media processor, uploader, and internal media client, captured by -/// the HTTPServer handler closure and re-read on each request. +/// the HTTPServer handler closure and read on each request. /// -/// The processor and uploader are held **weakly**. `EditorViewController` owns them -/// for its lifetime — its `mediaProcessor` / `.mediaUploader` are strong — and owns -/// this server too, so they stay alive for every request. The weak reference here is -/// solely to break the cycle the server would otherwise close (`EditorViewController → -/// uploadServer → HTTPServer → handler → UploadContext → host object → -/// EditorViewController`) whenever the host object retains the view controller: -/// capturing strongly would keep the view controller — and therefore the server — -/// alive forever, so `deinit` would never stop it. +/// Everything here is held **strongly**, so a handler that admitted a file for +/// processing will process it, and one that gated on an uploader will deliver through +/// it — the reads can't disagree within a request, and an in-flight upload keeps the +/// host's handlers alive until it unwinds. This matches Android, which holds its +/// `processor`/`uploader` as plain `val`s for the same reason. /// -/// `@unchecked Sendable`: `processor`/`uploader` are assigned once at init and only -/// read afterwards; weak-reference reads are thread-safe at runtime. -private final class UploadContext: @unchecked Sendable { - weak var processor: (any MediaProcessor)? - weak var uploader: (any MediaUploader)? +/// Strong is safe because `EditorViewController` owns `mediaProcessor` / +/// `mediaUploader` strongly too. A host object that retains the view controller back +/// already forms `EditorViewController → mediaUploader → EditorViewController`, a +/// cycle this container can neither create nor prevent — so holding weak here bought +/// no leak protection, only the risk of a reference vanishing mid-request. (It *was* +/// load-bearing when the view controller held its delegate `weak` and this was the +/// only strong path; that changed when those properties became strong.) +/// +/// A `struct`, so it is implicitly `Sendable`: `MediaProcessor` and `MediaUploader` +/// are `Sendable` protocols and `InternalMediaClient` is `@unchecked Sendable`. +private struct UploadContext: Sendable { + let processor: (any MediaProcessor)? + let uploader: (any MediaUploader)? let internalClient: InternalMediaClient - - init(processor: (any MediaProcessor)?, uploader: (any MediaUploader)?, internalClient: InternalMediaClient) { - self.processor = processor - self.uploader = uploader - self.internalClient = internalClient - } } // MARK: - Default Media Uploader diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 72346489a..671312364 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -502,24 +502,95 @@ struct MediaUploadServerTests { #expect(FileManager.default.fileExists(atPath: fresh.path(percentEncoded: false))) } - @Test("does not strongly retain the processor (weak — preserves deinit teardown)") - func doesNotStronglyRetainProcessor() async throws { + @Test("retains the processor for the server's lifetime, and releases it on stop") + func retainsProcessorForServerLifetime() async throws { weak var weakProcessor: PassthroughProcessor? - let server: MediaUploadServer + var server: MediaUploadServer? do { let processor = PassthroughProcessor() weakProcessor = processor server = try await MediaUploadServer.start(processor: processor, internalClient: MockInternalMediaClient()) } + + // The server (via UploadContext) holds the processor *strongly*, matching both the + // editor's own ownership (see `EditorMediaHandlerOwnershipTests`) and Android's + // plain `val`. Holding it weakly here bought no leak protection — a host object + // that retains the editor already cycles through the editor's own strong + // `mediaProcessor` — and only risked the reference vanishing mid-request. + #expect(weakProcessor != nil, "the server must own its processor while it runs") + + // …and releases it when the server goes away, so nothing outlives the editor. + // `stop()` cancels the NWListener, whose handlers are torn down asynchronously, + // so the final release lands a beat after `server = nil` — poll rather than + // assert instantly. + server?.stop() + server = nil + for _ in 0..<200 where weakProcessor != nil { + try await Task.sleep(for: .milliseconds(10)) + } + #expect(weakProcessor == nil, "releasing the server must release the processor") + } + + @Test("delivers through an uploader the host releases mid-request") + func uploaderReleasedMidRequestStillDelivers() async throws { + let mockClient = MockInternalMediaClient() + let gate = UnsafeMutableSendablePointer(false) + let didUpload = UnsafeMutableSendablePointer(false) + let processor = GatedProcessor(gate: gate) + + // `holder` stands in for the *host's* own reference to its uploader, which the + // docs say it may drop after assigning. It's built in a nested scope so the + // `start` call's existential temporary dies with that scope; left in the test's + // own frame, that temporary keeps the uploader alive whatever the server does, + // and the test would pass vacuously. The uploader's record of having run lives in + // `didUpload`, off the object, so it survives the release either way. + let holder = UnsafeMutableSendablePointer<(any MediaUploader)?>(nil) + let server: MediaUploadServer + do { + let uploader = ReleasableUploader(didUpload: didUpload) + holder.value = uploader + server = try await MediaUploadServer.start( + processor: processor, uploader: uploader, internalClient: mockClient + ) + } defer { server.stop() } - // The server (via UploadContext) holds the processor *weakly*, so once the only - // strong reference is released it deallocates. This is deliberately the opposite - // of the editor, which owns the processor *strongly* for its lifetime (see - // `EditorMediaHandlerOwnershipTests`): a strong capture here would reintroduce the - // EditorViewController → uploadServer → … → processor → EditorViewController cycle, - // so deinit would never fire and the server would never stop. - #expect(weakProcessor == nil) + 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")! + 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 requestTask = Task { try await URLSession.shared.data(for: request) } + + // Park the request inside `processFile` — past the gate that saw the uploader, + // before the delivery step. This is the window a real host sits in while a + // transcode runs. + while !processor.processFileStarted { + try await Task.sleep(for: .milliseconds(5)) + } + + // The host drops its reference mid-upload. While `UploadContext` held the uploader + // weakly this dropped the last one: delivery then read nil and silently uploaded + // through the internal client instead — breaking the "GutenbergKit out of the + // network entirely" contract and creating an attachment the host never learns + // about. The server owns the uploader now, so the request delivers as promised. + holder.value = nil + + gate.value = true + let (_, response) = try await requestTask.value + + let httpResponse = try #require(response as? HTTPURLResponse) + #expect(httpResponse.statusCode == 201) + #expect(didUpload.value, "the released uploader must still own delivery") + #expect(!mockClient.uploadCalled, "GutenbergKit must not upload behind an uploader") + #expect(!mockClient.passthroughUploadCalled) } private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data, fields: [(name: String, value: String)] = []) -> Data { @@ -960,6 +1031,45 @@ private final class MockUploader: MediaUploader, @unchecked Sendable { } } +/// Holds a request open inside `processFile` until the test opens `gate`, so the test +/// can act while the request is parked between the admission gate and delivery. +private final class GatedProcessor: MediaProcessor, @unchecked Sendable { + private let lock = NSLock() + private var _processFileStarted = false + private let gate: UnsafeMutableSendablePointer + + var processFileStarted: Bool { lock.withLock { _processFileStarted } } + + init(gate: UnsafeMutableSendablePointer) { + self.gate = gate + } + + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { true } + + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + lock.withLock { _processFileStarted = true } + while !gate.value { + try await Task.sleep(for: .milliseconds(5)) + } + return .original + } +} + +/// An uploader whose record of having run lives *outside* the object, so a test can +/// release its last strong reference mid-request and still assert that it delivered. +private final class ReleasableUploader: MediaUploader, @unchecked Sendable { + private let didUpload: UnsafeMutableSendablePointer + + init(didUpload: UnsafeMutableSendablePointer) { + self.didUpload = didUpload + } + + func upload(_ upload: MediaUpload) async throws -> Data { + didUpload.value = true + return Data(#"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"#.utf8) + } +} + private final class PassthroughProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false From 60eae7a9d9183842976b4a6528f7be87658f72ea Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 18:50:48 -0600 Subject: [PATCH 14/21] fix: don't start a media upload for a torn-down editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both delivery paths could put bytes on the wire after the editor was gone. `EditorViewController.deinit` calls `stop()`, which cancels the in-flight connection tasks, but Swift cancellation is cooperative: `writeStream` is an uninterruptible read loop and a host's `processFile` need not check at all, so a handler can reach delivery well after teardown. Whether the request then actually reached WordPress rested entirely on URLSession noticing the cancellation. That is not a guarantee the server can rely on. `URLSessionProtocol` is public and documented for dependency injection, and the obvious conformance for a host wrapping a callback-based stack — `withCheckedThrowingContinuation` around a completion handler — has no cancellation awareness at all. Such a host would upload deterministically after teardown, and the response is discarded either way, leaving an attachment on the site that nothing cleans up. Check cancellation explicitly before delivery in `processAndUpload` and before `passthroughUpload`, so the guarantee comes from this file rather than from the HTTP client's behaviour. CancellationError is already handled quietly by `uploadErrorResponse`, and HTTPServer drops the response for a cancelled task. --- .../Sources/Media/MediaUploadServer.swift | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 22ff10167..5bc41d2d8 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -192,6 +192,10 @@ final class MediaUploadServer: Sendable { private static func passthroughResponse( _ request: HTTPServer.Request, query: String, context: UploadContext ) async throws -> HTTPResponse { + // As in `processAndUpload`: don't put bytes on the wire for a torn-down + // editor, regardless of whether the HTTP client honors cancellation. + try Task.checkCancellation() + Logger.uploadServer.debug("Passthrough: forwarding original request body to WordPress") guard let body = request.parsed.body, let contentType = request.parsed.header("Content-Type") else { @@ -331,6 +335,13 @@ final class MediaUploadServer: Sendable { } } + // The editor was torn down (or the client disconnected) while we processed. + // Don't start an outbound upload whose response nobody will read — it would + // create an attachment neither GutenbergKit nor the host knows to clean up. + // Checking here rather than relying on the HTTP client to notice cancellation + // keeps this true for a host-injected `URLSessionProtocol` that doesn't. + try Task.checkCancellation() + // Step 2: deliver. An uploader owns delivery on the host's own stack and // returns the finished attachment JSON (or throws); GutenbergKit relays that // as a success and never runs its own recovery behind it. Otherwise the From 689bc9856863257700d60c9ee023e2103eeccef6 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 18:51:01 -0600 Subject: [PATCH 15/21] refactor: hand the media relay handlers only the client they use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `passthroughResponse` and `handleMediaDelete` took the whole UploadContext and touched only `internalClient`. Pass that directly. On the delete path this is more than tidiness. Every attachment lives on the configured site — even one a host uploader delivered — so a deletion always relays through the internal client, never the uploader. That was a convention the signature let you break; now it is a fact the compiler enforces. `handleUpload` and `processAndUpload` keep the context: they genuinely need all three, and spelling them out would push processAndUpload to nine parameters. --- .../Sources/Media/MediaUploadServer.swift | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 5bc41d2d8..a711c1278 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -101,7 +101,7 @@ final class MediaUploadServer: Sendable { } if method == "DELETE", let attachmentId = attachmentId(fromPath: parsed.path) { - return await handleMediaDelete(attachmentId, query: parsed.query, context: context) + return await handleMediaDelete(attachmentId, query: parsed.query, internalClient: context.internalClient) } return errorResponse(status: 404, message: "Not found") @@ -137,7 +137,7 @@ final class MediaUploadServer: Sendable { let processorWantsFile = context.processor?.handlesFile(ofType: mimeType, named: filename) ?? false if context.uploader == nil, !processorWantsFile { do { - return try await passthroughResponse(request, query: query, context: context) + return try await passthroughResponse(request, query: query, internalClient: context.internalClient) } catch { return uploadErrorResponse(error) } @@ -178,7 +178,7 @@ final class MediaUploadServer: Sendable { case .passthrough: // Delegate didn't modify the file — forward the original request // body to WordPress without re-encoding. - return try await passthroughResponse(request, query: query, context: context) + return try await passthroughResponse(request, query: query, internalClient: context.internalClient) } } catch { return uploadErrorResponse(error) @@ -190,7 +190,7 @@ final class MediaUploadServer: Sendable { /// processor won't touch the file — it declined by metadata (`handlesFile` /// returned false) or `processFile` returned `.original`. private static func passthroughResponse( - _ request: HTTPServer.Request, query: String, context: UploadContext + _ request: HTTPServer.Request, query: String, internalClient: InternalMediaClient ) async throws -> HTTPResponse { // As in `processAndUpload`: don't put bytes on the wire for a torn-down // editor, regardless of whether the HTTP client honors cancellation. @@ -201,7 +201,7 @@ final class MediaUploadServer: Sendable { let contentType = request.parsed.header("Content-Type") else { return errorResponse(status: 500, message: "Passthrough upload requires a request body and Content-Type") } - let response = try await context.internalClient.passthroughUpload(body: body, contentType: contentType, query: query) + let response = try await internalClient.passthroughUpload(body: body, contentType: contentType, query: query) return relayResponse(response) } @@ -229,7 +229,7 @@ final class MediaUploadServer: Sendable { /// delivered — so its deletion is relayed to the internal media client there. See /// the accepted-risk note in the body. private static func handleMediaDelete( - _ attachmentId: String, query: String, context: UploadContext + _ attachmentId: String, query: String, internalClient: InternalMediaClient ) async -> HTTPResponse { do { // Relay to the internal media client (the configured site) — every attachment @@ -241,7 +241,7 @@ final class MediaUploadServer: Sendable { // access, and a server-side compromise (a malicious plugin) deletes media // directly without the editor, so scoping this with a per-session ledger // buys little for the cost. - let response = try await context.internalClient.deleteMedia(attachmentId: attachmentId, query: query) + let response = try await internalClient.deleteMedia(attachmentId: attachmentId, query: query) return relayResponse(response) } catch { return uploadErrorResponse(error) From 95331edb98bf9a7715ebf7e41dd4ef45a1f7d84d Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:05:38 -0600 Subject: [PATCH 16/21] feat: let an HTTPServer serve requests from a handler object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The closure form of `start` can't capture the object that owns the server: the closure has to exist before the server does, and retrofitting `self` would form `owner -> HTTPServer -> handler -> owner`, so the owner's deinit — and its `stop()` — would never run. A consumer with dependencies to hold therefore ends up with static functions threading a context parameter through every call, which is how MediaUploadServer is written today. Add an `HTTPRequestHandler` protocol and a `start` overload that takes one. The dependencies become stored properties and the request logic becomes instance methods. The protocol is deliberately not `AnyObject`-constrained: a struct conformer cannot participate in a reference cycle at all, so the ownership question doesn't arise. A final class works too, under the same leaf discipline HTTPServerDelegate already documents. The closure overload is unchanged and forwards to the same code path, so this is purely additive — no existing caller, test, or the debug server is affected. Request handling is mandatory, so it can't be a defaulted HTTPServerDelegate method the way optional customization points are; hence an overload rather than a new delegate requirement. --- .../GutenbergKitHTTP/HTTPRequestHandler.swift | 35 ++++++++++++++++ ios/Sources/GutenbergKitHTTP/HTTPServer.swift | 40 +++++++++++++++++++ ios/Sources/GutenbergKitHTTP/README.md | 18 +++++++++ .../HTTPServerStartTests.swift | 29 ++++++++++++++ 4 files changed, 122 insertions(+) create mode 100644 ios/Sources/GutenbergKitHTTP/HTTPRequestHandler.swift diff --git a/ios/Sources/GutenbergKitHTTP/HTTPRequestHandler.swift b/ios/Sources/GutenbergKitHTTP/HTTPRequestHandler.swift new file mode 100644 index 000000000..2caa266af --- /dev/null +++ b/ios/Sources/GutenbergKitHTTP/HTTPRequestHandler.swift @@ -0,0 +1,35 @@ +#if canImport(Network) + +import Foundation + +/// Serves requests for an ``HTTPServer``. +/// +/// The closure form of +/// ``HTTPServer/start(name:port:listenOnAllInterfaces:requiresAuthentication:maxRequestBodySize:maxConnections:readTimeout:bodyReadTimeout:idleTimeout:startTimeout:cors:delegate:handler:)-(_,_,_,_,_,_,_,_,_,_,_,_,@escaping@Sendable(HTTPServer.Request)async->HTTPResponse)`` +/// is the right tool for a handler that needs no state. Conform to this instead when +/// the handler has dependencies: they become stored properties, and the request +/// methods become ordinary instance methods rather than statics threading a context +/// parameter through every call. +/// +/// ## Lifetimes +/// +/// The server retains its handler for its lifetime, and the handler must not be the +/// object that owns the server: `owner → HTTPServer → handler → owner` is a cycle, +/// so the owner's `deinit` would never run and `stop()` would never be called. +/// +/// This protocol is deliberately **not** `AnyObject`-constrained, because the +/// straightforward way to avoid that is a `struct` handler holding the dependencies +/// it needs. A value type cannot participate in a reference cycle at all, so the +/// question doesn't arise. A `final class` conformer is fine too — just keep it a +/// leaf, the same discipline ``HTTPServerDelegate`` documents. +public protocol HTTPRequestHandler: Sendable { + /// The response for a request the server has parsed and authenticated. + /// + /// Called once per request, concurrently across connections — hence `Sendable`. + /// Cancellation is cooperative: the server cancels this task when the client + /// disconnects or the server stops, and discards whatever a cancelled task + /// returns, so check `Task.isCancelled` before any side effect you can't undo. + func handle(_ request: HTTPServer.Request) async -> HTTPResponse +} + +#endif // canImport(Network) diff --git a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift index ac05fbb01..0f0a56f54 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift @@ -277,6 +277,46 @@ public final class HTTPServer: Sendable { } } + /// Starts a server that serves requests from an ``HTTPRequestHandler`` object + /// rather than a closure. + /// + /// Everything else behaves identically — this forwards to the closure form. Reach + /// for it when the handler has dependencies to hold: a `struct` conformer stores + /// them and serves from instance methods, instead of statics threading a context + /// parameter through every call. See ``HTTPRequestHandler`` for the (short) + /// lifetime rules. + public static func start( + name: String, + port: UInt16? = nil, + listenOnAllInterfaces: Bool = false, + requiresAuthentication: Bool = true, + maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize, + maxConnections: Int = HTTPServer.defaultMaxConnections, + readTimeout: Duration = HTTPServer.defaultReadTimeout, + bodyReadTimeout: Duration? = nil, + idleTimeout: Duration = HTTPServer.defaultIdleTimeout, + startTimeout: Duration = HTTPServer.defaultStartTimeout, + cors: CORSPolicy = .none, + delegate: HTTPServerDelegate? = nil, + handler: some HTTPRequestHandler + ) async throws -> HTTPServer { + try await start( + name: name, + port: port, + listenOnAllInterfaces: listenOnAllInterfaces, + requiresAuthentication: requiresAuthentication, + maxRequestBodySize: maxRequestBodySize, + maxConnections: maxConnections, + readTimeout: readTimeout, + bodyReadTimeout: bodyReadTimeout, + idleTimeout: idleTimeout, + startTimeout: startTimeout, + cors: cors, + delegate: delegate, + handler: { await handler.handle($0) } + ) + } + /// Races `operation` against `timeout`, throwing ``HTTPServerError/startTimeout`` /// if the timeout wins. Used to bound the wait for the listener to become ready /// so a caller — such as the editor load awaiting the upload server's bind — diff --git a/ios/Sources/GutenbergKitHTTP/README.md b/ios/Sources/GutenbergKitHTTP/README.md index 43721b305..42629aec0 100644 --- a/ios/Sources/GutenbergKitHTTP/README.md +++ b/ios/Sources/GutenbergKitHTTP/README.md @@ -46,6 +46,24 @@ server.stop() Pass `nil` (or omit `port`) to let the system assign an available port — useful for tests or when running multiple servers. +#### Handlers with state + +A closure is right for a handler that needs no state. When the handler has dependencies, conform a type to `HTTPRequestHandler` and pass it as `handler:` instead — the dependencies become stored properties and the request logic becomes instance methods, rather than statics threading a context parameter through every call. + +```swift +struct MediaHandler: HTTPRequestHandler { + let uploader: Uploader + + func handle(_ request: HTTPServer.Request) async -> HTTPResponse { + await uploader.upload(request.parsed.body) + } +} + +let server = try await HTTPServer.start(name: "media", handler: MediaHandler(uploader: uploader)) +``` + +The server retains its handler, so the handler must not be the object that owns the server — `owner → HTTPServer → handler → owner` is a cycle, and the owner's `deinit` would never run. `HTTPRequestHandler` is deliberately not `AnyObject`-constrained so a `struct` conformer sidesteps this entirely; a `final class` works too, as long as it stays a leaf. + When `requiresAuthentication` is enabled (the default), each request must include a `Proxy-Authorization: Bearer ` header carrying the server's randomly-generated token. The server uses `Proxy-Authorization` per RFC 9110 §11.7.1 rather than `Authorization`, so the client's `Authorization` header remains available for upstream credentials (e.g. HTTP Basic auth to the remote server). Unauthenticated requests receive a `407 Proxy Authentication Required` response with a `Proxy-Authenticate: Bearer` challenge header. ### Proxying via URLSession diff --git a/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift b/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift index 17be7d603..dda06a80e 100644 --- a/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift +++ b/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift @@ -38,6 +38,35 @@ struct HTTPServerStartTests { // rather than suspending its caller indefinitely. #expect(elapsed < .seconds(5)) } + + @Test("serves requests from an HTTPRequestHandler object, carrying its state") + func servesFromRequestHandlerObject() async throws { + // The point of the object overload: the handler holds its dependencies as + // stored properties and serves from an instance method, so a consumer with + // state doesn't need statics threading a context through every call. + let server = try await HTTPServer.start( + name: "handler-object-test", + requiresAuthentication: false, + handler: EchoHandler(greeting: "hello from a struct") + ) + defer { server.stop() } + + let url = URL(string: "http://127.0.0.1:\(server.port)/anything")! + let (data, response) = try await URLSession.shared.data(from: url) + + #expect((response as? HTTPURLResponse)?.statusCode == 200) + #expect(String(decoding: data, as: UTF8.self) == "hello from a struct") + } +} + +/// A value-type handler — it cannot form a reference cycle back to whatever owns +/// the server, which is why ``HTTPRequestHandler`` isn't `AnyObject`-constrained. +private struct EchoHandler: HTTPRequestHandler { + let greeting: String + + func handle(_ request: HTTPServer.Request) async -> HTTPResponse { + HTTPResponse(status: 200, body: Data(greeting.utf8)) + } } #endif From 380c42d66e41f24de5bacf5e45ee781b407ec214 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:08:42 -0600 Subject: [PATCH 17/21] refactor: serve media uploads from a handler object, not statics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every request function was `private static` taking an UploadContext, for one reason: the handler closure has to exist before MediaUploadServer does, so it couldn't capture `self`, and capturing it later would form `MediaUploadServer -> HTTPServer -> handler -> MediaUploadServer` and stop `deinit` from ever running `stop()`. Move them onto a `Handler` struct conforming to the HTTPRequestHandler protocol added in the previous commit. The dependencies become stored properties, so the five request functions become instance methods and drop their context parameter; UploadContext is deleted, since the handler now *is* the context. A struct can't participate in a reference cycle, so the constraint that forced the statics is gone rather than worked around. The statics that remain — errorResponse, relayResponse, attachmentId, formFields, sanitizeFilename, writeStream, cleanOrphanedUploads — are pure functions of their arguments. `static` there is not a workaround; it is the honest signal that they depend on nothing, which is now a meaningful distinction rather than an artifact of the closure. No behaviour change and no test changes: the only entry point is `MediaUploadServer.start`, whose signature is untouched. Reviewing with `--color-moved` will help — most of the diff is the request block moving into the struct and gaining a level of indentation. --- .../Sources/Media/MediaUploadServer.swift | 570 +++++++++--------- 1 file changed, 282 insertions(+), 288 deletions(-) diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index a711c1278..58bbc56f4 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -48,7 +48,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(processor: processor, uploader: uploader, internalClient: internalClient) + let handler = Handler(processor: processor, uploader: uploader, internalClient: internalClient) // A generous ceiling for receiving the upload body. The body read is // primarily bounded by the per-read idle timeout (which reaps a stalled @@ -65,9 +65,7 @@ final class MediaUploadServer: Sendable { bodyReadTimeout: bodyReadTimeout, cors: .permissive, delegate: ServerDelegate(), - handler: { request in - await Self.handleRequest(request, context: context) - } + handler: handler ) return MediaUploadServer(server: server, cleanupTask: cleanupTask) @@ -87,297 +85,320 @@ final class MediaUploadServer: Sendable { // MARK: - Request Handling - private static func handleRequest(_ request: HTTPServer.Request, context: UploadContext) async -> HTTPResponse { - let parsed = request.parsed + /// Serves each upload request, holding the media collaborators it needs. + /// + /// A value type, and deliberately separate from ``MediaUploadServer``: the handler + /// is captured by the `HTTPServer` that the server itself owns, so a handler that + /// *was* the server would form `MediaUploadServer -> HTTPServer -> handler -> + /// MediaUploadServer` and `deinit` would never run `stop()`. A struct can't + /// participate in a reference cycle, so that question doesn't arise — and its + /// dependencies are stored properties, so the request methods are ordinary + /// instance methods rather than statics threading a context through every call. + /// + /// Everything is held strongly. `EditorViewController` owns `mediaProcessor` / + /// `mediaUploader` strongly too, so a host object retaining the view controller + /// already cycles through the view controller's own properties — something this + /// handler can neither create nor prevent. Holding weak here bought no leak + /// protection, only the risk of a reference vanishing mid-request. Immutable + /// strong references also make the admission gate and delivery agree by + /// construction. (Matches Android, which holds plain `val`s.) + private struct Handler: HTTPRequestHandler { + let processor: (any MediaProcessor)? + let uploader: (any MediaUploader)? + let internalClient: InternalMediaClient + + func handle(_ request: HTTPServer.Request) async -> HTTPResponse { + let parsed = request.parsed + + // Routes: POST /upload, and DELETE /media/ for the editor's orphan + // cleanup. (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`, `?force=true`) relayed to WordPress. + let method = parsed.method.uppercased() + + if method == "POST", parsed.path == "/upload" { + return await handleUpload(request) + } - // Routes: POST /upload, and DELETE /media/ for the editor's orphan - // cleanup. (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`, `?force=true`) relayed to WordPress. - let method = parsed.method.uppercased() + if method == "DELETE", let attachmentId = Self.attachmentId(fromPath: parsed.path) { + return await handleMediaDelete(attachmentId, query: parsed.query) + } - if method == "POST", parsed.path == "/upload" { - return await handleUpload(request, context: context) + return MediaUploadServer.errorResponse(status: 404, message: "Not found") } - if method == "DELETE", let attachmentId = attachmentId(fromPath: parsed.path) { - return await handleMediaDelete(attachmentId, query: parsed.query, internalClient: context.internalClient) - } + private func handleUpload(_ request: HTTPServer.Request) async -> HTTPResponse { + let parts: [MultipartPart] + do { + parts = try request.parsed.multipartParts() + } catch { + Logger.uploadServer.error("Multipart parse failed: \(error)") + return MediaUploadServer.errorResponse(status: 400, message: "Expected multipart/form-data") + } - return errorResponse(status: 404, message: "Not found") - } + // Find the file part (the first part with a filename). + guard let filePart = parts.first(where: { $0.filename != nil }) else { + return MediaUploadServer.errorResponse(status: 400, message: "No file found in request") + } - private static func handleUpload(_ request: HTTPServer.Request, context: UploadContext) async -> HTTPResponse { - let parts: [MultipartPart] - do { - parts = try request.parsed.multipartParts() - } catch { - Logger.uploadServer.error("Multipart parse failed: \(error)") - return errorResponse(status: 400, message: "Expected multipart/form-data") - } + // 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 + + let filename = filePart.filename ?? "upload" + let mimeType = filePart.contentType + + // Materialize a temp file only if someone will touch it: a processor that + // claims this file, or an uploader (which always delivers the file itself). + // If GutenbergKit will deliver (no uploader) and no processor wants the + // file, forward the original request body directly, skipping a temp copy of + // a file nobody will process (e.g. a video handed to an image-only processor). + let processorWantsFile = processor?.handlesFile(ofType: mimeType, named: filename) ?? false + if uploader == nil, !processorWantsFile { + do { + return try await passthroughResponse(request, query: query) + } catch { + return Self.uploadErrorResponse(error) + } + } - // Find the file part (the first part with a filename). - guard let filePart = parts.first(where: { $0.filename != nil }) else { - return errorResponse(status: 400, message: "No file found in request") - } + // The delegate wants the file. Stream the part body to a dedicated temp + // file for it — the library's RequestBody may be a byte-range slice of a + // larger temp file whose lifecycle is tied to ARC, so the delegate needs a + // standalone file that outlives the handler return. + let tempDir = MediaUploadServer.uploadsTempDirectory + try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) - // 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 - - let filename = filePart.filename ?? "upload" - let mimeType = filePart.contentType - - // Materialize a temp file only if someone will touch it: a processor that - // claims this file, or an uploader (which always delivers the file itself). - // If GutenbergKit will deliver (no uploader) and no processor wants the - // file, forward the original request body directly, skipping a temp copy of - // a file nobody will process (e.g. a video handed to an image-only processor). - let processorWantsFile = context.processor?.handlesFile(ofType: mimeType, named: filename) ?? false - if context.uploader == nil, !processorWantsFile { + let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(MediaUploadServer.sanitizeFilename(filename))") do { - return try await passthroughResponse(request, query: query, internalClient: context.internalClient) + let inputStream = try filePart.body.makeInputStream() + try MediaUploadServer.writeStream(inputStream, to: fileURL) } catch { - return uploadErrorResponse(error) + try? FileManager.default.removeItem(at: fileURL) + Logger.uploadServer.error("Failed to write upload to disk: \(error)") + return MediaUploadServer.errorResponse(status: 500, message: "Failed to save file") } - } - // The delegate wants the file. Stream the part body to a dedicated temp - // file for it — the library's RequestBody may be a byte-range slice of a - // larger temp file whose lifecycle is tied to ARC, so the delegate needs a - // standalone file that outlives the handler return. - let tempDir = uploadsTempDirectory - try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) - - let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(sanitizeFilename(filename))") - do { - 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, message: "Failed to save file") - } - - // 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) } + // 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: filename, - extraParts: extraParts, query: query, - processorWantsFile: processorWantsFile, context: context - ) - switch uploadResult { - case .uploaded(let uploaded): - Logger.uploadServer.debug("Uploaded file to WordPress") - return relayResponse(uploaded) - case .passthrough: - // Delegate didn't modify the file — forward the original request - // body to WordPress without re-encoding. - return try await passthroughResponse(request, query: query, internalClient: context.internalClient) + do { + let uploadResult = try await processAndUpload( + fileURL: fileURL, mimeType: mimeType, filename: filename, + extraParts: extraParts, query: query, + processorWantsFile: processorWantsFile + ) + switch uploadResult { + case .uploaded(let uploaded): + Logger.uploadServer.debug("Uploaded file to WordPress") + return Self.relayResponse(uploaded) + case .passthrough: + // Delegate didn't modify the file — forward the original request + // body to WordPress without re-encoding. + return try await passthroughResponse(request, query: query) + } + } catch { + return Self.uploadErrorResponse(error) } - } catch { - return uploadErrorResponse(error) } - } - /// Forwards the original request body to WordPress unchanged (no multipart - /// re-encoding) and relays the response. Used on the no-uploader path when the - /// processor won't touch the file — it declined by metadata (`handlesFile` - /// returned false) or `processFile` returned `.original`. - private static func passthroughResponse( - _ request: HTTPServer.Request, query: String, internalClient: InternalMediaClient - ) async throws -> HTTPResponse { - // As in `processAndUpload`: don't put bytes on the wire for a torn-down - // editor, regardless of whether the HTTP client honors cancellation. - try Task.checkCancellation() - - Logger.uploadServer.debug("Passthrough: forwarding original request body to WordPress") - guard let body = request.parsed.body, - let contentType = request.parsed.header("Content-Type") else { - return errorResponse(status: 500, message: "Passthrough upload requires a request body and Content-Type") + /// Forwards the original request body to WordPress unchanged (no multipart + /// re-encoding) and relays the response. Used on the no-uploader path when the + /// processor won't touch the file — it declined by metadata (`handlesFile` + /// returned false) or `processFile` returned `.original`. + private func passthroughResponse( + _ request: HTTPServer.Request, query: String + ) async throws -> HTTPResponse { + // As in `processAndUpload`: don't put bytes on the wire for a torn-down + // editor, regardless of whether the HTTP client honors cancellation. + try Task.checkCancellation() + + Logger.uploadServer.debug("Passthrough: forwarding original request body to WordPress") + guard let body = request.parsed.body, + let contentType = request.parsed.header("Content-Type") else { + return MediaUploadServer.errorResponse(status: 500, message: "Passthrough upload requires a request body and Content-Type") + } + let response = try await internalClient.passthroughUpload(body: body, contentType: contentType, query: query) + return Self.relayResponse(response) } - let response = try await internalClient.passthroughUpload(body: body, contentType: contentType, query: query) - return relayResponse(response) - } - - /// The attachment ID in a `/media/` path, or `nil` if the path is not one. - /// - /// Deliberately narrow: this server relays media operations, not arbitrary - /// REST requests, so only a numeric attachment ID under `/media/` matches. - private static func attachmentId(fromPath path: String) -> String? { - let components = path.split(separator: "/", omittingEmptySubsequences: true) - guard components.count == 2, components[0] == "media" else { return nil } - let id = String(components[1]) - guard !id.isEmpty, id.allSatisfy(\.isNumber) else { return nil } - return id - } - /// Relays a media deletion. - /// - /// The editor deletes an attachment when the user removes it, and core deletes - /// an upload's orphan when every `post-process` retry fails. A cross-origin - /// editor cannot issue `DELETE` directly — api-fetch tunnels it as a `POST` - /// carrying `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the - /// browser blocks it at preflight; relaying it here lets the deletion run. - /// - /// Every attachment lives on the configured site — even one a host uploader - /// delivered — so its deletion is relayed to the internal media client there. See - /// the accepted-risk note in the body. - private static func handleMediaDelete( - _ attachmentId: String, query: String, internalClient: InternalMediaClient - ) async -> HTTPResponse { - do { - // Relay to the internal media client (the configured site) — every attachment - // lives there, even one a host uploader delivered. Core issues this only - // as orphan cleanup after failed recovery, but the relay can't tell that - // from any other DELETE the WebView sends: a compromised editor script - // holding the loopback token could force-delete arbitrary media on the - // configured site. Accepted risk — such a script already has broad write - // access, and a server-side compromise (a malicious plugin) deletes media - // directly without the editor, so scoping this with a per-session ledger - // buys little for the cost. - let response = try await internalClient.deleteMedia(attachmentId: attachmentId, query: query) - return relayResponse(response) - } catch { - return uploadErrorResponse(error) + /// The attachment ID in a `/media/` path, or `nil` if the path is not one. + /// + /// Deliberately narrow: this server relays media operations, not arbitrary + /// REST requests, so only a numeric attachment ID under `/media/` matches. + private static func attachmentId(fromPath path: String) -> String? { + let components = path.split(separator: "/", omittingEmptySubsequences: true) + guard components.count == 2, components[0] == "media" else { return nil } + let id = String(components[1]) + guard !id.isEmpty, id.allSatisfy(\.isNumber) else { return nil } + return id } - } - /// Relays WordPress's exact status, body, and relayable headers to the editor - /// so it sees the same attachment object (or error) as a direct upload. - /// - /// The headers matter for recovery: `x-wp-upload-attachment-id` is what lets - /// the editor retry `post-process` for an upload whose metadata generation - /// fataled server-side, rather than surfacing a permanent failure and - /// leaving an orphaned attachment behind. - /// - /// The relayed body is always WordPress REST JSON — an attachment, or a - /// `{code, message, data}` error — so the response is always `application/json`. - /// The relayed headers are a content-type-free allowlist (`relayableHeaderNames`), - /// so prepending the JSON default never collides with them. - private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { - return HTTPResponse( - status: response.statusCode, - headers: [("Content-Type", "application/json")] + response.headers.map { ($0.key, $0.value) }, - body: response.body - ) - } + /// Relays a media deletion. + /// + /// The editor deletes an attachment when the user removes it, and core deletes + /// an upload's orphan when every `post-process` retry fails. A cross-origin + /// editor cannot issue `DELETE` directly — api-fetch tunnels it as a `POST` + /// carrying `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the + /// browser blocks it at preflight; relaying it here lets the deletion run. + /// + /// Every attachment lives on the configured site — even one a host uploader + /// delivered — so its deletion is relayed to the internal media client there. See + /// the accepted-risk note in the body. + private func handleMediaDelete( + _ attachmentId: String, query: String + ) async -> HTTPResponse { + do { + // Relay to the internal media client (the configured site) — every attachment + // lives there, even one a host uploader delivered. Core issues this only + // as orphan cleanup after failed recovery, but the relay can't tell that + // from any other DELETE the WebView sends: a compromised editor script + // holding the loopback token could force-delete arbitrary media on the + // configured site. Accepted risk — such a script already has broad write + // access, and a server-side compromise (a malicious plugin) deletes media + // directly without the editor, so scoping this with a per-session ledger + // buys little for the cost. + let response = try await internalClient.deleteMedia(attachmentId: attachmentId, query: query) + return Self.relayResponse(response) + } catch { + return Self.uploadErrorResponse(error) + } + } - /// Builds the 500 response for a failed upload. A cancelled connection task - /// (editor abort / server stop) surfaces here too — as CancellationError or - /// URLError.cancelled — but isn't a failure and the server closes the - /// connection without sending this response (see HTTPServer's cancellation - /// check), so log that quietly. - private static func uploadErrorResponse(_ error: any Error) -> HTTPResponse { - if Task.isCancelled { - Logger.uploadServer.debug("Upload cancelled") - } else { - Logger.uploadServer.error("Upload processing failed: \(error)") + /// Relays WordPress's exact status, body, and relayable headers to the editor + /// so it sees the same attachment object (or error) as a direct upload. + /// + /// The headers matter for recovery: `x-wp-upload-attachment-id` is what lets + /// the editor retry `post-process` for an upload whose metadata generation + /// fataled server-side, rather than surfacing a permanent failure and + /// leaving an orphaned attachment behind. + /// + /// The relayed body is always WordPress REST JSON — an attachment, or a + /// `{code, message, data}` error — so the response is always `application/json`. + /// The relayed headers are a content-type-free allowlist (`relayableHeaderNames`), + /// so prepending the JSON default never collides with them. + private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { + return HTTPResponse( + status: response.statusCode, + headers: [("Content-Type", "application/json")] + response.headers.map { ($0.key, $0.value) }, + body: response.body + ) } - return errorResponse(status: 500, message: error.localizedDescription) - } - // MARK: - Delegate Pipeline + /// Builds the 500 response for a failed upload. A cancelled connection task + /// (editor abort / server stop) surfaces here too — as CancellationError or + /// URLError.cancelled — but isn't a failure and the server closes the + /// connection without sending this response (see HTTPServer's cancellation + /// check), so log that quietly. + private static func uploadErrorResponse(_ error: any Error) -> HTTPResponse { + if Task.isCancelled { + Logger.uploadServer.debug("Upload cancelled") + } else { + Logger.uploadServer.error("Upload processing failed: \(error)") + } + return MediaUploadServer.errorResponse(status: 500, message: error.localizedDescription) + } - /// Result of the process + deliver pipeline. - private enum UploadResult { - /// The uploader or internal media client completed the upload; carries the - /// response to relay. - case uploaded(MediaUploadResponse) - /// No uploader is set and the processor left the file unmodified, so the - /// caller should forward the original request body to the configured site. - case passthrough - } + // MARK: - Delegate Pipeline - private static func processAndUpload( - fileURL: URL, mimeType: String, filename: String, - extraParts: [MultipartPart], query: String, - processorWantsFile: Bool, context: UploadContext - ) async throws -> UploadResult { - // Step 1: transform (resize, transcode, …) if a processor claims the file. - // Reuse the gate's `handlesFile` decision from `handleUpload` rather than - // asking again — one metadata call per upload, and the admit and transform - // steps can't disagree. - let processed: ProcessedProxyFile - if processorWantsFile, let processor = context.processor { - processed = try await processor.processFile(at: fileURL, mimeType: mimeType, filename: filename) - } else { - processed = .original + /// Result of the process + deliver pipeline. + private enum UploadResult { + /// The uploader or internal media client completed the upload; carries the + /// response to relay. + case uploaded(MediaUploadResponse) + /// No uploader is set and the processor left the file unmodified, so the + /// caller should forward the original request body to the configured site. + case passthrough } - // Resolve the file to upload and its metadata. `.processed` uses the - // processor'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 - } + private func processAndUpload( + fileURL: URL, mimeType: String, filename: String, + extraParts: [MultipartPart], query: String, + processorWantsFile: Bool + ) async throws -> UploadResult { + // Step 1: transform (resize, transcode, …) if a processor claims the file. + // Reuse the gate's `handlesFile` decision from `handleUpload` rather than + // asking again — one metadata call per upload, and the admit and transform + // steps can't disagree. + let processed: ProcessedProxyFile + if processorWantsFile, let processor { + processed = try await processor.processFile(at: fileURL, mimeType: mimeType, filename: filename) + } else { + processed = .original + } - // The processed file (if the processor 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) + // Resolve the file to upload and its metadata. `.processed` uses the + // processor'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 editor was torn down (or the client disconnected) while we processed. - // Don't start an outbound upload whose response nobody will read — it would - // create an attachment neither GutenbergKit nor the host knows to clean up. - // Checking here rather than relying on the HTTP client to notice cancellation - // keeps this true for a host-injected `URLSessionProtocol` that doesn't. - try Task.checkCancellation() - - // Step 2: deliver. An uploader owns delivery on the host's own stack and - // returns the finished attachment JSON (or throws); GutenbergKit relays that - // as a success and never runs its own recovery behind it. Otherwise the - // internal media client delivers to the configured site. - if let uploader = context.uploader { - // Hand the host the editor's non-file fields (e.g. `post`) and query too, - // so its own POST can reproduce a native upload — otherwise the attachment - // is created unattached and `?_embed` is lost. - let fields = try await formFields(from: extraParts) - let upload = MediaUpload( - fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, - fields: fields, query: query - ) - let body = try await uploader.upload(upload) - return .uploaded(MediaUploadResponse(statusCode: 201, body: body)) - } else { - // Unmodified — forward the original request body directly, skipping - // multipart re-encoding. - if case .original = processed { - return .passthrough + // The processed file (if the processor 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) + } + } + + // The editor was torn down (or the client disconnected) while we processed. + // Don't start an outbound upload whose response nobody will read — it would + // create an attachment neither GutenbergKit nor the host knows to clean up. + // Checking here rather than relying on the HTTP client to notice cancellation + // keeps this true for a host-injected `URLSessionProtocol` that doesn't. + try Task.checkCancellation() + + // Step 2: deliver. An uploader owns delivery on the host's own stack and + // returns the finished attachment JSON (or throws); GutenbergKit relays that + // as a success and never runs its own recovery behind it. Otherwise the + // internal media client delivers to the configured site. + if let uploader { + // Hand the host the editor's non-file fields (e.g. `post`) and query too, + // so its own POST can reproduce a native upload — otherwise the attachment + // is created unattached and `?_embed` is lost. + let fields = try await Self.formFields(from: extraParts) + let upload = MediaUpload( + fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, + fields: fields, query: query + ) + let body = try await uploader.upload(upload) + return .uploaded(MediaUploadResponse(statusCode: 201, body: body)) + } else { + // Unmodified — forward the original request body directly, skipping + // multipart re-encoding. + if case .original = processed { + return .passthrough + } + let result = try await internalClient.upload(fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, extraParts: extraParts, query: query) + return .uploaded(result) } - let result = try await context.internalClient.upload(fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, extraParts: extraParts, query: query) - return .uploaded(result) } - } - /// Decodes the editor's non-file form parts (e.g. `post`, additionalData) into an - /// ordered list of name/value pairs for a host uploader, so it can send them on its - /// own `POST /wp/v2/media`. A list, not a dictionary, so repeated field names survive - /// in order. Values are WordPress form fields — UTF-8 text. - private static func formFields(from parts: [MultipartPart]) async throws -> [(name: String, value: String)] { - var fields: [(name: String, value: String)] = [] - for part in parts { - fields.append((name: part.name, value: String(decoding: try await part.body.data, as: UTF8.self))) + /// Decodes the editor's non-file form parts (e.g. `post`, additionalData) into an + /// ordered list of name/value pairs for a host uploader, so it can send them on its + /// own `POST /wp/v2/media`. A list, not a dictionary, so repeated field names survive + /// in order. Values are WordPress form fields — UTF-8 text. + private static func formFields(from parts: [MultipartPart]) async throws -> [(name: String, value: String)] { + var fields: [(name: String, value: String)] = [] + for part in parts { + fields.append((name: part.name, value: String(decoding: try await part.body.data, as: UTF8.self))) + } + return fields } - return fields } private static func errorResponse(status: Int, message: String) -> HTTPResponse { @@ -498,33 +519,6 @@ enum UploadError: Error, LocalizedError { } } -// MARK: - Upload Context - -/// Container for the media processor, uploader, and internal media client, captured by -/// the HTTPServer handler closure and read on each request. -/// -/// Everything here is held **strongly**, so a handler that admitted a file for -/// processing will process it, and one that gated on an uploader will deliver through -/// it — the reads can't disagree within a request, and an in-flight upload keeps the -/// host's handlers alive until it unwinds. This matches Android, which holds its -/// `processor`/`uploader` as plain `val`s for the same reason. -/// -/// Strong is safe because `EditorViewController` owns `mediaProcessor` / -/// `mediaUploader` strongly too. A host object that retains the view controller back -/// already forms `EditorViewController → mediaUploader → EditorViewController`, a -/// cycle this container can neither create nor prevent — so holding weak here bought -/// no leak protection, only the risk of a reference vanishing mid-request. (It *was* -/// load-bearing when the view controller held its delegate `weak` and this was the -/// only strong path; that changed when those properties became strong.) -/// -/// A `struct`, so it is implicitly `Sendable`: `MediaProcessor` and `MediaUploader` -/// are `Sendable` protocols and `InternalMediaClient` is `@unchecked Sendable`. -private struct UploadContext: Sendable { - let processor: (any MediaProcessor)? - let uploader: (any MediaUploader)? - let internalClient: InternalMediaClient -} - // MARK: - Default Media Uploader /// Uploads files to the WordPress REST API using site credentials from EditorConfiguration. From 8c59cebaee8407d302467373cf11b2450dc57679 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:13:23 -0600 Subject: [PATCH 18/21] fix: give a media upload's form fields a named type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MediaUpload.fields was `[(name: String, value: String)]` on iOS. Tuples are not nominal types, so a tuple-typed stored property permanently blocks synthesized Equatable, Hashable and Codable on MediaUpload — inside GutenbergKit as well as for hosts, and retroactively, so no later conformance can recover it. That matters for the offline queue the MediaUploader docs advertise as a motivating use case: a host that wants to persist a pending upload's fields has to hand-roll a mirror type. Unlike the missing public memberwise init on MediaUpload — which stays internal, matching how this library treats outbound types, and which could be added later without breaking anyone — this one is not fixable additively. Changing `fields` after release is a source break for every host, so it happens now or not at all. Introduce MediaUploadField (Sendable, Hashable, Codable, public init) and use it on both platforms. Android had no equivalent defect — Kotlin's Pair is nominal — but the same change lands there for parity, and `field.name`/`field.value` reads better than `first`/`second` in a host's upload code. Kotlin data class destructuring means the multipart writers are unchanged. MediaUpload itself is deliberately left non-Codable: it carries a `fileURL` pointing at a GutenbergKit temp file that will not exist after a relaunch, so a serialized MediaUpload would be a trap. A host queueing an upload should copy the bytes and persist the fields, which this type now supports. --- .../wordpress/gutenberg/MediaUploadServer.kt | 26 ++++++++++++++---- .../gutenberg/MediaUploadServerTest.kt | 18 ++++++++----- .../Sources/Media/MediaHandlers.swift | 27 ++++++++++++++++--- .../Sources/Media/MediaUploadServer.swift | 6 ++--- .../Media/MediaUploadServerTests.swift | 20 +++++++------- 5 files changed, 68 insertions(+), 29 deletions(-) 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 f02296fb1..a98ce667f 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -98,6 +98,22 @@ interface MediaProcessor { suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile = ProcessedProxyFile.Original } +/** + * One of the editor's non-file form fields, as sent with a media upload. + * + * A named type rather than a `Pair`, so the two platforms describe this the same way + * and reading a field says `name`/`value` rather than `first`/`second`. (On iOS the + * equivalent change is load-bearing: a tuple there would block Equatable/Hashable/ + * Codable synthesis on [MediaUpload] permanently.) + * + * @property name The field name, e.g. `post`. Not unique — a `field[]` array repeats it. + * @property value The field's value, decoded as UTF-8. + */ +data class MediaUploadField( + val name: String, + val value: String +) + /** * Everything a [MediaUploader] needs to reproduce a native upload: the file to send, * its metadata, the editor's non-file form fields, and the request's query. @@ -107,9 +123,9 @@ interface MediaProcessor { * @property filename The file's name. * @property fields The editor's non-file form fields, in order, each decoded as UTF-8 — * most importantly `post`, the parent post's ID, without which the attachment is - * created unattached. A list of `(name, value)` pairs, not a map, so repeated field - * names (e.g. a `field[]` array) survive verbatim. Send each as a form part on your - * `POST /wp/v2/media`, in the given order. + * created unattached. A list, not a map, so repeated field names (e.g. a `field[]` + * array) survive verbatim. Send each as a form part on your `POST /wp/v2/media`, + * in the given order. * @property query The request's query string (leading `?`, e.g. `?_embed=...`), or * empty. Carry it on your request so the editor gets the response it expects. */ @@ -117,7 +133,7 @@ data class MediaUpload( val file: File, val mimeType: String, val filename: String, - val fields: List>, + val fields: List, val query: String ) @@ -539,7 +555,7 @@ internal class MediaUploadServer( // too, so its own POST can reproduce a native upload — otherwise the // attachment is created unattached and `?_embed` is lost. val fields = extraParts.map { part -> - part.name to String(part.body.readBytes(), Charsets.UTF_8) + MediaUploadField(part.name, String(part.body.readBytes(), Charsets.UTF_8)) } val upload = MediaUpload(targetFile, targetMimeType, targetFilename, fields, query) return UploadResult.Uploaded(MediaUploadResponse(201, up.upload(upload))) 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 4ccc7e970..b5b812fde 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -256,7 +256,7 @@ class MediaUploadServerTest { val boundary = "test-boundary-123" val body = buildMultipartBody( boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray(), - fields = listOf("post" to "123") + fields = listOf(MediaUploadField("post", "123")) ) val response = sendRawRequest( @@ -275,7 +275,7 @@ class MediaUploadServerTest { assertEquals("photo.jpg", uploader.lastFilename) // The editor's post association and query must reach the host uploader, so it // can reproduce a native upload (attach to the post, honor ?_embed). - assertEquals("123", uploader.lastFields.first { it.first == "post" }.second) + assertEquals("123", uploader.lastFields.first { it.name == "post" }.value) assertEquals("?_embed=wp:featuredmedia", uploader.lastQuery) // …and the actual file bytes the editor sent — the host uploads them itself. assertEquals("fake image data", uploader.lastFileBytes?.decodeToString()) @@ -308,7 +308,11 @@ class MediaUploadServerTest { val boundary = "test-boundary-123" val body = buildMultipartBody( boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray(), - fields = listOf("post" to "123", "media_folder[]" to "12", "media_folder[]" to "45") + fields = listOf( + MediaUploadField("post", "123"), + MediaUploadField("media_folder[]", "12"), + MediaUploadField("media_folder[]", "45") + ) ) val response = sendRawRequest( @@ -326,9 +330,9 @@ class MediaUploadServerTest { // Both repeated values survive, in order — not collapsed to the last. assertEquals( listOf("12", "45"), - uploader.lastFields.filter { it.first == "media_folder[]" }.map { it.second } + uploader.lastFields.filter { it.name == "media_folder[]" }.map { it.value } ) - assertEquals("123", uploader.lastFields.first { it.first == "post" }.second) + assertEquals("123", uploader.lastFields.first { it.name == "post" }.value) } @Test @@ -904,7 +908,7 @@ class MediaUploadServerTest { filename: String, mimeType: String, data: ByteArray, - fields: List> = emptyList() + fields: List = emptyList() ): ByteArray { val out = java.io.ByteArrayOutputStream() for ((name, value) in fields) { @@ -935,7 +939,7 @@ class MediaUploadServerTest { @Volatile var uploadCalled = false @Volatile var lastMimeType: String? = null @Volatile var lastFilename: String? = null - @Volatile var lastFields: List> = emptyList() + @Volatile var lastFields: List = emptyList() @Volatile var lastQuery: String? = null @Volatile var lastFileBytes: ByteArray? = null diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift index c9aacc9b0..83d3a09c2 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift @@ -74,6 +74,25 @@ public protocol MediaProcessor: AnyObject, Sendable { func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile } +/// One of the editor's non-file form fields, as sent with a media upload. +/// +/// A named type rather than a `(name, value)` tuple: tuples are not nominal, so a +/// tuple-typed property would permanently block `Equatable`/`Hashable`/`Codable` +/// synthesis on ``MediaUpload`` — including inside GutenbergKit, and not fixable +/// later without a source break for every host. +public struct MediaUploadField: Sendable, Hashable, Codable { + /// The field name, e.g. `post`. Not unique — a `field[]` array repeats it. + public let name: String + + /// The field's value, decoded as UTF-8. + public let value: String + + public init(name: String, value: String) { + self.name = name + self.value = value + } +} + /// Everything a ``MediaUploader`` needs to reproduce a native upload: the file to /// send, its metadata, the editor's non-file form fields, and the request's query. public struct MediaUpload: Sendable { @@ -88,10 +107,10 @@ public struct MediaUpload: Sendable { /// The editor's non-file form fields, in order, each decoded as UTF-8 — most /// importantly `post`, the parent post's ID, without which the attachment is created - /// unattached. A list of `(name, value)` pairs, not a dictionary, so repeated field - /// names (e.g. a `field[]` array) survive verbatim. Send each as a form part on your - /// `POST /wp/v2/media`, in the given order. - public let fields: [(name: String, value: String)] + /// unattached. A list, not a dictionary, so repeated field names (e.g. a `field[]` + /// array) survive verbatim. Send each as a form part on your `POST /wp/v2/media`, + /// in the given order. + public let fields: [MediaUploadField] /// The request's query string (leading `?`, e.g. `?_embed=wp:featuredmedia`), or /// empty. Carry it on your request so the editor gets the response it expects. diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 58bbc56f4..968cf8567 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -392,10 +392,10 @@ final class MediaUploadServer: Sendable { /// ordered list of name/value pairs for a host uploader, so it can send them on its /// own `POST /wp/v2/media`. A list, not a dictionary, so repeated field names survive /// in order. Values are WordPress form fields — UTF-8 text. - private static func formFields(from parts: [MultipartPart]) async throws -> [(name: String, value: String)] { - var fields: [(name: String, value: String)] = [] + private static func formFields(from parts: [MultipartPart]) async throws -> [MediaUploadField] { + var fields: [MediaUploadField] = [] for part in parts { - fields.append((name: part.name, value: String(decoding: try await part.body.data, as: UTF8.self))) + fields.append(MediaUploadField(name: part.name, value: String(decoding: try await part.body.data, as: UTF8.self))) } return fields } diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 671312364..9d66e38c3 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -185,7 +185,7 @@ struct MediaUploadServerTests { let boundary = UUID().uuidString let body = buildMultipartBody( boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", - data: Data("fake image data".utf8), fields: [(name: "post", value: "123")] + data: Data("fake image data".utf8), fields: [MediaUploadField(name: "post", value: "123")] ) let url = URL(string: "http://127.0.0.1:\(server.port)/upload?_embed=wp:featuredmedia")! @@ -235,9 +235,9 @@ struct MediaUploadServerTests { boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8), fields: [ - (name: "post", value: "123"), - (name: "media_folder[]", value: "12"), - (name: "media_folder[]", value: "45"), + MediaUploadField(name: "post", value: "123"), + MediaUploadField(name: "media_folder[]", value: "12"), + MediaUploadField(name: "media_folder[]", value: "45"), ] ) @@ -593,12 +593,12 @@ struct MediaUploadServerTests { #expect(!mockClient.passthroughUploadCalled) } - private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data, fields: [(name: String, value: String)] = []) -> Data { + private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data, fields: [MediaUploadField] = []) -> Data { var body = Data() - for (name, value) in fields { + for field in fields { body.append("--\(boundary)\r\n") - body.append("Content-Disposition: form-data; name=\"\(name)\"\r\n\r\n") - body.append(value) + body.append("Content-Disposition: form-data; name=\"\(field.name)\"\r\n\r\n") + body.append(field.value) body.append("\r\n") } body.append("--\(boundary)\r\n") @@ -994,7 +994,7 @@ private final class MockUploader: MediaUploader, @unchecked Sendable { private var _uploadCalled = false private var _lastMimeType: String? private var _lastFilename: String? - private var _lastFields: [(name: String, value: String)] = [] + private var _lastFields: [MediaUploadField] = [] private var _lastQuery: String? private var _lastFileData: Data? private let uploadBody: Data @@ -1003,7 +1003,7 @@ private final class MockUploader: MediaUploader, @unchecked Sendable { var uploadCalled: Bool { lock.withLock { _uploadCalled } } var lastMimeType: String? { lock.withLock { _lastMimeType } } var lastFilename: String? { lock.withLock { _lastFilename } } - var lastFields: [(name: String, value: String)] { lock.withLock { _lastFields } } + var lastFields: [MediaUploadField] { lock.withLock { _lastFields } } var lastQuery: String? { lock.withLock { _lastQuery } } var lastFileData: Data? { lock.withLock { _lastFileData } } From 397a9766c2d47703cb126f398133a138a9b8ab67 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:18:33 -0600 Subject: [PATCH 19/21] fix: gate the media uploader trap on the site root too, on iOS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Android refused a mediaUploader when either `siteApiRoot` or `authHeader` was missing; iOS checked only `authHeader`. So a host with valid credentials but no site root crashed on Android and started a server on iOS — the same configuration, opposite outcomes, on a pair of fields that are both required for the internal media client to reach the configured site at all. Gate iOS on both. The types differ — `siteApiRoot` is a `URL` on iOS and a `String` on Android — so the equivalent of Android's `isEmpty()` is "not absolute": a URL with no scheme or host cannot address the site, and every media request built from it fails at the URLSession layer. Also covers the arm nothing tested. `GutenbergViewUploadServerTest` only exercised the missing-authHeader case; add its siteApiRoot sibling. There is no iOS equivalent because `precondition` takes the test process down, where Kotlin's `check` throws catchably. The iOS comment claimed every delete "would 500" without credentials. That is right for a missing site root, where okhttp/URLSession reject the schemeless URL, but a missing auth header relays WordPress's 401 instead. Say "would fail", which is true of both. --- .../GutenbergViewUploadServerTest.kt | 34 +++++++++++++++++++ .../Sources/EditorViewController.swift | 24 +++++++++---- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt index 8f673b592..b319c107d 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt @@ -147,6 +147,40 @@ class GutenbergViewUploadServerTest { } } + @Test + fun `an uploader without a site api root is a configuration error`() { + // The other arm of the same gate: an auth header is no use without a site to + // send it to. Both fields have to be present and usable for the internal media + // client to reach the configured site, so either one missing traps — iOS gates + // on the same pair. + val config = EditorConfiguration + .builder("https://example.com", "") + .setAuthHeader("Bearer token") + .build() // deliberately no site API root + val view = GutenbergView( + config, + EditorDependencies.empty, + testScope, + RuntimeEnvironment.getApplication() + ) + try { + view.mediaUploader = mock(MediaUploader::class.java) + val error = assertThrows(InvocationTargetException::class.java) { + startLoading(view) + } + assertTrue( + "an uploader without a site api root should fail with IllegalStateException", + error.cause is IllegalStateException + ) + assertNull( + "no server should be left running after the configuration error", + uploadServerOf(view) + ) + } finally { + detach(view) + } + } + @Test fun `detaching the view stops and clears the upload server`() { val view = makeView() diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 7fe9af377..b90062373 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -470,10 +470,13 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // An InternalMediaClient does two jobs: it delivers GutenbergKit-owned // uploads (when no `mediaUploader` is set), and it relays the editor's media // DELETEs to the configured site — every attachment lives there, even one a - // host uploader delivered, so that's where its deletion goes. It needs an auth - // header (the editor injects it because the WebView has no auth cookies). + // host uploader delivered, so that's where its deletion goes. It needs both a + // site API root to address and an auth header (the editor injects the latter + // because the WebView has no auth cookies); with either missing, every media + // request it makes fails. // - // Without one there's no internal media client, so the behavior forks by intent: + // Without them there's no usable internal media client, so the behavior forks + // by intent: // // - A `mediaProcessor` only enhances GutenbergKit-owned uploads. With no // credentials there's nothing to deliver through, so nothing to process — @@ -483,13 +486,20 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // would silently drop it, and its media deletes still need the default // uploader to reach the configured site. A host that sets an uploader must // provide credentials too; omitting them is a configuration error, so trap - // rather than start a server whose every delete would 500. - if configuration.authHeader.isEmpty { + // rather than start a server whose every delete would fail. + // + // `siteApiRoot` is a `URL` here where Android types it as a `String`, so the + // equivalent of Android's `isEmpty()` check is "not absolute" — a URL with no + // scheme or host can't address the site, and every request built from it fails + // at the URLSession layer. + let siteApiRootIsUsable = configuration.siteApiRoot.scheme != nil + && configuration.siteApiRoot.host() != nil + if !siteApiRootIsUsable || configuration.authHeader.isEmpty { precondition( mediaUploader == nil, "A mediaUploader needs site credentials so GutenbergKit can relay the " - + "editor's media deletes to the configured site. Set the auth header " - + "in the editor configuration." + + "editor's media deletes to the configured site. Set an absolute " + + "siteApiRoot and the auth header in the editor configuration." ) return } From 83d90776d2f4ce3d48e7bff97cbbbcd3e481152f Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:23:43 -0600 Subject: [PATCH 20/21] test: cover the media credentials trap with exit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit claimed this policy was untestable on iOS because `precondition` takes the test process down. That was wrong: Swift Testing has exit tests, which run the body in a child process. The real obstacle was narrower. Exit tests are unavailable on iOS and the simulator ("Exit tests are not available on this platform"), and the policy lived in EditorViewController, which is `#if canImport(UIKit)` and therefore absent from the macOS host — so the one platform that can run exit tests couldn't see the code. The intersection was empty because of where the code sat, not because of the tool. Move the decision into MediaServerCredentials, outside the UIKit gate, and have startUploadServer call it. The predicate and the fail-fast are now both reachable from the host suite: six tests pin the predicate (including the two arms of the site-root check that a `URL` makes different from Android's `String`), and two exit tests pin the trap itself. Neutering the precondition fails both, so they are not vacuous. This also gives the crash policy a named home. It diverged silently between iOS and Android once already; a host-testable predicate is harder to let drift again. --- .../Sources/EditorViewController.swift | 33 ++------- .../Media/MediaServerCredentials.swift | 55 ++++++++++++++ .../Media/MediaServerCredentialsTests.swift | 72 +++++++++++++++++++ 3 files changed, 134 insertions(+), 26 deletions(-) create mode 100644 ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift create mode 100644 ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index b90062373..cd9cc5e53 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -475,32 +475,13 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // because the WebView has no auth cookies); with either missing, every media // request it makes fails. // - // Without them there's no usable internal media client, so the behavior forks - // by intent: - // - // - A `mediaProcessor` only enhances GutenbergKit-owned uploads. With no - // credentials there's nothing to deliver through, so nothing to process — - // leave the server down and let uploads fall to the default WebView path. - // - // - A `mediaUploader` means the host is *taking over* uploads. Falling back - // would silently drop it, and its media deletes still need the default - // uploader to reach the configured site. A host that sets an uploader must - // provide credentials too; omitting them is a configuration error, so trap - // rather than start a server whose every delete would fail. - // - // `siteApiRoot` is a `URL` here where Android types it as a `String`, so the - // equivalent of Android's `isEmpty()` check is "not absolute" — a URL with no - // scheme or host can't address the site, and every request built from it fails - // at the URLSession layer. - let siteApiRootIsUsable = configuration.siteApiRoot.scheme != nil - && configuration.siteApiRoot.host() != nil - if !siteApiRootIsUsable || configuration.authHeader.isEmpty { - precondition( - mediaUploader == nil, - "A mediaUploader needs site credentials so GutenbergKit can relay the " - + "editor's media deletes to the configured site. Set an absolute " - + "siteApiRoot and the auth header in the editor configuration." - ) + // `MediaServerCredentials` owns that check and the fail-fast behind it, so the + // policy is reachable from the host test suite — this file is not. + guard MediaServerCredentials.canStartServer( + siteApiRoot: configuration.siteApiRoot, + authHeader: configuration.authHeader, + hasUploader: mediaUploader != nil + ) else { return } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift new file mode 100644 index 000000000..5ec9ee854 --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift @@ -0,0 +1,55 @@ +import Foundation + +/// Whether the editor configuration can reach the configured site for media, and the +/// fail-fast that enforces it. +/// +/// Deliberately outside `EditorViewController`. That type is `#if canImport(UIKit)`, +/// so on the macOS host it does not exist and nothing in it can be tested — including +/// this policy, which is a *crash* policy and already diverged silently between iOS +/// and Android once. Living here, it is reachable from the host test suite, where +/// Swift Testing's exit tests (unavailable on iOS/simulator) can assert the trap +/// itself rather than only the predicate. +enum MediaServerCredentials { + /// Whether an ``InternalMediaClient`` built from this configuration could actually + /// reach the site. + /// + /// Both fields are required. The client delivers GutenbergKit-owned uploads and + /// relays every media delete to the configured site, so it needs somewhere to send + /// them and credentials to be accepted; with either missing, every media request it + /// makes fails. + /// + /// `siteApiRoot` is a `URL` here where Android types it as a `String`, so the + /// equivalent of Android's `isEmpty()` check is "not absolute" — a URL with no + /// scheme or host cannot address the site, and every request built from it fails at + /// the URLSession layer. + static func areUsable(siteApiRoot: URL, authHeader: String) -> Bool { + siteApiRoot.scheme != nil && siteApiRoot.host() != nil && !authHeader.isEmpty + } + + /// Returns whether the upload server can start, trapping if the host set a + /// ``MediaUploader`` without usable credentials. + /// + /// The behavior forks by intent: + /// + /// - A ``MediaProcessor`` only enhances GutenbergKit-owned uploads. With no + /// credentials there is nothing to deliver through, so nothing to process — the + /// caller leaves the server down and uploads fall to the default WebView path. + /// + /// - A ``MediaUploader`` means the host is *taking over* uploads. Falling back would + /// silently drop it, and its media deletes still need the internal media client to + /// reach the configured site. A host that sets an uploader must provide credentials + /// too; omitting them is a configuration error, so trap rather than start a server + /// whose every delete would fail. (Matches Android's `check`.) + static func canStartServer(siteApiRoot: URL, authHeader: String, hasUploader: Bool) -> Bool { + if areUsable(siteApiRoot: siteApiRoot, authHeader: authHeader) { + return true + } + precondition( + !hasUploader, + "A mediaUploader needs site credentials so GutenbergKit can relay the " + + "editor's media deletes to the configured site. Set an absolute " + + "siteApiRoot and the auth header in the editor configuration." + ) + return false + } +} diff --git a/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift new file mode 100644 index 000000000..18d51ba82 --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift @@ -0,0 +1,72 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +/// The upload server's start policy: both a site API root and an auth header are +/// required, and a `mediaUploader` set without them is a configuration error rather +/// than a silent fallback. +/// +/// These run on the **host**, which is the point of `MediaServerCredentials` being +/// outside `EditorViewController` — that type is UIKit-gated and so untestable here, +/// and Swift Testing's exit tests (which is how the trap below is asserted) are +/// unavailable on iOS and the simulator. The Android counterparts live in +/// `GutenbergViewUploadServerTest`. +@Suite("Media server credentials") +struct MediaServerCredentialsTests { + static let apiRoot = URL(string: "https://example.com/wp-json/")! + + @Test("both a site API root and an auth header are usable") + func bothPresentIsUsable() { + #expect(MediaServerCredentials.areUsable(siteApiRoot: Self.apiRoot, authHeader: "Bearer t")) + } + + @Test("an empty auth header is not usable") + func emptyAuthHeaderIsNotUsable() { + #expect(!MediaServerCredentials.areUsable(siteApiRoot: Self.apiRoot, authHeader: "")) + } + + @Test("a relative site API root is not usable") + func relativeSiteApiRootIsNotUsable() { + // The iOS analogue of Android's `siteApiRoot.isEmpty()`. A host can't pass "", + // because the type is `URL` — but it can pass one with no scheme or host, which + // is just as unusable: every request built from it fails at the URLSession layer. + #expect(!MediaServerCredentials.areUsable(siteApiRoot: URL(string: "/wp-json/")!, authHeader: "Bearer t")) + } + + @Test("a scheme without a host is not usable") + func schemeWithoutHostIsNotUsable() { + #expect(!MediaServerCredentials.areUsable(siteApiRoot: URL(string: "https:///wp-json/")!, authHeader: "Bearer t")) + } + + @Test("a processor without credentials leaves the server down rather than trapping") + func processorWithoutCredentialsDoesNotTrap() { + // The other half of the fork: a processor only enhances GutenbergKit-owned + // uploads, so with nothing to deliver through there's nothing to process. The + // caller leaves the server down and uploads fall to the default WebView path. + #expect(!MediaServerCredentials.canStartServer(siteApiRoot: Self.apiRoot, authHeader: "", hasUploader: false)) + } + + @Test("credentials present means the server can start") + func credentialsPresentCanStart() { + #expect(MediaServerCredentials.canStartServer(siteApiRoot: Self.apiRoot, authHeader: "Bearer t", hasUploader: true)) + } + + @Test("an uploader without an auth header traps") + func uploaderWithoutAuthHeaderTraps() async { + await #expect(processExitsWith: .failure) { + _ = MediaServerCredentials.canStartServer( + siteApiRoot: MediaServerCredentialsTests.apiRoot, authHeader: "", hasUploader: true + ) + } + } + + @Test("an uploader without a usable site API root traps") + func uploaderWithoutSiteApiRootTraps() async { + await #expect(processExitsWith: .failure) { + _ = MediaServerCredentials.canStartServer( + siteApiRoot: URL(string: "/wp-json/")!, authHeader: "Bearer t", hasUploader: true + ) + } + } +} From 2bbc65d1bf5f9eb9309258f500341d8f6809ff1b Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:36:14 -0600 Subject: [PATCH 21/21] docs: state the invariant that makes the media field decode safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `formFields` decodes each non-file form value as UTF-8, which substitutes U+FFFD on malformed input. That is lossless today, but only because of an invariant nothing in the code states or enforces: the sole client is the editor's browser FormData. The server binds to loopback behind a per-session token; a FormData string value is a USVString, already well-formed at append time; and its only way to carry arbitrary bytes is a Blob, which always gets a filename and is filtered out of `extraParts`. Write that down on both platforms, including the part that makes it matter — if it stops holding, the two platforms are lossy *differently* (for ED A0 80, Swift's maximal-subpart rule yields three replacement characters where Java's decoder yields one), so there is no single behaviour that could be documented instead. Also reword the raw-bytes comment on the re-encode path. "So a non-UTF-8 value is forwarded verbatim" read as though malformed values were expected, which made the two delivery paths look contradictory. The actual hazard is the failable `String(data:encoding:)` returning nil and an obvious `?? ""` dropping the whole value; the reason to keep bytes is that the re-encode should stay byte-identical to the passthrough it stands in for. Cover the partition rather than the decode, since the partition is what makes the invariant true: a request carrying a second, Blob-shaped part whose bytes are not valid UTF-8 must not surface that part in `fields`. Both tests fail when the filename filter is relaxed, so neither is vacuous. Neither asserts what becomes of that second part — it is currently dropped rather than relayed, which is a separate open question. --- .../wordpress/gutenberg/MediaUploadServer.kt | 17 ++++- .../gutenberg/MediaUploadServerTest.kt | 63 +++++++++++++++++++ .../Sources/Media/MediaUploadServer.swift | 30 +++++++-- .../Media/MediaUploadServerTests.swift | 52 +++++++++++++++ 4 files changed, 156 insertions(+), 6 deletions(-) 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 a98ce667f..08e01630c 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -554,6 +554,16 @@ internal class MediaUploadServer( // Hand the host the editor's non-file fields (e.g. `post`) and query // too, so its own POST can reproduce a native upload — otherwise the // attachment is created unattached and `?_embed` is lost. + // + // The UTF-8 decode is lossless because of an invariant nothing enforces: + // the only client is the editor's browser FormData. The server binds to + // loopback behind a per-session token; a FormData string value is a + // USVString, already well-formed at append time; and its only way to carry + // arbitrary bytes is a Blob, which always gets a filename and so is + // filtered out of extraParts above. Valid UTF-8 — emoji, any script — + // round-trips exactly. If that stops holding this silently substitutes + // U+FFFD, and differently from iOS (Java reports one replacement char for + // ED A0 80 where Swift's maximal-subpart rule reports three). val fields = extraParts.map { part -> MediaUploadField(part.name, String(part.body.readBytes(), Charsets.UTF_8)) } @@ -663,8 +673,11 @@ internal open class InternalMediaClient( val mediaType = mimeType.toMediaType() 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 + // Each field's raw bytes are appended rather than round-tripped through String + // — not because malformed values are expected (they can't reach here; see the + // invariant where `fields` is built), but so this re-encode stays byte-identical + // to the passthrough it stands in for: a user's upload shouldn't change shape + // just because a processor resized the image. 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()) 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 b5b812fde..8da157973 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -335,6 +335,69 @@ class MediaUploadServerTest { assertEquals("123", uploader.lastFields.first { it.name == "post" }.value) } + @Test + fun `keeps a filename-bearing part out of the fields handed to an uploader`() { + // Pins the invariant that makes the UTF-8 decode of `fields` lossless. A browser + // FormData can only carry arbitrary bytes as a Blob, and a Blob always gets a + // filename, so the partition on `filename == null` is what keeps binary out of + // `fields`. Change it and the decode silently substitutes U+FFFD — and does so + // differently from iOS. + // + // Deliberately not asserted: what becomes of the second filename-bearing part. + // It is currently dropped rather than relayed, which is a separate open question. + val uploader = MockUploader() + val internalClient = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + processor = null, + uploader = uploader, + internalClient = internalClient, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-123" + val out = java.io.ByteArrayOutputStream() + // A plain field — no filename, so it belongs in `fields`. + out.write("--$boundary\r\n".toByteArray()) + out.write("Content-Disposition: form-data; name=\"post\"\r\n\r\n".toByteArray()) + out.write("123".toByteArray()) + out.write("\r\n".toByteArray()) + // The file. + out.write("--$boundary\r\n".toByteArray()) + out.write("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n".toByteArray()) + out.write("Content-Type: image/jpeg\r\n\r\n".toByteArray()) + out.write("fake image data".toByteArray()) + out.write("\r\n".toByteArray()) + // A Blob-shaped sidecar: filename present, bytes not valid UTF-8. If the + // partition admitted this to `fields`, the lone 0xFF would become U+FFFD. + out.write("--$boundary\r\n".toByteArray()) + out.write("Content-Disposition: form-data; name=\"sidecar\"; filename=\"blob\"\r\n".toByteArray()) + out.write("Content-Type: application/octet-stream\r\n\r\n".toByteArray()) + out.write(byteArrayOf(0x61, 0xFF.toByte(), 0x62)) + out.write("\r\n--$boundary--\r\n".toByteArray()) + + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = out.toByteArray() + ) + + assertTrue(uploader.uploadCalled) + assertEquals( + "only filename-less parts belong in fields", + listOf("post"), + uploader.lastFields.map { it.name } + ) + assertTrue( + "no field value should have been lossily decoded", + uploader.lastFields.none { it.value.contains('�') } + ) + } + @Test fun `hands the processed file and its new metadata to the uploader`() { // A processor transcodes the file; the host uploader must receive the processed diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 968cf8567..b0ac09f42 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -391,7 +391,22 @@ final class MediaUploadServer: Sendable { /// Decodes the editor's non-file form parts (e.g. `post`, additionalData) into an /// ordered list of name/value pairs for a host uploader, so it can send them on its /// own `POST /wp/v2/media`. A list, not a dictionary, so repeated field names survive - /// in order. Values are WordPress form fields — UTF-8 text. + /// in order. + /// + /// The UTF-8 decode is lossless here because of an invariant worth stating, since + /// nothing in the type system enforces it: **the only client is the editor's + /// browser `FormData`.** The server binds to loopback behind a per-session token, + /// so nothing else can reach it; a `FormData` string value is a `USVString`, which + /// the browser has already made well-formed at `append` time; and its only way to + /// carry arbitrary bytes is a Blob, which always gets a filename and is therefore + /// filtered out of `extraParts` by `handleUpload`. Valid UTF-8 — including emoji + /// and any non-Latin script — round-trips exactly, so real captions and titles are + /// unaffected. + /// + /// If that ever stops holding, this decode starts substituting U+FFFD *and* the two + /// platforms disagree about how: for `ED A0 80`, Swift's maximal-subpart rule yields + /// three replacement characters where Java's decoder yields one. `MediaUploadServerTests` + /// pins the partition that keeps binary parts out of here. private static func formFields(from parts: [MultipartPart]) async throws -> [MediaUploadField] { var fields: [MediaUploadField] = [] for part in parts { @@ -674,9 +689,16 @@ class InternalMediaClient: @unchecked Sendable { ) throws -> (InputStream, Int) { // 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. + // fine; `contentLength` counts them via `preamble.count`. + // + // Field values are appended as raw bytes rather than round-tripped through + // `String`. This is not because malformed values are expected — they can't + // reach here; see the invariant on `formFields`. It is because the failable + // `String(data:encoding:)` returns nil on invalid UTF-8, and the obvious + // `?? ""` behind it would silently drop a whole field's value. Appending the + // bytes keeps this re-encode byte-identical to the passthrough it stands in + // for, so a user's upload doesn't change shape just because a processor + // resized the image. var preamble = Data() for field in extraFields { preamble.append(Data("--\(boundary)\r\n".utf8)) diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 9d66e38c3..e6dc21cca 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -593,6 +593,58 @@ struct MediaUploadServerTests { #expect(!mockClient.passthroughUploadCalled) } + @Test("keeps a filename-bearing part out of the fields handed to an uploader") + func filenameBearingPartsNeverReachFields() async throws { + // This pins the invariant that makes the UTF-8 decode in `formFields` lossless. + // A browser FormData can only carry arbitrary bytes as a Blob, and a Blob always + // gets a filename, so `handleUpload`'s partition on `filename == nil` is what keeps + // binary out of `fields`. Change that partition and the decode silently starts + // substituting U+FFFD — differently on each platform. + // + // Deliberately *not* asserted: what becomes of the second filename-bearing part. + // It is currently dropped rather than relayed, which is a separate open question; + // this test is about what must never reach `fields`. + let uploader = MockUploader() + let mockClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: mockClient) + defer { server.stop() } + + let boundary = UUID().uuidString + var body = Data() + // A plain field — no filename, so it belongs in `fields`. + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"post\"\r\n\r\n") + body.append("123") + body.append("\r\n") + // The file. + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n") + body.append("Content-Type: image/jpeg\r\n\r\n") + body.append(Data("fake image data".utf8)) + body.append("\r\n") + // A Blob-shaped sidecar: has a filename, and carries bytes that are not valid + // UTF-8. If the partition ever admitted this to `fields`, the lone 0xFF would + // become U+FFFD and the value would be silently corrupted. + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"sidecar\"; filename=\"blob\"\r\n") + body.append("Content-Type: application/octet-stream\r\n\r\n") + body.append(Data([0x61, 0xFF, 0x62])) + body.append("\r\n--\(boundary)--\r\n") + + 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) + + #expect(uploader.uploadCalled) + #expect(uploader.lastFields.map(\.name) == ["post"], "only filename-less parts belong in fields") + #expect(!uploader.lastFields.contains { $0.value.contains("\u{FFFD}") }, "no field value should have been lossily decoded") + } + private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data, fields: [MediaUploadField] = []) -> Data { var body = Data() for field in fields {