From 5a5b95d47defb88740f20278afdab87e2a8b5ba7 Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Tue, 22 Sep 2026 17:24:05 +0200 Subject: [PATCH 1/4] fix(android): render require() marker images on development builds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Image.resolveAssetSource` returns a Metro packager URL in development, so `MarkerIconFactory` — which classified a marker image by its host — rejected every bundled asset: a packager 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). The marker silently fell back to the default pin and only a logcat line said why, while the README recommends `require()` as the primary source. Carry the provenance across Nitro instead of inferring it from the host. `MarkerImage` gains an optional `origin`, `resolveMarkerImage` stamps `'bundled'` on the `require()` branch, and the host policy applies only when the stamp is absent — so the threat it guards against, a URL arriving as data, is still checked. An `origin` on a user-supplied image is stripped and warned about in dev, so an image built from an API response cannot claim to be bundled, and the same field is part of the native icon cache key so a bundled entry cannot be reused for a user-supplied URI. Extract the ~90-line policy out of the 541-line `MarkerIconFactory` into `RemoteMarkerUriPolicy`, which makes it unit-testable, and document the policy in the README next to the image source table. Closes #129 --- README.md | 14 +- .../nitro/nitromaps/MarkerIconFactory.kt | 121 ++-------------- .../nitro/nitromaps/RemoteMarkerUriPolicy.kt | 133 ++++++++++++++++++ .../nitromaps/MarkerDisplayedIdentityTest.kt | 4 +- .../nitromaps/RemoteMarkerUriPolicyTest.kt | 106 ++++++++++++++ package/src/native/specs/overlays.ts | 10 ++ .../__tests__/descriptorEquality.test.ts | 7 + .../normalizeMarkerDescriptors.test.ts | 1 + .../__tests__/resolveMarkerImage.test.ts | 43 +++++- package/src/overlays/descriptorEquality.ts | 3 +- .../overlays/markerImageFromResolvedAsset.ts | 5 + package/src/overlays/resolveMarkerImage.ts | 23 ++- 12 files changed, 345 insertions(+), 125 deletions(-) create mode 100644 package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicy.kt create mode 100644 package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicyTest.kt diff --git a/README.md b/README.md index 5a7a89d0..dd0f1c3b 100644 --- a/README.md +++ b/README.md @@ -369,7 +369,19 @@ 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. 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 884fc6d5..d8524192 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,7 @@ 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 @@ -163,7 +157,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( @@ -184,8 +180,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 @@ -194,7 +190,7 @@ internal class MarkerIconFactory( loadExecutor.execute { val descriptor = - if (isRemoteMarkerUri(image.uri)) { + if (RemoteMarkerUriPolicy.isRemoteUri(image.uri)) { loadRemoteIcon(image, key) } else { loadLocalIcon(image, key) @@ -244,7 +240,7 @@ internal class MarkerIconFactory( image: MarkerImage, key: String, ): BitmapDescriptor? { - remoteMarkerUriRejectReason(image.uri, resolveHostAddress = true)?.let { reason -> + RemoteMarkerUriPolicy.rejectReason(image, resolveHostAddress = true)?.let { reason -> logRejectedRemoteMarkerUri(image.uri, reason) return null } @@ -275,7 +271,7 @@ internal class MarkerIconFactory( return decodeFile(uri, image) } - if (isRemoteMarkerUri(uri)) { + if (RemoteMarkerUriPolicy.isRemoteUri(uri)) { return null } @@ -404,101 +400,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, @@ -530,11 +431,5 @@ internal class MarkerIconFactory( private const val MAX_DECODE_PIXELS = 2048L * 2048L private val loadExecutor: ExecutorService = Executors.newSingleThreadExecutor() - - private val BLOCKED_REMOTE_HOSTS = - setOf( - "metadata.google.internal", - "metadata.goog", - ) } } 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 00000000..06af4ff2 --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicy.kt @@ -0,0 +1,133 @@ +package com.margelo.nitro.nitromaps + +import java.net.Inet6Address +import java.net.InetAddress +import java.net.URI +import java.net.URISyntaxException +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 + } + + val uri = parseUri(image.uri) ?: 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)) { + return "host not allowlisted" + } + + return null + } + + private fun parseUri(uriString: String): URI? = + try { + URI(uriString) + } catch (_: URISyntaxException) { + null + } + + private fun isAllowlistedHost( + host: String, + resolveHostAddress: Boolean, + ): 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 address = + try { + InetAddress.getByName(host) + } catch (_: UnknownHostException) { + return !resolveHostAddress + } + + return isAllowlistedAddress(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 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 3aad9495..ab128223 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/RemoteMarkerUriPolicyTest.kt b/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicyTest.kt new file mode 100644 index 00000000..b0ee1dad --- /dev/null +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicyTest.kt @@ -0,0 +1,106 @@ +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 + +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), + ) + } + + /** + * 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 c05a4c6e..1fdcce30 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 936d38a8..21b68a54 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 62bf6bc1..7b530232 100644 --- a/package/src/overlays/__tests__/normalizeMarkerDescriptors.test.ts +++ b/package/src/overlays/__tests__/normalizeMarkerDescriptors.test.ts @@ -58,6 +58,7 @@ describe('normalizeMarkerDescriptors', () => { width: 32, height: 32, scale: 2, + origin: 'bundled', }); }); diff --git a/package/src/overlays/__tests__/resolveMarkerImage.test.ts b/package/src/overlays/__tests__/resolveMarkerImage.test.ts index 21bb1cd0..08670d73 100644 --- a/package/src/overlays/__tests__/resolveMarkerImage.test.ts +++ b/package/src/overlays/__tests__/resolveMarkerImage.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, mock, test } from 'bun:test'; +import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from 'bun:test'; import { isMarkerImage, markerImageFromResolvedAsset, @@ -7,8 +7,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 +28,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 +45,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 +58,7 @@ describe('markerImageFromResolvedAsset', () => { width: 32, height: 32, scale: 2, + origin: 'bundled', }); }); @@ -82,10 +94,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); @@ -93,13 +106,33 @@ 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('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 unchanged', () => { const image = { uri: 'asset:/pin.png', diff --git a/package/src/overlays/descriptorEquality.ts b/package/src/overlays/descriptorEquality.ts index c5187356..a31ccf1d 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 6fe09b91..4b7d2db7 100644 --- a/package/src/overlays/markerImageFromResolvedAsset.ts +++ b/package/src/overlays/markerImageFromResolvedAsset.ts @@ -1,5 +1,9 @@ import type { MarkerImage } from '../native/specs/overlays'; +/** + * 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 { @@ -12,6 +16,7 @@ export function markerImageFromResolvedAsset( width: resolved.width, height: resolved.height, scale: resolved.scale, + origin: 'bundled', }; } diff --git a/package/src/overlays/resolveMarkerImage.ts b/package/src/overlays/resolveMarkerImage.ts index 4a53d071..7e302fef 100644 --- a/package/src/overlays/resolveMarkerImage.ts +++ b/package/src/overlays/resolveMarkerImage.ts @@ -5,6 +5,7 @@ import { isMarkerImage, markerImageFromResolvedAsset, } from './markerImageFromResolvedAsset'; +import { warnOverlay } from './warnOverlay'; const MARKER_IMAGE_CACHE_SIZE = 64; const resolvedImageCache = new LruCache(MARKER_IMAGE_CACHE_SIZE); @@ -13,6 +14,15 @@ function markerImageCacheKey(image: MarkerImage): string { return `${image.uri}|${image.width ?? ''}|${image.height ?? ''}|${image.scale ?? ''}`; } +/** + * Provenance is stamped by this module alone: an image whose fields come from an API + * response must not be able to claim `'bundled'` and skip the Android host policy. + */ +function withoutOrigin(image: MarkerImage): MarkerImage { + const { origin: _origin, ...rest } = image; + return rest; +} + export function resolveMarkerImage( source: MarkerImageSource | undefined, ): MarkerImage | undefined { @@ -21,13 +31,20 @@ export function resolveMarkerImage( } if (isMarkerImage(source)) { - const key = markerImageCacheKey(source); + const image = source.origin === undefined ? source : withoutOrigin(source); + const key = markerImageCacheKey(image); const cached = resolvedImageCache.get(key); if (cached != null) { return cached; } - resolvedImageCache.set(key, source); - return source; + if (source.origin !== undefined) { + // On the cache miss only, so a re-rendering marker list does not repeat it. + warnOverlay( + `marker image "${image.uri}": "origin" is set by the library and was ignored`, + ); + } + resolvedImageCache.set(key, image); + return image; } if (typeof source === 'number') { From bc8c09537b23309765c12a483495e19b17fd30e1 Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Sat, 26 Sep 2026 20:41:31 +0200 Subject: [PATCH 2/4] fix: cache a copy of a user-supplied marker image, never the caller's object resolveMarkerImage cached a user-supplied image by reference whenever it had no origin and no null field. Setting origin: 'bundled' on that object afterwards changed the cached entry too, so every later render of the same image would skip the Android host policy. Cache a copy instead. copyMarkerImage keeps only uri and the size fields, so it drops a claimed origin and null fields in one step, and replaces both withoutOrigin and markerImageWithoutNulls. --- .../__tests__/resolveMarkerImage.test.ts | 20 ++++++++++++-- .../overlays/markerImageFromResolvedAsset.ts | 15 +++++------ package/src/overlays/resolveMarkerImage.ts | 27 +++++++------------ 3 files changed, 34 insertions(+), 28 deletions(-) diff --git a/package/src/overlays/__tests__/resolveMarkerImage.test.ts b/package/src/overlays/__tests__/resolveMarkerImage.test.ts index adce5084..2eef54b4 100644 --- a/package/src/overlays/__tests__/resolveMarkerImage.test.ts +++ b/package/src/overlays/__tests__/resolveMarkerImage.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from 'bun:test'; +import type { MarkerImage } from '../../native/specs/overlays'; import { isMarkerImage, markerImageFromResolvedAsset, @@ -150,18 +151,33 @@ describe('resolveMarkerImage', () => { expect(warnSpy).toHaveBeenCalledTimes(1); }); - test('passes through MarkerImage objects unchanged', () => { + 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/markerImageFromResolvedAsset.ts b/package/src/overlays/markerImageFromResolvedAsset.ts index 5c1ef638..131ba324 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, @@ -23,14 +28,6 @@ export function markerImageFromResolvedAsset( return { ...copyMarkerImage(resolved), origin: 'bundled' }; } -export function markerImageWithoutNulls(image: MarkerImage): MarkerImage { - if (image.width === null || image.height === null || image.scale === null) { - return copyMarkerImage(image); - } - - return image; -} - export function isMarkerImage(value: unknown): value is MarkerImage { if (typeof value !== 'object' || value == null) { return false; diff --git a/package/src/overlays/resolveMarkerImage.ts b/package/src/overlays/resolveMarkerImage.ts index 208fb301..37f8303c 100644 --- a/package/src/overlays/resolveMarkerImage.ts +++ b/package/src/overlays/resolveMarkerImage.ts @@ -2,9 +2,9 @@ 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'; @@ -15,15 +15,6 @@ function markerImageCacheKey(image: MarkerImage): string { return `${image.uri}|${image.width ?? ''}|${image.height ?? ''}|${image.scale ?? ''}`; } -/** - * Provenance is stamped by this module alone: an image whose fields come from an API - * response must not be able to claim `'bundled'` and skip the Android host policy. - */ -function withoutOrigin(image: MarkerImage): MarkerImage { - const { origin: _origin, ...rest } = image; - return rest; -} - export function resolveMarkerImage( source: MarkerImageSource | undefined, ): MarkerImage | undefined { @@ -32,21 +23,23 @@ export function resolveMarkerImage( } if (isMarkerImage(source)) { - const image = source.origin === undefined ? source : withoutOrigin(source); - const key = markerImageCacheKey(image); + const key = markerImageCacheKey(source); const cached = resolvedImageCache.get(key); if (cached != null) { return cached; } - if (source.origin !== undefined) { + if (source.origin != null) { // On the cache miss only, so a re-rendering marker list does not repeat it. warnOverlay( - `marker image "${image.uri}": "origin" is set by the library and was ignored`, + `marker image "${source.uri}": "origin" is set by the library and was ignored`, ); } - const resolved = markerImageWithoutNulls(image); - resolvedImageCache.set(key, resolved); - return resolved; + // 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; } if (typeof source === 'number') { From 11219ccb6131dfd173117a503ad48a68ffcce83a Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Sat, 26 Sep 2026 20:42:05 +0200 Subject: [PATCH 3/4] fix(android): check every redirect of a remote marker image against the host policy HttpURLConnection follows redirects on its own, so the host policy only ever saw the URL the app passed in: a public URL that redirected to 192.168.x.x or 169.254.169.254 was fetched unchecked. RemoteMarkerImageFetcher turns automatic redirects off and follows up to five itself, running the policy on each target before it connects. A redirect has to stay on http(s) and may not go from https to http, for a bundled image too. A redirect from http to https, which the platform did not follow, now loads. The policy also checks every address a host name resolves to, not only the first: the connection moves on to the next address when the first does not answer. The JVM tests run the real policy over fake connections and a fake DNS, so none of them touch the network. --- README.md | 7 +- .../nitro/nitromaps/MarkerIconFactory.kt | 17 +- .../nitromaps/RemoteMarkerImageFetcher.kt | 88 +++++++++ .../nitro/nitromaps/RemoteMarkerUriPolicy.kt | 46 ++++- .../nitromaps/RemoteMarkerImageFetcherTest.kt | 173 ++++++++++++++++++ .../nitromaps/RemoteMarkerUriPolicyTest.kt | 19 ++ 6 files changed, 329 insertions(+), 21 deletions(-) create mode 100644 package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcher.kt create mode 100644 package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcherTest.kt diff --git a/README.md b/README.md index 42e7b994..8a56b186 100644 --- a/README.md +++ b/README.md @@ -521,9 +521,10 @@ Supported image sources: 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. 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. +`.local`/`.localhost` name, or a cloud metadata endpoint. 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. 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 17e507f9..ef7f4d07 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,7 +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.URL import java.util.WeakHashMap import java.util.concurrent.ExecutorService import java.util.concurrent.Executors @@ -30,6 +29,7 @@ internal class MarkerIconFactory( private val appliedIconKeys = WeakHashMap() private val pendingIconLoads = WeakHashMap() private val iconLoadGeneration = WeakHashMap() + private val remoteImageFetcher = RemoteMarkerImageFetcher() private data class PendingIconLoad( val iconKey: String, @@ -244,19 +244,10 @@ internal class MarkerIconFactory( image: MarkerImage, key: String, ): BitmapDescriptor? { - RemoteMarkerUriPolicy.rejectReason(image, 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 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 00000000..5ec2ab55 --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcher.kt @@ -0,0 +1,88 @@ +package com.margelo.nitro.nitromaps + +import java.net.HttpURLConnection +import java.net.MalformedURLException +import java.net.URL + +/** + * Downloads a remote marker image and follows its redirects itself, so that every hop of a + * user-supplied URL passes [RemoteMarkerUriPolicy]. `HttpURLConnection` would otherwise follow a + * public URL's redirect to a private host without asking the policy. + * + * @param hostRejectReason the policy check for one hop; a seam for tests. + * @param openConnection opens one hop; a seam for tests. + */ +internal class RemoteMarkerImageFetcher( + private val hostRejectReason: (uri: String) -> String? = { uri -> + RemoteMarkerUriPolicy.rejectReason(uri, resolveHostAddress = true) + }, + private val openConnection: (URL) -> HttpURLConnection = { url -> + url.openConnection() as HttpURLConnection + }, +) { + /** + * 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 check, on every hop. + val checksHost = image.origin != MarkerImageOrigin.BUNDLED + var url = URL(image.uri) + + repeat(MAX_REDIRECTS + 1) { + if (checksHost) { + // The URL about to be opened, as it will be opened, rather than the string it came from. + hostRejectReason(url.toString())?.let { reason -> + onRejected(url.toString(), reason) + return null + } + } + + val connection = openConnection(url) + connection.instanceFollowRedirects = false + connection.connectTimeout = TIMEOUT_MS + connection.readTimeout = TIMEOUT_MS + if (connection.responseCode !in REDIRECT_STATUS_CODES) { + return connection.inputStream.use { it.readBytes() } + } + + val location = connection.getHeaderField("Location") + connection.disconnect() + val target = 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 + } + + companion object { + const val MAX_REDIRECTS = 5 + private const val TIMEOUT_MS = 10_000 + + /** The redirect statuses Android's `HttpURLConnection` follows on its own. */ + private val REDIRECT_STATUS_CODES = setOf(300, 301, 302, 303, 307, 308) + } +} 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 index 06af4ff2..f35bd2f1 100644 --- a/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicy.kt +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicy.kt @@ -4,6 +4,7 @@ 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 @@ -39,7 +40,21 @@ internal object RemoteMarkerUriPolicy { return null } - val uri = parseUri(image.uri) ?: return "invalid URI" + 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 @@ -52,13 +67,31 @@ internal object RemoteMarkerUriPolicy { } val host = uri.host?.lowercase(Locale.US)?.takeIf { it.isNotEmpty() } ?: return "missing host" - if (!isAllowlistedHost(host, resolveHostAddress)) { + 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) @@ -69,6 +102,7 @@ internal object RemoteMarkerUriPolicy { private fun isAllowlistedHost( host: String, resolveHostAddress: Boolean, + resolveHost: (String) -> Array, ): Boolean { if (host == "localhost" || host.endsWith(".localhost") || host.endsWith(".local")) { return false @@ -81,14 +115,16 @@ internal object RemoteMarkerUriPolicy { return true } - val address = + val addresses = try { - InetAddress.getByName(host) + resolveHost(host) } catch (_: UnknownHostException) { return !resolveHostAddress } - return isAllowlistedAddress(address) + // 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 { 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 00000000..96fc9df6 --- /dev/null +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcherTest.kt @@ -0,0 +1,173 @@ +package com.margelo.nitro.nitromaps + +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Test +import java.io.ByteArrayInputStream +import java.io.InputStream +import java.net.HttpURLConnection +import java.net.InetAddress +import java.net.URL + +class RemoteMarkerImageFetcherTest { + private val pin = byteArrayOf(1, 2, 3) + private val responses = mutableMapOf() + private val opened = mutableListOf() + private val rejections = mutableListOf>() + + private val fetcher = + RemoteMarkerImageFetcher( + // The real policy, over a DNS that answers without the network. + hostRejectReason = { uri -> + RemoteMarkerUriPolicy.rejectReason(uri, resolveHostAddress = true, resolveHost = ::fakeDns) + }, + openConnection = { url -> + opened += url.toString() + responses.getValue(url.toString()) + }, + ) + + @Test + fun refusesARedirectFromAPublicHostToAPrivateOne() { + redirect("http://cdn.example.com/pin.png", to = "http://192.168.1.1/admin/pin.png") + + assertNull(fetch("http://cdn.example.com/pin.png")) + assertEquals(listOf("http://192.168.1.1/admin/pin.png" to "host not allowlisted"), rejections) + assertEquals(listOf("http://cdn.example.com/pin.png"), opened) + } + + @Test + fun refusesARedirectToAHostNameThatResolvesToAPrivateAddress() { + redirect("https://cdn.example.com/pin.png", to = "https://rebind.example.com/pin.png") + + assertNull(fetch("https://cdn.example.com/pin.png")) + assertEquals(listOf("https://rebind.example.com/pin.png" to "host not allowlisted"), rejections) + assertEquals(listOf("https://cdn.example.com/pin.png"), opened) + } + + @Test + fun followsRedirectsBetweenPublicHosts() { + redirect("http://cdn.example.com/pin.png", to = "https://img.example.com/pin.png", status = 301) + serve("https://img.example.com/pin.png") + + assertArrayEquals(pin, fetch("http://cdn.example.com/pin.png")) + assertEquals(emptyList>(), rejections) + assertFalse(responses.getValue("http://cdn.example.com/pin.png").instanceFollowRedirects) + } + + @Test + fun resolvesARelativeLocationAgainstTheRedirectingUrl() { + redirect("https://cdn.example.com/v1/pin.png", to = "../v2/pin.png") + serve("https://cdn.example.com/v2/pin.png") + + assertArrayEquals(pin, fetch("https://cdn.example.com/v1/pin.png")) + } + + @Test + fun refusesARedirectFromHttpsToHttp() { + redirect("https://cdn.example.com/pin.png", to = "http://cdn.example.com/pin.png") + + assertNull(fetch("https://cdn.example.com/pin.png")) + assertEquals(listOf("http://cdn.example.com/pin.png" to "redirect from https to http"), rejections) + } + + @Test + fun refusesARedirectWithoutALocation() { + redirect("https://cdn.example.com/pin.png", to = null) + + assertNull(fetch("https://cdn.example.com/pin.png")) + assertEquals( + listOf("https://cdn.example.com/pin.png" to "redirect without a usable location"), + rejections, + ) + } + + @Test + fun givesUpAfterTooManyRedirects() { + val hops = (0..RemoteMarkerImageFetcher.MAX_REDIRECTS + 1).map { "https://cdn.example.com/$it.png" } + hops.zipWithNext().forEach { (from, to) -> redirect(from, to) } + + assertNull(fetch(hops.first())) + assertEquals(RemoteMarkerImageFetcher.MAX_REDIRECTS + 1, opened.size) + assertEquals( + listOf(hops.first() to "more than ${RemoteMarkerImageFetcher.MAX_REDIRECTS} redirects"), + rejections, + ) + } + + @Test + fun fetchesABundledImageFromThePackagerWithoutTheHostPolicy() { + serve("http://10.0.2.2:8081/assets/pin.png") + + assertArrayEquals(pin, fetch("http://10.0.2.2:8081/assets/pin.png", MarkerImageOrigin.BUNDLED)) + assertEquals(emptyList>(), rejections) + } + + @Test + fun keepsTheRedirectOfABundledImageOnHttp() { + redirect("http://10.0.2.2:8081/assets/pin.png", to = "file:///sdcard/pin.png") + + assertNull(fetch("http://10.0.2.2:8081/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 } + + private fun serve(uri: String) { + responses[uri] = FakeConnection(URL(uri), status = 200, body = pin) + } + + private fun redirect( + uri: String, + to: String?, + status: Int = 302, + ) { + responses[uri] = FakeConnection(URL(uri), status = status, location = to) + } + + /** Answers from [zone] for a host name, and parses an IP literal as the real lookup does. */ + private fun fakeDns(host: String): Array { + zone[host]?.let { return it } + check(host.all { it.isDigit() || it == '.' }) { "no fake DNS answer for $host" } + return InetAddress.getAllByName(host) + } + + private val zone = + mapOf( + "cdn.example.com" to arrayOf(PUBLIC_ADDRESS), + "img.example.com" to arrayOf(PUBLIC_ADDRESS), + // A public answer next to a private one, as a rebinding DNS server would give. + "rebind.example.com" to arrayOf(PUBLIC_ADDRESS, InetAddress.getByAddress(byteArrayOf(10, 0, 0, 1))), + ) + + private class FakeConnection( + url: URL, + private val status: Int, + private val location: String? = null, + private val body: ByteArray = ByteArray(0), + ) : HttpURLConnection(url) { + override fun connect() = Unit + + override fun disconnect() = Unit + + override fun usingProxy() = false + + override fun getResponseCode() = status + + override fun getHeaderField(name: String?): String? = if (name.equals("Location", ignoreCase = true)) location else null + + override fun getInputStream(): InputStream = ByteArrayInputStream(body) + } + + private companion object { + val PUBLIC_ADDRESS: InetAddress = InetAddress.getByAddress(byteArrayOf(93, 184.toByte(), 216.toByte(), 34)) + } +} 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 index b0ee1dad..6292304c 100644 --- a/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicyTest.kt +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicyTest.kt @@ -5,6 +5,7 @@ import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test +import java.net.InetAddress class RemoteMarkerUriPolicyTest { @Test @@ -84,6 +85,24 @@ class RemoteMarkerUriPolicyTest { ) } + @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()) + } + /** * Runs the main-thread pre-check, which classifies a numeric host literal without a DNS * lookup — so every assertion here stays off the network. From 5eca08c8661b90c17f456ec0c923176fdab86720 Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Sat, 26 Sep 2026 21:16:05 +0200 Subject: [PATCH 4/4] fix(android): send no marker image request over a connection to a private address The host policy resolved a user-supplied image's host before the request, but the connection resolved it again. A DNS answer that changed in between (rebinding) could still send the request to a private address, and CodeRabbit's security check flagged that gap. Fetch remote marker images with OkHttp, which react-android already exposes, so that a network interceptor sees the address a connection was actually made to. It closes a direct connection to an address the policy rejects before the request is written to it. Behind a proxy, the proxy resolves the host, so only the check on the URL applies there, as before. Tests against a local HTTP server cover a host that answers public to the policy and private to the client, requested directly and as a redirect target. The client connects, but no request arrives. --- README.md | 5 +- .../nitro/nitromaps/MarkerIconFactory.kt | 5 +- .../nitromaps/RemoteMarkerImageFetcher.kt | 115 +++++-- .../nitro/nitromaps/RemoteMarkerUriPolicy.kt | 3 +- .../nitromaps/RemoteMarkerImageFetcherTest.kt | 318 +++++++++++++----- .../nitromaps/RemoteMarkerUriPolicyTest.kt | 20 ++ 6 files changed, 338 insertions(+), 128 deletions(-) diff --git a/README.md b/README.md index 8a56b186..1856e8d7 100644 --- a/README.md +++ b/README.md @@ -521,8 +521,9 @@ Supported image sources: 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. 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 +`.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. 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 ef7f4d07..e6a7e25c 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 @@ -29,7 +29,6 @@ internal class MarkerIconFactory( private val appliedIconKeys = WeakHashMap() private val pendingIconLoads = WeakHashMap() private val iconLoadGeneration = WeakHashMap() - private val remoteImageFetcher = RemoteMarkerImageFetcher() private data class PendingIconLoad( val iconKey: String, @@ -436,5 +435,9 @@ internal class MarkerIconFactory( } private val loadExecutor: ExecutorService = Executors.newSingleThreadExecutor() + + // 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 index 5ec2ab55..cf29a97f 100644 --- a/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcher.kt +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcher.kt @@ -1,25 +1,48 @@ package com.margelo.nitro.nitromaps -import java.net.HttpURLConnection +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]. `HttpURLConnection` would otherwise follow a - * public URL's redirect to a private host without asking the policy. + * user-supplied URL passes [RemoteMarkerUriPolicy]. An automatic redirect would reach a private + * host without asking the policy. * - * @param hostRejectReason the policy check for one hop; a seam for tests. - * @param openConnection opens one hop; a seam for tests. + * 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) }, - private val openConnection: (URL) -> HttpURLConnection = { url -> - url.openConnection() as HttpURLConnection - }, ) { + /** 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. @@ -28,39 +51,47 @@ internal class RemoteMarkerImageFetcher( image: MarkerImage, onRejected: (uri: String, reason: String) -> Unit, ): ByteArray? { - // Like the policy itself, a bundled image skips the host check, on every hop. + // 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 opened, as it will be opened, rather than the string it came from. + // 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 connection = openConnection(url) - connection.instanceFollowRedirects = false - connection.connectTimeout = TIMEOUT_MS - connection.readTimeout = TIMEOUT_MS - if (connection.responseCode !in REDIRECT_STATUS_CODES) { - return connection.inputStream.use { it.readBytes() } - } + val response = + try { + client.newCall(Request.Builder().url(url).build()).execute() + } catch (rejected: RejectedConnectionException) { + onRejected(url.toString(), rejected.reason) + return null + } - val location = connection.getHeaderField("Location") - connection.disconnect() - val target = 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 + 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 } - url = target } onRejected(image.uri, "more than $MAX_REDIRECTS redirects") @@ -78,11 +109,31 @@ internal class RemoteMarkerImageFetcher( 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_000 - - /** The redirect statuses Android's `HttpURLConnection` follows on its own. */ - private val REDIRECT_STATUS_CODES = setOf(300, 301, 302, 303, 307, 308) + 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 index f35bd2f1..9678f67e 100644 --- a/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicy.kt +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicy.kt @@ -140,7 +140,8 @@ internal object RemoteMarkerUriPolicy { } } - private fun isAllowlistedAddress(address: InetAddress): Boolean { + /** Whether a connection to [address] may carry a request for a user-supplied image. */ + fun isAllowlistedAddress(address: InetAddress): Boolean { if ( address.isLoopbackAddress || address.isAnyLocalAddress || 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 index 96fc9df6..2dbb6d94 100644 --- a/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcherTest.kt +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerImageFetcherTest.kt @@ -1,115 +1,181 @@ 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.assertFalse import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows import org.junit.Test -import java.io.ByteArrayInputStream -import java.io.InputStream -import java.net.HttpURLConnection +import java.io.IOException import java.net.InetAddress -import java.net.URL - +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 pin = byteArrayOf(1, 2, 3) - private val responses = mutableMapOf() - private val opened = mutableListOf() + 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( - // The real policy, over a DNS that answers without the network. + 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, resolveHost = ::fakeDns) - }, - openConnection = { url -> - opened += url.toString() - responses.getValue(url.toString()) + RemoteMarkerUriPolicy.rejectReason(uri, resolveHostAddress = true) { host -> + lookUp(checkedZone, host).toTypedArray() + } }, ) + @After + fun closeServer() { + server.close() + } + @Test - fun refusesARedirectFromAPublicHostToAPrivateOne() { - redirect("http://cdn.example.com/pin.png", to = "http://192.168.1.1/admin/pin.png") + fun fetchesAPublicImage() { + server.respond("cdn.example.com:$port/pin.png", ok("pin")) - assertNull(fetch("http://cdn.example.com/pin.png")) - assertEquals(listOf("http://192.168.1.1/admin/pin.png" to "host not allowlisted"), rejections) - assertEquals(listOf("http://cdn.example.com/pin.png"), opened) + 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 refusesARedirectToAHostNameThatResolvesToAPrivateAddress() { - redirect("https://cdn.example.com/pin.png", to = "https://rebind.example.com/pin.png") + fun sendsNothingToAPrivateAddressAHostRebindsToAfterTheCheck() { + server.respond("rebind.example.com:$port/pin.png", ok("pin")) - assertNull(fetch("https://cdn.example.com/pin.png")) - assertEquals(listOf("https://rebind.example.com/pin.png" to "host not allowlisted"), rejections) - assertEquals(listOf("https://cdn.example.com/pin.png"), opened) + 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 followsRedirectsBetweenPublicHosts() { - redirect("http://cdn.example.com/pin.png", to = "https://img.example.com/pin.png", status = 301) - serve("https://img.example.com/pin.png") + fun refusesARedirectFromAPublicHostToAPrivateOne() { + server.respond("cdn.example.com:$port/pin.png", redirect("http://192.168.1.1:$port/admin/pin.png")) - assertArrayEquals(pin, fetch("http://cdn.example.com/pin.png")) - assertEquals(emptyList>(), rejections) - assertFalse(responses.getValue("http://cdn.example.com/pin.png").instanceFollowRedirects) + 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 resolvesARelativeLocationAgainstTheRedirectingUrl() { - redirect("https://cdn.example.com/v1/pin.png", to = "../v2/pin.png") - serve("https://cdn.example.com/v2/pin.png") + 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")) - assertArrayEquals(pin, fetch("https://cdn.example.com/v1/pin.png")) + 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 refusesARedirectFromHttpsToHttp() { - redirect("https://cdn.example.com/pin.png", to = "http://cdn.example.com/pin.png") + 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")) - assertNull(fetch("https://cdn.example.com/pin.png")) - assertEquals(listOf("http://cdn.example.com/pin.png" to "redirect from https to http"), rejections) + 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() { - redirect("https://cdn.example.com/pin.png", to = null) + server.respond("cdn.example.com:$port/pin.png", redirect(location = null)) - assertNull(fetch("https://cdn.example.com/pin.png")) + assertNull(fetch("http://cdn.example.com:$port/pin.png")) assertEquals( - listOf("https://cdn.example.com/pin.png" to "redirect without a usable location"), + 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 { "https://cdn.example.com/$it.png" } - hops.zipWithNext().forEach { (from, to) -> redirect(from, to) } + 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(hops.first())) - assertEquals(RemoteMarkerImageFetcher.MAX_REDIRECTS + 1, opened.size) + assertNull(fetch("http://cdn.example.com:$port${hops.first()}")) + assertEquals(RemoteMarkerImageFetcher.MAX_REDIRECTS + 1, server.requests.size) assertEquals( - listOf(hops.first() to "more than ${RemoteMarkerImageFetcher.MAX_REDIRECTS} redirects"), + listOf("http://cdn.example.com:$port${hops.first()}" to "more than ${RemoteMarkerImageFetcher.MAX_REDIRECTS} redirects"), rejections, ) } @Test - fun fetchesABundledImageFromThePackagerWithoutTheHostPolicy() { - serve("http://10.0.2.2:8081/assets/pin.png") + 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, fetch("http://10.0.2.2:8081/assets/pin.png", MarkerImageOrigin.BUNDLED)) + assertArrayEquals("pin".toByteArray(), fetch("http://10.0.2.2:$port/assets/pin.png", MarkerImageOrigin.BUNDLED)) assertEquals(emptyList>(), rejections) } @Test fun keepsTheRedirectOfABundledImageOnHttp() { - redirect("http://10.0.2.2:8081/assets/pin.png", to = "file:///sdcard/pin.png") + server.respond("10.0.2.2:$port/assets/pin.png", redirect("file:///sdcard/pin.png")) - assertNull(fetch("http://10.0.2.2:8081/assets/pin.png", MarkerImageOrigin.BUNDLED)) + 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) } @@ -121,53 +187,121 @@ class RemoteMarkerImageFetcherTest { MarkerImage(uri = uri, width = null, height = null, scale = null, origin = origin), ) { rejectedUri, reason -> rejections += rejectedUri to reason } - private fun serve(uri: String) { - responses[uri] = FakeConnection(URL(uri), status = 200, body = pin) - } - - private fun redirect( - uri: String, - to: String?, - status: Int = 302, - ) { - responses[uri] = FakeConnection(URL(uri), status = status, location = to) - } - - /** Answers from [zone] for a host name, and parses an IP literal as the real lookup does. */ - private fun fakeDns(host: String): Array { - zone[host]?.let { return it } + /** 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) + return InetAddress.getAllByName(host).toList() } - private val zone = - mapOf( - "cdn.example.com" to arrayOf(PUBLIC_ADDRESS), - "img.example.com" to arrayOf(PUBLIC_ADDRESS), - // A public answer next to a private one, as a rebinding DNS server would give. - "rebind.example.com" to arrayOf(PUBLIC_ADDRESS, InetAddress.getByAddress(byteArrayOf(10, 0, 0, 1))), - ) - - private class FakeConnection( - url: URL, - private val status: Int, - private val location: String? = null, - private val body: ByteArray = ByteArray(0), - ) : HttpURLConnection(url) { - override fun connect() = Unit - - override fun disconnect() = Unit + private fun ok(body: String) = "HTTP/1.1 200 OK\r\nContent-Length: ${body.length}\r\nConnection: close\r\n\r\n$body" - override fun usingProxy() = false - - override fun getResponseCode() = status - - override fun getHeaderField(name: String?): String? = if (name.equals("Location", ignoreCase = true)) location else null - - override fun getInputStream(): InputStream = ByteArrayInputStream(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 index 6292304c..4762eb8f 100644 --- a/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicyTest.kt +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/RemoteMarkerUriPolicyTest.kt @@ -6,6 +6,7 @@ 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 @@ -103,6 +104,25 @@ class RemoteMarkerUriPolicyTest { 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.