diff --git a/README.md b/README.md index 4b05286..1856e8d 100644 --- a/README.md +++ b/README.md @@ -514,7 +514,21 @@ Supported image sources: | ------------- | ---------------------- | ----------------------------------------- | | Bundled asset | `require('./pin.png')` | Resolved on JS side before crossing Nitro | | Local URI | `{ uri: 'file:///…' }` | Platform file paths | -| Remote URL | `{ uri: 'https://…' }` | Async fetch with in-memory cache | +| Remote URL | `{ uri: 'https://…' }` | Async fetch with in-memory cache; Android restricts the host, see below | + +### Remote image hosts on Android + +A remote URL you pass yourself — `image={{ uri: someUrl }}` — is checked before Android fetches +it. The URL must use `http`/`https`, must not carry `user:password@`, and its host must not +resolve to a private address: loopback, link-local, site-local, multicast, IPv6 unique-local, a +`.local`/`.localhost` name, or a cloud metadata endpoint. The address the connection is actually +made to is checked again before the request is sent, every redirect target is checked the same +way, and a redirect from `https` to `http` is not followed. This guards the common case of a +marker icon URL arriving as data from an API. A rejected URL falls back to the default pin and +logs `Rejected remote marker image URI` under the `NitroMaps` tag. + +`require()`d assets are exempt, including the packager URL Metro resolves them to in a +development build. iOS does not apply this policy today. Additional props: diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt index 837072f..e6a7e25 100644 --- a/package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt @@ -11,13 +11,6 @@ import android.util.LruCache import com.google.android.gms.maps.model.BitmapDescriptor import com.google.android.gms.maps.model.BitmapDescriptorFactory import com.google.android.gms.maps.model.Marker -import java.net.Inet6Address -import java.net.InetAddress -import java.net.URI -import java.net.URISyntaxException -import java.net.URL -import java.net.UnknownHostException -import java.util.Locale import java.util.WeakHashMap import java.util.concurrent.ExecutorService import java.util.concurrent.Executors @@ -167,7 +160,9 @@ internal class MarkerIconFactory( private fun defaultMarkerDisplaySizePx(): Pair = (DEFAULT_MARKER_WIDTH_DP * density) to (DEFAULT_MARKER_HEIGHT_DP * density) private fun cacheKey(image: MarkerImage): String { - return "${image.uri}|${image.width ?: ""}|${image.height ?: ""}|${image.scale ?: ""}" + // `origin` is part of the key so a bundled image cannot warm the cache for a + // user-supplied one with the same URI, which would hand it a policy-free entry. + return "${image.uri}|${image.width ?: ""}|${image.height ?: ""}|${image.scale ?: ""}|${image.origin ?: ""}" } private fun loadLocalIcon( @@ -188,8 +183,8 @@ internal class MarkerIconFactory( return } - if (isRemoteMarkerUri(image.uri)) { - remoteMarkerUriRejectReason(image.uri, resolveHostAddress = false)?.let { reason -> + if (RemoteMarkerUriPolicy.isRemoteUri(image.uri)) { + RemoteMarkerUriPolicy.rejectReason(image, resolveHostAddress = false)?.let { reason -> logRejectedRemoteMarkerUri(image.uri, reason) deliverOnMainThread { onLoaded(null) } return @@ -198,7 +193,7 @@ internal class MarkerIconFactory( loadExecutor.execute { val descriptor = - if (isRemoteMarkerUri(image.uri)) { + if (RemoteMarkerUriPolicy.isRemoteUri(image.uri)) { loadRemoteIcon(image, key) } else { loadLocalIcon(image, key) @@ -248,19 +243,10 @@ internal class MarkerIconFactory( image: MarkerImage, key: String, ): BitmapDescriptor? { - remoteMarkerUriRejectReason(image.uri, resolveHostAddress = true)?.let { reason -> - logRejectedRemoteMarkerUri(image.uri, reason) - return null - } - return try { - val connection = URL(image.uri).openConnection() - connection.connectTimeout = 10_000 - connection.readTimeout = 10_000 - connection.getInputStream().use { stream -> - val bitmap = decodeByteArray(stream.readBytes(), image) ?: return null - cacheBitmap(key, resizeBitmap(bitmap, image)) - } + val bytes = remoteImageFetcher.fetch(image, ::logRejectedRemoteMarkerUri) ?: return null + val bitmap = decodeByteArray(bytes, image) ?: return null + cacheBitmap(key, resizeBitmap(bitmap, image)) } catch (error: Exception) { Log.w(NITRO_MAPS_LOG_TAG, "Failed to load marker image: ${image.uri}", error) null @@ -279,7 +265,7 @@ internal class MarkerIconFactory( return decodeFile(uri, image) } - if (isRemoteMarkerUri(uri)) { + if (RemoteMarkerUriPolicy.isRemoteUri(uri)) { return null } @@ -408,101 +394,6 @@ internal class MarkerIconFactory( return descriptor } - private fun isRemoteMarkerUri(uriString: String): Boolean { - val scheme = parseRemoteMarkerUri(uriString)?.scheme?.lowercase(Locale.US) ?: return false - return scheme == "http" || scheme == "https" - } - - private fun parseRemoteMarkerUri(uriString: String): URI? = - try { - URI(uriString) - } catch (_: URISyntaxException) { - null - } - - private fun remoteMarkerUriRejectReason( - uriString: String, - resolveHostAddress: Boolean, - ): String? { - val uri = parseRemoteMarkerUri(uriString) ?: return "invalid URI" - - when (uri.scheme?.lowercase(Locale.US)) { - "http", "https" -> Unit - null -> return "missing scheme" - else -> return "unsupported scheme" - } - - if (uri.userInfo != null) { - return "user info not allowed" - } - - val host = uri.host?.lowercase(Locale.US)?.takeIf { it.isNotEmpty() } ?: return "missing host" - if (!isAllowlistedRemoteHost(host, resolveHostAddress)) { - return "host not allowlisted" - } - - return null - } - - private fun isAllowlistedRemoteHost( - host: String, - resolveHostAddress: Boolean, - ): Boolean { - if (host == "localhost" || host.endsWith(".localhost") || host.endsWith(".local")) { - return false - } - if (host in BLOCKED_REMOTE_HOSTS) { - return false - } - - if (!resolveHostAddress && !isNumericHostLiteral(host)) { - return true - } - - val address = - try { - InetAddress.getByName(host) - } catch (_: UnknownHostException) { - return !resolveHostAddress - } - - return isAllowlistedRemoteAddress(address) - } - - private fun isNumericHostLiteral(host: String): Boolean { - if (host.startsWith("[") && host.endsWith("]")) { - return true - } - - val parts = host.split('.') - return parts.size == 4 && - parts.all { part -> - val value = part.toIntOrNull() ?: return@all false - value in 0..255 - } - } - - private fun isAllowlistedRemoteAddress(address: InetAddress): Boolean { - if ( - address.isLoopbackAddress || - address.isAnyLocalAddress || - address.isLinkLocalAddress || - address.isSiteLocalAddress || - address.isMulticastAddress - ) { - return false - } - - if (address is Inet6Address) { - val firstOctet = address.address[0].toInt() and 0xff - if ((firstOctet and 0xfe) == 0xfc) { - return false - } - } - - return true - } - private fun logRejectedRemoteMarkerUri( uri: String, reason: String, @@ -545,10 +436,8 @@ internal class MarkerIconFactory( private val loadExecutor: ExecutorService = Executors.newSingleThreadExecutor() - private val BLOCKED_REMOTE_HOSTS = - setOf( - "metadata.google.internal", - "metadata.goog", - ) + // Shared by every map, and first built on the load executor: an HTTP client loads the system + // trust store when it is created. + private val remoteImageFetcher by lazy { RemoteMarkerImageFetcher() } } } diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcher.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcher.kt new file mode 100644 index 0000000..cf29a97 --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcher.kt @@ -0,0 +1,139 @@ +package com.margelo.nitro.nitromaps + +import okhttp3.Interceptor +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.Response +import java.io.IOException +import java.net.MalformedURLException +import java.net.Proxy +import java.net.URL +import java.util.concurrent.TimeUnit + +/** + * Downloads a remote marker image and follows its redirects itself, so that every hop of a + * user-supplied URL passes [RemoteMarkerUriPolicy]. An automatic redirect would reach a private + * host without asking the policy. + * + * Each hop is checked twice. First the URL, before the request: that gives a reason to log, and + * in the common case no connection is made at all. Then the address the connection was actually + * made to, before anything is sent over it ([ConnectedAddressCheck]). The client resolves the host + * again to connect, and a DNS answer that changed in between (rebinding) would otherwise send the + * request to a private address the first check never saw. + * + * @param baseClient the client the fetching clients are derived from; a seam for tests. + * @param hostRejectReason the policy check on one hop's URL; a seam for tests. + */ +internal class RemoteMarkerImageFetcher( + baseClient: OkHttpClient = OkHttpClient(), + private val hostRejectReason: (uri: String) -> String? = { uri -> + RemoteMarkerUriPolicy.rejectReason(uri, resolveHostAddress = true) + }, +) { + /** Fetches a `require()`d image, whose packager URL is private in a development build. */ + private val bundledClient = + baseClient + .newBuilder() + .followRedirects(false) + .followSslRedirects(false) + .connectTimeout(TIMEOUT_MS, TimeUnit.MILLISECONDS) + .readTimeout(TIMEOUT_MS, TimeUnit.MILLISECONDS) + .build() + + /** Fetches a user-supplied image, and only over a connection the policy allows. */ + private val userClient = bundledClient.newBuilder().addNetworkInterceptor(ConnectedAddressCheck).build() + + /** + * Returns the body at [image]'s URL, or `null` after telling [onRejected] which URI may not be + * fetched and why. A failed request throws. + */ + fun fetch( + image: MarkerImage, + onRejected: (uri: String, reason: String) -> Unit, + ): ByteArray? { + // Like the policy itself, a bundled image skips the host checks, on every hop. + val checksHost = image.origin != MarkerImageOrigin.BUNDLED + val client = if (checksHost) userClient else bundledClient + var url = URL(image.uri) + + repeat(MAX_REDIRECTS + 1) { + if (checksHost) { + // The URL about to be requested, as it will be requested, rather than the string it came from. + hostRejectReason(url.toString())?.let { reason -> + onRejected(url.toString(), reason) + return null + } + } + + val response = + try { + client.newCall(Request.Builder().url(url).build()).execute() + } catch (rejected: RejectedConnectionException) { + onRejected(url.toString(), rejected.reason) + return null + } + + response.use { + if (!response.isRedirect) { + if (!response.isSuccessful) { + throw IOException("HTTP ${response.code} for $url") + } + return response.body?.bytes() + } + + val target = response.header("Location")?.let { resolveLocation(url, it) } + if (target == null) { + onRejected(url.toString(), "redirect without a usable location") + return null + } + RemoteMarkerUriPolicy.redirectRejectReason(url, target)?.let { reason -> + onRejected(target.toString(), reason) + return null + } + url = target + } + } + + onRejected(image.uri, "more than $MAX_REDIRECTS redirects") + return null + } + + private fun resolveLocation( + from: URL, + location: String, + ): URL? = + try { + // Unlike `URI.resolve`, keeps the slash when `from` has an empty path. + URL(from, location) + } catch (_: MalformedURLException) { + null + } + + /** + * Closes a direct connection to an address [RemoteMarkerUriPolicy] rejects before the request is + * written to it. Behind a proxy, the proxy resolves the host, so only the check on the URL applies. + */ + private object ConnectedAddressCheck : Interceptor { + override fun intercept(chain: Interceptor.Chain): Response { + val connection = chain.connection() + if (connection != null && connection.route().proxy.type() == Proxy.Type.DIRECT) { + val address = connection.route().socketAddress.address + if (address == null || !RemoteMarkerUriPolicy.isAllowlistedAddress(address)) { + connection.socket().close() + throw RejectedConnectionException("connected address not allowlisted") + } + } + return chain.proceed(chain.request()) + } + } + + /** A connection the policy refused before anything was sent over it. */ + private class RejectedConnectionException( + val reason: String, + ) : IOException(reason) + + companion object { + const val MAX_REDIRECTS = 5 + private const val TIMEOUT_MS = 10_000L + } +} diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicy.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicy.kt new file mode 100644 index 0000000..9678f67 --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicy.kt @@ -0,0 +1,170 @@ +package com.margelo.nitro.nitromaps + +import java.net.Inet6Address +import java.net.InetAddress +import java.net.URI +import java.net.URISyntaxException +import java.net.URL +import java.net.UnknownHostException +import java.util.Locale + +/** + * Decides whether a marker image may be fetched over the network. + * + * The policy exists for URLs that arrive as *data* — `image={{ uri: apiResponse.iconUrl }}` + * pointing at a router admin page or a cloud metadata endpoint — so it only applies to images the + * app did not bundle. A `require()`d asset carries [MarkerImageOrigin.BUNDLED] from the JS side and + * skips the check: a development build resolves it to a Metro packager URL, whose address is always + * private (`10.0.2.2` on the emulator, `127.0.0.1` over `adb reverse`, `192.168.x.x` over Wi-Fi), so + * the host cannot tell a bundled asset apart from the addresses this policy blocks. + */ +internal object RemoteMarkerUriPolicy { + /** Whether [uriString] is fetched over the network rather than read from the app package. */ + fun isRemoteUri(uriString: String): Boolean { + val scheme = parseUri(uriString)?.scheme?.lowercase(Locale.US) ?: return false + return scheme == "http" || scheme == "https" + } + + /** + * Returns why [image] must not be fetched, or `null` when the fetch is allowed. + * + * @param resolveHostAddress whether a host name may be resolved through DNS. `false` for the + * pre-check on the main thread, `true` once the caller is on the load executor, so only a + * numeric host literal is classified before the thread hop. + */ + fun rejectReason( + image: MarkerImage, + resolveHostAddress: Boolean, + ): String? { + if (image.origin == MarkerImageOrigin.BUNDLED) { + return null + } + + return rejectReason(image.uri, resolveHostAddress) + } + + /** + * The same check for a URI that carries no bundled origin, such as where a user-supplied + * image redirects to. + * + * @param resolveHost looks up every address of a host name; a seam for tests. + */ + fun rejectReason( + uriString: String, + resolveHostAddress: Boolean, + resolveHost: (String) -> Array = { host -> InetAddress.getAllByName(host) }, + ): String? { + val uri = parseUri(uriString) ?: return "invalid URI" + + when (uri.scheme?.lowercase(Locale.US)) { + "http", "https" -> Unit + null -> return "missing scheme" + else -> return "unsupported scheme" + } + + if (uri.userInfo != null) { + return "user info not allowed" + } + + val host = uri.host?.lowercase(Locale.US)?.takeIf { it.isNotEmpty() } ?: return "missing host" + if (!isAllowlistedHost(host, resolveHostAddress, resolveHost)) { + return "host not allowlisted" + } + + return null + } + + /** + * Returns why a redirect from [from] to [target] must not be followed, or `null` when it may + * be. This holds for bundled images too: a redirect stays on `http`/`https`, and never goes from + * `https` down to `http`, which the platform does not follow either. The host of a user-supplied + * image's target still has to pass [rejectReason], like any other URI. + */ + fun redirectRejectReason( + from: URL, + target: URL, + ): String? { + val scheme = target.protocol.lowercase(Locale.US) + return when { + scheme != "http" && scheme != "https" -> "redirect to an unsupported scheme" + scheme == "http" && from.protocol.equals("https", ignoreCase = true) -> "redirect from https to http" + else -> null + } + } + + private fun parseUri(uriString: String): URI? = + try { + URI(uriString) + } catch (_: URISyntaxException) { + null + } + + private fun isAllowlistedHost( + host: String, + resolveHostAddress: Boolean, + resolveHost: (String) -> Array, + ): Boolean { + if (host == "localhost" || host.endsWith(".localhost") || host.endsWith(".local")) { + return false + } + if (host in BLOCKED_HOSTS) { + return false + } + + if (!resolveHostAddress && !isNumericHostLiteral(host)) { + return true + } + + val addresses = + try { + resolveHost(host) + } catch (_: UnknownHostException) { + return !resolveHostAddress + } + + // Every address, not just the first: the connection moves on to the next one when the first + // does not answer, so a public address listed ahead of a private one proves nothing. + return addresses.isNotEmpty() && addresses.all(::isAllowlistedAddress) + } + + private fun isNumericHostLiteral(host: String): Boolean { + if (host.startsWith("[") && host.endsWith("]")) { + return true + } + + val parts = host.split('.') + return parts.size == 4 && + parts.all { part -> + val value = part.toIntOrNull() ?: return@all false + value in 0..255 + } + } + + /** Whether a connection to [address] may carry a request for a user-supplied image. */ + fun isAllowlistedAddress(address: InetAddress): Boolean { + if ( + address.isLoopbackAddress || + address.isAnyLocalAddress || + address.isLinkLocalAddress || + address.isSiteLocalAddress || + address.isMulticastAddress + ) { + return false + } + + if (address is Inet6Address) { + val firstOctet = address.address[0].toInt() and 0xff + if ((firstOctet and 0xfe) == 0xfc) { + return false + } + } + + return true + } + + private val BLOCKED_HOSTS = + setOf( + "metadata.google.internal", + "metadata.goog", + ) +} diff --git a/package/android/src/test/java/com/margelo/nitro/nitromaps/MarkerDisplayedIdentityTest.kt b/package/android/src/test/java/com/margelo/nitro/nitromaps/MarkerDisplayedIdentityTest.kt index 3aad949..ab12822 100644 --- a/package/android/src/test/java/com/margelo/nitro/nitromaps/MarkerDisplayedIdentityTest.kt +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/MarkerDisplayedIdentityTest.kt @@ -11,8 +11,8 @@ class MarkerDisplayedIdentityTest { val pairs = listOf( "image" to ( - marker(image = MarkerImage("asset:/pin.png", 32.0, 32.0, 2.0)) to - marker(image = MarkerImage("asset:/pin-alt.png", 32.0, 32.0, 2.0)) + marker(image = MarkerImage("asset:/pin.png", 32.0, 32.0, 2.0, MarkerImageOrigin.BUNDLED)) to + marker(image = MarkerImage("asset:/pin-alt.png", 32.0, 32.0, 2.0, MarkerImageOrigin.BUNDLED)) ), "rotation" to (marker(rotation = 0.0) to marker(rotation = 45.0)), "opacity" to (marker(opacity = 1.0) to marker(opacity = 0.4)), diff --git a/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcherTest.kt b/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcherTest.kt new file mode 100644 index 0000000..2dbb6d9 --- /dev/null +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcherTest.kt @@ -0,0 +1,307 @@ +package com.margelo.nitro.nitromaps + +import okhttp3.Dns +import okhttp3.OkHttpClient +import org.junit.After +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows +import org.junit.Test +import java.io.IOException +import java.net.InetAddress +import java.net.InetSocketAddress +import java.net.Proxy +import java.net.ServerSocket +import java.net.Socket +import java.net.SocketAddress +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.CopyOnWriteArrayList +import javax.net.SocketFactory +import kotlin.concurrent.thread + +/** + * Runs the fetcher against a local HTTP server. Every connection lands on that server whatever + * address the client connects to, so no host name below reaches the network, while the client + * still believes it is connected to the address its DNS answered. + */ +class RemoteMarkerImageFetcherTest { + private val server = FakeHttpServer() + private val port = server.port + private val rejections = mutableListOf>() + + /** What the policy's lookup, before each request, answers. */ + private val checkedZone = + mapOf( + "cdn.example.com" to PUBLIC_ADDRESS, + "img.example.com" to PUBLIC_ADDRESS, + "rebind.example.com" to PUBLIC_ADDRESS, + ) + + /** What the client's own lookup, when it connects, answers. */ + private val connectedZone = + mapOf( + "cdn.example.com" to PUBLIC_ADDRESS, + "img.example.com" to PUBLIC_ADDRESS, + // Rebinding: public when the policy looked, private once the client connects. + "rebind.example.com" to PRIVATE_ADDRESS, + ) + + private val fetcher = + RemoteMarkerImageFetcher( + baseClient = + OkHttpClient + .Builder() + .dns( + object : Dns { + override fun lookup(hostname: String) = lookUp(connectedZone, hostname) + }, + ).socketFactory(server.socketFactory) + .proxy(Proxy.NO_PROXY) + .build(), + hostRejectReason = { uri -> + RemoteMarkerUriPolicy.rejectReason(uri, resolveHostAddress = true) { host -> + lookUp(checkedZone, host).toTypedArray() + } + }, + ) + + @After + fun closeServer() { + server.close() + } + + @Test + fun fetchesAPublicImage() { + server.respond("cdn.example.com:$port/pin.png", ok("pin")) + + assertArrayEquals("pin".toByteArray(), fetch("http://cdn.example.com:$port/pin.png")) + assertEquals(listOf("cdn.example.com:$port/pin.png"), server.requests) + assertEquals(emptyList>(), rejections) + } + + @Test + fun sendsNothingToAPrivateAddressAHostRebindsToAfterTheCheck() { + server.respond("rebind.example.com:$port/pin.png", ok("pin")) + + assertNull(fetch("http://rebind.example.com:$port/pin.png")) + assertEquals( + listOf("http://rebind.example.com:$port/pin.png" to "connected address not allowlisted"), + rejections, + ) + // The client did connect, but closed the connection without writing the request to it. + server.awaitConnections(1) + assertEquals(emptyList(), server.requests) + } + + @Test + fun refusesARedirectFromAPublicHostToAPrivateOne() { + server.respond("cdn.example.com:$port/pin.png", redirect("http://192.168.1.1:$port/admin/pin.png")) + + assertNull(fetch("http://cdn.example.com:$port/pin.png")) + assertEquals(listOf("http://192.168.1.1:$port/admin/pin.png" to "host not allowlisted"), rejections) + assertEquals(listOf("cdn.example.com:$port/pin.png"), server.requests) + } + + @Test + fun refusesARedirectToAHostThatRebindsToAPrivateAddress() { + server.respond("cdn.example.com:$port/pin.png", redirect("http://rebind.example.com:$port/pin.png")) + server.respond("rebind.example.com:$port/pin.png", ok("pin")) + + assertNull(fetch("http://cdn.example.com:$port/pin.png")) + assertEquals( + listOf("http://rebind.example.com:$port/pin.png" to "connected address not allowlisted"), + rejections, + ) + server.awaitConnections(2) + assertEquals(listOf("cdn.example.com:$port/pin.png"), server.requests) + } + + @Test + fun followsRedirectsBetweenPublicHosts() { + server.respond("cdn.example.com:$port/pin.png", redirect("http://img.example.com:$port/pin.png", status = 301)) + server.respond("img.example.com:$port/pin.png", ok("pin")) + + assertArrayEquals("pin".toByteArray(), fetch("http://cdn.example.com:$port/pin.png")) + assertEquals(listOf("cdn.example.com:$port/pin.png", "img.example.com:$port/pin.png"), server.requests) + } + + @Test + fun resolvesARelativeLocationAgainstTheRedirectingUrl() { + server.respond("cdn.example.com:$port/v1/pin.png", redirect("../v2/pin.png")) + server.respond("cdn.example.com:$port/v2/pin.png", ok("pin")) + + assertArrayEquals("pin".toByteArray(), fetch("http://cdn.example.com:$port/v1/pin.png")) + } + + @Test + fun refusesARedirectWithoutALocation() { + server.respond("cdn.example.com:$port/pin.png", redirect(location = null)) + + assertNull(fetch("http://cdn.example.com:$port/pin.png")) + assertEquals( + listOf("http://cdn.example.com:$port/pin.png" to "redirect without a usable location"), + rejections, + ) + } + + @Test + fun givesUpAfterTooManyRedirects() { + val hops = (0..RemoteMarkerImageFetcher.MAX_REDIRECTS + 1).map { "/$it.png" } + hops.zipWithNext().forEach { (from, to) -> server.respond("cdn.example.com:$port$from", redirect(to)) } + + assertNull(fetch("http://cdn.example.com:$port${hops.first()}")) + assertEquals(RemoteMarkerImageFetcher.MAX_REDIRECTS + 1, server.requests.size) + assertEquals( + listOf("http://cdn.example.com:$port${hops.first()}" to "more than ${RemoteMarkerImageFetcher.MAX_REDIRECTS} redirects"), + rejections, + ) + } + + @Test + fun throwsForAnHttpError() { + assertThrows(IOException::class.java) { fetch("http://cdn.example.com:$port/missing.png") } + } + + @Test + fun fetchesABundledImageFromThePackagerWithoutEitherCheck() { + server.respond("10.0.2.2:$port/assets/pin.png", ok("pin")) + + assertArrayEquals("pin".toByteArray(), fetch("http://10.0.2.2:$port/assets/pin.png", MarkerImageOrigin.BUNDLED)) + assertEquals(emptyList>(), rejections) + } + + @Test + fun keepsTheRedirectOfABundledImageOnHttp() { + server.respond("10.0.2.2:$port/assets/pin.png", redirect("file:///sdcard/pin.png")) + + assertNull(fetch("http://10.0.2.2:$port/assets/pin.png", MarkerImageOrigin.BUNDLED)) + assertEquals(listOf("file:/sdcard/pin.png" to "redirect to an unsupported scheme"), rejections) + } + + private fun fetch( + uri: String, + origin: MarkerImageOrigin? = null, + ): ByteArray? = + fetcher.fetch( + MarkerImage(uri = uri, width = null, height = null, scale = null, origin = origin), + ) { rejectedUri, reason -> rejections += rejectedUri to reason } + + /** Answers from [zone] for a host name, and parses an IP literal as a real lookup does. */ + private fun lookUp( + zone: Map, + host: String, + ): List { + zone[host]?.let { return listOf(it) } + check(host.all { it.isDigit() || it == '.' }) { "no fake DNS answer for $host" } + return InetAddress.getAllByName(host).toList() + } + + private fun ok(body: String) = "HTTP/1.1 200 OK\r\nContent-Length: ${body.length}\r\nConnection: close\r\n\r\n$body" + + private fun redirect( + location: String?, + status: Int = 302, + ) = "HTTP/1.1 $status Redirect\r\n${location?.let { "Location: $it\r\n" } ?: ""}Content-Length: 0\r\nConnection: close\r\n\r\n" + + /** + * Answers each request with the response registered for its `Host` and path, and records the + * requests it receives. A connection that carries no request records nothing. + */ + private class FakeHttpServer : AutoCloseable { + private val serverSocket = ServerSocket(0, 50, InetAddress.getLoopbackAddress()) + private val responses = ConcurrentHashMap() + private val handlers = CopyOnWriteArrayList() + val requests = CopyOnWriteArrayList() + val port = serverSocket.localPort + + /** Connects every socket to this server, whatever address it is asked to connect to. */ + val socketFactory = + object : SocketFactory() { + override fun createSocket(): Socket = + object : Socket() { + override fun connect( + endpoint: SocketAddress?, + timeout: Int, + ) = super.connect(InetSocketAddress(InetAddress.getLoopbackAddress(), this@FakeHttpServer.port), timeout) + } + + override fun createSocket( + host: String?, + port: Int, + ): Socket = throw UnsupportedOperationException() + + override fun createSocket( + host: String?, + port: Int, + localHost: InetAddress?, + localPort: Int, + ): Socket = throw UnsupportedOperationException() + + override fun createSocket( + host: InetAddress?, + port: Int, + ): Socket = throw UnsupportedOperationException() + + override fun createSocket( + address: InetAddress?, + port: Int, + localAddress: InetAddress?, + localPort: Int, + ): Socket = throw UnsupportedOperationException() + } + + private val acceptor = + thread(isDaemon = true) { + while (true) { + val socket = + try { + serverSocket.accept() + } catch (_: IOException) { + break + } + handlers += thread(isDaemon = true) { socket.use(::answer) } + } + } + + fun respond( + hostAndPath: String, + response: String, + ) { + responses[hostAndPath] = response + } + + /** Waits until [count] connections were accepted and each was answered or dropped. */ + fun awaitConnections(count: Int) { + val deadline = System.nanoTime() + 5_000_000_000L + while (handlers.size < count && System.nanoTime() < deadline) { + Thread.sleep(10) + } + handlers.forEach { it.join(5_000) } + } + + override fun close() { + serverSocket.close() + acceptor.join(5_000) + } + + private fun answer(socket: Socket) { + val reader = socket.getInputStream().bufferedReader(Charsets.ISO_8859_1) + val requestLine = reader.readLine() ?: return + val headers = generateSequence { reader.readLine()?.takeIf { it.isNotEmpty() } }.toList() + val host = headers.first { it.startsWith("Host:", ignoreCase = true) }.substringAfter(':').trim() + val target = host + requestLine.split(' ')[1] + requests += target + val response = responses[target] ?: "HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + socket.getOutputStream().apply { + write(response.toByteArray(Charsets.ISO_8859_1)) + flush() + } + } + } + + private companion object { + val PUBLIC_ADDRESS: InetAddress = InetAddress.getByAddress(byteArrayOf(93, 184.toByte(), 216.toByte(), 34)) + val PRIVATE_ADDRESS: InetAddress = InetAddress.getByAddress(byteArrayOf(10, 0, 0, 1)) + } +} diff --git a/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicyTest.kt b/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicyTest.kt new file mode 100644 index 0000000..4762eb8 --- /dev/null +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicyTest.kt @@ -0,0 +1,145 @@ +package com.margelo.nitro.nitromaps + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import java.net.InetAddress +import java.net.URL + +class RemoteMarkerUriPolicyTest { + @Test + fun treatsOnlyHttpUrisAsRemote() { + assertTrue(RemoteMarkerUriPolicy.isRemoteUri("http://example.com/pin.png")) + assertTrue(RemoteMarkerUriPolicy.isRemoteUri("HTTPS://example.com/pin.png")) + assertFalse(RemoteMarkerUriPolicy.isRemoteUri("file:///data/pin.png")) + assertFalse(RemoteMarkerUriPolicy.isRemoteUri("pin")) + assertFalse(RemoteMarkerUriPolicy.isRemoteUri("asset:/pin.png")) + } + + @Test + fun rejectsPrivateAddressesTheDeveloperMachineCouldBeReachedAt() { + // Every Android development setup lands on one of these: the emulator host alias, + // a device over `adb reverse`, and a device on the same Wi-Fi. + assertEquals( + "host not allowlisted", + rejectReason("http://10.0.2.2:8095/assets/pin.png"), + ) + assertEquals( + "host not allowlisted", + rejectReason("http://127.0.0.1:8081/assets/pin.png"), + ) + assertEquals( + "host not allowlisted", + rejectReason("http://192.168.1.14:8081/assets/pin.png"), + ) + } + + @Test + fun rejectsLinkLocalAndMetadataEndpoints() { + assertEquals( + "host not allowlisted", + rejectReason("http://169.254.169.254/latest/meta-data/"), + ) + assertEquals( + "host not allowlisted", + rejectReason("http://metadata.google.internal/computeMetadata/v1/"), + ) + assertEquals("host not allowlisted", rejectReason("http://localhost/pin.png")) + assertEquals("host not allowlisted", rejectReason("http://printer.local/pin.png")) + } + + @Test + fun rejectsMalformedAndCredentialBearingUris() { + assertEquals("unsupported scheme", rejectReason("ftp://example.com/pin.png")) + assertEquals("missing scheme", rejectReason("//example.com/pin.png")) + assertEquals("missing host", rejectReason("http:///pin.png")) + assertEquals( + "user info not allowed", + rejectReason("https://user:secret@example.com/pin.png"), + ) + } + + @Test + fun allowsAPublicHost() { + assertNull(rejectReason("https://cdn.example.com/pin.png")) + } + + @Test + fun skipsTheHostPolicyForABundledImage() { + // The packager URL a development build resolves `require('./pin.png')` to; the origin + // stamp is the only thing keeping it out of the rejections above. + assertNull( + rejectReason( + "http://10.0.2.2:8095/assets/?unstable_path=./assets/pin.png", + origin = MarkerImageOrigin.BUNDLED, + ), + ) + } + + @Test + fun appliesTheHostPolicyToAnExplicitlyRemoteImage() { + assertEquals( + "host not allowlisted", + rejectReason("http://10.0.2.2:8095/pin.png", origin = MarkerImageOrigin.REMOTE), + ) + } + + @Test + fun checksEveryAddressAHostNameResolvesTo() { + val publicAddress = InetAddress.getByAddress(byteArrayOf(93, 184.toByte(), 216.toByte(), 34)) + val privateAddress = InetAddress.getByAddress(byteArrayOf(10, 0, 0, 1)) + + fun reasonFor(vararg addresses: InetAddress) = + RemoteMarkerUriPolicy.rejectReason( + "https://cdn.example.com/pin.png", + resolveHostAddress = true, + resolveHost = { arrayOf(*addresses) }, + ) + + assertNull(reasonFor(publicAddress)) + // The connection would move on to the private address if the public one did not answer. + assertEquals("host not allowlisted", reasonFor(publicAddress, privateAddress)) + assertEquals("host not allowlisted", reasonFor()) + } + + @Test + fun followsARedirectOnlyOnHttpAndNeverDownToIt() { + fun reasonFor( + from: String, + to: String, + ) = RemoteMarkerUriPolicy.redirectRejectReason(URL(from), URL(to)) + + assertNull(reasonFor("http://cdn.example.com/pin.png", "https://cdn.example.com/pin.png")) + assertNull(reasonFor("https://cdn.example.com/pin.png", "https://img.example.com/pin.png")) + assertEquals( + "redirect from https to http", + reasonFor("https://cdn.example.com/pin.png", "http://cdn.example.com/pin.png"), + ) + assertEquals( + "redirect to an unsupported scheme", + reasonFor("http://cdn.example.com/pin.png", "file:///sdcard/pin.png"), + ) + } + + /** + * Runs the main-thread pre-check, which classifies a numeric host literal without a DNS + * lookup — so every assertion here stays off the network. + */ + private fun rejectReason( + uri: String, + origin: MarkerImageOrigin? = null, + ): String? = + RemoteMarkerUriPolicy.rejectReason( + image = + MarkerImage( + uri = uri, + width = null, + height = null, + scale = null, + origin = origin, + ), + resolveHostAddress = false, + ) +} diff --git a/package/src/native/specs/overlays.ts b/package/src/native/specs/overlays.ts index 2ab949c..93015e0 100644 --- a/package/src/native/specs/overlays.ts +++ b/package/src/native/specs/overlays.ts @@ -1,10 +1,20 @@ import type { Coordinate } from '../../types/coordinate'; +/** + * Where a marker image came from. Stamped by `resolveMarkerImage`, never read from user + * input: a development build resolves a `require()`d asset to a Metro packager URL, whose + * host is indistinguishable from the private addresses the Android policy blocks. + */ +export type MarkerImageOrigin = 'bundled' | 'remote'; + export interface MarkerImage { uri: string; width?: number; height?: number; scale?: number; + + /** Absent means user-supplied — the Android remote host policy applies. */ + origin?: MarkerImageOrigin; } export type MarkerImageSource = number | MarkerImage; diff --git a/package/src/overlays/__tests__/descriptorEquality.test.ts b/package/src/overlays/__tests__/descriptorEquality.test.ts index 936d38a..21b68a5 100644 --- a/package/src/overlays/__tests__/descriptorEquality.test.ts +++ b/package/src/overlays/__tests__/descriptorEquality.test.ts @@ -119,6 +119,13 @@ describeFieldCoverage( 'image.scale', (d) => ({ ...d, image: { ...d.image, uri: 'asset:/pin.png', scale: 3 } }), ], + [ + 'image.origin', + (d) => ({ + ...d, + image: { ...d.image, uri: 'asset:/pin.png', origin: 'bundled' as const }, + }), + ], ['a cleared image', (d) => ({ ...d, image: undefined })], ['markerColor', (d) => ({ ...d, markerColor: '#007AFF' })], ['a cleared markerColor', (d) => ({ ...d, markerColor: undefined })], diff --git a/package/src/overlays/__tests__/normalizeMarkerDescriptors.test.ts b/package/src/overlays/__tests__/normalizeMarkerDescriptors.test.ts index 1befb30..2760c8d 100644 --- a/package/src/overlays/__tests__/normalizeMarkerDescriptors.test.ts +++ b/package/src/overlays/__tests__/normalizeMarkerDescriptors.test.ts @@ -135,6 +135,7 @@ describe('normalizeMarkerDescriptors', () => { width: 32, height: 32, scale: 2, + origin: 'bundled', }); }); diff --git a/package/src/overlays/__tests__/nullOptionalFields.test.ts b/package/src/overlays/__tests__/nullOptionalFields.test.ts index 12777c0..43da2a6 100644 --- a/package/src/overlays/__tests__/nullOptionalFields.test.ts +++ b/package/src/overlays/__tests__/nullOptionalFields.test.ts @@ -161,6 +161,8 @@ const imageFields = { width: 32, height: 48, scale: 2, + // The library stamps it, so it is dropped from app input whatever its value. + origin: 'remote', } satisfies EveryOptionalField; const animation = { @@ -400,10 +402,14 @@ describe('complete descriptors', () => { }); test('markers keep every field, as a child or in the bulk prop', () => { + // All but a claimed image `origin`, which only the library may stamp. + const { origin: _claimedOrigin, ...imageWithoutOrigin } = + markerFields.image; const expected: NativeMarkerDescriptor = { id: 'pin', coordinate: WARSAW, ...markerFields, + image: imageWithoutOrigin, enteringAnimation: { kind: 'fade', ...animationFields }, }; diff --git a/package/src/overlays/__tests__/resolveMarkerImage.test.ts b/package/src/overlays/__tests__/resolveMarkerImage.test.ts index 08a4427..2eef54b 100644 --- a/package/src/overlays/__tests__/resolveMarkerImage.test.ts +++ b/package/src/overlays/__tests__/resolveMarkerImage.test.ts @@ -1,4 +1,5 @@ -import { beforeEach, describe, expect, mock, test } from 'bun:test'; +import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from 'bun:test'; +import type { MarkerImage } from '../../native/specs/overlays'; import { isMarkerImage, markerImageFromResolvedAsset, @@ -7,8 +8,9 @@ import { const resolveAssetSourceMock = mock( (source: number | { uri: string; width?: number; height?: number; scale?: number }) => { if (typeof source === 'number') { + // A development build: Metro serves the asset from the packager, not as a drawable. return { - uri: `asset:/require-${source}.png`, + uri: `http://10.0.2.2:8081/assets/require-${source}.png`, width: 32, height: 32, scale: 2, @@ -27,6 +29,16 @@ const { clearResolvedMarkerImageCacheForTests, resolveMarkerImage } = await impo '../resolveMarkerImage' ); +const warnSpy = spyOn(console, 'warn'); + +beforeEach(() => { + (globalThis as { __DEV__?: boolean }).__DEV__ = true; +}); + +afterEach(() => { + warnSpy.mockClear(); +}); + describe('markerImageFromResolvedAsset', () => { test('returns undefined for missing asset', () => { expect(markerImageFromResolvedAsset(undefined)).toBeUndefined(); @@ -34,7 +46,7 @@ describe('markerImageFromResolvedAsset', () => { expect(markerImageFromResolvedAsset({ uri: '' })).toBeUndefined(); }); - test('maps resolved asset records to MarkerImage', () => { + test('maps resolved asset records to MarkerImage stamped as bundled', () => { expect( markerImageFromResolvedAsset({ uri: 'asset:/pin.png', @@ -47,6 +59,7 @@ describe('markerImageFromResolvedAsset', () => { width: 32, height: 32, scale: 2, + origin: 'bundled', }); }); @@ -62,6 +75,7 @@ describe('markerImageFromResolvedAsset', () => { expect(markerImageFromResolvedAsset(resolved as never)).toEqual({ uri: 'asset:/pin.png', scale: 2, + origin: 'bundled', }); expect(isMarkerImage(resolved)).toBe(true); }); @@ -98,10 +112,11 @@ describe('resolveMarkerImage', () => { test('resolves require() module ids through resolveAssetSource', () => { expect(resolveMarkerImage(99)).toEqual({ - uri: 'asset:/require-99.png', + uri: 'http://10.0.2.2:8081/assets/require-99.png', width: 32, height: 32, scale: 2, + origin: 'bundled', }); expect(resolveAssetSourceMock).toHaveBeenCalledTimes(1); expect(resolveAssetSourceMock).toHaveBeenCalledWith(99); @@ -109,25 +124,60 @@ describe('resolveMarkerImage', () => { test('passes through uri-only sources as MarkerImage without resolveAssetSource', () => { const source = { uri: 'https://example.com/pin.png' }; + const resolved = resolveMarkerImage(source); - expect(resolveMarkerImage(source)).toEqual({ + expect(resolved).toEqual({ uri: 'https://example.com/pin.png', }); + expect(resolved?.origin).toBeUndefined(); expect(resolveAssetSourceMock).not.toHaveBeenCalled(); }); - test('passes through MarkerImage objects unchanged', () => { + test('drops an origin claimed by a user-supplied image', () => { + // `image={apiResponse.icon}` must not stamp itself bundled and skip the host policy. + const source = { + uri: 'http://192.168.1.1/admin/pin.png', + origin: 'bundled', + } as const; + + const resolved = resolveMarkerImage(source); + + expect(resolved).toEqual({ uri: 'http://192.168.1.1/admin/pin.png' }); + expect(resolved?.origin).toBeUndefined(); + expect(warnSpy).toHaveBeenCalledTimes(1); + + // A re-render hits the cache, so the warning does not repeat. + resolveMarkerImage({ ...source }); + expect(warnSpy).toHaveBeenCalledTimes(1); + }); + + test('passes through MarkerImage objects as an equal copy', () => { const image = { uri: 'asset:/pin.png', width: 32, height: 32, scale: 2, }; + const resolved = resolveMarkerImage(image); - expect(resolveMarkerImage(image)).toBe(image); + expect(resolved).toEqual(image); + expect(resolved).not.toBe(image); expect(resolveAssetSourceMock).not.toHaveBeenCalled(); }); + test('keeps no reference to the caller object in the cache', () => { + // Otherwise stamping the caller's object bundled after the first render would reach + // every later cache hit, and Android would skip the host policy for it. + const source: MarkerImage = { uri: 'http://192.168.1.1/admin/pin.png' }; + const resolved = resolveMarkerImage(source); + + source.origin = 'bundled'; + const rerendered = resolveMarkerImage({ uri: 'http://192.168.1.1/admin/pin.png' }); + + expect(rerendered).toBe(resolved); + expect(rerendered?.origin).toBeUndefined(); + }); + test('caches require() resolutions by module id', () => { const first = resolveMarkerImage(5); const second = resolveMarkerImage(5); diff --git a/package/src/overlays/descriptorEquality.ts b/package/src/overlays/descriptorEquality.ts index c518735..a31ccf1 100644 --- a/package/src/overlays/descriptorEquality.ts +++ b/package/src/overlays/descriptorEquality.ts @@ -101,7 +101,8 @@ function markerImagesEqual( left.uri === right.uri && left.width === right.width && left.height === right.height && - left.scale === right.scale + left.scale === right.scale && + left.origin === right.origin ); } diff --git a/package/src/overlays/markerImageFromResolvedAsset.ts b/package/src/overlays/markerImageFromResolvedAsset.ts index 74430f7..131ba32 100644 --- a/package/src/overlays/markerImageFromResolvedAsset.ts +++ b/package/src/overlays/markerImageFromResolvedAsset.ts @@ -1,6 +1,11 @@ import type { MarkerImage } from '../native/specs/overlays'; -function copyMarkerImage(image: MarkerImage): MarkerImage { +/** + * A new `MarkerImage` holding only the fields the native side reads, with a `null` field + * left out, since Nitro throws on one. `origin` is never copied: the library stamps + * provenance, it does not read it from the input. + */ +export function copyMarkerImage(image: MarkerImage): MarkerImage { return { uri: image.uri, width: image.width ?? undefined, @@ -9,6 +14,10 @@ function copyMarkerImage(image: MarkerImage): MarkerImage { }; } +/** + * Converts `Image.resolveAssetSource` output into a `MarkerImage`. Only a `require()`d + * asset reaches this function, so the result carries `origin: 'bundled'`. + */ export function markerImageFromResolvedAsset( resolved: MarkerImage | null | undefined, ): MarkerImage | undefined { @@ -16,15 +25,7 @@ export function markerImageFromResolvedAsset( return undefined; } - return copyMarkerImage(resolved); -} - -export function markerImageWithoutNulls(image: MarkerImage): MarkerImage { - if (image.width === null || image.height === null || image.scale === null) { - return copyMarkerImage(image); - } - - return image; + return { ...copyMarkerImage(resolved), origin: 'bundled' }; } export function isMarkerImage(value: unknown): value is MarkerImage { diff --git a/package/src/overlays/resolveMarkerImage.ts b/package/src/overlays/resolveMarkerImage.ts index 050c297..37f8303 100644 --- a/package/src/overlays/resolveMarkerImage.ts +++ b/package/src/overlays/resolveMarkerImage.ts @@ -2,10 +2,11 @@ import type { MarkerImage, MarkerImageSource } from '../native/specs/overlays'; import { resolveAssetSource } from './assetSourceResolver'; import { LruCache } from './lruCache'; import { + copyMarkerImage, isMarkerImage, markerImageFromResolvedAsset, - markerImageWithoutNulls, } from './markerImageFromResolvedAsset'; +import { warnOverlay } from './warnOverlay'; const MARKER_IMAGE_CACHE_SIZE = 64; const resolvedImageCache = new LruCache(MARKER_IMAGE_CACHE_SIZE); @@ -27,7 +28,16 @@ export function resolveMarkerImage( if (cached != null) { return cached; } - const image = markerImageWithoutNulls(source); + if (source.origin != null) { + // On the cache miss only, so a re-rendering marker list does not repeat it. + warnOverlay( + `marker image "${source.uri}": "origin" is set by the library and was ignored`, + ); + } + // Always a copy: only the library stamps `origin`, so an image whose fields come from an + // API response cannot claim `'bundled'` and skip the Android host policy — not even by + // setting `origin` on its object after the cache took it. + const image = copyMarkerImage(source); resolvedImageCache.set(key, image); return image; }