From b28cbe0600e3c3292a5793e42752850b7e80e560 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 6 Oct 2026 23:02:07 -0400 Subject: [PATCH] feat(android): refuse plain http:// to a public server address (#5111) Cleartext stays permitted app-wide for LAN servers and UPnP (#2439), but a password or session cookie sent over plain HTTP to a public address can be read by anyone on the path. A network interceptor now refuses a cleartext request to the Minstrel server when the connection lands on a public address, before any request byte is written. Checked per connection, on the address actually reached, rather than when the URL is typed: a name that resolved to the home network at entry resolves to a public address once the phone leaves home. Allowed: loopback, 10/8, 172.16/12, 192.168/16, link-local, 100.64/10 (Tailscale and other overlay VPNs) and fc00::/7. Only requests BaseUrlInterceptor tagged as server-bound are checked; external fetches and UPnP are untouched. The refusal has its own message. Family baseline #5105, practice 13. Co-Authored-By: Claude Opus 5.5 --- .../minstrel/api/BaseUrlInterceptor.kt | 9 +- .../minstrel/api/CleartextGuard.kt | 84 +++++++++++++++++ .../com/fabledsword/minstrel/api/ErrorCopy.kt | 4 + .../fabledsword/minstrel/api/NetworkModule.kt | 3 + .../main/res/xml/network_security_config.xml | 8 +- .../minstrel/api/CleartextGuardTest.kt | 89 +++++++++++++++++++ 6 files changed, 194 insertions(+), 3 deletions(-) create mode 100644 android/app/src/main/java/com/fabledsword/minstrel/api/CleartextGuard.kt create mode 100644 android/app/src/test/java/com/fabledsword/minstrel/api/CleartextGuardTest.kt diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/BaseUrlInterceptor.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/BaseUrlInterceptor.kt index 8bc5fee1..42d38e51 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/api/BaseUrlInterceptor.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/BaseUrlInterceptor.kt @@ -50,7 +50,14 @@ class BaseUrlInterceptor @Inject constructor( .port(baseUrl.port) .build() } ?: original.url - return chain.proceed(original.newBuilder().url(rewritten).build()) + return chain.proceed( + original.newBuilder() + .url(rewritten) + // Lets CleartextGuardInterceptor tell server requests from + // external fetches once the placeholder host is gone. + .tag(MinstrelServerRequest::class.java, MinstrelServerRequest) + .build(), + ) } companion object { diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/CleartextGuard.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/CleartextGuard.kt new file mode 100644 index 00000000..e60857c2 --- /dev/null +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/CleartextGuard.kt @@ -0,0 +1,84 @@ +package com.fabledsword.minstrel.api + +import okhttp3.Interceptor +import okhttp3.Response +import java.io.IOException +import java.net.Inet4Address +import java.net.Inet6Address +import java.net.InetAddress + +/** + * Marks a request as bound for the Minstrel server, set by + * [BaseUrlInterceptor] when it retargets the placeholder host. Those are the + * requests that carry the session cookie and the password. + */ +object MinstrelServerRequest + +/** + * Plain `http://` to the Minstrel server is allowed only when the connection + * actually lands on a private address (family security baseline #5105, + * practice 13). + * + * Cleartext stays permitted app-wide for LAN servers and UPnP + * (network_security_config.xml, #2439), but a password or session cookie sent + * over plain HTTP to a public address can be read by anyone on the path. + * + * Checked per connection, on the address the socket really reached, not on + * the URL when it was typed: a name that resolved to the home network when it + * was entered resolves to a public address once the phone leaves home, and + * that is exactly when the password would go out in the clear. A network + * interceptor runs after the connection is made and before any request byte + * is written, so nothing is sent. + */ +class CleartextGuardInterceptor( + // The policy is a parameter so a test can refuse loopback, the only + // address a test server can listen on. + private val allows: (InetAddress) -> Boolean = CleartextPolicy::allows, +) : Interceptor { + override fun intercept(chain: Interceptor.Chain): Response { + val request = chain.request() + if (request.isHttps || request.tag(MinstrelServerRequest::class.java) == null) { + return chain.proceed(request) + } + val address = chain.connection()?.route()?.socketAddress?.address + if (address != null && !allows(address)) { + throw CleartextToPublicHostException(request.url.host) + } + return chain.proceed(request) + } +} + +/** The server was reached over plain HTTP at a public address, and refused. */ +class CleartextToPublicHostException(host: String) : + IOException("refusing plain http:// to $host: it is a public address") + +/** Which addresses plain HTTP may reach: the home network, never the internet. */ +object CleartextPolicy { + private const val CGNAT_FIRST_OCTET = 100 + private const val CGNAT_SECOND_MASK = 0xC0 + private const val CGNAT_SECOND_PREFIX = 64 + private const val ULA_MASK = 0xFE + private const val ULA_PREFIX = 0xFC + private const val BYTE = 0xFF + + fun allows(address: InetAddress): Boolean = + address.isLoopbackAddress || + address.isSiteLocalAddress || // 10/8, 172.16/12, 192.168/16 + address.isLinkLocalAddress || // 169.254/16, fe80::/10 + address.isAnyLocalAddress || + isCarrierGradeNat(address) || + isUniqueLocal(address) + + // 100.64/10. Tailscale and other overlay VPNs hand these out; the overlay + // encrypts the traffic itself. + private fun isCarrierGradeNat(address: InetAddress): Boolean { + if (address !is Inet4Address) return false + val b = address.address + return (b[0].toInt() and BYTE) == CGNAT_FIRST_OCTET && + (b[1].toInt() and CGNAT_SECOND_MASK) == CGNAT_SECOND_PREFIX + } + + // fc00::/7, IPv6's private range. + private fun isUniqueLocal(address: InetAddress): Boolean = + address is Inet6Address && (address.address[0].toInt() and ULA_MASK) == ULA_PREFIX +} diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt index 9a7314ad..360ba82b 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt @@ -38,6 +38,7 @@ object ErrorCopy { */ fun fromThrowable(t: Throwable): String = when (t) { is HttpException -> fromHttp(t) + is CleartextToPublicHostException -> messageFor("cleartext_public") is IOException -> messageFor("connection_refused") else -> TABLE.getValue("unknown") } @@ -101,6 +102,9 @@ object ErrorCopy { "mbid_required" to "An MBID is required for this lookup.", "system_playlist_readonly" to "System playlists can't be edited directly.", "connection_refused" to "Couldn't reach the server. Check the URL and try again.", + "cleartext_public" to + "This server is on the internet, so its URL must start with https://. " + + "Plain http:// only works on your home network.", "lidarr_unreachable" to "Lidarr is unreachable right now. Try again, or check Admin → Integrations.", "lidarr_disabled" to "Lidarr integration is not enabled.", diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/NetworkModule.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/NetworkModule.kt index 91233f84..475b2842 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/api/NetworkModule.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/NetworkModule.kt @@ -70,6 +70,9 @@ object NetworkModule { .addInterceptor(auth) .addInterceptor(baseUrl) .addInterceptor(logging) + // A network interceptor, so it sees the address the connection + // really reached and runs before any request byte is written. + .addNetworkInterceptor(CleartextGuardInterceptor()) .connectTimeout(CONNECT_TIMEOUT_SECONDS, TimeUnit.SECONDS) .readTimeout(READ_TIMEOUT_SECONDS, TimeUnit.SECONDS) .build() diff --git a/android/app/src/main/res/xml/network_security_config.xml b/android/app/src/main/res/xml/network_security_config.xml index 1fb8f930..11f3b9db 100644 --- a/android/app/src/main/res/xml/network_security_config.xml +++ b/android/app/src/main/res/xml/network_security_config.xml @@ -17,8 +17,12 @@ hostnames rather than CIDR ranges, and both sets of hosts above are unknowable until runtime. So a permissive base-config is an honest description of our situation — the gain over the manifest attribute is that the reasoning now - lives somewhere, and there is one place to tighten if a future settings screen - can distinguish a LAN server from a WAN one. + lives somewhere. + + The LAN/WAN line this file cannot draw is drawn in code instead: + api/CleartextGuard.kt refuses plain http:// to the Minstrel server whenever + the connection lands on a public address (family baseline #5105, practice + 13). LAN servers and UPnP speakers are unaffected. Worth stating because it looks worse than it is: this is NOT a tamper risk for the in-app updater. An APK altered in transit and re-signed is rejected by the diff --git a/android/app/src/test/java/com/fabledsword/minstrel/api/CleartextGuardTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/api/CleartextGuardTest.kt new file mode 100644 index 00000000..77f78910 --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/api/CleartextGuardTest.kt @@ -0,0 +1,89 @@ +package com.fabledsword.minstrel.api + +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertThrows +import java.net.InetAddress +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** Plain http:// to the Minstrel server only on a private address (#5105, practice 13). */ +class CleartextGuardTest { + private lateinit var server: MockWebServer + + @BeforeEach + fun setup() { + server = MockWebServer().apply { start() } + } + + @AfterEach + fun teardown() { + server.shutdown() + } + + private fun ip(s: String) = InetAddress.getByName(s) + + @Test + fun `home network and overlay addresses may use plain http`() { + for (a in listOf( + "127.0.0.1", "10.1.2.3", "172.16.0.9", "172.31.255.1", "192.168.1.20", + "169.254.3.4", "100.64.0.1", "100.127.255.254", "::1", "fd12:3456::1", "fe80::1", + )) { + assertTrue(CleartextPolicy.allows(ip(a)), "$a should be allowed") + } + } + + @Test + fun `public addresses may not`() { + for (a in listOf( + "8.8.8.8", "172.32.0.1", "100.128.0.1", "100.63.255.255", "203.0.113.5", + "2001:db8::1", "2606:4700::1111", + )) { + assertFalse(CleartextPolicy.allows(ip(a)), "$a should be refused") + } + } + + private fun client(allows: Boolean) = OkHttpClient.Builder() + .addNetworkInterceptor(CleartextGuardInterceptor { allows }) + .build() + + private fun serverRequest() = Request.Builder() + .url(server.url("/api/auth/login")) + .tag(MinstrelServerRequest::class.java, MinstrelServerRequest) + .build() + + @Test + fun `a refused server request sends nothing`() { + server.enqueue(MockResponse().setResponseCode(200)) + assertThrows { + client(allows = false).newCall(serverRequest()).execute() + } + assertEquals(0, server.requestCount, "the request reached the server") + } + + @Test + fun `an allowed server request goes through`() { + server.enqueue(MockResponse().setResponseCode(200)) + client(allows = true).newCall(serverRequest()).execute().use { assertEquals(200, it.code) } + assertEquals(1, server.requestCount) + } + + @Test + fun `requests not bound for the Minstrel server are left alone`() { + server.enqueue(MockResponse().setResponseCode(200)) + client(allows = false).newCall(Request.Builder().url(server.url("/art.jpg")).build()) + .execute().use { assertEquals(200, it.code) } + } + + @Test + fun `the refusal has its own message`() { + val msg = ErrorCopy.fromThrowable(CleartextToPublicHostException("music.example.com")) + assertTrue(msg.contains("https://"), msg) + } +}