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/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..96f999dfe 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,54 @@ 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. + * + * 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 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, 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) { + 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 +662,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 +695,32 @@ 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 + + // 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 (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 + } // The editor reaches the loopback server over cleartext http://localhost. If // the host app's network-security config doesn't permit cleartext to @@ -701,15 +740,19 @@ class GutenbergView : FrameLayout { } try { - val defaultUploader = DefaultMediaUploader( + // 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( - uploadDelegate = mediaUploadDelegate, - defaultUploader = defaultUploader, + processor = mediaProcessor, + uploader = mediaUploader, + 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 66cb94791..08e01630c 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,49 +68,113 @@ 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 +} + +/** + * 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. + * + * @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, in order, each decoded as UTF-8 — + * most importantly `post`, the parent post's ID, without which the attachment is + * 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. + */ +data class MediaUpload( + val file: File, + val mimeType: String, + val filename: String, + val fields: List, + 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, + * 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. + * 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 [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 + * 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 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. + * 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 uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null + suspend fun upload(upload: MediaUpload): ByteArray } /** @@ -126,9 +190,10 @@ interface MediaUploadDelegate { * stop on detach. */ internal class MediaUploadServer( - private val uploadDelegate: MediaUploadDelegate?, - private val defaultUploader: DefaultMediaUploader?, - cacheDir: File? = null, + private val processor: MediaProcessor?, + private val uploader: MediaUploader?, + private val internalClient: InternalMediaClient, + cacheDir: File, scope: CoroutineScope? = null, ioDispatcher: CoroutineDispatcher = Dispatchers.IO ) : HttpServerDelegate { @@ -141,11 +206,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. @@ -234,7 +298,7 @@ internal class MediaUploadServer( if (method == "DELETE") { attachmentIdFromPath(request.path)?.let { attachmentId -> - return handleDelete(attachmentId, request.query) + return handleMediaDelete(attachmentId, request.query) } } @@ -255,19 +319,34 @@ internal class MediaUploadServer( } /** - * Relays the editor's orphan cleanup to WordPress. + * 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. + * + * 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 suspend fun handleDelete(attachmentId: String, query: String): HttpResponse { - val uploader = defaultUploader ?: return errorResponse(500, "No uploader configured") + @Suppress("TooGenericExceptionCaught") + private suspend fun handleMediaDelete(attachmentId: String, query: String): HttpResponse { return try { - relayResponse(uploader.deleteMedia(attachmentId, query)) - } catch (e: IOException) { + // 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. + relayResponse(internalClient.deleteMedia(attachmentId, query)) + } 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") } @@ -286,18 +365,20 @@ 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) } 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") @@ -321,6 +402,11 @@ 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 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 { return HttpResponse( @@ -374,11 +460,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 -> { @@ -386,8 +473,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) } @@ -412,7 +499,7 @@ internal class MediaUploadServer( } } - // MARK: - Delegate Pipeline + // MARK: - Process + Deliver Pipeline private sealed class UploadResult { data class Uploaded(val response: MediaUploadResponse) : UploadResult() @@ -422,21 +509,27 @@ 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 { - val processed = uploadDelegate?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original + // 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 + } // 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 @@ -454,9 +547,28 @@ 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 { 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. + // + // 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)) + } + val upload = MediaUpload(targetFile, targetMimeType, targetFilename, fields, query) + return UploadResult.Uploaded(MediaUploadResponse(201, up.upload(upload))) } // Unmodified — forward the original request body directly, skipping @@ -465,8 +577,7 @@ internal class MediaUploadServer( return UploadResult.Passthrough } - val result = defaultUploader?.upload(targetFile, targetMimeType, targetFilename, extraParts, query) - ?: error("No upload delegate 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 @@ -523,7 +634,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, @@ -562,8 +673,11 @@ internal open class DefaultMediaUploader( 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/GutenbergViewUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt index a83cfe5f5..b319c107d 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 @@ -62,15 +64,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 +81,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,25 +97,94 @@ 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) } } + @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 `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() - 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 1d1f5ed6a..8da157973 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(uploadDelegate = 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(uploadDelegate = 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) @@ -63,8 +73,9 @@ 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, - defaultUploader = null, + processor = null, + uploader = null, + internalClient = MockInternalMediaClient(), cacheDir = tempFolder.root, scope = callerScope ) @@ -140,12 +151,66 @@ class MediaUploadServerTest { assertTrue(response.statusLine.contains("404")) } + @Test + 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 internal media client — the host + // uploader owns uploads, not deletes. + val uploader = MockUploader() + val internalClient = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + processor = null, + uploader = uploader, + internalClient = internalClient, + 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")) + assertTrue(internalClient.deleteMediaCalled) + assertEquals("42", internalClient.deletedAttachmentId) + } + + // MARK: - Media deletion + + @Test + 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 internal media client (the configured site). + val mockUploader = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + processor = null, + uploader = null, + internalClient = mockUploader, + cacheDir = tempFolder.root + ) + + val response = sendRawRequest( + method = "DELETE", + 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")) + assertTrue(mockUploader.deleteMediaCalled) + assertEquals("512", mockUploader.deletedAttachmentId) + } + @Test fun `routes upload with a query string and relays the query`() { - val delegate = ProcessOnlyDelegate() - val mockUploader = MockDefaultUploader() + val processor = PassthroughProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, 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 @@ -164,7 +229,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) @@ -172,20 +237,31 @@ 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 internal media client (configured site) is never used. + val uploader = MockUploader() + val internalClient = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + 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()) + val body = buildMultipartBody( + boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray(), + fields = listOf(MediaUploadField("post", "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" @@ -194,12 +270,20 @@ 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 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.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()) + // The host owns delivery — GutenbergKit must not upload to the configured site. + assertFalse(internalClient.uploadCalled) + assertFalse(internalClient.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) @@ -207,11 +291,181 @@ class MediaUploadServerTest { } @Test - fun `forwards the delegate's processed metadata to the uploader`() { - val delegate = TranscodingDelegate() - val mockUploader = MockDefaultUploader() + 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(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + 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( + MediaUploadField("post", "123"), + MediaUploadField("media_folder[]", "12"), + MediaUploadField("media_folder[]", "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.name == "media_folder[]" }.map { it.value } + ) + 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 + // 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 = MockInternalMediaClient() + server.stop() + 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()) @@ -226,7 +480,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) @@ -234,11 +488,11 @@ class MediaUploadServerTest { } @Test - fun `deletes the delegate's processed file after upload`() { - val delegate = TranscodingDelegate() - val mockUploader = MockDefaultUploader() + fun `deletes the processor's processed file after upload`() { + val processor = TranscodingProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, 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()) @@ -253,10 +507,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()) } @@ -276,8 +530,9 @@ class MediaUploadServerTest { // one — a flipped comparison would do the opposite and wipe an in-flight upload. server.stop() server = MediaUploadServer( - uploadDelegate = null, - defaultUploader = null, + processor = null, + uploader = null, + internalClient = MockInternalMediaClient(), cacheDir = tempFolder.root, ioDispatcher = Dispatchers.Unconfined ) @@ -286,15 +541,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 delegate does not modify file`() { - val delegate = ProcessOnlyDelegate() - val mockUploader = MockDefaultUploader() + fun `uses passthrough when the processor does not modify the file`() { + val processor = PassthroughProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, 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()) @@ -310,7 +565,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) @@ -320,12 +575,12 @@ class MediaUploadServerTest { } @Test - fun `skips processing and the temp copy when the delegate declines by metadata`() { - val delegate = DeclineByMetadataDelegate() - val mockUploader = MockDefaultUploader() + fun `skips processing and the temp copy when the processor declines by metadata`() { + val processor = DecliningProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, 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()) @@ -341,17 +596,17 @@ 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) } - // 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"}""" @@ -364,7 +619,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" @@ -389,13 +644,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" @@ -414,7 +669,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 @@ -429,7 +684,7 @@ class MediaUploadServerTest { ) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -447,12 +702,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", @@ -470,12 +725,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", @@ -507,7 +762,7 @@ class MediaUploadServerTest { ) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -535,12 +790,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" @@ -575,7 +830,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" @@ -638,8 +893,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, @@ -694,16 +963,23 @@ class MediaUploadServerTest { } } - return RawHttpResponse(statusLine, responseHeaders, responseBody) + return RawHttpResponse(statusLine, responseHeaders, responseBody, lines.drop(1)) } private fun buildMultipartBody( boundary: String, filename: String, mimeType: String, - data: ByteArray + data: ByteArray, + fields: List = emptyList() ): 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()) @@ -714,27 +990,35 @@ 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(), + private val error: Exception? = null + ) : MediaUploader { + @Volatile var uploadCalled = false @Volatile var lastMimeType: String? = null @Volatile var lastFilename: String? = null + @Volatile var lastFields: List = emptyList() + @Volatile var lastQuery: String? = null + @Volatile var lastFileBytes: ByteArray? = null - override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { - processFileCalled = 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()) + override suspend fun upload(upload: MediaUpload): ByteArray { + uploadCalled = true + lastMimeType = upload.mimeType + lastFilename = upload.filename + lastFields = upload.fields + lastQuery = upload.query + lastFileBytes = upload.file.readBytes() + error?.let { throw it } + 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 { @@ -747,7 +1031,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 @@ -758,9 +1042,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 { @@ -771,7 +1055,13 @@ class MediaUploadServerTest { } } - private class MockDefaultUploader : DefaultMediaUploader( + 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() + ) + ) : InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = "https://example.com/wp-json/", authHeader = "Bearer mock" @@ -781,6 +1071,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, @@ -790,7 +1082,7 @@ class MediaUploadServerTest { lastUploadMimeType = mimeType lastUploadFilename = filename lastQuery = query - return mockResponse() + return uploadResponse } override suspend fun passthroughUpload( @@ -800,13 +1092,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/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 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/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..cd9cc5e53 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -105,52 +105,60 @@ 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 + /// 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`. It + /// is captured once, when the editor begins loading; setting it afterward has no + /// effect, so the setter traps. + /// + /// 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 { + precondition(!hasStartedLoading, Self.lateMediaAssignmentMessage("mediaProcessor")) + } + } - /// Delegate for customizing media file processing and upload behavior. + /// 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. /// - /// 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. + /// 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. /// - /// - 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)? { + /// 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 { - // 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." - ) + 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 @@ -384,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() @@ -451,28 +459,33 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// falls back to Gutenberg's default upload behavior (the JS override won't activate /// because `nativeUploadPort` will be nil in GBKit). private func startUploadServer() async { - // A delegate that was provided but is already nil here was deallocated before - // the editor finished loading — the host didn't hold a strong reference to it. - // That silently disables native uploads, so trap loudly instead. - precondition( - !(mediaUploadDelegateWasAssigned && mediaUploadDelegate == nil), - "mediaUploadDelegate was released before the editor loaded — hold a strong reference to it." - ) - - guard mediaUploadDelegate != nil else { + // Nothing to route through the native server unless the host provided a + // 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 } - // 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 { + // 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 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. + // + // `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 } - let defaultUploader = DefaultMediaUploader( + let internalClient = InternalMediaClient( httpClient: httpClient.uploadClient(), siteApiRoot: configuration.siteApiRoot, siteApiNamespace: configuration.siteApiNamespace @@ -480,8 +493,9 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro do { self.uploadServer = try await MediaUploadServer.start( - uploadDelegate: mediaUploadDelegate, - defaultUploader: defaultUploader + processor: mediaProcessor, + uploader: mediaUploader, + 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/MediaHandlers.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift new file mode 100644 index 000000000..83d3a09c2 --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift @@ -0,0 +1,166 @@ +import Foundation + +/// A raw response from the WordPress REST API media endpoint. +/// +/// GutenbergKit relays this to the editor verbatim — it does not interpret the +/// body. The editor therefore receives the exact attachment object (on success) +/// or WordPress REST error object (on failure) it would get from a direct +/// upload, so every consumer — image sub-sizes, attachment links, error notices — +/// behaves identically to a non-native upload. +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. + let body: Data + + /// The response headers to relay to the editor. + /// + /// `x-wp-upload-attachment-id` is the one that carries behavior: WordPress + /// sets it on a failed upload whose attachment row was created before + /// 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. + let headers: [String: String] + + init(statusCode: Int, body: Data, headers: [String: String] = [:]) { + self.statusCode = statusCode + self.body = body + self.headers = headers + } +} + +/// 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. + case original + + /// The delegate produced a file to upload, along with its MIME type and + /// filename. Both are used verbatim, so a format change (e.g. transcoding + /// MOV to MP4, or an in-place EXIF strip) must report the resulting type and + /// filename for WordPress to store the file correctly. + case processed(URL, mimeType: String, filename: String) +} + +/// Transforms media before GutenbergKit delivers it. +/// +/// 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 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`. 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 + + /// 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 +} + +/// 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 { + /// 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, in order, each decoded as UTF-8 — most + /// importantly `post`, the parent post's ID, without which the attachment is created + /// 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. + 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. +/// +/// 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. + /// + /// 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 + /// 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. + /// + /// 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(_ upload: MediaUpload) async throws -> Data +} + +/// Default implementations for the optional ``MediaProcessor`` methods. +extension MediaProcessor { + public func handlesFile(ofType mimeType: String, named filename: String) -> Bool { + true + } + + public func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + .original + } +} diff --git a/ios/Sources/GutenbergKit/Sources/Media/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/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift deleted file mode 100644 index 73752166b..000000000 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ /dev/null @@ -1,100 +0,0 @@ -import Foundation - -/// A raw response from the WordPress REST API media endpoint. -/// -/// GutenbergKit relays this to the editor verbatim — it does not interpret the -/// body. The editor therefore receives the exact attachment object (on success) -/// or WordPress REST error object (on failure) it would get from a direct -/// upload, so every consumer — image sub-sizes, attachment links, error notices — -/// behaves identically to a non-native upload. -public struct MediaUploadResponse: Sendable { - /// The HTTP status code WordPress (or the host's upload service) returned. - public let statusCode: Int - - /// The raw response body — a WordPress REST attachment on success, or a - /// WordPress REST error object (`{ "code", "message", "data" }`) on failure. - public let body: Data - - /// The response headers to relay to the editor. - /// - /// `x-wp-upload-attachment-id` is the one that carries behavior: WordPress - /// sets it on a failed upload whose attachment row was created before - /// 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] - - public 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:)``. -public enum ProcessedProxyFile: Sendable { - /// The delegate did not modify the file; the original upload is forwarded - /// to WordPress unchanged. - case original - - /// The delegate produced a file to upload, along with its MIME type and - /// filename. Both are used verbatim, so a format change (e.g. transcoding - /// MOV to MP4, or an in-place EXIF strip) must report the resulting type and - /// filename for WordPress to store the file correctly. - case processed(URL, mimeType: String, filename: String) -} - -/// Protocol for customizing media upload behavior. -/// -/// The native host app can provide an implementation to resize images, -/// transcode video, or use its own upload service. Default implementations -/// pass files through unchanged and upload via the WordPress REST API. -public protocol MediaUploadDelegate: AnyObject, Sendable { - /// Whether this delegate might handle a file with the given metadata — either - /// processing it (``processFile(at:mimeType:filename:)``) or uploading it - /// itself (``uploadFile(at:mimeType:filename:)``). - /// - /// A cheap, metadata-only gate the server consults *before* materializing the - /// upload to a temp file. Return `false` to decline a file by type — e.g. an - /// image-only delegate returning `false` for a video — so the server forwards - /// the original upload to WordPress without first copying a file the delegate - /// won't touch. Because it gates the temp-file copy needed by *both* - /// `processFile` and `uploadFile`, return `true` for any file the delegate - /// will either process or upload itself. - /// - /// Defaults to `true`: every file is materialized and the full pipeline runs. - /// A `true` here is not a commitment — `processFile` may still return - /// `.original` after inspecting the file's contents. - func handlesFile(ofType mimeType: String, named filename: String) -> Bool - - /// Process a file before upload (e.g., resize image, transcode video). - /// - /// Return ``ProcessedProxyFile/original`` to upload the file unchanged, or - /// ``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? -} - -/// Default implementations. -extension MediaUploadDelegate { - public func handlesFile(ofType mimeType: String, named filename: String) -> Bool { - true - } - - public func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { - .original - } - - public func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { - nil - } -} diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index a2159d762..b0ac09f42 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -29,13 +29,16 @@ 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. + /// - 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( - uploadDelegate: (any MediaUploadDelegate)? = nil, - defaultUploader: DefaultMediaUploader? = nil, + processor: (any MediaProcessor)? = nil, + uploader: (any MediaUploader)? = nil, + internalClient: InternalMediaClient, maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize ) async throws -> MediaUploadServer { // Sweep temp files orphaned by a prior crash, off the editor-startup @@ -45,7 +48,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(uploadDelegate: uploadDelegate, defaultUploader: defaultUploader) + 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 @@ -62,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) @@ -84,244 +85,334 @@ 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 handleDelete(attachmentId, query: parsed.query, context: context) - } + 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 fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(MediaUploadServer.sanitizeFilename(filename))") + do { + let inputStream = try filePart.body.makeInputStream() + try MediaUploadServer.writeStream(inputStream, to: fileURL) + } catch { + 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") + } - let filename = filePart.filename ?? "upload" - let mimeType = filePart.contentType + // 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) } - // Ask the delegate — from metadata alone — whether it will touch a file - // like this. If not, forward the original upload to WordPress directly, - // skipping a full temp-file copy of a file the delegate won't process or - // upload (e.g. a video handed to an image-only delegate). - guard context.uploadDelegate?.handlesFile(ofType: mimeType, named: filename) ?? false else { do { - return try await passthroughResponse(request, query: query, context: context) + 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 uploadErrorResponse(error) + return Self.uploadErrorResponse(error) } } - // The delegate wants the file. Stream the part body to a dedicated temp - // file for it — the library's RequestBody may be a byte-range slice of a - // larger temp file whose lifecycle is tied to ARC, so the delegate needs a - // standalone file that outlives the handler return. - let tempDir = uploadsTempDirectory - try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) - - let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(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") + /// 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) } - // 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) } + /// 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 + } - do { - let uploadResult = try await processAndUpload( - fileURL: fileURL, mimeType: mimeType, filename: filename, - extraParts: extraParts, query: query, 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, context: context) + /// 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) } - } catch { - return uploadErrorResponse(error) } - } - /// Forwards the original request body to WordPress unchanged (no multipart - /// re-encoding) and relays the response. Used when the delegate won't touch - /// the file — it declined by metadata (`handlesFile` returned false) or - /// `processFile` returned `.original`. - private static func passthroughResponse( - _ request: HTTPServer.Request, query: String, context: UploadContext - ) async throws -> HTTPResponse { - Logger.uploadServer.debug("Passthrough: forwarding original request body to WordPress") - guard let body = request.parsed.body, - let contentType = request.parsed.header("Content-Type"), - let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + /// 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 + ) } - let response = try await defaultUploader.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 the editor's orphan cleanup to WordPress. - /// - /// 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( - _ attachmentId: String, query: String, context: UploadContext - ) async -> HTTPResponse { - guard let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) - } - do { - let response = try await defaultUploader.deleteMedia(attachmentId: attachmentId, query: query) - return relayResponse(response) - } catch { - return 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)") + } + return MediaUploadServer.errorResponse(status: 500, message: error.localizedDescription) } - } - /// 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. - private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { - HTTPResponse( - status: response.statusCode, - headers: [("Content-Type", "application/json")] - + response.headers.map { ($0.key, $0.value) }, - body: response.body - ) - } + // 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)") + /// 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 } - return errorResponse(status: 500, message: error.localizedDescription) - } - // MARK: - Delegate Pipeline - - /// Result of the delegate processing + upload pipeline. - private enum UploadResult { - /// The delegate (or default uploader) completed the upload; carries the - /// raw WordPress response to relay. - case uploaded(MediaUploadResponse) - /// The delegate didn't modify the file and `uploadFile` returned nil. - /// The caller should forward the original request body to WordPress. - case passthrough - } + private 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 + } - private static func processAndUpload( - fileURL: URL, mimeType: String, filename: String, - extraParts: [MultipartPart], query: String, context: UploadContext - ) async throws -> UploadResult { - // Step 1: Process (resize, transcode, etc.) - let processed: ProcessedProxyFile - if let delegate = context.uploadDelegate { - processed = try await delegate.processFile(at: fileURL, mimeType: mimeType, filename: filename) - } else { - processed = .original - } + // 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 + } - // Resolve the file to upload and its metadata. `.processed` uses the - // delegate's values verbatim, so a format change is reported to WordPress. - let uploadURL: URL - let uploadMimeType: String - let uploadFilename: String - switch processed { - case .original: - uploadURL = fileURL - uploadMimeType = mimeType - uploadFilename = filename - case let .processed(url, processedMimeType, processedFilename): - uploadURL = url - uploadMimeType = processedMimeType - uploadFilename = processedFilename - } + // The processed file (if the 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 processed file (if the delegate produced a new one) is ours to - // clean up — on success it has been uploaded, on failure it is abandoned. - // Cleaning up here rather than in the caller covers the throw paths too. - defer { - if uploadURL != fileURL { - try? FileManager.default.removeItem(at: uploadURL) + // 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) } } - // 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) - } else if let defaultUploader = context.defaultUploader { - // Unmodified — forward the original request body directly, skipping - // multipart re-encoding. - if case .original = processed { - return .passthrough + /// 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. + /// + /// 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 { + fields.append(MediaUploadField(name: part.name, value: String(decoding: try await part.body.data, as: UTF8.self))) } - let result = try await defaultUploader.upload(fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, extraParts: extraParts, query: query) - return .uploaded(result) - } else { - throw UploadError.noUploader + return fields } } @@ -432,47 +523,21 @@ 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" } } } -// MARK: - Upload Context - -/// Container for the upload delegate and default uploader, captured by the -/// HTTPServer handler closure and re-read on each request. -/// -/// The delegate is held **weakly**. `EditorViewController.mediaUploadDelegate` is -/// declared `weak` — the host owns the delegate's lifetime. Capturing it strongly -/// here would silently defeat that contract and, worse, risk a retain cycle -/// (`EditorViewController → uploadServer → HTTPServer → handler → UploadContext → -/// delegate → EditorViewController`) that would keep the view controller — and -/// therefore the server — alive forever, so `deinit` would never stop it. -/// -/// `@unchecked Sendable`: `uploadDelegate` is assigned once at init and only read -/// afterwards; weak-reference reads are thread-safe at runtime. -private final class UploadContext: @unchecked Sendable { - weak var uploadDelegate: (any MediaUploadDelegate)? - let defaultUploader: DefaultMediaUploader? - - init(uploadDelegate: (any MediaUploadDelegate)?, defaultUploader: DefaultMediaUploader?) { - self.uploadDelegate = uploadDelegate - self.defaultUploader = defaultUploader - } -} - // MARK: - Default Media Uploader /// 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? @@ -624,9 +689,16 @@ class DefaultMediaUploader: @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/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 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/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 + ) + } + } +} diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index fa87bbc4f..e6dc21cca 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")! @@ -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 mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let processor = PassthroughProcessor() + 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`, @@ -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,17 +131,64 @@ struct MediaUploadServerTests { #expect(mockUploader.lastQuery == "?_embed=wp:featuredmedia") } - @Test("calls delegate and returns upload result") - func delegateProcessAndUpload() async throws { - let delegate = MockUploadDelegate() - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + @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 internal media client — the host uploader owns + // uploads, not deletes. Held strongly: UploadContext keeps the uploader weakly. + let uploader = MockUploader() + 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")! + 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(internalClient.deleteMediaCalled) + #expect(internalClient.deletedAttachmentId == "42") + } + + @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 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")! + 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) + + #expect((response as? HTTPURLResponse)?.statusCode == 200) + #expect(mockUploader.deleteMediaCalled) + #expect(mockUploader.deletedAttachmentId == "512") + } + + @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 internal media client (configured site) is never used. + let uploader = MockUploader() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: internalClient) 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), fields: [MediaUploadField(name: "post", value: "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") @@ -152,12 +199,20 @@ 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") - - // The server relays WordPress's raw response body verbatim. + #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.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)) + // The host owns delivery — GutenbergKit must not upload to the configured site. + #expect(!internalClient.uploadCalled) + #expect(!internalClient.passthroughUploadCalled) + + // 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) @@ -165,11 +220,99 @@ 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() - let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + @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: [ + MediaUploadField(name: "post", value: "123"), + MediaUploadField(name: "media_folder[]", value: "12"), + MediaUploadField(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 + // 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 = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -187,7 +330,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) @@ -198,11 +341,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() - let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + @Test("skips processing and the temp copy when the processor declines by metadata") + func processorDeclinesByMetadata() async throws { + let processor = DecliningProcessor() + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -219,18 +362,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 mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let processor = ResizingProcessor() + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -245,18 +388,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 mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let processor = ResizingProcessor() + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -271,16 +414,16 @@ 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))) } @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 @@ -305,7 +448,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 @@ -351,7 +494,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() @@ -359,26 +502,157 @@ 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("retains the processor for the server's lifetime, and releases it on stop") + func retainsProcessorForServerLifetime() async throws { + weak var weakProcessor: PassthroughProcessor? + 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 delegate = MockUploadDelegate() - weakDelegate = delegate - server = try await MediaUploadServer.start(uploadDelegate: delegate) + let uploader = ReleasableUploader(didUpload: didUpload) + holder.value = uploader + server = try await MediaUploadServer.start( + processor: processor, uploader: uploader, internalClient: mockClient + ) + } + 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 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) + } + + @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() } - // UploadContext holds the delegate weakly, so releasing the host's strong - // reference deallocates it. A strong reference here would reintroduce the - // EditorViewController → uploadServer → … → delegate → EditorViewController - // cycle, so deinit would never fire and the server would never stop. - #expect(weakDelegate == nil) + 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) -> Data { + private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data, fields: [MediaUploadField] = []) -> Data { var body = Data() + for field in fields { + body.append("--\(boundary)\r\n") + 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") body.append("Content-Disposition: form-data; name=\"file\"; filename=\"\(filename)\"\r\n") body.append("Content-Type: \(mimeType)\r\n\r\n") @@ -390,7 +664,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") @@ -413,7 +687,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) @@ -430,7 +704,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", @@ -465,7 +739,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))] ) @@ -497,7 +771,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)] ) @@ -513,7 +787,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: [] ) @@ -537,7 +811,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 ) @@ -564,7 +838,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 ) @@ -577,17 +851,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) @@ -614,7 +888,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( @@ -637,7 +911,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( @@ -655,7 +929,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"] @@ -671,7 +945,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"] @@ -694,7 +968,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 @@ -765,37 +1039,90 @@ 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 var _lastFields: [MediaUploadField] = [] + private var _lastQuery: String? + private var _lastFileData: Data? + private let uploadBody: Data + private let error: (any Error)? - 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 } } + var lastFields: [MediaUploadField] { 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), + error: (any Error)? = nil + ) { + self.uploadBody = uploadBody + self.error = error + } - func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + 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 { - _processFileCalled = true - _lastMimeType = mimeType + _uploadCalled = true + _lastMimeType = upload.mimeType + _lastFilename = upload.filename + _lastFields = upload.fields + _lastQuery = upload.query + _lastFileData = fileData } - return .original + if let error { throw error } + return uploadBody } +} - func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { - lock.withLock { - _uploadFileCalled = true - _lastFilename = filename +/// 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)) } - let json = #"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"# - return MediaUploadResponse(statusCode: 201, body: Data(json.utf8)) + 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 ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class PassthroughProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -807,10 +1134,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 @@ -824,12 +1151,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 { @@ -840,21 +1167,29 @@ private final class ResizingDelegate: MediaUploadDelegate, @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 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/")!) } @@ -865,7 +1200,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 { @@ -873,13 +1208,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 { 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 2c0edce44..43c7907e5 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 && @@ -197,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 @@ -227,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 @@ -257,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 @@ -273,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, @@ -292,9 +300,10 @@ export function nativeMediaUploadMiddleware( options, next ) { // 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;