From 7481ae83a82112f9f083611aca71564762034fda Mon Sep 17 00:00:00 2001 From: AadhilFarhan <62174733+AadhilFarhan@users.noreply.github.com> Date: Mon, 20 Jul 2026 23:23:58 +0530 Subject: [PATCH] Harden ShareProxy: validate ports, avoid blocking the main actor, fix a shutdown race - ShareProxy.init force-unwrapped an NWEndpoint.Port built from an unvalidated Int, trapping on any out-of-range port. Now validates 1...65535 and throws ShareProxyError instead, matching the project's degrade-don't-crash convention. Reachable today via the CLI's --share flag with no prior validation. - ShareProxy.init can block its calling thread briefly waiting for the listener to come up. AppModel.startSharing called it synchronously on the main actor from QRPanel's onAppear, so tapping the QR button could freeze the UI. startSharing is now async and dispatches the construction the same way PortScanner.scan() already is; re-checks shares[port] after resuming in case another call raced it. - stop() could lose a race with an in-flight accept: a connection already progressing through relay() when cancel() ran could still land in the connections dict after stop() had already drained it, leaking an un-cancelled relay. relay() now checks a stopped flag under the same lock stop() uses. - Added deinit { stop() } as a safety net if a caller drops the last reference without calling stop() explicitly. --- Sources/Portside/AppModel.swift | 21 ++++++++-- Sources/Portside/CLI.swift | 2 +- Sources/Portside/UI/QRPanel.swift | 2 +- Sources/PortsideCore/ShareProxy.swift | 39 ++++++++++++++++--- Tests/PortsideCoreTests/ShareProxyTests.swift | 23 +++++++++++ 5 files changed, 75 insertions(+), 12 deletions(-) create mode 100644 Tests/PortsideCoreTests/ShareProxyTests.swift diff --git a/Sources/Portside/AppModel.swift b/Sources/Portside/AppModel.swift index ba4e592..b303fe9 100644 --- a/Sources/Portside/AppModel.swift +++ b/Sources/Portside/AppModel.swift @@ -57,8 +57,10 @@ final class AppModel { } /// Returns a URL reachable from other devices on the Wi-Fi. Servers bound - /// to all interfaces are reachable directly; loopback-bound ones get a relay. - func startSharing(_ server: DevServer) -> ActiveShare? { + /// to all interfaces are reachable directly; loopback-bound ones get a + /// relay. `ShareProxy`'s init can block briefly waiting for its listener, + /// so — like `PortScanner.scan()` — it's built off the main actor. + func startSharing(_ server: DevServer) async -> ActiveShare? { if let existing = shares[server.port] { return existing } guard let ip = lanAddress else { return nil } @@ -67,9 +69,20 @@ final class AppModel { shares[server.port] = share return share } - guard let relay = try? ShareProxy(targetPort: server.port), relay.listenPort > 0 else { - return nil + + let targetPort = server.port + let relay = await Task.detached(priority: .utility) { + try? ShareProxy(targetPort: targetPort) + }.value + + // Another call could have started (or stopped) a share for this port + // while we were suspended constructing the relay above. + if let existing = shares[server.port] { + relay?.stop() + return existing } + guard let relay, relay.listenPort > 0 else { return nil } + relays[server.port] = relay let share = ActiveShare(url: URL(string: "http://\(ip):\(relay.listenPort)")!, isRelayed: true) shares[server.port] = share diff --git a/Sources/Portside/CLI.swift b/Sources/Portside/CLI.swift index 3e9388d..ff5d334 100644 --- a/Sources/Portside/CLI.swift +++ b/Sources/Portside/CLI.swift @@ -55,7 +55,7 @@ enum CLI { print("Relaying 127.0.0.1:\(port) -> http://\(ip):\(proxy.listenPort) (Ctrl-C to stop)") dispatchMain() } catch { - print("error: \(error)") + print("error: \(error.localizedDescription)") exit(1) } } diff --git a/Sources/Portside/UI/QRPanel.swift b/Sources/Portside/UI/QRPanel.swift index f98783d..8ec4bb0 100644 --- a/Sources/Portside/UI/QRPanel.swift +++ b/Sources/Portside/UI/QRPanel.swift @@ -67,6 +67,6 @@ struct QRPanel: View { ) .padding(.horizontal, 10) .padding(.bottom, 6) - .onAppear { share = model.startSharing(server) } + .task { share = await model.startSharing(server) } } } diff --git a/Sources/PortsideCore/ShareProxy.swift b/Sources/PortsideCore/ShareProxy.swift index 251db16..6195761 100644 --- a/Sources/PortsideCore/ShareProxy.swift +++ b/Sources/PortsideCore/ShareProxy.swift @@ -1,6 +1,17 @@ import Foundation import Network +public enum ShareProxyError: LocalizedError, Sendable, Equatable { + case invalidPort(Int) + + public var errorDescription: String? { + switch self { + case .invalidPort(let port): + return "Invalid port \(port): must be between 1 and 65535." + } + } +} + /// A TCP relay that listens on every interface and pipes bytes to a target /// port on loopback. This is what lets a phone on the same Wi-Fi reach a dev /// server that bound only to 127.0.0.1 — the relay is reachable from the LAN, @@ -9,18 +20,26 @@ import Network public final class ShareProxy: @unchecked Sendable { public let targetPort: Int + private let targetEndpoint: NWEndpoint.Port private let listener: NWListener private let queue = DispatchQueue(label: "portside.proxy") private let lock = NSLock() private var connections: [ObjectIdentifier: (NWConnection, NWConnection)] = [:] + private var stopped = false public var listenPort: Int { Int(listener.port?.rawValue ?? 0) } - /// Starts immediately on an OS-assigned port. + /// Starts immediately on an OS-assigned port. Blocks the calling thread + /// briefly (up to 2s) while the listener comes up — callers on the main + /// actor must dispatch this off-main, the same way `PortScanner.scan()` is. public init(targetPort: Int) throws { + guard (1...65535).contains(targetPort), let endpoint = NWEndpoint.Port(rawValue: UInt16(targetPort)) else { + throw ShareProxyError.invalidPort(targetPort) + } self.targetPort = targetPort + self.targetEndpoint = endpoint let params = NWParameters.tcp params.allowLocalEndpointReuse = true listener = try NWListener(using: params, on: .any) @@ -38,9 +57,14 @@ public final class ShareProxy: @unchecked Sendable { listener.stateUpdateHandler = nil } + deinit { + stop() + } + public func stop() { listener.cancel() lock.lock() + stopped = true let open = connections.values connections.removeAll() lock.unlock() @@ -51,12 +75,15 @@ public final class ShareProxy: @unchecked Sendable { } private func relay(_ inbound: NWConnection) { - let outbound = NWConnection( - host: "127.0.0.1", - port: NWEndpoint.Port(rawValue: UInt16(targetPort))!, - using: .tcp - ) + let outbound = NWConnection(host: "127.0.0.1", port: targetEndpoint, using: .tcp) + lock.lock() + guard !stopped else { + lock.unlock() + inbound.cancel() + outbound.cancel() + return + } connections[ObjectIdentifier(inbound)] = (inbound, outbound) lock.unlock() diff --git a/Tests/PortsideCoreTests/ShareProxyTests.swift b/Tests/PortsideCoreTests/ShareProxyTests.swift new file mode 100644 index 0000000..4e5a01a --- /dev/null +++ b/Tests/PortsideCoreTests/ShareProxyTests.swift @@ -0,0 +1,23 @@ +import XCTest +@testable import PortsideCore + +final class ShareProxyTests: XCTestCase { + + func testRejectsPortZero() { + XCTAssertThrowsError(try ShareProxy(targetPort: 0)) { error in + XCTAssertEqual(error as? ShareProxyError, .invalidPort(0)) + } + } + + func testRejectsNegativePort() { + XCTAssertThrowsError(try ShareProxy(targetPort: -1)) { error in + XCTAssertEqual(error as? ShareProxyError, .invalidPort(-1)) + } + } + + func testRejectsPortAboveValidRange() { + XCTAssertThrowsError(try ShareProxy(targetPort: 70000)) { error in + XCTAssertEqual(error as? ShareProxyError, .invalidPort(70000)) + } + } +}