From e6d317dd5914239ad275c6ff56bc931124893184 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 21 Aug 2026 19:24:26 -0700 Subject: [PATCH 1/8] ADFA-5241: Declare utf-8 on text responses from the documentation database MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WebServer wrote the stored MIME type into Content-Type verbatim, and no ContentTypes.value in documentation.db carries a charset, so every text response left its encoding unstated. A client that does not assume UTF-8 falls back to a legacy single-byte encoding: of 500 sampled text rows, 328 (66%) contain non-ASCII bytes with no BOM, and those render as mojibake. Observed on device while confirming ADFA-5239 -- a/android/R.id.html shows "   ↳android.R.id" where the page says "↳android.R.id". Two things made this invisible until now. Those pages were being served as text/plain (ADFA-5239), so they rendered as source rather than as documents, and Android's WebSettings.defaultTextEncodingName defaults to UTF-8, which likely spares the in-app viewer -- nothing in the app sets that property, so it relies on the default. Neither is a reason to leave the header unstated: correctness by client default is per-client behaviour, not something this server declares, and the same file already sends "; charset=utf-8" on its hardcoded responses (lines 1176, 1225, 1247, 1310). Only database-sourced content was missing it. The predicate lives in common/ContentTypeHeaders rather than in WebServer, because ADFA-5176's in-process transport already answers this question on its own -- DocumentationRequestInterceptor.mimeAndCharset defaults text/* to utf-8 -- and the two transports disagreeing about what a response says is worse than either answer. That branch should adopt this on its rebase. What gets a charset, and why not more: every text subtype, including the database's malformed bare "text" (which 726 TooltipButtons reach via x.html) and "text/text"; XML-based types, since SVG usually omits its own declaration and a transport charset takes precedence anyway. Not application/json -- RFC 8259 defines no charset parameter for it and fixes the encoding as UTF-8, so declaring one says nothing; there is a test asserting that, so nobody "fixes" it later. An already-declared charset is never doubled. Deliberately not fixed in the database. Putting the parameter in ContentTypes.value would work for both readers, but that column doubles as a lookup key matched exactly by ExtensionToContentTypeResolver (the plugin installer would skip every HTML asset), by docdb-studio's anchor extraction, and by three OfflineDocumentationTools scripts. Two of those fail silently. The analysis is on ADFA-5241. Tests: 7 for the helper -- text, the malformed types, XML, the binary set, the JSON exclusion, no doubling, and casing/parameter tolerance -- plus one that asserts the header a real client receives, since the helper being right does not prove the response is. Co-Authored-By: Claude Opus 5 (1M context) --- .../androidide/localWebServer/WebServer.kt | 3 +- .../localWebServer/WebServerTest.kt | 59 +++++++++++++++ .../androidide/utils/ContentTypeHeaders.kt | 54 ++++++++++++++ .../utils/ContentTypeHeadersTest.kt | 73 +++++++++++++++++++ 4 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt create mode 100644 common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index a978a8f286..d9c6c03537 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -10,6 +10,7 @@ import com.google.gson.Gson import com.google.gson.GsonBuilder import com.google.gson.ToNumberPolicy import com.google.gson.reflect.TypeToken +import com.itsaky.androidide.utils.ContentTypeHeaders import com.itsaky.androidide.utils.DatabaseVersionResolver import io.pebbletemplates.pebble.PebbleEngine import io.pebbletemplates.pebble.loader.StringLoader @@ -658,7 +659,7 @@ class WebServer( } writer.println("HTTP/1.1 200 OK") - writer.println("Content-Type: $dbMimeType") + writer.println("Content-Type: ${ContentTypeHeaders.headerValue(dbMimeType)}") writer.println("Content-Length: ${dbContent.size}") writer.println("Connection: close") writer.println() diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt index e68b2e05e4..4f66e603e5 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -10,6 +10,7 @@ import io.mockk.mockkStatic import io.mockk.unmockkAll import io.mockk.verify import org.junit.After +import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before @@ -377,6 +378,64 @@ class WebServerTest { } } + // ADFA-5241: the helper decides the charset, but only a real response proves the header that + // reaches a client. Two thirds of the database's text rows are non-ASCII with no BOM, so an + // undeclared encoding renders them as mojibake in any client that does not assume UTF-8. + @Test + fun `a text response declares utf-8 and a binary one does not`() { + assertContentTypeHeader(storedMimeType = "text/html", expected = "text/html; charset=utf-8") + assertContentTypeHeader(storedMimeType = "image/png", expected = "image/png") + } + + private fun assertContentTypeHeader( + storedMimeType: String, + expected: String, + ) { + val port = freePort() + val db = mockk(relaxed = true) + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns db + stubDeclaredMajorVersion(db, DatabaseVersionResolver.MAJOR_VERSION_WITH_COMPRESSION_DICTIONARY) + every { + db.rawQuery(match { it.contains("FROM Content") }, any()) + } returns + mockk(relaxed = true) { + every { count } returns 1 + every { moveToFirst() } returns true + every { getBlob(0) } returns "payload".toByteArray() + every { getString(1) } returns storedMimeType + every { getString(2) } returns "none" + every { getInt(3) } returns 0 + } + + val server = WebServer(testConfig(port)) + val serverThread = Thread { server.start() }.apply { isDaemon = true } + serverThread.start() + try { + awaitPortBound(port) + val response = sendRawGetRequest(port, "/some/path") + val header = response.lineSequence().first { it.startsWith("Content-Type:", ignoreCase = true) } + assertEquals("Content-Type: $expected", header.trim()) + } finally { + server.stop() + serverThread.join(2_000) + } + } + + // Same as sendRawGetRequestAndAwaitClose, but hands back what the server actually wrote. + private fun sendRawGetRequest( + port: Int, + path: String, + ): String = + Socket().use { socket -> + socket.connect(InetSocketAddress("localhost", port), 2_000) + socket.soTimeout = 2_000 + socket.getOutputStream().apply { + write("GET $path HTTP/1.1\r\n\r\n".toByteArray(Charsets.ISO_8859_1)) + flush() + } + socket.getInputStream().readBytes().toString(Charsets.ISO_8859_1) + } + // Blocks until the server closes the connection (every response sends "Connection: close"), // so by the time this returns the server has fully finished processing this one request -- // making repeated calls a reliable way to serialize several full request/response cycles. diff --git a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt new file mode 100644 index 0000000000..c94d62915b --- /dev/null +++ b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt @@ -0,0 +1,54 @@ +package com.itsaky.androidide.utils + +/** + * Turns a stored `ContentTypes.value` into the `Content-Type` a client should be sent. + * + * `documentation.db` stores bare MIME types -- no `ContentTypes.value` carries a charset -- so a + * text response says nothing about its encoding, and a client that does not assume UTF-8 falls back + * to a legacy single-byte encoding. Two thirds of the database's text rows contain non-ASCII + * bytes with no BOM, so they render as mojibake wherever that guess goes wrong (ADFA-5241): + * `↳android.R.id` arrives as `   ↳android.R.id`. + * + * This is deliberately *not* fixed by storing the parameter in the database. `ContentTypes.value` + * doubles as a lookup key matched exactly by the plugin installer + * (`ExtensionToContentTypeResolver`), by `docdb-studio`'s anchor extraction, and by three scripts in + * `OfflineDocumentationTools`; two of those would fail silently rather than loudly. + * + * Kept in `common` so both documentation transports answer the same way -- the socket server here + * and ADFA-5176's `shouldInterceptRequest` interceptor, which otherwise carries its own rule. + */ +object ContentTypeHeaders { + private const val UTF_8 = "utf-8" + + /** + * The charset to declare for [mimeType], or null when the type is binary, when it already + * carries a charset, or when the format defines its encoding itself. + * + * `application/json` is deliberately absent: RFC 8259 defines no charset parameter for it and + * fixes the encoding as UTF-8, so declaring one is meaningless rather than helpful. XML-based + * types are included even though a document may carry its own declaration, because a + * transport-level charset takes precedence and SVG in particular usually omits the declaration. + */ + fun charsetFor(mimeType: String): String? { + if (mimeType.contains("charset=", ignoreCase = true)) { + return null + } + val type = mimeType.substringBefore(';').trim().lowercase() + return when { + // Covers every text subtype, plus the database's bare "text" and "text/text" oddities. + type.startsWith("text") -> UTF_8 + + type.endsWith("+xml") || type == "application/xml" -> UTF_8 + + type == "application/x-typescript" -> UTF_8 + + else -> null + } + } + + /** [mimeType] with a charset appended when [charsetFor] gives one, otherwise unchanged. */ + fun headerValue(mimeType: String): String { + val charset = charsetFor(mimeType) ?: return mimeType + return "$mimeType; charset=$charset" + } +} diff --git a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt new file mode 100644 index 0000000000..17b96bc4c8 --- /dev/null +++ b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt @@ -0,0 +1,73 @@ +package com.itsaky.androidide.utils + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** ADFA-5241: every text response has to declare its encoding, and no binary response may. */ +class ContentTypeHeadersTest { + @Test + fun `text types get utf-8`() { + for (type in listOf("text/html", "text/css", "text/javascript", "text/markdown", "text/plain")) { + assertEquals("$type; charset=utf-8", ContentTypeHeaders.headerValue(type)) + } + } + + // documentation.db really contains these two: a bare "text" (which 726 TooltipButtons point at, + // via x.html) and a "text/text". Neither is a valid MIME type, but both are text. + @Test + fun `the database's malformed text types are still treated as text`() { + assertEquals("text; charset=utf-8", ContentTypeHeaders.headerValue("text")) + assertEquals("text/text; charset=utf-8", ContentTypeHeaders.headerValue("text/text")) + } + + @Test + fun `xml-based types get utf-8, since svg rarely declares its own`() { + assertEquals("image/svg+xml; charset=utf-8", ContentTypeHeaders.headerValue("image/svg+xml")) + assertEquals("application/xml; charset=utf-8", ContentTypeHeaders.headerValue("application/xml")) + } + + @Test + fun `binary types are left alone`() { + for (type in listOf( + "image/png", + "image/gif", + "image/jpeg", + "image/webp", + "image/x-icon", + "video/mp4", + "video/quicktime", + "application/pdf", + "application/wasm", + "font/woff2", + "font/ttf", + "application/octet-stream", + "application/vnd-iccprofile", + )) { + assertNull(ContentTypeHeaders.charsetFor(type)) + assertEquals(type, ContentTypeHeaders.headerValue(type)) + } + } + + // RFC 8259 defines no charset parameter for JSON and fixes the encoding as UTF-8, so declaring + // one says nothing. Asserted so nobody "fixes" this by adding it. + @Test + fun `json is left alone`() { + assertNull(ContentTypeHeaders.charsetFor("application/json")) + assertEquals("application/json", ContentTypeHeaders.headerValue("application/json")) + } + + @Test + fun `an existing charset is never doubled`() { + assertEquals("text/html; charset=utf-8", ContentTypeHeaders.headerValue("text/html; charset=utf-8")) + assertEquals("text/html; charset=iso-8859-1", ContentTypeHeaders.headerValue("text/html; charset=iso-8859-1")) + assertEquals("text/html; CHARSET=UTF-8", ContentTypeHeaders.headerValue("text/html; CHARSET=UTF-8")) + } + + @Test + fun `parameters and casing on the type itself are tolerated`() { + assertEquals("TEXT/HTML; charset=utf-8", ContentTypeHeaders.headerValue("TEXT/HTML")) + assertEquals("text/html ; charset=utf-8", ContentTypeHeaders.headerValue("text/html ")) + assertEquals("text/html;boundary=x; charset=utf-8", ContentTypeHeaders.headerValue("text/html;boundary=x")) + } +} From 7cc8cbbe65b6d00eee3477cda2e6d7299c2786fb Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 21 Aug 2026 19:36:16 -0700 Subject: [PATCH 2/8] ADFA-5241: Match the media type and the charset parameter at their boundaries Both from CodeRabbit on PR #1725. `type.startsWith("text")` classified `textual/example` as a text response. The comment beside it already said the intent was "every text subtype, plus the bare text oddity" -- the code just did not say that. Now `type == "text" || type.startsWith("text/")`. `mimeType.contains("charset=")` found the substring inside *another* parameter's value, so `text/html; note="charset=utf-8"` looked like it already declared an encoding and got none added. Parameters are now split on `;` and matched by name, which also keeps `text/html;boundary=x` working. Neither case exists in documentation.db today -- no ContentTypes.value carries a parameter at all -- so this is about the helper being honest rather than a live defect. Both have regression tests. Also dropped the non-ASCII from the KDoc, which quoted the mojibake it was describing. The ASCII policy exempts a glyph doing real visual work, and I had read the example as qualifying; naming the code point instead reads the same, which means it does not qualify. The file is now pure ASCII. Co-Authored-By: Claude Opus 5 (1M context) --- .../androidide/utils/ContentTypeHeaders.kt | 22 ++++++++++++++----- .../utils/ContentTypeHeadersTest.kt | 17 ++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt index c94d62915b..13e3ecd821 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt @@ -6,8 +6,8 @@ package com.itsaky.androidide.utils * `documentation.db` stores bare MIME types -- no `ContentTypes.value` carries a charset -- so a * text response says nothing about its encoding, and a client that does not assume UTF-8 falls back * to a legacy single-byte encoding. Two thirds of the database's text rows contain non-ASCII - * bytes with no BOM, so they render as mojibake wherever that guess goes wrong (ADFA-5241): - * `↳android.R.id` arrives as `   ↳android.R.id`. + * bytes with no BOM, so they render as mojibake wherever that guess goes wrong (ADFA-5241): a + * page's U+21B3 arrow arriving as the three Latin-1 characters its UTF-8 bytes decode to. * * This is deliberately *not* fixed by storing the parameter in the database. `ContentTypes.value` * doubles as a lookup key matched exactly by the plugin installer @@ -30,13 +30,14 @@ object ContentTypeHeaders { * transport-level charset takes precedence and SVG in particular usually omits the declaration. */ fun charsetFor(mimeType: String): String? { - if (mimeType.contains("charset=", ignoreCase = true)) { + if (declaresCharset(mimeType)) { return null } val type = mimeType.substringBefore(';').trim().lowercase() return when { - // Covers every text subtype, plus the database's bare "text" and "text/text" oddities. - type.startsWith("text") -> UTF_8 + // Every text subtype, plus the database's bare "text" oddity. Matched at the boundary: + // "textual/example" is not a text type, and startsWith("text") would say it is. + type == "text" || type.startsWith("text/") -> UTF_8 type.endsWith("+xml") || type == "application/xml" -> UTF_8 @@ -46,6 +47,17 @@ object ContentTypeHeaders { } } + /** + * Whether [mimeType] already carries a `charset` *parameter*. Substring-matching "charset=" + * instead would be fooled by another parameter's value -- `note="charset=utf-8"` -- and would + * suppress a declaration the response needs. + */ + private fun declaresCharset(mimeType: String): Boolean = + mimeType + .split(';') + .drop(1) + .any { it.substringBefore('=').trim().equals("charset", ignoreCase = true) } + /** [mimeType] with a charset appended when [charsetFor] gives one, otherwise unchanged. */ fun headerValue(mimeType: String): String { val charset = charsetFor(mimeType) ?: return mimeType diff --git a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt index 17b96bc4c8..3dfcb0f82b 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt @@ -64,6 +64,23 @@ class ContentTypeHeadersTest { assertEquals("text/html; CHARSET=UTF-8", ContentTypeHeaders.headerValue("text/html; CHARSET=UTF-8")) } + // startsWith("text") would call this a text type. The intent is "text" or "text/", nothing else. + @Test + fun `a type that merely begins with text is not a text type`() { + assertNull(ContentTypeHeaders.charsetFor("textual/example")) + assertEquals("textual/example", ContentTypeHeaders.headerValue("textual/example")) + } + + // Substring-matching "charset=" would find it inside another parameter's value and suppress the + // declaration this response actually needs. + @Test + fun `charset inside another parameter's value does not count as a declaration`() { + assertEquals( + """text/html; note="charset=utf-8"; charset=utf-8""", + ContentTypeHeaders.headerValue("""text/html; note="charset=utf-8""""), + ) + } + @Test fun `parameters and casing on the type itself are tolerated`() { assertEquals("TEXT/HTML; charset=utf-8", ContentTypeHeaders.headerValue("TEXT/HTML")) From f245b631eb5ebbaf36865775e123efaf7c834322 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 21 Aug 2026 20:55:16 -0700 Subject: [PATCH 3/8] ADFA-5241: Parse Content-Type parameters properly, and widen what counts as text From the code review on PR #1725. Four correctness findings, all real. Splitting on ';' still finds "charset=" inside a quoted value that contains a semicolon -- text/html; note="x; charset=utf-8" -- so the helper concluded a charset was already declared and sent none at all: the exact failure its own KDoc claimed the split prevented. And a parameter with no value (; charset) or an empty one (; charset=) was read as a declaration, with the same result. Parameters are now parsed with quote awareness, and only a charset parameter with a non-empty value counts as declared. The textual application/* list omitted application/javascript, which does have rows and is what ExtensionToContentTypeResolver maps ".mjs" to, so real files served undeclared -- the bug this class exists to prevent. Added it along with ecmascript and x-sh, and said in the comment that the list is a list precisely because these types share no marker, so anything textual arriving later has to be added rather than assumed covered. The header value is now built before the status line goes out. The writer autoflushes, so a throw after the first println made sendError append a second status line to a response that already claimed 200, which a client parses as a malformed header rather than as an error. dbMimeType is a platform type from Cursor.getString, so a NULL ContentTypes.value is a real way to reach that throw. typeAndCharset is exposed because ADFA-5176's interceptor needs the type and the charset apart for WebResourceResponse and was re-implementing the parse to get them -- with the naive substring match this file warns against. The class was created so both transports answer alike; keeping the parse private meant they agreed on the default and disagreed on reading what was already there. The "two thirds" statistic is replaced with a full census of the database it was measured on: 17,903 of 29,139 text rows, 61.4%. The review measured 22.5% against the bundled asset, which is an older export -- both numbers are right for their own database, so the KDoc now names which one and says the rate is per-generation. Four regression tests for the parsing cases, one for the textual types, one for typeAndCharset. The header assertion now prints the whole response when no Content-Type is found instead of throwing NoSuchElementException, and no longer declares a compression-dictionary version it does not use. Co-Authored-By: Claude Opus 5 (1M context) --- .../androidide/localWebServer/WebServer.kt | 9 +- .../localWebServer/WebServerTest.kt | 8 +- .../androidide/utils/ContentTypeHeaders.kt | 103 +++++++++++++++--- .../utils/ContentTypeHeadersTest.kt | 38 +++++++ 4 files changed, 139 insertions(+), 19 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index d9c6c03537..659ec53cdc 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -658,8 +658,15 @@ class WebServer( dbContent = instantiatePebbleTemplate(templateId, dbContent, path, dbMimeType, compression) } + // Built before the status line goes out: everything after the first println is on the + // wire (the writer autoflushes), so a throw past that point makes sendError append a + // second status line to a response that already claimed 200, which a client parses as a + // malformed header rather than as an error. dbMimeType is a platform type from + // Cursor.getString, so a NULL ContentTypes.value throws here rather than there. + val contentTypeHeader = ContentTypeHeaders.headerValue(dbMimeType) + writer.println("HTTP/1.1 200 OK") - writer.println("Content-Type: ${ContentTypeHeaders.headerValue(dbMimeType)}") + writer.println("Content-Type: $contentTypeHeader") writer.println("Content-Length: ${dbContent.size}") writer.println("Connection: close") writer.println() diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt index 4f66e603e5..3bbb403556 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -394,7 +394,9 @@ class WebServerTest { val port = freePort() val db = mockk(relaxed = true) every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns db - stubDeclaredMajorVersion(db, DatabaseVersionResolver.MAJOR_VERSION_WITH_COMPRESSION_DICTIONARY) + // Deliberately no version stub: this row is compression = "none", so nothing decodes and no + // dictionary is consulted. Declaring one would couple the assertion to a lazy-load path it + // does not exercise. every { db.rawQuery(match { it.contains("FROM Content") }, any()) } returns @@ -413,7 +415,9 @@ class WebServerTest { try { awaitPortBound(port) val response = sendRawGetRequest(port, "/some/path") - val header = response.lineSequence().first { it.startsWith("Content-Type:", ignoreCase = true) } + val header = + response.lineSequence().firstOrNull { it.startsWith("Content-Type:", ignoreCase = true) } + ?: error("No Content-Type in the response:\n$response") assertEquals("Content-Type: $expected", header.trim()) } finally { server.stop() diff --git a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt index 13e3ecd821..8553025fd2 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt @@ -5,9 +5,11 @@ package com.itsaky.androidide.utils * * `documentation.db` stores bare MIME types -- no `ContentTypes.value` carries a charset -- so a * text response says nothing about its encoding, and a client that does not assume UTF-8 falls back - * to a legacy single-byte encoding. Two thirds of the database's text rows contain non-ASCII - * bytes with no BOM, so they render as mojibake wherever that guess goes wrong (ADFA-5241): a - * page's U+21B3 arrow arriving as the three Latin-1 characters its UTF-8 bytes decode to. + * to a legacy single-byte encoding. 17,903 of the 29,139 text rows in the 21-Aug database -- + * 61.4%, a full census rather than a sample -- contain non-ASCII bytes with no BOM, so they render + * as mojibake wherever that guess goes wrong (ADFA-5241): a page's U+21B3 arrow arriving as the + * three Latin-1 characters its UTF-8 bytes decode to. (The rate is per-generation; an older export + * measures far lower, so quote the database when quoting the number.) * * This is deliberately *not* fixed by storing the parameter in the database. `ContentTypes.value` * doubles as a lookup key matched exactly by the plugin installer @@ -20,6 +22,17 @@ package com.itsaky.androidide.utils object ContentTypeHeaders { private const val UTF_8 = "utf-8" + // Textual types outside text/* and the +xml family. application/x-typescript has no rows in the + // current database but costs nothing to keep; application/javascript does have rows, and is what + // ExtensionToContentTypeResolver maps ".mjs" to, so omitting it served real files undeclared. + private val TEXTUAL_APPLICATION_TYPES = + setOf( + "application/javascript", + "application/ecmascript", + "application/x-typescript", + "application/x-sh", + ) + /** * The charset to declare for [mimeType], or null when the type is binary, when it already * carries a charset, or when the format defines its encoding itself. @@ -29,10 +42,22 @@ object ContentTypeHeaders { * types are included even though a document may carry its own declaration, because a * transport-level charset takes precedence and SVG in particular usually omits the declaration. */ - fun charsetFor(mimeType: String): String? { - if (declaresCharset(mimeType)) { - return null - } + fun charsetFor(mimeType: String): String? = if (declaredCharset(mimeType) != null) null else defaultCharsetFor(mimeType) + + /** + * The bare media type and the charset to send with it: whatever [mimeType] already declares, + * otherwise this class's default for that type, otherwise null. + * + * Exists because `WebResourceResponse(type, encoding, stream)` wants the two apart, and the + * in-process transport was re-implementing the parse to get them -- with the naive substring + * match this file warns against below. One parse, both transports. + */ + fun typeAndCharset(mimeType: String): Pair { + val type = mimeType.substringBefore(';').trim() + return type to (declaredCharset(mimeType) ?: defaultCharsetFor(mimeType)) + } + + private fun defaultCharsetFor(mimeType: String): String? { val type = mimeType.substringBefore(';').trim().lowercase() return when { // Every text subtype, plus the database's bare "text" oddity. Matched at the boundary: @@ -41,22 +66,68 @@ object ContentTypeHeaders { type.endsWith("+xml") || type == "application/xml" -> UTF_8 - type == "application/x-typescript" -> UTF_8 + // Textual application/* types share no syntactic marker, hence a list. application/json + // is deliberately absent (see the class KDoc). Anything textual that turns up later and + // is not here serves undeclared -- the bug this class exists to prevent -- so add it + // rather than assuming the list is complete. + type in TEXTUAL_APPLICATION_TYPES -> UTF_8 else -> null } } /** - * Whether [mimeType] already carries a `charset` *parameter*. Substring-matching "charset=" - * instead would be fooled by another parameter's value -- `note="charset=utf-8"` -- and would - * suppress a declaration the response needs. + * The charset [mimeType] already declares, or null when it declares none that is usable. + * + * Parsed rather than substring-matched: "charset=" occurs inside other parameters' values + * (`note="charset=utf-8"`), and splitting naively on ';' still finds it when the value itself + * contains a semicolon (`note="x; charset=utf-8"`). A parameter with no value (`; charset`) or + * an empty one (`; charset=`) declares nothing and must not suppress the default -- treating it + * as a declaration is how a response ends up with no encoding at all. */ - private fun declaresCharset(mimeType: String): Boolean = - mimeType - .split(';') - .drop(1) - .any { it.substringBefore('=').trim().equals("charset", ignoreCase = true) } + private fun declaredCharset(mimeType: String): String? = + parameters(mimeType) + .firstOrNull { (name, _) -> name.equals("charset", ignoreCase = true) } + ?.second + ?.ifEmpty { null } + + /** [mimeType]'s `name=value` parameters, with semicolons inside quoted values left alone. */ + private fun parameters(mimeType: String): List> { + val found = mutableListOf>() + val token = StringBuilder() + var quoted = false + + fun take() { + val text = token.toString().trim() + token.setLength(0) + if (text.isEmpty()) return + val name = text.substringBefore('=').trim() + val value = if (text.contains('=')) text.substringAfter('=').trim().trim('"') else "" + found += name to value + } + + var index = mimeType.indexOf(';') + if (index < 0) return found + while (++index < mimeType.length) { + val character = mimeType[index] + when { + character == '"' -> { + quoted = !quoted + token.append(character) + } + + character == ';' && !quoted -> { + take() + } + + else -> { + token.append(character) + } + } + } + take() + return found + } /** [mimeType] with a charset appended when [charsetFor] gives one, otherwise unchanged. */ fun headerValue(mimeType: String): String { diff --git a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt index 3dfcb0f82b..bef239bb63 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt @@ -65,6 +65,44 @@ class ContentTypeHeadersTest { } // startsWith("text") would call this a text type. The intent is "text" or "text/", nothing else. + // Splitting on ';' alone still finds "charset=" inside a quoted value that contains a semicolon, + // and then suppresses the declaration the response actually needs. + @Test + fun `a semicolon inside a quoted parameter value does not hide the charset`() { + assertEquals( + """text/html; note="x; charset=utf-8"; charset=utf-8""", + ContentTypeHeaders.headerValue("""text/html; note="x; charset=utf-8""""), + ) + } + + // A parameter with no value, or an empty one, declares nothing -- so it must not stop the + // default from being added. Treating it as a declaration ships a response with no encoding. + @Test + fun `a valueless or empty charset parameter is not a declaration`() { + assertEquals("text/html; charset; charset=utf-8", ContentTypeHeaders.headerValue("text/html; charset")) + assertEquals("text/html; charset=; charset=utf-8", ContentTypeHeaders.headerValue("text/html; charset=")) + } + + // Textual application/* types have no syntactic marker in common. application/javascript is the + // one with rows in the database and is what ".mjs" resolves to. + @Test + fun `textual application types get utf-8 and json still does not`() { + assertEquals("application/javascript; charset=utf-8", ContentTypeHeaders.headerValue("application/javascript")) + assertEquals("application/ecmascript; charset=utf-8", ContentTypeHeaders.headerValue("application/ecmascript")) + assertNull(ContentTypeHeaders.charsetFor("application/json")) + } + + // The other transport needs the two apart for WebResourceResponse, and must not re-parse. + @Test + fun `typeAndCharset splits the type from the charset it should declare`() { + assertEquals("text/html" to "utf-8", ContentTypeHeaders.typeAndCharset("text/html")) + assertEquals("image/png" to null, ContentTypeHeaders.typeAndCharset("image/png")) + assertEquals("text/html" to "iso-8859-1", ContentTypeHeaders.typeAndCharset("text/html; charset=iso-8859-1")) + assertEquals("text/html" to "UTF-8", ContentTypeHeaders.typeAndCharset("text/html; CHARSET=UTF-8")) + // a declared-but-empty parameter falls back to the default rather than to nothing + assertEquals("text/html" to "utf-8", ContentTypeHeaders.typeAndCharset("text/html; charset=")) + } + @Test fun `a type that merely begins with text is not a text type`() { assertNull(ContentTypeHeaders.charsetFor("textual/example")) From c98e5a210aeb0d9db6604ccb9653b1e61c06cdf4 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 21 Aug 2026 22:47:47 -0700 Subject: [PATCH 4/8] ADFA-5241: Harden the Content-Type parameter parse against two malformed inputs Both from the re-review on #1725. An escaped quote no longer ends a quoted parameter value. RFC 9110's quoted-pair means \" does not close the string, but the parser toggled on every quote, so text/html; note="a\"; charset=iso-8859-1 parsed as two parameters and the charset inside note read as a declaration -- the same false match this class exists to prevent. An empty charset parameter is now left alone rather than contradicted. headerValue used to append a second charset, producing text/html; charset=; charset=utf-8. Recipients keep an empty valued parameter and ignore a repeated name, so that append claims a fix it does not make. A parameter with no = at all is still appended to, because that one really is dropped during parsing, so the appended charset takes effect. typeAndCharset keeps substituting the default either way -- it hands the charset back as its own value, where nothing can conflict with it. Both tests were confirmed to fail against the previous parser. Co-Authored-By: Claude Opus 5 --- .../androidide/utils/ContentTypeHeaders.kt | 60 +++++++++++++++---- .../utils/ContentTypeHeadersTest.kt | 33 ++++++++-- 2 files changed, 78 insertions(+), 15 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt index 8553025fd2..4aa53508e4 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt @@ -42,7 +42,7 @@ object ContentTypeHeaders { * types are included even though a document may carry its own declaration, because a * transport-level charset takes precedence and SVG in particular usually omits the declaration. */ - fun charsetFor(mimeType: String): String? = if (declaredCharset(mimeType) != null) null else defaultCharsetFor(mimeType) + fun charsetFor(mimeType: String): String? = if (carriesCharsetParameter(mimeType)) null else defaultCharsetFor(mimeType) /** * The bare media type and the charset to send with it: whatever [mimeType] already declares, @@ -85,24 +85,52 @@ object ContentTypeHeaders { * an empty one (`; charset=`) declares nothing and must not suppress the default -- treating it * as a declaration is how a response ends up with no encoding at all. */ - private fun declaredCharset(mimeType: String): String? = - parameters(mimeType) - .firstOrNull { (name, _) -> name.equals("charset", ignoreCase = true) } - ?.second - ?.ifEmpty { null } - - /** [mimeType]'s `name=value` parameters, with semicolons inside quoted values left alone. */ - private fun parameters(mimeType: String): List> { - val found = mutableListOf>() + private fun declaredCharset(mimeType: String): String? = firstCharsetParameter(mimeType)?.second?.ifEmpty { null } + + /** + * Whether [mimeType] already carries a `charset` parameter *with an `=`*, usable or not -- the + * question [headerValue] has to ask, which is not the same as whether the charset is usable. + * + * `; charset=` (empty) and `; charset` (no value at all) are both useless as declarations, but + * recipients treat them differently. A parameter with no `=` is dropped during parsing, so an + * appended `; charset=utf-8` becomes the only one and takes effect. An empty *valued* parameter + * is kept, and a repeated parameter name is ignored, so appending a second one gets us a header + * carrying two conflicting charsets and, in a first-wins recipient, no change in behaviour. Not + * worth emitting: the empty parameter is a defect in the stored `ContentTypes.value` and belongs + * fixed there. [typeAndCharset] can and does still substitute the default, because it hands the + * charset back as its own value where nothing can conflict with it. + */ + private fun carriesCharsetParameter(mimeType: String): Boolean = firstCharsetParameter(mimeType)?.second != null + + /** + * [mimeType]'s first `charset` parameter, or null when it has none. First, not first-usable: + * that is the one a recipient keeps when a name repeats, so reading any other would honour a + * parameter the client ignores. + */ + private fun firstCharsetParameter(mimeType: String): Pair? = + parameters(mimeType).firstOrNull { (name, _) -> name.equals("charset", ignoreCase = true) } + + /** + * [mimeType]'s `name=value` parameters, with semicolons inside quoted values left alone. A null + * value means the parameter carried no `=` at all, which recipients drop entirely -- see + * [carriesCharsetParameter]. + */ + private fun parameters(mimeType: String): List> { + val found = mutableListOf>() val token = StringBuilder() var quoted = false + // RFC 9110 quoted-pair: inside a quoted string a backslash escapes the next character, so + // \" does not end the value. Without this, text/html; note="a\"; charset=iso-8859-1 parses + // as two parameters and the charset inside note reads as a declaration. + var escaped = false + fun take() { val text = token.toString().trim() token.setLength(0) if (text.isEmpty()) return val name = text.substringBefore('=').trim() - val value = if (text.contains('=')) text.substringAfter('=').trim().trim('"') else "" + val value = if (text.contains('=')) text.substringAfter('=').trim().trim('"') else null found += name to value } @@ -111,6 +139,16 @@ object ContentTypeHeaders { while (++index < mimeType.length) { val character = mimeType[index] when { + escaped -> { + escaped = false + token.append(character) + } + + quoted && character == '\\' -> { + escaped = true + token.append(character) + } + character == '"' -> { quoted = !quoted token.append(character) diff --git a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt index bef239bb63..aff0f235c1 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt @@ -75,12 +75,37 @@ class ContentTypeHeadersTest { ) } - // A parameter with no value, or an empty one, declares nothing -- so it must not stop the - // default from being added. Treating it as a declaration ships a response with no encoding. + // A parameter with no value declares nothing and is dropped during parsing, so appending the + // default works and is what the response needs. @Test - fun `a valueless or empty charset parameter is not a declaration`() { + fun `a valueless charset parameter is not a declaration`() { assertEquals("text/html; charset; charset=utf-8", ContentTypeHeaders.headerValue("text/html; charset")) - assertEquals("text/html; charset=; charset=utf-8", ContentTypeHeaders.headerValue("text/html; charset=")) + } + + // An empty *valued* parameter is kept by recipients, and a repeated name is ignored, so a second + // charset would conflict with it and change nothing. Emitting one claims a fix it does not make; + // the empty parameter is a defect in the stored value and belongs fixed there. + @Test + fun `an empty charset parameter is left alone rather than contradicted`() { + assertEquals("text/html; charset=", ContentTypeHeaders.headerValue("text/html; charset=")) + assertEquals( + "text/html; charset=; charset=iso-8859-1", + ContentTypeHeaders.headerValue("text/html; charset=; charset=iso-8859-1"), + ) + } + + // RFC 9110 quoted-pair. Toggling on every quote ends the value at the escaped one, and then the + // charset inside note parses as a declaration -- the same false match this class exists to stop. + @Test + fun `an escaped quote does not end a quoted parameter value`() { + assertEquals( + """text/html; note="a\"; charset=iso-8859-1"; charset=utf-8""", + ContentTypeHeaders.headerValue("""text/html; note="a\"; charset=iso-8859-1""""), + ) + assertEquals( + "text/html" to "utf-8", + ContentTypeHeaders.typeAndCharset("""text/html; note="a\"; charset=iso-8859-1""""), + ) } // Textual application/* types have no syntactic marker in common. application/javascript is the From f4e747a91e51aa28abba518d343e872ac5b4d314 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 11:45:04 -0700 Subject: [PATCH 5/8] ADFA-5241: Say on charsetFor what it does with an unusable charset Its KDoc said null meant the type already carried a charset, without saying that an empty one counts -- which is the interesting case, and the reason typeAndCharset answers differently for the same input. Both now state the asymmetry and why it exists. Co-Authored-By: Claude Opus 5 --- .../itsaky/androidide/utils/ContentTypeHeaders.kt | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt index 4aa53508e4..59ed1f682c 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt @@ -34,8 +34,14 @@ object ContentTypeHeaders { ) /** - * The charset to declare for [mimeType], or null when the type is binary, when it already - * carries a charset, or when the format defines its encoding itself. + * The charset to *append* to [mimeType], or null when there is nothing to add: the type is + * binary, the format defines its own encoding, or a `charset` parameter is already present. + * + * "Already present" includes an unusable one -- `text/html; charset=` gets nothing appended, + * because a recipient keeps that empty parameter and ignores a repeated name, so a second + * charset would conflict with it and change nothing. [typeAndCharset] deliberately answers + * differently for the same input: it returns the charset as its own value, where nothing can + * conflict with it, so it substitutes the default. The asymmetry is the point. * * `application/json` is deliberately absent: RFC 8259 defines no charset parameter for it and * fixes the encoding as UTF-8, so declaring one is meaningless rather than helpful. XML-based @@ -46,7 +52,9 @@ object ContentTypeHeaders { /** * The bare media type and the charset to send with it: whatever [mimeType] already declares, - * otherwise this class's default for that type, otherwise null. + * otherwise this class's default for that type, otherwise null. An empty `charset=` counts as + * declaring nothing, so the default applies -- unlike [charsetFor], which cannot append past one + * without contradicting it. * * Exists because `WebResourceResponse(type, encoding, stream)` wants the two apart, and the * in-process transport was re-implementing the parse to get them -- with the naive substring From 7e19ae30b385229cbfee30cf325566b2bac225dc Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 16:05:06 -0700 Subject: [PATCH 6/8] ADFA-5241: Build the Content-Type instead of appending to what was stored Review of this PR found that appending a charset to the stored string leaves the header no better formed than the database happened to be -- and that on the row this change was written for, it does nothing at all. The database stores a bare "text" (one row, x.html) and a "text/text" (ten licence files). Neither is a media type -- type "/" subtype is required -- and a client that cannot parse the type discards the charset with it, so those rows went on being sniffed exactly as before. Both normalize to text/plain now. A stored value carrying a control character was written into the response verbatim by println(). ContentTypes.value comes from a database that a debug build swaps in from shared storage, so a planted value containing CR/LF would split one response into two. Such a value is refused rather than repaired: application/octet-stream renders nothing and injects nothing. charsetFor and typeAndCharset disagreed for "text/html; charset=" -- one appended nothing, the other substituted utf-8 -- so the two documentation transports declared different encodings for one stored value, which is the divergence this class exists to remove. Both take the same decision from the same call now, because headerValue rebuilds the header from the parsed parts: one normalized type, the other parameters as they were, exactly one charset. Rebuilding also makes "; charset" and "; charset=" replaceable rather than contradictable, so the malformed forms are no longer emitted at all. With rebuilding, the first *usable* charset became the right one to read rather than simply the first. While this appended, the first mattered, because that is the one a first-wins recipient keeps; now that exactly one is emitted, serving "charset=; charset=iso-8859-1" as utf-8 would garble a page that says plainly what it is. My own test caught that. Also from the review: handleClient's error path called sendError without outputStarted, so a failure while writing the body -- a dropped connection being the common one -- appended a second status line to a response that had already claimed 200. It reports whether the response had started now, as the other call sites in this file already do. The tests move to Truth, which ARCHITECTURE.md requires and this file was not using, and the comment volume comes down: the asymmetry was explained in four separate KDocs and no longer exists to explain. 341 tests pass across :common and :app. Co-Authored-By: Claude Opus 5 --- .../androidide/localWebServer/WebServer.kt | 29 ++- .../androidide/utils/ContentTypeHeaders.kt | 141 +++++++++----- .../utils/ContentTypeHeadersTest.kt | 174 ++++++++---------- 3 files changed, 189 insertions(+), 155 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 659ec53cdc..714219e460 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -481,6 +481,10 @@ class WebServer( } private fun handleClient(clientSocket: Socket) { + // Whether the client has already been told 200. Read by the error path, which must not put a + // second status line on a response that has already started (see the assignment below). + var responseStarted = false + if (debugEnabled) log.debug("In handleClient(), socket is {}.", clientSocket) val input = clientSocket.getInputStream() @@ -626,6 +630,8 @@ class WebServer( while (nextChunk.size == contentChunkSize) { val path2 = "$path-$fragmentNumber" val cursor2 = database.rawQuery(query2, arrayOf(path2)) + // Whether the client has already been told 200; see the assignment below. + var responseStarted = false try { if (cursor2.moveToFirst()) { nextChunk = cursor2.getBlob(0) @@ -659,13 +665,17 @@ class WebServer( } // Built before the status line goes out: everything after the first println is on the - // wire (the writer autoflushes), so a throw past that point makes sendError append a - // second status line to a response that already claimed 200, which a client parses as a - // malformed header rather than as an error. dbMimeType is a platform type from - // Cursor.getString, so a NULL ContentTypes.value throws here rather than there. + // wire (the writer autoflushes), so a throw past that point cannot be answered with a + // fresh error response. dbMimeType is a platform type from Cursor.getString, so a NULL + // ContentTypes.value throws here rather than there. val contentTypeHeader = ContentTypeHeaders.headerValue(dbMimeType) writer.println("HTTP/1.1 200 OK") + // From here on the client has been told 200. A failure while writing the body -- a + // dropped connection is the common one -- must not append a second status line to that + // response; sendError writes nothing at all when told the output has started, which is + // the only honest thing left to do with a half-sent reply. + responseStarted = true writer.println("Content-Type: $contentTypeHeader") writer.println("Content-Length: ${dbContent.size}") writer.println("Connection: close") @@ -674,8 +684,15 @@ class WebServer( output.write(dbContent) output.flush() } catch (e: Exception) { - log.error("Error processing request: {}", e.message) - sendError(writer, output, httpInternalServerError, "Internal Server Error", e.message ?: "") + log.error("Error processing request: {}", e.message, e) + sendError( + writer, + output, + httpInternalServerError, + "Internal Server Error", + e.message ?: "", + outputStarted = responseStarted, + ) } finally { cursor.close() } diff --git a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt index 59ed1f682c..7637520ebb 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt @@ -21,6 +21,10 @@ package com.itsaky.androidide.utils */ object ContentTypeHeaders { private const val UTF_8 = "utf-8" + private const val TEXT_PLAIN = "text/plain" + + // What a value carrying a control character becomes: renders nothing, injects nothing. + private const val OCTET_STREAM = "application/octet-stream" // Textual types outside text/* and the +xml family. application/x-typescript has no rows in the // current database but costs nothing to keep; application/javascript does have rows, and is what @@ -34,43 +38,65 @@ object ContentTypeHeaders { ) /** - * The charset to *append* to [mimeType], or null when there is nothing to add: the type is - * binary, the format defines its own encoding, or a `charset` parameter is already present. - * - * "Already present" includes an unusable one -- `text/html; charset=` gets nothing appended, - * because a recipient keeps that empty parameter and ignores a repeated name, so a second - * charset would conflict with it and change nothing. [typeAndCharset] deliberately answers - * differently for the same input: it returns the charset as its own value, where nothing can - * conflict with it, so it substitutes the default. The asymmetry is the point. + * The charset to send with [mimeType], or null when there is none to send: the type is binary, or + * the format fixes its own encoding. * * `application/json` is deliberately absent: RFC 8259 defines no charset parameter for it and * fixes the encoding as UTF-8, so declaring one is meaningless rather than helpful. XML-based * types are included even though a document may carry its own declaration, because a - * transport-level charset takes precedence and SVG in particular usually omits the declaration. + * transport-level charset takes precedence and SVG in particular usually omits it. */ - fun charsetFor(mimeType: String): String? = if (carriesCharsetParameter(mimeType)) null else defaultCharsetFor(mimeType) + internal fun charsetFor(mimeType: String): String? = typeAndCharset(mimeType).second /** - * The bare media type and the charset to send with it: whatever [mimeType] already declares, - * otherwise this class's default for that type, otherwise null. An empty `charset=` counts as - * declaring nothing, so the default applies -- unlike [charsetFor], which cannot append past one - * without contradicting it. + * The media type and the charset to send with it, which is what + * `WebResourceResponse(type, encoding, stream)` wants and what [headerValue] builds its header + * from. One decision, so the two documentation transports cannot disagree. * - * Exists because `WebResourceResponse(type, encoding, stream)` wants the two apart, and the - * in-process transport was re-implementing the parse to get them -- with the naive substring - * match this file warns against below. One parse, both transports. + * The type is normalized, not passed through: the database stores a bare `text` and a + * `text/text`, neither of which is a media type (`type "/" subtype` is required), and a client + * that cannot parse the type discards the charset with it -- which would have made this whole + * change a no-op on exactly those rows. A value carrying a control character is refused outright; + * see [safeType]. + * + * An unusable `charset=` counts as declaring nothing, so the default applies. [headerValue] + * rebuilds the header rather than appending to the stored string, so that substitution reaches + * both transports instead of only this one. */ - fun typeAndCharset(mimeType: String): Pair { + internal fun typeAndCharset(mimeType: String): Pair { + val type = safeType(mimeType) + return type to (declaredCharset(mimeType) ?: defaultCharsetFor(type)) + } + + /** + * [mimeType]'s media type, normalized and safe to put in a header. + * + * A control character makes the whole value untrustworthy: `ContentTypes.value` comes from a + * database that a debug build will swap in from shared storage (`WebServer`'s + * `debugDatabasePath`), and a stored `text/html\r\n\r\n...` would otherwise be written + * straight into the response by `println`, splitting it into two. Such a value is not repaired, + * it is refused: `application/octet-stream` renders nothing and injects nothing. + */ + private fun safeType(mimeType: String): String { val type = mimeType.substringBefore(';').trim() - return type to (declaredCharset(mimeType) ?: defaultCharsetFor(mimeType)) + if (type.isEmpty() || type.any { it.isISOControl() }) { + return OCTET_STREAM + } + // "text" and "text/text" are the database's own spellings for plain text, and neither parses + // as a media type. + return if (type.equals("text", ignoreCase = true) || type.equals("text/text", ignoreCase = true)) { + TEXT_PLAIN + } else { + type + } } - private fun defaultCharsetFor(mimeType: String): String? { - val type = mimeType.substringBefore(';').trim().lowercase() + private fun defaultCharsetFor(safeType: String): String? { + val type = safeType.lowercase() return when { - // Every text subtype, plus the database's bare "text" oddity. Matched at the boundary: - // "textual/example" is not a text type, and startsWith("text") would say it is. - type == "text" || type.startsWith("text/") -> UTF_8 + // Matched at the boundary: "textual/example" is not a text type, and startsWith("text") + // would say it is. + type.startsWith("text/") -> UTF_8 type.endsWith("+xml") || type == "application/xml" -> UTF_8 @@ -92,31 +118,18 @@ object ContentTypeHeaders { * contains a semicolon (`note="x; charset=utf-8"`). A parameter with no value (`; charset`) or * an empty one (`; charset=`) declares nothing and must not suppress the default -- treating it * as a declaration is how a response ends up with no encoding at all. - */ - private fun declaredCharset(mimeType: String): String? = firstCharsetParameter(mimeType)?.second?.ifEmpty { null } - - /** - * Whether [mimeType] already carries a `charset` parameter *with an `=`*, usable or not -- the - * question [headerValue] has to ask, which is not the same as whether the charset is usable. * - * `; charset=` (empty) and `; charset` (no value at all) are both useless as declarations, but - * recipients treat them differently. A parameter with no `=` is dropped during parsing, so an - * appended `; charset=utf-8` becomes the only one and takes effect. An empty *valued* parameter - * is kept, and a repeated parameter name is ignored, so appending a second one gets us a header - * carrying two conflicting charsets and, in a first-wins recipient, no change in behaviour. Not - * worth emitting: the empty parameter is a defect in the stored `ContentTypes.value` and belongs - * fixed there. [typeAndCharset] can and does still substitute the default, because it hands the - * charset back as its own value where nothing can conflict with it. + * The first *usable* one, not simply the first. While this class appended to the stored string, + * reading the first mattered -- that is the one a recipient keeps when a name repeats, so + * honouring a later one would have meant acting on a parameter the client ignores. [headerValue] + * rebuilds the header now and emits exactly one charset, so that no longer applies, and reading + * past an unusable parameter is what keeps `charset=; charset=iso-8859-1` from being served as + * utf-8 -- which would garble a page that says plainly what it is. */ - private fun carriesCharsetParameter(mimeType: String): Boolean = firstCharsetParameter(mimeType)?.second != null - - /** - * [mimeType]'s first `charset` parameter, or null when it has none. First, not first-usable: - * that is the one a recipient keeps when a name repeats, so reading any other would honour a - * parameter the client ignores. - */ - private fun firstCharsetParameter(mimeType: String): Pair? = - parameters(mimeType).firstOrNull { (name, _) -> name.equals("charset", ignoreCase = true) } + private fun declaredCharset(mimeType: String): String? = + parameters(mimeType) + .firstOrNull { (name, value) -> name.equals("charset", ignoreCase = true) && !value.isNullOrEmpty() } + ?.second /** * [mimeType]'s `name=value` parameters, with semicolons inside quoted values left alone. A null @@ -175,9 +188,37 @@ object ContentTypeHeaders { return found } - /** [mimeType] with a charset appended when [charsetFor] gives one, otherwise unchanged. */ + /** + * The `Content-Type` header value for a row stored as [mimeType]. + * + * Rebuilt from the parsed parts rather than appended to. Appending produced a header no stricter + * than what the database happened to hold: a bare `text` stayed unparseable, an unusable + * `charset=` stayed and contradicted the one added after it, and a valueless `charset` was left + * beside its replacement. Rebuilding emits one normalized type, the other parameters as they + * were, and exactly one charset -- the same one [typeAndCharset] hands the other transport. + */ fun headerValue(mimeType: String): String { - val charset = charsetFor(mimeType) ?: return mimeType - return "$mimeType; charset=$charset" + val (type, charset) = typeAndCharset(mimeType) + return buildString { + append(type) + for ((name, value) in parameters(mimeType)) { + if (name.equals("charset", ignoreCase = true)) continue + append("; ").append(name) + if (value != null) append('=').append(quoteIfNeeded(value)) + } + if (charset != null) append("; charset=").append(charset) + } } + + /** + * Re-quotes a parameter value that needed quoting in the first place. [parameters] strips the + * quotes it parsed, so a value containing a space or a separator has to get them back or the + * rebuilt header means something different from the stored one. + */ + private fun quoteIfNeeded(value: String): String = + if (value.isNotEmpty() && value.none { it.isWhitespace() || it in "\"(),/:;<=>?@[\\]{}" }) { + value + } else { + "\"" + value.replace("\\", "\\\\").replace("\"", "\\\"") + "\"" + } } diff --git a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt index aff0f235c1..031baafeed 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt @@ -1,153 +1,129 @@ package com.itsaky.androidide.utils -import org.junit.Assert.assertEquals -import org.junit.Assert.assertNull +import com.google.common.truth.Truth.assertThat import org.junit.Test -/** ADFA-5241: every text response has to declare its encoding, and no binary response may. */ +/** + * ADFA-5241: text served without a charset renders as mojibake wherever the client's guess goes + * wrong. These cover what is actually sent, which is not always what was stored -- the header is + * rebuilt from the parsed parts so a malformed stored value cannot produce a malformed response. + */ class ContentTypeHeadersTest { @Test fun `text types get utf-8`() { - for (type in listOf("text/html", "text/css", "text/javascript", "text/markdown", "text/plain")) { - assertEquals("$type; charset=utf-8", ContentTypeHeaders.headerValue(type)) + for (type in listOf("text/html", "text/css", "text/javascript", "text/markdown")) { + assertThat(ContentTypeHeaders.headerValue(type)).isEqualTo("$type; charset=utf-8") } } - // documentation.db really contains these two: a bare "text" (which 726 TooltipButtons point at, - // via x.html) and a "text/text". Neither is a valid MIME type, but both are text. + // The database stores a bare "text" (one row, x.html) and a "text/text" (ten licence files). + // Neither is a media type -- type "/" subtype is required -- and a client that cannot parse the + // type discards the charset with it, which would have made this change a no-op on exactly the + // rows it was written for. @Test - fun `the database's malformed text types are still treated as text`() { - assertEquals("text; charset=utf-8", ContentTypeHeaders.headerValue("text")) - assertEquals("text/text; charset=utf-8", ContentTypeHeaders.headerValue("text/text")) + fun `the database's non-media-type spellings become text-plain`() { + assertThat(ContentTypeHeaders.headerValue("text")).isEqualTo("text/plain; charset=utf-8") + assertThat(ContentTypeHeaders.headerValue("text/text")).isEqualTo("text/plain; charset=utf-8") + assertThat(ContentTypeHeaders.typeAndCharset("text")).isEqualTo("text/plain" to "utf-8") } + // ContentTypes.value comes from a database a debug build will swap in from shared storage, and + // the header is written with println(). A stored CR/LF would split the response in two. @Test - fun `xml-based types get utf-8, since svg rarely declares its own`() { - assertEquals("image/svg+xml; charset=utf-8", ContentTypeHeaders.headerValue("image/svg+xml")) - assertEquals("application/xml; charset=utf-8", ContentTypeHeaders.headerValue("application/xml")) + fun `a type carrying a control character is refused, not repaired`() { + val injected = "text/html\r\nContent-Length: 0\r\n\r\nHTTP/1.1 200 OK" + + assertThat(ContentTypeHeaders.headerValue(injected)).isEqualTo("application/octet-stream") + assertThat(ContentTypeHeaders.headerValue("text/html\u0000")).isEqualTo("application/octet-stream") + assertThat(ContentTypeHeaders.headerValue("")).isEqualTo("application/octet-stream") } @Test fun `binary types are left alone`() { - for (type in listOf( - "image/png", - "image/gif", - "image/jpeg", - "image/webp", - "image/x-icon", - "video/mp4", - "video/quicktime", - "application/pdf", - "application/wasm", - "font/woff2", - "font/ttf", - "application/octet-stream", - "application/vnd-iccprofile", - )) { - assertNull(ContentTypeHeaders.charsetFor(type)) - assertEquals(type, ContentTypeHeaders.headerValue(type)) + for (type in listOf("image/png", "image/gif", "application/pdf", "font/woff2", "video/mp4")) { + assertThat(ContentTypeHeaders.charsetFor(type)).isNull() + assertThat(ContentTypeHeaders.headerValue(type)).isEqualTo(type) } } // RFC 8259 defines no charset parameter for JSON and fixes the encoding as UTF-8, so declaring - // one says nothing. Asserted so nobody "fixes" this by adding it. + // one says nothing. @Test fun `json is left alone`() { - assertNull(ContentTypeHeaders.charsetFor("application/json")) - assertEquals("application/json", ContentTypeHeaders.headerValue("application/json")) - } - - @Test - fun `an existing charset is never doubled`() { - assertEquals("text/html; charset=utf-8", ContentTypeHeaders.headerValue("text/html; charset=utf-8")) - assertEquals("text/html; charset=iso-8859-1", ContentTypeHeaders.headerValue("text/html; charset=iso-8859-1")) - assertEquals("text/html; CHARSET=UTF-8", ContentTypeHeaders.headerValue("text/html; CHARSET=UTF-8")) + assertThat(ContentTypeHeaders.charsetFor("application/json")).isNull() + assertThat(ContentTypeHeaders.headerValue("application/json")).isEqualTo("application/json") } - // startsWith("text") would call this a text type. The intent is "text" or "text/", nothing else. - // Splitting on ';' alone still finds "charset=" inside a quoted value that contains a semicolon, - // and then suppresses the declaration the response actually needs. @Test - fun `a semicolon inside a quoted parameter value does not hide the charset`() { - assertEquals( - """text/html; note="x; charset=utf-8"; charset=utf-8""", - ContentTypeHeaders.headerValue("""text/html; note="x; charset=utf-8""""), - ) + fun `xml-based types get utf-8, since svg rarely declares its own`() { + assertThat(ContentTypeHeaders.headerValue("image/svg+xml")).isEqualTo("image/svg+xml; charset=utf-8") + assertThat(ContentTypeHeaders.headerValue("application/xml")).isEqualTo("application/xml; charset=utf-8") } - // A parameter with no value declares nothing and is dropped during parsing, so appending the - // default works and is what the response needs. @Test - fun `a valueless charset parameter is not a declaration`() { - assertEquals("text/html; charset; charset=utf-8", ContentTypeHeaders.headerValue("text/html; charset")) + fun `an existing charset is kept, never doubled`() { + assertThat(ContentTypeHeaders.headerValue("text/html; charset=iso-8859-1")) + .isEqualTo("text/html; charset=iso-8859-1") + assertThat(ContentTypeHeaders.headerValue("text/html; CHARSET=UTF-8")).isEqualTo("text/html; charset=UTF-8") } - // An empty *valued* parameter is kept by recipients, and a repeated name is ignored, so a second - // charset would conflict with it and change nothing. Emitting one claims a fix it does not make; - // the empty parameter is a defect in the stored value and belongs fixed there. + // An unusable charset is replaced rather than contradicted. Appending left the empty one in + // place, where a first-wins recipient keeps it and ignores what follows -- so the response still + // had no usable encoding, which is the bug this class exists to remove. @Test - fun `an empty charset parameter is left alone rather than contradicted`() { - assertEquals("text/html; charset=", ContentTypeHeaders.headerValue("text/html; charset=")) - assertEquals( - "text/html; charset=; charset=iso-8859-1", - ContentTypeHeaders.headerValue("text/html; charset=; charset=iso-8859-1"), - ) + fun `an unusable charset is replaced`() { + assertThat(ContentTypeHeaders.headerValue("text/html; charset=")).isEqualTo("text/html; charset=utf-8") + assertThat(ContentTypeHeaders.headerValue("text/html; charset")).isEqualTo("text/html; charset=utf-8") + assertThat(ContentTypeHeaders.headerValue("text/html; charset=; charset=iso-8859-1")) + .isEqualTo("text/html; charset=iso-8859-1") } - // RFC 9110 quoted-pair. Toggling on every quote ends the value at the escaped one, and then the - // charset inside note parses as a declaration -- the same false match this class exists to stop. + // Both transports take the same decision from the same call, so a WebView and the socket server + // cannot declare different encodings for one stored value. @Test - fun `an escaped quote does not end a quoted parameter value`() { - assertEquals( - """text/html; note="a\"; charset=iso-8859-1"; charset=utf-8""", - ContentTypeHeaders.headerValue("""text/html; note="a\"; charset=iso-8859-1""""), - ) - assertEquals( - "text/html" to "utf-8", - ContentTypeHeaders.typeAndCharset("""text/html; note="a\"; charset=iso-8859-1""""), - ) + fun `both transports agree on every form of unusable charset`() { + for (stored in listOf("text/html; charset=", "text/html; charset", "text", "text/html")) { + val (type, charset) = ContentTypeHeaders.typeAndCharset(stored) + assertThat(ContentTypeHeaders.headerValue(stored)).isEqualTo("$type; charset=$charset") + } } - // Textual application/* types have no syntactic marker in common. application/javascript is the - // one with rows in the database and is what ".mjs" resolves to. + // Substring-matching "charset=" would find it inside another parameter's value and suppress the + // declaration this response actually needs. The quotes have to survive the rebuild. @Test - fun `textual application types get utf-8 and json still does not`() { - assertEquals("application/javascript; charset=utf-8", ContentTypeHeaders.headerValue("application/javascript")) - assertEquals("application/ecmascript; charset=utf-8", ContentTypeHeaders.headerValue("application/ecmascript")) - assertNull(ContentTypeHeaders.charsetFor("application/json")) + fun `charset inside another parameter's value does not count as a declaration`() { + assertThat(ContentTypeHeaders.headerValue("""text/html; note="charset=utf-8"""")) + .isEqualTo("""text/html; note="charset=utf-8"; charset=utf-8""") + assertThat(ContentTypeHeaders.headerValue("""text/html; note="x; charset=utf-8"""")) + .isEqualTo("""text/html; note="x; charset=utf-8"; charset=utf-8""") } - // The other transport needs the two apart for WebResourceResponse, and must not re-parse. + // RFC 9110 quoted-pair: an escaped quote does not end the value, so the charset inside note is + // not a declaration either. @Test - fun `typeAndCharset splits the type from the charset it should declare`() { - assertEquals("text/html" to "utf-8", ContentTypeHeaders.typeAndCharset("text/html")) - assertEquals("image/png" to null, ContentTypeHeaders.typeAndCharset("image/png")) - assertEquals("text/html" to "iso-8859-1", ContentTypeHeaders.typeAndCharset("text/html; charset=iso-8859-1")) - assertEquals("text/html" to "UTF-8", ContentTypeHeaders.typeAndCharset("text/html; CHARSET=UTF-8")) - // a declared-but-empty parameter falls back to the default rather than to nothing - assertEquals("text/html" to "utf-8", ContentTypeHeaders.typeAndCharset("text/html; charset=")) + fun `an escaped quote does not end a quoted parameter value`() { + assertThat(ContentTypeHeaders.typeAndCharset("""text/html; note="a\"; charset=iso-8859-1"""")) + .isEqualTo("text/html" to "utf-8") } @Test fun `a type that merely begins with text is not a text type`() { - assertNull(ContentTypeHeaders.charsetFor("textual/example")) - assertEquals("textual/example", ContentTypeHeaders.headerValue("textual/example")) + assertThat(ContentTypeHeaders.charsetFor("textual/example")).isNull() + assertThat(ContentTypeHeaders.headerValue("textual/example")).isEqualTo("textual/example") } - // Substring-matching "charset=" would find it inside another parameter's value and suppress the - // declaration this response actually needs. @Test - fun `charset inside another parameter's value does not count as a declaration`() { - assertEquals( - """text/html; note="charset=utf-8"; charset=utf-8""", - ContentTypeHeaders.headerValue("""text/html; note="charset=utf-8""""), - ) + fun `casing and stray whitespace on the type are tolerated`() { + assertThat(ContentTypeHeaders.headerValue("TEXT/HTML")).isEqualTo("TEXT/HTML; charset=utf-8") + assertThat(ContentTypeHeaders.headerValue("text/html ")).isEqualTo("text/html; charset=utf-8") + assertThat(ContentTypeHeaders.headerValue("text/html;boundary=x")).isEqualTo("text/html; boundary=x; charset=utf-8") } @Test - fun `parameters and casing on the type itself are tolerated`() { - assertEquals("TEXT/HTML; charset=utf-8", ContentTypeHeaders.headerValue("TEXT/HTML")) - assertEquals("text/html ; charset=utf-8", ContentTypeHeaders.headerValue("text/html ")) - assertEquals("text/html;boundary=x; charset=utf-8", ContentTypeHeaders.headerValue("text/html;boundary=x")) + fun `typeAndCharset splits the type from the charset it should declare`() { + assertThat(ContentTypeHeaders.typeAndCharset("text/html")).isEqualTo("text/html" to "utf-8") + assertThat(ContentTypeHeaders.typeAndCharset("image/png")).isEqualTo("image/png" to null) + assertThat(ContentTypeHeaders.typeAndCharset("text/html; charset=iso-8859-1")) + .isEqualTo("text/html" to "iso-8859-1") } } From d126354ddd9351b66482faad45cf847db268f9bd Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 17:02:33 -0700 Subject: [PATCH 7/8] ADFA-5241: Refuse a control character anywhere in the value, not just the type The sanitising check covered only the segment before the first ';', so text/html; note=xX-Injected: y passed it -- and the parameter loop then wrote that CR/LF into the response header. Same response splitting the check was added to stop, one segment further along, in the fix for it. The whole stored value is checked now, and a refused value emits nothing but application/octet-stream: its parameters are exactly where the control characters would have been. Test: 'a control character in a parameter is refused too', which fails against the type-only check. Co-Authored-By: Claude Opus 5 --- .../itsaky/androidide/utils/ContentTypeHeaders.kt | 14 +++++++++++++- .../androidide/utils/ContentTypeHeadersTest.kt | 10 ++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt index 7637520ebb..75df918767 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt @@ -78,8 +78,15 @@ object ContentTypeHeaders { * it is refused: `application/octet-stream` renders nothing and injects nothing. */ private fun safeType(mimeType: String): String { + // The *whole* stored value, not just the segment before the first ';'. A control character in + // a parameter -- text/html; note=xX-Injected: y -- would otherwise pass this check and + // then be written into the header by the parameter loop in headerValue, which is the same + // response splitting, one segment further along. + if (mimeType.any { it.isISOControl() }) { + return OCTET_STREAM + } val type = mimeType.substringBefore(';').trim() - if (type.isEmpty() || type.any { it.isISOControl() }) { + if (type.isEmpty()) { return OCTET_STREAM } // "text" and "text/text" are the database's own spellings for plain text, and neither parses @@ -199,6 +206,11 @@ object ContentTypeHeaders { */ fun headerValue(mimeType: String): String { val (type, charset) = typeAndCharset(mimeType) + // Nothing from a refused value is re-emitted: its parameters are exactly where the control + // characters would have been. + if (type == OCTET_STREAM && charset == null) { + return OCTET_STREAM + } return buildString { append(type) for ((name, value) in parameters(mimeType)) { diff --git a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt index 031baafeed..678aeea1d7 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ContentTypeHeadersTest.kt @@ -38,6 +38,16 @@ class ContentTypeHeadersTest { assertThat(ContentTypeHeaders.headerValue("")).isEqualTo("application/octet-stream") } + // The type segment being clean is not enough: a parameter is just as much a part of the header + // line, so a CR/LF there splits the response exactly the same way. + @Test + fun `a control character in a parameter is refused too`() { + val injected = "text/html; note=x\r\nX-Injected: y" + + assertThat(ContentTypeHeaders.headerValue(injected)).isEqualTo("application/octet-stream") + assertThat(ContentTypeHeaders.typeAndCharset(injected)).isEqualTo("application/octet-stream" to null) + } + @Test fun `binary types are left alone`() { for (type in listOf("image/png", "image/gif", "application/pdf", "font/woff2", "video/mp4")) { From 6eff54973a9adae1cbc4eae830a00a41f86831ac Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 26 Aug 2026 15:21:48 -0700 Subject: [PATCH 8/8] ADFA-5241: Close the response-splitting hole the charset parameter left open safeType refused a value carrying a control character, but the refusal was read back out of its return value -- OCTET_STREAM with no charset -- and a control character inside the charset parameter defeated that: declaredCharset re-parsed the original string, found a charset, so "refused" was never true, and headerValue appended the CRLF-bearing value verbatim. WebServer writes the result with println, so text/html; charset=x split the reply into two HTTP responses with an attacker-chosen body -- the one thing this class exists to prevent. The two existing "refused, not repaired" tests put their control character in the type segment or a note= parameter, so neither could see it. Refusal is asked now, not inferred: one isUntrustworthy() consulted by both typeAndCharset and headerValue. That also stops a legitimately stored application/octet-stream; name=file.bin from being mistaken for a refusal and losing its parameters. The charset is the one value that skipped quoteIfNeeded, so a stored charset="utf-8; x=y" -- whose quotes parameters() strips -- came back out as a charset plus a smuggled second parameter. It goes through the same quoting as every other parameter value now. Four tests, three of which fail against the previous logic. 84 common tests pass. Found in review of PR #1725. --- .../androidide/utils/ContentTypeHeaders.kt | 31 ++++++++++++-- .../utils/ContentTypeHeadersTest.kt | 41 +++++++++++++++++++ 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt index 75df918767..298ebe4761 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ContentTypeHeaders.kt @@ -64,10 +64,28 @@ object ContentTypeHeaders { * both transports instead of only this one. */ internal fun typeAndCharset(mimeType: String): Pair { + // Refusal has to be asked, not inferred. This used to read the answer out of safeType's return + // value -- OCTET_STREAM with no charset -- and a control character inside the charset parameter + // defeated it: safeType refused the type, declaredCharset re-parsed the *original* string and + // found a charset, so "refused" was never true and the CRLF went into the header verbatim. + // text/html; charset=x" + + val header = ContentTypeHeaders.headerValue(split) + + assertThat(header).isEqualTo("application/octet-stream") + assertThat(header).doesNotContain("\r") + assertThat(header).doesNotContain("\n") + assertThat(ContentTypeHeaders.typeAndCharset(split).second).isNull() + } + + @Test + fun `a control character in a parameter name refuses the whole value`() { + val header = ContentTypeHeaders.headerValue("text/html; x\r\nX-Injected: y=1") + + assertThat(header).isEqualTo("application/octet-stream") + } + + // parameters() strips the quotes, so appending the value raw turned one charset into a charset + // plus a second parameter the client would honour. + @Test + fun `a quoted charset cannot smuggle a second parameter`() { + val header = ContentTypeHeaders.headerValue("""text/html; charset="utf-8; x=y"""") + + // Quoted, so the recipient reads one charset whose value happens to contain a semicolon -- + // not a charset plus a second parameter. Appended raw it was the latter. + assertThat(header).isEqualTo("""text/html; charset="utf-8; x=y"""") + } + + // Refusal used to be read off the returned string, so a genuine octet-stream looked refused. + @Test + fun `a stored octet-stream keeps its own parameters`() { + val header = ContentTypeHeaders.headerValue("application/octet-stream; name=file.bin") + + assertThat(header).contains("name=file.bin") + } }