From 7d967965057dc34f959e58dc58dc4fdf575dddb5 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:46:46 -0600 Subject: [PATCH] docs: state the invariant that makes the media field decode safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `formFields` decodes each non-file form value as UTF-8, which substitutes U+FFFD on malformed input. That is lossless today, but only because of an invariant nothing in the code states or enforces: the sole client is the editor's browser FormData. The server binds to loopback behind a per-session token; a FormData string value is a USVString, already well-formed at append time; and its only way to carry arbitrary bytes is a Blob, which always gets a filename and is filtered out of `extraParts`. Write that down on both platforms, including the part that makes it matter — if it stops holding, the two platforms are lossy *differently*, so there is no single behavior that could be documented instead. Relaxing each platform's filename filter demonstrates it: for `ED A0 80` Swift yields three replacement characters under its maximal-subpart rule where Java's decoder yields one. Cover the partition rather than the decode, since the partition is what makes the invariant true: a request carrying a second, Blob-shaped part whose bytes are not valid UTF-8 must not surface that part in `fields`. A second test pins the other half — valid UTF-8 (emoji, non-Latin scripts) round-trips exactly, so real captions are unaffected. Both fail when the filter is relaxed, so neither is vacuous. Neither asserts what becomes of that second part — it is currently dropped rather than relayed, which is a separate open question. Also reword the raw-bytes comment on the re-encode path. "So a non-UTF-8 value is forwarded verbatim" read as though malformed values were expected, which made the two delivery paths look contradictory. The actual hazard is the failable decode returning nil and an obvious `?? ""` dropping the whole value; the reason to keep bytes is that the re-encode should stay byte-identical to the passthrough it stands in for. Finally, restore `attachmentId`'s doc comment, which an earlier commit in this series orphaned onto `formFields` when it inserted the helper above it. --- .../wordpress/gutenberg/MediaUploadServer.kt | 18 +++- .../gutenberg/MediaUploadServerTest.kt | 82 +++++++++++++++++++ .../Sources/Media/MediaUploadServer.swift | 29 ++++++- .../Media/MediaUploadServerTests.swift | 73 +++++++++++++++++ 4 files changed, 196 insertions(+), 6 deletions(-) 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 ad440bd24..c6da826eb 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -600,6 +600,15 @@ internal class MediaUploadServer( * * A list rather than a map so repeated names (e.g. a `field[]` array) survive * verbatim, in the order the editor sent them. + * + * The UTF-8 decode is lossless because of an invariant nothing enforces: the only + * client is the editor's browser FormData. The server binds to loopback behind a + * per-session token; a FormData string value is a USVString, already well-formed at + * append time; and its only way to carry arbitrary bytes is a Blob, which always + * gets a filename and so is filtered out of extraParts. Valid UTF-8 — emoji, any + * script — round-trips exactly. If that stops holding this silently substitutes + * U+FFFD, and differently from iOS (Java reports one replacement char for ED A0 80 + * where Swift's maximal-subpart rule reports three). */ private fun formFields(parts: List): List = parts.map { MediaUploadField(it.name, String(it.body.readBytes(), Charsets.UTF_8)) } @@ -695,9 +704,12 @@ internal open class InternalMediaClient( 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. + // Each field's raw bytes are appended rather than round-tripped through String + // — not because malformed values are expected (they can't reach here; see the + // invariant on formFields), but so this re-encode stays byte-identical to the + // passthrough it stands in for: a user's upload shouldn't change shape just + // because a processor resized the image. filename=null makes it a plain field, + // matching okhttp's String overload byte-for-byte. for (part in extraParts) { builder.addFormDataPart(part.name, null, part.body.readBytes().toRequestBody()) } diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt index fc0a73da4..d92d53f57 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -274,6 +274,88 @@ class MediaUploadServerTest { assertFalse(client.uploadCalled) } + @Test + fun `keeps a binary Blob part out of an uploader's fields`() { + // formFields decodes each value as UTF-8, which is lossless only because the + // editor's FormData can't put arbitrary bytes in a *non-file* part: a Blob always + // carries a filename, so the partition drops it before the decode. Pin the + // partition, not the decode — the partition is what makes the invariant true. + 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 { + write("--$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()) + 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 + ) + + // Only the real form field surfaces; the Blob part never reaches the decode. + // (What becomes of that part is a separate question — it is currently dropped.) + assertEquals(listOf(MediaUploadField("post", "42")), uploader.received?.fields) + } + + @Test + fun `round-trips a non-Latin field value exactly`() { + // The other half of the invariant: valid UTF-8 is lossless, so real captions and + // titles are unaffected by the decode. + 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 delegate's metadata gate would have declined`() { // The gate exists to skip a temp copy for a file the processor won't touch. An diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 456b1fca4..2bc901a22 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -397,6 +397,23 @@ final class MediaUploadServer: Sendable { /// /// A list rather than a dictionary so repeated names (e.g. a `field[]` array) /// survive verbatim, in the order the editor sent them. + /// + /// The UTF-8 decode is lossless here because of an invariant worth stating, since + /// nothing in the type system enforces it: **the only client is the editor's + /// browser `FormData`.** The server binds to loopback behind a per-session token, + /// so nothing else can reach it; a `FormData` string value is a `USVString`, which + /// the browser has already made well-formed at `append` time; and its only way to + /// carry arbitrary bytes is a Blob, which always gets a filename and is therefore + /// filtered out of `extraParts` by `handleUpload`. Valid UTF-8 — including emoji + /// and any non-Latin script — round-trips exactly, so real captions and titles are + /// unaffected. + /// + /// If that ever stops holding, this decode starts substituting U+FFFD *and* the two + /// platforms disagree about how: for `ED A0 80`, Swift's maximal-subpart rule yields + /// three replacement characters where Java's decoder yields one. There is no single + /// behavior that could be documented instead, which is why the invariant is the + /// thing to pin. `MediaUploadServerTests` covers the partition that keeps binary + /// parts out of here. private static func formFields(from parts: [MultipartPart]) async throws -> [MediaUploadField] { var fields: [MediaUploadField] = [] for part in parts { @@ -687,9 +704,15 @@ class InternalMediaClient: @unchecked Sendable { ) throws -> (InputStream, Int) { // Serialize the non-file parts (post, additionalData) into the preamble // ahead of the streamed file. They are small, so keeping them in memory is - // fine; `contentLength` counts them via `preamble.count`. Field values are - // appended as raw bytes (not through String) so a non-UTF-8 value is - // forwarded verbatim rather than coerced to empty. + // fine; `contentLength` counts them via `preamble.count`. + // + // Field values are appended as raw bytes rather than round-tripped through + // `String`. Not because malformed values are expected — they can't reach here; + // see the invariant on `formFields`. It is because the failable + // `String(data:encoding:)` returns nil on invalid UTF-8, and the obvious `?? ""` + // behind it would silently drop a whole field's value. Appending the bytes keeps + // this re-encode byte-identical to the passthrough it stands in for, so a user's + // upload doesn't change shape just because a processor resized the image. var preamble = Data() for field in extraFields { preamble.append(Data("--\(boundary)\r\n".utf8)) diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 15adf10ad..e167c75cd 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -451,6 +451,79 @@ struct MediaUploadServerTests { #expect(received.query == "?_embed=wp:featuredmedia") } + @Test("keeps a binary Blob part out of an uploader's fields") + func binaryPartExcludedFromFields() async throws { + // `formFields` decodes each value as UTF-8, which is lossless only because the + // editor's FormData can't put arbitrary bytes in a *non-file* part: a Blob always + // carries a filename, so the partition drops it before the decode. Pin the + // partition, not the decode — the partition is what makes the invariant true. + let uploader = RecordingUploader() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + let boundary = UUID().uuidString + var body = Data() + body.append("--\(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") + 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) + + // Only the real form field surfaces; the Blob part never reaches the decode. + // (What becomes of that part is a separate question — it is currently dropped.) + let received = try #require(uploader.received) + #expect(received.fields == [MediaUploadField(name: "post", value: "42")]) + } + + @Test("round-trips a non-Latin field value exactly") + func nonLatinFieldRoundTrips() async throws { + // The other half of the invariant: valid UTF-8 is lossless, so real captions and + // titles are unaffected by the decode. + 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 delegate still processes the file an uploader delivers") func delegateProcessesForUploader() async throws { let delegate = MockProcessor()