From 5a5b95d47defb88740f20278afdab87e2a8b5ba7 Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Tue, 22 Sep 2026 17:24:05 +0200 Subject: [PATCH 1/8] 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 1541bb025cac2be8c8f3020e43cf034dfbab4ad6 Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Tue, 22 Sep 2026 17:57:09 +0200 Subject: [PATCH 2/8] fix(ios): apply the remote marker image host policy on iOS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Android refused to load a marker image from a private-network host while iOS loaded any URL it could parse, so an app shipping on both platforms was protected on one of them. The threat the policy guards against — a URL arriving as data, `image={{ uri: apiResponse.iconUrl }}` pointing at a router admin page or a cloud metadata endpoint — is not platform-specific. `RemoteMarkerUriPolicy` ports the Kotlin rules to Swift: `http`/`https` only, no `user:password@`, and a host that must not resolve to a loopback, any-local, link-local, site-local, multicast or IPv6 unique-local address, a `.local`/`.localhost` name, or a cloud metadata endpoint. A rejected image falls back to the default pin and logs `Rejected remote marker image URI`. `require()`d assets keep their exemption through the `origin` stamp, which is what stops this reproducing #129 on iOS: `resolveAssetSource` hands a development build a `http://localhost:8081/assets/...` URL, which is loopback. Three things Foundation forces, rather than a line-for-line port: - `URLComponents.host` is the decoded form, so `http://%31%30.0.2.2/` is classified as the 10.0.2.2 it reaches rather than as an opaque name. - An IPv4 address in an IPv6 shape is unwrapped by hand. `InetAddress` does it on its own; without it `::ffff:127.0.0.1` would pass every IPv6 rule. - A name that resolves to both a public and a private address is rejected. `InetAddress.getByName` classifies its first answer only. The policy lives in the SwiftPM support package, so `swift test` covers it the way `RemoteMarkerUriPolicyTest` covers the Kotlin side. `origin` joins the iOS icon cache key for the reason it joined the Android one: a bundled image must not warm the cache for a user-supplied URI that matches. --- README.md | 25 +- package/ios/MarkerImageLoader.swift | 68 ++++- package/ios/Package.swift | 9 + .../RemoteMarkerUriPolicy.swift | 239 ++++++++++++++++++ .../RemoteMarkerUriPolicyTests.swift | 129 ++++++++++ .../GoogleMarkerVisualApplierTests.swift | 9 +- 6 files changed, 464 insertions(+), 15 deletions(-) create mode 100644 package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift create mode 100644 package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift diff --git a/README.md b/README.md index dd0f1c3b..c8e3f869 100644 --- a/README.md +++ b/README.md @@ -369,19 +369,26 @@ 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; Android restricts the host, see below | +| Remote URL | `{ uri: 'https://…' }` | Async fetch with in-memory cache; the host is restricted, see below | -### Remote image hosts on Android +### Remote image hosts -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. +A remote URL you pass yourself — `image={{ uri: someUrl }}` — is checked on both platforms before +the image is fetched. The URL must use `http`/`https`, must not carry `user:password@`, and its +host must not resolve to a private address: loopback, any-local, 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 logs `Rejected remote marker image URI` — under the `NitroMaps` logcat tag on +Android, and under the `NitroMaps` category of the `com.nitromaps` subsystem in the unified log +on iOS. The marker itself falls back to the default pin on both Google providers. On Apple Maps +it draws with no image at all, the same as any marker image that fails to load. `require()`d assets are exempt, including the packager URL Metro resolves them to in a -development build. iOS does not apply this policy today. +development build. + +If you need markers served from a private host, resolve it to an image yourself and pass the +bytes as a local file, or proxy it through a public endpoint. Additional props: diff --git a/package/ios/MarkerImageLoader.swift b/package/ios/MarkerImageLoader.swift index c1ca8fe9..c5634c4a 100644 --- a/package/ios/MarkerImageLoader.swift +++ b/package/ios/MarkerImageLoader.swift @@ -1,3 +1,4 @@ +import OSLog import UIKit /// Loads marker images from bundled assets or remote URLs with in-memory caching. @@ -9,6 +10,16 @@ enum MarkerImageLoader { label: "com.nitromaps.markerImageDecode", qos: .userInitiated ) + /// Resolving a host blocks, and a slow lookup must not hold up local decoding. Serial rather + /// than concurrent so a screen full of markers cannot spawn a thread per lookup; the image + /// cache means each distinct URI is resolved once. + private static let policyQueue = DispatchQueue( + label: "com.nitromaps.markerImagePolicy", + qos: .userInitiated + ) + /// The category matches `NITRO_MAPS_LOG_TAG` on Android, so a rejected image is found the same + /// way on both platforms. + private static let logger = Logger(subsystem: "com.nitromaps", category: "NitroMaps") static func cachedImage(for image: MarkerImage) -> UIImage? { cache.object(forKey: cacheKey(for: image)) @@ -25,7 +36,7 @@ enum MarkerImageLoader { } let uri = image.uri - if uri.hasPrefix("http://") || uri.hasPrefix("https://") { + if RemoteMarkerUriPolicy.isRemoteUri(uri) { loadRemote(uri: uri, cacheKey: cacheKey, image: image, completion: completion) return } @@ -45,7 +56,10 @@ enum MarkerImageLoader { let width = image.width.map { String($0) } ?? "" let height = image.height.map { String($0) } ?? "" let scale = image.scale.map { String($0) } ?? "" - return "\(image.uri)|\(width)|\(height)|\(scale)" as NSString + // `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. + let origin = image.origin.map { $0.stringValue } ?? "" + return "\(image.uri)|\(width)|\(height)|\(scale)|\(origin)" as NSString } private static func loadLocal(uri: String, image: MarkerImage) -> UIImage? { @@ -78,11 +92,59 @@ enum MarkerImageLoader { image: MarkerImage, completion: @escaping (UIImage?) -> Void ) { - guard let url = URL(string: uri) else { + // No policy applies to an image the app bundled, so it must not wait behind the host lookup + // of an unrelated one: in a development build every `require()`d marker takes this path. + if image.origin == .bundled { + guard let url = URL(string: uri) else { + completion(nil) + return + } + startRemoteTask(url: url, cacheKey: cacheKey, image: image, completion: completion) + return + } + + // The pre-check classifies a numeric host literal without touching DNS, so the common + // rejection costs nothing and never leaves the caller's thread. + if let reason = RemoteMarkerUriPolicy.rejectReason( + uri: uri, + isBundled: false, + resolveHostAddress: false + ) { + logRejected(uri: uri, reason: reason) completion(nil) return } + policyQueue.async { + if let reason = RemoteMarkerUriPolicy.rejectReason( + uri: uri, + isBundled: false, + resolveHostAddress: true + ) { + logRejected(uri: uri, reason: reason) + DispatchQueue.main.async { completion(nil) } + return + } + + guard let url = URL(string: uri) else { + DispatchQueue.main.async { completion(nil) } + return + } + + startRemoteTask(url: url, cacheKey: cacheKey, image: image, completion: completion) + } + } + + private static func logRejected(uri: String, reason: String) { + logger.warning("Rejected remote marker image URI (\(reason, privacy: .public)): \(uri, privacy: .public)") + } + + private static func startRemoteTask( + url: URL, + cacheKey: NSString, + image: MarkerImage, + completion: @escaping (UIImage?) -> Void + ) { session.dataTask(with: url) { data, _, _ in let uiImage: UIImage? if let data, let decoded = UIImage(data: data) { diff --git a/package/ios/Package.swift b/package/ios/Package.swift index c4378bf4..2d207cd5 100644 --- a/package/ios/Package.swift +++ b/package/ios/Package.swift @@ -16,6 +16,10 @@ let package = Package( name: "NitroMapsGeometry", path: "Geometry" ), + .target( + name: "NitroMapsRemoteImagePolicy", + path: "RemoteImagePolicy" + ), .testTarget( name: "NitroMapsColorParserTests", dependencies: ["NitroMapsColorParser"], @@ -26,5 +30,10 @@ let package = Package( dependencies: ["NitroMapsGeometry"], path: "Tests/Geometry" ), + .testTarget( + name: "NitroMapsRemoteImagePolicyTests", + dependencies: ["NitroMapsRemoteImagePolicy"], + path: "Tests/RemoteImagePolicy" + ), ] ) diff --git a/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift b/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift new file mode 100644 index 00000000..dbde3b20 --- /dev/null +++ b/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift @@ -0,0 +1,239 @@ +import Foundation + +/// 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 (`localhost` through the simulator, `192.168.x.x` on a device), so the host cannot tell a +/// bundled asset apart from the addresses this policy blocks. +/// +/// Mirrors `RemoteMarkerUriPolicy.kt`. The two are meant to answer the same way for the same URL; +/// the divergences that Foundation forces are called out where they happen. +enum RemoteMarkerUriPolicy { + /// Whether `uriString` is fetched over the network rather than read from the app bundle. + static func isRemoteUri(_ uriString: String) -> Bool { + guard let scheme = URLComponents(string: uriString)?.scheme?.lowercased() else { + return false + } + return scheme == "http" || scheme == "https" + } + + /// Returns why the image must not be fetched, or `nil` when the fetch is allowed. + /// + /// - Parameters: + /// - isBundled: whether the image was stamped `MarkerImageOrigin.bundled` on the JS side. + /// - resolveHostAddress: whether a host name may be resolved through DNS. `false` for the + /// pre-check on the calling thread, `true` once the caller has hopped to a background queue, + /// so only a numeric host literal is classified before the thread hop. + static func rejectReason( + uri uriString: String, + isBundled: Bool, + resolveHostAddress: Bool + ) -> String? { + if isBundled { + return nil + } + + guard let components = URLComponents(string: uriString) else { + return "invalid URI" + } + + switch components.scheme?.lowercased() { + case "http", "https": break + case nil: return "missing scheme" + default: return "unsupported scheme" + } + + // Two accessors, because `user` is `nil` for `http://:secret@host` while `password` is not. + if components.user != nil || components.password != nil { + return "user info not allowed" + } + + // `host` is the decoded form, so `http://%31%30.0.2.2/` is classified as the 10.0.2.2 it + // resolves to rather than as an opaque name. `encodedHost` is the ASCII wire form, which is + // what DNS takes for an internationalised name. + guard let host = components.host?.lowercased(), !host.isEmpty else { + return "missing host" + } + + let allowed = isAllowlistedHost( + host, + encodedHost: components.encodedHost?.lowercased(), + resolveHostAddress: resolveHostAddress + ) + return allowed ? nil : "host not allowlisted" + } + + /// The resolving half of the host check, without the URL parsing around it. + /// + /// Blocking: only call it off the thread the caller came in on. + static func isAllowlistedResolvedHost(_ host: String) -> Bool { + let addresses = resolveAddresses(of: bracketless(host)) + if addresses.isEmpty { + return false + } + // Stricter than the Kotlin original, which classifies `InetAddress.getByName`'s first answer + // only: a name that resolves to both a public and a private address is rejected. + return addresses.allSatisfy(isAllowlistedAddress) + } + + private static func isAllowlistedHost( + _ host: String, + encodedHost: String?, + resolveHostAddress: Bool + ) -> Bool { + if host == "localhost" || host.hasSuffix(".localhost") || host.hasSuffix(".local") { + return false + } + if blockedHosts.contains(host) { + return false + } + + if let literal = parseLiteral(bracketless(host)) { + return isAllowlistedAddress(literal) + } + + // A name can only be classified by resolving it, and that must not happen on the calling + // thread. Allow it here; the pass that may resolve makes the real decision. + if !resolveHostAddress { + return true + } + + return isAllowlistedResolvedHost(encodedHost ?? host) + } + + /// An address reduced to the bytes the classification needs. + private enum IPAddress { + /// Host byte order. + case v4(UInt32) + /// 16 bytes, network order. + case v6([UInt8]) + } + + private static func parseLiteral(_ text: String) -> IPAddress? { + var v4 = in_addr() + if inet_pton(AF_INET, text, &v4) == 1 { + return .v4(UInt32(bigEndian: v4.s_addr)) + } + + var v6 = in6_addr() + if inet_pton(AF_INET6, text, &v6) == 1 { + return ipAddress(from: v6) + } + + return nil + } + + private static func ipAddress(from address: in6_addr) -> IPAddress { + var bytes = [UInt8](repeating: 0, count: 16) + withUnsafeBytes(of: address) { raw in + for index in 0..<16 { + bytes[index] = raw[index] + } + } + + if let embedded = embeddedIPv4(in: bytes) { + return .v4(embedded) + } + return .v6(bytes) + } + + /// The IPv4 address inside `::ffff:a.b.c.d` or `::a.b.c.d`, which every stack routes to the v4 + /// address it wraps. `InetAddress.getByName` unwraps the mapped form on its own, so without this + /// `::ffff:127.0.0.1` would pass every IPv6 rule. + private static func embeddedIPv4(in bytes: [UInt8]) -> UInt32? { + guard bytes[0..<10].allSatisfy({ $0 == 0 }) else { + return nil + } + + let isMapped = bytes[10] == 0xff && bytes[11] == 0xff + let isCompatible = bytes[10] == 0 && bytes[11] == 0 + guard isMapped || isCompatible else { + return nil + } + + let value = + UInt32(bytes[12]) << 24 | UInt32(bytes[13]) << 16 | UInt32(bytes[14]) << 8 | UInt32(bytes[15]) + // `::` and `::1` are the unspecified and loopback addresses, not an embedded IPv4. + if isCompatible && value <= 1 { + return nil + } + return value + } + + private static func isAllowlistedAddress(_ address: IPAddress) -> Bool { + switch address { + case .v4(let value): + let first = UInt8(truncatingIfNeeded: value >> 24) + let second = UInt8(truncatingIfNeeded: value >> 16) + + if value == 0 { return false } // any-local + if first == 127 { return false } // loopback + if first == 169 && second == 254 { return false } // link-local + if first == 10 { return false } // site-local + if first == 172 && (second & 0xf0) == 16 { return false } // site-local + if first == 192 && second == 168 { return false } // site-local + if (first & 0xf0) == 0xe0 { return false } // multicast + return true + + case .v6(let bytes): + if bytes.allSatisfy({ $0 == 0 }) { return false } // any-local + if bytes[0..<15].allSatisfy({ $0 == 0 }) && bytes[15] == 1 { return false } // loopback + if bytes[0] == 0xfe && (bytes[1] & 0xc0) == 0x80 { return false } // link-local + if bytes[0] == 0xfe && (bytes[1] & 0xc0) == 0xc0 { return false } // site-local + if bytes[0] == 0xff { return false } // multicast + if (bytes[0] & 0xfe) == 0xfc { return false } // unique-local + return true + } + } + + /// Blocking, so it may only run off the thread the caller came in on. + private static func resolveAddresses(of host: String) -> [IPAddress] { + var hints = addrinfo() + hints.ai_family = AF_UNSPEC + hints.ai_socktype = SOCK_STREAM + + var result: UnsafeMutablePointer? + guard getaddrinfo(host, nil, &hints, &result) == 0, let head = result else { + return [] + } + defer { freeaddrinfo(head) } + + var addresses: [IPAddress] = [] + var entry: UnsafeMutablePointer? = head + while let current = entry { + if let socketAddress = current.pointee.ai_addr { + switch Int32(socketAddress.pointee.sa_family) { + case AF_INET: + socketAddress.withMemoryRebound(to: sockaddr_in.self, capacity: 1) { + addresses.append(.v4(UInt32(bigEndian: $0.pointee.sin_addr.s_addr))) + } + case AF_INET6: + socketAddress.withMemoryRebound(to: sockaddr_in6.self, capacity: 1) { + addresses.append(ipAddress(from: $0.pointee.sin6_addr)) + } + default: + break + } + } + entry = current.pointee.ai_next + } + return addresses + } + + /// `URLComponents.host` keeps the brackets around an IPv6 literal, the way `URI.getHost` does; + /// `inet_pton` and `getaddrinfo` reject them. + private static func bracketless(_ host: String) -> String { + guard host.count > 2, host.hasPrefix("["), host.hasSuffix("]") else { + return host + } + return String(host.dropFirst().dropLast()) + } + + private static let blockedHosts: Set = [ + "metadata.google.internal", + "metadata.goog", + ] +} diff --git a/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift b/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift new file mode 100644 index 00000000..a6b41bd6 --- /dev/null +++ b/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift @@ -0,0 +1,129 @@ +import Testing + +@testable import NitroMapsRemoteImagePolicy + +/// Runs the pre-check, which classifies a numeric host literal without a DNS lookup — so every +/// assertion in this file stays off the network. +private func rejectReason(_ uri: String, isBundled: Bool = false) -> String? { + RemoteMarkerUriPolicy.rejectReason(uri: uri, isBundled: isBundled, resolveHostAddress: false) +} + +@Test +func treatsOnlyHttpUrisAsRemote() { + #expect(RemoteMarkerUriPolicy.isRemoteUri("http://example.com/pin.png")) + #expect(RemoteMarkerUriPolicy.isRemoteUri("HTTPS://example.com/pin.png")) + #expect(!RemoteMarkerUriPolicy.isRemoteUri("file:///data/pin.png")) + #expect(!RemoteMarkerUriPolicy.isRemoteUri("pin")) + #expect(!RemoteMarkerUriPolicy.isRemoteUri("asset:/pin.png")) +} + +@Test +func rejectsPrivateAddressesTheDeveloperMachineCouldBeReachedAt() { + // Every development setup lands on one of these: the simulator sharing the host's loopback, + // a device on the same Wi-Fi, and the Android emulator's host alias. + #expect(rejectReason("http://127.0.0.1:8081/assets/pin.png") == "host not allowlisted") + #expect(rejectReason("http://192.168.1.14:8081/assets/pin.png") == "host not allowlisted") + #expect(rejectReason("http://10.0.2.2:8095/assets/pin.png") == "host not allowlisted") + #expect(rejectReason("http://172.16.0.1/pin.png") == "host not allowlisted") + #expect(rejectReason("http://0.0.0.0/pin.png") == "host not allowlisted") + #expect(rejectReason("http://239.1.2.3/pin.png") == "host not allowlisted") +} + +@Test +func rejectsLinkLocalAndMetadataEndpoints() { + #expect(rejectReason("http://169.254.169.254/latest/meta-data/") == "host not allowlisted") + #expect( + rejectReason("http://metadata.google.internal/computeMetadata/v1/") == "host not allowlisted") + #expect(rejectReason("http://localhost/pin.png") == "host not allowlisted") + #expect(rejectReason("http://printer.local/pin.png") == "host not allowlisted") + #expect(rejectReason("http://metro.localhost/pin.png") == "host not allowlisted") +} + +@Test +func rejectsPrivateIPv6Literals() { + #expect(rejectReason("http://[::1]:8081/pin.png") == "host not allowlisted") + #expect(rejectReason("http://[::]/pin.png") == "host not allowlisted") + #expect(rejectReason("http://[fe80::1]/pin.png") == "host not allowlisted") + #expect(rejectReason("http://[fec0::1]/pin.png") == "host not allowlisted") + #expect(rejectReason("http://[fd12:3456::1]/pin.png") == "host not allowlisted") + #expect(rejectReason("http://[ff02::1]/pin.png") == "host not allowlisted") +} + +@Test +func rejectsAPrivateIPv4AddressWearingAnIPv6Shape() { + // `::ffff:127.0.0.1` matches no IPv6 rule, and every stack routes it to 127.0.0.1. + #expect(rejectReason("http://[::ffff:127.0.0.1]/pin.png") == "host not allowlisted") + #expect(rejectReason("http://[::ffff:10.0.2.2]/pin.png") == "host not allowlisted") + #expect(rejectReason("http://[::192.168.1.14]/pin.png") == "host not allowlisted") +} + +@Test +func rejectsAPrivateHostHiddenByPercentEncoding() { + // `URLComponents.host` decodes, so the address the request would reach is the one classified. + #expect(rejectReason("http://%31%30.0.2.2/pin.png") == "host not allowlisted") + #expect(rejectReason("http://%6c%6f%63%61%6c%68%6f%73%74/pin.png") == "host not allowlisted") +} + +@Test +func rejectsMalformedAndCredentialBearingUris() { + #expect(rejectReason("ftp://example.com/pin.png") == "unsupported scheme") + #expect(rejectReason("//example.com/pin.png") == "missing scheme") + #expect(rejectReason("http:///pin.png") == "missing host") + #expect(rejectReason("http://exa mple.com/pin.png") == "invalid URI") + #expect(rejectReason("https://user:secret@example.com/pin.png") == "user info not allowed") + #expect(rejectReason("https://:secret@example.com/pin.png") == "user info not allowed") +} + +@Test +func allowsAPublicHost() { + #expect(rejectReason("https://cdn.example.com/pin.png") == nil) + #expect(rejectReason("https://93.184.216.34/pin.png") == nil) + #expect(rejectReason("https://[2606:2800:220:1:248:1893:25c8:1946]/pin.png") == nil) +} + +@Test +func 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. + #expect( + rejectReason("http://localhost:8081/assets/?unstable_path=./assets/pin.png", isBundled: true) + == nil) +} + +@Test +func appliesTheHostPolicyToAnImageTheAppDidNotBundle() { + #expect( + rejectReason("http://localhost:8081/assets/?unstable_path=./assets/pin.png") + == "host not allowlisted") +} + +@Test +func classifiesANumericLiteralWithoutDeferringToDns() { + // The resolving pass answers a literal the same way the pre-check does, so a reachable + // private address never reaches the network even once the caller is off the main thread. + #expect( + RemoteMarkerUriPolicy.rejectReason( + uri: "http://127.0.0.1:8081/pin.png", isBundled: false, resolveHostAddress: true) + == "host not allowlisted") + #expect( + RemoteMarkerUriPolicy.rejectReason( + uri: "http://[fd00::1]/pin.png", isBundled: false, resolveHostAddress: true) + == "host not allowlisted") +} + +@Test +func classifiesWhatTheResolverAnswers() { + // A numeric literal is answered by `getaddrinfo` without a DNS query, so this exercises the + // real resolver path — sockaddr parsing and byte order included — while staying off the + // network. Reading `sin_addr` in the wrong order would let 10.0.2.2 through as 2.2.0.10. + #expect(!RemoteMarkerUriPolicy.isAllowlistedResolvedHost("127.0.0.1")) + #expect(!RemoteMarkerUriPolicy.isAllowlistedResolvedHost("10.0.2.2")) + #expect(!RemoteMarkerUriPolicy.isAllowlistedResolvedHost("192.168.1.14")) + #expect(RemoteMarkerUriPolicy.isAllowlistedResolvedHost("93.184.216.34")) + + #expect(!RemoteMarkerUriPolicy.isAllowlistedResolvedHost("::1")) + #expect(!RemoteMarkerUriPolicy.isAllowlistedResolvedHost("[::1]")) + #expect(!RemoteMarkerUriPolicy.isAllowlistedResolvedHost("fd00::1")) + #expect(!RemoteMarkerUriPolicy.isAllowlistedResolvedHost("::ffff:10.0.2.2")) + #expect(RemoteMarkerUriPolicy.isAllowlistedResolvedHost("2606:2800:220:1:248:1893:25c8:1946")) +} diff --git a/package/iosTests/GoogleMarkerVisualApplierTests.swift b/package/iosTests/GoogleMarkerVisualApplierTests.swift index 72383830..3f5f1c14 100644 --- a/package/iosTests/GoogleMarkerVisualApplierTests.swift +++ b/package/iosTests/GoogleMarkerVisualApplierTests.swift @@ -17,7 +17,8 @@ final class GoogleMarkerVisualApplierTests: XCTestCase { uri: "https://example.test/pin.png", width: 40, height: 40, - scale: 1 + scale: 1, + origin: nil ) applier.apply(markerDescriptor(image: image, anchor: MarkerAnchor(x: 0, y: 0)), to: marker) @@ -46,13 +47,15 @@ final class GoogleMarkerVisualApplierTests: XCTestCase { uri: "https://example.test/a.png", width: 40, height: 40, - scale: 1 + scale: 1, + origin: nil ) let imageB = MarkerImage( uri: "https://example.test/b.png", width: 40, height: 40, - scale: 1 + scale: 1, + origin: nil ) let applier = GoogleMarkerVisualApplier( cachedImage: { image in image.uri.hasSuffix("a.png") ? iconA : nil }, From 86e6642f74c56352b90a93d8cabeea1b7996c8d1 Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Tue, 22 Sep 2026 18:27:25 +0200 Subject: [PATCH 3/8] fix(ios): check redirect destinations and stop logging whole URIs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from review on the host policy. The policy checked only the URL the app supplied. `URLSession.shared` follows redirects on its own, so an attacker-controlled public host answering 302 to `169.254.169.254` walked straight past it — the cheapest possible bypass, and one that made the rest of the check close to decorative. The loader now owns a session with a delegate that re-runs the policy on every redirect destination and refuses the hop. A bundled image is exempt there too, carried on the task rather than the image, because the delegate sees tasks. The rejection log wrote the whole URI as public. A rejected URI can carry the very thing that got it rejected — `user info not allowed` is one of the reasons — and a query string can hold a signed token. It is now logged twice: stripped of credentials, query and fragment as public, and in full as private. The full half matters because Metro puts the asset's identity in the query string, so the stripped form alone does not say which image was refused; the unified log keeps it out of a shipped app while a developer on the simulator or a debugger still reads it. Not addressed: binding the fetch to the address that passed the check. The gap is real — the name is resolved once to classify it and again to connect — but closing it means giving up URLSession for a hand-rolled connection with its own TLS verification, on both platforms. That is a larger change than this one and it belongs in its own issue. --- README.md | 4 ++ package/ios/MarkerImageLoader.swift | 71 +++++++++++++++++-- .../RemoteMarkerUriPolicy.swift | 16 +++++ .../RemoteMarkerUriPolicyTests.swift | 28 ++++++++ 4 files changed, 115 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index c8e3f869..59d7711b 100644 --- a/README.md +++ b/README.md @@ -379,6 +379,10 @@ host must not resolve to a private address: loopback, any-local, link-local, sit 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. +On iOS every redirect destination is checked the same way, so a permitted host cannot forward the +fetch to a blocked one. Android follows redirects without re-checking them; until that is fixed, +treat the Android check as covering the URL you pass, not the address finally reached. + A rejected URL logs `Rejected remote marker image URI` — under the `NitroMaps` logcat tag on Android, and under the `NitroMaps` category of the `com.nitromaps` subsystem in the unified log on iOS. The marker itself falls back to the default pin on both Google providers. On Apple Maps diff --git a/package/ios/MarkerImageLoader.swift b/package/ios/MarkerImageLoader.swift index c5634c4a..91840a9d 100644 --- a/package/ios/MarkerImageLoader.swift +++ b/package/ios/MarkerImageLoader.swift @@ -5,7 +5,16 @@ import UIKit /// Local images decode off-thread; completions always land on main. enum MarkerImageLoader { private static let cache = NSCache() - private static let session = URLSession.shared + /// Not `URLSession.shared`, which cannot carry a delegate: without one the policy would check + /// the URL the app supplied and then follow a redirect anywhere, which is the cheapest way to + /// defeat it — an attacker-controlled public host answering 302 to a private address. + private static let session: URLSession = { + URLSession( + configuration: .default, + delegate: RedirectPolicyDelegate(), + delegateQueue: nil + ) + }() private static let decodeQueue = DispatchQueue( label: "com.nitromaps.markerImageDecode", qos: .userInitiated @@ -135,8 +144,20 @@ enum MarkerImageLoader { } } + /// Logged twice on purpose. A rejected URI can carry the very thing that got it rejected — + /// `user:password@` is one of the reasons — and a query string can hold a signed token, so the + /// public half is stripped of both. The full URI follows as private: the unified log keeps it + /// out of a shipped app's logs, and a developer attached to the simulator or a debugger still + /// sees it. That matters because Metro puts the asset's identity in the query string, so the + /// stripped form alone does not say which image was refused. private static func logRejected(uri: String, reason: String) { - logger.warning("Rejected remote marker image URI (\(reason, privacy: .public)): \(uri, privacy: .public)") + logger.warning( + """ + Rejected remote marker image URI (\(reason, privacy: .public)): \ + \(RemoteMarkerUriPolicy.loggableURI(uri), privacy: .public) \ + [full: \(uri, privacy: .private)] + """ + ) } private static func startRemoteTask( @@ -145,7 +166,7 @@ enum MarkerImageLoader { image: MarkerImage, completion: @escaping (UIImage?) -> Void ) { - session.dataTask(with: url) { data, _, _ in + let task = session.dataTask(with: url) { data, _, _ in let uiImage: UIImage? if let data, let decoded = UIImage(data: data) { uiImage = resize(decoded, image: image) @@ -159,7 +180,49 @@ enum MarkerImageLoader { DispatchQueue.main.async { completion(uiImage) } - }.resume() + } + // The delegate sees tasks, not images, so the exemption has to travel with the task. + task.taskDescription = image.origin == .bundled ? bundledTaskDescription : nil + task.resume() + } + + private static let bundledTaskDescription = "com.nitromaps.bundledMarkerImage" + + /// Re-runs the host policy on every redirect destination. Without it the policy would only ever + /// see the URL the app supplied. + private final class RedirectPolicyDelegate: NSObject, URLSessionTaskDelegate { + func urlSession( + _ session: URLSession, + task: URLSessionTask, + willPerformHTTPRedirection response: HTTPURLResponse, + newRequest request: URLRequest, + completionHandler: @escaping (URLRequest?) -> Void + ) { + if task.taskDescription == MarkerImageLoader.bundledTaskDescription { + completionHandler(request) + return + } + + guard let uri = request.url?.absoluteString else { + completionHandler(nil) + return + } + + // Off the delegate queue: resolving blocks, and this queue also delivers completions. + MarkerImageLoader.policyQueue.async { + if let reason = RemoteMarkerUriPolicy.rejectReason( + uri: uri, + isBundled: false, + resolveHostAddress: true + ) { + MarkerImageLoader.logRejected(uri: uri, reason: "redirect: \(reason)") + // `nil` stops the redirect; the task completes with the redirect response body. + completionHandler(nil) + return + } + completionHandler(request) + } + } } private static func resize(_ uiImage: UIImage, image: MarkerImage) -> UIImage { diff --git a/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift b/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift index dbde3b20..508f51aa 100644 --- a/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift +++ b/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift @@ -66,6 +66,22 @@ enum RemoteMarkerUriPolicy { return allowed ? nil : "host not allowlisted" } + /// The parts of a rejected URI that are safe to write to the log. + /// + /// A rejected URI can carry the very thing that got it rejected — `user:password@` is one of the + /// reasons — and a query string can hold a signed token. What is left identifies which image + /// failed, which is what a developer reading the log needs. + static func loggableURI(_ uriString: String) -> String { + guard var components = URLComponents(string: uriString) else { + return "" + } + components.user = nil + components.password = nil + components.query = nil + components.fragment = nil + return components.string ?? "" + } + /// The resolving half of the host check, without the URL parsing around it. /// /// Blocking: only call it off the thread the caller came in on. diff --git a/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift b/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift index a6b41bd6..776e0134 100644 --- a/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift +++ b/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift @@ -127,3 +127,31 @@ func classifiesWhatTheResolverAnswers() { #expect(!RemoteMarkerUriPolicy.isAllowlistedResolvedHost("::ffff:10.0.2.2")) #expect(RemoteMarkerUriPolicy.isAllowlistedResolvedHost("2606:2800:220:1:248:1893:25c8:1946")) } + +@Test +func classifiesARedirectDestinationTheSameWayAsAnOriginalUri() { + // A redirect destination goes through the same call the delegate makes, so an attacker-controlled + // public host answering 302 to a private address gains nothing over naming it directly. + #expect(rejectReason("http://169.254.169.254/latest/meta-data/") == "host not allowlisted") + #expect(rejectReason("http://127.0.0.1/pin.png") == "host not allowlisted") + #expect(rejectReason("ftp://example.com/pin.png") == "unsupported scheme") + #expect(rejectReason("https://cdn.example.com/pin.png") == nil) +} + +@Test +func stripsCredentialsAndQueryFromALoggedUri() { + #expect( + RemoteMarkerUriPolicy.loggableURI("https://user:secret@example.com/pin.png") + == "https://example.com/pin.png") + #expect( + RemoteMarkerUriPolicy.loggableURI("https://example.com/pin.png?token=abc123&x=1") + == "https://example.com/pin.png") + #expect( + RemoteMarkerUriPolicy.loggableURI("https://example.com/pin.png#frag") + == "https://example.com/pin.png") + // The part a developer needs to recognise the image survives untouched. + #expect( + RemoteMarkerUriPolicy.loggableURI("http://10.0.2.2:8095/assets/pin.png") + == "http://10.0.2.2:8095/assets/pin.png") + #expect(RemoteMarkerUriPolicy.loggableURI("http://exa mple.com/x") == "") +} From bc8c09537b23309765c12a483495e19b17fd30e1 Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Sat, 26 Sep 2026 20:41:31 +0200 Subject: [PATCH 4/8] 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 5/8] 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 6/8] 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. From 49aaebea153ce92f88f26f49fe8323174f5ef7e7 Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Sun, 27 Sep 2026 14:05:34 +0200 Subject: [PATCH 7/8] fix(ios): keep marker image redirects on http(s), and never down from https Android refuses a redirect that leaves http(s) or goes from https to http, for a bundled image too, and so does RemoteMarkerUriPolicy.redirectRejectReason in Kotlin. iOS re-checked the host of each redirect but followed a downgrade, so the two platforms answered the same redirect differently. RemoteMarkerUriPolicy.redirectRejectReason(from:to:) now makes that decision in Swift, with the same reasons, and the redirect delegate asks it first, before the bundled exemption and the host check. --- package/ios/MarkerImageLoader.swift | 16 ++++++++++++--- .../RemoteMarkerUriPolicy.swift | 17 ++++++++++++++++ .../RemoteMarkerUriPolicyTests.swift | 20 +++++++++++++++++++ 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/package/ios/MarkerImageLoader.swift b/package/ios/MarkerImageLoader.swift index e2785e06..1a5338d3 100644 --- a/package/ios/MarkerImageLoader.swift +++ b/package/ios/MarkerImageLoader.swift @@ -205,16 +205,26 @@ enum MarkerImageLoader { newRequest request: URLRequest, completionHandler: @escaping (URLRequest?) -> Void ) { - if task.taskDescription == MarkerImageLoader.bundledTaskDescription { - completionHandler(request) + guard let destination = request.url else { + completionHandler(nil) return } - guard let uri = request.url?.absoluteString else { + if let source = response.url, + let reason = RemoteMarkerUriPolicy.redirectRejectReason(from: source, to: destination) + { + MarkerImageLoader.logRejected(uri: destination.absoluteString, reason: reason) completionHandler(nil) return } + if task.taskDescription == MarkerImageLoader.bundledTaskDescription { + completionHandler(request) + return + } + + let uri = destination.absoluteString + // Off the delegate queue: resolving blocks, and this queue also delivers completions. MarkerImageLoader.policyQueue.async { if let reason = RemoteMarkerUriPolicy.rejectReason( diff --git a/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift b/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift index 508f51aa..815daf0e 100644 --- a/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift +++ b/package/ios/RemoteImagePolicy/RemoteMarkerUriPolicy.swift @@ -66,6 +66,23 @@ enum RemoteMarkerUriPolicy { return allowed ? nil : "host not allowlisted" } + /// Returns why a redirect from `source` to `destination` must not be followed, or `nil` when it + /// may be. + /// + /// This holds for a bundled image too: a redirect stays on `http`/`https`, and never goes from + /// `https` down to `http`. The destination of a user-supplied image still has to pass + /// `rejectReason(uri:isBundled:resolveHostAddress:)`, like any other URI. + static func redirectRejectReason(from source: URL, to destination: URL) -> String? { + let scheme = destination.scheme?.lowercased() + if scheme != "http" && scheme != "https" { + return "redirect to an unsupported scheme" + } + if scheme == "http" && source.scheme?.lowercased() == "https" { + return "redirect from https to http" + } + return nil + } + /// The parts of a rejected URI that are safe to write to the log. /// /// A rejected URI can carry the very thing that got it rejected — `user:password@` is one of the diff --git a/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift b/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift index 776e0134..3713927e 100644 --- a/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift +++ b/package/ios/Tests/RemoteImagePolicy/RemoteMarkerUriPolicyTests.swift @@ -1,3 +1,4 @@ +import Foundation import Testing @testable import NitroMapsRemoteImagePolicy @@ -138,6 +139,25 @@ func classifiesARedirectDestinationTheSameWayAsAnOriginalUri() { #expect(rejectReason("https://cdn.example.com/pin.png") == nil) } +@Test +func followsARedirectOnlyOnHttpAndNeverDownToIt() { + func reason(from source: String, to destination: String) -> String? { + RemoteMarkerUriPolicy.redirectRejectReason( + from: URL(string: source)!, + to: URL(string: destination)! + ) + } + + #expect(reason(from: "http://cdn.example.com/pin.png", to: "https://cdn.example.com/pin.png") == nil) + #expect(reason(from: "https://cdn.example.com/pin.png", to: "https://img.example.com/pin.png") == nil) + #expect( + reason(from: "https://cdn.example.com/pin.png", to: "http://cdn.example.com/pin.png") + == "redirect from https to http") + #expect( + reason(from: "http://cdn.example.com/pin.png", to: "file:///var/mobile/pin.png") + == "redirect to an unsupported scheme") +} + @Test func stripsCredentialsAndQueryFromALoggedUri() { #expect( From 317e7ce465ccbb1151fdcf9de8af701eadd3930e Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Sun, 27 Sep 2026 14:11:22 +0200 Subject: [PATCH 8/8] docs(ios): say where the marker image host check stops on iOS URLSession resolves a host again to connect and offers no hook between that lookup and the request, so iOS cannot re-check the connected address the way Android does. Say so in the README and next to the fetch, together with what still covers the device's own network: the user's Local Network permission. --- README.md | 3 ++- package/ios/MarkerImageLoader.swift | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index c121c853..7df6e118 100644 --- a/README.md +++ b/README.md @@ -528,7 +528,8 @@ marker icon URL arriving as data from an API. Android also checks the address the connection is actually made to, before it sends the request. iOS checks the name only: `URLSession` resolves it again to connect, so a DNS answer that changes -in between is not re-checked there. +in between is not re-checked there. Reaching the network the device itself is on still needs the +user's Local Network permission on iOS. A rejected URL logs `Rejected remote marker image URI` — under the `NitroMaps` logcat tag on Android, and under the `NitroMaps` category of the `com.nitromaps` subsystem in the unified log diff --git a/package/ios/MarkerImageLoader.swift b/package/ios/MarkerImageLoader.swift index 1a5338d3..3eb5974e 100644 --- a/package/ios/MarkerImageLoader.swift +++ b/package/ios/MarkerImageLoader.swift @@ -147,6 +147,10 @@ enum MarkerImageLoader { return } + // URLSession resolves the host again to connect, and it has no hook between that lookup and + // the request. So unlike Android, which checks the address it connected to, an answer that + // changes after the check above (DNS rebinding) is not re-checked. Reaching the network the + // device itself is on still needs the user's Local Network permission. startRemoteTask(url: url, cacheKey: cacheKey, image: image, completion: completion) } }