From 23b7b81be01f41e3377d4089b4c187d7e5de2841 Mon Sep 17 00:00:00 2001 From: David Monagle Date: Tue, 4 Aug 2026 08:05:21 +1000 Subject: [PATCH 1/3] Reject interface-scoped addresses in NetworkAddress, prefer IPv4 on resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NWEndpoint's resolved remote address carries a % scope whenever a path is tied to a specific local interface — routine on any multi-interface host, for both IPv4 and IPv6, not just a link-local-IPv6 edge case. That scoped string isn't a valid URL host, and every URL-based transport built on this library (acumen-shelly's HTTP/WebSocket transports) crashed the whole process outright when it reached one, rather than failing the one resolve. Confirmed against real hardware: a Mac with two active interfaces resolved a perfectly reachable Shelly device to "192.168.173.104%en1". NetworkAddress(endpoint:) now rejects any scoped address instead of handing it out, which surfaces as an ordinary resolveFailed the setup flow already treats as "fall back to the .local hostname" — resolved fresh and unscoped at actual connection time. Unscoped IPv6 is now properly bracketed per RFC 3986. resolve() also tries an IPv4-only constrained connection first (bounded by a 2s timeout, since a no-viable-path NWConnection commonly parks in .waiting rather than .failed) before falling back to an unconstrained attempt. --- Sources/MDNSApple/AppleServiceDiscovery.swift | 113 +++++++++++++----- .../MDNSApple/NWEndpoint+NetworkAddress.swift | 26 +++- .../MDNSAppleTests/NetworkAddressTests.swift | 85 +++++++++++++ 3 files changed, 194 insertions(+), 30 deletions(-) create mode 100644 Tests/MDNSAppleTests/NetworkAddressTests.swift diff --git a/Sources/MDNSApple/AppleServiceDiscovery.swift b/Sources/MDNSApple/AppleServiceDiscovery.swift index e0e32a9..25c5180 100644 --- a/Sources/MDNSApple/AppleServiceDiscovery.swift +++ b/Sources/MDNSApple/AppleServiceDiscovery.swift @@ -219,47 +219,102 @@ public final class AppleServiceDiscovery: ServiceDiscovery, @unchecked Sendable } } + /// How long an IPv4-constrained resolve attempt gets before falling back + /// to an unconstrained one. Needed because an `NWConnection` with no + /// viable path for its constraint (e.g. an IPv4-only connection to a + /// device with no IPv4 address) commonly parks in `.waiting` rather than + /// transitioning to `.failed` — without a deadline, ``resolve(_:)`` + /// would simply hang for an IPv6-only device instead of falling back. + static let ipv4PreferenceTimeout: Duration = .seconds(2) + public func resolve(_ record: ServiceRecord) async throws -> NetworkAddress { - // Create a connection to the browsed service to trigger resolution let endpoint = NWEndpoint.service( name: record.name, type: record.serviceType.rawValue, domain: "local.", interface: nil ) - let connection = NWConnection(to: endpoint, using: .udp) - return try await withCheckedThrowingContinuation { (cont: CheckedContinuation) in - nonisolated(unsafe) var resumed = false - connection.stateUpdateHandler = { [weak self] state in - guard let self else { return } - guard !resumed else { return } - switch state { - case .ready: - if let remoteEndpoint = connection.currentPath?.remoteEndpoint, - let address = NetworkAddress(endpoint: remoteEndpoint) { - resumed = true - cont.resume(returning: address) - } else { - resumed = true - cont.resume(throwing: ServiceDiscoveryError.resolveFailed(record.name)) + // Prefer IPv4: most LAN smart-home devices this library targets + // (Shelly, Hue, Matter accessories) are IPv4-first in practice, and + // an unscoped IPv6 candidate isn't guaranteed to exist or work end + // to end even when mDNS opportunistically announces one. If no + // IPv4-constrained attempt succeeds in time, fall through to an + // unconstrained attempt — whatever `currentPath.remoteEndpoint` + // resolves to. Either attempt can still yield a scoped address, + // which `NetworkAddress(endpoint:)` rejects — see its doc comment. + let ipv4Parameters = NWParameters.udp + if let ipOptions = ipv4Parameters.defaultProtocolStack.internetProtocol as? NWProtocolIP.Options { + ipOptions.version = .v4 + } + if let address = try? await resolveOnce( + endpoint: endpoint, + parameters: ipv4Parameters, + recordName: record.name, + timeout: Self.ipv4PreferenceTimeout + ) { + return address + } + + return try await resolveOnce(endpoint: endpoint, parameters: .udp, recordName: record.name, timeout: nil) + } + + /// One resolve attempt over one `NWConnection`, optionally bounded by + /// `timeout` (used only for the IPv4-preference attempt — an + /// unconstrained fallback attempt has nothing further to fall back to, + /// so it runs until `NWConnection` itself reports `.ready`/`.failed`). + private func resolveOnce( + endpoint: NWEndpoint, + parameters: NWParameters, + recordName: String, + timeout: Duration? + ) async throws -> NetworkAddress { + let connection = NWConnection(to: endpoint, using: parameters) + + return try await withThrowingTaskGroup(of: NetworkAddress.self) { group in + group.addTask { + try await withCheckedThrowingContinuation { (cont: CheckedContinuation) in + nonisolated(unsafe) var resumed = false + connection.stateUpdateHandler = { [weak self] state in + guard let self else { return } + guard !resumed else { return } + switch state { + case .ready: + if let remoteEndpoint = connection.currentPath?.remoteEndpoint, + let address = NetworkAddress(endpoint: remoteEndpoint) { + resumed = true + cont.resume(returning: address) + } else { + resumed = true + cont.resume(throwing: ServiceDiscoveryError.resolveFailed(recordName)) + } + connection.cancel() + case .failed(let error): + self.logger.error("Resolve failed for '\(recordName)': \(error)") + resumed = true + cont.resume(throwing: error) + connection.cancel() + case .cancelled: + if !resumed { + resumed = true + cont.resume(throwing: CancellationError()) + } + default: + break + } } + connection.start(queue: self.queue) + } + } + if let timeout { + group.addTask { + try await Task.sleep(for: timeout) connection.cancel() - case .failed(let error): - self.logger.error("Resolve failed for '\(record.name)': \(error)") - resumed = true - cont.resume(throwing: error) - connection.cancel() - case .cancelled: - if !resumed { - resumed = true - cont.resume(throwing: CancellationError()) - } - default: - break + throw ServiceDiscoveryError.resolveFailed(recordName) } } - connection.start(queue: self.queue) + defer { group.cancelAll() } + return try await group.next()! } } diff --git a/Sources/MDNSApple/NWEndpoint+NetworkAddress.swift b/Sources/MDNSApple/NWEndpoint+NetworkAddress.swift index 02eb954..801d2c7 100644 --- a/Sources/MDNSApple/NWEndpoint+NetworkAddress.swift +++ b/Sources/MDNSApple/NWEndpoint+NetworkAddress.swift @@ -14,6 +14,25 @@ extension NetworkAddress { /// /// Only `.hostPort` endpoints are supported — returns `nil` for /// `.service`, `.unix`, `.url`, or other endpoint types. + /// + /// Also returns `nil` for an **interface-scoped** `.ipv4`/`.ipv6` + /// address (`addr.interface != nil` — both address families support + /// this, not just IPv6). Network.framework attaches a scope whenever a + /// resolved path is tied to a specific local interface (routine on a + /// multi-interface host — Wi-Fi + Ethernet, a VPN, a bridge — not just a + /// link-local-address edge case), and its string form appends a + /// `%` suffix (`"192.168.1.23%en1"`, + /// `"fe80::1%en0"`). That suffix is not valid inside a URL authority + /// component — `URLComponents` cannot parse it — and it is not portable + /// even where some caller could work around that: `en1` names an + /// interface on *this* process's view of the network, meaningless after + /// a restart, from a different host, or once persisted as a plain + /// string. Rejecting it here (rather than producing an address a + /// downstream URL-based transport cannot use) surfaces as an ordinary + /// `ServiceDiscoveryError.resolveFailed` from ``resolve(_:)``, which + /// callers already treat as "fall back to the plain `.local` hostname" — + /// resolved fresh, and unscoped, at actual connection time instead of + /// baked in here. public init?(endpoint: NWEndpoint) { guard case .hostPort(let host, let port) = endpoint else { return nil @@ -21,9 +40,14 @@ extension NetworkAddress { let hostString: String switch host { case .ipv4(let addr): + guard addr.interface == nil else { return nil } hostString = "\(addr)" case .ipv6(let addr): - hostString = "\(addr)" + guard addr.interface == nil else { return nil } + // Bracket per RFC 3986 — a bare IPv6 literal is not a valid URL + // host. Only reachable for a non-scoped address (global unicast + // or ULA); a scoped one was already rejected above. + hostString = "[\(addr)]" case .name(let name, _): hostString = name @unknown default: diff --git a/Tests/MDNSAppleTests/NetworkAddressTests.swift b/Tests/MDNSAppleTests/NetworkAddressTests.swift new file mode 100644 index 0000000..1459784 --- /dev/null +++ b/Tests/MDNSAppleTests/NetworkAddressTests.swift @@ -0,0 +1,85 @@ +// NetworkAddressTests.swift +// Copyright 2026 Monagle Pty Ltd + +import Testing +import Foundation +import MDNSCore + +#if canImport(Network) +import Network +@testable import MDNSApple + +/// `NetworkAddress(endpoint:)` used to hand back whatever raw string +/// `IPv4Address`/`IPv6Address` interpolation produced — including, for an +/// interface-scoped address (routine on any multi-interface host, not just +/// a link-local-IPv6 edge case), a `%` suffix that +/// `URLComponents` cannot parse at all. That broke every URL-based +/// transport built on top of this library with a crash, not a catchable +/// error — confirmed against real hardware, see `acumen-shelly`'s CLAUDE.md. +/// These pin down both the rejection of scoped addresses and the URL +/// safety of everything this initializer does still accept. +@Suite("NetworkAddress(endpoint:) never yields a host that breaks URL construction") +struct NetworkAddressTests { + @Test("a plain (unscoped) IPv4 endpoint resolves to a usable, URL-safe host") + func plainIPv4() throws { + let addr = try #require(IPv4Address("192.168.1.23")) + let endpoint = NWEndpoint.hostPort(host: .ipv4(addr), port: 80) + let address = try #require(NetworkAddress(endpoint: endpoint)) + #expect(address.host == "192.168.1.23") + #expect(address.port == 80) + try #require(urlBuilds(fromHost: address.host)) + } + + @Test("an interface-scoped IPv4 endpoint (e.g. \"192.168.1.23%en1\") is rejected") + func scopedIPv4Rejected() throws { + let addr = try #require(IPv4Address("192.168.1.23%en1")) + #expect(addr.interface != nil) // sanity: this really is what we think it is + let endpoint = NWEndpoint.hostPort(host: .ipv4(addr), port: 80) + #expect(NetworkAddress(endpoint: endpoint) == nil) + } + + @Test("a plain (unscoped) global/ULA IPv6 endpoint resolves to a bracketed, URL-safe host") + func plainIPv6() throws { + let addr = try #require(IPv6Address("fd00::1")) + let endpoint = NWEndpoint.hostPort(host: .ipv6(addr), port: 8080) + let address = try #require(NetworkAddress(endpoint: endpoint)) + #expect(address.host == "[fd00::1]") + try #require(urlBuilds(fromHost: address.host)) + } + + @Test("a link-local (interface-scoped) IPv6 endpoint is rejected, not bracketed as-is") + func scopedIPv6Rejected() throws { + let addr = try #require(IPv6Address("fe80::1234:5678:9abc:def0%en0")) + #expect(addr.interface != nil) + let endpoint = NWEndpoint.hostPort(host: .ipv6(addr), port: 80) + #expect(NetworkAddress(endpoint: endpoint) == nil) + } + + @Test("a .name host (an mDNS hostname target) passes through unchanged") + func nameHostPassesThrough() throws { + let endpoint = NWEndpoint.hostPort(host: .name("shelly1pmg4-abc123.local", nil), port: 80) + let address = try #require(NetworkAddress(endpoint: endpoint)) + #expect(address.host == "shelly1pmg4-abc123.local") + try #require(urlBuilds(fromHost: address.host)) + } + + @Test("a non-hostPort endpoint (e.g. .service) yields nil") + func nonHostPortEndpointRejected() { + let endpoint = NWEndpoint.service(name: "x", type: "_shelly._tcp", domain: "local.", interface: nil) + #expect(NetworkAddress(endpoint: endpoint) == nil) + } + + /// The actual regression guard: whatever `NetworkAddress(endpoint:)` + /// does hand back must be usable to build a real URL — this is the + /// exact construction `ShellyHTTPTransportConfiguration.makeURL()` (and + /// any future consumer) performs. + private func urlBuilds(fromHost host: String) -> Bool { + var components = URLComponents() + components.scheme = "http" + components.host = host + components.port = 80 + components.path = "/rpc" + return components.url != nil + } +} +#endif From bd38cccd637e5f7c56304e93c4fdd9d7e4d302a1 Mon Sep 17 00:00:00 2001 From: David Monagle Date: Tue, 4 Aug 2026 08:05:29 +1000 Subject: [PATCH 2/3] Bracket AAAA-derived hosts and prefer A over AAAA in Linux mDNS resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors the MDNSApple fix in the same PR, for the hand-rolled Linux backend: a resolved AAAA record formatted as a bare, unbracketed colon-separated IPv6 literal, which is not a valid URL host either. handleResponse also resolved on whichever record type it encountered first in a response with no preference, unlike MDNSApple's IPv4-first behavior. Extracted the pure formatting/ordering logic into LinuxServiceDiscovery.resolvableHosts(from:) — A records always ordered before AAAA regardless of packet order, AAAA formatted bracketed — so both fixes are unit-testable without a live socket. --- Sources/MDNSLinux/LinuxServiceDiscovery.swift | 92 ++++++++++++------- .../LinuxServiceDiscoveryResolveTests.swift | 83 +++++++++++++++++ 2 files changed, 144 insertions(+), 31 deletions(-) create mode 100644 Tests/MDNSLinuxTests/LinuxServiceDiscoveryResolveTests.swift diff --git a/Sources/MDNSLinux/LinuxServiceDiscovery.swift b/Sources/MDNSLinux/LinuxServiceDiscovery.swift index 6e6544b..128f9ae 100644 --- a/Sources/MDNSLinux/LinuxServiceDiscovery.swift +++ b/Sources/MDNSLinux/LinuxServiceDiscovery.swift @@ -252,40 +252,70 @@ public actor LinuxServiceDiscovery: ServiceDiscovery { } private func handleResponse(_ msg: DNSMessage) { - for record in msg.answers + msg.additionals { - switch record.rdata { - case .ptr(let domain): - let parts = record.name.split(separator: ".") - guard parts.count >= 3 else { continue } - // record.name is e.g. "_hap._tcp.local" — strip ".local" for lookup key - let serviceTypeKey = parts.dropLast().joined(separator: ".") - guard let conts = browseStreams[serviceTypeKey], !conts.isEmpty else { continue } - // domain is "InstanceName._hap._tcp.local" - let domainParts = domain.split(separator: ".") - let instanceName = domainParts.first.map(String.init) ?? domain - let serviceTypeParts = domainParts.dropFirst().dropLast().joined(separator: ".") - let discovered = ServiceRecord( - name: instanceName, serviceType: ServiceType(serviceTypeParts), - host: "", port: 0, txtRecords: [:] - ) - for cont in conts.values { cont.yield(discovered) } - - case .a(let bytes): - guard bytes.count == 4 else { continue } - let host = "\(bytes[0]).\(bytes[1]).\(bytes[2]).\(bytes[3])" - fulfillResolve(recordName: record.name, host: host) - - case .aaaa(let bytes): - guard bytes.count == 16 else { continue } - let groups = stride(from: 0, to: 16, by: 2).map { i in - String(format: "%x", UInt16(bytes[i]) << 8 | UInt16(bytes[i + 1])) - } - fulfillResolve(recordName: record.name, host: groups.joined(separator: ":")) + let records = msg.answers + msg.additionals + + for record in records { + guard case .ptr(let domain) = record.rdata else { continue } + let parts = record.name.split(separator: ".") + guard parts.count >= 3 else { continue } + // record.name is e.g. "_hap._tcp.local" — strip ".local" for lookup key + let serviceTypeKey = parts.dropLast().joined(separator: ".") + guard let conts = browseStreams[serviceTypeKey], !conts.isEmpty else { continue } + // domain is "InstanceName._hap._tcp.local" + let domainParts = domain.split(separator: ".") + let instanceName = domainParts.first.map(String.init) ?? domain + let serviceTypeParts = domainParts.dropFirst().dropLast().joined(separator: ".") + let discovered = ServiceRecord( + name: instanceName, serviceType: ServiceType(serviceTypeParts), + host: "", port: 0, txtRecords: [:] + ) + for cont in conts.values { cont.yield(discovered) } + } + + for (recordName, host) in Self.resolvableHosts(from: records) { + fulfillResolve(recordName: recordName, host: host) + } + } - default: - break + /// Extracts `(recordName, host)` pairs from A/AAAA records in resolve + /// order — **every A match before any AAAA match**, regardless of the + /// order records happened to appear in the packet. `fulfillResolve` + /// resolves and discards the pending continuation on its first call for + /// a given name, so whichever pair is considered first always wins; + /// without this ordering, an AAAA record placed earlier in the same + /// response would win over an A record placed later, even though this + /// library otherwise prefers IPv4 throughout (see `MDNSApple`'s + /// equivalent preference in `AppleServiceDiscovery.resolve`). This only + /// resolves the single-packet case — an AAAA-only packet arriving + /// before a separate, later A-only packet for the same name is a + /// cross-packet race this cannot fix without buffering, which isn't + /// implemented here. + /// + /// An AAAA record's 16 raw bytes are formatted as a bracketed IPv6 + /// literal (`"[fe80:0:0:0:1234:5678:9abc:def0]"`) — RFC 3986 requires + /// the brackets; a bare colon-separated literal is not a valid URL host + /// and previously broke `URLComponents`-based transports downstream + /// (see the sibling fix in `MDNSApple`'s `NetworkAddress(endpoint:)`). + /// No zero-run compression (`::`) — verbose but unambiguous, and this + /// hand-rolled parser has no need to match `nw_path`/`getnameinfo`'s + /// exact canonical form. + /// + /// Pure and side-effect-free — the actual regression coverage for both + /// fixes lives here, unit-testable with no live socket involved. + static func resolvableHosts(from records: [DNSRecord]) -> [(recordName: String, host: String)] { + var results: [(recordName: String, host: String)] = [] + for record in records { + guard case .a(let bytes) = record.rdata, bytes.count == 4 else { continue } + results.append((record.name, "\(bytes[0]).\(bytes[1]).\(bytes[2]).\(bytes[3])")) + } + for record in records { + guard case .aaaa(let bytes) = record.rdata, bytes.count == 16 else { continue } + let groups = stride(from: 0, to: 16, by: 2).map { i in + String(format: "%x", UInt16(bytes[i]) << 8 | UInt16(bytes[i + 1])) } + results.append((record.name, "[\(groups.joined(separator: ":"))]")) } + return results } private func fulfillResolve(recordName: String, host: String) { diff --git a/Tests/MDNSLinuxTests/LinuxServiceDiscoveryResolveTests.swift b/Tests/MDNSLinuxTests/LinuxServiceDiscoveryResolveTests.swift new file mode 100644 index 0000000..07fe38a --- /dev/null +++ b/Tests/MDNSLinuxTests/LinuxServiceDiscoveryResolveTests.swift @@ -0,0 +1,83 @@ +// LinuxServiceDiscoveryResolveTests.swift +// Copyright 2026 Monagle Pty Ltd + +import Testing +import Foundation +@testable import MDNSLinux + +/// `LinuxServiceDiscovery.resolvableHosts(from:)` — the pure, side-effect-free +/// core of `handleResponse`'s A/AAAA handling. Pins down two fixes: +/// +/// 1. An AAAA record used to format as a bare, unbracketed colon-separated +/// literal (`"fe80:0:0:0:1234:5678:9abc:def0"`), which is not a valid URL +/// host — the same class of defect fixed in `MDNSApple`'s +/// `NetworkAddress(endpoint:)` (see that file's tests), just reached via +/// hand-rolled DNS message parsing instead of Network.framework. +/// 2. `handleResponse` resolved on whichever record type it encountered +/// first in the response, with no preference — an AAAA record appearing +/// before an A record in the same packet would silently win. This +/// library prefers IPv4 throughout (see `MDNSApple`'s equivalent +/// preference in `AppleServiceDiscovery.resolve`); the Linux backend +/// should not have been the odd one out. +@Suite("LinuxServiceDiscovery.resolvableHosts — A/AAAA formatting and ordering") +struct LinuxServiceDiscoveryResolveTests { + @Test("an A record formats as a plain, URL-safe dotted-quad host") + func aRecordFormatsPlain() throws { + let record = DNSRecord(name: "shelly1pmg4-abc123.local", type: .a, ttl: 120, rdata: .a(Data([192, 168, 1, 23]))) + let results = LinuxServiceDiscovery.resolvableHosts(from: [record]) + #expect(results.count == 1) + #expect(results[0].recordName == "shelly1pmg4-abc123.local") + #expect(results[0].host == "192.168.1.23") + try #require(urlBuilds(fromHost: results[0].host)) + } + + @Test("an AAAA record formats as a bracketed, URL-safe IPv6 host") + func aaaaRecordFormatsBracketed() throws { + let bytes = Data([0xfe, 0x80, 0, 0, 0, 0, 0, 0, 0x12, 0x34, 0x56, 0x78, 0x9a, 0xbc, 0xde, 0xf0]) + let record = DNSRecord(name: "shelly1pmg4-abc123.local", type: .aaaa, ttl: 120, rdata: .aaaa(bytes)) + let results = LinuxServiceDiscovery.resolvableHosts(from: [record]) + #expect(results.count == 1) + #expect(results[0].host == "[fe80:0:0:0:1234:5678:9abc:def0]") + try #require(urlBuilds(fromHost: results[0].host)) + } + + @Test("when both an A and AAAA record are present, the A-derived host is ordered first regardless of packet order") + func preferOrdersARecordFirst() { + let name = "shelly1pmg4-abc123.local" + let aaaaRecord = DNSRecord( + name: name, type: .aaaa, ttl: 120, + rdata: .aaaa(Data([0xfe, 0x80, 0, 0, 0, 0, 0, 0, 0x12, 0x34, 0x56, 0x78, 0x9a, 0xbc, 0xde, 0xf0])) + ) + let aRecord = DNSRecord(name: name, type: .a, ttl: 120, rdata: .a(Data([192, 168, 1, 23]))) + + // AAAA listed BEFORE A in the input, matching a packet where the + // responder happened to put the AAAA answer first. + let results = LinuxServiceDiscovery.resolvableHosts(from: [aaaaRecord, aRecord]) + + #expect(results.count == 2) + #expect(results[0].host == "192.168.1.23") // A wins the race regardless of input order + #expect(results[1].host == "[fe80:0:0:0:1234:5678:9abc:def0]") + } + + @Test("malformed byte counts are skipped rather than producing a garbled host") + func malformedByteCountsSkipped() { + let shortA = DNSRecord(name: "x.local", type: .a, ttl: 120, rdata: .a(Data([192, 168, 1]))) + let shortAAAA = DNSRecord(name: "x.local", type: .aaaa, ttl: 120, rdata: .aaaa(Data([0xfe, 0x80]))) + #expect(LinuxServiceDiscovery.resolvableHosts(from: [shortA, shortAAAA]).isEmpty) + } + + @Test("non-address records (e.g. PTR) contribute nothing") + func nonAddressRecordsIgnored() { + let ptr = DNSRecord(name: "_shelly._tcp.local", type: .ptr, ttl: 120, rdata: .ptr(domain: "shelly1.local")) + #expect(LinuxServiceDiscovery.resolvableHosts(from: [ptr]).isEmpty) + } + + private func urlBuilds(fromHost host: String) -> Bool { + var components = URLComponents() + components.scheme = "http" + components.host = host + components.port = 80 + components.path = "/rpc" + return components.url != nil + } +} From f70145cf08c644851204d15e44cf0db3c75f002c Mon Sep 17 00:00:00 2001 From: David Monagle Date: Tue, 4 Aug 2026 08:18:26 +1000 Subject: [PATCH 3/3] Preserve the resolved port when substituting a scoped host's fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rejecting a scoped address outright (the previous commit) discarded its port along with it. That port came from the very same successful SRV resolution as the rejected host — mDNS *browsing* alone never carries a port at all (ServiceRecord.port is 0 until something resolves it) — so a caller falling back to a bare ".local" guess loses it, producing "http://foo.local:0/rpc": a URL that builds fine via URLComponents but fails at request-submission time (NSURLErrorBadURL), just moving the same "not usable" problem from the host to the port. Confirmed against real hardware immediately after the previous fix shipped. NetworkAddress(endpoint:) now takes an optional fallbackHostname, substituted for a scoped host while keeping its correctly-resolved port; AppleServiceDiscovery.resolve passes ".local". --- Sources/MDNSApple/AppleServiceDiscovery.swift | 20 +++++- .../MDNSApple/NWEndpoint+NetworkAddress.swift | 67 ++++++++++++------- .../MDNSAppleTests/NetworkAddressTests.swift | 43 ++++++++++-- 3 files changed, 97 insertions(+), 33 deletions(-) diff --git a/Sources/MDNSApple/AppleServiceDiscovery.swift b/Sources/MDNSApple/AppleServiceDiscovery.swift index 25c5180..7772e37 100644 --- a/Sources/MDNSApple/AppleServiceDiscovery.swift +++ b/Sources/MDNSApple/AppleServiceDiscovery.swift @@ -241,8 +241,9 @@ public final class AppleServiceDiscovery: ServiceDiscovery, @unchecked Sendable // to end even when mDNS opportunistically announces one. If no // IPv4-constrained attempt succeeds in time, fall through to an // unconstrained attempt — whatever `currentPath.remoteEndpoint` - // resolves to. Either attempt can still yield a scoped address, - // which `NetworkAddress(endpoint:)` rejects — see its doc comment. + // resolves to. Either attempt can still yield a scoped address; see + // `resolveOnce`'s `fallbackHostname` use for why that's not a + // failure. let ipv4Parameters = NWParameters.udp if let ipOptions = ipv4Parameters.defaultProtocolStack.internetProtocol as? NWProtocolIP.Options { ipOptions.version = .v4 @@ -280,8 +281,21 @@ public final class AppleServiceDiscovery: ServiceDiscovery, @unchecked Sendable guard !resumed else { return } switch state { case .ready: + // A `fallbackHostname` matters even here, not + // just in the caller's own resolve-failure + // fallback: a scoped host still carries a + // correctly-resolved port from this same SRV + // lookup, which mDNS *browsing* alone never + // provides (`ServiceRecord.port` is `0` until + // something resolves it) — losing that port by + // treating a scoped host as a hard failure would + // just move the same "not a valid URL" problem + // from a bad host to a bad (`0`) port. if let remoteEndpoint = connection.currentPath?.remoteEndpoint, - let address = NetworkAddress(endpoint: remoteEndpoint) { + let address = NetworkAddress( + endpoint: remoteEndpoint, + fallbackHostname: "\(recordName).local" + ) { resumed = true cont.resume(returning: address) } else { diff --git a/Sources/MDNSApple/NWEndpoint+NetworkAddress.swift b/Sources/MDNSApple/NWEndpoint+NetworkAddress.swift index 801d2c7..056bb86 100644 --- a/Sources/MDNSApple/NWEndpoint+NetworkAddress.swift +++ b/Sources/MDNSApple/NWEndpoint+NetworkAddress.swift @@ -15,39 +15,54 @@ extension NetworkAddress { /// Only `.hostPort` endpoints are supported — returns `nil` for /// `.service`, `.unix`, `.url`, or other endpoint types. /// - /// Also returns `nil` for an **interface-scoped** `.ipv4`/`.ipv6` - /// address (`addr.interface != nil` — both address families support - /// this, not just IPv6). Network.framework attaches a scope whenever a - /// resolved path is tied to a specific local interface (routine on a - /// multi-interface host — Wi-Fi + Ethernet, a VPN, a bridge — not just a - /// link-local-address edge case), and its string form appends a - /// `%` suffix (`"192.168.1.23%en1"`, - /// `"fe80::1%en0"`). That suffix is not valid inside a URL authority - /// component — `URLComponents` cannot parse it — and it is not portable - /// even where some caller could work around that: `en1` names an - /// interface on *this* process's view of the network, meaningless after - /// a restart, from a different host, or once persisted as a plain - /// string. Rejecting it here (rather than producing an address a - /// downstream URL-based transport cannot use) surfaces as an ordinary - /// `ServiceDiscoveryError.resolveFailed` from ``resolve(_:)``, which - /// callers already treat as "fall back to the plain `.local` hostname" — - /// resolved fresh, and unscoped, at actual connection time instead of - /// baked in here. - public init?(endpoint: NWEndpoint) { + /// Substitutes `fallbackHostname` (if provided) for an + /// **interface-scoped** `.ipv4`/`.ipv6` address (`addr.interface != nil` + /// — both address families support this, not just IPv6). + /// Network.framework attaches a scope whenever a resolved path is tied + /// to a specific local interface (routine on a multi-interface host — + /// Wi-Fi + Ethernet, a VPN, a bridge — not just a link-local-address + /// edge case), and its string form appends a `%` suffix + /// (`"192.168.1.23%en1"`, `"fe80::1%en0"`). That suffix is not valid + /// inside a URL authority component — `URLComponents` cannot parse it — + /// and it is not portable even where some caller could work around + /// that: `en1` names an interface on *this* process's view of the + /// network, meaningless after a restart, from a different host, or once + /// persisted as a plain string. + /// + /// Substituting the hostname (rather than failing the whole resolve) + /// matters because **`port` is still correct** — it came from the same + /// successful SRV resolution as the rejected host, and mDNS browsing + /// alone never carries a port at all (`ServiceRecord.port` is `0` until + /// something resolves it). A caller that instead discards this whole + /// address and falls back to a bare "`.local`" guess, as if + /// nothing had been resolved, loses that port along with it — producing + /// a URL like `http://foo.local:0/rpc`, invalid for an entirely + /// different reason than the one this fix addresses. With no + /// `fallbackHostname` supplied, the address is rejected (`nil`) exactly + /// as before. + public init?(endpoint: NWEndpoint, fallbackHostname: String? = nil) { guard case .hostPort(let host, let port) = endpoint else { return nil } let hostString: String switch host { case .ipv4(let addr): - guard addr.interface == nil else { return nil } - hostString = "\(addr)" + if let fallbackHostname, addr.interface != nil { + hostString = fallbackHostname + } else { + guard addr.interface == nil else { return nil } + hostString = "\(addr)" + } case .ipv6(let addr): - guard addr.interface == nil else { return nil } - // Bracket per RFC 3986 — a bare IPv6 literal is not a valid URL - // host. Only reachable for a non-scoped address (global unicast - // or ULA); a scoped one was already rejected above. - hostString = "[\(addr)]" + if let fallbackHostname, addr.interface != nil { + hostString = fallbackHostname + } else { + guard addr.interface == nil else { return nil } + // Bracket per RFC 3986 — a bare IPv6 literal is not a valid + // URL host. Only reachable for a non-scoped address (global + // unicast or ULA); a scoped one is handled above. + hostString = "[\(addr)]" + } case .name(let name, _): hostString = name @unknown default: diff --git a/Tests/MDNSAppleTests/NetworkAddressTests.swift b/Tests/MDNSAppleTests/NetworkAddressTests.swift index 1459784..ea3a02c 100644 --- a/Tests/MDNSAppleTests/NetworkAddressTests.swift +++ b/Tests/MDNSAppleTests/NetworkAddressTests.swift @@ -30,7 +30,7 @@ struct NetworkAddressTests { try #require(urlBuilds(fromHost: address.host)) } - @Test("an interface-scoped IPv4 endpoint (e.g. \"192.168.1.23%en1\") is rejected") + @Test("an interface-scoped IPv4 endpoint (e.g. \"192.168.1.23%en1\") is rejected when no fallback is given") func scopedIPv4Rejected() throws { let addr = try #require(IPv4Address("192.168.1.23%en1")) #expect(addr.interface != nil) // sanity: this really is what we think it is @@ -38,6 +38,23 @@ struct NetworkAddressTests { #expect(NetworkAddress(endpoint: endpoint) == nil) } + @Test("a scoped IPv4 endpoint substitutes the fallback hostname but keeps the resolved port") + func scopedIPv4UsesFallbackHostnamePreservingPort() throws { + let addr = try #require(IPv4Address("192.168.173.104%en1")) + let endpoint = NWEndpoint.hostPort(host: .ipv4(addr), port: 80) + let address = try #require(NetworkAddress(endpoint: endpoint, fallbackHostname: "shelly1.local")) + #expect(address.host == "shelly1.local") + // The real, actual bug this guards against: a caller that discards + // a scoped address's port along with its rejected host — falling + // back to a bare ".local" guess with no port info, since mDNS + // browsing alone never carries one — ends up dispatching to port 0, + // which fails at request-submission time (NSURLErrorBadURL) even + // though `URLComponents` itself doesn't reject it. Confirmed + // against real hardware — see CLAUDE.md item 5's fix history. + #expect(address.port == 80) + try #require(urlBuilds(fromHost: address.host, port: Int(address.port))) + } + @Test("a plain (unscoped) global/ULA IPv6 endpoint resolves to a bracketed, URL-safe host") func plainIPv6() throws { let addr = try #require(IPv6Address("fd00::1")) @@ -47,7 +64,7 @@ struct NetworkAddressTests { try #require(urlBuilds(fromHost: address.host)) } - @Test("a link-local (interface-scoped) IPv6 endpoint is rejected, not bracketed as-is") + @Test("a link-local (interface-scoped) IPv6 endpoint is rejected, not bracketed as-is, when no fallback is given") func scopedIPv6Rejected() throws { let addr = try #require(IPv6Address("fe80::1234:5678:9abc:def0%en0")) #expect(addr.interface != nil) @@ -55,6 +72,24 @@ struct NetworkAddressTests { #expect(NetworkAddress(endpoint: endpoint) == nil) } + @Test("a scoped IPv6 endpoint substitutes the fallback hostname but keeps the resolved port") + func scopedIPv6UsesFallbackHostnamePreservingPort() throws { + let addr = try #require(IPv6Address("fe80::1234:5678:9abc:def0%en0")) + let endpoint = NWEndpoint.hostPort(host: .ipv6(addr), port: 8080) + let address = try #require(NetworkAddress(endpoint: endpoint, fallbackHostname: "shelly1.local")) + #expect(address.host == "shelly1.local") + #expect(address.port == 8080) + try #require(urlBuilds(fromHost: address.host, port: Int(address.port))) + } + + @Test("a plain, unscoped address ignores a supplied fallback hostname entirely") + func unscopedAddressIgnoresFallback() throws { + let addr = try #require(IPv4Address("192.168.1.23")) + let endpoint = NWEndpoint.hostPort(host: .ipv4(addr), port: 80) + let address = try #require(NetworkAddress(endpoint: endpoint, fallbackHostname: "shelly1.local")) + #expect(address.host == "192.168.1.23") + } + @Test("a .name host (an mDNS hostname target) passes through unchanged") func nameHostPassesThrough() throws { let endpoint = NWEndpoint.hostPort(host: .name("shelly1pmg4-abc123.local", nil), port: 80) @@ -73,11 +108,11 @@ struct NetworkAddressTests { /// does hand back must be usable to build a real URL — this is the /// exact construction `ShellyHTTPTransportConfiguration.makeURL()` (and /// any future consumer) performs. - private func urlBuilds(fromHost host: String) -> Bool { + private func urlBuilds(fromHost host: String, port: Int = 80) -> Bool { var components = URLComponents() components.scheme = "http" components.host = host - components.port = 80 + components.port = port components.path = "/rpc" return components.url != nil }