Harden SSRF guard and public-key substitution defenses#2107
Open
bruschill wants to merge 4 commits into
Open
Conversation
SSRF (MapDataManager.importFromRemote — reachable from the untrusted importGeoJSON deep link): - Re-validate every HTTP redirect hop's host so a public allow-listed URL can't 302 the request onto localhost/LAN/169.254.169.254. - Discard the response if the connection lands on an internal peer (DNS-rebinding defense-in-depth via transaction metrics). - Add RFC 5737 test-nets and 255.255.255.255/32 to the IPv4 denylist. - Block the NAT64 well-known prefix 64:ff9b::/96 by re-checking the embedded IPv4. Public-key substitution / MITM (node ingestion): - Extract first-wins key handling into UserEntity.applyInboundPublicKey and route all three ingestion paths (UpdateSwiftData NodeInfo + User, MeshPackets NodeInfo) through it. - On a differing inbound key, keep the trusted stored key AND surface the attempt to the UI (keyMatch=false, newPublicKey=inbound) instead of only logging — drives the red key.slash indicator across the node views. - Fix the divergent `publicKey == nil` guard in MeshPackets that previously ignored mismatches silently. Tests: - 21-case host/IP denylist suite + file:// rejection (fixes a stale test that asserted the opposite contract). - 6-case suite for the key-substitution helper.
Contributor
📝 WalkthroughWalkthroughRemote map imports now enforce HTTP(S) destinations and SSRF protections. Inbound mesh public-key updates use shared first-wins handling across packet and persistence paths, with mismatch tracking and security logging. ChangesRemote import security
Inbound public-key security
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MapDataManager
participant URLSession
participant SSRFGuardDelegate
Client->>MapDataManager: Request remote map import
MapDataManager->>URLSession: Stream guarded HTTP(S) download
URLSession->>SSRFGuardDelegate: Validate redirects and peer metrics
SSRFGuardDelegate-->>URLSession: Allow or reject destination
URLSession-->>MapDataManager: Return bounded response
MapDataManager-->>Client: Import metadata or disallowed-host error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
MeshtasticTests/MapDataManagerTests.swift (1)
139-145: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a public IPv6 control case.
This allow-list test covers only
AF_INET. Include a globally routable IPv6 literal to ensure IPv6 byte extraction and denylist changes do not reject all IPv6 destinations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MeshtasticTests/MapDataManagerTests.swift` around lines 139 - 145, Add a globally routable IPv6 literal to the arguments of allowsPublicHosts in the public-host allow-list test, preserving the existing IPv4 cases and expectation so IPv6 destinations are verified as allowed.Meshtastic/Helpers/MapDataManager.swift (1)
112-155: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer configuration injection here.
session: injectedSessionbypassesSSRFGuardDelegate, so a caller can skip redirect and peer checks entirely. If this hook is only for tests, pass aURLSessionConfiguration/URLProtocolstub and always construct the session withguardDelegate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meshtastic/Helpers/MapDataManager.swift` around lines 112 - 155, Update importFromRemote so callers cannot supply a URLSession that bypasses SSRFGuardDelegate; replace the injectedSession hook with test-oriented URLSessionConfiguration/URLProtocol injection, while always constructing the session with guardDelegate and retaining the existing cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Meshtastic/Helpers/MapDataManager.swift`:
- Around line 595-619: Update isDisallowedIPv6 to reject deprecated site-local
fec0::/10 and multicast ff00::/8 addresses, alongside the existing
special-purpose range checks. Add regression cases covering representative
addresses from both ranges and verify they are classified as disallowed.
- Line 157: Update the remote download flow around the session.data call to use
streaming via bytes(from:) or an equivalent delegate, accumulating the response
bytes while enforcing maxFileSize during reads. Stop reading and reject the
download immediately once the accumulated size exceeds maxFileSize, then
preserve the existing response handling for valid files.
- Around line 669-681: Update the transaction loop in
urlSession(_:task:didFinishCollecting:) to skip transactions whose
isProxyConnection is true before reading remoteAddress or calling
MapDataManager.isDisallowedHost. Continue evaluating non-proxy transactions with
the existing peer validation and flagging behavior.
In `@MeshtasticTests/MapDataManagerTests.swift`:
- Around line 93-104: The rejectsLocalFileURL test should verify the specific
MapDataError.disallowedHost case rather than accepting any MapDataError. Update
the `#expect` assertion around MapDataManager.importFromRemote to pattern-match
the thrown error as .disallowedHost while preserving the existing file URL setup
and session.
---
Nitpick comments:
In `@Meshtastic/Helpers/MapDataManager.swift`:
- Around line 112-155: Update importFromRemote so callers cannot supply a
URLSession that bypasses SSRFGuardDelegate; replace the injectedSession hook
with test-oriented URLSessionConfiguration/URLProtocol injection, while always
constructing the session with guardDelegate and retaining the existing cleanup
behavior.
In `@MeshtasticTests/MapDataManagerTests.swift`:
- Around line 139-145: Add a globally routable IPv6 literal to the arguments of
allowsPublicHosts in the public-host allow-list test, preserving the existing
IPv4 cases and expectation so IPv6 destinations are verified as allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d934745-06b7-4035-b067-f3a2595b5c8d
📒 Files selected for processing (8)
Meshtastic/Helpers/MapDataManager.swiftMeshtastic/Helpers/MeshPackets.swiftMeshtastic/Model/UserEntity.swiftMeshtastic/Persistence/NodeBackupManager+Import.swiftMeshtastic/Persistence/UpdateSwiftData.swiftMeshtastic/Views/Settings/Config/SecurityConfig.swiftMeshtasticTests/MapDataManagerTests.swiftMeshtasticTests/UserEntityPublicKeyTests.swift
Make SSRFGuardDelegate internal (was private) so its redirect control can be driven directly, and add SSRFGuardDelegateRedirectTests: a 302 to loopback / link-local (169.254.169.254) / private LAN is refused, a 302 to a public host is followed, and the peer flag defaults false. Hermetic (IP literals only).
thebentern
approved these changes
Jul 17, 2026
- Stream remote GeoJSON downloads via bytes(from:) and abort once the running total exceeds maxFileSize, with an early reject on an honest oversized Content-Length (avoids buffering an attacker-sized body). - Inject only a URLSessionConfiguration so the session is always built with SSRFGuardDelegate; callers can no longer bypass the guard. - Reject deprecated site-local (fec0::/10) and multicast (ff00::/8) IPv6. - Skip proxied transactions before the remoteAddress peer check. - Tests: assert .disallowedHost specifically for file:// rejection, add public IPv6 control cases, stub advertises Content-Length.
bruschill
added
the
skip-docs-check
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
Defense-in-depth hardening for two areas of the map-overlay import and node-ingestion paths. Focused on tightening validation and making an existing trust signal visible; exploit specifics intentionally omitted here.
1. Overlay fetch —
MapDataManager.importFromRemoteThis path is reachable from the untrusted
importGeoJSONdeep link, so it now applies stricter validation of the fetch target:http(s)schemes outright.2. Inbound public-key handling — node ingestion
Aligns all three ingestion paths (UpdateSwiftData NodeInfo + User, MeshPackets NodeInfo) with the project's documented TOFU model:
UserEntity.applyInboundPublicKey(_:nodeNum:).keyMatch = false,newPublicKey) — driving the existing key-change indicator — instead of being silently dropped on those paths.publicKey == nilguard inMeshPacketsthat didn't flag mismatches.SecurityConfig,NodeBackupManagerrestore) as intended/exempt, consistent with the documented local trust boundary.Why did it change?
Both areas were partial-trust boundaries that could be tightened. The overlay fetch is reachable from an untrusted deep link and could be steered toward internal/reserved hosts (SSRF); the redirect-hop host re-validation plus the expanded denylist close that. On the ingestion side, two of the three paths silently dropped a mismatching inbound public key instead of surfacing it, diverging from the documented TOFU/first-wins model — so a key change on those paths was invisible to the user. This unifies the behavior and makes the trust signal visible.
How is this tested?
http(s)rejection test (replaces a stale test that asserted the opposite contract).xcodebuild build-for-testing+ targeted test runs — all passing.Screenshots/Videos (when applicable)
N/A — no UI layout change. The key-change path drives the existing key-mismatch indicator; no new screens or labels.
Checklist
docs/user/ordocs/developer/. No doc update is needed (no user-visible UI/label/navigation change) —skip-docs-checklabel applies.Summary by CodeRabbit
file://(and other unsupported URL schemes) from being used for remote map imports.