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 d893265f4..a36485db5 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -113,29 +113,60 @@ class GutenbergView : FrameLayout { var requestInterceptor: GutenbergRequestInterceptor = DefaultGutenbergRequestInterceptor() /** - * Optional delegate for customizing media upload behavior (resize, transcode, - * custom upload). + * Optional processor that transforms media before upload (resize, transcode, + * strip EXIF). + * + * To perform the upload yourself, set [mediaUploader] instead. * * Provide this **before the editor loads** — typically right after * construction (e.g. in the `AndroidView` factory). It is captured once, when * the page begins loading, and advertised to the page then; setting it * afterward has no effect, so the setter throws to surface the mistake. */ - var mediaUploadDelegate: MediaUploadDelegate? = null + 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. + * + * A [mediaProcessor] can still transform the file first; only delivery moves + * to the uploader. + */ + var mediaUploader: MediaUploader? = null + set(value) { + check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaUploader") } + // An uploader's media deletes still relay through the internal media client, + // which needs a site root and an auth header to reach the configured site. + // Check it here, where the host hands the uploader over, rather than at + // server start: the stack trace names the caller's own line, and the mistake + // can't hide until the page loads. `configuration` is assigned in the + // constructor, so it is always available by the time this runs. + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot = configuration.siteApiRoot, + authHeader = configuration.authHeader, + hasUploader = value != null + ) 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 + * configuration has been captured. After this the [mediaProcessor] can no * longer take effect, so its setter throws. */ @Volatile private var hasStartedLoading = false @@ -643,13 +674,13 @@ class GutenbergView : FrameLayout { /** * Invoked when the editor page begins loading. Starts the upload server once — - * capturing the [mediaUploadDelegate] provided before load — then advertises + * capturing the [mediaProcessor] 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 + * [mediaProcessor] setter keeps its whole lifecycle — start here, stop in * [onDetachedFromWindow] — on the UI thread, so it can't race a - * background-thread delegate assignment. + * background-thread processor assignment. */ private fun onEditorPageStarted() { if (!hasStartedLoading) { @@ -676,17 +707,22 @@ class GutenbergView : FrameLayout { } private fun startUploadServer() { - // No delegate means nothing wants to customize uploads, so there's no reason - // to route them through the native server — leave it down and let uploads - // fall to the default WebView path. (Matches iOS.) - if (mediaUploadDelegate == null) return - - // The native upload server relays through DefaultMediaUploader, which needs a - // site root and an auth header (every host provides one — the editor injects - // it because the WebView has no auth cookies). Without both there is nothing - // to upload through, so leave the server down and let uploads fall to the - // default WebView path rather than start a server that could only fail. - if (configuration.siteApiRoot.isEmpty() || configuration.authHeader.isEmpty()) return + // Nothing to route through the native server unless the host provided a + // processor or an uploader — leave it down and let uploads fall to the default + // WebView path. (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 the latter because the WebView + // has no auth cookies). Without them 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. + // + // Only a mediaProcessor can reach this return: a mediaUploader without + // credentials already failed in its setter, so by here it has them. + if (!MediaServerCredentials.areUsable(configuration.siteApiRoot, configuration.authHeader)) return // The editor reaches the loopback server over cleartext http://localhost. If // the host app's network-security config doesn't permit cleartext to @@ -694,7 +730,14 @@ class GutenbergView : FrameLayout { // before it leaves the page. Detect that here and don't start the server, so // the JS middleware routes uploads down the default path instead of a server // it can never reach. Hosts that want native media processing must permit - // cleartext to localhost (see the demo's res/xml/network_security_config.xml). + // cleartext to localhost — see "Android: permit cleartext to localhost" in + // docs/integration.md, and the demo's res/xml/network_security_config.xml. + // + // This drops a mediaUploader as silently as missing credentials would, and still + // only warns. The difference is the cause, not the symptom: the configuration is + // sound here — permit cleartext and the same setup works unchanged — so there is + // nothing for the host to fix in what it handed us. See + // MediaServerCredentials.requireCredentialsForUploader. if (!NetworkSecurityPolicy.getInstance().isCleartextTrafficPermitted(LOOPBACK_HOST)) { Log.w( TAG, @@ -706,15 +749,16 @@ class GutenbergView : FrameLayout { } try { - val defaultUploader = DefaultMediaUploader( + val internalClient = InternalMediaClient( httpClient = uploadHttpClient, siteApiRoot = configuration.siteApiRoot, authHeader = configuration.authHeader, siteApiNamespace = configuration.siteApiNamespace.toList() ) uploadServer = MediaUploadServer( - uploadDelegate = mediaUploadDelegate, - defaultUploader = defaultUploader, + processor = mediaProcessor, + internalClient = internalClient, + uploader = mediaUploader, cacheDir = context.cacheDir, scope = coroutineScope ) diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaServerCredentials.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaServerCredentials.kt new file mode 100644 index 000000000..d173c8087 --- /dev/null +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaServerCredentials.kt @@ -0,0 +1,83 @@ +package org.wordpress.gutenberg + +import android.net.Uri + +/** + * Whether the editor configuration can reach the configured site for media, and the + * fail-fast that enforces it. + * + * The counterpart of iOS's `MediaServerCredentials`, kept deliberately close to it. + * This is a *crash* policy, and it has already diverged silently between the platforms + * once: iOS required an absolute site root while this side checked only `isEmpty()`, so + * a scheme-less root trapped on iOS and started a server whose every delete failed on + * Android. Both platforms keep the policy in a type of this name so the two can be + * diffed against each other rather than hunted for across view code. + */ +internal object MediaServerCredentials { + /** + * Whether an [InternalMediaClient] built from this configuration could actually + * reach the site. + * + * Both fields are required. The client delivers GutenbergKit's uploads 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. + * + * "Somewhere to send them" means an *absolute* root, not merely a non-empty one. + * OkHttp rejects a scheme-less URL from `Request.Builder.url` with + * `IllegalArgumentException`, which is not an `IOException` — so it escapes + * [MediaUploadServer]'s delete handler and degrades to a generic 500 the editor + * cannot parse into an error, and the orphan cleanup that delete exists for fails + * silently. An empty root parses to the same nulls, so this still rejects + * everything the older `isEmpty()` check did. + * + * Emptiness is tested as well as nullity because `Uri` and Swift's `URL` disagree + * on how they report a missing authority: `file:///tmp/wp-json` yields a `null` + * host on iOS but an *empty* one here. Treating both as absent is what keeps the + * two predicates answering alike. + */ + fun areUsable(siteApiRoot: String, authHeader: String): Boolean { + if (authHeader.isEmpty()) return false + val uri = Uri.parse(siteApiRoot) + return !uri.scheme.isNullOrEmpty() && !uri.host.isNullOrEmpty() + } + + /** + * Throws if the host supplied 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 + * server simply stays down and uploads fall to the default WebView path. That is + * [areUsable]'s job, at the point the server would start. + * + * - A [MediaUploader] means the host is *taking over* uploads, and falling back + * would drop that whole stack — its queueing, its retries — while media appeared + * to keep working. Worth failing over rather than logging. + * + * What makes it a *failure* rather than a warning is that the configuration is + * incoherent, not merely unlucky: an uploader's media deletes still relay through + * the internal media client, so there is no site root and auth header under which + * this host's uploader could have worked. Contrast the conditions the host's + * environment imposes at server start — the network policy that blocks cleartext to + * localhost, a port that won't bind — which log and degrade, because the very same + * configuration works once the environment allows it. Dropping the uploader is the + * symptom both share; only this one has a cause the host can fix in the + * configuration it just handed over. + * + * Called from [GutenbergView.mediaUploader]'s setter, not from the server start — + * the earliest point available here, since this platform takes its media handlers + * as mutable properties rather than at construction. Checking where the host hands + * the uploader over puts the caller's own line in the stack trace, instead of + * surfacing the mistake later from inside a page-load callback. (iOS checks in + * `EditorViewController.init`, for the same reason.) + */ + fun requireCredentialsForUploader(siteApiRoot: String, authHeader: String, hasUploader: Boolean) { + if (!hasUploader) return + check(areUsable(siteApiRoot, authHeader)) { + "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." + } + } +} 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 6ce861d4b..05cfb9f4d 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -6,6 +6,8 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.cancel +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine import kotlin.coroutines.resume @@ -31,8 +33,11 @@ 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, or 201 for an upload a + * [MediaUploader] delivered. + */ val statusCode: Int, /** * The raw response body — a WordPress REST attachment on success, or a @@ -52,14 +57,14 @@ class MediaUploadResponse( ) /** - * The result of a delegate's [MediaUploadDelegate.processFile]. + * The result of a processor's [MediaProcessor.processFile]. */ sealed class ProcessedProxyFile { - /** The delegate did not modify the file; the original upload is forwarded unchanged. */ + /** The processor did not modify the file; the original upload is forwarded unchanged. */ data object Original : ProcessedProxyFile() /** - * The delegate produced a file to upload, along with its MIME type and + * The processor 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. @@ -68,23 +73,30 @@ sealed class ProcessedProxyFile { } /** - * Interface for customizing media upload behavior. + * 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, it cannot deliver media to the wrong place. Set + * [GutenbergView.mediaProcessor] to resize images, transcode video, strip EXIF, + * etc. * - * The native host app can provide an implementation to resize images, - * transcode video, or use its own upload service. + * This is the safe, common extension point: most hosts want only this. To perform the + * upload yourself, implement [MediaUploader] instead. */ -interface MediaUploadDelegate { +interface MediaProcessor { /** - * Whether this delegate might handle a file with the given metadata — either - * processing it ([processFile]) or uploading it itself ([uploadFile]). + * Whether this processor might transform a file with the given metadata. * * A cheap, metadata-only gate the server consults *before* materializing the * upload to a temp file. Return false to decline a file by type — e.g. an - * image-only delegate returning false for a video — so the server forwards - * the original upload to WordPress without first copying a file the delegate - * won't touch. Because it gates the temp-file copy needed by *both* - * [processFile] and [uploadFile], return true for any file the delegate will - * either process or upload itself. + * image-only processor returning false for a video — so the server forwards + * the original upload to WordPress without first copying a file the processor + * won't touch. + * + * With a [MediaUploader] set this can't decline the upload itself — an uploader + * delivers every file, so there is no passthrough to fall to — but it still gates + * [processFile]: a declined file reaches the uploader unprocessed. * * Defaults to true: every file is materialized and the full pipeline runs. A * true here is not a commitment — [processFile] may still return @@ -101,16 +113,81 @@ interface MediaUploadDelegate { * 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 field's meaning is legible at every call + * site, and so the type can gain members without a source break for every host. + * + * @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=wp:featuredmedia`), + * 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 internal media client 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 the editor to retry behind it. + */ +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. * - * 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. + * 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. That request needs a body of `{"action": "create-image-subsizes"}` — + * core registers `action` as **required**, so a post-process request without it + * fails with a 400 every time rather than recovering. + * + * 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 the editor + * cleans up behind you. */ - suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null + suspend fun upload(upload: MediaUpload): ByteArray } /** @@ -126,8 +203,9 @@ interface MediaUploadDelegate { * stop on detach. */ internal class MediaUploadServer( - private val uploadDelegate: MediaUploadDelegate?, - private val defaultUploader: DefaultMediaUploader?, + private val processor: MediaProcessor?, + private val internalClient: InternalMediaClient?, + private val uploader: MediaUploader? = null, cacheDir: File? = null, scope: CoroutineScope? = null, ioDispatcher: CoroutineDispatcher = Dispatchers.IO @@ -264,9 +342,9 @@ internal class MediaUploadServer( * browser blocks it at preflight. Relaying it here lets the cleanup run. */ private suspend fun handleDelete(attachmentId: String, query: String): HttpResponse { - val uploader = defaultUploader ?: return errorResponse(500, "No uploader configured") + val client = internalClient ?: return errorResponse(500, "No internal media client configured") return try { - relayResponse(uploader.deleteMedia(attachmentId, query)) + relayResponse(client.deleteMedia(attachmentId, query)) } catch (e: IOException) { Log.e(TAG, "Media deletion failed", e) errorResponse(500, e.message ?: "Deletion failed") @@ -286,18 +364,25 @@ internal class MediaUploadServer( val mimeType = filePart.contentType val filename = filePart.filename ?: "upload" - // Ask the delegate — from metadata alone — whether it will touch a file + // Ask the processor — 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) { + // skipping a full temp-file copy of a file the processor won't process + // (e.g. a video handed to an image-only processor). + // An uploader takes over delivery for *every* file, so with one set there is no + // passthrough to fall to and the gate can't decline the upload outright. It + // still decides whether processFile runs, though — a declined file is handed to + // the uploader unprocessed rather than to a processor that said it won't touch it + // — so the answer is carried into processAndUpload rather than short-circuited + // away here. Asked exactly once per upload, matching iOS. + 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") @@ -325,7 +410,7 @@ internal class MediaUploadServer( * The response's own `Content-Type` wins over the JSON default, matched * case-insensitively — HTTP header names are case-insensitive, and * [HttpResponse] serializes every entry it is given, so a plain map merge - * would emit the name twice for a delegate that spells it `content-type`. + * would emit the name twice for a processor that spells it `content-type`. */ private fun relayResponse(response: MediaUploadResponse): HttpResponse { val hasContentType = response.headers.keys.any { it.lowercase() == "content-type" } @@ -381,11 +466,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 -> { @@ -393,7 +479,7 @@ internal class MediaUploadServer( uploadResult.response } is UploadResult.Passthrough -> { - // Delegate didn't modify the file — forward the original + // The processor didn't modify the file — forward the original // request body to WordPress without re-encoding. Log.d(TAG, "Passthrough: forwarding original request body to WordPress") performPassthroughUpload(request, query) @@ -407,11 +493,12 @@ internal class MediaUploadServer( throw e // Never swallow coroutine cancellation. } catch (e: Exception) { // Any other failure — IOException from the upload call, JSON parse - // errors, a throwing host delegate, or "no uploader configured" — - // must still be answered WITH CORS headers. Otherwise it escapes to - // HttpServer's header-less 500 fallback and the browser rejects the - // preflighted cross-origin fetch with an opaque "Failed to fetch", - // hiding the real error from the editor (mirrors the iOS catch-all). + // errors, a throwing host processor, or "no internal media client + // configured" — must still be answered WITH CORS headers. Otherwise + // it escapes to HttpServer's header-less 500 fallback and the browser + // rejects the preflighted cross-origin fetch with an opaque "Failed to + // fetch", hiding the real error from the editor (mirrors the iOS + // catch-all). Log.e(TAG, "Upload failed", e) return errorResponse(500, e.message ?: "Upload failed") } finally { @@ -419,7 +506,7 @@ internal class MediaUploadServer( } } - // MARK: - Delegate Pipeline + // MARK: - Processor Pipeline private sealed class UploadResult { data class Uploaded(val response: MediaUploadResponse) : UploadResult() @@ -429,21 +516,32 @@ 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") + val client = internalClient + if (body == null || contentType == null || client == null) { + throw MediaUploadException( + "Passthrough upload requires a request body, Content-Type, and internal media client" + ) } - return uploader.passthroughUpload(body, contentType, query) + return client.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 + // Process (resize, transcode, etc.) — but only for a file the processor's + // metadata gate accepted. handlesFile returning false is the processor saying it + // won't touch a file like this, so handing it one anyway would break the + // contract the gate documents. With an uploader set the file still gets + // delivered; it just skips processing on its way there. + val processed = if (processorWantsFile) { + processor?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original + } 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 @@ -461,9 +559,25 @@ internal class MediaUploadServer( } try { - // If the delegate provided its own upload, use that. - uploadDelegate?.uploadFile(targetFile, targetMimeType, targetFilename)?.let { - return UploadResult.Uploaded(it) + // The editor was torn down (or the client disconnected) while we processed. + // Don't put an upload on the wire 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 delivery path to notice keeps this + // true for a host uploader that isn't cancellation-cooperative. (Matches iOS.) + currentCoroutineContext().ensureActive() + + // 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 { hostUploader -> + val upload = MediaUpload( + file = targetFile, + mimeType = targetMimeType, + filename = targetFilename, + fields = formFields(extraParts), + query = query + ) + return UploadResult.Uploaded(MediaUploadResponse(201, hostUploader.upload(upload))) } // Unmodified — forward the original request body directly, skipping @@ -472,11 +586,11 @@ 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) + ?: error("No media uploader or internal media client configured") return UploadResult.Uploaded(result) } finally { - // The processed file (if the delegate produced a new one) is ours to + // The processed file (if the processor produced a new one) is ours to // clean up — covers the success and throw paths alike. if (targetFile != file) { targetFile.delete() @@ -484,6 +598,31 @@ internal class MediaUploadServer( } } + /** + * The editor's non-file form parts as ordered, UTF-8-decoded fields. + * + * A list rather than a map so repeated names (e.g. a `field[]` array) survive + * verbatim, in the order the editor sent them. + * + * This decode can't mangle anything, but only because of who is on the other end — + * nothing in the code enforces it. Three things have to stay true: + * + * 1. Only the editor's own web page can reach this server. It listens on loopback, + * and every request has to carry a per-session token. + * 2. Text the editor puts in a form field is already valid Unicode. The browser + * guarantees that when the value is set, so it cannot hand us bad bytes. + * 3. The only way a browser can put *raw* bytes in a form is a file or a Blob, and + * those always arrive with a filename. Anything with a filename is handled as the + * file, never as a field — so raw bytes never reach this decode. + * + * If one of those stops being true, bad bytes quietly turn into replacement + * characters, and the platforms don't even agree on how many: ED A0 80 becomes one + * of them here and three on iOS. There is no single behavior worth documenting, so + * the tests pin rule 3 instead. + */ + private fun formFields(parts: List): List = + parts.map { MediaUploadField(it.name, String(it.body.readBytes(), Charsets.UTF_8)) } + // MARK: - Response Building private fun errorResponse(status: Int, message: String): HttpResponse { @@ -528,9 +667,15 @@ internal class MediaUploadServer( internal class MediaUploadException(message: String, cause: Throwable? = null) : Exception(message, cause) /** - * Uploads files to the WordPress REST API using OkHttp. + * GutenbergKit's own client for the configured site, built from the site credentials + * in the editor configuration. + * + * Not an implementation of any host-facing interface — it is the thing that actually + * performs GutenbergKit's media requests. It delivers uploads the host did not take + * over, and relays the editor's media deletes: the editor only ever asks to delete + * `/wp/v2/media/` on the configured site, so that is where the relay sends it. */ -internal open class DefaultMediaUploader( +internal open class InternalMediaClient( private val httpClient: okhttp3.OkHttpClient, private val siteApiRoot: String, private val authHeader: String, @@ -568,10 +713,10 @@ internal open class DefaultMediaUploader( ): MediaUploadResponse { 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 - // field, matching okhttp's String overload byte-for-byte. + // Non-file parts (post, additionalData) have to survive the re-encode unchanged: + // appending raw bytes keeps them byte-for-byte identical to the plain passthrough, + // and filename=null makes each a plain field, exactly what okhttp's String overload + // would emit. (Bad bytes can't get here; see formFields.) for (part in extraParts) { builder.addFormDataPart(part.name, null, part.body.readBytes().toRequestBody()) } @@ -589,7 +734,7 @@ internal open class DefaultMediaUploader( /** * Forwards the original request body to WordPress without re-encoding. * - * Used when the delegate's `processFile` returned the file unchanged — + * Used when the processor's `processFile` returned the file unchanged — * the incoming multipart body is already valid for WordPress. */ open suspend fun passthroughUpload( 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..3242cb576 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt @@ -3,6 +3,7 @@ package org.wordpress.gutenberg import android.os.Looper import android.view.View import kotlinx.coroutines.test.TestScope +import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertThrows @@ -22,10 +23,10 @@ class GutenbergViewUploadServerTest { private val testScope = TestScope() - private fun makeView(): GutenbergView { + private fun makeView(authHeader: String = "Bearer test", siteApiRoot: String = "https://example.com/wp-json/"): GutenbergView { val config = EditorConfiguration - .builder("https://example.com", "https://example.com/wp-json/") - .setAuthHeader("Bearer test") + .builder("https://example.com", siteApiRoot) + .setAuthHeader(authHeader) .build() return GutenbergView( config, @@ -44,7 +45,7 @@ class GutenbergViewUploadServerTest { /** * Invokes the private `onEditorPageStarted` hook (fired from the WebViewClient's * `onPageStarted`) to simulate the editor page beginning to load — the point at - * which the delegate is captured and the upload server starts. + * which the processor is captured and the upload server starts. */ private fun startLoading(view: GutenbergView) { val method = GutenbergView::class.java.getDeclaredMethod("onEditorPageStarted") @@ -62,15 +63,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 +80,14 @@ class GutenbergViewUploadServerTest { } @Test - fun `no delegate means no upload server`() { + fun `no processor means no upload server`() { val view = makeView() try { - // No delegate provided — uploads should use the default WebView path. + // No processor provided — uploads should use the default WebView path. startLoading(view) idle() assertNull( - "with no delegate, no upload server should be started", + "with no processor, no upload server should be started", uploadServerOf(view) ) } finally { @@ -95,15 +96,158 @@ class GutenbergViewUploadServerTest { } @Test - fun `setting the delegate after the page has started loading throws`() { + fun `the upload server starts for an uploader with no processor`() { val view = makeView() try { + // An uploader alone must bring the server up: it is the only route the + // editor has to the host's upload stack. Without this, `startUploadServer` + // could drop the `mediaUploader` clause from its gate and stay green. + view.mediaUploader = mock(MediaUploader::class.java) startLoading(view) idle() - // The delegate is captured at load; a later assignment is a programmer + assertNotNull( + "an uploader provided before load should bring up the upload server", + uploadServerOf(view) + ) + } finally { + detach(view) // stops the server, releasing the bound socket + } + } + + @Test + fun `setting the uploader after the page has started loading throws`() { + val view = makeView() + try { + startLoading(view) + idle() + assertThrows(IllegalStateException::class.java) { + view.mediaUploader = mock(MediaUploader::class.java) + } + } finally { + detach(view) + } + } + + @Test + fun `an uploader without credentials fails at assignment`() { + // Falling back would silently drop the uploader, and its media deletes still + // need the internal media client to reach the configured site. It fails in the + // setter rather than at page load so the stack trace names the host's own line. + val view = makeView(authHeader = "") + try { + assertThrows(IllegalStateException::class.java) { + view.mediaUploader = mock(MediaUploader::class.java) + } + // The failed assignment left nothing behind, so loading still works — + // it just falls to the default WebView path. + startLoading(view) + idle() + assertNull("no server should be left behind by the rejected uploader", uploadServerOf(view)) + } finally { + detach(view) + } + } + + @Test + fun `an uploader without a site root fails at assignment too`() { + val view = makeView(siteApiRoot = "") + try { + assertThrows(IllegalStateException::class.java) { + view.mediaUploader = mock(MediaUploader::class.java) + } + } finally { + detach(view) + } + } + + @Test + fun `an uploader with a scheme-less site root fails at assignment`() { + // What a user types when asked for their site address. This used to pass the + // isEmpty() check and start a server whose every relayed delete threw + // IllegalArgumentException out of OkHttp — while the same config trapped on + // iOS, whose check has always required an absolute root. + val view = makeView(siteApiRoot = "example.com/wp-json/") + try { + assertThrows(IllegalStateException::class.java) { + view.mediaUploader = mock(MediaUploader::class.java) + } + } finally { + detach(view) + } + } + + @Test + fun `a processor with a scheme-less site root leaves the server down`() { + // Same root, no uploader: not an error, but the server must still stay down + // rather than come up and fail every request. (Matches iOS.) + val view = makeView(siteApiRoot = "example.com/wp-json/") + try { + view.mediaProcessor = mock(MediaProcessor::class.java) + startLoading(view) + idle() + assertNull( + "a scheme-less site root should not bring up the server", + uploadServerOf(view) + ) + } finally { + detach(view) + } + } + + @Test + fun `an uploader with credentials assigns cleanly`() { + val view = makeView() + try { + val uploader = mock(MediaUploader::class.java) + view.mediaUploader = uploader + assertEquals(uploader, view.mediaUploader) + } finally { + detach(view) + } + } + + @Test + fun `a processor without credentials assigns cleanly`() { + // Only an uploader requires credentials — a processor has nothing to deliver + // through, so assigning one with no credentials is not an error. + val view = makeView(authHeader = "") + try { + val processor = mock(MediaProcessor::class.java) + view.mediaProcessor = processor + assertEquals(processor, view.mediaProcessor) + } finally { + detach(view) + } + } + + @Test + fun `a processor without credentials just leaves the server down`() { + // Nothing to deliver through, so nothing to process — uploads fall to the + // default WebView path rather than trapping. + val view = makeView(authHeader = "") + try { + view.mediaProcessor = mock(MediaProcessor::class.java) + startLoading(view) + idle() + assertNull( + "a processor with no credentials should not bring up the server", + uploadServerOf(view) + ) + } finally { + detach(view) + } + } + + @Test + fun `setting the processor after the page has started loading throws`() { + val view = makeView() + try { + startLoading(view) + idle() + // The processor is captured at load; a later assignment is a programmer // error and must surface loudly rather than silently do nothing. assertThrows(IllegalStateException::class.java) { - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + view.mediaProcessor = mock(MediaProcessor::class.java) } } finally { detach(view) @@ -113,7 +257,7 @@ class GutenbergViewUploadServerTest { @Test fun `detaching the view stops and clears the upload server`() { val view = makeView() - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + view.mediaProcessor = mock(MediaProcessor::class.java) startLoading(view) idle() assertNotNull(uploadServerOf(view)) diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaServerCredentialsTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaServerCredentialsTest.kt new file mode 100644 index 000000000..6dacbac56 --- /dev/null +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaServerCredentialsTest.kt @@ -0,0 +1,95 @@ +package org.wordpress.gutenberg + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * The counterpart of iOS's `MediaServerCredentialsTests`. The two suites assert the + * same cases on purpose — this policy already diverged silently between the platforms + * once, and matching cases are what makes a future divergence show up as a failing + * test rather than as a crash on one platform and a broken server on the other. + * + * Robolectric is required only because [MediaServerCredentials] parses with + * `android.net.Uri`, which is a framework class. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [28], manifest = Config.NONE) +class MediaServerCredentialsTest { + + private val siteRoot = "https://example.com/wp-json/" + + @Test + fun `accepts an absolute site root with an auth header`() { + assertTrue(MediaServerCredentials.areUsable(siteRoot, "Bearer t")) + } + + @Test + fun `rejects an empty auth header`() { + assertFalse(MediaServerCredentials.areUsable(siteRoot, "")) + } + + // The two arms below are the ones an `isEmpty()` check used to let through. + + @Test + fun `rejects a site root with no scheme`() { + // What a user types when asked for their site address. OkHttp rejects the + // resulting URL with IllegalArgumentException, which is not an IOException. + assertFalse(MediaServerCredentials.areUsable("example.com/wp-json/", "Bearer t")) + } + + @Test + fun `rejects a site root with no host`() { + assertFalse(MediaServerCredentials.areUsable("file:///tmp/wp-json", "Bearer t")) + } + + @Test + fun `rejects an empty site root, the default when a host configures none`() { + assertFalse(MediaServerCredentials.areUsable("", "Bearer t")) + } + + // MARK: - requireCredentialsForUploader + + @Test + fun `accepts usable credentials, uploader or not`() { + for (hasUploader in listOf(true, false)) { + MediaServerCredentials.requireCredentialsForUploader(siteRoot, "Bearer t", hasUploader) + } + } + + @Test + fun `ignores missing credentials when there is no uploader`() { + // Nothing to deliver through, so nothing to process. This is not an error — the + // server just stays down (areUsable decides that) and uploads fall to the + // default WebView path, so a processor-only host must not fail here. + MediaServerCredentials.requireCredentialsForUploader(siteRoot, "", hasUploader = false) + } + + @Test + fun `throws for an uploader with no auth header`() { + assertThrows(IllegalStateException::class.java) { + MediaServerCredentials.requireCredentialsForUploader(siteRoot, "", hasUploader = true) + } + } + + @Test + fun `throws for an uploader with no site root`() { + assertThrows(IllegalStateException::class.java) { + MediaServerCredentials.requireCredentialsForUploader("", "Bearer t", hasUploader = true) + } + } + + @Test + fun `throws for an uploader with a scheme-less site root`() { + // The case that previously trapped on iOS and started a doomed server here. + assertThrows(IllegalStateException::class.java) { + MediaServerCredentials.requireCredentialsForUploader( + "example.com/wp-json/", "Bearer t", hasUploader = true + ) + } + } +} 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 e7d10e942..ff22f8c1f 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -33,7 +33,7 @@ class MediaUploadServerTest { @Before fun setUp() { - server = MediaUploadServer(uploadDelegate = null, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = null, internalClient = null, cacheDir = tempFolder.root) } @After @@ -53,7 +53,7 @@ class MediaUploadServerTest { fun `stop cancels an internally-created scope but leaves a caller-supplied one alone`() { // No scope supplied → the server owns one, which stop() must cancel. val owningServer = - MediaUploadServer(uploadDelegate = null, defaultUploader = null, cacheDir = tempFolder.root) + MediaUploadServer(processor = null, internalClient = null, cacheDir = tempFolder.root) val ownedScope = ownedScopeOf(owningServer) assertNotNull("server should own a scope when none is supplied", ownedScope) assertTrue(ownedScope!!.isActive) @@ -63,8 +63,8 @@ class MediaUploadServerTest { // A caller-supplied scope belongs to the caller — stop() must not cancel it. val callerScope = CoroutineScope(Dispatchers.IO) val borrowingServer = MediaUploadServer( - uploadDelegate = null, - defaultUploader = null, + processor = null, + internalClient = null, cacheDir = tempFolder.root, scope = callerScope ) @@ -150,9 +150,9 @@ class MediaUploadServerTest { // Exercised through the delete relay because every response relayResponse // handles — WordPress's own included — carries a `Content-Type`, so this is // the ordinary path rather than an edge case. - val uploader = ContentTypeDeleteUploader() + val uploader = ContentTypeDeleteClient() server.stop() - server = MediaUploadServer(uploadDelegate = null, defaultUploader = uploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = null, internalClient = uploader, cacheDir = tempFolder.root) val response = sendRawRequest( method = "DELETE", @@ -168,12 +168,230 @@ class MediaUploadServerTest { assertEquals(listOf("text/plain"), response.rawHeaderValues("content-type")) } + @Test + fun `an uploader performs the upload and its result is relayed`() { + val uploader = RecordingUploader() + val client = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + processor = null, internalClient = client, uploader = uploader, cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-uploader" + 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 201 but got: ${response.statusLine}", response.statusLine.contains("201")) + assertTrue(response.body.contains("\"id\":7")) + // GutenbergKit stays out of the network when a host uploader is set. + assertFalse(client.uploadCalled) + assertFalse(client.passthroughUploadCalled) + assertEquals("photo.jpg", uploader.received?.filename) + assertEquals("image/jpeg", uploader.received?.mimeType) + } + + @Test + fun `an uploader receives the editor's form fields in order, and the query`() { + // Without `post` the attachment is created unattached, and repeated names (a + // `field[]` array) must survive as repeats rather than collapse into a map. + val uploader = RecordingUploader() + server.stop() + server = MediaUploadServer( + processor = null, internalClient = MockInternalMediaClient(), uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-fields" + val body = java.io.ByteArrayOutputStream().apply { + for ((name, value) in listOf("post" to "42", "tags[]" to "a", "tags[]" to "b")) { + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"$name\"\r\n\r\n".toByteArray()) + write("$value\r\n".toByteArray()) + } + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n".toByteArray()) + write("Content-Type: image/jpeg\r\n\r\n".toByteArray()) + write("fake image data".toByteArray()) + write("\r\n--$boundary--\r\n".toByteArray()) + }.toByteArray() + + sendRawRequest( + method = "POST", + path = "/upload?_embed=wp:featuredmedia", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertEquals( + listOf( + MediaUploadField("post", "42"), + MediaUploadField("tags[]", "a"), + MediaUploadField("tags[]", "b") + ), + uploader.received?.fields + ) + assertEquals("?_embed=wp:featuredmedia", uploader.received?.query) + } + + @Test + fun `a processor still processes the file an uploader delivers`() { + // With both set, the processor still processes — only delivery moves to + // the uploader. + val uploader = RecordingUploader() + val processor = ProcessOnlyProcessor() + val client = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + processor = processor, internalClient = client, uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-precedence" + val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "data".toByteArray()) + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertNotNull(uploader.received) + assertTrue(processor.processFileCalled) + assertFalse(client.uploadCalled) + } + + @Test + fun `keeps a binary Blob part out of an uploader's fields`() { + // Pin rule 3: a Blob always has a filename, so it's dropped before the decode. + val uploader = RecordingUploader() + server.stop() + server = MediaUploadServer( + processor = null, internalClient = MockInternalMediaClient(), uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-blob" + val body = java.io.ByteArrayOutputStream().apply { + // Ordered as uploadToServer emits it: the file first, then additionalData. + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n".toByteArray()) + write("Content-Type: image/jpeg\r\n\r\n".toByteArray()) + write("fake image data".toByteArray()) + write("\r\n--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"post\"\r\n\r\n".toByteArray()) + write("42\r\n".toByteArray()) + // A Blob-shaped part: it has a filename, and its bytes are not valid UTF-8. + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"blob\"; filename=\"blob\"\r\n".toByteArray()) + write("Content-Type: application/octet-stream\r\n\r\n".toByteArray()) + write(byteArrayOf(0xED.toByte(), 0xA0.toByte(), 0x80.toByte())) + write("\r\n--$boundary--\r\n".toByteArray()) + }.toByteArray() + + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + // The Blob is dropped rather than decoded, and file is still the file. + assertEquals("photo.jpg", uploader.received?.filename) + assertEquals(listOf(MediaUploadField("post", "42")), uploader.received?.fields) + } + + @Test + fun `round-trips a non-Latin field value exactly`() { + // The other half: valid UTF-8 round-trips, so real captions and titles survive. + val uploader = RecordingUploader() + server.stop() + server = MediaUploadServer( + processor = null, internalClient = MockInternalMediaClient(), uploader = uploader, + cacheDir = tempFolder.root + ) + + val caption = "Grüße 🎉 日本語" + val boundary = "test-boundary-utf8" + val body = java.io.ByteArrayOutputStream().apply { + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"caption\"\r\n\r\n".toByteArray()) + write("$caption\r\n".toByteArray()) + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n".toByteArray()) + write("Content-Type: image/jpeg\r\n\r\n".toByteArray()) + write("fake image data".toByteArray()) + write("\r\n--$boundary--\r\n".toByteArray()) + }.toByteArray() + + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertEquals(listOf(MediaUploadField("caption", caption)), uploader.received?.fields) + } + + @Test + fun `an uploader sees a file the processor's metadata gate would have declined`() { + // The gate exists to skip a temp copy for a file the processor won't touch. An + // uploader takes over delivery for every file, so passing through here would + // silently bypass it. + val uploader = RecordingUploader() + val client = MockInternalMediaClient() + val processor = DeclineByMetadataProcessor() + server.stop() + server = MediaUploadServer( + processor = processor, internalClient = client, uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-declined" + val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertEquals("clip.mov", uploader.received?.filename) + assertFalse(client.passthroughUploadCalled) + // ...but a declined file must still not reach processFile: handlesFile + // returning false is the processor saying it won't touch a file like this. + assertFalse(processor.processFileCalled) + } + @Test fun `routes upload with a query string and relays the query`() { - val delegate = ProcessOnlyDelegate() - val mockUploader = MockDefaultUploader() + val processor = ProcessOnlyProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, 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 @@ -192,7 +410,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) @@ -200,13 +418,14 @@ class MediaUploadServerTest { assertEquals("?_embed=wp:featuredmedia", mockUploader.lastQuery) } - // MARK: - Upload with delegate + // MARK: - Upload with processor @Test - fun `calls delegate processFile and uploadFile`() { - val delegate = MockUploadDelegate() + fun `processes with the processor, then delivers through the internal client`() { + val processor = TranscodingProcessor() + val client = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = client, cacheDir = tempFolder.root) val boundary = "test-boundary-123" val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) @@ -222,24 +441,22 @@ 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 processor only transforms; GutenbergKit performs the upload. + assertTrue(client.uploadCalled) // The server relays WordPress's raw response body verbatim. val json = JsonParser.parseString(response.body).asJsonObject - assertEquals(42, json.get("id").asInt) - assertEquals("https://example.com/photo.jpg", json.get("source_url").asString) - assertEquals("image", json.get("media_type").asString) + assertEquals(99, json.get("id").asInt) + assertEquals("https://example.com/doc.pdf", json.get("source_url").asString) + assertEquals("file", json.get("media_type").asString) } @Test - fun `forwards the delegate's processed metadata to the uploader`() { - val delegate = TranscodingDelegate() - val mockUploader = MockDefaultUploader() + fun `forwards the processor's processed metadata to the uploader`() { + val processor = TranscodingProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-meta" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -254,7 +471,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) @@ -262,11 +479,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, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-cleanup" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -281,10 +498,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()) } @@ -304,8 +521,8 @@ 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, + internalClient = null, cacheDir = tempFolder.root, ioDispatcher = Dispatchers.Unconfined ) @@ -314,15 +531,15 @@ class MediaUploadServerTest { assertTrue("Fresh temp should be preserved", fresh.exists()) } - // MARK: - Fallback to default uploader + // MARK: - Fallback to the internal media client @Test - fun `uses passthrough when delegate does not modify file`() { - val delegate = ProcessOnlyDelegate() - val mockUploader = MockDefaultUploader() + fun `uses passthrough when processor does not modify file`() { + val processor = ProcessOnlyProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-456" val body = buildMultipartBody(boundary, "doc.pdf", "application/pdf", "fake pdf data".toByteArray()) @@ -338,7 +555,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) @@ -348,12 +565,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 = DeclineByMetadataProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-decline" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "fake movie".toByteArray()) @@ -369,17 +586,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"}""" @@ -392,7 +609,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" @@ -417,13 +634,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" @@ -442,7 +659,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 @@ -457,7 +674,7 @@ class MediaUploadServerTest { ) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -475,12 +692,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", @@ -498,12 +715,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", @@ -535,7 +752,7 @@ class MediaUploadServerTest { ) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -563,12 +780,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" @@ -603,7 +820,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" @@ -756,27 +973,7 @@ class MediaUploadServerTest { // MARK: - Mocks - private class MockUploadDelegate : MediaUploadDelegate { - @Volatile var processFileCalled = false - @Volatile var uploadFileCalled = false - @Volatile var lastMimeType: String? = null - @Volatile var lastFilename: String? = 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()) - } - } - - private class ProcessOnlyDelegate : MediaUploadDelegate { + private class ProcessOnlyProcessor : MediaProcessor { @Volatile var processFileCalled = false override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { @@ -786,10 +983,11 @@ class MediaUploadServerTest { } /** - * Declines every file by metadata via [handlesFile], so the server must pass - * through without materializing the file or calling [processFile]. + * Declines every file by metadata via [handlesFile]. With no uploader the server + * must pass through without materializing the file; with one, delivery still + * happens but [processFile] must not be called. [processFileCalled] pins both. */ - private class DeclineByMetadataDelegate : MediaUploadDelegate { + private class DeclineByMetadataProcessor : MediaProcessor { @Volatile var processFileCalled = false override fun handlesFile(mimeType: String, filename: String): Boolean = false @@ -800,9 +998,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 { @@ -814,11 +1012,11 @@ class MediaUploadServerTest { } /** - * A default uploader whose delete response carries its own `Content-Type`, + * An internal media client whose delete response carries its own `Content-Type`, * lowercased, so the relay must override the JSON default rather than emit * the header twice. */ - private class ContentTypeDeleteUploader : DefaultMediaUploader( + private class ContentTypeDeleteClient : InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = "https://example.com/wp-json/", authHeader = "Bearer mock" @@ -830,7 +1028,22 @@ class MediaUploadServerTest { ) } - private class MockDefaultUploader : DefaultMediaUploader( + /** Records the [MediaUpload] it is handed, and returns a finished attachment. */ + private class RecordingUploader : MediaUploader { + @Volatile var received: MediaUpload? = null + + override suspend fun upload(upload: MediaUpload): ByteArray { + received = upload + // Shaped like a real attachment: the editor's `transformAttachment` + // reads `title.raw`, so an example without it would model a body that + // fails in the editor. + val attachment = """{"id":7,"source_url":"https://example.com/photo.jpg",""" + + """"media_type":"image","title":{"raw":"photo"},"caption":{"raw":""}}""" + return attachment.toByteArray() + } + } + + private class MockInternalMediaClient : InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = "https://example.com/wp-json/", authHeader = "Bearer mock" 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 92% 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..b50477dd2 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt @@ -5,24 +5,24 @@ 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. + * Demo media processor that resizes images to a maximum dimension of 2000px. * - * Only overrides [processFile] — [uploadFile] returns null so the default uploader is used. + * Only transforms the file; GutenbergKit performs the upload. */ -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 // everything else by metadata — the server then skips copying a file this - // delegate would only pass through. + // processor would only pass through. override fun handlesFile(mimeType: String, filename: String): Boolean { return mimeType.startsWith("image/") && mimeType != "image/gif" } 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/docs/code/physical-device-setup.md b/docs/code/physical-device-setup.md index 5a081dd06..271b0f0f2 100644 --- a/docs/code/physical-device-setup.md +++ b/docs/code/physical-device-setup.md @@ -64,7 +64,7 @@ Look for your local network IP address (typically in the format `192.168.x.x` or ### 2. Modify Network Security Configuration -Android requires explicit network security configuration to allow cleartext (http) traffic to non-localhost addresses. +Android requires explicit network security configuration to allow cleartext (http) traffic. The demo app's config already covers `localhost` and the emulator's `10.0.2.2` alias; a development machine reached over the LAN needs its own entry. **Temporarily** modify `android/app/src/main/res/xml/network_security_config.xml` to include your development machine's IP address: @@ -72,6 +72,9 @@ Android requires explicit network security configuration to allow cleartext (htt + + localhost + 127.0.0.1 10.0.2.2 192.168.1.100 diff --git a/docs/integration.md b/docs/integration.md index 4ac4e2ea5..a40caba38 100644 --- a/docs/integration.md +++ b/docs/integration.md @@ -246,33 +246,34 @@ val configuration = EditorConfiguration.builder() ## Media Handling -The host can customize how media is processed and uploaded by supplying a -`MediaUploadDelegate` at init: +The host can transform media before upload by supplying a `MediaProcessor` at init. +To take over the upload itself, supply a `MediaUploader` instead — a processor only +changes bytes; GutenbergKit still delivers them. ```swift let editor = EditorViewController( configuration: configuration, - mediaUploadDelegate: ResizingDelegate(maxDimension: 2000) + mediaProcessor: ResizingProcessor(maxDimension: 2000) ) ``` ### Don't conform the object that owns the editor -GutenbergKit never hands your delegate the editor: every value crossing that boundary is a -value type — a file URL, a MIME type, a filename. So a delegate can only reach the editor +GutenbergKit never hands your processor the editor: every value crossing that boundary is a +value type — a file URL, a MIME type, a filename. So a processor can only reach the editor if you put it there. That happens when you conform the object that already holds the editor in order to drive -it. The editor holds the delegate strongly in return — deliberately, so an in-flight upload +it. The editor holds the processor strongly in return — deliberately, so an in-flight upload can't lose it mid-request — which closes a retain cycle ARC cannot break. The editor is never deallocated, and each one strands a bound loopback listener. ```swift -// Leaks: coordinator -> editor -> mediaUploadDelegate -> coordinator -final class PostEditorCoordinator: MediaUploadDelegate { +// Leaks: coordinator -> editor -> mediaProcessor -> coordinator +final class PostEditorCoordinator: MediaProcessor { var editor: EditorViewController! init(blog: Blog, configuration: EditorConfiguration) { - editor = EditorViewController(configuration: configuration, mediaUploadDelegate: self) + editor = EditorViewController(configuration: configuration, mediaProcessor: self) } } ``` @@ -287,7 +288,7 @@ final class PostEditorCoordinator { init(blog: Blog, configuration: EditorConfiguration) { editor = EditorViewController( configuration: configuration, - mediaUploadDelegate: BlogMediaDelegate(siteID: blog.dotComID, maxDimension: 2000) + mediaProcessor: BlogMediaProcessor(siteID: blog.dotComID, maxDimension: 2000) ) } } @@ -298,12 +299,72 @@ are finished with the editor. It is terminal — the editor cannot upload or del afterwards — so call it when the editor is going away, not when it is merely covered or backgrounded. -### Reusing a delegate across editor sessions +### Android: permit cleartext to localhost -The editor holds the delegate for its lifetime and releases it when it goes, so a delegate +**Android hosts must add localhost to their network security configuration, or native +media handling will silently not run.** + +GutenbergKit serves media through a loopback HTTP server, which the editor reaches over +cleartext `http://localhost`. Apps targeting API 28 or above deny cleartext by default, so +without an entry the WebView blocks every upload request with +`ERR_CLEARTEXT_NOT_PERMITTED` before it leaves the page. `GutenbergView` detects this and +leaves the server down, so uploads fall back to the WebView's own path rather than failing +against a server they can never reach. + +The failure is quiet by design — media still uploads — so the symptom is that your +`MediaProcessor` or `MediaUploader` is simply never called. The only signal is a warning +in logcat: + +``` +Cleartext to localhost is not permitted, so the native media upload server can't be +reached from the WebView. Permit cleartext to localhost in the app's network security +config to enable native media processing. +``` + +Add a `domain-config` to the file referenced by your ``'s +`android:networkSecurityConfig`: + +```xml + + + + localhost + 127.0.0.1 + + +``` + +This narrows cleartext to loopback only. It does not permit cleartext anywhere else — the +rest of the app keeps whatever `base-config` (or the platform default) already applies. + +#### Why GutenbergKit can't ship this for you + +`android:networkSecurityConfig` is a single-valued attribute on ``: an app +has exactly one, and the XML files do not merge. A library that declares it collides with +the host's, and the manifest merger fails the build until the app adds +`tools:replace="android:networkSecurityConfig"` — which then discards the library's +version entirely. It also collides with _other_ libraries that declare one; the WordPress +Rust API client already does. And for a host that has no config of its own, a library's +file would silently become the app's entire network security policy, replacing any +certificate pinning or trust anchors it would otherwise have had. + +So the attribute has to be the app's. Only the app can arbitrate between the libraries +that want a say in it. + +#### Devices running Android 16 and above + +API 36 added an implicit cleartext-permitted configuration for localhost, applied when the +app's own config does not already name it. On those devices native media handling works +without the entry above. GutenbergKit supports API 24 and up, so the entry is still +required in practice — and it remains correct on Android 16, where naming localhost +explicitly simply takes precedence over the implicit one. + +### Reusing a processor across editor sessions + +The editor holds the processor for its lifetime and releases it when it goes, so a processor built for a single editor needs no reference of its own. To use the same instance for several editors, keep your own reference — the editor drops only its own. Sharing is also -the safer shape: a delegate owned by something longer-lived than any editor is a leaf, so +the safer shape: a processor owned by something longer-lived than any editor is a leaf, so it cannot form the cycle above and there is nothing to tear down. It may be called concurrently if more than one editor is live, and it must not hold on to any editor it has served. diff --git a/ios/Demo-iOS/Sources/Views/EditorList.swift b/ios/Demo-iOS/Sources/Views/EditorList.swift index d637858be..7a5449bc2 100644 --- a/ios/Demo-iOS/Sources/Views/EditorList.swift +++ b/ios/Demo-iOS/Sources/Views/EditorList.swift @@ -8,6 +8,7 @@ struct EditorList: View { @State private var showAddDialog = false @State private var showDebugSettings = false @State private var showMediaProxyServer = false + @State private var showUploadServerDiagnostic = false @State var configurationToDelete: ConfigurationItem? @State private var errorMessage: String? @@ -91,6 +92,9 @@ struct EditorList: View { .navigationDestination(isPresented: $showMediaProxyServer) { MediaProxyServerView() } + .navigationDestination(isPresented: $showUploadServerDiagnostic) { + UploadServerDiagnosticView() + } .navigationTitle("GutenbergKit") .toolbar { ToolbarItem(placement: .primaryAction) { @@ -109,6 +113,12 @@ struct EditorList: View { Label("Media Proxy Server", systemImage: "server.rack") } + Button { + showUploadServerDiagnostic = true + } label: { + Label("Upload Server Diagnostic", systemImage: "stethoscope") + } + Button { showDebugSettings = true } label: { diff --git a/ios/Demo-iOS/Sources/Views/EditorView.swift b/ios/Demo-iOS/Sources/Views/EditorView.swift index f2104dacd..8d358c48a 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, - mediaUploadDelegate: enableNativeMediaUpload ? context.coordinator : nil + mediaProcessor: enableNativeMediaUpload ? context.coordinator : nil ) viewController.delegate = context.coordinator viewController.webView.isInspectable = true @@ -190,7 +190,7 @@ private struct _EditorView: UIViewControllerRepresentable { } @MainActor - class Coordinator: NSObject, EditorViewControllerDelegate, MediaUploadDelegate { + class Coordinator: NSObject, EditorViewControllerDelegate, MediaProcessor { let viewModel: EditorViewModel init(viewModel: EditorViewModel) { @@ -296,11 +296,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/Demo-iOS/Sources/Views/UploadServerDiagnosticView.swift b/ios/Demo-iOS/Sources/Views/UploadServerDiagnosticView.swift new file mode 100644 index 000000000..675b86f7c --- /dev/null +++ b/ios/Demo-iOS/Sources/Views/UploadServerDiagnosticView.swift @@ -0,0 +1,395 @@ +import SwiftUI +import UIKit +import Network +import OSLog +import GutenbergKitHTTP + +/// Diagnostic for the "backgrounded upload server loses its socket" behaviour. +/// +/// The editor's media upload server is a loopback ``HTTPServer``. When the device becomes +/// eligible for idle sleep — e.g. the phone is locked while unplugged and left to idle — +/// iOS reclaims the listening socket out from under a suspended app: connections are then +/// refused even though `NWListener` still reports `.ready`, so nothing is logged and the +/// editor returns advertising a dead port. `EditorViewController` recovers by re-checking +/// the port on `willEnterForeground` and restarting the server if it stopped answering. +/// +/// This screen exercises the same shape with a standalone ``HTTPServer`` so the behaviour +/// and the recovery can be validated on a device: +/// +/// - **Live monitor** — start the server, then lock the phone (unplugged, left to idle) and +/// reopen. The monitor reports whether the socket was reclaimed while away, and if so +/// restarts it and confirms it answers again. +/// - **Self-test** — stops the server to stand in for the OS reclaiming the socket, confirms +/// it stops answering, restarts it, and confirms it answers on the new port. This proves +/// the recovery path deterministically, without needing to lock the phone. +@MainActor +final class UploadServerDiagnostic: ObservableObject { + enum Reachability { case unknown, reachable, unreachable } + + struct Outcome: Identifiable { + let id = UUID() + let text: String + let passed: Bool + } + + @Published private(set) var reachability: Reachability = .unknown + @Published private(set) var port: UInt16 = 0 + @Published private(set) var power = "" + @Published private(set) var awaySeconds = "" + @Published private(set) var events: [String] = [] + @Published private(set) var outcomes: [Outcome] = [] + @Published private(set) var isRunningSelfTest = false + @Published private(set) var uploadSimulationActive = false + @Published private(set) var backgroundTimeRemaining = "" + + private var server: HTTPServer? + private var timer: Timer? + private var observers: [NSObjectProtocol] = [] + private var backgroundedAt: Date? + /// Signalled to release the simulated upload's expiring activity. + private var uploadActivityRelease: DispatchSemaphore? + + // MARK: - Lifecycle + + func onAppear() { + UIDevice.current.isBatteryMonitoringEnabled = true + Task { await startServer(reason: "initial start") } + + let timer = Timer(timeInterval: 1, repeats: true) { [weak self] _ in + Task { @MainActor in await self?.refresh() } + } + RunLoop.main.add(timer, forMode: .common) + self.timer = timer + + observers.append(NotificationCenter.default.addObserver( + forName: UIApplication.didEnterBackgroundNotification, object: nil, queue: .main + ) { [weak self] _ in + Task { @MainActor in self?.backgroundedAt = Date() } + }) + observers.append(NotificationCenter.default.addObserver( + forName: UIApplication.willEnterForegroundNotification, object: nil, queue: .main + ) { [weak self] _ in + Task { @MainActor in await self?.handleForeground() } + }) + } + + func onDisappear() { + timer?.invalidate() + timer = nil + observers.forEach(NotificationCenter.default.removeObserver) + observers.removeAll() + endUploadSimulation() + server?.stop() + server = nil + } + + // MARK: - Active-upload simulation (background-task assertion) + + /// Holds a `ProcessInfo` expiring activity — the same primitive the editor's + /// upload path holds while relaying a media upload. With it held, locking the phone keeps + /// the app running for the system's grace period (~30s) instead of suspending it, so a + /// short upload finishes and the loopback socket survives a brief lock. + func toggleUploadSimulation() { + uploadSimulationActive ? endUploadSimulation() : beginUploadSimulation() + } + + private func beginUploadSimulation() { + // The activity lasts as long as the block runs, so the block waits on `release`. + // If the system expires the activity, or can't grant it, it calls the block with + // `expired` set, and that call lets any waiting one return. + let release = DispatchSemaphore(value: 0) + uploadActivityRelease = release + uploadSimulationActive = true + ProcessInfo.processInfo.performExpiringActivity(withReason: "diagnostic-upload") { [weak self] expired in + guard expired else { + release.wait() + return + } + release.signal() + Task { @MainActor in + self?.log("The system expired the background-task assertion.") + self?.endUploadSimulation() + } + } + log("Simulated upload started — holding a background-task assertion. Lock the phone now; the app should keep running (~30s) and the socket should stay ALIVE.") + } + + private func endUploadSimulation() { + guard uploadSimulationActive else { return } + uploadActivityRelease?.signal() + uploadActivityRelease = nil + uploadSimulationActive = false + backgroundTimeRemaining = "" + log("Simulated upload ended — assertion released.") + } + + // MARK: - Live monitor + + /// Runs when the app returns to the foreground — the same moment the editor's fix runs. + private func handleForeground() async { + let away = backgroundedAt.map { Int(Date().timeIntervalSince($0)) } + let awayText = away.map { "\($0)s" } ?? "?" + // Freeze the away time now and stop the running counter — the timer is suspended + // while backgrounded, so leaving `backgroundedAt` set would make it climb in the + // foreground. + awaySeconds = away != nil ? awayText : awaySeconds + backgroundedAt = nil + guard let server else { return } + + if await isAnswering(port: server.port) { + log("Returned after \(awayText): still answering — not reclaimed this cycle.") + return + } + + log("Returned after \(awayText): port \(server.port) refused — the OS reclaimed the socket while away.") + await startServer(reason: "recovery after reclamation") + if let restarted = self.server, await isAnswering(port: restarted.port) { + record("Reclaimed after \(awayText), recovered on port \(restarted.port)", passed: true) + } else { + record("Reclaimed after \(awayText), but recovery failed", passed: false) + } + } + + private func refresh() async { + guard let server else { reachability = .unknown; return } + reachability = await isAnswering(port: server.port) ? .reachable : .unreachable + power = powerLine() + if uploadSimulationActive { + let remaining = UIApplication.shared.backgroundTimeRemaining + backgroundTimeRemaining = remaining > 1_000_000 ? "∞ (foreground)" : "\(Int(remaining))s" + } + } + + // MARK: - Self-test + + /// Proves the recovery path without waiting for the OS: stop the server (standing in for + /// the OS reclaiming the socket), confirm it stops answering, restart, confirm it answers. + func runSelfTest() async { + guard !isRunningSelfTest else { return } + isRunningSelfTest = true + defer { isRunningSelfTest = false } + + if server == nil { await startServer(reason: "self-test start") } + guard let original = server, await isAnswering(port: original.port) else { + record("Self-test: server was not answering at the start", passed: false) + return + } + + original.stop() + server = nil + guard await stoppedAnswering(port: original.port) else { + record("Self-test: server kept answering after stop", passed: false) + return + } + log("Self-test: server on port \(original.port) stopped answering (stands in for OS reclamation).") + + await startServer(reason: "self-test recovery") + guard let restarted = server, await isAnswering(port: restarted.port) else { + record("Self-test: did not recover after restart", passed: false) + return + } + record("Self-test: stopped → restarted → answering on port \(restarted.port)", passed: true) + } + + // MARK: - Server + + private func startServer(reason: String) async { + server?.stop() + do { + let server = try await HTTPServer.start(name: "upload-diagnostic", requiresAuthentication: false) { _ in + HTTPResponse(status: 200, body: Data("ok".utf8)) + } + self.server = server + self.port = server.port + log("Server \(reason): listening on port \(server.port).") + } catch { + log("Server \(reason): failed to start — \(error).") + } + } + + /// A stopped listener can keep answering briefly, because `cancel()` completes on the + /// listener's own queue. Poll rather than race it. + private func stoppedAnswering(port: UInt16) async -> Bool { + for _ in 0..<20 { + if await !isAnswering(port: port) { return true } + try? await Task.sleep(for: .milliseconds(100)) + } + return false + } + + // MARK: - Reachability probe + + /// Sends a real request over `NWConnection` (not a bare connect, which the server would + /// log as a dropped connection, and not `URLSession`, so App Transport Security can't + /// decide the outcome). Any response means the socket is still there. + private func isAnswering(port: UInt16, timeout: Duration = .seconds(2)) async -> Bool { + guard let endpointPort = NWEndpoint.Port(rawValue: port) else { return false } + let connection = NWConnection(host: .ipv4(.loopback), port: endpointPort, using: .tcp) + let deadline = Task { + try await Task.sleep(for: timeout) + connection.cancel() + } + defer { + deadline.cancel() + connection.cancel() + } + return await withCheckedContinuation { continuation in + let once = OnceFlag() + connection.stateUpdateHandler = { state in + switch state { + case .ready: + let request = Data("GET / HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n".utf8) + connection.send(content: request, completion: .contentProcessed { error in + guard error == nil else { + if once.claim() { continuation.resume(returning: false) } + return + } + connection.receive(minimumIncompleteLength: 1, maximumLength: 64) { data, _, _, error in + let answered = error == nil && !(data ?? Data()).isEmpty + if once.claim() { continuation.resume(returning: answered) } + } + }) + // A refused connection waits in `.waiting(ECONNREFUSED)` rather than failing; + // on loopback there's no path change to wait for, so it means dead. + case .waiting, .failed, .cancelled: + if once.claim() { continuation.resume(returning: false) } + default: + break + } + } + connection.start(queue: Self.probeQueue) + } + } + + private static let probeQueue = DispatchQueue(label: "com.gutenbergkit.demo.upload-diagnostic-probe") + + private final class OnceFlag: @unchecked Sendable { + private let lock = NSLock() + private var claimed = false + func claim() -> Bool { + lock.lock() + defer { lock.unlock() } + if claimed { return false } + claimed = true + return true + } + } + + // MARK: - Helpers + + private func powerLine() -> String { + let state: String + switch UIDevice.current.batteryState { + case .charging: state = "charging" + case .full: state = "plugged in" + case .unplugged: state = "unplugged" + default: state = "unknown power" + } + let lowPower = ProcessInfo.processInfo.isLowPowerModeEnabled ? ", Low Power" : "" + return "\(state)\(lowPower)" + } + + private func log(_ line: String) { + let stamped = "\(Date().formatted(date: .omitted, time: .standard)) \(line)" + events.insert(stamped, at: 0) + if events.count > 100 { events.removeLast() } + } + + private func record(_ text: String, passed: Bool) { + outcomes.insert(Outcome(text: text, passed: passed), at: 0) + log((passed ? "PASS — " : "FAIL — ") + text) + } +} + +struct UploadServerDiagnosticView: View { + @StateObject private var model = UploadServerDiagnostic() + + var body: some View { + List { + Section { + HStack { + Circle().fill(statusColor).frame(width: 12, height: 12) + Text(statusText).font(.headline) + Spacer() + Text("port \(model.port)").font(.caption).monospaced().foregroundStyle(.secondary) + } + LabeledContent("Power", value: model.power.isEmpty ? "—" : model.power) + if !model.awaySeconds.isEmpty { + LabeledContent("Last time away", value: model.awaySeconds) + } + } header: { + Text("Upload server socket") + } footer: { + Text("Lock the phone (unplugged, left to idle) and reopen. If iOS reclaimed the socket, the monitor restarts it and confirms it answers again.") + } + + Section("Self-test") { + Button { + Task { await model.runSelfTest() } + } label: { + HStack { + Text("Run recovery self-test") + Spacer() + if model.isRunningSelfTest { ProgressView() } + } + } + .disabled(model.isRunningSelfTest) + } + + Section { + Toggle("Simulate an active upload", isOn: Binding( + get: { model.uploadSimulationActive }, + set: { _ in model.toggleUploadSimulation() } + )) + if model.uploadSimulationActive, !model.backgroundTimeRemaining.isEmpty { + LabeledContent("Background time remaining", value: model.backgroundTimeRemaining) + } + } header: { + Text("Active upload") + } footer: { + Text("Holds the same background-task assertion the editor holds while relaying an upload. With it on, lock the phone: the app keeps running for the grace period, so a short upload finishes and the socket survives the brief lock.") + } + + if !model.outcomes.isEmpty { + Section("Results") { + ForEach(model.outcomes) { outcome in + HStack(alignment: .top) { + Image(systemName: outcome.passed ? "checkmark.circle.fill" : "xmark.circle.fill") + .foregroundStyle(outcome.passed ? .green : .red) + Text(outcome.text).font(.callout) + } + } + } + } + + if !model.events.isEmpty { + Section("Log") { + ForEach(Array(model.events.enumerated()), id: \.offset) { _, line in + Text(line).font(.system(size: 12, design: .monospaced)) + .foregroundStyle(.secondary) + } + } + } + } + .navigationTitle("Upload Server Diagnostic") + .navigationBarTitleDisplayMode(.inline) + .onAppear { model.onAppear() } + .onDisappear { model.onDisappear() } + } + + private var statusColor: Color { + switch model.reachability { + case .reachable: return .green + case .unreachable: return .red + case .unknown: return .gray + } + } + + private var statusText: String { + switch model.reachability { + case .reachable: return "Answering" + case .unreachable: return "Not answering" + case .unknown: return "Starting…" + } + } +} diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 1f01737d8..b242f8983 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -84,7 +84,6 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// The fetched or provided editor dependencies (settings, assets, preload data). private var dependencies: EditorDependencies? - private var dependencyTaskHandle: Task? /// Error encountered while loading dependencies. private var error: Error? { @@ -104,44 +103,66 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// Used by `EditorViewController.warmup()` to reduce first-render latency. private let isWarmupMode: Bool - /// Customizes media file processing and upload behavior. + /// Transforms media before upload — resize, transcode, strip EXIF. + /// + /// To perform the upload yourself, pass a ``mediaUploader`` instead. /// /// Supplied at `init`, with the rest of the editor's configuration, because that is - /// when it takes effect: the delegate is captured into the page's initial + /// when it takes effect: the processor is captured into the page's initial /// configuration as the editor begins loading. Taking it there rather than through a /// settable property leaves no window in which a host can hand one over too late for /// it to ever run. (Android keeps a settable property and a fail-fast for exactly /// that case — a `View` is inflated, not constructed by the host, so there is no /// initializer to put this in.) /// - /// The editor holds this strongly for its lifetime, so a delegate built for a single + /// The rest of this describes a **reference-type** conformer, which is what a host + /// that needs to observe or reuse its processor will write. ``MediaProcessor`` is not + /// class-bound, and a value-type conformer is copied at `init` — see the protocol's + /// documentation for what that changes. + /// + /// The editor holds this strongly for its lifetime, so a processor built for a single /// editor needs no reference of its own. **To reuse one across editor sessions, keep /// your own reference to it.** The editor's release — on `deinit`, or on - /// ``stopMediaHandling()`` — drops only *its* reference: a delegate the host still + /// ``stopMediaHandling()`` — drops only *its* reference: a processor the host still /// holds survives to be passed to the next editor, and one nobody else holds does not. /// /// That release is not always prompt, and not always on the main thread. A request in - /// flight holds its own reference until it unwinds, so if this editor is the delegate's - /// last owner, the delegate is freed when the host's `processFile` returns — on the + /// flight holds its own reference until it unwinds, so if this editor is the processor's + /// last owner, the processor is freed when the host's `processFile` returns — on the /// task's executor, not the caller's thread. Keep a reference of your own if that /// matters to the conformer. /// - /// Sharing an instance is the safer shape rather than a compromise. A delegate owned + /// Sharing an instance is the safer shape rather than a compromise. A processor owned /// by something longer-lived than any editor is a leaf, so the cycle below cannot form /// and there is nothing to call. Two caveats when you do: it may be called /// concurrently if more than one editor is live, and it must not hold on to any editor /// it has served. /// /// The one rule: **don't conform the object that owns this editor.** Nothing here - /// hands a delegate the editor — every value crossing this boundary is a value type — + /// hands a processor the editor — every value crossing this boundary is a value type — /// so the only way one reaches the editor is if you store it there, which is what /// happens when the coordinator that drives the editor also conforms. Holding this - /// strongly is deliberate — losing the delegate mid-request was the failure actually + /// strongly is deliberate — losing the processor mid-request was the failure actually /// being hit — but it means that shape closes a cycle ARC cannot break, and the editor /// cannot detect its own teardown to break it for you. If you must write it, call /// ``stopMediaHandling()`` when you are done with the editor. - // swiftlint:disable:next weak_delegate - public private(set) var mediaUploadDelegate: (any MediaUploadDelegate)? + public private(set) var mediaProcessor: (any MediaProcessor)? + + /// Takes over media upload on the host's own stack (background session, offline + /// queue, resumable transport). Passing one makes the host own every upload and its + /// whole lifecycle; GutenbergKit stays out of the network entirely for media. + /// + /// Same ownership rules as ``mediaProcessor``: supplied at `init`, held for the + /// editor's lifetime, and not conformed by the object that owns the editor. + /// + /// Reuse is the expected shape here, more so than for a processor: the transports this + /// exists for outlive any one editor by definition — a background `URLSession` has a + /// fixed identifier and must survive app relaunch, an offline queue spans sessions. + /// Build the uploader once, hold it, and pass the same instance to each editor. + /// + /// A ``mediaProcessor`` can still transform the file first; only delivery + /// moves to the uploader. + public private(set) var mediaUploader: (any MediaUploader)? // MARK: - Private Properties (Services) private let editorService: EditorService @@ -150,7 +171,23 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro private let controller: GutenbergEditorController private let bundleProvider: EditorAssetBundleProvider private let lockdownModeMonitor: LockdownModeMonitor - private var uploadServer: MediaUploadServer? + /// Whether the host supplied anything for the native upload server to route. + /// + /// Read twice by `startUploadServer()` — once before starting, once after the bind + /// returns — and the two reads have to agree. They did not: the first gained + /// `mediaUploader` and the second was left checking the processor alone, so an + /// uploader-only host bound a listener, immediately stopped it, and fell back to the + /// WebView path with nothing logged. One property, so they cannot disagree again. + private var hasMediaHandling: Bool { + mediaProcessor != nil || mediaUploader != nil + } + + private(set) var uploadServer: MediaUploadServer? + + /// Shared by every upload server this editor starts, so a server that replaces a lost + /// one can still say whether an upload sent to the old one got through. See + /// ``UploadLedger``. + private let uploadLedger = UploadLedger() // MARK: - Private Properties (UI) @@ -199,23 +236,35 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// - dependencies: Pre-fetched editor dependencies. Pass them when you have them — /// the editor fetches its own otherwise, behind a progress bar. /// - mediaPicker: Supplies media from the host's own picker. - /// - mediaUploadDelegate: Customizes media processing and upload. **Don't conform - /// the object that owns this editor.** Nothing here hands the delegate the editor, - /// so the only way one reaches it is if you store it there — and the editor holds - /// the delegate strongly in return, closing a cycle ARC cannot break. Use a leaf + /// - mediaProcessor: Transforms media before upload. **Don't conform the object + /// that owns this editor.** Nothing here hands the processor the editor, so the + /// only way one reaches it is if you store it there — and the editor holds the + /// processor strongly in return, closing a cycle ARC cannot break. Use a leaf /// object carrying the settings it needs. If you must write the retaining shape, - /// call ``stopMediaHandling()`` when you are done. To reuse one delegate across - /// editors, keep your own reference — the editor drops only its own when it goes. + /// call ``stopMediaHandling()`` when you are done. See ``mediaProcessor`` for the + /// lifetime rules, including what a value-type conformer does differently. + /// - mediaUploader: Takes over media upload on the host's own stack. Same ownership + /// rules as `mediaProcessor`. /// - httpClient: Replaces the client used for editor and media requests. /// - isWarmupMode: Loads the editor shell without dependencies, to warm WebKit. public init( configuration: EditorConfiguration, dependencies: EditorDependencies? = nil, mediaPicker: MediaPickerController? = nil, - mediaUploadDelegate: (any MediaUploadDelegate)? = nil, + mediaProcessor: (any MediaProcessor)? = nil, + mediaUploader: (any MediaUploader)? = nil, httpClient: EditorHTTPClient? = nil, isWarmupMode: Bool = false ) { + // A `mediaUploader` needs site credentials for its media deletes. Check it here, + // where the host hands it over, rather than at server start: the stack trace + // names the caller's own line, and the mistake can't hide until the page loads. + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot: configuration.siteApiRoot, + authHeader: configuration.authHeader, + hasUploader: mediaUploader != nil + ) + let httpClient = httpClient ?? EditorHTTPClient( urlSession: URLSession.shared, authHeader: configuration.authHeader @@ -230,7 +279,8 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro ) self.bundleProvider = EditorAssetBundleProvider(httpClient: httpClient) self.mediaPicker = mediaPicker - self.mediaUploadDelegate = mediaUploadDelegate + self.mediaProcessor = mediaProcessor + self.mediaUploader = mediaUploader self.lockdownModeMonitor = LockdownModeMonitor() self.controller = GutenbergEditorController(configuration: configuration, lockdownModeMonitor: self.lockdownModeMonitor) @@ -247,6 +297,10 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // This allows JavaScript to request the latest persisted content from the native host. config.userContentController.addScriptMessageHandler(controller, contentWorld: .page, name: "requestLatestContent") + // Lets the page ask for a check of the upload server after a request to it fails, + // and whether the upload may be sent again. See `checkUploadServer(afterFailedUpload:)`. + config.userContentController.addScriptMessageHandler(controller, contentWorld: .page, name: "checkUploadServer") + self.bundleProvider.bind(to: config) // Register media file scheme handler for serving local media via gbk-media-file:// URLs @@ -283,6 +337,16 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // Set up Lockdown Mode monitoring with foreground detection lockdownModeMonitor.setup(presentingViewController: self) + // Once the device can idle-sleep, iOS reclaims a suspended app's sockets, the + // upload server's listener included, so check it on the way back. See + // `restartUploadServerIfUnreachable()`. + NotificationCenter.default.addObserver( + self, + selector: #selector(handleWillEnterForeground), + name: UIApplication.willEnterForegroundNotification, + object: nil + ) + // FIXME: implement with CSS (bottom toolbar) webView.scrollView.verticalScrollIndicatorInsets = UIEdgeInsets(top: 0, left: 0, bottom: 47, right: 0) @@ -305,14 +369,8 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro if let dependencies { // FAST PATH: Dependencies were provided at init() - load immediately. - // - // Deliberately NOT tracked in `dependencyTaskHandle`: `viewDidDisappear` - // cancels that handle to abort the async dependency *fetch*, but the - // fast path is cheap local work that must run to completion — a - // transient disappearance (e.g. a modal presented over the editor) - // cancelling it mid `startUploadServer()` silently disabled native - // uploads for the session. `[weak self]` still makes it a no-op once - // the controller is torn down. + // Not cancellable: cancelling mid-`startUploadServer()` silently disables + // native uploads for the session (#357). Task(priority: .userInitiated) { [weak self] in do { try await self?.loadEditor(dependencies: dependencies) @@ -321,8 +379,11 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro } } } else { - // ASYNC FLOW: No dependencies - fetch them asynchronously - self.dependencyTaskHandle = Task(priority: .userInitiated) { [weak self] in + // ASYNC FLOW: No dependencies - fetch them, then load as above. + // Not cancellable either, for the same reason plus one: nothing restarts + // the fetch, so the editor never recovers from a cancel. Note that + // `viewDidDisappear` fires when the editor is merely covered. See #651. + Task(priority: .userInitiated) { [weak self] in await self?.prepareEditor() } } @@ -338,28 +399,24 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro removeNavigationOverlay() } - public override func viewDidDisappear(_ animated: Bool) { - super.viewDidDisappear(animated) - self.dependencyTaskHandle?.cancel() - } - /// Releases the editor's media handling: stops the local upload server, drops the - /// host's ``mediaUploadDelegate``, and withdraws the upload endpoint from the page. + /// host's ``mediaProcessor`` and ``mediaUploader``, and withdraws the upload + /// endpoint from the page. /// /// Most hosts never need this. Releasing the editor runs `deinit`, which does the - /// same work. It is only required when the delegate holds the editor back — which - /// happens if you conformed the object that owns it, the one shape the delegate - /// documentation asks you to avoid — because that cycle keeps `deinit` from ever + /// same work. It is only required when a handler holds the editor back — which + /// happens if you conformed the object that owns it, the one shape ``mediaProcessor`` + /// asks you to avoid — because that cycle keeps `deinit` from ever /// running, stranding a bound loopback `NWListener` for every editor opened. /// /// Terminal, not a pause: this editor cannot upload or delete media afterwards, and /// any upload in flight is cancelled — though cancellation is cooperative, so a - /// `processFile` that ignores it runs to completion and holds the delegate until it + /// `processFile` that ignores it runs to completion and holds the processor until it /// returns. Call it when the editor is going away — not /// when it is covered, backgrounded, or otherwise coming back. Calling it more than /// once is safe. /// - /// Scoped to this editor. It drops this editor's reference, so a delegate you share + /// Scoped to this editor. It drops this editor's reference, so a handler you share /// across editors keeps working for the others. public func stopMediaHandling() { // Host-driven, and the reason is narrower than "UIKit can't tell us". It can. @@ -387,12 +444,88 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // it in at document start) and the trade reverses. uploadServer?.stop() uploadServer = nil - mediaUploadDelegate = nil - revokeNativeUploadEndpoint() + mediaProcessor = nil + mediaUploader = nil + syncNativeUploadEndpoint() + } + + /// Restarts the upload server if its port stopped answering. + /// + /// Once nothing is keeping the device awake — an unplugged phone locked and left to + /// idle — iOS reclaims the sockets of suspended apps, and says nothing: the listener + /// still reports `.ready` on the same port, so watching listener state never finds out + /// (see ``MediaUploadServer/isAnswering(timeout:)``). Suspension alone doesn't do it, so + /// how long the app was away doesn't tell us either. Asking the port is the only way, + /// and this is the only thing between a reclaimed socket and uploads that fail for the + /// rest of the session, because the page holds a port that has stopped working. + /// + /// It runs on two triggers: every return to the foreground, and a request from the page + /// after a request to the server failed (``checkUploadServer(afterFailedUpload:)``). The + /// second covers a socket lost while the app is in the foreground, which no foreground + /// transition would catch — the kernel can defunct every process's sockets, foreground + /// apps included, when it runs out of network buffers. + /// + /// If the restart fails, the endpoint is withdrawn instead, which is the existing + /// fallback: uploads go the WebView's own way rather than to a dead port. + /// + /// - Parameter isAnswering: Asks a server whether its port still answers. Tests pass + /// their own, to decide when each check resumes. + func restartUploadServerIfUnreachable( + isAnswering: (MediaUploadServer) async -> Bool = { await $0.isAnswering() } + ) async { + guard let server = uploadServer else { return } + guard await !isAnswering(server) else { return } + + // Another check can run while this one waits on the probe: two foregrounds in quick + // succession start one each. If that one has already replaced `server`, replacing it + // again throws away the server it just started, along with any upload sent to it. + guard uploadServer === server else { return } + + Logger.uploadServer.warning( + "Upload server on port \(server.port) stopped answering; restarting it" + ) + server.stop() + uploadServer = nil + await startUploadServer() + syncNativeUploadEndpoint() + + if let restarted = uploadServer { + Logger.uploadServer.info("Upload server restarted on port \(restarted.port)") + } + } + + @objc private func handleWillEnterForeground() { + Task { @MainActor [weak self] in + await self?.restartUploadServerIfUnreachable() + } } - /// Withdraws the loopback endpoint from the page so media requests fall back to the - /// WebView's default path instead of failing against a port nothing is listening on. + /// Answers the page after a request to the upload server failed at the transport layer. + /// + /// Runs the same check as a return to the foreground, so a socket lost while the app is + /// in the foreground gets a new server too. Then says where the server is now, and + /// whether the failed upload may be sent again: only if no server ever began passing it + /// on to WordPress, which ``UploadLedger`` settles for good, so a copy the old server + /// still holds can't go out as well. + /// + /// - Parameter uploadID: The ID the page sent with the upload that failed, or `nil` + /// for a request that isn't an upload. + func checkUploadServer(afterFailedUpload uploadID: String?) async -> UploadServerCheck { + await restartUploadServerIfUnreachable() + guard let uploadServer else { + // No server to retry on: the restart failed, or media handling was stopped. + // The endpoint is already withdrawn, so later uploads take the WebView's path. + return UploadServerCheck(port: nil, token: nil, mayRetry: false) + } + let mayRetry = uploadID.map(uploadLedger.abandon) ?? false + return UploadServerCheck(port: uploadServer.port, token: uploadServer.token, mayRetry: mayRetry) + } + + /// Tells the page which loopback endpoint to use, or that there is none. + /// + /// With no server, media requests fall back to the WebView's default path instead of + /// failing against a port nothing is listening on. With a restarted one, they reach the + /// new port instead of the old, dead one. /// /// `nativeMediaUploadMiddleware` re-reads `nativeUploadPort`/`nativeUploadToken` on /// every request and skips the native path when no port is advertised — but it @@ -401,37 +534,45 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// this existed nothing cleared it, so stopping the server left every image insert /// failing with a connection error on a working connection. /// - /// Three copies hold the endpoint and all three have to go: the live page, the + /// Three copies hold the endpoint and all three have to change: the live page, the /// `localStorage` copy `getGBKit()` falls back to, and the injected user script, - /// which would otherwise restore the dead port verbatim at the next document start + /// which would otherwise restore the old port verbatim at the next document start /// — including the reload that recovers a terminated WebContent process. - private func revokeNativeUploadEndpoint() { + private func syncNativeUploadEndpoint() { + var endpoint: [String: Any] = ["nativeUploadPort": NSNull(), "nativeUploadToken": NSNull()] + if let uploadServer { + endpoint["nativeUploadPort"] = Int(uploadServer.port) + endpoint["nativeUploadToken"] = uploadServer.token + } + guard let encoded = try? JSONSerialization.data(withJSONObject: endpoint), + let literal = String(data: encoded, encoding: .utf8) else { return } + webView.evaluateJavaScript( """ - if (window.GBKit) { - window.GBKit.nativeUploadPort = null; - window.GBKit.nativeUploadToken = null; - } - try { - const stored = JSON.parse(localStorage.getItem('GBKit') || '{}'); - stored.nativeUploadPort = null; - stored.nativeUploadToken = null; - localStorage.setItem('GBKit', JSON.stringify(stored)); - } catch (error) {} + (() => { + const endpoint = \(literal); + if (window.GBKit) { + Object.assign(window.GBKit, endpoint); + } + try { + const stored = JSON.parse(localStorage.getItem('GBKit') || '{}'); + localStorage.setItem('GBKit', JSON.stringify({ ...stored, ...endpoint })); + } catch (error) {} + })(); """ ) { _, error in - // Logged rather than surfaced: this runs while the editor is going away, so - // there is no one to tell. Silence would be worse than noise — a failure here + // Logged rather than surfaced: on the withdrawal path the editor is going away, + // so there is no one to tell. Silence would be worse than noise — a failure here // leaves the live page pointed at a port nothing is listening on, which is the // exact failure this method exists to prevent. if let error { - Logger.uploadServer.error("Failed to withdraw the native upload endpoint from the page: \(error)") + Logger.uploadServer.error("Failed to update the native upload endpoint in the page: \(error)") } } - // Rebuilt with `uploadServer` already nil, so the replacement advertises no - // endpoint. The load path is the only other `addUserScript` call site, so removing - // all of them drops exactly the script being replaced. + // Rebuilt from the current `uploadServer`, so the replacement advertises whatever + // it is now — a new port, or none. The load path is the only other `addUserScript` + // call site, so removing all of them drops exactly the script being replaced. webView.configuration.userContentController.removeAllUserScripts() guard let dependencies else { return } do { @@ -442,14 +583,14 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // The load path lets this throw and aborts; here the page is already up, so // the cost is narrower and lands later: the next document start gets no // `window.GBKit` at all rather than one with a stale port. - Logger.uploadServer.error("Failed to rebuild the editor configuration after stopping media handling: \(error)") + Logger.uploadServer.error("Failed to rebuild the editor configuration after changing the upload endpoint: \(error)") } } deinit { - // The ordinary path: with no cycle, ARC releases the delegate when the editor + // The ordinary path: with no cycle, ARC releases the handlers when the editor // goes and this stops the server. A host that retains the editor from its own - // delegate never reaches here — `stopMediaHandling()` is its way out. + // handler never reaches here — `stopMediaHandling()` is its way out. uploadServer?.stop() } @@ -555,23 +696,26 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// The server binds to localhost on a random port. If it fails to start, the editor /// 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 { + func startUploadServer() async { // Nothing to route through the native server unless the host provided a - // delegate. The editor owns it — `mediaUploadDelegate` is strong — so there's - // no released-before-load case to guard against; it lives as long as the - // editor does. - guard mediaUploadDelegate != nil else { + // processor or an uploader. The editor owns whichever it was given — both + // properties are strong — so there's no released-before-load case to guard + // against; they live as long as it does. + guard hasMediaHandling else { return } - // The native upload server relays through DefaultMediaUploader, which needs a + // The native upload server relays through InternalMediaClient, 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. // - // `MediaServerCredentials` owns the check so it is reachable from the host - // test suite — this file is not. + // Only a `mediaProcessor` can reach this return: a `mediaUploader` without + // usable credentials already trapped in `init`, so by here it has credentials. + // + // `MediaServerCredentials` owns both the predicate and that trap so they are + // reachable from the host test suite — this file is not. guard MediaServerCredentials.areUsable( siteApiRoot: configuration.siteApiRoot, authHeader: configuration.authHeader @@ -579,7 +723,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro return } - let defaultUploader = DefaultMediaUploader( + let internalClient = InternalMediaClient( httpClient: httpClient.uploadClient(), siteApiRoot: configuration.siteApiRoot, siteApiNamespace: configuration.siteApiNamespace @@ -587,17 +731,19 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro do { let server = try await MediaUploadServer.start( - uploadDelegate: mediaUploadDelegate, - defaultUploader: defaultUploader + processor: mediaProcessor, + uploader: mediaUploader, + internalClient: internalClient, + ledger: uploadLedger ) // `stopMediaHandling()` can land while the bind is in flight: it is a - // main-actor call and this is suspended. It clears the delegate, so a nil one - // here means media handling was stopped after this started, and storing the - // server would undo a terminal call — the page would be handed a port that was - // just withdrawn, and in the cycle the call exists for, `deinit` never runs to - // stop it. - guard mediaUploadDelegate != nil else { + // main-actor call and this is suspended. It clears both handlers, so the + // entry guard's condition failing here means media handling was stopped after + // this started, and storing the server would undo a terminal call — the page + // would be handed a port that was just withdrawn, and in the cycle the call + // exists for, `deinit` never runs to stop it. + guard hasMediaHandling else { server.stop() return } @@ -968,6 +1114,13 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro return delegate?.editorDidRequestLatestContent(self) } + fileprivate func controller( + _ controller: GutenbergEditorController, + didRequestUploadServerCheckFor uploadID: String? + ) async -> UploadServerCheck { + await checkUploadServer(afterFailedUpload: uploadID) + } + fileprivate func controllerWebContentProcessDidTerminate(_ controller: GutenbergEditorController) { // Reset readiness so JS bridge calls are blocked until the editor // re-emits onEditorLoaded after the reload completes. @@ -1037,10 +1190,31 @@ public struct EditorNotReadyError: LocalizedError { } } +/// The editor's answer to the page's `checkUploadServer` request. +struct UploadServerCheck: Equatable { + /// Where the upload server is listening now, or `nil` if there isn't one. + let port: UInt16? + let token: String? + /// Whether the page may send the failed upload again: no server ever began passing it + /// on to WordPress, and none ever will. + let mayRetry: Bool + + /// The reply for `checkUploadServer()` in `bridge.js`. + var reply: [String: Any] { + var reply: [String: Any] = ["retry": mayRetry] + if let port, let token { + reply["port"] = Int(port) + reply["token"] = token + } + return reply + } +} + @MainActor private protocol GutenbergEditorControllerDelegate: AnyObject { func controller(_ controller: GutenbergEditorController, didReceiveMessage message: EditorJSMessage) func controllerDidRequestLatestContent(_ controller: GutenbergEditorController) -> (title: String, content: String)? + func controller(_ controller: GutenbergEditorController, didRequestUploadServerCheckFor uploadID: String?) async -> UploadServerCheck func controllerWebContentProcessDidTerminate(_ controller: GutenbergEditorController) } @@ -1062,6 +1236,13 @@ private final class GutenbergEditorController: NSObject, WKNavigationDelegate, W // MARK: - WKScriptMessageHandlerWithReply func userContentController(_ userContentController: WKUserContentController, didReceive message: WKScriptMessage) async -> (Any?, String?) { + if message.name == "checkUploadServer" { + let uploadID = (message.body as? [String: Any])?["uploadId"] as? String + guard let delegate else { return (nil, nil) } + let check = await delegate.controller(self, didRequestUploadServerCheckFor: uploadID) + return (check.reply, nil) + } + guard message.name == "requestLatestContent" else { return (nil, "Unknown message handler: \(message.name)") } diff --git a/ios/Sources/GutenbergKit/Sources/Media/BackgroundActivity.swift b/ios/Sources/GutenbergKit/Sources/Media/BackgroundActivity.swift new file mode 100644 index 000000000..5c1329ab7 --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Media/BackgroundActivity.swift @@ -0,0 +1,59 @@ +#if !os(macOS) +import Foundation + +/// Keeps the app running for the system's background grace period (~30s) while `operation` +/// runs, so a media upload already in flight can finish if the user locks the phone mid- +/// transfer — rather than the app being suspended and its loopback socket reclaimed before +/// the upload completes. +/// +/// Best-effort by design: the OS grants a short, fixed window and no more, so a long upload +/// on a slow link still ends when the grace expires. The upload then fails and the user +/// retries, exactly as before — the assertion only widens the window that already works, it +/// doesn't make an arbitrarily long upload survive suspension. Real background continuation +/// belongs with a host ``MediaUploader`` over its own background `URLSession`. +/// +/// Takes the assertion through `ProcessInfo` rather than `UIApplication`, so it needs neither +/// `UIApplication.shared` nor the main thread. That matters because the upload server serves +/// every connection inside this, including its own liveness probe. A probe that had to wait +/// out a busy main thread (while a WebView launches, say) would time out and restart a healthy +/// server, cancelling any upload on it. +/// +/// The assertion is always released: when `operation` finishes, or earlier if the system +/// expires it first. +func withBackgroundActivity(_ reason: String, _ operation: () async -> T) async -> T { + let activity = ExpiringActivity(reason: reason) + let result = await operation() + activity.end() + return result +} + +/// One `ProcessInfo` expiring activity, held until ``end()`` or until the system expires it. +/// +/// The activity lasts as long as its block runs, so the block parks on a semaphore until +/// `end()` signals it. That holds one dispatch thread per activity, and there's at most one +/// activity per connection, which `HTTPServer` caps. +/// +/// If the system expires the activity first, it calls the block a second time with `expired` +/// set, and that call releases the parked one, so the activity ends promptly as the system +/// requires. If it can't grant the activity at all, that `expired` call is the only one. Every +/// order of these calls and `end()` comes out balanced: a signal that arrives before the wait +/// just lets the wait through, and a spare one is harmless. +private struct ExpiringActivity { + private let released = DispatchSemaphore(value: 0) + + init(reason: String) { + let released = released + ProcessInfo.processInfo.performExpiringActivity(withReason: reason) { expired in + if expired { + released.signal() + } else { + released.wait() + } + } + } + + func end() { + released.signal() + } +} +#endif diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift new file mode 100644 index 000000000..8acb65673 --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift @@ -0,0 +1,227 @@ +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, or 201 for an upload a + /// ``MediaUploader`` delivered. + 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 processor's ``MediaProcessor/processFile(at:mimeType:filename:)``. +public enum ProcessedProxyFile: Sendable { + /// The processor did not modify the file; the original upload is forwarded + /// to WordPress unchanged. + case original + + /// The processor 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, it cannot deliver media to the wrong place. Pass one +/// as ``EditorViewController/mediaProcessor`` to resize images, transcode video, +/// strip EXIF, etc. +/// +/// This is the safe, common extension point: most hosts want only this. To perform +/// the upload yourself, conform to ``MediaUploader`` instead. +/// +/// Deliberately **not** class-bound. ``EditorViewController`` holds its processor +/// strongly for its own lifetime, so a conformer that holds the view controller back +/// closes a retain cycle ARC cannot break — neither object is freed, and the editor +/// stops tearing down its upload server. Dropping the class requirement lets you +/// conform with a `struct` capturing only what the transform needs, which is the +/// shape that avoids this; a class bound invited the opposite. Note a +/// value type is not automatic protection — a `struct` that stores the view +/// controller cycles just the same. The rule is simply: do not hold it back. +/// +/// A value-type conformer is **copied** when you hand it to the editor's initializer, +/// and the editor holds that copy for its lifetime. Mutating your own instance +/// afterwards changes nothing the editor will run, and there is no way to swap in a +/// new value — the property is `private(set)`, so a different processor means a +/// different editor. If you need settings the host can change while an editor is open, +/// read them inside `processFile` through a reference the conformer captures. That +/// reference must itself be `Sendable` — an actor, or a class made safe with a lock — +/// because this protocol is `Sendable` and a `struct` conformer's stored properties +/// inherit that requirement. +/// +/// Two requirements a `struct` makes easy to miss, both of which compile silently: +/// `processFile` cannot be `mutating` (a `mutating` witness does not satisfy a +/// non-mutating requirement), and its argument labels must match exactly. Either +/// mistake resolves to the no-op default below instead of failing to build, leaving a +/// processor that is never called. That +/// reference must itself be `Sendable` — an actor, or a class made safe with a lock — +/// because this protocol is `Sendable` and a `struct` conformer's stored properties +/// inherit that requirement. +/// +/// Two requirements a `struct` makes easy to miss, both of which compile silently: +/// `processFile` cannot be `mutating` (a `mutating` witness does not satisfy a +/// non-mutating requirement), and its argument labels must match exactly. Either +/// mistake resolves to the no-op default below instead of failing to build, leaving a +/// processor that is never called. +public protocol MediaProcessor: Sendable { + /// Whether this processor might transform a file with the given metadata. + /// + /// A cheap, metadata-only gate the server consults *before* materializing the + /// upload to a temp file. Return `false` to decline a file by type — e.g. an + /// image-only processor returning `false` for a video — so the server forwards + /// the original upload to WordPress without first copying a file the processor + /// won't touch. + /// + /// With a ``MediaUploader`` set this can't decline the upload itself — an + /// uploader delivers every file, so there is no passthrough to fall to — but it + /// still gates `processFile`: a declined file reaches the uploader unprocessed. + /// + /// 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 +} + +/// Default implementations. +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 + } +} + +/// 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 + + public init(fileURL: URL, mimeType: String, filename: String, fields: [MediaUploadField], query: String) { + self.fileURL = fileURL + self.mimeType = mimeType + self.filename = filename + self.fields = fields + self.query = query + } +} + +/// 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 internal media client both target the same configured site. +/// Supplying ``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 the editor to retry behind it. The +/// attachment you return lives on that same configured site, where the editor reads +/// and updates it by ID. +/// +/// Deliberately **not** class-bound, for the same reason as ``MediaProcessor``: the +/// editor holds its uploader strongly, so a conformer that holds the view controller +/// back forms a retain cycle neither object escapes. The operative rule is that one: +/// do not store the ``EditorViewController``. A value type does not enforce it — a +/// `struct` holding the view controller cycles the same way — and it carries the same +/// copy-at-`init` caveat described on ``MediaProcessor``. An uploader that owns a +/// queue, a background session, or a retry counter wants a class; capture it behind a +/// reference either way. +public protocol MediaUploader: 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. That request needs a body of `{"action": "create-image-subsizes"}` + /// — core registers `action` as **required**, so a post-process request without + /// it fails with a 400 every time rather than recovering. + /// + /// 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 the editor cleans up behind you. + func upload(_ upload: MediaUpload) async throws -> Data +} diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift index a8ce4ee51..2b2514e0a 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift @@ -1,24 +1,73 @@ import Foundation -/// Whether the editor configuration can reach the configured site for media. +/// 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 check, which already diverged silently between iOS and Android once. Living -/// here, it is reachable from the host test suite. +/// 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 a ``DefaultMediaUploader`` built from this configuration could actually + /// Whether an ``InternalMediaClient`` built from this configuration could actually /// reach the site. /// - /// Both fields are required. The uploader delivers GutenbergKit's uploads to the + /// Both fields are required. The client delivers GutenbergKit's uploads 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. + /// "Somewhere to send them" means an *absolute* root: a URL with no scheme or host + /// cannot address the site, and every request built from it fails at the URLSession + /// layer. `siteApiRoot` is a `URL` here where Android types it as a `String`, but + /// the rule is the same on both sides — Android spells it `Uri.parse(...)` with the + /// same scheme-and-host test, having previously checked only `isEmpty()` and so + /// accepted roots this rejects. static func areUsable(siteApiRoot: URL, authHeader: String) -> Bool { siteApiRoot.scheme != nil && siteApiRoot.host() != nil && !authHeader.isEmpty } + + /// Traps if the host supplied 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 + /// server simply stays down and uploads fall to the default WebView path. That + /// is ``areUsable``'s job, at the point the server would start. + /// + /// - A ``MediaUploader`` means the host is *taking over* uploads, and falling back + /// would drop that whole stack — its queueing, its retries — while media appeared + /// to keep working. Worth failing over rather than logging. + /// + /// What makes it a *trap* rather than a warning is that the configuration is + /// incoherent, not merely unlucky: an uploader's media deletes still relay through + /// the internal media client, so there is no site root and auth header under which + /// this host's uploader could have worked. Contrast the conditions the host's + /// environment imposes at server start — a network policy that blocks the loopback + /// endpoint, a port that won't bind — which log and degrade, because the very same + /// configuration works once the environment allows it. Dropping the uploader is the + /// symptom both share; only this one has a cause the host can fix in the + /// configuration it just handed over. + /// + /// Called from `EditorViewController.init`, not from the server start. The uploader + /// is `private(set)` and assigned only there, so a non-nil uploader at load time was + /// necessarily passed at `init` — checking it then puts the host's own call site in + /// the stack trace, instead of surfacing the mistake later from inside a page-load + /// callback where the trace names only GutenbergKit. This mirrors what moving the + /// handlers into `init` already did for the set-before-load contract: enforce the + /// rule where the host states its intent. + /// + /// (Android enforces this in `GutenbergView.mediaUploader`'s setter — the earliest + /// point available there, since it takes its handlers as mutable properties rather + /// than at construction.) + static func requireCredentialsForUploader(siteApiRoot: URL, authHeader: String, hasUploader: Bool) { + guard hasUploader else { return } + precondition( + areUsable(siteApiRoot: siteApiRoot, authHeader: authHeader), + "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." + ) + } } 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 6d0561416..689ed60ff 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -1,5 +1,6 @@ import Foundation import GutenbergKitHTTP +import Network import OSLog /// A local HTTP server that receives file uploads from the WebView and routes @@ -20,6 +21,9 @@ final class MediaUploadServer: Sendable { /// Per-session auth token for validating incoming requests. let token: String + /// The request header carrying the page's ID for an upload. See ``UploadLedger``. + static let uploadIDHeader = "Relay-Upload-ID" + private let server: HTTPServer /// Sweeps crash-orphaned upload temp files off the editor-startup path. @@ -29,13 +33,20 @@ 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 that transforms the file before delivery. + /// - uploader: Optional host uploader that performs the upload on its own stack. + /// - internalClient: GutenbergKit's own client for the configured site. Delivers + /// uploads when no host uploader does, and every media delete. + /// - ledger: Records which uploads this server has begun passing on to WordPress, + /// so the page can safely retry one that never got that far. Pass the same ledger + /// to a server that replaces this one. /// - 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? = nil, + ledger: UploadLedger = UploadLedger(), maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize ) async throws -> MediaUploadServer { // Sweep temp files orphaned by a prior crash, off the editor-startup @@ -45,7 +56,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(uploadDelegate: uploadDelegate, defaultUploader: defaultUploader) + let handler = Handler(processor: processor, uploader: uploader, internalClient: internalClient, ledger: ledger) // 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,14 +73,12 @@ final class MediaUploadServer: Sendable { bodyReadTimeout: bodyReadTimeout, cors: .permissive, delegate: ServerDelegate(), - handler: { request in - await Self.handleRequest(request, context: context) - } + handler: handler ) let uploadServer = MediaUploadServer(server: server, cleanupTask: cleanupTask) #if DEBUG - countServerStarted(delegate: uploadDelegate) + countServerStarted(processor: processor, uploader: uploader) #endif return uploadServer } @@ -83,7 +92,7 @@ final class MediaUploadServer: Sendable { /// editor stops it on `deinit`, so returning to zero is the normal outcome — monotone /// growth is the ownership cycle described on /// ``EditorViewController/stopMediaHandling()``. Nothing else produces it: - /// `EditorViewController.warmup()` passes no delegate, so it never starts a server. + /// `EditorViewController.warmup()` passes neither handler, so it never starts a server. /// /// This population is the only detectable symptom of that cycle. A `deinit` assertion /// on the editor cannot work — a cycle is precisely what stops `deinit` from running — @@ -99,7 +108,10 @@ final class MediaUploadServer: Sendable { /// overlap across a push or a modal transition; four is not a shape hosts produce. private static let liveServerLeakThreshold = 4 - private static func countServerStarted(delegate: (any MediaUploadDelegate)?) { + private static func countServerStarted( + processor: (any MediaProcessor)?, + uploader: (any MediaUploader)? + ) { let count = censusLock.withLock { liveServerCount += 1 return liveServerCount @@ -107,15 +119,20 @@ final class MediaUploadServer: Sendable { guard count >= liveServerLeakThreshold else { return } - let name = delegate.map { String(describing: type(of: $0)) } ?? "the host's delegate" + // Name every handler that was supplied, not just the first. With both set the + // retainer is as likely to be the uploader, and naming only the processor sends + // the reader to audit an object that may be a value type holding nothing at all. + let names = [processor.map { String(describing: type(of: $0)) }, + uploader.map { String(describing: type(of: $0)) }].compactMap { $0 } + let name = names.isEmpty ? "the host's media handler" : names.joined(separator: ", ") Logger.uploadServer.fault( """ \(count, privacy: .public) media upload servers are live, one bound loopback \ listener each. Editors are leaking: a host that both owns EditorViewController \ - and is its own media upload delegate (\(name, privacy: .public)) forms a retain \ + and is one of its own media handlers (\(name, privacy: .public)) forms a retain \ cycle ARC cannot break, so the editor's deinit never runs. Call \ EditorViewController.stopMediaHandling() when you are done with the editor, or \ - keep the delegate a leaf object that doesn't reference the editor. + keep the handler a leaf object that doesn't reference the editor. """ ) } @@ -137,262 +154,434 @@ final class MediaUploadServer: Sendable { server.stop() } - // MARK: - Request Handling + // MARK: - Liveness - private static func handleRequest(_ request: HTTPServer.Request, context: UploadContext) async -> HTTPResponse { - let parsed = request.parsed + /// Whether the port still answers, which is not the same as the listener looking healthy. + /// + /// Once the device becomes eligible for idle sleep, iOS reclaims the sockets of suspended + /// apps and reports nothing: `NWListener` still says `.ready` on the same port, and no + /// state is delivered. Suspension alone isn't enough — on an iPhone 15 Pro running + /// iOS 27.0 the socket survived 27 minutes in the background while plugged in, and was + /// gone after 6 minutes locked, unplugged, and left to idle. Asking the port is the only + /// way to find out. + /// + /// The request deliberately carries no token. The server answers `407` and logs nothing, + /// so a check that runs on every foreground stays silent, and any answer at all means + /// the socket is still there. A socket the system took refuses the connection instead. + /// + /// Uses `NWConnection` rather than `URLSession` so no host's App Transport Security + /// settings can decide the outcome. + func isAnswering(timeout: Duration = .seconds(2)) async -> Bool { + guard let endpointPort = NWEndpoint.Port(rawValue: port) else { return false } + let connection = NWConnection(host: .ipv4(.loopback), port: endpointPort, using: .tcp) + // Cancelling makes the probe below finish as a failure, so it can't outlive this. + let deadline = Task { + try await Task.sleep(for: timeout) + connection.cancel() + } + defer { + deadline.cancel() + connection.cancel() + } + return await Self.ask(connection) + } + + private static let probeQueue = DispatchQueue(label: "com.gutenbergkit.upload-server-probe") + + private static func ask(_ connection: NWConnection) async -> Bool { + await withCheckedContinuation { continuation in + // A failed send reports both an error and a state change, so the answer has to + // be claimed once. + let answer = OnceFlag() + connection.stateUpdateHandler = { state in + switch state { + case .ready: + let request = Data("GET / HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n".utf8) + connection.send(content: request, completion: .contentProcessed { error in + guard error == nil else { + if answer.claim() { continuation.resume(returning: false) } + return + } + connection.receive(minimumIncompleteLength: 1, maximumLength: 64) { data, _, _, error in + let answered = error == nil && !(data ?? Data()).isEmpty + if answer.claim() { continuation.resume(returning: answered) } + } + }) + // A refused connection doesn't fail: it waits in `.waiting(ECONNREFUSED)` to + // retry when the network path changes, which on loopback it never does. So + // `.waiting` means nothing is listening — treating it as anything else only + // delays the same answer until the timeout. + case .waiting, .failed, .cancelled: + if answer.claim() { continuation.resume(returning: false) } + default: + break + } + } + connection.start(queue: probeQueue) + } + } - // 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() + /// Lets exactly one of several callbacks resume a continuation. + private final class OnceFlag: @unchecked Sendable { + private let lock = NSLock() + private var claimed = false - if method == "POST", parsed.path == "/upload" { - return await handleUpload(request, context: context) + func claim() -> Bool { + lock.lock() + defer { lock.unlock() } + if claimed { return false } + claimed = true + return true } + } - if method == "DELETE", let attachmentId = attachmentId(fromPath: parsed.path) { - return await handleDelete(attachmentId, query: parsed.query, context: context) - } + // MARK: - Request Handling - return errorResponse(status: 404, message: "Not found") - } + /// Serves the upload server's requests. + /// + /// A `struct` rather than a closure over a context object: the dependencies become + /// stored properties and the request logic becomes instance methods, instead of + /// statics threading a context parameter through every call. It stores no reference + /// back to the `MediaUploadServer`, so it can't close the + /// `MediaUploadServer -> HTTPServer -> handler -> MediaUploadServer` loop that + /// would keep `deinit` — and therefore `stop()` — from ever running. Being a value + /// type is not what buys that: a `struct` storing the server would close the loop + /// just the same, which is why the statics above stay static. + /// + /// Everything here is held **strongly**, so a processor that admitted a file for + /// processing will process it, and an upload gated on a host uploader will be + /// delivered by it — the reads within a request can't disagree, and an in-flight + /// upload keeps the host's handlers alive until it unwinds. This matches Android, + /// which holds its `processor`/`uploader` as plain `val`s for the same reason. + /// + /// Strong is safe because `EditorViewController` owns `mediaProcessor` and + /// `mediaUploader` strongly too. A host object that retains the view controller + /// back already forms `EditorViewController -> mediaUploader -> + /// EditorViewController`, a cycle this handler can neither create nor prevent. + /// + /// Implicitly `Sendable`: `MediaProcessor` and `MediaUploader` are `Sendable` + /// protocols and `InternalMediaClient` is `@unchecked Sendable`. + private struct Handler: HTTPRequestHandler { + let processor: (any MediaProcessor)? + let uploader: (any MediaUploader)? + let internalClient: InternalMediaClient? + let ledger: UploadLedger + + 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) + } - 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") - } + if method == "DELETE", let attachmentId = Self.attachmentId(fromPath: parsed.path) { + return await handleDelete(attachmentId, query: parsed.query) + } - // 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") + return MediaUploadServer.errorResponse(status: 404, message: "Not found") } - // 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 + 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") + } + + // 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") + } + + // Nothing below has reached WordPress yet. If the page has given up on this + // upload, it may already be sending the file again, so this copy must stop here. + // Nobody reads the response: the page's connection for it is gone. + guard ledger.begin(request.parsed.header(MediaUploadServer.uploadIDHeader)) else { + Logger.uploadServer.info("Dropped an upload the editor had already given up on") + return MediaUploadServer.errorResponse(status: 409, message: "The editor gave up on this upload") + } - let filename = filePart.filename ?? "upload" - let mimeType = filePart.contentType + // 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 + + // Ask the processor — 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 processor won't process (e.g. a video handed + // to an image-only processor). + // + // An uploader takes over delivery for *every* file, so with one set there is + // no passthrough to fall to and the gate can't decline the upload outright. + // It still decides whether `processFile` runs, though — a declined file is + // handed to the uploader unprocessed rather than to a processor that said it + // won't touch it — so the answer is carried into `processAndUpload` rather + // than discarded here. Asked exactly once per upload, matching Android. + let processorWantsFile = processor?.handlesFile(ofType: mimeType, named: filename) ?? false + guard uploader != nil || processorWantsFile else { + do { + return try await passthroughResponse(request, query: query) + } catch { + return Self.uploadErrorResponse(error) + } + } - // 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 { + // Someone wants the file — the processor, the uploader, or both. Stream the + // part body to a dedicated temp file for them: the library's RequestBody may + // be a byte-range slice of a larger temp file whose lifecycle is tied to ARC, + // so they need a standalone file that outlives the handler return. + let tempDir = MediaUploadServer.uploadsTempDirectory + try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) + + let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(MediaUploadServer.sanitizeFilename(filename))") do { - return try await passthroughResponse(request, query: query, context: context) + let inputStream = try filePart.body.makeInputStream() + try MediaUploadServer.writeStream(inputStream, to: fileURL) } catch { - return uploadErrorResponse(error) + try? FileManager.default.removeItem(at: fileURL) + Logger.uploadServer.error("Failed to write upload to disk: \(error)") + return MediaUploadServer.errorResponse(status: 500, message: "Failed to save file") } - } - // The delegate wants the file. Stream the part body to a dedicated temp - // file for it — the library's RequestBody may be a byte-range slice of a - // larger temp file whose lifecycle is tied to ARC, so the delegate needs a - // standalone file that outlives the handler return. - let tempDir = uploadsTempDirectory - try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) - - let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(sanitizeFilename(filename))") - do { - let inputStream = try filePart.body.makeInputStream() - try writeStream(inputStream, to: fileURL) - } catch { - try? FileManager.default.removeItem(at: fileURL) - Logger.uploadServer.error("Failed to write upload to disk: \(error)") - return errorResponse(status: 500, message: "Failed to save file") - } + // From here on always clean up the original temp file. The processed + // file (if the processor produced a new one) is cleaned up inside + // processAndUpload so its throw paths are covered too. + defer { try? FileManager.default.removeItem(at: fileURL) } - // From here on always clean up the original temp file. The processed - // file (if the delegate produced a new one) is cleaned up inside - // processAndUpload so its throw paths are covered too. - defer { try? FileManager.default.removeItem(at: fileURL) } + do { + let uploadResult = try await processAndUpload( + fileURL: fileURL, mimeType: mimeType, filename: filename, + extraParts: extraParts, query: query, + processorWantsFile: processorWantsFile + ) + switch uploadResult { + case .uploaded(let uploaded): + Logger.uploadServer.debug("Uploaded file to WordPress") + return Self.relayResponse(uploaded) + case .passthrough: + // The processor didn't modify the file — forward the original request + // body to WordPress without re-encoding. + return try await passthroughResponse(request, query: query) + } + } catch { + return Self.uploadErrorResponse(error) + } + } - 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) + /// Forwards the original request body to WordPress unchanged (no multipart + /// re-encoding) and relays the response. Used 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"), + let internalClient else { + return MediaUploadServer.errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) } - } catch { - return uploadErrorResponse(error) + let response = try await internalClient.passthroughUpload(body: body, contentType: contentType, query: query) + return Self.relayResponse(response) } - } - /// 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 { - // 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"), - let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + /// 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 } - 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 func handleDelete( + _ attachmentId: String, query: String + ) async -> HTTPResponse { + guard let internalClient else { + return MediaUploadServer.errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + } + do { + let response = try await internalClient.deleteMedia(attachmentId: attachmentId, query: query) + return Self.relayResponse(response) + } catch { + return Self.uploadErrorResponse(error) + } + } - /// 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) + /// 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 response's own `Content-Type` wins over the JSON default. `HTTPResponse` + /// serializes every header it is given, so appending the default unconditionally + /// would emit the name twice for a processor that sets it. + private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { + let hasContentType = response.headers.keys.contains { $0.lowercased() == "content-type" } + return HTTPResponse( + status: response.statusCode, + headers: (hasContentType ? [] : [("Content-Type", "application/json")]) + + response.headers.map { ($0.key, $0.value) }, + body: response.body + ) } - 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. - /// - /// The response's own `Content-Type` wins over the JSON default. `HTTPResponse` - /// serializes every header it is given, so appending the default unconditionally - /// would emit the name twice for a delegate that sets it. - private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { - let hasContentType = response.headers.keys.contains { $0.lowercased() == "content-type" } - return HTTPResponse( - status: response.statusCode, - headers: (hasContentType ? [] : [("Content-Type", "application/json")]) - + response.headers.map { ($0.key, $0.value) }, - body: response.body - ) - } + // MARK: - Processor 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 processing + upload pipeline. + private enum UploadResult { + /// The uploader or internal media client completed the upload; + /// carries the raw WordPress response to relay. + case uploaded(MediaUploadResponse) + /// The processor didn't modify the file, so the original body is forwarded. + /// The caller should forward the original request body to WordPress. + case passthrough } - return errorResponse(status: 500, message: error.localizedDescription) - } - // MARK: - Delegate Pipeline + private func processAndUpload( + fileURL: URL, mimeType: String, filename: String, + extraParts: [MultipartPart], query: String, processorWantsFile: Bool + ) async throws -> UploadResult { + // Step 1: Process (resize, transcode, etc.) — but only for a file the + // processor's metadata gate accepted. `handlesFile` returning false is the + // processor saying it won't touch a file like this, so handing it one anyway + // would break the contract the gate documents. With an uploader set the file + // still gets delivered; it just skips processing on its way there. + let processed: ProcessedProxyFile + if let processor, processorWantsFile { + processed = try await processor.processFile(at: fileURL, mimeType: mimeType, filename: filename) + } else { + processed = .original + } - /// 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 - } + // Resolve the file to upload and its metadata. `.processed` uses the + // processor's values verbatim, so a format change is reported to WordPress. + let uploadURL: URL + let uploadMimeType: String + let uploadFilename: String + switch processed { + case .original: + uploadURL = fileURL + uploadMimeType = mimeType + uploadFilename = filename + case let .processed(url, processedMimeType, processedFilename): + uploadURL = url + uploadMimeType = processedMimeType + uploadFilename = processedFilename + } - private 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 - } + // The processed file (if the processor produced a new one) is ours to + // clean up — on success it has been uploaded, on failure it is abandoned. + // Cleaning up here rather than in the caller covers the throw paths too. + defer { + if uploadURL != fileURL { + try? FileManager.default.removeItem(at: uploadURL) + } + } - // Resolve the file to upload and its metadata. `.processed` uses the - // 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 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. + if let uploader { + let upload = MediaUpload( + fileURL: uploadURL, + mimeType: uploadMimeType, + filename: uploadFilename, + fields: try await Self.formFields(from: extraParts), + query: query + ) + let attachment = try await uploader.upload(upload) + return .uploaded(MediaUploadResponse(statusCode: 201, body: attachment)) + } - // 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) + if let internalClient { + // 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) + } else { + throw UploadError.noUploader } } - // 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: 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 + /// The editor's non-file form parts as ordered, UTF-8-decoded fields. + /// + /// A list rather than a dictionary so repeated names (e.g. a `field[]` array) + /// survive verbatim, in the order the editor sent them. + /// + /// This decode can't mangle anything, but only because of who is on the other end — + /// nothing in the code enforces it. Three things have to stay true: + /// + /// 1. Only the editor's own web page can reach this server. It listens on loopback, + /// and every request has to carry a per-session token. + /// 2. Text the editor puts in a form field is already valid Unicode. The browser + /// guarantees that when the value is set, so it cannot hand us bad bytes. + /// 3. The only way a browser can put *raw* bytes in a form is a file or a Blob, and + /// those always arrive with a filename. Anything with a filename is handled as + /// the file, never as a field — so raw bytes never reach this decode. + /// + /// If one of those stops being true, bad bytes quietly turn into replacement + /// characters, and the platforms don't even agree on how many: `ED A0 80` becomes + /// three of them here and one on Android. There is no single behavior worth + /// documenting, so the tests pin rule 3 instead. + 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 } } @@ -413,7 +602,8 @@ final class MediaUploadServer: Sendable { /// Answers the server's recoverable parse errors (e.g. an over-limit body) /// with the same JSON `{code, message}` shape the editor expects, so the /// middleware surfaces a real message ("The file is too large…") instead of a - /// generic parse-failure. A leaf object — the HTTP server retains it. + /// generic parse-failure, and keeps the app running while a connection is + /// served. A leaf object — the HTTP server retains it. private final class ServerDelegate: HTTPServerDelegate { func response(forRecoverableParseError error: HTTPRequestParseError) -> HTTPResponse { let message: String = switch error { @@ -422,6 +612,26 @@ final class MediaUploadServer: Sendable { } return MediaUploadServer.errorResponse(status: error.httpStatus, message: message) } + + /// Holds a background-task assertion from the first byte of the request to the + /// last byte of the response, so locking the phone mid-upload doesn't suspend the + /// app at either end of the exchange. + /// + /// Wrapping only the handler misses both ends. The file arrives from the WebView + /// before the handler runs, and WordPress's answer is written back after it + /// returns. An app suspended in the second gap has already created the attachment, + /// and if iOS reclaims the socket before the app resumes, the editor never hears + /// about it: it shows a failure, and a retry makes a duplicate. + /// + /// Best-effort: the grace is fixed (~30s), so a long upload still ends when it + /// expires. See `withBackgroundActivity`. + func withConnectionActivity(_ body: () async -> Void) async { + #if !os(macOS) + await withBackgroundActivity("gutenbergkit-media-upload", body) + #else + await body() + #endif + } } // MARK: - Helpers @@ -509,43 +719,23 @@ enum UploadError: Error, LocalizedError { var errorDescription: String? { switch self { - case .noUploader: "No upload delegate or default uploader configured" + case .noUploader: "No media uploader or internal media client configured" case .streamReadFailed: "Failed to read upload stream" case .streamWriteFailed: "Failed to write upload to disk" } } } -// MARK: - Upload Context +// MARK: - Internal Media Client -/// Container for the upload delegate and default uploader, captured by the -/// HTTPServer handler closure and read on each request. +/// GutenbergKit's own client for the configured site, built from the site credentials +/// in `EditorConfiguration`. /// -/// Both are held **strongly**, so a delegate that admitted a file for processing -/// will process it — the three reads within a request can't disagree, and an -/// in-flight upload keeps the host's delegate alive until it unwinds. This matches -/// Android, which holds its `uploadDelegate` as a plain `val` for the same reason. -/// -/// Strong is safe *given* `EditorViewController` now owns `mediaUploadDelegate` -/// strongly too — but be exact about what that trades away. Weak here did break one -/// ring: every other edge in `EditorViewController → uploadServer → HTTPServer → -/// listener → newConnectionHandler → handler → UploadContext → delegate` is strong, -/// so this was its only weak link. What it could not break is the shorter ring -/// straight through the property. A host that retains the view controller back now -/// leaks either way, so weak here buys a partial guard in exchange for the delegate -/// vanishing mid-request — which is the failure that was actually being hit. -/// -/// A `struct`, so it is implicitly `Sendable`: `MediaUploadDelegate` is a `Sendable` -/// protocol and `DefaultMediaUploader` is `@unchecked Sendable`. -private struct UploadContext: Sendable { - let uploadDelegate: (any MediaUploadDelegate)? - let defaultUploader: DefaultMediaUploader? -} - -// MARK: - Default Media Uploader - -/// Uploads files to the WordPress REST API using site credentials from EditorConfiguration. -class DefaultMediaUploader: @unchecked Sendable { +/// Not an implementation of any host-facing protocol — it is the thing that actually +/// performs GutenbergKit's media requests. It delivers uploads the host did not take +/// over, and relays the editor's media deletes: every attachment lives on the +/// configured site, so that is where its deletion goes. +class InternalMediaClient: @unchecked Sendable { private let httpClient: EditorHTTPClientProtocol private let siteApiRoot: URL private let siteApiNamespace: String? @@ -596,7 +786,7 @@ class DefaultMediaUploader: @unchecked Sendable { /// Forwards the original request body to WordPress without re-encoding. /// - /// Used when the delegate's `processFile` returned the file unchanged — + /// Used when the processor's `processFile` returned the file unchanged — /// the incoming multipart body is already valid for WordPress. func passthroughUpload(body: RequestBody, contentType: String, query: String) async throws -> MediaUploadResponse { var request = URLRequest(url: mediaEndpointURL(query: query)) @@ -695,11 +885,12 @@ class DefaultMediaUploader: @unchecked Sendable { mimeType: String, extraFields: [(name: String, value: Data)] ) 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. + // The non-file parts (post, additionalData) go into the preamble ahead of the streamed + // file; they're small, and `contentLength` counts them via `preamble.count`. Their + // values are appended as raw bytes rather than through `String(data:encoding:)`, which + // returns nil on bad UTF-8 — and the `?? ""` you'd reach for behind it would quietly + // drop a whole field. Raw bytes also keep this re-encode byte-for-byte identical to + // the plain passthrough it replaces. (Bad bytes can't get here; see `formFields`.) var preamble = Data() for field in extraFields { preamble.append(Data("--\(boundary)\r\n".utf8)) diff --git a/ios/Sources/GutenbergKit/Sources/Media/UploadLedger.swift b/ios/Sources/GutenbergKit/Sources/Media/UploadLedger.swift new file mode 100644 index 000000000..cde1041d6 --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Media/UploadLedger.swift @@ -0,0 +1,54 @@ +import Foundation +import os + +/// Which uploads the upload server has begun passing on to WordPress, and which the page +/// has given up on. +/// +/// This is what lets the page retry an upload it lost without risking a duplicate +/// attachment. When a request to the server fails at the transport layer, the page can't +/// tell a connection that was refused (nothing received the upload) from one cut off after +/// the server had handed the file on (the attachment may already exist). The server can, +/// if every upload carries an ID: an upload the server never *began* can't have reached +/// WordPress. +/// +/// ``begin(_:)`` and ``abandon(_:)`` settle each ID once, under one lock, so exactly one of +/// them wins. If the page gives up on an upload the server hasn't begun, the server will +/// refuse to begin it later, and the page's retry is the only copy that reaches WordPress. +/// +/// The editor owns the ledger and hands the same one to every server it starts. The +/// question is usually about an upload the *previous* server may have received, so the +/// answer has to outlive the restart. +final class UploadLedger: Sendable { + private enum Outcome { + case begun + case abandoned + } + + private let outcomes = OSAllocatedUnfairLock<[String: Outcome]>(initialState: [:]) + + /// Records that the server is about to pass upload `id` on toward WordPress. + /// + /// - Returns: `false` if the page has already given up on the upload, in which case it + /// must not be sent. An upload without an ID, from a page that doesn't send them, is + /// always allowed. + func begin(_ id: String?) -> Bool { + guard let id else { return true } + return outcomes.withLock { outcomes in + if outcomes[id] == .abandoned { return false } + outcomes[id] = .begun + return true + } + } + + /// Records that the page has given up on upload `id`. + /// + /// - Returns: `true` if the server never began the upload, so WordPress can't have + /// received it and the page may send the file again. + func abandon(_ id: String) -> Bool { + outcomes.withLock { outcomes in + if outcomes[id] == .begun { return false } + outcomes[id] = .abandoned + return true + } + } +} diff --git a/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift b/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift index 642053b1e..81faa70aa 100644 --- a/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift +++ b/ios/Sources/GutenbergKitHTTP/CORSPolicy.swift @@ -32,7 +32,10 @@ public enum CORSPolicy: Sendable { // can't be cleanly allowlisted. ("Access-Control-Allow-Origin", "*"), ("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS"), - ("Access-Control-Allow-Headers", "Authorization, Relay-Authorization, Content-Type"), + // `Relay-Upload-ID` identifies an upload so the editor can retry one that + // never reached WordPress. It only matters where CORS is enforced on the + // editor's own `file://` page, which is Lockdown Mode. + ("Access-Control-Allow-Headers", "Authorization, Relay-Authorization, Relay-Upload-ID, Content-Type"), // Only CORS-safelisted response headers are readable // cross-origin by default. The editor needs to read // `x-wp-upload-attachment-id` off a relayed media upload diff --git a/ios/Sources/GutenbergKitHTTP/HTTPRequestHandler.swift b/ios/Sources/GutenbergKitHTTP/HTTPRequestHandler.swift new file mode 100644 index 000000000..e4ec8c020 --- /dev/null +++ b/ios/Sources/GutenbergKitHTTP/HTTPRequestHandler.swift @@ -0,0 +1,42 @@ +#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, so a handler must not strongly +/// hold the object that owns the server, directly or transitively: +/// `owner → HTTPServer → handler → owner` is a cycle, the owner's `deinit` never runs, +/// and `stop()` is never called — a silently stranded listener, not a crash. +/// +/// A value type is **not** protection. A `struct` handler storing the owner closes the +/// same ring: the server captures the struct into a heap node, and its stored properties +/// are strong edges out of it. This protocol is deliberately **not** `AnyObject`-constrained +/// so a handler *can* be a `struct` holding only what it needs — not because a `struct` is +/// safe by construction. Either shape works; both must stay leaves, the same discipline +/// ``HTTPServerDelegate`` documents. +/// +/// The usual trap is the object that starts the server also serving it — a view controller +/// starting it in `viewDidLoad` and stopping it in `deinit` is the shape that bites, because +/// the cycle disables the very teardown meant to break it. Conform a separate leaf type, or +/// call `stop()` from a hook that does run. +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 ae5f0a33d..6f3d52d80 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift @@ -156,14 +156,15 @@ public final class HTTPServer: Sendable { /// consumers expecting large uploads should pass a generous value. /// - idleTimeout: The maximum time to wait between consecutive reads before closing /// the connection. Prevents slow-loris attacks. Defaults to 5 seconds. - /// - startTimeout: The maximum time to wait for the listener to become ready - /// before giving up with ``HTTPServerError/failedToStart``. Bounds a listener - /// stuck in the `.waiting` state. Defaults to 5 seconds. + /// - startTimeout: The maximum time to wait for the listener to become ready before + /// throwing ``HTTPServerError/startTimeout`` — for example, when it's stuck in + /// `.waiting`. Defaults to 5 seconds. /// - handler: A closure invoked for each fully-parsed request. Return an ``HTTPResponse`` /// to send back to the client. /// - Returns: A running ``HTTPServer`` instance. - /// - Throws: ``HTTPServerError/failedToStart`` if the listener cannot bind to the port - /// or does not become ready within `startTimeout`. + /// - Throws: ``HTTPServerError/failedToStart`` if the listener fails, for example + /// because the port is already in use, or ``HTTPServerError/startTimeout`` if it + /// isn't ready within `startTimeout`. public static func start( name: String, port: UInt16? = nil, @@ -232,9 +233,31 @@ public final class HTTPServer: Sendable { // Bridge listener state callbacks to an AsyncStream so we can await readiness. // The listener is started synchronously — only the wait is async. + // + // This handler stays installed for the listener's whole life. Until the listener is + // ready, it passes each state to the wait below; after that, it hands them to + // `logStateAfterStart`. Swapping in a new handler at `.ready` would lose states: + // Network.framework picks the handler when it queues a state, not when it delivers + // it, so a state queued just before the swap would still reach the old handler, + // with nothing left listening for it. + // + // Nothing removes this handler, so it's released on `queue` once the listener is + // cancelled. Don't capture the server (that's a retain cycle) or anything whose + // `deinit` must run on the caller's thread (see `releaseConnectionHandler()`). let (states, statesContinuation) = AsyncStream.makeStream(of: NWListener.State.self) - listener.stateUpdateHandler = { state in + // `nil` until the listener is ready, then its port. It's a lock only because the + // handler must be `@Sendable`; the handler is only ever called on `queue`. + let readyPort = OSAllocatedUnfairLock(initialState: nil) + listener.stateUpdateHandler = { [weak listener] state in + if let boundPort = readyPort.withLock({ $0 }) { + logStateAfterStart(state, port: boundPort) + return + } statesContinuation.yield(state) + if case .ready = state { + let boundPort = listener?.port?.rawValue ?? 0 + readyPort.withLock { $0 = boundPort } + } } listener.start(queue: queue) @@ -251,7 +274,6 @@ public final class HTTPServer: Sendable { for await state in states { switch state { case .ready: - listener.stateUpdateHandler = nil guard let p = listener.port else { throw HTTPServerError.failedToStart } @@ -259,10 +281,14 @@ public final class HTTPServer: Sendable { Logger.httpServer.info("HTTP server started on port \(p.rawValue)") return server case .failed(let error): - Logger.httpServer.error("Listener failed: \(error)") + Logger.httpServer.error("Listener failed: \(error, privacy: .public)") throw HTTPServerError.failedToStart case .cancelled: throw HTTPServerError.failedToStart + case .waiting(let error): + // Not a failure yet, but if the listener stays here the start + // times out, and the timeout error doesn't say why. This does. + Logger.httpServer.warning("Listener is waiting to start: \(error, privacy: .public)") default: continue } @@ -277,6 +303,67 @@ 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) } + ) + } + + /// Logs a `.failed` or `.waiting` that the listener reports after it's ready. + /// + /// A listener that fails after starting leaves the server handing out a `port` and + /// `token` that no longer work, so it's worth a line in the log. This only hears about + /// problems Network.framework reports, though: if the system shuts the socket down (as + /// iOS can for a suspended app), connections are refused but the listener still + /// reports `.ready`, so this is never called. + /// + /// `.cancelled` isn't logged. It only follows a call to `cancel()`, which is normal + /// shutdown. + private static func logStateAfterStart(_ state: NWListener.State, port: UInt16) { + switch state { + case .failed(let error): + Logger.httpServer.error("Listener on port \(port) failed after a successful start: \(error, privacy: .public)") + case .waiting(let error): + Logger.httpServer.warning("Listener on port \(port) is waiting after a successful start: \(error, privacy: .public)") + default: + break + } + } + /// 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 — @@ -372,160 +459,175 @@ public final class HTTPServer: Sendable { connectionCounter.decrement() } - do { - let parser = HTTPRequestParser(maxBodySize: maxRequestBodySize, tempDirectory: tempDirectory) - var request: ParsedHTTPRequest! - let duration = try await ContinuousClock().measure { - // Phase 1 (pre-body): receive and validate headers, authenticate, - // and drain any oversized body — all bounded by `readTimeout`. This is - // the unauthenticated-reachable portion of the request, so it keeps a - // strict total-duration cap. - let partial = try await Self.withReadTimeout(readTimeout) { () -> ParsedHTTPRequest in - // Receive headers only. - try await Self.receiveUntil(\.hasHeaders, parser: parser, on: connection, idleTimeout: idleTimeout) - - // Validate headers (triggers full RFC validation). - guard let partial = try parser.parseRequest() else { - throw HTTPServerError.connectionClosed - } + // Everything this connection does, from the first byte of the request to the + // last byte of the response, runs inside the delegate's activity. See + // `HTTPServerDelegate.withConnectionActivity(_:)`. + await Self.withConnectionActivity(of: delegate) { + do { + let parser = HTTPRequestParser(maxBodySize: maxRequestBodySize, tempDirectory: tempDirectory) + var request: ParsedHTTPRequest! + let duration = try await ContinuousClock().measure { + // Phase 1 (pre-body): receive and validate headers, authenticate, + // and drain any oversized body — all bounded by `readTimeout`. This is + // the unauthenticated-reachable portion of the request, so it keeps a + // strict total-duration cap. + let partial = try await Self.withReadTimeout(readTimeout) { () -> ParsedHTTPRequest in + // Receive headers only. + try await Self.receiveUntil(\.hasHeaders, parser: parser, on: connection, idleTimeout: idleTimeout) + + // Validate headers (triggers full RFC validation). + guard let partial = try parser.parseRequest() else { + throw HTTPServerError.connectionClosed + } - // Check auth on headers alone, before draining or consuming any - // body bytes — an unauthenticated client must not be able to make - // the server read (and discard) an arbitrarily large body, and the - // handler must never see an unauthenticated request. OPTIONS is - // exempt because CORS preflight requests never include credentials - // (Fetch spec §3.3.5). - if requiresAuthentication && partial.method.uppercased() != "OPTIONS" { - guard authenticate(partial, token: token) else { - throw HTTPServerError.authenticationFailed + // Check auth on headers alone, before draining or consuming any + // body bytes — an unauthenticated client must not be able to make + // the server read (and discard) an arbitrarily large body, and the + // handler must never see an unauthenticated request. OPTIONS is + // exempt because CORS preflight requests never include credentials + // (Fetch spec §3.3.5). + if requiresAuthentication && partial.method.uppercased() != "OPTIONS" { + guard authenticate(partial, token: token) else { + throw HTTPServerError.authenticationFailed + } } - } - // Reject auth-exempt OPTIONS that carry a body. Real CORS preflight - // requests are bodyless; a body on the auth-exempt path would - // otherwise be read/drained without authentication — and the - // accepted-body read below is bounded only by the idle timeout. - if partial.method.uppercased() == "OPTIONS", (parser.expectedBodyLength ?? 0) > 0 { - throw HTTPServerError.unexpectedBody - } + // Reject auth-exempt OPTIONS that carry a body. Real CORS preflight + // requests are bodyless; a body on the auth-exempt path would + // otherwise be read/drained without authentication — and the + // accepted-body read below is bounded only by the idle timeout. + if partial.method.uppercased() == "OPTIONS", (parser.expectedBodyLength ?? 0) > 0 { + throw HTTPServerError.unexpectedBody + } - // Drain the oversized body before responding so the (authenticated) - // client receives the 413 instead of a connection reset - // (RFC 9110 §15.5.14). Still bounded by `readTimeout`. - if parser.state == .draining { - try await Self.receiveUntil(\.isComplete, parser: parser, on: connection, idleTimeout: idleTimeout) - } + // Drain the oversized body before responding so the (authenticated) + // client receives the 413 instead of a connection reset + // (RFC 9110 §15.5.14). Still bounded by `readTimeout`. + if parser.state == .draining { + try await Self.receiveUntil(\.isComplete, parser: parser, on: connection, idleTimeout: idleTimeout) + } - return partial - } + return partial + } - // If the parser detected a recoverable error (e.g. payload too - // large, drained above), stop reading and let the post-measure - // branch answer it via the delegate. `request` is the body-less - // partial; the main handler is never invoked for it. - if parser.parseError != nil { - request = partial - return - } + // If the parser detected a recoverable error (e.g. payload too + // large, drained above), stop reading and let the post-measure + // branch answer it via the delegate. `request` is the body-less + // partial; the main handler is never invoked for it. + if parser.parseError != nil { + request = partial + return + } - // Reject body-bearing methods without Content-Length. We don't support - // Transfer-Encoding: chunked, so Content-Length is the only way to - // determine body size. - let upperMethod = partial.method.uppercased() - if ["POST", "PUT", "PATCH"].contains(upperMethod) && partial.header("Content-Length") == nil { - throw HTTPServerError.lengthRequired - } + // Reject body-bearing methods without Content-Length. We don't support + // Transfer-Encoding: chunked, so Content-Length is the only way to + // determine body size. + let upperMethod = partial.method.uppercased() + if ["POST", "PUT", "PATCH"].contains(upperMethod) && partial.header("Content-Length") == nil { + throw HTTPServerError.lengthRequired + } - // Phase 2 (accepted body): the client is authenticated, so read the body - // bounded by `bodyReadTimeout` (a generous backstop) plus the per-read - // `idleTimeout`. A large upload that streams steadily is never failed on - // total duration — only a genuine stall (idle) or the generous ceiling - // ends it. - if !parser.state.isComplete { - try await Self.withReadTimeout(bodyReadTimeout) { - try await Self.receiveUntil(\.isComplete, parser: parser, on: connection, idleTimeout: idleTimeout) + // Phase 2 (accepted body): the client is authenticated, so read the body + // bounded by `bodyReadTimeout` (a generous backstop) plus the per-read + // `idleTimeout`. A large upload that streams steadily is never failed on + // total duration — only a genuine stall (idle) or the generous ceiling + // ends it. + if !parser.state.isComplete { + try await Self.withReadTimeout(bodyReadTimeout) { + try await Self.receiveUntil(\.isComplete, parser: parser, on: connection, idleTimeout: idleTimeout) + } } - } - guard let complete = try parser.parseRequest(), complete.isComplete else { - throw HTTPServerError.connectionClosed + guard let complete = try parser.parseRequest(), complete.isComplete else { + throw HTTPServerError.connectionClosed + } + request = complete } - request = complete - } - // A recoverable parse error (payload too large, drained above): the - // request was never fully read, so it must not reach the handler. - // The library owns the response — the delegate customizes the body if - // it wants, otherwise a correct generic error. `send` stamps CORS. - let response: HTTPResponse - if let parseError = parser.parseError { - response = delegate?.response(forRecoverableParseError: parseError) - ?? Self.defaultErrorResponse(for: parseError) - } else if cors == .permissive, request.method.uppercased() == "OPTIONS" { - // Under a permissive CORS policy the library answers the OPTIONS - // preflight itself; the send layer stamps the CORS headers. - response = HTTPResponse(status: 204) - } else { - // Run the handler, but race it against the peer closing the - // connection. Once the request has been fully read, no bytes - // flow on this connection until the response is sent, so a - // handler that awaits slow outbound work — the media-upload - // relay awaiting `POST /wp/v2/media` — leaves the connection - // idle. If the client (the editor WebView) aborts the upload - // during that window, nothing here would otherwise notice, and - // the outbound request would run to completion, creating an - // orphaned attachment that a retry then duplicates. Watching for - // the close and cancelling the handler propagates cancellation - // through structured concurrency to the outbound URLSession - // task, so a cancelled upload is actually cancelled. - switch await Self.runHandler( - handler, - Request(parsed: request, parseDuration: duration), - racingCloseOf: connection - ) { - case .completed(let handlerResponse): - response = handlerResponse - case .clientDisconnected: - Logger.httpServer.debug("\(request.method) \(request.target) → client disconnected before response; cancelled in-flight handler") - connection.cancel() - return + // A recoverable parse error (payload too large, drained above): the + // request was never fully read, so it must not reach the handler. + // The library owns the response — the delegate customizes the body if + // it wants, otherwise a correct generic error. `send` stamps CORS. + let response: HTTPResponse + if let parseError = parser.parseError { + response = delegate?.response(forRecoverableParseError: parseError) + ?? Self.defaultErrorResponse(for: parseError) + } else if cors == .permissive, request.method.uppercased() == "OPTIONS" { + // Under a permissive CORS policy the library answers the OPTIONS + // preflight itself; the send layer stamps the CORS headers. + response = HTTPResponse(status: 204) + } else { + // Run the handler, but race it against the peer closing the + // connection. Once the request has been fully read, no bytes + // flow on this connection until the response is sent, so a + // handler that awaits slow outbound work — the media-upload + // relay awaiting `POST /wp/v2/media` — leaves the connection + // idle. If the client (the editor WebView) aborts the upload + // during that window, nothing here would otherwise notice, and + // the outbound request would run to completion, creating an + // orphaned attachment that a retry then duplicates. Watching for + // the close and cancelling the handler propagates cancellation + // through structured concurrency to the outbound URLSession + // task, so a cancelled upload is actually cancelled. + switch await Self.runHandler( + handler, + Request(parsed: request, parseDuration: duration), + racingCloseOf: connection + ) { + case .completed(let handlerResponse): + response = handlerResponse + case .clientDisconnected: + Logger.httpServer.debug("\(request.method) \(request.target) → client disconnected before response; cancelled in-flight handler") + connection.cancel() + return + } } + // The handler type is non-throwing and maps cancellation to a 500, + // so if the connection task was cancelled while it ran (server stop / + // editor teardown), honor that here rather than writing a doomed + // response: propagate so the outer handler just closes the connection. + try Task.checkCancellation() + await send(response, on: connection, cors: cors) + let (sec, atto) = duration.components + let ms = Double(sec) * 1000.0 + Double(atto) / 1_000_000_000_000_000.0 + Logger.httpServer.debug("\(request.method) \(request.target) → \(response.status) (\(String(format: "%.1f", ms))ms)") + } catch HTTPServerError.authenticationFailed { + await send(HTTPResponse(status: 407, headers: [("Content-Type", "text/plain"), ("Proxy-Authenticate", "Bearer")]), on: connection, cors: cors) + } catch HTTPServerError.lengthRequired { + await send(HTTPResponse(status: 411, statusText: "Length Required", body: Data("Length Required".utf8)), on: connection, cors: cors) + } catch HTTPServerError.unexpectedBody { + Logger.httpServer.warning("Rejected auth-exempt request carrying a body") + await send(HTTPResponse(status: 400, statusText: "Bad Request", body: Data("Unexpected request body".utf8)), on: connection, cors: cors) + } catch is CancellationError { + Logger.httpServer.debug("Connection cancelled during shutdown") + connection.cancel() + } catch HTTPServerError.readTimeout { + Logger.httpServer.warning("Read timeout, closing connection") + await send(HTTPResponse(status: 408, statusText: "Request Timeout", body: Data("Request Timeout".utf8)), on: connection, cors: cors) + } catch let error as HTTPRequestParseError { + // Fatal parse error (malformed framing, smuggling-relevant, etc.): + // always answered by the library, never routed to the delegate. + Logger.httpServer.error("Parse error: \(error)") + await send(Self.defaultErrorResponse(for: error), on: connection, cors: cors) + } catch { + Logger.httpServer.error("Unexpected error: \(error)") + await send(HTTPResponse(status: 400, statusText: "Bad Request", body: Data("Malformed HTTP request".utf8)), on: connection, cors: cors) } - // The handler type is non-throwing and maps cancellation to a 500, - // so if the connection task was cancelled while it ran (server stop / - // editor teardown), honor that here rather than writing a doomed - // response: propagate so the outer handler just closes the connection. - try Task.checkCancellation() - await send(response, on: connection, cors: cors) - let (sec, atto) = duration.components - let ms = Double(sec) * 1000.0 + Double(atto) / 1_000_000_000_000_000.0 - Logger.httpServer.debug("\(request.method) \(request.target) → \(response.status) (\(String(format: "%.1f", ms))ms)") - } catch HTTPServerError.authenticationFailed { - await send(HTTPResponse(status: 407, headers: [("Content-Type", "text/plain"), ("Proxy-Authenticate", "Bearer")]), on: connection, cors: cors) - } catch HTTPServerError.lengthRequired { - await send(HTTPResponse(status: 411, statusText: "Length Required", body: Data("Length Required".utf8)), on: connection, cors: cors) - } catch HTTPServerError.unexpectedBody { - Logger.httpServer.warning("Rejected auth-exempt request carrying a body") - await send(HTTPResponse(status: 400, statusText: "Bad Request", body: Data("Unexpected request body".utf8)), on: connection, cors: cors) - } catch is CancellationError { - Logger.httpServer.debug("Connection cancelled during shutdown") - connection.cancel() - } catch HTTPServerError.readTimeout { - Logger.httpServer.warning("Read timeout, closing connection") - await send(HTTPResponse(status: 408, statusText: "Request Timeout", body: Data("Request Timeout".utf8)), on: connection, cors: cors) - } catch let error as HTTPRequestParseError { - // Fatal parse error (malformed framing, smuggling-relevant, etc.): - // always answered by the library, never routed to the delegate. - Logger.httpServer.error("Parse error: \(error)") - await send(Self.defaultErrorResponse(for: error), on: connection, cors: cors) - } catch { - Logger.httpServer.error("Unexpected error: \(error)") - await send(HTTPResponse(status: 400, statusText: "Bad Request", body: Data("Malformed HTTP request".utf8)), on: connection, cors: cors) } } connectionTasks.track(taskID, task) } + /// Runs `body` inside the delegate's ``HTTPServerDelegate/withConnectionActivity(_:)``, + /// or on its own when the server has no delegate. + private static func withConnectionActivity( + of delegate: HTTPServerDelegate?, + _ body: () async -> Void + ) async { + guard let delegate else { return await body() } + await delegate.withConnectionActivity(body) + } + /// Runs `operation` under a total-duration timeout, racing it against a sleep /// task. Used to bound one phase of the request read (pre-body vs. accepted /// body). The per-read `idleTimeout` inside `operation` still applies diff --git a/ios/Sources/GutenbergKitHTTP/HTTPServerDelegate.swift b/ios/Sources/GutenbergKitHTTP/HTTPServerDelegate.swift index 281533746..5897bd2fb 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPServerDelegate.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPServerDelegate.swift @@ -30,12 +30,30 @@ public protocol HTTPServerDelegate: AnyObject, Sendable { /// Fatal parse errors (malformed framing, header smuggling, etc.) are always /// answered by the library and never routed here. func response(forRecoverableParseError error: HTTPRequestParseError) -> HTTPResponse + + /// Runs `body`, which serves one connection: reading the request, running the + /// handler, and writing the response. The server's own responses (407, 408, 413, + /// and so on) are written inside it too. + /// + /// `body` returns once the response has been handed to the network stack, so + /// anything wrapped around it covers the whole exchange with the client. That's + /// what makes it the place to keep the process alive for a connection, e.g. with + /// a background-task assertion. Wrapping only the handler would miss both ends: + /// the client sending the request body before the handler runs, and the server + /// writing the response after it returns. + /// + /// The default runs `body` and nothing else. + func withConnectionActivity(_ body: () async -> Void) async } public extension HTTPServerDelegate { func response(forRecoverableParseError error: HTTPRequestParseError) -> HTTPResponse { HTTPServer.defaultErrorResponse(for: error) } + + func withConnectionActivity(_ body: () async -> Void) async { + await body() + } } #endif // canImport(Network) diff --git a/ios/Sources/GutenbergKitHTTP/README.md b/ios/Sources/GutenbergKitHTTP/README.md index 43721b305..86c14e380 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 a handler must not strongly hold the object that owns the server, directly or transitively — `owner → HTTPServer → handler → owner` is a cycle, and the owner's `deinit` would never run. A value type is **not** protection here: a `struct` handler storing the owner closes the same ring, because the server captures the struct into a heap node and its stored properties are strong edges out of it. `HTTPRequestHandler` is deliberately not `AnyObject`-constrained so a handler *can* be a `struct` holding only what it needs — not because a `struct` is safe by construction. Either shape works, 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/HTTPServerConnectionActivityTests.swift b/ios/Tests/GutenbergKitHTTPTests/HTTPServerConnectionActivityTests.swift new file mode 100644 index 000000000..a20d1642a --- /dev/null +++ b/ios/Tests/GutenbergKitHTTPTests/HTTPServerConnectionActivityTests.swift @@ -0,0 +1,176 @@ +#if canImport(Network) + +import Foundation +import Network +import Testing +@testable import GutenbergKitHTTP + +/// Covers ``HTTPServerDelegate/withConnectionActivity(_:)``: the scope a delegate wraps +/// around a connection has to cover the whole exchange with the client, not just the +/// handler. The media upload server holds a background-task assertion there, and a gap +/// at either end is a window where locking the phone suspends the app mid-upload. +/// +/// Each test would hang rather than pass if the scope were narrower, so every wait has a +/// timeout and a failure reads as the gap it found. +@Suite("HTTPServer Connection Activity") +struct HTTPServerConnectionActivityTests { + + @Test("the activity starts before the request body arrives and ends after the response is written") + func activitySpansTheWholeExchange() async throws { + let delegate = RecordingActivityDelegate() + let handlerRanInside = TestFlag() + let server = try await HTTPServer.start( + name: "activity-whole-exchange", + requiresAuthentication: true, + delegate: delegate + ) { _ in + if delegate.isInside { handlerRanInside.raise() } + return HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + defer { server.stop() } + + let connection = try await connect(toPort: server.port) + defer { connection.cancel() } + let body = Data("hello".utf8) + let header = "POST /upload HTTP/1.1\r\nHost: 127.0.0.1\r\nProxy-Authorization: Bearer \(server.token)\r\nContent-Length: \(body.count)\r\n\r\n" + try await send(Data(header.utf8), on: connection) + + // The body hasn't been sent, so the handler can't have run. An activity that only + // wrapped the handler wouldn't have started yet. + let startedBeforeBody = await delegate.entered.wait(timeout: .seconds(3)) + #expect(startedBeforeBody, "the activity didn't start until the request body arrived") + + try await send(body, on: connection) + let response = try await receiveResponse(on: connection) + delegate.clientHasResponse.raise() + + #expect(response.hasPrefix("HTTP/1.1 200")) + #expect(handlerRanInside.isRaised, "the handler ran outside the activity") + let exited = await delegate.exited.wait(timeout: .seconds(5)) + #expect(exited, "the activity never ended") + #expect(delegate.responseArrivedInside == true, "the response was written after the activity ended") + } + + @Test("the server's own responses are written inside the activity too") + func libraryResponsesAreWrittenInsideTheActivity() async throws { + let delegate = RecordingActivityDelegate() + let server = try await HTTPServer.start( + name: "activity-library-response", + requiresAuthentication: true, + delegate: delegate + ) { _ in + HTTPResponse(status: 200, body: Data("OK\n".utf8)) + } + defer { server.stop() } + + // No token, so the server answers 407 itself and the handler never runs. + let connection = try await connect(toPort: server.port) + defer { connection.cancel() } + let request = "POST /upload HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: 0\r\n\r\n" + try await send(Data(request.utf8), on: connection) + let response = try await receiveResponse(on: connection) + delegate.clientHasResponse.raise() + + #expect(response.hasPrefix("HTTP/1.1 407")) + let exited = await delegate.exited.wait(timeout: .seconds(5)) + #expect(exited, "the activity never ended") + #expect(delegate.responseArrivedInside == true, "the 407 was written after the activity ended") + } + + // MARK: - Helpers + + private func connect(toPort port: UInt16) async throws -> NWConnection { + let connection = NWConnection( + host: .ipv4(.loopback), + port: NWEndpoint.Port(rawValue: port)!, + using: .tcp + ) + try await withCheckedThrowingContinuation { (cont: CheckedContinuation) in + connection.stateUpdateHandler = { state in + switch state { + case .ready: + connection.stateUpdateHandler = nil + cont.resume() + case .failed(let error): + connection.stateUpdateHandler = nil + cont.resume(throwing: error) + default: + break + } + } + connection.start(queue: .global()) + } + return connection + } + + private func send(_ data: Data, on connection: NWConnection) async throws { + try await withCheckedThrowingContinuation { (cont: CheckedContinuation) in + connection.send(content: data, completion: .contentProcessed { error in + if let error { cont.resume(throwing: error) } else { cont.resume() } + }) + } + } + + private func receiveResponse(on connection: NWConnection) async throws -> String { + try await withCheckedThrowingContinuation { (cont: CheckedContinuation) in + connection.receive(minimumIncompleteLength: 1, maximumLength: 8192) { data, _, _, error in + if let error { + cont.resume(throwing: error) + } else { + cont.resume(returning: String(data: data ?? Data(), encoding: .utf8) ?? "") + } + } + } + } +} + +/// Records what happens inside its connection activity. +/// +/// After `body` returns it waits for the test to say the client has the response. If the +/// server wrote the response only after `body` returned, the client can't have it until +/// this returns, so the wait runs out and ``responseArrivedInside`` is `false`. +private final class RecordingActivityDelegate: HTTPServerDelegate, @unchecked Sendable { + let entered = TestFlag() + let clientHasResponse = TestFlag() + let exited = TestFlag() + + private let lock = NSLock() + private var inside = false + private var arrivedInside: Bool? + + var isInside: Bool { lock.withLock { inside } } + var responseArrivedInside: Bool? { lock.withLock { arrivedInside } } + + func withConnectionActivity(_ body: () async -> Void) async { + lock.withLock { inside = true } + entered.raise() + await body() + let arrived = await clientHasResponse.wait(timeout: .seconds(3)) + lock.withLock { + inside = false + arrivedInside = arrived + } + exited.raise() + } +} + +/// A one-way flag a test can wait on. +private final class TestFlag: @unchecked Sendable { + private let lock = NSLock() + private var raised = false + + func raise() { lock.withLock { raised = true } } + var isRaised: Bool { lock.withLock { raised } } + + func wait(timeout: Duration) async -> Bool { + let clock = ContinuousClock() + let deadline = clock.now + timeout + while clock.now < deadline { + if isRaised { return true } + try? await Task.sleep(for: .milliseconds(10)) + } + return isRaised + } +} + +#endif // canImport(Network) diff --git a/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift b/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift index 17be7d603..fc86f22a9 100644 --- a/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift +++ b/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift @@ -38,6 +38,36 @@ 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, holding only a `String` — so it has no strong edge back to +/// whatever owns the server. ``HTTPRequestHandler`` isn't `AnyObject`-constrained so a +/// handler *can* take this shape; a `struct` storing the owner would still cycle. +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/EditorViewControllerLifecycleTests.swift b/ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift new file mode 100644 index 000000000..5e3aa8102 --- /dev/null +++ b/ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift @@ -0,0 +1,164 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +#if canImport(UIKit) +import UIKit + +/// What happens to an in-flight dependency fetch when its editor is covered or +/// released. Nothing restarts the fetch, so the editor never recovers from a cancel. +@Suite("EditorViewController dependency fetch lifecycle") +struct EditorViewControllerLifecycleTests: 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("covering the editor leaves the dependency fetch running") + func coveringTheEditorDoesNotCancelTheDependencyFetch() async throws { + let session = ParkedURLSession() + let configuration = makeIsolatedConfiguration() + defer { removeStorage(for: configuration) } + defer { session.release() } + let editor = makeEditor(configuration: configuration, session: session) + + _ = editor.view // triggers `viewDidLoad`, which starts the fetch + try await session.waitUntilStarted() + + // The editor appears, then a full-screen modal or a push covers it. + editor.beginAppearanceTransition(true, animated: false) + editor.endAppearanceTransition() + editor.beginAppearanceTransition(false, animated: false) + editor.endAppearanceTransition() + + let cancelled = await session.waitUntilCancelled(timeout: .milliseconds(500)) + #expect(!cancelled) + } + + /// Why `deinit` can't cancel the fetch: `await self?.prepareEditor()` keeps the + /// editor alive until the load finishes, so `deinit` only runs once it's over. + @MainActor + @Test("the in-flight fetch keeps the editor alive until it finishes") + func theInFlightFetchKeepsTheEditorAlive() async throws { + let session = ParkedURLSession() + let configuration = makeIsolatedConfiguration() + defer { removeStorage(for: configuration) } + // Safety net if a throw skips the `release()` below; calling it twice is fine. + defer { session.release() } + var editor: EditorViewController? = makeEditor(configuration: configuration, session: session) + weak let releasedEditor = editor + + _ = editor?.view + try await session.waitUntilStarted() + + editor = nil + try await Task.sleep(for: .milliseconds(250)) + #expect(releasedEditor != nil, "the fetch should hold the editor alive") + + session.release() + let clock = ContinuousClock() + let deadline = clock.now + .seconds(10) + while releasedEditor != nil && clock.now < deadline { + try await Task.sleep(for: .milliseconds(20)) + } + #expect(releasedEditor == nil, "the editor should be freed once the fetch ends") + } + + /// A unique `siteId` per call, so no earlier run's cache can serve the fetch. + /// Pair every call with `removeStorage(for:)`: nothing else deletes the site's files. + private func makeIsolatedConfiguration() -> EditorConfiguration { + makeConfiguration( + siteURL: URL(string: "https://\(UUID().uuidString).example.invalid")! + ) + } + + /// An editor whose every network call lands in `session`. + @MainActor + private func makeEditor( + configuration: EditorConfiguration, + session: ParkedURLSession + ) -> EditorViewController { + EditorViewController( + configuration: configuration, + httpClient: EditorHTTPClient(urlSession: session, authHeader: configuration.authHeader) + ) + } + + /// Deletes what the editor wrote for this site. `EditorViewController` can't be + /// pointed at a temporary directory the way `MakesTestFixtures.makeService` can. + private func removeStorage(for configuration: EditorConfiguration) { + try? FileManager.default.removeItem(at: Paths.storageRoot(for: configuration)) + try? FileManager.default.removeItem(at: Paths.cacheRoot(for: configuration)) + } +} + +/// A `URLSessionProtocol` whose requests hang until `release()`, so a fetch stays in +/// flight for as long as the test needs. Records whether any request was cancelled. +/// +/// Also lets a test load the editor's view, which starts the dependency fetch, without +/// the editor then loading a page over whatever the test put in its WebView. +final class ParkedURLSession: URLSessionProtocol, @unchecked Sendable { + private let lock = NSLock() + private var started = false + private var cancelled = false + private var released = false + + private var isStarted: Bool { lock.withLock { started } } + private var isCancelled: Bool { lock.withLock { cancelled } } + private var isReleased: Bool { lock.withLock { released } } + + func data(for request: URLRequest) async throws -> (Data, URLResponse) { + try await park() + } + + func download(for request: URLRequest, delegate: (any URLSessionTaskDelegate)?) async throws -> (URL, URLResponse) { + try await park() + } + + /// Makes every parked request fail, so the fetch ends. Always call it: a request + /// left parked keeps its editor alive for the rest of the run. + func release() { + lock.withLock { released = true } + } + + /// Suspends until `release()` or until the calling task is cancelled. + /// `Never` because every exit throws, so it fits both methods' return types. + private func park() async throws -> Never { + lock.withLock { started = true } + while !isReleased { + do { + try await Task.sleep(for: .milliseconds(20)) + } catch { + lock.withLock { cancelled = true } + throw URLError(.cancelled) + } + } + throw URLError(.networkConnectionLost) + } + + func waitUntilStarted(timeout: Duration = .seconds(10)) async throws { + let clock = ContinuousClock() + let deadline = clock.now + timeout + while clock.now < deadline { + if isStarted { return } + try await Task.sleep(for: .milliseconds(20)) + } + throw ParkedURLSessionTimeout.requestNeverStarted + } + + func waitUntilCancelled(timeout: Duration) async -> Bool { + let clock = ContinuousClock() + let deadline = clock.now + timeout + while clock.now < deadline { + if isCancelled { return true } + try? await Task.sleep(for: .milliseconds(20)) + } + return isCancelled + } +} + +enum ParkedURLSessionTimeout: Error { + case requestNeverStarted +} + +#endif diff --git a/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift b/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift index 726b44746..9387d91b6 100644 --- a/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift @@ -4,11 +4,12 @@ import Testing @testable import GutenbergKit #if canImport(UIKit) +import WebKit /// Pins that ``EditorViewController/stopMediaHandling()`` opens the ownership cycle a host /// can form, and that a host which doesn't form one needs nothing. /// -/// The editor holds `mediaUploadDelegate` strongly so an in-flight upload can't lose it +/// The editor holds `mediaProcessor` strongly so an in-flight upload can't lose it /// mid-request. The cost is that a host which holds the editor back closes a cycle ARC /// cannot break — and `deinit`, which does this work on every other path, is exactly what /// a cycle prevents. `stopMediaHandling()` is the way out, and it has to be the host's @@ -21,13 +22,13 @@ struct EditorViewControllerMediaTeardownTests: MakesTestFixtures { static let testApiRoot = URL(string: "https://test.example.com/wp-json/wp/v2")! @MainActor - @Test("stopMediaHandling frees the editor and the host delegate that owns it") + @Test("stopMediaHandling frees the editor and the host processor that owns it") func stopMediaHandlingBreaksTheOwnershipCycle() async { weak var weakEditor: EditorViewController? - weak var weakHost: EditorOwningDelegate? + weak var weakHost: EditorOwningProcessor? do { - let host = EditorOwningDelegate(configuration: makeConfiguration()) + let host = EditorOwningProcessor(configuration: makeConfiguration()) weakEditor = host.editor weakHost = host host.editor.stopMediaHandling() @@ -35,19 +36,19 @@ struct EditorViewControllerMediaTeardownTests: MakesTestFixtures { await waitForRelease { weakHost == nil && weakEditor == nil } - #expect(weakHost == nil, "host delegate leaked — stopMediaHandling did not release it") - #expect(weakEditor == nil, "EditorViewController leaked — cycle through mediaUploadDelegate") + #expect(weakHost == nil, "host processor leaked — stopMediaHandling did not release it") + #expect(weakEditor == nil, "EditorViewController leaked — cycle through mediaProcessor") } @MainActor @Test("a host that does not retain the editor is freed without stopMediaHandling") - func standaloneDelegateIsFreed() async { + func standaloneProcessorIsFreed() async { weak var weakEditor: EditorViewController? do { let editor = EditorViewController( configuration: makeConfiguration(), - mediaUploadDelegate: StandaloneDelegate() + mediaProcessor: StandaloneProcessor() ) weakEditor = editor } @@ -57,6 +58,354 @@ struct EditorViewControllerMediaTeardownTests: MakesTestFixtures { #expect(weakEditor == nil, "EditorViewController leaked — nothing here retains it") } + // MARK: - Which handlers bring the server up + + /// The regression this pins: `startUploadServer()` reads "did the host supply a + /// handler" twice — once before starting, once after the bind returns — and the two + /// reads drifted. The first gained `mediaUploader`, the second kept checking the + /// processor alone, so an uploader-only host bound a listener and then immediately + /// stopped it. `uploadServer` stayed nil, the page was advertised `nativeUploadPort: + /// nil`, and `api-fetch.js` fell through to the plain WebView path — so the host's + /// `upload(_:)` was never called for any file, with nothing logged. + /// + /// Android pins the same gate (`GutenbergViewUploadServerTest`, "the upload server + /// starts for an uploader with no processor"); iOS had no equivalent, which is why the + /// drift survived three commits with a green suite. + @MainActor + @Test( + "the upload server starts for whichever handler the host supplied", + .enabled(if: canBindUploadServer), + arguments: [ + ("uploader only", false, true), + ("processor only", true, false), + ("both", true, true) + ] + ) + func uploadServerStartsForAnyHandler(_ label: String, processor: Bool, uploader: Bool) async { + let editor = EditorViewController( + configuration: makeConfiguration(), + mediaProcessor: processor ? StandaloneProcessor() : nil, + mediaUploader: uploader ? InertUploader() : nil + ) + defer { editor.stopMediaHandling() } + + await editor.startUploadServer() + + #expect(editor.uploadServer != nil, "\(label): no upload server, so the host's media handling never runs") + } + + // MARK: - Coming back from the background + + /// The failure this file's sibling PR is named for. Once the device can idle-sleep, the + /// system reclaims a suspended app's listening socket and reports nothing, so the editor + /// returns advertising a port that refuses connections, and every upload in that session + /// fails. Stopping the server behind the editor's back leaves exactly that state. + @MainActor + @Test("an upload server whose port stopped answering is replaced", .enabled(if: canBindUploadServer)) + func restartsAnUnreachableUploadServer() async throws { + let editor = EditorViewController( + configuration: makeConfiguration(), + mediaProcessor: StandaloneProcessor() + ) + defer { editor.stopMediaHandling() } + await editor.startUploadServer() + + guard let original = editor.uploadServer else { + Issue.record("no upload server to begin with") + return + } + original.stop() + try await waitUntilSilent(original) + + await editor.restartUploadServerIfUnreachable() + + guard let restarted = editor.uploadServer else { + Issue.record("the editor was left without a server, so uploads fall back to the WebView path") + return + } + #expect(restarted !== original, "kept the server whose port had stopped answering") + #expect(await restarted.isAnswering(), "the replacement server does not answer either") + } + + /// The other half: a check that runs on every foreground must not churn the port, which + /// would mean re-advertising it to the page for no reason. + @MainActor + @Test("an upload server that still answers is left alone", .enabled(if: canBindUploadServer)) + func leavesAnAnsweringUploadServerAlone() async { + let editor = EditorViewController( + configuration: makeConfiguration(), + mediaProcessor: StandaloneProcessor() + ) + defer { editor.stopMediaHandling() } + await editor.startUploadServer() + let original = editor.uploadServer + + await editor.restartUploadServerIfUnreachable() + + #expect(editor.uploadServer === original, "replaced a server that was answering") + } + + /// Checks can overlap: every foreground starts one, and each waits on its probe. When + /// both find the port dead, only the first may restart the server. The second resumes + /// holding a verdict about a server that has already been replaced, and acting on it + /// throws away the fresh one, along with any upload the page has already sent to it. + /// + /// The probes answer only when the test says so, so the second check resumes after the + /// first has finished restarting — the order that loses the fresh server. + @MainActor + @Test( + "a check that resumes after another restarted the server leaves the new one alone", + .enabled(if: canBindUploadServer) + ) + func overlappingChecksRestartTheServerOnce() async throws { + let editor = EditorViewController( + configuration: makeConfiguration(), + mediaProcessor: StandaloneProcessor() + ) + defer { editor.stopMediaHandling() } + await editor.startUploadServer() + let original = try #require(editor.uploadServer) + original.stop() + try await waitUntilSilent(original) + + let firstProbe = HeldProbe() + let secondProbe = HeldProbe() + let firstCheck = Task { await editor.restartUploadServerIfUnreachable(isAnswering: firstProbe.ask) } + let secondCheck = Task { await editor.restartUploadServerIfUnreachable(isAnswering: secondProbe.ask) } + try await firstProbe.waitUntilAsked() + try await secondProbe.waitUntilAsked() + + firstProbe.answer(false) + await firstCheck.value + let restarted = try #require(editor.uploadServer, "the first check left the editor without a server") + #expect(restarted !== original, "the first check didn't replace the dead server") + + secondProbe.answer(false) + await secondCheck.value + #expect(editor.uploadServer === restarted, "the late check replaced the server the first one had just started") + #expect(await restarted.isAnswering(), "the server the first check started no longer answers") + } + + /// What the page goes through between losing the socket and the restart, run in the + /// editor's own `WKWebView` from a `file://` page, which is where the editor loads from. + /// + /// An upload sent in that window fails, reaching nothing; the window closes promptly; and + /// the next upload after the restart lands on the new port. The request is the one + /// `nativeMediaUploadMiddleware` sends, built from `window.GBKit`. The middleware's own + /// part is covered in JS: that it reads the endpoint on every request + /// (`api-fetch-upload-middleware.test.js`) from the live `window.GBKit` + /// (`bridge.test.js`), and that it neither retries a rejected `fetch` nor falls back to + /// the WebView's own upload. + @MainActor + @Test( + "an upload sent before the restart fails, and the next one lands on the new port", + .enabled(if: canBindUploadServer) + ) + func uploadFailsUntilTheRestartThenLands() async throws { + let uploader = CountingUploader() + let editor = EditorViewController(configuration: makeConfiguration(), mediaUploader: uploader) + defer { editor.stopMediaHandling() } + await editor.startUploadServer() + let original = try #require(editor.uploadServer) + + try await loadFilePage(in: editor.webView) + // What the injected user script sets at document start. + try await editor.webView.callAsyncJavaScript( + "window.GBKit = { nativeUploadPort: port, nativeUploadToken: token };", + arguments: ["port": Int(original.port), "token": original.token], + contentWorld: .page + ) + + let beforeLoss = try await sendNativeUpload(from: editor.webView) + #expect(beforeLoss.status == 201, "the upload never worked, so the rest proves nothing: \(beforeLoss)") + + original.stop() + try await waitUntilSilent(original) + + let duringLoss = try await sendNativeUpload(from: editor.webView) + #expect(duringLoss.port == Int(original.port)) + #expect(duringLoss.error != nil, "an upload to the dead port didn't fail: \(duringLoss)") + // Failing is the point; failing *promptly* rules out WebKit holding the request open + // and presenting a hang instead. + #expect(duringLoss.milliseconds < 1000, "the upload to the dead port hung: \(duringLoss)") + #expect(uploader.uploads == 1, "the upload sent to the dead port reached the uploader") + + // Until this returns, the page holds the dead port and every upload fails like the + // one above, so its duration is how long that lasts after the app comes back. + let restartDuration = await ContinuousClock().measure { + await editor.restartUploadServerIfUnreachable() + } + #expect(restartDuration < .seconds(1), "took \(restartDuration) to replace the dead server") + let restarted = try #require(editor.uploadServer) + + let afterRestart = try await sendNativeUpload(from: editor.webView) + #expect(afterRestart.port == Int(restarted.port), "the page still holds the old port") + #expect(afterRestart.status == 201, "the upload after the restart didn't land: \(afterRestart)") + #expect(uploader.uploads == 2) + } + + // MARK: - While the app is in the foreground + + /// The socket can also go while the app is in the foreground: the kernel defuncts every + /// process's sockets when it runs out of network buffers, and no foreground transition + /// follows to trigger the check. So a failed request makes the page ask for the check + /// itself (`checkUploadServer()` in `bridge.js`), and the answer says whether the failed + /// upload may be sent again, which it may only if no server ever began it. + /// + /// The requests go through the editor's real script message handler, so this covers the + /// handler name and the shape of the reply the page relies on, as well as the check. + @MainActor + @Test( + "the page's check replaces a lost server, and clears only an upload that never got through", + .enabled(if: canBindUploadServer) + ) + func pageCheckReplacesALostServer() async throws { + let uploader = CountingUploader() + let session = ParkedURLSession() + defer { session.release() } + let configuration = makeConfiguration(siteURL: URL(string: "https://\(UUID().uuidString).example.invalid")!) + defer { + try? FileManager.default.removeItem(at: Paths.storageRoot(for: configuration)) + try? FileManager.default.removeItem(at: Paths.cacheRoot(for: configuration)) + } + let editor = EditorViewController( + configuration: configuration, + mediaUploader: uploader, + httpClient: EditorHTTPClient(urlSession: session, authHeader: configuration.authHeader) + ) + defer { editor.stopMediaHandling() } + + // Connects the page's messages to the editor. The dependency fetch this starts stays + // parked, so the editor never loads a page of its own over this one. + _ = editor.view + try await session.waitUntilStarted() + + await editor.startUploadServer() + let original = try #require(editor.uploadServer) + // What the injected user script sets at document start. + try await editor.webView.callAsyncJavaScript( + "window.GBKit = { nativeUploadPort: port, nativeUploadToken: token };", + arguments: ["port": Int(original.port), "token": original.token], + contentWorld: .page + ) + + // An upload gets through, then the socket goes. + let sent = try await sendNativeUpload(from: editor.webView, uploadID: "sent") + #expect(sent.status == 201, "the upload never worked, so the rest proves nothing: \(sent)") + original.stop() + try await waitUntilSilent(original) + + // The page asks about an upload the old server never saw. + let unsent = try await askToCheckUploadServer(from: editor.webView, uploadID: "never-sent") + let restarted = try #require(editor.uploadServer) + #expect(restarted !== original, "kept the server whose port had stopped answering") + #expect(unsent == UploadServerCheck(port: restarted.port, token: restarted.token, mayRetry: true)) + + // The page sends it again, and it lands on the new server. + let retried = try await sendNativeUpload(from: editor.webView, uploadID: "retry") + #expect(retried.port == Int(restarted.port), "the page still holds the old port") + #expect(retried.status == 201, "the upload sent again didn't land: \(retried)") + + // The upload that got through before the socket went is never cleared for a retry. + let again = try await askToCheckUploadServer(from: editor.webView, uploadID: "sent") + #expect(!again.mayRetry, "cleared an upload that had already reached the uploader") + #expect(editor.uploadServer === restarted, "replaced a server that was answering") + #expect(uploader.uploads == 2) + } + + /// Sends the page's `checkUploadServer` request the way `checkUploadServer()` in + /// `bridge.js` does, and decodes the editor's answer. + @MainActor + private func askToCheckUploadServer(from webView: WKWebView, uploadID: String) async throws -> UploadServerCheck { + let result = try await webView.callAsyncJavaScript( + "return await window.webkit.messageHandlers.checkUploadServer.postMessage({ uploadId });", + arguments: ["uploadId": uploadID], + contentWorld: .page + ) + let reply = try #require(result as? [String: Any], "the editor didn't answer") + return UploadServerCheck( + port: (reply["port"] as? Int).flatMap(UInt16.init(exactly:)), + token: reply["token"] as? String, + mayRetry: try #require(reply["retry"] as? Bool, "the answer has no retry flag") + ) + } + + /// Loads an empty `file://` page, the origin the editor itself runs from. + @MainActor + private func loadFilePage(in webView: WKWebView) async throws { + let directory = URL.temporaryDirectory.appending(path: UUID().uuidString, directoryHint: .isDirectory) + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + let page = directory.appending(path: "index.html") + try Data("upload recovery".utf8).write(to: page) + + webView.loadFileURL(page, allowingReadAccessTo: directory) + for _ in 0..<200 { + let loaded = try? await webView.evaluateJavaScript( + "location.protocol === 'file:' && document.readyState === 'complete'" + ) as? Bool + if loaded == true { return } + try await Task.sleep(for: .milliseconds(10)) + } + Issue.record("the file:// page never finished loading") + } + + /// Sends the request `nativeMediaUploadMiddleware` sends for `POST /wp/v2/media`, + /// with `uploadID` as its `Relay-Upload-ID` if there is one. + @MainActor + private func sendNativeUpload(from webView: WKWebView, uploadID: String? = nil) async throws -> NativeUploadOutcome { + let result = try await webView.callAsyncJavaScript( + """ + const { nativeUploadPort: port, nativeUploadToken: token } = window.GBKit; + const body = new FormData(); + body.append('file', new File(['not really a jpeg'], 'photo.jpg', { type: 'image/jpeg' })); + const headers = { 'Relay-Authorization': `Bearer ${token}` }; + if (uploadID) { + headers['Relay-Upload-ID'] = uploadID; + } + const started = performance.now(); + try { + const response = await fetch(`http://localhost:${port}/upload`, { + method: 'POST', + headers, + body, + }); + return { port, status: response.status, milliseconds: performance.now() - started }; + } catch (error) { + return { port, error: `${error.name}: ${error.message}`, milliseconds: performance.now() - started }; + } + """, + arguments: ["uploadID": uploadID ?? NSNull()], + contentWorld: .page + ) + let outcome = try #require(result as? [String: Any]) + return NativeUploadOutcome( + port: outcome["port"] as? Int, + status: outcome["status"] as? Int, + error: outcome["error"] as? String, + milliseconds: outcome["milliseconds"] as? Double ?? -1 + ) + } + + /// `cancel()` completes on the listener's own queue, so the socket can outlive `stop()` + /// by a moment. + private func waitUntilSilent(_ server: MediaUploadServer) async throws { + for _ in 0..<20 { + if await !server.isAnswering(timeout: .milliseconds(300)) { return } + try await Task.sleep(for: .milliseconds(100)) + } + Issue.record("the stopped server kept answering, so this test could not set up its own premise") + } + + @MainActor + @Test("no handler leaves the upload server down", .enabled(if: canBindUploadServer)) + func noHandlerLeavesServerDown() async { + let editor = EditorViewController(configuration: makeConfiguration()) + + await editor.startUploadServer() + + #expect(editor.uploadServer == nil, "started a server with nothing to route through it") + } + /// Polls instead of asserting outright, because a `UIViewController` can sit in an /// autorelease pool past the end of the scope that held it. Asserting synchronously /// passes in isolation and fails in a full suite, where other tests keep the main @@ -69,18 +418,18 @@ struct EditorViewControllerMediaTeardownTests: MakesTestFixtures { } } -/// The shape that cycles: owns the editor *and* is its delegate. Hosts reach for this +/// The shape that cycles: owns the editor *and* is its processor. Hosts reach for this /// because the coordinator driving the editor already has the site context. @MainActor -private final class EditorOwningDelegate: MediaUploadDelegate { - /// Implicitly unwrapped so `self` can be passed as the editor's delegate: every stored +private final class EditorOwningProcessor: MediaProcessor { + /// Implicitly unwrapped so `self` can be passed as the editor's processor: every stored /// property then has a value (nil) on entry to `init`, which is what makes `self` - /// available there. Taking the delegate at `init` doesn't prevent this shape — it just + /// available there. Taking the processor at `init` doesn't prevent this shape — it just /// moves where the host writes it. private(set) var editor: EditorViewController! init(configuration: EditorConfiguration) { - editor = EditorViewController(configuration: configuration, mediaUploadDelegate: self) + editor = EditorViewController(configuration: configuration, mediaProcessor: self) } nonisolated func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } @@ -90,7 +439,7 @@ private final class EditorOwningDelegate: MediaUploadDelegate { } } -private final class StandaloneDelegate: MediaUploadDelegate { +private final class StandaloneProcessor: MediaProcessor { func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { @@ -99,3 +448,79 @@ private final class StandaloneDelegate: MediaUploadDelegate { } #endif + +/// Supplied only to bring the upload server up; never invoked by these tests. +private struct InertUploader: MediaUploader { + func upload(_ upload: MediaUpload) async throws -> Data { Data() } +} + +/// Counts the uploads that reach it, and returns a finished attachment. +private final class CountingUploader: MediaUploader, @unchecked Sendable { + private let lock = NSLock() + private var count = 0 + + var uploads: Int { lock.withLock { count } } + + func upload(_ upload: MediaUpload) async throws -> Data { + lock.withLock { count += 1 } + return Data(#"{"id":7,"source_url":"https://example.com/photo.jpg","title":{"raw":"photo"}}"#.utf8) + } +} + +/// What a WebView upload came back with: an HTTP status, or the error `fetch` rejected with. +private struct NativeUploadOutcome: CustomStringConvertible { + let port: Int? + let status: Int? + let error: String? + let milliseconds: Double + + var description: String { + let result = status.map { "HTTP \($0)" } ?? error ?? "nothing" + return "\(result) from port \(port.map(String.init) ?? "none") after \(Int(milliseconds))ms" + } +} + +/// A port probe that answers only when the test tells it to, so the test decides when the +/// check waiting on it resumes. +@MainActor +private final class HeldProbe { + private var pending: CheckedContinuation? + + func ask(_ server: MediaUploadServer) async -> Bool { + await withCheckedContinuation { pending = $0 } + } + + func answer(_ isAnswering: Bool) { + pending?.resume(returning: isAnswering) + pending = nil + } + + func waitUntilAsked() async throws { + for _ in 0..<200 { + if pending != nil { return } + try await Task.sleep(for: .milliseconds(10)) + } + Issue.record("the check never asked its probe") + } +} + +/// Whether `HTTPServer` can bind here — it cannot in some sandboxes, and these tests +/// assert on a real listener. +private let canBindUploadServer: Bool = { + let result = UnsafeSendableBox(false) + let semaphore = DispatchSemaphore(value: 0) + Task { + if let server = try? await MediaUploadServer.start() { + server.stop() + result.value = true + } + semaphore.signal() + } + semaphore.wait() + return result.value +}() + +private final class UnsafeSendableBox: @unchecked Sendable { + var value: T + init(_ value: T) { self.value = value } +} diff --git a/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift index 140a0d0f1..a809295d6 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift @@ -17,10 +17,10 @@ struct MediaServerCredentialsTests { #expect(!MediaServerCredentials.areUsable(siteApiRoot: Self.siteRoot, authHeader: "")) } - // The two arms below are what a `URL` makes different from Android's `String`: - // `isEmpty()` has no direct equivalent, so "addressable" is spelled as scheme and - // host both being present. A URL missing either cannot reach the site, and every - // request built from it fails at the URLSession layer. + // "Addressable" is spelled as scheme and host both being present. A URL missing + // either cannot reach the site, and every request built from it fails at the + // URLSession layer. Android asserts the same two arms in its own + // `MediaServerCredentialsTest` — keep the cases in step. @Test("rejects a site root with no scheme") func rejectsSchemelessSiteRoot() { @@ -38,4 +38,51 @@ struct MediaServerCredentialsTests { func rejectsEmptySiteRoot() { #expect(!MediaServerCredentials.areUsable(siteApiRoot: URL(string: "/")!, authHeader: "Bearer t")) } + + // MARK: - requireCredentialsForUploader + + @Test("accepts usable credentials, uploader or not", arguments: [true, false]) + func acceptsUsableCredentials(hasUploader: Bool) { + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot: Self.siteRoot, authHeader: "Bearer t", hasUploader: hasUploader + ) + } + + @Test("ignores missing credentials when there is no uploader") + func ignoresMissingCredentialsWithoutUploader() { + // Nothing to deliver through, so nothing to process. This is not an error — the + // server just stays down (`areUsable` decides that) and uploads fall to the + // default WebView path, so a processor-only host must not trap here. + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot: Self.siteRoot, authHeader: "", hasUploader: false + ) + } + + // Exit tests run the body in a child process, so they can assert the trap itself. + // They are unavailable on iOS — including the simulator — and referencing them + // there is a *compile* error rather than a skip, so the whole block is gated to + // the host platform. This is exactly why the policy lives outside + // `EditorViewController`: that type is `#if canImport(UIKit)`, so on the one + // platform that can run these tests it does not exist. +#if os(macOS) + + @Test("traps for an uploader with no auth header") + func trapsForUploaderWithoutAuthHeader() async { + await #expect(processExitsWith: .failure) { + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot: URL(string: "https://example.com/wp-json/")!, authHeader: "", hasUploader: true + ) + } + } + + @Test("traps for an uploader with no site root") + func trapsForUploaderWithoutSiteRoot() async { + await #expect(processExitsWith: .failure) { + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot: URL(string: "/")!, authHeader: "Bearer t", hasUploader: true + ) + } + } + +#endif } diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 6e2c6af1d..5deaa9295 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -32,6 +32,79 @@ private final class UnsafeMutableSendablePointer: @unchecked Sendable { @Suite("MediaUploadServer Integration", .enabled(if: _canStartUploadServer)) struct MediaUploadServerTests { + /// The check that makes a dead listener detectable. + /// + /// Once the device can idle-sleep, iOS reclaims a suspended app's listening socket and + /// reports nothing — the listener still says `.ready` on the same port — so asking the + /// port is the only way to find out. `stop()` stands in for the system reclaiming it: both + /// leave the port refusing connections while the server object still reports one. + @Test("isAnswering tells a live server from one whose port is gone") + func isAnsweringTracksTheSocket() async throws { + let server = try await MediaUploadServer.start() + #expect(await server.isAnswering(), "a running server did not answer its own port") + + server.stop() + + // `cancel()` completes on the listener's own queue, so the socket can outlive the call + // by a moment. Poll rather than race it. + var stillAnswering = true + for _ in 0..<20 where stillAnswering { + stillAnswering = await server.isAnswering(timeout: .milliseconds(300)) + if stillAnswering { try await Task.sleep(for: .milliseconds(100)) } + } + #expect(!stillAnswering, "a stopped server kept answering, so a dead port would look healthy") + } + + /// A dead port has to be reported promptly, not when the probe gives up. + /// + /// The foreground check runs while the page still holds the old port, so for as long as + /// the probe takes to decide, uploads go to a port that refuses them. A refused connection + /// doesn't fail an `NWConnection` — it waits in `.waiting(ECONNREFUSED)` to retry when the + /// network path changes, which on loopback it never does — so a probe that only treats + /// `.failed` as dead gets its answer from the timeout. + @Test("isAnswering reports a refused port without waiting out its timeout") + func isAnsweringFailsFastOnARefusedPort() async throws { + let server = try await MediaUploadServer.start() + server.stop() + try await waitUntilRefused(port: server.port) + + let timeout = Duration.seconds(5) + var answered = true + let elapsed = await ContinuousClock().measure { + answered = await server.isAnswering(timeout: timeout) + } + + #expect(!answered, "a refused port was reported as answering") + #expect( + elapsed < .seconds(1), + "took \(elapsed) to report a refused port — it waited out the \(timeout) timeout" + ) + } + + /// No connection may wait for the main thread, and that includes the probe. + /// + /// The foreground check probes the port as the app comes back, which is when the main + /// thread is busiest: launching a WebView's content process can hold it for seconds. A + /// connection that waited for it would time the probe out, and the check would restart a + /// healthy server, cancelling any upload on it. + @Test("a connection is served while the main thread is busy") + func servesWhileTheMainThreadIsBusy() async throws { + let server = try await MediaUploadServer.start() + defer { server.stop() } + + let release = DispatchSemaphore(value: 0) + defer { release.signal() } + // Returns once the main thread is blocked, and leaves it blocked until `release`. + await withCheckedContinuation { (continuation: CheckedContinuation) in + DispatchQueue.main.async { + continuation.resume() + _ = release.wait(timeout: .now() + 5) + } + } + + #expect(await server.isAnswering(timeout: .seconds(1)), "the connection waited for the busy main thread") + } + @Test("starts and provides a port and token") func startAndStop() async throws { let server = try await MediaUploadServer.start() @@ -102,9 +175,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 = ProcessOnlyProcessor() + 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 +196,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) @@ -141,8 +214,8 @@ struct MediaUploadServerTests { // Exercised through the delete relay because every response `relayResponse` // handles — WordPress's own included — carries a `Content-Type`, so this is // the ordinary path rather than an edge case. - let uploader = ContentTypeDeleteUploader() - let server = try await MediaUploadServer.start(defaultUploader: uploader) + let uploader = ContentTypeDeleteClient() + let server = try await MediaUploadServer.start(internalClient: uploader) defer { server.stop() } let url = URL(string: "http://127.0.0.1:\(server.port)/media/42?force=true")! @@ -157,10 +230,11 @@ struct MediaUploadServerTests { #expect(httpResponse.value(forHTTPHeaderField: "Content-Type") == "text/plain") } - @Test("calls delegate and returns upload result") - func delegateProcessAndUpload() async throws { - let delegate = MockUploadDelegate() - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + @Test("processes with the processor, then delivers and relays verbatim") + func processesThenDelivers() async throws { + let processor = ResizingProcessor() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: internalClient) defer { server.stop() } let boundary = UUID().uuidString @@ -178,24 +252,62 @@ 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 processor only transforms; GutenbergKit performs the upload. + #expect(internalClient.uploadCalled) // The server relays WordPress's raw response body verbatim. let object = try JSONSerialization.jsonObject(with: data) let json = try #require(object as? [String: Any]) - #expect(json["id"] as? Int == 42) - #expect(json["source_url"] as? String == "https://example.com/photo.jpg") - #expect(json["media_type"] as? String == "image") + #expect(json["id"] as? Int == 99) + #expect(json["source_url"] as? String == "https://example.com/doc.pdf") + #expect(json["media_type"] as? String == "file") + } + + /// Pins the one capability dropping `: AnyObject` exists to deliver: a value type can + /// conform, and the server actually calls it. + /// + /// Every other conformer in the tree is a class, so without this nothing exercises the + /// boxed-existential path — copied into `Handler`, captured by the `@Sendable` + /// handler closure, read again at `processFile`. Re-imposing a class requirement, or + /// breaking that path, would otherwise compile and pass green and surface only in a + /// host's build. + /// + /// Asserts through the client's recorded metadata rather than state on the processor, + /// because a `struct` witnessing a non-mutating requirement cannot record anything — + /// which is the point. + @Test("a value-type processor is admitted, called, and its result delivered") + func valueTypeProcessorRuns() async throws { + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start( + processor: ValueTypeProcessor(), internalClient: internalClient + ) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody( + boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", + data: Data("movie".utf8) + ) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + // The transcoded metadata could only come from `processFile` having run. + #expect(internalClient.uploadCalled) + #expect(internalClient.lastUploadMimeType == "video/mp4") + #expect(internalClient.lastUploadFilename == "clip.mp4") } - @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("uses passthrough when processor does not modify file") + func processorPassthrough() async throws { + let processor = ProcessOnlyProcessor() + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -213,7 +325,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) @@ -224,11 +336,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 = DeclineByMetadataProcessor() + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -245,18 +357,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 @@ -271,18 +383,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 @@ -297,10 +409,10 @@ struct MediaUploadServerTests { _ = try await URLSession.shared.data(for: request) - // The server owns the file the delegate produced and must delete it once the + // The server owns the file the processor produced and must delete it once the // upload finishes — the defer in processAndUpload covers the success and throw // paths alike. A leaked processed file is a full-size temp per upload. - let processedURL = try #require(delegate.producedURL) + let processedURL = try #require(processor.producedURL) #expect(!FileManager.default.fileExists(atPath: processedURL.path(percentEncoded: false))) } @@ -385,85 +497,332 @@ struct MediaUploadServerTests { #expect(FileManager.default.fileExists(atPath: fresh.path(percentEncoded: false))) } - @Test("retains the delegate for the server's lifetime, and releases it after") - func retainsDelegateForServerLifetime() async throws { - weak var weakDelegate: MockUploadDelegate? + @Test("an uploader performs the upload and its result is relayed") + func uploaderPerformsUpload() async throws { + let uploader = RecordingUploader() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: internalClient) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8)) + let 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 (data, response) = try await URLSession.shared.data(for: request) + let httpResponse = try #require(response as? HTTPURLResponse) + + #expect(httpResponse.statusCode == 201) + #expect(String(decoding: data, as: UTF8.self).contains("\"id\":7")) + // GutenbergKit stays out of the network when a host uploader is set. + #expect(!internalClient.uploadCalled) + #expect(!internalClient.passthroughUploadCalled) + #expect(uploader.received?.filename == "photo.jpg") + #expect(uploader.received?.mimeType == "image/jpeg") + } + + // MARK: - Upload IDs + + /// The page may retry an upload it lost, but only one no server ever began. When it + /// gives up on an upload the old server is still holding, that server must drop it, + /// or WordPress would get the file twice: once from the old server, once from the retry. + @Test("an upload the editor gave up on never reaches the uploader") + func abandonedUploadIsDropped() async throws { + let uploader = RecordingUploader() + let ledger = UploadLedger() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient(), ledger: ledger) + defer { server.stop() } + #expect(ledger.abandon("given-up")) + + let (_, response) = try await URLSession.shared.data(for: uploadRequest(to: server, uploadID: "given-up")) + + #expect((response as? HTTPURLResponse)?.statusCode == 409) + #expect(uploader.received == nil, "an upload the editor was already sending again reached the uploader") + } + + /// The other half: once the server has begun an upload, the page can't clear it for a + /// retry, because WordPress may already have it. + @Test("an upload that reached the uploader can't be sent again") + func begunUploadCannotBeAbandoned() async throws { + let uploader = RecordingUploader() + let ledger = UploadLedger() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient(), ledger: ledger) + defer { server.stop() } + + let (_, response) = try await URLSession.shared.data(for: uploadRequest(to: server, uploadID: "sent")) + + #expect((response as? HTTPURLResponse)?.statusCode == 201) + #expect(!ledger.abandon("sent"), "cleared an upload that had already reached the uploader") + } + + @Test("an uploader receives the editor's form fields in order, and the query") + func uploaderReceivesFieldsAndQuery() async throws { + // Without `post` the attachment is created unattached, and repeated names (a + // `field[]` array) must survive as repeats rather than collapse into a dictionary. + let uploader = RecordingUploader() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + let boundary = UUID().uuidString + var body = Data() + for (name, value) in [("post", "42"), ("tags[]", "a"), ("tags[]", "b")] { + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"\(name)\"\r\n\r\n") + body.append("\(value)\r\n") + } + 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--\(boundary)--\r\n") + + let url = URL(string: "http://127.0.0.1:\(server.port)/upload?_embed=wp:featuredmedia")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + let received = try #require(uploader.received) + #expect(received.fields == [ + MediaUploadField(name: "post", value: "42"), + MediaUploadField(name: "tags[]", value: "a"), + MediaUploadField(name: "tags[]", value: "b"), + ]) + #expect(received.query == "?_embed=wp:featuredmedia") + } + + @Test("keeps a binary Blob part out of an uploader's fields") + func binaryPartExcludedFromFields() async throws { + // Pin rule 3: a Blob always has a filename, so it's dropped before the decode. + let uploader = RecordingUploader() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + let boundary = UUID().uuidString + var body = Data() + // Ordered as `uploadToServer` emits it: the file first, then additionalData. + 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--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"post\"\r\n\r\n") + body.append("42\r\n") + // A Blob-shaped part: it has a filename, and its bytes are not valid UTF-8. + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"blob\"; filename=\"blob\"\r\n") + body.append("Content-Type: application/octet-stream\r\n\r\n") + body.append(Data([0xED, 0xA0, 0x80])) + 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) + + // The Blob is dropped rather than decoded, and `file` is still the file. + let received = try #require(uploader.received) + #expect(received.filename == "photo.jpg") + #expect(received.fields == [MediaUploadField(name: "post", value: "42")]) + } + + @Test("round-trips a non-Latin field value exactly") + func nonLatinFieldRoundTrips() async throws { + // The other half: valid UTF-8 round-trips, so real captions and titles survive. + let uploader = RecordingUploader() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + let caption = "Grüße 🎉 日本語" + let boundary = UUID().uuidString + var body = Data() + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"caption\"\r\n\r\n") + body.append("\(caption)\r\n") + 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--\(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.received?.fields == [MediaUploadField(name: "caption", value: caption)]) + } + + @Test("a processor still processes the file an uploader delivers") + func processorRunsForUploader() async throws { + let processor = ProcessOnlyProcessor() + let uploader = RecordingUploader() + 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: "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 + + _ = try await URLSession.shared.data(for: request) + + // The processor still processes; only delivery moves to the uploader. + #expect(processor.processFileCalled) + #expect(uploader.received != nil) + } + + @Test("an uploader sees a file the processor's metadata gate would have declined") + func uploaderSeesDeclinedFile() async throws { + // The gate exists to skip a temp copy for a file the processor won't touch. An + // uploader takes over delivery for every file, so passing through here would + // silently bypass it. + let processor = DeclineByMetadataProcessor() + let uploader = RecordingUploader() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, uploader: uploader, internalClient: internalClient) + 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.received?.filename == "clip.mov") + #expect(!internalClient.passthroughUploadCalled) + // ...but a declined file must still not reach `processFile`: `handlesFile` + // returning false is the processor saying it won't touch a file like this. + #expect(!processor.processFileCalled) + } + + @Test("an uploader that throws surfaces as a failure, with no GutenbergKit retry") + func uploaderThrowSurfaces() async throws { + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: ThrowingUploader(), internalClient: internalClient) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + let (_, response) = try await URLSession.shared.data(for: request) + let httpResponse = try #require(response as? HTTPURLResponse) + + #expect(httpResponse.statusCode == 500) + // Recovery is the uploader's, not GutenbergKit's — it must not re-deliver. + #expect(!internalClient.uploadCalled) + #expect(!internalClient.passthroughUploadCalled) + } + + @Test("retains the processor for the server's lifetime, and releases it after") + func retainsProcessorForServerLifetime() async throws { + weak var weakProcessor: ProcessOnlyProcessor? do { - var delegate: MockUploadDelegate? = MockUploadDelegate() - weakDelegate = delegate - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + var processor: ProcessOnlyProcessor? = ProcessOnlyProcessor() + weakProcessor = processor + let server = try await MediaUploadServer.start(processor: processor) defer { server.stop() } - // The server owns the delegate while it runs: the host can assign one and drop + // The server owns the processor while it runs: the host can assign one and drop // its own reference, and every request still sees it. The host reference has to // go *before* the assert, or the local satisfies it and the server's ownership // is never what is under test — held weakly, this is already nil here. - delegate = nil - #expect(weakDelegate != nil) + processor = nil + #expect(weakProcessor != nil) } - // …and lets go when it stops, so the delegate isn't leaked for the process's + // …and lets go when it stops, so the processor isn't leaked for the process's // lifetime. Asserted outright rather than polled: `HTTPServer.stop()` clears the // listener's `newConnectionHandler`, which is what holds the handler closure and - // through it this delegate, so the release lands synchronously on this thread + // through it this processor, so the release lands synchronously on this thread // instead of trailing an asynchronous `NWListener` cancellation onto its queue. - #expect(weakDelegate == nil) + #expect(weakProcessor == nil) } - @Test("stopping frees a delegate that holds the server back") - func stopReleasesDelegateThatRetainsTheServer() async throws { + @Test("stopping frees a processor that holds the server back") + func stopReleasesProcessorThatRetainsTheServer() async throws { // The server-side half of the ownership story, and the one nothing else covers. // `EditorViewController.stopMediaHandling()` clears its own properties *and* stops // the server, because releasing only one leaves the loop routed through the other: - // `listener -> newConnectionHandler -> handler -> UploadContext -> delegate -> server`. + // `listener -> newConnectionHandler -> Handler -> processor -> server`. // - // Polled rather than asserted outright, unlike `retainsDelegateForServerLifetime`: + // Polled rather than asserted outright, unlike `retainsProcessorForServerLifetime`: // `releaseConnectionHandler()` opens the loop on the caller's thread, but it is not // the only thing that does. Cancelling an `NWListener` also releases the blocks it // captured, for a deployment target of iOS 16 or later (this package requires 17) — // rdar://89677097, documented in the macOS 13 release notes — and that release lands // on the listener's own queue. Confirmed by no-op'ing `releaseConnectionHandler()`: - // the delegate is still freed, a poll tick later. Before that OS change the blocks + // the processor is still freed, a poll tick later. Before that OS change the blocks // were held for the listener's lifetime, so a lowered deployment target hangs here // instead of quietly stranding listeners. - weak var weakDelegate: ServerRetainingDelegate? + weak var weakProcessor: ServerRetainingProcessor? var server: MediaUploadServer? do { - let delegate = ServerRetainingDelegate() - weakDelegate = delegate - let started = try await MediaUploadServer.start(uploadDelegate: delegate) - delegate.server = started // closes the loop: server -> handler -> delegate -> server + let processor = ServerRetainingProcessor() + weakProcessor = processor + let started = try await MediaUploadServer.start(processor: processor) + processor.server = started // closes the loop: server -> handler -> processor -> server server = started } - #expect(weakDelegate != nil, "the server should own the delegate while it runs") + #expect(weakProcessor != nil, "the server should own the processor while it runs") server?.stop() server = nil - for _ in 0..<100 where weakDelegate != nil { + for _ in 0..<100 where weakProcessor != nil { try await Task.sleep(for: .milliseconds(10)) } - #expect(weakDelegate == nil, "delegate leaked — stopping did not release the handler's references") + #expect(weakProcessor == nil, "processor leaked — stopping did not release the handler's references") } - @Test("still processes for a delegate the host has dropped its reference to") - func processesForHostReleasedDelegate() async throws { - // The delegate is read at the admission gate and again at processFile and - // uploadFile, separated by a synchronous disk copy and an unbounded processFile. + @Test("still processes for a processor the host has dropped its reference to") + func processesForHostReleasedProcessor() async throws { + // The processor is read at the admission gate and again at processFile, separated + // by a synchronous disk copy and an unbounded processFile. // Held weakly, a host that dropped its reference changed the answer between // those reads: a file admitted for processing was forwarded unprocessed. The // host dropping it before the request is the same condition, deterministically. - let mockUploader = MockDefaultUploader() - var delegate: TranscodingDelegate? = TranscodingDelegate() - weak let weakDelegate = delegate - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let mockUploader = MockInternalMediaClient() + var processor: ResizingProcessor? = ResizingProcessor() + weak let weakProcessor = processor + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } // Drop the host's only strong reference. Under the documented contract the - // server owns the delegate from here, so the upload must still be processed. - delegate = nil + // server owns the processor from here, so the upload must still be processed. + processor = nil let boundary = UUID().uuidString let body = buildMultipartBody(boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", data: Data("movie".utf8)) @@ -479,12 +838,55 @@ struct MediaUploadServerTests { // The server kept it alive, so the processed metadata reached the uploader. // Against a weak container this fails with the real symptom: the passthrough // branch runs and the original video/quicktime is forwarded unprocessed. - #expect(weakDelegate != nil) + #expect(weakProcessor != nil) #expect(mockUploader.uploadCalled) #expect(mockUploader.lastUploadMimeType == "video/mp4") #expect(!mockUploader.passthroughUploadCalled) } + /// Waits until the port refuses connections, checked with a plain BSD `connect()` so the + /// probe under test isn't also what decides the port is dead. + private func waitUntilRefused(port: UInt16) async throws { + for _ in 0..<50 { + if connectionIsRefused(port: port) { return } + try await Task.sleep(for: .milliseconds(20)) + } + Issue.record("port \(port) never started refusing connections after stop()") + } + + /// A blocking loopback `connect()` reports a refusal immediately, as `ECONNREFUSED`. + private func connectionIsRefused(port: UInt16) -> Bool { + let descriptor = socket(AF_INET, SOCK_STREAM, 0) + guard descriptor >= 0 else { return false } + defer { close(descriptor) } + + var address = sockaddr_in() + address.sin_len = UInt8(MemoryLayout.size) + address.sin_family = sa_family_t(AF_INET) + address.sin_port = port.bigEndian + address.sin_addr.s_addr = inet_addr("127.0.0.1") + let result = withUnsafePointer(to: &address) { + $0.withMemoryRebound(to: sockaddr.self, capacity: 1) { + connect(descriptor, $0, socklen_t(MemoryLayout.size)) + } + } + return result == -1 && errno == ECONNREFUSED + } + + /// An authenticated upload of a small JPEG, carrying `uploadID` as the page would. + private func uploadRequest(to server: MediaUploadServer, uploadID: String) -> URLRequest { + let boundary = UUID().uuidString + var request = URLRequest(url: URL(string: "http://127.0.0.1:\(server.port)/upload")!) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue(uploadID, forHTTPHeaderField: MediaUploadServer.uploadIDHeader) + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = buildMultipartBody( + boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8) + ) + return request + } + private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data) -> Data { var body = Data() body.append("--\(boundary)\r\n") @@ -498,7 +900,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") @@ -521,7 +923,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) @@ -538,7 +940,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", @@ -573,7 +975,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))] ) @@ -605,7 +1007,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)] ) @@ -621,7 +1023,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: [] ) @@ -645,7 +1047,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 ) @@ -672,7 +1074,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 ) @@ -685,17 +1087,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) @@ -722,7 +1124,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( @@ -745,7 +1147,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( @@ -763,7 +1165,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"] @@ -779,7 +1181,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"] @@ -802,7 +1204,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 @@ -873,51 +1275,33 @@ private func readAllFromStream(_ stream: InputStream) -> Data { // MARK: - Mocks -/// A delegate that transcodes, used to check the server holds it across the whole -/// request rather than re-reading a reference the host may have dropped. -private final class TranscodingDelegate: MediaUploadDelegate, @unchecked Sendable { - func handlesFile(ofType mimeType: String, named filename: String) -> Bool { - true - } - - func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { - let processed = FileManager.default.temporaryDirectory.appendingPathComponent("clip.mp4") - try? Data("transcoded".utf8).write(to: processed) - return .processed(processed, mimeType: "video/mp4", filename: "clip.mp4") - } -} - -private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable { +/// Records the ``MediaUpload`` it is handed, and returns a finished attachment. +private final class RecordingUploader: MediaUploader, @unchecked Sendable { private let lock = NSLock() - private var _processFileCalled = false - private var _uploadFileCalled = false - private var _lastMimeType: String? - private var _lastFilename: String? + private var _received: MediaUpload? - var processFileCalled: Bool { lock.withLock { _processFileCalled } } - var uploadFileCalled: Bool { lock.withLock { _uploadFileCalled } } - var lastMimeType: String? { lock.withLock { _lastMimeType } } - var lastFilename: String? { lock.withLock { _lastFilename } } + var received: MediaUpload? { lock.withLock { _received } } - func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { - lock.withLock { - _processFileCalled = true - _lastMimeType = mimeType - } - return .original + func upload(_ upload: MediaUpload) async throws -> Data { + lock.withLock { _received = upload } + // Shaped like a real attachment: the editor's `transformAttachment` reads + // `title.raw`, so an example without it would model a body that fails in the + // editor. + return Data(#"{"id":7,"source_url":"https://example.com/photo.jpg","media_type":"image","title":{"raw":"photo"},"caption":{"raw":""}}"#.utf8) } +} - func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { - lock.withLock { - _uploadFileCalled = true - _lastFilename = filename - } - let json = #"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"# - return MediaUploadResponse(statusCode: 201, body: Data(json.utf8)) +/// An uploader whose delivery fails terminally, as one would after exhausting its own +/// post-process recovery and force-deleting the orphan. +private final class ThrowingUploader: MediaUploader { + struct Failure: Error {} + + func upload(_ upload: MediaUpload) async throws -> Data { + throw Failure() } } -private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class ProcessOnlyProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -929,10 +1313,11 @@ private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendabl } } -/// A delegate that declines every file by metadata via `handlesFile`, so the -/// server must pass through without ever materializing the file or calling -/// `processFile`. -private final class DeclineByMetadataDelegate: MediaUploadDelegate, @unchecked Sendable { +/// A processor that declines every file by metadata via `handlesFile`. With no +/// uploader the server must pass through without ever materializing the file; with +/// one, delivery still happens but `processFile` must not be called. +/// `processFileCalled` pins both. +private final class DeclineByMetadataProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -946,12 +1331,23 @@ 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). +/// A value-type processor. `struct`, and `Sendable` without `@unchecked` — both are the +/// point: this is the shape ``MediaProcessor``'s documentation now recommends. +private struct ValueTypeProcessor: MediaProcessor { + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + let processed = url.deletingLastPathComponent() + .appending(component: "value-\(UUID().uuidString).mp4") + try Data("transcoded".utf8).write(to: processed) + return .processed(processed, mimeType: "video/mp4", filename: "clip.mp4") + } +} + +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 { @@ -962,7 +1358,7 @@ 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 @@ -1004,9 +1400,9 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab } } -/// A default uploader whose delete response carries its own `Content-Type`, so the +/// An internal media client whose delete response carries its own `Content-Type`, so the /// relay must override the JSON default rather than emit the header twice. -private final class ContentTypeDeleteUploader: DefaultMediaUploader, @unchecked Sendable { +private final class ContentTypeDeleteClient: InternalMediaClient, @unchecked Sendable { init() { super.init(httpClient: MockHTTPClient(), siteApiRoot: URL(string: "https://example.com/wp-json/")!) } @@ -1038,9 +1434,9 @@ private extension Data { } } -/// Holds the server that owns it, closing `server -> handler -> delegate -> server`. +/// Holds the server that owns it, closing `server -> handler -> processor -> server`. /// Only `stop()` — which drops the listener's captured blocks — opens it. -private final class ServerRetainingDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class ServerRetainingProcessor: MediaProcessor, @unchecked Sendable { var server: MediaUploadServer? func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } diff --git a/ios/Tests/GutenbergKitTests/Media/UploadLedgerTests.swift b/ios/Tests/GutenbergKitTests/Media/UploadLedgerTests.swift new file mode 100644 index 000000000..1994d6e98 --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Media/UploadLedgerTests.swift @@ -0,0 +1,62 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +/// `begin` and `abandon` must settle each upload once. If both could win, the old server +/// would send an upload the page is also sending again, and WordPress would get it twice. +@Suite("UploadLedger") +struct UploadLedgerTests { + + @Test("an upload the server began can't be cleared for a retry") + func begunCannotBeAbandoned() { + let ledger = UploadLedger() + #expect(ledger.begin("upload")) + #expect(!ledger.abandon("upload")) + } + + @Test("an upload the page gave up on can't be begun") + func abandonedCannotBegin() { + let ledger = UploadLedger() + #expect(ledger.abandon("upload")) + #expect(!ledger.begin("upload")) + } + + @Test("an upload without an ID always begins") + func uploadWithoutAnIDBegins() { + let ledger = UploadLedger() + #expect(ledger.begin(nil)) + #expect(ledger.begin(nil)) + } + + @Test("each upload is settled on its own") + func uploadsAreIndependent() { + let ledger = UploadLedger() + #expect(ledger.begin("sent")) + #expect(ledger.abandon("unsent")) + #expect(!ledger.abandon("sent")) + #expect(!ledger.begin("unsent")) + } + + /// The race this exists for: the server begins an upload on its connection's task while + /// the page's check abandons it on the main actor. Whichever runs first, exactly one wins. + @Test("exactly one of begin and abandon wins, whichever runs first") + func beginAndAbandonRace() async { + let ledger = UploadLedger() + let ids = (0..<500).map { "upload-\($0)" } + + let outcomes = await withTaskGroup(of: (String, Bool, Bool).self) { group in + for id in ids { + group.addTask { + async let began = Task.detached { ledger.begin(id) }.value + async let abandoned = Task.detached { ledger.abandon(id) }.value + return await (id, began, abandoned) + } + } + return await group.reduce(into: [(String, Bool, Bool)]()) { $0.append($1) } + } + + let bothOrNeither = outcomes.filter { $0.1 == $0.2 }.map(\.0) + #expect(bothOrNeither.isEmpty, "begin and abandon agreed on \(bothOrNeither.prefix(5))") + } +} diff --git a/src/utils/api-fetch-upload-middleware.test.js b/src/utils/api-fetch-upload-middleware.test.js index 288e70daa..c39064f0d 100644 --- a/src/utils/api-fetch-upload-middleware.test.js +++ b/src/utils/api-fetch-upload-middleware.test.js @@ -11,6 +11,8 @@ import { nativeMediaUploadMiddleware } from './api-fetch'; // Mock dependencies vi.mock( './bridge', () => ( { getGBKit: vi.fn( () => ( {} ) ), + canCheckUploadServer: vi.fn( () => false ), + checkUploadServer: vi.fn( () => Promise.resolve( null ) ), } ) ); vi.mock( './logger', () => ( { @@ -18,7 +20,7 @@ vi.mock( './logger', () => ( { error: vi.fn(), } ) ); -import { getGBKit } from './bridge'; +import { canCheckUploadServer, checkUploadServer, getGBKit } from './bridge'; function makeNext() { return vi.fn( () => Promise.resolve( { passthrough: true } ) ); @@ -43,6 +45,10 @@ function makeFile( name = 'photo.jpg', type = 'image/jpeg' ) { describe( 'nativeMediaUploadMiddleware', () => { beforeEach( () => { vi.restoreAllMocks(); + // A host that can't check its server (Android, a browser) unless a test + // says otherwise: no upload IDs, and `checkUploadServer` answers `null`. + canCheckUploadServer.mockReset().mockReturnValue( false ); + checkUploadServer.mockReset().mockResolvedValue( null ); global.fetch = vi.fn(); } ); @@ -220,6 +226,44 @@ describe( 'nativeMediaUploadMiddleware', () => { expect( options.body ).toBeInstanceOf( FormData ); } ); + it( 'reads the endpoint on every request, so a restarted server is picked up', async () => { + const next = makeNext(); + global.fetch = vi.fn( () => + Promise.resolve( { + ok: true, + json: () => Promise.resolve( { id: 42 } ), + } ) + ); + + getGBKit.mockReturnValue( { + nativeUploadPort: 12345, + nativeUploadToken: 'old-token', + } ); + await nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + next + ); + + // What the native side advertises after replacing a server whose + // socket iOS reclaimed. Nothing re-registers the middleware, so it + // only reaches the new server by reading the endpoint afresh. + getGBKit.mockReturnValue( { + nativeUploadPort: 23456, + nativeUploadToken: 'new-token', + } ); + await nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + next + ); + + expect( global.fetch ).toHaveBeenCalledTimes( 2 ); + const [ url, options ] = global.fetch.mock.calls[ 1 ]; + expect( url ).toBe( 'http://localhost:23456/upload' ); + expect( options.headers[ 'Relay-Authorization' ] ).toBe( + 'Bearer new-token' + ); + } ); + it( 'forwards the original body and query to the native server', async () => { getGBKit.mockReturnValue( { nativeUploadPort: 12345, @@ -374,6 +418,9 @@ describe( 'nativeMediaUploadMiddleware', () => { code: 'rest_cannot_create', message: expect.stringContaining( 'not allowed' ), } ); + + // The server answered, so there's nothing wrong with it to report. + expect( checkUploadServer ).not.toHaveBeenCalled(); } ); it( 'rejects with invalid_json when the error body is not JSON', async () => { @@ -507,11 +554,205 @@ describe( 'nativeMediaUploadMiddleware', () => { expect( typeof error.message ).toBe( 'string' ); expect( error.message.length ).toBeGreaterThan( 0 ); - // No silent fallback to a direct re-upload — retrying a non-idempotent - // POST could duplicate the attachment. + // No retry and no silent fallback to a direct re-upload — repeating a + // non-idempotent POST could duplicate the attachment. + expect( global.fetch ).toHaveBeenCalledTimes( 1 ); expect( next ).not.toHaveBeenCalled(); } ); + // MARK: - Retrying an upload that never reached WordPress + + describe( 'on a host that can check its upload server', () => { + const firstEndpoint = { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + }; + const restarted = { retry: true, port: 23456, token: 'new-token' }; + + beforeEach( () => { + getGBKit.mockReturnValue( firstEndpoint ); + canCheckUploadServer.mockReturnValue( true ); + } ); + + function uploadIdOf( call ) { + return global.fetch.mock.calls[ call ][ 1 ].headers[ + 'Relay-Upload-ID' + ]; + } + + function refuseThenAnswer( body = { id: 42 } ) { + global.fetch = vi + .fn() + .mockRejectedValueOnce( new TypeError( 'Failed to fetch' ) ) + .mockResolvedValueOnce( { + ok: true, + json: () => Promise.resolve( body ), + } ); + } + + it( 'sends each upload with an ID', async () => { + global.fetch = vi.fn( () => + Promise.resolve( { + ok: true, + json: () => Promise.resolve( { id: 42 } ), + } ) + ); + + await nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + makeNext() + ); + + expect( uploadIdOf( 0 ) ).toMatch( /^[0-9a-f]{32}$/ ); + } ); + + it( 'asks the host about the failed upload by its ID', async () => { + global.fetch = vi.fn( () => + Promise.reject( new TypeError( 'Failed to fetch' ) ) + ); + + await nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + makeNext() + ).catch( () => {} ); + + // Asking is also what gets a server lost in the foreground replaced, + // since no foreground transition follows to trigger the host's check. + expect( checkUploadServer ).toHaveBeenCalledOnce(); + expect( checkUploadServer ).toHaveBeenCalledWith( uploadIdOf( 0 ) ); + } ); + + it( 'sends the upload again, to the restarted server, when it never reached WordPress', async () => { + refuseThenAnswer( { id: 42 } ); + checkUploadServer.mockResolvedValue( restarted ); + const next = makeNext(); + + await expect( + nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + next + ) + ).resolves.toEqual( { id: 42 } ); + + expect( global.fetch ).toHaveBeenCalledTimes( 2 ); + const [ url, options ] = global.fetch.mock.calls[ 1 ]; + expect( url ).toBe( 'http://localhost:23456/upload' ); + expect( options.headers[ 'Relay-Authorization' ] ).toBe( + 'Bearer new-token' + ); + expect( options.body ).toBe( + global.fetch.mock.calls[ 0 ][ 1 ].body + ); + // A fresh ID: the host has settled the first one as abandoned, and + // would refuse to begin it. + expect( uploadIdOf( 1 ) ).toMatch( /^[0-9a-f]{32}$/ ); + expect( uploadIdOf( 1 ) ).not.toBe( uploadIdOf( 0 ) ); + expect( next ).not.toHaveBeenCalled(); + } ); + + it( 'keeps raw Response semantics for the second attempt under parse: false', async () => { + const answer = new Response( '{"id":42}', { status: 201 } ); + global.fetch = vi + .fn() + .mockRejectedValueOnce( new TypeError( 'Failed to fetch' ) ) + .mockResolvedValueOnce( answer ); + checkUploadServer.mockResolvedValue( restarted ); + + await expect( + nativeMediaUploadMiddleware( + { ...makePostMediaOptions( makeFile() ), parse: false }, + makeNext() + ) + ).resolves.toBe( answer ); + } ); + + it( 'surfaces the failure when the upload may have reached WordPress', async () => { + global.fetch = vi.fn( () => + Promise.reject( new TypeError( 'Failed to fetch' ) ) + ); + checkUploadServer.mockResolvedValue( { + ...restarted, + retry: false, + } ); + + const error = await nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + makeNext() + ).catch( ( e ) => e ); + + expect( error.code ).toBe( 'fetch_error' ); + expect( global.fetch ).toHaveBeenCalledTimes( 1 ); + } ); + + it( 'sends an upload at most twice', async () => { + global.fetch = vi.fn( () => + Promise.reject( new TypeError( 'Failed to fetch' ) ) + ); + checkUploadServer.mockResolvedValue( restarted ); + + const error = await nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + makeNext() + ).catch( ( e ) => e ); + + expect( error.code ).toBe( 'fetch_error' ); + expect( global.fetch ).toHaveBeenCalledTimes( 2 ); + // The second failure still gets the server checked, for later uploads. + expect( checkUploadServer ).toHaveBeenCalledTimes( 2 ); + expect( checkUploadServer ).toHaveBeenLastCalledWith( + uploadIdOf( 1 ) + ); + } ); + + it( 'does not send an upload again once it was cancelled', async () => { + const controller = new AbortController(); + global.fetch = vi.fn( () => + Promise.reject( new TypeError( 'Failed to fetch' ) ) + ); + // The user cancels while the host is checking. + checkUploadServer.mockImplementation( async () => { + controller.abort(); + return restarted; + } ); + + const error = await nativeMediaUploadMiddleware( + { + ...makePostMediaOptions( makeFile() ), + signal: controller.signal, + }, + makeNext() + ).catch( ( e ) => e ); + + // Read after the fact: the reason only exists once the abort happens. + expect( error ).toBe( controller.signal.reason ); + + expect( global.fetch ).toHaveBeenCalledTimes( 1 ); + } ); + } ); + + it( 'sends no upload ID, and nothing again, on a host that can’t check its server', async () => { + getGBKit.mockReturnValue( { + nativeUploadPort: 8080, + nativeUploadToken: 'token', + } ); + global.fetch = vi.fn( () => + Promise.reject( new TypeError( 'Failed to fetch' ) ) + ); + + const error = await nativeMediaUploadMiddleware( + makePostMediaOptions( makeFile() ), + makeNext() + ).catch( ( e ) => e ); + + // Android's server doesn't allow the header, so sending it would fail + // the CORS preflight for every upload. + expect( + global.fetch.mock.calls[ 0 ][ 1 ].headers[ 'Relay-Upload-ID' ] + ).toBeUndefined(); + expect( error.code ).toBe( 'fetch_error' ); + expect( global.fetch ).toHaveBeenCalledTimes( 1 ); + } ); + it( 'normalizes an offline transport failure to offline_error', async () => { getGBKit.mockReturnValue( { nativeUploadPort: 8080, @@ -534,6 +775,8 @@ describe( 'nativeMediaUploadMiddleware', () => { expect( error.code ).toBe( 'offline_error' ); expect( next ).not.toHaveBeenCalled(); + // Loopback doesn't need a network, so the server is still suspect. + expect( checkUploadServer ).toHaveBeenCalledOnce(); } finally { onLineSpy.mockRestore(); } @@ -568,6 +811,8 @@ describe( 'nativeMediaUploadMiddleware', () => { // An explicit cancellation must not be retried via the default path. expect( next ).not.toHaveBeenCalled(); + // Nor reported: a cancellation says nothing about the server. + expect( checkUploadServer ).not.toHaveBeenCalled(); } ); it( 'propagates a timeout cancellation (aborted signal, non-AbortError) instead of falling back', async () => { @@ -814,6 +1059,23 @@ describe( 'nativeMediaUploadMiddleware', () => { ).catch( ( error ) => error ); expect( thrown ).toEqual( { code: 'rest_cannot_delete' } ); + expect( checkUploadServer ).not.toHaveBeenCalled(); + } ); + + it( 'asks the host to check the upload server when a deletion can’t reach it', async () => { + global.fetch = vi.fn( () => + Promise.reject( new TypeError( 'Failed to fetch' ) ) + ); + + const thrown = await nativeMediaUploadMiddleware( + { method: 'DELETE', path: '/wp/v2/media/42?force=true' }, + makeNext() + ).catch( ( error ) => error ); + + expect( thrown.code ).toBe( 'fetch_error' ); + expect( global.fetch ).toHaveBeenCalledTimes( 1 ); + expect( checkUploadServer ).toHaveBeenCalledOnce(); + expect( checkUploadServer ).toHaveBeenCalledWith(); } ); } ); } ); diff --git a/src/utils/api-fetch.js b/src/utils/api-fetch.js index ba435a568..9a6775973 100644 --- a/src/utils/api-fetch.js +++ b/src/utils/api-fetch.js @@ -8,7 +8,12 @@ import { __ } from '@wordpress/i18n'; /** * Internal dependencies */ -import { getGBKit, POST_FALLBACKS } from './bridge'; +import { + canCheckUploadServer, + checkUploadServer, + getGBKit, + POST_FALLBACKS, +} from './bridge'; import { info, error as logError } from './logger'; /** @@ -279,80 +284,44 @@ function nativeMediaUpload( options, port, token ) { // body with only `file` would drop the post association and additionalData. const query = requestQuery( options.path ); + return sendNativeUpload( options, query, { port, token }, true ); +} + +/** + * Sends a media upload to the native upload server — and once more, if the + * request fails at the transport layer and the host confirms WordPress never + * received it. + * + * @param {Object} options The api-fetch options. + * @param {string} query The request's query, relayed to WordPress. + * @param {Object} endpoint The native upload server to send to. + * @param {number} endpoint.port Its port. + * @param {string} endpoint.token Its bearer token. + * @param {boolean} mayRetry Whether a failed attempt may be sent again. + * @return {Promise} The relayed upload. + */ +function sendNativeUpload( options, query, { port, token }, mayRetry ) { + // Each attempt gets its own ID, which the host records as it starts passing + // the upload on to WordPress. Only a host that can answer + // `checkUploadServer` gets one: a server that doesn't expect the header + // would reject the CORS preflight that carries it. + const uploadId = canCheckUploadServer() ? createUploadId() : undefined; + const headers = { 'Relay-Authorization': `Bearer ${ token }` }; + if ( uploadId ) { + headers[ 'Relay-Upload-ID' ] = uploadId; + } + // 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:${ port }/upload${ query }`, { method: 'POST', - headers: { - 'Relay-Authorization': `Bearer ${ token }`, - }, + headers, body: options.body, signal: options.signal, } ).then( - ( response ) => { - // `parse: false` asks for raw `Response` semantics. Core's media - // upload middleware runs above this one and makes exactly that - // request so it can read `x-wp-upload-attachment-id` off a failed - // upload and retry `post-process`. Honor it by resolving or - // rejecting with the `Response` itself, leaving the parsing (and - // the recovery decision) to that middleware — parsing here would - // hide the header and turn a recoverable upload into a permanent - // failure. - if ( options.parse === false ) { - if ( ! response.ok ) { - // 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; - } - - // The native server relays WordPress's response verbatim. On a - // non-2xx, mirror @wordpress/api-fetch: reject with the parsed WP - // error body ({ code, message, data }) so @wordpress/media-utils - // surfaces WordPress's real message. On success, return WordPress's - // attachment object unchanged so every consumer behaves exactly as - // it would for a non-native upload. - if ( ! response.ok ) { - return response - .json() - .catch( () => { - // An abort during the body read rejects json() too; surface - // the cancellation, not an "invalid response" error. - if ( options.signal?.aborted ) { - throw uploadAbortError( options.signal ); - } - return invalidUploadResponseError(); - } ) - .then( ( body ) => { - logError( 'Native upload failed', body ); - // Throw the parsed body verbatim, even if it isn't the usual - // WordPress `{ code, message, data }` shape. This is - // deliberate: it mirrors `@wordpress/api-fetch`'s - // `parseAndThrowError`, so a native-relayed error reaches - // consumers identically to a direct upload's. We intentionally - // don't reshape or second-guess a non-standard error body. - throw body; - } ); - } - // A 2xx with a non-JSON body (e.g. an HTML error page injected by an - // intermediary) rejects json(); normalize it the same way as the - // non-ok path rather than surfacing a raw SyntaxError. - return response.json().catch( () => { - // An abort during the body read rejects json(); surface the - // cancellation rather than an "invalid response" error notice. - if ( options.signal?.aborted ) { - throw uploadAbortError( options.signal ); - } - const error = invalidUploadResponseError(); - logError( 'Native upload returned an invalid response', error ); - throw error; - } ); - }, - ( connectionError ) => { + ( response ) => handleNativeUploadResponse( response, options ), + async ( connectionError ) => { // A caller-initiated cancellation must propagate as the cancellation, // never be retried. Detect it via `signal.aborted` — the cancellation // *state* — rather than `connectionError.name === 'AbortError'`: the @@ -368,43 +337,144 @@ function nativeMediaUpload( options, port, token ) { throw uploadAbortError( options.signal ); } // Otherwise the loopback upload server is unreachable at the transport - // layer. We deliberately do NOT fall back to a direct re-upload: - // reachability is gated proactively upstream — this middleware's guard - // skips the native path when no port is advertised, and the native side - // only advertises a port the WebView can actually reach (server running - // + cleartext-to-localhost permitted, cleared on stop). So reaching here - // means the server died out-of-band after a valid start; retrying a - // non-idempotent POST /wp/v2/media could duplicate the attachment if the - // native server had already relayed it to WordPress. + // layer, typically because iOS took its socket. We deliberately do NOT + // fall back to a direct re-upload or retry blindly: from here, a + // connection refused before the server saw anything looks the same as + // one cut off after it relayed the file to WordPress, and repeating a + // non-idempotent POST /wp/v2/media in the second case would duplicate + // the attachment. logError( 'Native upload failed at the transport layer', connectionError ); - // Normalize to the same `{ code, message }` shape - // `@wordpress/api-fetch`'s default handler produces for a failed fetch, - // so a native-upload transport failure surfaces to consumers (which key - // off `error.code` and show `error.message`) exactly like a direct - // upload's would — not as a raw, code-less TypeError with an - // untranslated message. Same codes and strings as api-fetch, so the - // existing translations apply. - if ( ! globalThis.navigator.onLine ) { - throw { - code: 'offline_error', - message: __( - 'Unable to connect. Please check your Internet connection.' - ), - }; + + // The host can tell the two apart. Asking also gets a lost server + // replaced, even with the app in the foreground where nothing else + // would notice, and the endpoint re-advertised for later uploads. + const check = await checkUploadServer( uploadId ); + if ( options.signal?.aborted ) { + throw uploadAbortError( options.signal ); } - throw { - code: 'fetch_error', - message: __( - 'Could not get a valid response from the server.' - ), - }; + if ( mayRetry && check?.retry && check.port ) { + info( 'Sending the upload again: it never reached WordPress' ); + return sendNativeUpload( options, query, check, false ); + } + + throw nativeUploadTransportError(); } ); } +/** + * A random ID for one attempt at an upload, sent as `Relay-Upload-ID`. + * + * @return {string} 32 hex characters. + */ +function createUploadId() { + const bytes = globalThis.crypto.getRandomValues( new Uint8Array( 16 ) ); + return Array.from( bytes, ( byte ) => + byte.toString( 16 ).padStart( 2, '0' ) + ).join( '' ); +} + +/** + * Turns the native server's response to an upload into what api-fetch's + * callers expect. + * + * @param {Response} response The native server's response. + * @param {Object} options The api-fetch options. + * @return {Promise} The attachment, or a rejection shaped like api-fetch's. + */ +function handleNativeUploadResponse( response, options ) { + // `parse: false` asks for raw `Response` semantics. Core's media + // upload middleware runs above this one and makes exactly that + // request so it can read `x-wp-upload-attachment-id` off a failed + // upload and retry `post-process`. Honor it by resolving or + // rejecting with the `Response` itself, leaving the parsing (and + // the recovery decision) to that middleware — parsing here would + // hide the header and turn a recoverable upload into a permanent + // failure. + if ( options.parse === false ) { + if ( ! response.ok ) { + // 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; + } + + // The native server relays WordPress's response verbatim. On a + // non-2xx, mirror @wordpress/api-fetch: reject with the parsed WP + // error body ({ code, message, data }) so @wordpress/media-utils + // surfaces WordPress's real message. On success, return WordPress's + // attachment object unchanged so every consumer behaves exactly as + // it would for a non-native upload. + if ( ! response.ok ) { + return response + .json() + .catch( () => { + // An abort during the body read rejects json() too; surface + // the cancellation, not an "invalid response" error. + if ( options.signal?.aborted ) { + throw uploadAbortError( options.signal ); + } + return invalidUploadResponseError(); + } ) + .then( ( body ) => { + logError( 'Native upload failed', body ); + // Throw the parsed body verbatim, even if it isn't the usual + // WordPress `{ code, message, data }` shape. This is + // deliberate: it mirrors `@wordpress/api-fetch`'s + // `parseAndThrowError`, so a native-relayed error reaches + // consumers identically to a direct upload's. We intentionally + // don't reshape or second-guess a non-standard error body. + throw body; + } ); + } + // A 2xx with a non-JSON body (e.g. an HTML error page injected by an + // intermediary) rejects json(); normalize it the same way as the + // non-ok path rather than surfacing a raw SyntaxError. + return response.json().catch( () => { + // An abort during the body read rejects json(); surface the + // cancellation rather than an "invalid response" error notice. + if ( options.signal?.aborted ) { + throw uploadAbortError( options.signal ); + } + const error = invalidUploadResponseError(); + logError( 'Native upload returned an invalid response', error ); + throw error; + } ); +} + +/** + * The error for an upload that couldn't reach the native server. + * + * Normalized to the same `{ code, message }` shape `@wordpress/api-fetch`'s + * default handler produces for a failed fetch, so a native-upload transport + * failure surfaces to consumers (which key off `error.code` and show + * `error.message`) exactly like a direct upload's would — not as a raw, + * code-less TypeError with an untranslated message. Same codes and strings as + * api-fetch, so the existing translations apply. + * + * @return {{code: string, message: string}} The error. + */ +function nativeUploadTransportError() { + if ( ! globalThis.navigator.onLine ) { + return { + code: 'offline_error', + message: __( + 'Unable to connect. Please check your Internet connection.' + ), + }; + } + return { + code: 'fetch_error', + message: __( 'Could not get a valid response from the server.' ), + }; +} + /** * Routes a media attachment deletion through the native upload server. * @@ -488,6 +558,10 @@ function nativeMediaDelete( options, port, token ) { 'Native media deletion failed at the transport layer', connectionError ); + // Same server as uploads, so ask for the same check (see + // `sendNativeUpload`). The deletion isn't retried: it's core's + // best-effort cleanup, and the check's answer is only about uploads. + checkUploadServer(); throw { code: 'fetch_error', message: __( diff --git a/src/utils/bridge.js b/src/utils/bridge.js index 45f84174d..5c761adf9 100644 --- a/src/utils/bridge.js +++ b/src/utils/bridge.js @@ -190,6 +190,47 @@ export function onModalDialogClosed( dialogType ) { dispatchToBridge( 'onModalDialogClosed', { dialogType } ); } +/** + * Whether the native host can check its local upload server on request. Only + * such a host is sent upload IDs, and only its answer can clear a failed upload + * for a retry. + * + * @return {boolean} Whether `checkUploadServer` reaches a host that answers. + */ +export function canCheckUploadServer() { + return Boolean( window.webkit?.messageHandlers?.checkUploadServer ); +} + +/** + * Asks the native host to check its local upload server after a request to it + * failed at the transport layer, and to replace the server if its socket is + * gone. + * + * The answer says where the server is now, and whether the upload may be sent + * again. The host allows that only if no server ever began passing the upload + * on to WordPress, and it makes sure none ever will, so a retry can't create a + * duplicate attachment. + * + * @param {string} [uploadId] The ID sent with the failed upload, if any. + * + * @return {Promise} The + * host's answer, or `null` if it can't check its server (Android, a browser). + */ +export async function checkUploadServer( uploadId ) { + if ( ! canCheckUploadServer() ) { + return null; + } + + try { + return await window.webkit.messageHandlers.checkUploadServer.postMessage( + { uploadId } + ); + } catch ( err ) { + error( 'Failed to check the native upload server', err ); + return null; + } +} + /** * Notifies the native host about a network request and its response. * diff --git a/src/utils/bridge.test.js b/src/utils/bridge.test.js index 62178fb1a..dcf6815c1 100644 --- a/src/utils/bridge.test.js +++ b/src/utils/bridge.test.js @@ -6,7 +6,14 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; /** * Internal dependencies */ -import { requestLatestContent, getPost, showBlockInserter } from './bridge'; +import { + requestLatestContent, + getPost, + showBlockInserter, + getGBKit, + canCheckUploadServer, + checkUploadServer, +} from './bridge'; vi.mock( './logger.js', () => ( { error: vi.fn(), @@ -545,3 +552,86 @@ describe( 'showBlockInserter', () => { expect( postMessage ).toHaveBeenCalledTimes( 1 ); } ); } ); + +describe( 'getGBKit', () => { + let originalGBKit; + + beforeEach( () => { + originalGBKit = window.GBKit; + } ); + + afterEach( () => { + window.GBKit = originalGBKit; + } ); + + it( 'returns the live window.GBKit, so a native update to it is seen by the next read', () => { + window.GBKit = { + nativeUploadPort: 12345, + nativeUploadToken: 'old-token', + }; + expect( getGBKit().nativeUploadPort ).toBe( 12345 ); + + // What `syncNativeUploadEndpoint()` evaluates in the page after the + // upload server is restarted on a new port. + Object.assign( window.GBKit, { + nativeUploadPort: 23456, + nativeUploadToken: 'new-token', + } ); + + expect( getGBKit() ).toMatchObject( { + nativeUploadPort: 23456, + nativeUploadToken: 'new-token', + } ); + } ); +} ); + +describe( 'checkUploadServer', () => { + let originalWindow; + + beforeEach( () => { + originalWindow = { + webkit: window.webkit, + editorDelegate: window.editorDelegate, + }; + delete window.webkit; + delete window.editorDelegate; + } ); + + afterEach( () => { + window.webkit = originalWindow.webkit; + window.editorDelegate = originalWindow.editorDelegate; + } ); + + function installHandler( postMessage ) { + window.webkit = { + messageHandlers: { checkUploadServer: { postMessage } }, + }; + } + + it( 'asks the iOS host about the failed upload and resolves with its answer', async () => { + const answer = { retry: true, port: 23456, token: 'new-token' }; + const postMessage = vi.fn( () => Promise.resolve( answer ) ); + installHandler( postMessage ); + + expect( canCheckUploadServer() ).toBe( true ); + await expect( checkUploadServer( 'abc123' ) ).resolves.toEqual( + answer + ); + // The handler name and body are the contract with `EditorViewController`. + expect( postMessage ).toHaveBeenCalledWith( { uploadId: 'abc123' } ); + } ); + + it( 'resolves with null on a host that can’t check its server', async () => { + // Android has an upload server but nothing that answers this. + window.editorDelegate = {}; + + expect( canCheckUploadServer() ).toBe( false ); + await expect( checkUploadServer( 'abc123' ) ).resolves.toBeNull(); + } ); + + it( 'resolves with null when the host fails to answer', async () => { + installHandler( vi.fn( () => Promise.reject( new Error( 'gone' ) ) ) ); + + await expect( checkUploadServer( 'abc123' ) ).resolves.toBeNull(); + } ); +} );