diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt index abfc9c7d4..8d0885539 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/HttpServer.kt @@ -45,6 +45,23 @@ data class HttpRequest( * for building an appropriate error response. */ val serverError: org.wordpress.gutenberg.http.HTTPRequestParseError? = null ) { + /** + * The path portion of [target], without the query component + * (e.g., "/wp/v2/posts" for "/wp/v2/posts?per_page=10"). + * + * Use this for routing — matching against [target] fails as soon as a + * client appends a query string. + */ + val path: String + get() = target.substringBefore('?') + + /** + * The query component of [target], including the leading "?" + * (e.g., "?per_page=10"), or an empty string when there is no query. + */ + val query: String + get() = target.substringAfter('?', "").let { if (it.isEmpty()) "" else "?$it" } + /** * Returns the value of the first header matching the given name (case-insensitive). */ 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 b1d60c868..c289866a8 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -175,8 +175,10 @@ internal class MediaUploadServer( } // Route: only POST /upload is handled. (OPTIONS preflight is answered by - // the HTTP library under its permissive CORS policy.) - if (request.method.uppercase() != "POST" || request.target != "/upload") { + // the HTTP library under its permissive CORS policy.) Match on the path + // alone — the target carries a query string (e.g. `?_embed`) that the + // upload handler relays on to WordPress. + if (request.method.uppercase() != "POST" || request.path != "/upload") { return errorResponse(404, "Not found") } @@ -192,8 +194,7 @@ internal class MediaUploadServer( // The non-file parts (post, additionalData) and the original query // (e.g. ?_embed) must reach WordPress too — relay them alongside the file. val extraParts = parts.filter { it.filename == null } - val queryValue = request.target.substringAfter('?', "") - val query = if (queryValue.isEmpty()) "" else "?$queryValue" + val query = request.query val tempFile = writePartToTempFile(filePart) ?: return errorResponse(500, "Failed to save file") diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpRequestTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpRequestTest.kt new file mode 100644 index 000000000..6aae33ebd --- /dev/null +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/HttpRequestTest.kt @@ -0,0 +1,47 @@ +package org.wordpress.gutenberg + +import org.junit.Assert.assertEquals +import org.junit.Test + +class HttpRequestTest { + + private fun request(target: String) = HttpRequest( + method = "POST", + target = target, + headers = emptyMap() + ) + + @Test + fun `path is the whole target when there is no query`() { + assertEquals("/upload", request("/upload").path) + assertEquals("", request("/upload").query) + } + + @Test + fun `path and query split on the first question mark`() { + val parsed = request("/upload?_embed=wp:featuredmedia") + assertEquals("/upload", parsed.path) + assertEquals("?_embed=wp:featuredmedia", parsed.query) + } + + @Test + fun `a trailing question mark yields an empty query`() { + val parsed = request("/upload?") + assertEquals("/upload", parsed.path) + assertEquals("", parsed.query) + } + + @Test + fun `later question marks belong to the query`() { + val parsed = request("/search?q=a?b") + assertEquals("/search", parsed.path) + assertEquals("?q=a?b", parsed.query) + } + + @Test + fun `multiple query parameters are preserved`() { + val parsed = request("/wp/v2/posts?per_page=10&page=2") + assertEquals("/wp/v2/posts", parsed.path) + assertEquals("?per_page=10&page=2", parsed.query) + } +} diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt index 78276226b..7ce925f0f 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -101,6 +101,38 @@ class MediaUploadServerTest { assertTrue(response.statusLine.contains("404")) } + @Test + fun `routes upload with a query string and relays the query`() { + val delegate = ProcessOnlyDelegate() + val mockUploader = MockDefaultUploader() + server.stop() + server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + + // `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, + // so the middleware forwards that query on to the native server. Routing must + // match on the path alone, and the query must reach WordPress unchanged. + val boundary = "test-boundary-query" + val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) + + val response = 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 + ) + + assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) + // The delegate 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) + assertFalse(mockUploader.uploadCalled) + assertEquals("?_embed=wp:featuredmedia", mockUploader.lastQuery) + } + // MARK: - Upload with delegate @Test @@ -566,6 +598,7 @@ class MediaUploadServerTest { @Volatile var passthroughUploadCalled = false @Volatile var lastUploadMimeType: String? = null @Volatile var lastUploadFilename: String? = null + @Volatile var lastQuery: String? = null override suspend fun upload( file: File, mimeType: String, filename: String, @@ -574,6 +607,7 @@ class MediaUploadServerTest { uploadCalled = true lastUploadMimeType = mimeType lastUploadFilename = filename + lastQuery = query return mockResponse() } @@ -583,6 +617,7 @@ class MediaUploadServerTest { query: String ): MediaUploadResponse { passthroughUploadCalled = true + lastQuery = query return mockResponse() } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index d2396f274..53795d79f 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -79,8 +79,10 @@ final class MediaUploadServer: Sendable { } // Route: only POST /upload is handled. (OPTIONS preflight is answered by - // the HTTP library under its permissive CORS policy.) - guard parsed.method.uppercased() == "POST", parsed.target == "/upload" else { + // the HTTP library under its permissive CORS policy.) Match on the path + // alone — the target carries a query string (e.g. `?_embed`) that the + // upload handler relays on to WordPress. + guard parsed.method.uppercased() == "POST", parsed.path == "/upload" else { return errorResponse(status: 404, message: "Not found") } @@ -104,9 +106,7 @@ final class MediaUploadServer: Sendable { // 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.target.firstIndex(of: "?").map { - String(request.parsed.target[$0...]) - } ?? "" + let query = request.parsed.query // Write part body to a dedicated temp file for the delegate. // diff --git a/ios/Sources/GutenbergKitHTTP/ParsedHTTPRequest.swift b/ios/Sources/GutenbergKitHTTP/ParsedHTTPRequest.swift index d68774921..95222d1c0 100644 --- a/ios/Sources/GutenbergKitHTTP/ParsedHTTPRequest.swift +++ b/ios/Sources/GutenbergKitHTTP/ParsedHTTPRequest.swift @@ -32,6 +32,29 @@ extension ParsedHTTPRequest { } } + /// The path portion of ``target``, without the query component + /// (e.g., "/wp/v2/posts" for "/wp/v2/posts?per_page=10"). + /// + /// Use this for routing — matching against ``target`` fails as soon as a + /// client appends a query string. + public var path: String { + let target = target + guard let separator = target.firstIndex(of: "?") else { return target } + return String(target.prefix(upTo: separator)) + } + + /// The query component of ``target``, including the leading "?" + /// (e.g., "?per_page=10"), or an empty string when there is no query. + /// + /// A bare trailing "?" carries no parameters and yields an empty string, so + /// the value can be appended to an upstream URL unconditionally. + public var query: String { + let target = target + guard let separator = target.firstIndex(of: "?") else { return "" } + let value = String(target[target.index(after: separator)...]) + return value.isEmpty ? "" : "?\(value)" + } + /// The HTTP-version from the request line (e.g., "HTTP/1.1"), per RFC 9112 §2.3. public var httpVersion: String { switch self { diff --git a/ios/Tests/GutenbergKitHTTPTests/ParsedHTTPRequestTests.swift b/ios/Tests/GutenbergKitHTTPTests/ParsedHTTPRequestTests.swift index 2d37a9abe..82e17b2da 100644 --- a/ios/Tests/GutenbergKitHTTPTests/ParsedHTTPRequestTests.swift +++ b/ios/Tests/GutenbergKitHTTPTests/ParsedHTTPRequestTests.swift @@ -5,6 +5,47 @@ import Testing @Suite("ParsedHTTPRequest") struct ParsedHTTPRequestTests { + // MARK: - path / query + + @Test( + "path and query split the target", + arguments: [ + ("/upload", "/upload", ""), + ("/upload?_embed=wp:featuredmedia", "/upload", "?_embed=wp:featuredmedia"), + // A bare "?" carries no parameters, so the query is empty. + ("/upload?", "/upload", ""), + ("/wp/v2/posts?per_page=10&page=2", "/wp/v2/posts", "?per_page=10&page=2"), + // Only the first "?" separates path from query; later ones belong to it. + ("/search?q=a?b", "/search", "?q=a?b"), + ("/", "/", ""), + ] + ) + func pathAndQuery(target: String, expectedPath: String, expectedQuery: String) { + let request = ParsedHTTPRequest.complete( + method: "POST", + target: target, + httpVersion: "HTTP/1.1", + headers: [:], + body: nil + ) + + #expect(request.path == expectedPath) + #expect(request.query == expectedQuery) + } + + @Test("path and query are available on a partial request") + func pathAndQueryOnPartial() { + let request = ParsedHTTPRequest.partial( + method: "POST", + target: "/upload?_embed=wp:featuredmedia", + httpVersion: "HTTP/1.1", + headers: [:] + ) + + #expect(request.path == "/upload") + #expect(request.query == "?_embed=wp:featuredmedia") + } + // MARK: - urlRequest(relativeTo:) @Test("urlRequest resolves path against base URL") diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index ddb5d0cee..1d29e4ddc 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -100,6 +100,37 @@ struct MediaUploadServerTests { #expect(httpResponse.statusCode == 404) } + @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) + defer { server.stop() } + + // `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, + // so the middleware forwards that query on to the native server. Routing must + // match on the path alone, and the query must reach WordPress unchanged. + 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?_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 + + 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. + // 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) + #expect(!mockUploader.uploadCalled) + #expect(mockUploader.lastQuery == "?_embed=wp:featuredmedia") + } + @Test("calls delegate and returns upload result") func delegateProcessAndUpload() async throws { let delegate = MockUploadDelegate() @@ -599,11 +630,13 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab private var _passthroughUploadCalled = false private var _lastUploadMimeType: String? private var _lastUploadFilename: String? + private var _lastQuery: String? var uploadCalled: Bool { lock.withLock { _uploadCalled } } var passthroughUploadCalled: Bool { lock.withLock { _passthroughUploadCalled } } var lastUploadMimeType: String? { lock.withLock { _lastUploadMimeType } } var lastUploadFilename: String? { lock.withLock { _lastUploadFilename } } + var lastQuery: String? { lock.withLock { _lastQuery } } init() { super.init(httpClient: MockHTTPClient(), siteApiRoot: URL(string: "https://example.com/wp-json/")!) @@ -614,12 +647,16 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab _uploadCalled = true _lastUploadMimeType = mimeType _lastUploadFilename = filename + _lastQuery = query } return mockResponse() } override func passthroughUpload(body: RequestBody, contentType: String, query: String) async throws -> MediaUploadResponse { - lock.withLock { _passthroughUploadCalled = true } + lock.withLock { + _passthroughUploadCalled = true + _lastQuery = query + } return mockResponse() } diff --git a/src/utils/api-fetch-upload-middleware.test.js b/src/utils/api-fetch-upload-middleware.test.js index 586cf9602..07bac0062 100644 --- a/src/utils/api-fetch-upload-middleware.test.js +++ b/src/utils/api-fetch-upload-middleware.test.js @@ -238,6 +238,37 @@ describe( 'nativeMediaUploadMiddleware', () => { expect( fetchOptions.body.get( 'post' ) ).toBe( '123' ); } ); + it.each( [ + // A bare trailing `?` carries no parameters. The native `query` + // accessors normalize it away, so this side must too — otherwise the + // upload server receives a `?` the two platforms agree cannot exist. + [ '/wp/v2/media?', 'http://localhost:12345/upload' ], + [ '/wp/v2/media', 'http://localhost:12345/upload' ], + ] )( 'normalizes the query of %s to %s', async ( path, expectedUrl ) => { + getGBKit.mockReturnValue( { + nativeUploadPort: 12345, + nativeUploadToken: 'test-token', + } ); + + global.fetch = vi.fn( () => + Promise.resolve( { + ok: true, + json: () => Promise.resolve( { id: 1 } ), + } ) + ); + + const body = new FormData(); + body.append( 'file', makeFile(), 'photo.jpg' ); + + await nativeMediaUploadMiddleware( + { method: 'POST', path, body }, + makeNext() + ); + + const [ url ] = global.fetch.mock.calls[ 0 ]; + expect( url ).toBe( expectedUrl ); + } ); + it( 'returns the relayed WordPress attachment unchanged', async () => { getGBKit.mockReturnValue( { nativeUploadPort: 8080, diff --git a/src/utils/api-fetch.js b/src/utils/api-fetch.js index 7cf5673ff..e08d03220 100644 --- a/src/utils/api-fetch.js +++ b/src/utils/api-fetch.js @@ -206,8 +206,7 @@ export function nativeMediaUploadMiddleware( options, next ) { // (`post`, additionalData) — and the original query string (e.g. `?_embed`) // so the native server can relay them to WordPress unchanged. Rebuilding the // body with only `file` would drop the post association and additionalData. - const queryIndex = options.path.indexOf( '?' ); - const query = queryIndex === -1 ? '' : options.path.slice( queryIndex ); + const query = requestQuery( options.path ); // Use the two-argument form of `.then()` so the rejection handler catches // *only* a connection-level failure of the `fetch()` itself — not errors @@ -278,6 +277,28 @@ export function nativeMediaUploadMiddleware( options, next ) { ); } +/** + * The query component of a request path, including the leading `?`, or an empty + * string when there is no query. + * + * Mirrors the `query` accessors on the native request types (`HttpRequest` on + * Android, `ParsedHTTPRequest` on iOS): the split is on the first `?`, and a + * bare trailing `?` carries no parameters so it yields an empty string. Keeping + * the three in agreement means the value can be appended to an upstream URL + * unconditionally, whichever side derived it. + * + * @param {string} path The request path, e.g. `/wp/v2/media?_embed`. + * @return {string} The query, e.g. `?_embed`, or `''`. + */ +function requestQuery( path ) { + const separator = path.indexOf( '?' ); + if ( separator === -1 ) { + return ''; + } + const value = path.slice( separator + 1 ); + return value ? `?${ value }` : ''; +} + /** * The error rejected when the upload server's response body can't be parsed as * JSON. Shaped like a WordPress REST error so `@wordpress/media-utils` surfaces