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 f445b704a..a40caba38 100644 --- a/docs/integration.md +++ b/docs/integration.md @@ -244,6 +244,131 @@ val configuration = EditorConfiguration.builder() .build() ``` +## Media Handling + +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, + mediaProcessor: ResizingProcessor(maxDimension: 2000) +) +``` + +### Don't conform the object that owns 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 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 -> mediaProcessor -> coordinator +final class PostEditorCoordinator: MediaProcessor { + var editor: EditorViewController! + init(blog: Blog, configuration: EditorConfiguration) { + editor = EditorViewController(configuration: configuration, mediaProcessor: self) + } +} +``` + +Use a leaf object instead. Nothing is lost: `processFile` is called off the main actor, so +it could not have touched your coordinator's state regardless — whatever it needs is +already separable: + +```swift +final class PostEditorCoordinator { + private let editor: EditorViewController + init(blog: Blog, configuration: EditorConfiguration) { + editor = EditorViewController( + configuration: configuration, + mediaProcessor: BlogMediaProcessor(siteID: blog.dotComID, maxDimension: 2000) + ) + } +} +``` + +If your design genuinely requires the retaining shape, call `stopMediaHandling()` when you +are finished with the editor. It is terminal — the editor cannot upload or delete media +afterwards — so call it when the editor is going away, not when it is merely covered or +backgrounded. + +### Android: permit cleartext to localhost + +**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 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. + ## Common Patterns ### Plugin Support 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 0f9b56ca4..8d358c48a 100644 --- a/ios/Demo-iOS/Sources/Views/EditorView.swift +++ b/ios/Demo-iOS/Sources/Views/EditorView.swift @@ -133,11 +133,12 @@ private struct _EditorView: UIViewControllerRepresentable { } func makeUIViewController(context: Context) -> EditorViewController { - let viewController = EditorViewController(configuration: configuration, dependencies: dependencies) + let viewController = EditorViewController( + configuration: configuration, + dependencies: dependencies, + mediaProcessor: enableNativeMediaUpload ? context.coordinator : nil + ) viewController.delegate = context.coordinator - if enableNativeMediaUpload { - viewController.mediaUploadDelegate = context.coordinator - } viewController.webView.isInspectable = true viewModel.perform = { [weak viewController] in @@ -189,7 +190,7 @@ private struct _EditorView: UIViewControllerRepresentable { } @MainActor - class Coordinator: NSObject, EditorViewControllerDelegate, MediaUploadDelegate { + class Coordinator: NSObject, EditorViewControllerDelegate, MediaProcessor { let viewModel: EditorViewModel init(viewModel: EditorViewModel) { @@ -295,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 da4c1fefe..bb0a792cd 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,52 +103,66 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// Used by `EditorViewController.warmup()` to reduce first-render latency. private let isWarmupMode: Bool - /// Set once the editor has begun loading and captured its configuration - /// (including ``mediaUploadDelegate``). After this, that delegate can no longer - /// take effect, so its setter traps if written. - private var hasStartedLoading = false - - /// Whether a non-nil ``mediaUploadDelegate`` was ever assigned. Lets the load - /// path tell "the delegate was released before load" (a retention mistake to - /// trap) apart from "no delegate was configured" (a valid opt-out). - private var mediaUploadDelegateWasAssigned = false - - /// Delegate for customizing media file processing and upload behavior. + /// Transforms media before upload — resize, transcode, strip EXIF. /// - /// Provide this **before the editor loads** — typically right after `init`, the - /// same way the rest of the editor configuration is supplied. It is captured - /// once, when the editor begins loading, and injected into the page's initial - /// configuration; setting it afterward has no effect, so the setter traps. + /// To perform the upload yourself, pass a ``mediaUploader`` instead. /// - /// - Important: This is a `weak` reference — you must hold a strong reference to - /// your delegate until the editor has loaded, or native uploads are silently - /// disabled. To surface that mistake, the editor traps at load time if a - /// delegate that was assigned here has already been deallocated. - public weak var mediaUploadDelegate: (any MediaUploadDelegate)? { - didSet { - // Record whether a delegate was provided so the load path can tell a - // premature deallocation apart from a deliberate opt-out (see - // `startUploadServer`). - mediaUploadDelegateWasAssigned = mediaUploadDelegate != nil - // Deliberate fail-fast, not a defensive check. The delegate is captured - // into the page's initial configuration when the editor begins loading, - // so a delegate assigned afterward would silently never take effect; - // trapping surfaces that misuse loudly instead of failing quietly. - // - // `hasStartedLoading` flips at the start of the async load (see - // `loadEditor`), which runs at or after `viewDidLoad` — so this only - // *widens* the safe window versus a synchronous flip. A host that - // follows the documented contract (set right after `init`, before - // presenting) can never race it; the trap fires only on a genuinely - // late assignment. Do not soften this to a no-op or a log — silently - // dropping the delegate is exactly the failure this is here to catch. - precondition( - !hasStartedLoading, - "mediaUploadDelegate must be set before the editor loads (e.g. right after init). " - + "It is captured into the editor configuration at load; setting it afterward has no effect." - ) - } - } + /// Supplied at `init`, with the rest of the editor's configuration, because that is + /// 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 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 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 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 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 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 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. + 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 @@ -158,7 +171,18 @@ 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? // MARK: - Private Properties (UI) @@ -200,13 +224,42 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro return HTMLPreviewManager(themeStyles: dependencies.editorSettings.themeStyles) }() + /// Creates an editor. + /// + /// - Parameters: + /// - configuration: Site, post, and editor settings to load with. + /// - 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. + /// - 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. 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, + 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 @@ -221,6 +274,8 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro ) self.bundleProvider = EditorAssetBundleProvider(httpClient: httpClient) self.mediaPicker = mediaPicker + self.mediaProcessor = mediaProcessor + self.mediaUploader = mediaUploader self.lockdownModeMonitor = LockdownModeMonitor() self.controller = GutenbergEditorController(configuration: configuration, lockdownModeMonitor: self.lockdownModeMonitor) @@ -273,6 +328,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) @@ -295,14 +360,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) @@ -311,8 +370,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() } } @@ -328,19 +390,172 @@ 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 ``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 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 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 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. + // + // The editor's own `isBeingDismissed`/`isMovingFromParent` read false — they are + // true on an ancestor, because the editor is a child view controller in every + // real host — but walking to that ancestor works, and WordPress-iOS already ships + // `isBeingDismissedDirectlyOrByAncestor()` for it. Pair it with an orphan check + // (`parent`, `presentingViewController`, `presentedViewController` and + // `viewIfLoaded?.window` all nil) at `viewDidDisappear`, and a probe across + // fourteen hosting shapes fires correctly on every dismissal and pop — including + // this editor's shape in WordPress-iOS — without a single false positive on being + // covered, tab-switched, re-parented by a `UIPageViewController`, or left behind + // by a cancelled interactive pop. Detaching and being covered are distinguishable. + // + // What is *not* observable is whether a detachment is permanent. A host may + // re-present or re-attach the same editor instance later, and at the moment of + // the callback that is indistinguishable from the last one. Because this call is + // terminal — the listener cannot restart and the page is told to stop using it — + // guessing wrong permanently disables media in an editor that survived, which is + // strictly worse than the leak it would have prevented. + // + // So this stays the host's call while the action is terminal. Make the endpoint + // recoverable (have the page request the port over the bridge instead of baking + // it in at document start) and the trade reverses. + uploadServer?.stop() + uploadServer = nil + mediaProcessor = nil + mediaUploader = nil + syncNativeUploadEndpoint() + } + + /// Restarts the upload server if its port stopped answering while the app was away. + /// + /// 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 and + /// `nativeMediaUploadMiddleware` doesn't retry. + /// + /// 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 while the app was in the background; 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() + } + } + + /// 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 + /// deliberately does *not* retry a failed native upload directly, on the stated + /// assumption that an advertised port is a reachable one ("cleared on stop"). Until + /// 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 change: the live page, the + /// `localStorage` copy `getGBKit()` falls back to, and the injected user script, + /// which would otherwise restore the old port verbatim at the next document start + /// — including the reload that recovers a terminated WebContent process. + 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( + """ + (() => { + 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: 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 update the native upload endpoint in the page: \(error)") + } + } + + // 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 { + webView.configuration.userContentController.addUserScript( + try buildEditorConfiguration(dependencies: dependencies) + ) + } catch { + // 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 changing the upload endpoint: \(error)") + } } deinit { - // Stop the upload server when the editor is permanently torn down. - // - // This deliberately does NOT happen in `viewDidDisappear`, which also - // fires when another view controller is merely pushed or presented over - // the editor. `HTTPServer.stop()` cancels the `NWListener`, which is - // terminal and has no restart path — stopping on disappear left uploads - // permanently broken once the user returned to the editor. + // 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 + // handler never reaches here — `stopMediaHandling()` is its way out. uploadServer?.stop() } @@ -384,10 +599,6 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// @MainActor private func loadEditor(dependencies: EditorDependencies) async throws { - // From here on the editor configuration — including `mediaUploadDelegate` — - // is captured, so the delegate setter traps if written after this point. - self.hasStartedLoading = true - self.displayActivityView() // Set asset bundle for the URL scheme handler to serve cached plugin/theme assets @@ -450,39 +661,57 @@ 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 { - // A delegate that was provided but is already nil here was deallocated before - // the editor finished loading — the host didn't hold a strong reference to it. - // That silently disables native uploads, so trap loudly instead. - precondition( - !(mediaUploadDelegateWasAssigned && mediaUploadDelegate == nil), - "mediaUploadDelegate was released before the editor loaded — hold a strong reference to it." - ) - - guard mediaUploadDelegate != nil else { + func startUploadServer() async { + // Nothing to route through the native server unless the host provided a + // 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. - guard !configuration.authHeader.isEmpty else { + // + // 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 + ) else { return } - let defaultUploader = DefaultMediaUploader( + let internalClient = InternalMediaClient( httpClient: httpClient.uploadClient(), siteApiRoot: configuration.siteApiRoot, siteApiNamespace: configuration.siteApiNamespace ) do { - self.uploadServer = try await MediaUploadServer.start( - uploadDelegate: mediaUploadDelegate, - defaultUploader: defaultUploader + let server = try await MediaUploadServer.start( + processor: mediaProcessor, + uploader: mediaUploader, + internalClient: internalClient ) + + // `stopMediaHandling()` can land while the bind is in flight: it is a + // 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 + } + self.uploadServer = server } catch { Logger.uploadServer.error("Failed to start upload server: \(error). Falling back to default upload behavior.") } 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 new file mode 100644 index 000000000..2b2514e0a --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift @@ -0,0 +1,73 @@ +import Foundation + +/// Whether the editor configuration can reach the configured site for media, and the +/// fail-fast that enforces it. +/// +/// Deliberately outside `EditorViewController`. That type is `#if canImport(UIKit)`, +/// so on the macOS host it does not exist and nothing in it can be tested — including +/// this policy, which is a *crash* policy and already diverged silently between iOS +/// and Android once. Living here, it is reachable from the host test suite, where +/// Swift Testing's exit tests (unavailable on iOS/simulator) can assert the trap +/// itself rather than only the predicate. +enum MediaServerCredentials { + /// Whether an ``InternalMediaClient`` built from this configuration could actually + /// reach the site. + /// + /// Both fields are required. The client delivers GutenbergKit'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: 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 3318aeac8..95b40ee4a 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 @@ -29,13 +30,16 @@ final class MediaUploadServer: Sendable { /// Creates and starts a new upload server. /// /// - Parameters: - /// - uploadDelegate: Optional delegate for customizing file processing and upload. - /// - defaultUploader: Fallback uploader used when no delegate provides `uploadFile`. + /// - processor: Optional processor 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. /// - 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, maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize ) async throws -> MediaUploadServer { // Sweep temp files orphaned by a prior crash, off the editor-startup @@ -45,7 +49,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(uploadDelegate: uploadDelegate, defaultUploader: defaultUploader) + let handler = Handler(processor: processor, uploader: uploader, internalClient: internalClient) // A generous ceiling for receiving the upload body. The body read is // primarily bounded by the per-read idle timeout (which reaps a stalled @@ -62,13 +66,74 @@ 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(processor: processor, uploader: uploader) + #endif + return uploadServer + } + +#if DEBUG + // MARK: - Leak Census (DEBUG) + + /// Counts live servers so a host that leaks editors finds out in its own debug build. + /// + /// Every live server is a bound loopback `NWListener`. There is one per editor and the + /// 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 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 — + /// and no UIKit callback distinguishes teardown from being covered or re-parented. + /// + /// Logged, never fatal. The threshold is a heuristic, and crashing a host's debug + /// build over a heuristic is a worse trade than the leak it reports. + private static let censusLock = NSLock() + // Guarded by `censusLock` on every access. + nonisolated(unsafe) private static var liveServerCount = 0 + + /// Live servers tolerated before the count reads as a leak. Two editors can briefly + /// overlap across a push or a modal transition; four is not a shape hosts produce. + private static let liveServerLeakThreshold = 4 + + private static func countServerStarted( + processor: (any MediaProcessor)?, + uploader: (any MediaUploader)? + ) { + let count = censusLock.withLock { + liveServerCount += 1 + return liveServerCount + } + + guard count >= liveServerLeakThreshold else { return } + + // 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 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 handler a leaf object that doesn't reference the editor. + """ ) + } - return MediaUploadServer(server: server, cleanupTask: cleanupTask) + deinit { + Self.censusLock.withLock { Self.liveServerCount -= 1 } } +#endif private init(server: HTTPServer, cleanupTask: Task) { self.server = server @@ -82,251 +147,425 @@ final class MediaUploadServer: Sendable { server.stop() } - // MARK: - Request Handling - - private static func handleRequest(_ request: HTTPServer.Request, context: UploadContext) 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() + // MARK: - Liveness - if method == "POST", parsed.path == "/upload" { - return await handleUpload(request, context: context) + /// 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() } - - if method == "DELETE", let attachmentId = attachmentId(fromPath: parsed.path) { - return await handleDelete(attachmentId, query: parsed.query, context: context) + defer { + deadline.cancel() + connection.cancel() } + return await Self.ask(connection) + } - return errorResponse(status: 404, message: "Not found") + 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) + } } - 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") + /// Lets exactly one of several callbacks resume a continuation. + 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: - Request Handling + + /// 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? + + 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) + } - // 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") + if method == "DELETE", let attachmentId = Self.attachmentId(fromPath: parsed.path) { + return await handleDelete(attachmentId, query: parsed.query) + } + + 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") + } - let filename = filePart.filename ?? "upload" - let mimeType = filePart.contentType + // 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") + } - // 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 { + // 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) + } + } + + // 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 { - 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 } } - // 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 } } @@ -347,7 +586,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 { @@ -356,6 +596,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 @@ -443,41 +703,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 re-read on each request. +/// GutenbergKit's own client for the configured site, built from the site credentials +/// in `EditorConfiguration`. /// -/// The delegate is held **weakly**. `EditorViewController.mediaUploadDelegate` is -/// declared `weak` — the host owns the delegate's lifetime. Capturing it strongly -/// here would silently defeat that contract and, worse, risk a retain cycle -/// (`EditorViewController → uploadServer → HTTPServer → handler → UploadContext → -/// delegate → EditorViewController`) that would keep the view controller — and -/// therefore the server — alive forever, so `deinit` would never stop it. -/// -/// `@unchecked Sendable`: `uploadDelegate` is assigned once at init and only read -/// afterwards; weak-reference reads are thread-safe at runtime. -private final class UploadContext: @unchecked Sendable { - weak var uploadDelegate: (any MediaUploadDelegate)? - let defaultUploader: DefaultMediaUploader? - - init(uploadDelegate: (any MediaUploadDelegate)?, defaultUploader: DefaultMediaUploader?) { - self.uploadDelegate = uploadDelegate - self.defaultUploader = defaultUploader - } -} - -// MARK: - Default Media Uploader - -/// Uploads files to the WordPress REST API using site credentials from EditorConfiguration. -class DefaultMediaUploader: @unchecked Sendable { +/// 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? @@ -528,7 +770,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)) @@ -627,11 +869,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/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 ac05fbb01..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 — @@ -305,15 +392,37 @@ public final class HTTPServer: Sendable { /// are currently executing will receive a `CancellationError`. public func stop() { listener.cancel() + releaseConnectionHandler() connectionTasks.cancelAll() Logger.httpServer.info("HTTP server stopped") } deinit { listener.cancel() + releaseConnectionHandler() connectionTasks.cancelAll() } + /// Drops the connection handler so teardown releases what it captured *here*, + /// on the caller's thread. + /// + /// `newConnectionHandler` retains the request handler, and through it whatever + /// the caller's closure captured. `cancel()` alone does not drop the block: + /// Network.framework holds the listener until cancellation completes on its own + /// queue, so the final release — and therefore the captured object's `deinit` — + /// lands there rather than wherever `stop()` was called. + /// + /// That covers an idle server. A request still in flight holds its own copy of what + /// the handler captured until that task unwinds, so a server stopped mid-request + /// releases last on the task's executor no matter what this does. + /// + /// Clearing it after `cancel()` rather than before is deliberate: the listener is + /// already torn down, so there is no window in which it is live but has no handler + /// to hand a connection to. + private func releaseConnectionHandler() { + listener.newConnectionHandler = nil + } + /// The library's default response for a parse error: the mapped status code /// with a plain-text body echoing the RFC reason phrase (e.g. 413 "Content Too /// Large"). This is what fatal errors always use, what a recoverable error uses @@ -350,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..a672b38d1 --- /dev/null +++ b/ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift @@ -0,0 +1,161 @@ +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. +private 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 + } +} + +private enum ParkedURLSessionTimeout: Error { + case requestNeverStarted +} + +#endif diff --git a/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift b/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift new file mode 100644 index 000000000..143ea6b37 --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift @@ -0,0 +1,434 @@ +import Foundation +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 `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 +/// call: not because UIKit can't report a teardown, but because it can't report whether +/// one is permanent. A host may re-present or re-attach the same editor, and the call is +/// terminal, so guessing wrong disables media in an editor that survived. +@Suite("EditorViewController media teardown") +struct EditorViewControllerMediaTeardownTests: 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("stopMediaHandling frees the editor and the host processor that owns it") + func stopMediaHandlingBreaksTheOwnershipCycle() async { + weak var weakEditor: EditorViewController? + weak var weakHost: EditorOwningProcessor? + + do { + let host = EditorOwningProcessor(configuration: makeConfiguration()) + weakEditor = host.editor + weakHost = host + host.editor.stopMediaHandling() + } + + await waitForRelease { weakHost == nil && weakEditor == nil } + + #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 standaloneProcessorIsFreed() async { + weak var weakEditor: EditorViewController? + + do { + let editor = EditorViewController( + configuration: makeConfiguration(), + mediaProcessor: StandaloneProcessor() + ) + weakEditor = editor + } + + await waitForRelease { weakEditor == nil } + + #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) + } + + /// 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`. + @MainActor + private func sendNativeUpload(from webView: WKWebView) 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 started = performance.now(); + try { + const response = await fetch(`http://localhost:${port}/upload`, { + method: 'POST', + headers: { 'Relay-Authorization': `Bearer ${token}` }, + body, + }); + return { port, status: response.status, milliseconds: performance.now() - started }; + } catch (error) { + return { port, error: `${error.name}: ${error.message}`, milliseconds: performance.now() - started }; + } + """, + 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 + /// actor busy and the pool drains later. A real leak still fails this, a second later. + @MainActor + private func waitForRelease(_ isReleased: () -> Bool) async { + for _ in 0..<100 where !isReleased() { + try? await Task.sleep(for: .milliseconds(10)) + } + } +} + +/// 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 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 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, mediaProcessor: self) + } + + nonisolated func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } + + nonisolated func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + .original + } +} + +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 { + .original + } +} + +#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 new file mode 100644 index 000000000..a809295d6 --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift @@ -0,0 +1,88 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +@Suite("MediaServerCredentials") +struct MediaServerCredentialsTests { + private static let siteRoot = URL(string: "https://example.com/wp-json/")! + + @Test("accepts an absolute site root with an auth header") + func acceptsUsableCredentials() { + #expect(MediaServerCredentials.areUsable(siteApiRoot: Self.siteRoot, authHeader: "Bearer t")) + } + + @Test("rejects an empty auth header") + func rejectsEmptyAuthHeader() { + #expect(!MediaServerCredentials.areUsable(siteApiRoot: Self.siteRoot, authHeader: "")) + } + + // "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() { + let relative = URL(string: "example.com/wp-json/")! + #expect(!MediaServerCredentials.areUsable(siteApiRoot: relative, authHeader: "Bearer t")) + } + + @Test("rejects a site root with no host") + func rejectsHostlessSiteRoot() { + let fileURL = URL(fileURLWithPath: "/tmp/wp-json") + #expect(!MediaServerCredentials.areUsable(siteApiRoot: fileURL, authHeader: "Bearer t")) + } + + @Test("rejects an empty site root, the default when a host configures none") + 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 60040e670..719db9c56 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,22 +497,346 @@ struct MediaUploadServerTests { #expect(FileManager.default.fileExists(atPath: fresh.path(percentEncoded: false))) } - @Test("does not strongly retain the upload delegate (weak — preserves deinit teardown)") - func doesNotStronglyRetainDelegate() async throws { - weak var weakDelegate: MockUploadDelegate? - let server: MediaUploadServer + @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") + } + + @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 { - let delegate = MockUploadDelegate() - weakDelegate = delegate - 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 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. + processor = nil + #expect(weakProcessor != nil) } + + // …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 processor, so the release lands synchronously on this thread + // instead of trailing an asynchronous `NWListener` cancellation onto its queue. + #expect(weakProcessor == nil) + } + + @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 -> processor -> server`. + // + // 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 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 weakProcessor: ServerRetainingProcessor? + var server: MediaUploadServer? + + do { + 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(weakProcessor != nil, "the server should own the processor while it runs") + + server?.stop() + server = nil + + for _ in 0..<100 where weakProcessor != nil { + try await Task.sleep(for: .milliseconds(10)) + } + #expect(weakProcessor == nil, "processor leaked — stopping did not release the handler's references") + } + + @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 = MockInternalMediaClient() + var processor: ResizingProcessor? = ResizingProcessor() + weak let weakProcessor = processor + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } - // UploadContext holds the delegate weakly, so releasing the host's strong - // reference deallocates it. A strong reference here would reintroduce the - // EditorViewController → uploadServer → … → delegate → EditorViewController - // cycle, so deinit would never fire and the server would never stop. - #expect(weakDelegate == nil) + // Drop the host's only strong reference. Under the documented contract the + // 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)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + // The server 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(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 } private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data) -> Data { @@ -416,7 +852,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") @@ -439,7 +875,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) @@ -456,7 +892,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", @@ -491,7 +927,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))] ) @@ -523,7 +959,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)] ) @@ -539,7 +975,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: [] ) @@ -563,7 +999,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 ) @@ -590,7 +1026,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 ) @@ -603,17 +1039,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) @@ -640,7 +1076,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( @@ -663,7 +1099,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( @@ -681,7 +1117,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"] @@ -697,7 +1133,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"] @@ -720,7 +1156,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 @@ -791,37 +1227,33 @@ private func readAllFromStream(_ stream: InputStream) -> Data { // MARK: - Mocks -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 @@ -833,10 +1265,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 @@ -850,12 +1283,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 { @@ -866,7 +1310,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 @@ -908,9 +1352,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/")!) } @@ -941,3 +1385,15 @@ private extension Data { append(string.data(using: .utf8)!) } } + +/// 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 ServerRetainingProcessor: MediaProcessor, @unchecked Sendable { + var server: MediaUploadServer? + + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } + + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + .original + } +} diff --git a/src/utils/api-fetch-upload-middleware.test.js b/src/utils/api-fetch-upload-middleware.test.js index 288e70daa..5c34dc3fb 100644 --- a/src/utils/api-fetch-upload-middleware.test.js +++ b/src/utils/api-fetch-upload-middleware.test.js @@ -220,6 +220,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, @@ -507,8 +545,9 @@ 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(); } ); diff --git a/src/utils/bridge.test.js b/src/utils/bridge.test.js index 62178fb1a..0f55110d1 100644 --- a/src/utils/bridge.test.js +++ b/src/utils/bridge.test.js @@ -6,7 +6,12 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; /** * Internal dependencies */ -import { requestLatestContent, getPost, showBlockInserter } from './bridge'; +import { + requestLatestContent, + getPost, + showBlockInserter, + getGBKit, +} from './bridge'; vi.mock( './logger.js', () => ( { error: vi.fn(), @@ -545,3 +550,35 @@ 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', + } ); + } ); +} );