diff --git a/Makefile b/Makefile index 283f2d6a3..75fa08f9c 100644 --- a/Makefile +++ b/Makefile @@ -219,7 +219,7 @@ wp-env-android-reset: ## Remove the Android emulator URL remap and restart @RESET=1 $(MAKE) wp-env-start .PHONY: wp-env-media-failure -wp-env-media-failure: ## Report the media upload failure simulation mode (MODE=off|recover|always to set it) +wp-env-media-failure: ## Report the media upload failure simulation mode (set via MODE=off|recover|always) @MODE=$(MODE) bash bin/wp-env-media-failure.sh ################################################################################ diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt index 66cb94791..6bc3d2e6f 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -111,6 +111,20 @@ interface MediaUploadDelegate { * the editor sees a complete attachment object. */ suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null + + /** + * Delete a previously uploaded attachment. + * + * The editor deletes the attachment when an upload's server-side + * post-processing fails past recovery, so it does not leave an orphan + * behind. A delegate that uploaded the attachment itself via [uploadFile] + * owns an ID only it can resolve, so it must delete the attachment itself + * too — the default uploader would address the wrong site. + * + * Return the raw response (status code + body), which GutenbergKit relays + * to the editor unchanged, or null to use the default uploader. + */ + suspend fun deleteFile(attachmentId: String): MediaUploadResponse? = null } /** @@ -234,7 +248,7 @@ internal class MediaUploadServer( if (method == "DELETE") { attachmentIdFromPath(request.path)?.let { attachmentId -> - return handleDelete(attachmentId, request.query) + return handleMediaDelete(attachmentId, request.query) } } @@ -255,17 +269,22 @@ internal class MediaUploadServer( } /** - * Relays the editor's orphan cleanup to WordPress. + * Relays the editor's orphan cleanup. * * Core's media upload middleware deletes the attachment when every * `post-process` retry fails. A cross-origin editor cannot issue that * request directly — api-fetch tunnels `DELETE` as a `POST` carrying * `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the * browser blocks it at preflight. Relaying it here lets the cleanup run. + * + * Offers the deletion to the delegate first, as [handleUpload] does, so a + * host that uploaded the attachment itself deletes it from the same place. */ - private suspend fun handleDelete(attachmentId: String, query: String): HttpResponse { - val uploader = defaultUploader ?: return errorResponse(500, "No uploader configured") + private suspend fun handleMediaDelete(attachmentId: String, query: String): HttpResponse { return try { + uploadDelegate?.deleteFile(attachmentId)?.let { return relayResponse(it) } + + val uploader = defaultUploader ?: return errorResponse(500, "No uploader configured") relayResponse(uploader.deleteMedia(attachmentId, query)) } catch (e: IOException) { Log.e(TAG, "Media deletion failed", e) diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt index 1d1f5ed6a..f4da7f671 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -140,6 +140,27 @@ class MediaUploadServerTest { assertTrue(response.statusLine.contains("404")) } + @Test + fun `routes a deletion to the delegate when it handles one`() { + // A host that uploaded the attachment itself owns an ID only it can + // resolve, so the default uploader must not be asked to delete it. No + // default uploader is configured, so a 200 here can only come from the + // delegate — the fallback path would fail with "no uploader". + val delegate = DeletingDelegate() + server.stop() + server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + + val response = sendRawRequest( + method = "DELETE", + path = "/media/42?force=true", + headers = mapOf("Relay-Authorization" to "Bearer ${server.token}"), + body = ByteArray(0) + ) + + assertTrue("Expected 200 but got: ${response.statusLine}", response.statusLine.contains("200")) + assertEquals("42", delegate.deletedAttachmentId) + } + @Test fun `routes upload with a query string and relays the query`() { val delegate = ProcessOnlyDelegate() @@ -734,6 +755,19 @@ class MediaUploadServerTest { } } + /** + * A delegate that handles deletions itself, as a host uploading to its own + * media service would. + */ + private class DeletingDelegate : MediaUploadDelegate { + @Volatile var deletedAttachmentId: String? = null + + override suspend fun deleteFile(attachmentId: String): MediaUploadResponse? { + deletedAttachmentId = attachmentId + return MediaUploadResponse(200, """{"deleted":true}""".toByteArray()) + } + } + private class ProcessOnlyDelegate : MediaUploadDelegate { @Volatile var processFileCalled = false diff --git a/docs/code/local-wordpress.md b/docs/code/local-wordpress.md index 051568c07..a8fac7eea 100644 --- a/docs/code/local-wordpress.md +++ b/docs/code/local-wordpress.md @@ -129,12 +129,6 @@ The mode is stored server-side, so it persists across uploads and retries until Then upload an image from a demo app and watch the network requests. In `recover` mode the upload 500s and the following `post-process` call succeeds, leaving a complete attachment; in `always` mode you should see five `post-process` attempts followed by a `DELETE`. -The mode is stored as an option rather than per-request state, because the upload and each retry are separate requests — and an upload routed through the native upload server is relayed by URLSession/OkHttp, which carries no browser cookie. - -**Note:** the plugin sets the 500 status itself. A real PHP fatal under FPM surfaces as a 500, but the Playground runtime wp-env uses returns 200, which the editor's `status >= 500` check would ignore. - -**Note:** a simulated fatal aborts the request before WordPress adds CORS headers, so a cross-origin editor reports these responses as CORS errors with provisional request headers rather than as a readable 500. That is expected for the upload and `post-process` responses — the editor only needs their status and the attachment ID header. It is why the plugin never fails a `DELETE`, including the `POST` + `X-Http-Method-Override: DELETE` form api-fetch actually sends: failing the orphan cleanup would make a correctly working retry look broken. - **Only the native upload server path recovers locally.** Reading `X-WP-Upload-Attachment-ID` cross-origin requires the site to list it in `Access-Control-Expose-Headers`, and WordPress core's `rest_send_cors_headers()` does not. Uploads routed through the native upload server recover on both platforms, since that server exposes the header itself. A **direct** upload (native media upload disabled) never recovers on iOS, which loads the editor from `file://`. It does not recover against wp-env on Android either: `GutenbergView` derives the asset domain from the site's _host_, which drops the port, so the editor at `http://10.0.2.2` is cross-origin with the site at `http://10.0.2.2:8888`. Direct uploads are only same-origin — and therefore only recover — when the site runs on the scheme's default port, as production sites do. @@ -162,21 +156,6 @@ Another service is using port 8888. Stop the conflicting service or change the w } ``` -Under the Playground runtime the culprit is often a previous wp-env server that outlived `make wp-env-stop`, which then makes every subsequent start fail with `EADDRINUSE`: - -```bash -lsof -ti:8888 # confirm what holds the port -pkill -f "wp-playground.js" -``` - -### Credentials rejected with HTTP 401 - -The Playground runtime starts from a fresh database on each start, so an existing `.wp-env.credentials.json` no longer matches the site's application password. Regenerate it: - -```bash -make wp-env-start RESET=1 -``` - ### Resetting the environment To start fresh, destroy the environment and recreate it: diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index 73752166b..d74743f97 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -82,6 +82,19 @@ public protocol MediaUploadDelegate: AnyObject, Sendable { /// host that uploads to WordPress should return the exact response it /// received so the editor sees a complete attachment object. func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? + + /// Delete a previously uploaded attachment. + /// + /// The editor deletes the attachment when an upload's server-side + /// post-processing fails past recovery, so it does not leave an orphan + /// behind. A delegate that uploaded the attachment itself via + /// ``uploadFile(at:mimeType:filename:)`` owns an ID only it can resolve, so + /// it must delete the attachment itself too — the default uploader would + /// address the wrong site. + /// + /// Return the raw response (status code + body), which GutenbergKit relays + /// to the editor unchanged, or `nil` to use the default uploader. + func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? } /// Default implementations. @@ -97,4 +110,8 @@ extension MediaUploadDelegate { public func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { nil } + + public func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { + nil + } } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index a2159d762..13b7047e6 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -98,7 +98,7 @@ final class MediaUploadServer: Sendable { } if method == "DELETE", let attachmentId = attachmentId(fromPath: parsed.path) { - return await handleDelete(attachmentId, query: parsed.query, context: context) + return await handleMediaDelete(attachmentId, query: parsed.query, context: context) } return errorResponse(status: 404, message: "Not found") @@ -208,20 +208,28 @@ final class MediaUploadServer: Sendable { return id } - /// Relays the editor's orphan cleanup to WordPress. + /// Relays the editor's orphan cleanup. /// /// Core's media upload middleware deletes the attachment when every /// `post-process` retry fails. A cross-origin editor cannot issue that /// request directly — api-fetch tunnels `DELETE` as a `POST` carrying /// `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the /// browser blocks it at preflight. Relaying it here lets the cleanup run. - private static func handleDelete( + /// + /// Offers the deletion to the delegate first, as ``handleUpload(_:context:)`` + /// does, so a host that uploaded the attachment itself deletes it from the + /// same place. + private static func handleMediaDelete( _ attachmentId: String, query: String, context: UploadContext ) async -> HTTPResponse { - guard let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) - } do { + if let response = try await context.uploadDelegate?.deleteFile(attachmentId: attachmentId) { + return relayResponse(response) + } + + guard let defaultUploader = context.defaultUploader else { + return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + } let response = try await defaultUploader.deleteMedia(attachmentId: attachmentId, query: query) return relayResponse(response) } catch { diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index fa87bbc4f..598067463 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -131,6 +131,28 @@ struct MediaUploadServerTests { #expect(mockUploader.lastQuery == "?_embed=wp:featuredmedia") } + @Test("routes a deletion to the delegate when it handles one") + func delegateHandlesDeletion() async throws { + // A host that uploaded the attachment itself owns an ID only it can + // resolve, so the default uploader must not be asked to delete it. + // No default uploader is configured, so a 200 here can only come from the + // delegate — the fallback path would fail with "no uploader". + let delegate = DeletingDelegate() + let server = try await MediaUploadServer.start(uploadDelegate: delegate) + defer { server.stop() } + + let url = URL(string: "http://127.0.0.1:\(server.port)/media/42?force=true")! + var request = URLRequest(url: url) + request.httpMethod = "DELETE" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + + let (_, response) = try await URLSession.shared.data(for: request) + let httpResponse = try #require(response as? HTTPURLResponse) + + #expect(httpResponse.statusCode == 200) + #expect(delegate.deletedAttachmentId == "42") + } + @Test("calls delegate and returns upload result") func delegateProcessAndUpload() async throws { let delegate = MockUploadDelegate() @@ -795,6 +817,20 @@ private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable } } +/// A delegate that handles deletions itself, as a host uploading to its own +/// media service would. +private final class DeletingDelegate: MediaUploadDelegate, @unchecked Sendable { + private let lock = NSLock() + private var _deletedAttachmentId: String? + + var deletedAttachmentId: String? { lock.withLock { _deletedAttachmentId } } + + func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { + lock.withLock { _deletedAttachmentId = attachmentId } + return MediaUploadResponse(statusCode: 200, body: Data(#"{"deleted":true}"#.utf8)) + } +} + private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false diff --git a/src/utils/api-fetch.js b/src/utils/api-fetch.js index 2c0edce44..d5fee136d 100644 --- a/src/utils/api-fetch.js +++ b/src/utils/api-fetch.js @@ -94,10 +94,6 @@ function corsMiddleware( options, next ) { */ function apiPathModifierMiddleware( options, next ) { const { siteApiNamespace, namespaceExcludedPaths } = getGBKit(); - // Self-hosted sites configure no namespace, so there is nothing to insert. - // This has to gate the rewrite explicitly: the namespace match below cannot - // stand in for it, and an empty namespace would otherwise interpolate - // `undefined` into the path. const isEligiblePath = options.path && siteApiNamespace.length > 0 && @@ -113,10 +109,11 @@ function apiPathModifierMiddleware( options, next ) { ).test( options.path ) || /\/sites\/[^/]+\//.test( options.path ); if ( isEligiblePath && ! alreadyHasSiteNamespace ) { - // Insert the API namespace after the first two path segments. + // Insert the API namespace after the first two path segments, with a + // single trailing slash. options.path = options.path.replace( /^(?\/?(?:[\w.-]+\/){2})/, - `$${ siteApiNamespace[ 0 ] }` + `$${ siteApiNamespace[ 0 ].replace( /\/+$/, '' ) }/` ); } @@ -197,17 +194,12 @@ function filterEndpointsMiddleware( options, next ) { } /** - * Middleware that routes media uploads through the native host's local HTTP - * server for processing (e.g. image resizing) before uploading to WordPress. + * Middleware that routes media requests through the native host's local HTTP + * server: uploads for processing (e.g. image resizing) before they reach + * WordPress, and attachment deletions for the editor's orphan cleanup. * * Exported for testing only. * - * When `nativeUploadPort` is configured in GBKit, this middleware intercepts - * `POST /wp/v2/media` requests, forwards the file to the native server, and - * returns the response in WordPress REST API attachment format so the existing - * Gutenberg upload pipeline (blob previews, save locking, entity caching) - * works unchanged. - * * When the native server is not configured, requests pass through unmodified. * * Note: Ideally, media uploads would be handled via the `mediaUpload` editor @@ -227,27 +219,44 @@ function filterEndpointsMiddleware( options, next ) { export function nativeMediaUploadMiddleware( options, next ) { const { nativeUploadPort, nativeUploadToken } = getGBKit(); - if ( nativeUploadPort && nativeUploadToken ) { - const deletion = nativeMediaDelete( - options, - nativeUploadPort, - nativeUploadToken - ); - if ( deletion ) { - return deletion; - } + if ( ! nativeUploadPort || ! nativeUploadToken ) { + return next( options ); } + // Each helper returns `null` when the request is not its concern, so an + // unhandled request falls through to the default path. + return ( + nativeMediaDelete( options, nativeUploadPort, nativeUploadToken ) ?? + nativeMediaUpload( options, nativeUploadPort, nativeUploadToken ) ?? + next( options ) + ); +} + +/** + * Routes a media upload through the native upload server. + * + * Returns `null` when the request is not a media upload, so the caller falls + * through to its normal handling. + * + * Intercepts `POST /wp/v2/media`, forwards the file to the native server, and + * returns the response in WordPress REST API attachment format so the existing + * Gutenberg upload pipeline (blob previews, save locking, entity caching) works + * unchanged. + * + * @param {Object} options The api-fetch options. + * @param {number} port The native upload server port. + * @param {string} token The native upload server bearer token. + * @return {?Promise} The relayed upload, or `null` if not applicable. + */ +function nativeMediaUpload( options, port, token ) { if ( - ! nativeUploadPort || - ! nativeUploadToken || ! options.method || options.method.toUpperCase() !== 'POST' || ! options.path || ! MEDIA_UPLOAD_PATH.test( options.path ) || ! ( options.body instanceof FormData ) ) { - return next( options ); + return null; } // Only intercept a genuine file upload. `FormData.get('file')` returns a @@ -257,11 +266,11 @@ export function nativeMediaUploadMiddleware( options, next ) { // through to the default path — and guarantees `file.name` below is safe. const file = options.body.get( 'file' ); if ( ! ( file instanceof File ) ) { - return next( options ); + return null; } info( - `Routing upload of ${ file.name } through native server on port ${ nativeUploadPort }` + `Routing upload of ${ file.name } through native server on port ${ port }` ); // Forward the original request body — the file plus every sibling field @@ -273,10 +282,10 @@ export function nativeMediaUploadMiddleware( options, next ) { // Use the two-argument form of `.then()` so the rejection handler catches // *only* a connection-level failure of the `fetch()` itself — not errors // thrown while handling a response (those must surface as real failures). - return fetch( `http://localhost:${ nativeUploadPort }/upload${ query }`, { + return fetch( `http://localhost:${ port }/upload${ query }`, { method: 'POST', headers: { - 'Relay-Authorization': `Bearer ${ nativeUploadToken }`, + 'Relay-Authorization': `Bearer ${ token }`, }, body: options.body, signal: options.signal, diff --git a/src/utils/api-fetch.test.js b/src/utils/api-fetch.test.js index 63cac028b..f4025333a 100644 --- a/src/utils/api-fetch.test.js +++ b/src/utils/api-fetch.test.js @@ -212,6 +212,33 @@ describe( 'api-fetch credentials handling', () => { expect( requestedUrl() ).toContain( '/wp/v2/sites/123/posts' ); } ); + it( 'inserts a namespace configured without a trailing slash', async () => { + // Both forms are supported; the native URL builders normalize them + // identically. Without normalizing here the namespace would run into + // the following segment: `/wp/v2/sites/123posts`. + bridge.getGBKit.mockReturnValue( { + siteApiRoot: 'https://example.com/wp-json/', + siteApiNamespace: [ 'sites/123' ], + namespaceExcludedPaths: [], + } ); + + await apiFetch( { path: '/wp/v2/posts' } ).catch( () => {} ); + + expect( requestedUrl() ).toContain( '/wp/v2/sites/123/posts' ); + } ); + + it( 'does not double the slash on a namespace that already ends with one', async () => { + bridge.getGBKit.mockReturnValue( { + siteApiRoot: 'https://example.com/wp-json/', + siteApiNamespace: [ 'sites/123/' ], + namespaceExcludedPaths: [], + } ); + + await apiFetch( { path: '/wp/v2/posts' } ).catch( () => {} ); + + expect( requestedUrl() ).not.toContain( 'sites/123//' ); + } ); + it( 'leaves the path alone when it already carries the namespace', async () => { bridge.getGBKit.mockReturnValue( { siteApiRoot: 'https://example.com/wp-json/',