From 9201fe9857b0a4ae2f9d03a8297e4324c12db813 Mon Sep 17 00:00:00 2001 From: Ryanmello07 <67509637+Ryanmello07@users.noreply.github.com> Date: Mon, 7 Sep 2026 21:55:05 -0700 Subject: [PATCH 1/2] add a rule by hand on the split rules sheet A split rule could only be made by tapping a row in the live Activity list, so a host that had not been seen could not be routed at all -- and traffic you want to exclude is often traffic you have not sent yet. Add a rule row at the top of the Rules section, and a text field as the first row of the editor's host list, so hand-typed values and tapped ones land in the same rule through the same path. The grammar is checked before the value is stored, because the matcher has no error channel: anything it cannot parse as a wildcard, a prefix or an address is filed as an exact host name, so a typo becomes a rule that is created, persisted, mirrored, counted on the connect card and matched never. SplitRuleHostInput refuses what would be dead -- unicode names, single-label names, malformed ranges -- and says why, in a line under the field. It also normalizes the two values Go rewrites silently, masking a prefix to its network address and unmapping an ipv4-mapped address, so the chip shows what is actually in force. It is deliberately never more permissive than the matcher: over-rejection costs a rephrase, over- acceptance costs a rule the user believes is working. Creation is gated on the store having a list it can vouch for. With the rpc down the remote builds the list it will push on the next connect from whatever base it can see, and an unseeded base is empty, so a rule created then would REPLACE the extension's saved rules rather than join them. Tapping an activity row could never reach that state -- the activity list is empty with no rpc -- so the protection was accidental, and a hand- written rule removes it. The latch lives in the store rather than only on the affordance, and is monotonic: a disconnect clears the remote's service but never its last known list. New rules default to route-locally, the only mode guaranteed to take effect on its own: a merge replaces an existing route override when the incoming one is local, so a first hand-written rule in another mode could silently do nothing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AWV4MaF9JBcQppSX9UxATa --- .../Shared/Utilities/SplitRuleHostInput.swift | 237 ++++++++++++++++++ .../Shared/ViewModels/BlockActionsStore.swift | 27 +- .../Shared/Views/Stats/SplitRulesView.swift | 140 ++++++++++- .../SplitRuleHostInputTests.swift | 129 ++++++++++ 4 files changed, 529 insertions(+), 4 deletions(-) create mode 100644 app/network/Shared/Utilities/SplitRuleHostInput.swift create mode 100644 app/networkTests/SplitRuleHostInputTests.swift diff --git a/app/network/Shared/Utilities/SplitRuleHostInput.swift b/app/network/Shared/Utilities/SplitRuleHostInput.swift new file mode 100644 index 00000000..c404f2c9 --- /dev/null +++ b/app/network/Shared/Utilities/SplitRuleHostInput.swift @@ -0,0 +1,237 @@ +// +// SplitRuleHostInput.swift +// URnetwork +// +// What a hand-typed split rule host is allowed to be. +// +// The invariant this file exists to hold: NEVER be more permissive than the +// Go matcher. That matcher has no error channel -- anything it cannot parse +// as a wildcard, a prefix or an address is filed as an exact host name +// (connect/ip_block_action.go:385-406), so a typo produces a rule that is +// created, persisted, mirrored, seeded, counted in the "N split rules" card +// and matched never. There is nowhere downstream to report that. Rejecting a +// value the matcher would have accepted only costs the user a rephrase; +// accepting one it will never match costs them a rule they believe is +// working. When in doubt, reject. +// +// Kept free of SwiftUI and of the SDK so it can be tested directly. The +// durable fix is one normalizer in Go called by the matcher itself and bound +// through gomobile, the way host-name collapsing already works; until that +// lands this is the second implementation of a grammar that has one owner. +// + +import Foundation +import Network + +enum SplitRuleHostError: Equatable { + case notAscii + case badName + case badWildcard + case badRange + case duplicate + /// Another value in the same rule already matches everything this would. + case covered(by: String) +} + +struct SplitRuleHostValidation: Equatable { + /// The value as it will be stored, lowercased and trimmed, with an IP + /// range masked to its network address the way the matcher keys it. + let normalized: String? + let error: SplitRuleHostError? + /// Set when the stored value differs from what was typed, so the change + /// is something the user agreed to rather than something Go did quietly. + let note: String? + + var isAccepted: Bool { normalized != nil && error == nil } +} + +enum SplitRuleHostInput { + + /// Longest legal DNS name, and longest legal label. + private static let maxNameLength = 253 + private static let maxLabelLength = 63 + + /// Validates one typed value against the values already in the rule. + static func validate(_ raw: String, existing: [String] = []) -> SplitRuleHostValidation { + let host = raw.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() + if host.isEmpty { + return SplitRuleHostValidation(normalized: nil, error: nil, note: nil) + } + // the matcher lowercases and compares bytes; a resolved name arrives + // as punycode, so a unicode name here could only ever be dead + guard host.allSatisfy({ $0.isASCII }) else { + return rejected(.notAscii) + } + + let normalized: String + var note: String? = nil + + if let base = host.dropPrefixIfPresent("**.") ?? host.dropPrefixIfPresent("*.") { + guard isValidName(base) else { + return rejected(.badWildcard) + } + normalized = host + } else if host.contains("/") { + guard let masked = maskedPrefix(host) else { + return rejected(.badRange) + } + normalized = masked + if masked != host { + note = masked + } + } else if let address = normalizedAddress(host) { + normalized = address + if address != host { + note = address + } + } else { + guard isValidName(host) else { + return rejected(.badName) + } + normalized = host + } + + if existing.contains(normalized) { + return rejected(.duplicate) + } + if let cover = existing.first(where: { covers($0, normalized) }) { + return SplitRuleHostValidation(normalized: nil, error: .covered(by: cover), note: nil) + } + return SplitRuleHostValidation(normalized: normalized, error: nil, note: note) + } + + private static func rejected(_ error: SplitRuleHostError) -> SplitRuleHostValidation { + SplitRuleHostValidation(normalized: nil, error: error, note: nil) + } + + /// A dotted name of at least two labels. Single-label names parse as + /// exact hosts in Go but can never match a resolved destination, so they + /// are refused here rather than stored as a rule that does nothing. + static func isValidName(_ name: String) -> Bool { + guard !name.isEmpty, name.count <= maxNameLength, !name.hasSuffix(".") else { + return false + } + let labels = name.split(separator: ".", omittingEmptySubsequences: false) + guard 2 <= labels.count else { + return false + } + return labels.allSatisfy { label in + guard !label.isEmpty, label.count <= maxLabelLength else { + return false + } + guard !label.hasPrefix("-"), !label.hasSuffix("-") else { + return false + } + return label.allSatisfy { $0.isLowercaseASCIILetter || $0.isASCIIDigit || $0 == "-" } + } + } + + /// An address, in the form the matcher stores it: an ipv4-mapped ipv6 + /// address is unmapped there, so it is unmapped here too. + static func normalizedAddress(_ value: String) -> String? { + if let v4 = IPv4Address(value) { + return v4.debugDescription + } + guard let v6 = IPv6Address(value) else { + return nil + } + // ::ffff:1.2.3.4 -> 1.2.3.4, matching netip's Unmap() + let bytes = [UInt8](v6.rawValue) + if bytes.count == 16, + bytes[0..<10].allSatisfy({ $0 == 0 }), + bytes[10] == 0xff, bytes[11] == 0xff, + let mapped = IPv4Address(Data(bytes[12..<16]), nil) { + return mapped.debugDescription + } + return v6.debugDescription + } + + /// A CIDR range, masked to its network address. Go masks it before + /// matching, so `192.168.1.42/24` becomes `192.168.1.0/24` there; doing + /// it here means the chip shows what is actually in force. + static func maskedPrefix(_ value: String) -> String? { + let parts = value.split(separator: "/", omittingEmptySubsequences: false) + guard parts.count == 2, + let bits = Int(parts[1]), + !parts[1].isEmpty else { + return nil + } + let address = String(parts[0]) + if let v4 = IPv4Address(address) { + guard 0...32 ~= bits, let masked = mask(v4.rawValue, bits: bits, of: 4), + let result = IPv4Address(masked, nil) else { + return nil + } + return "\(result.debugDescription)/\(bits)" + } + if let v6 = IPv6Address(address) { + guard 0...128 ~= bits, let masked = mask(v6.rawValue, bits: bits, of: 16), + let result = IPv6Address(masked, nil) else { + return nil + } + return "\(result.debugDescription)/\(bits)" + } + return nil + } + + private static func mask(_ raw: Data, bits: Int, of byteCount: Int) -> Data? { + var bytes = [UInt8](raw) + guard bytes.count == byteCount else { + return nil + } + for index in 0.. Bool { + if let base = wildcard.dropPrefixIfPresent("**.") { + return candidate == base || candidate.hasSuffix("." + base) + } + if let base = wildcard.dropPrefixIfPresent("*.") { + return candidate.hasSuffix("." + base) + } + return false + } + + /// The reason a value was refused, for the line under the field. + static func message(for error: SplitRuleHostError) -> String { + switch error { + case .notAscii: + return String(localized: "Use the ASCII form of the name.") + case .badName: + return String(localized: "Enter a host name like example.com.") + case .badWildcard: + return String(localized: "A wildcard needs a name after it, like *.example.com.") + case .badRange: + return String(localized: "Enter an IP range like 10.0.0.0/8.") + case .duplicate: + return String(localized: "Already in this rule.") + case .covered(let by): + return String(format: String(localized: "Already covered by %@ in this rule."), by) + } + } +} + +private extension String { + func dropPrefixIfPresent(_ prefix: String) -> String? { + hasPrefix(prefix) ? String(dropFirst(prefix.count)) : nil + } +} + +private extension Character { + var isLowercaseASCIILetter: Bool { "a"..."z" ~= self } + var isASCIIDigit: Bool { "0"..."9" ~= self } +} diff --git a/app/network/Shared/ViewModels/BlockActionsStore.swift b/app/network/Shared/ViewModels/BlockActionsStore.swift index 331472d0..82cc5dbb 100644 --- a/app/network/Shared/ViewModels/BlockActionsStore.swift +++ b/app/network/Shared/ViewModels/BlockActionsStore.swift @@ -176,6 +176,9 @@ class BlockActionsStore: ObservableObject { */ @Published private(set) var blockActions: [BlockActionItem] = [] @Published private(set) var splitRules: [SplitRuleItem] = [] + /// A rule may be created from here only once this process has a list it + /// can vouch for -- see `createRule`. + @Published private(set) var canCreateRule: Bool = false @Published private(set) var allowedCount: Int = 0 @Published private(set) var blockedCount: Int = 0 @@ -322,6 +325,7 @@ class BlockActionsStore: ObservableObject { blockActions = [] splitRules = [] sdkOverrides = [] + canCreateRule = false allowedCount = 0 blockedCount = 0 exitsByIp = [:] @@ -590,6 +594,16 @@ class BlockActionsStore: ObservableObject { if afterEdit || device.getConnected() { persistOverrides() } + // Latched here rather than in `setup` because the change listeners are + // registered before the first read, so a connect landing in between + // cannot be missed. Monotonic: a disconnect clears the remote's + // service but never its last known list, so a base that was real stays + // real for this device's life. The local state is optional -- a build + // with no app group has none -- and that degrades correctly, to + // "connected only", which is the safe half of the test. + if !canCreateRule, device.getConnected() || localState?.getBlockActionOverrides() != nil { + canCreateRule = true + } } /** @@ -634,8 +648,19 @@ class BlockActionsStore: ObservableObject { * creates a split rule applying `mode` to the selected host values; * see `SplitRuleMode` */ + /// Refuses to create until this process has seen the whole list, because + /// creating against a list it has not seen would DELETE the rest. + /// + /// With the rpc down the remote builds the full list it will push on the + /// next connect out of whatever base it can see, and an unseeded base is + /// empty -- so a rule created then would replace the extension's saved + /// rules rather than join them (see `DeviceManager.initDevice`). The base + /// is real once either the mirror exists or the device has connected, and + /// tapping an activity row could never reach this state because the + /// activity list is empty with no rpc. A hand-written rule can, so the + /// check has to live here and not only on the affordance. func createRule(hosts: [String], mode: SplitRuleMode) { - guard let device = self.device, !hosts.isEmpty else { + guard let device = self.device, !hosts.isEmpty, canCreateRule else { return } let override = SdkBlockActionOverride() diff --git a/app/network/Shared/Views/Stats/SplitRulesView.swift b/app/network/Shared/Views/Stats/SplitRulesView.swift index 5e97863d..d9ee880a 100644 --- a/app/network/Shared/Views/Stats/SplitRulesView.swift +++ b/app/network/Shared/Views/Stats/SplitRulesView.swift @@ -42,6 +42,7 @@ struct SplitRulesView: View { @State private var topBaseline: CGFloat? = nil private static let topMarkerId = "split-rules-top" + private static let newRuleTargetId = "split-rules-new" private var pendingCount: Int { let displayedIds = Set(displayedActions.map { $0.id }) @@ -99,8 +100,10 @@ struct SplitRulesView: View { header: sectionHeader("Rules") ) { + addRuleRow + if blockActionsStore.splitRules.isEmpty { - Text("Tap traffic below to route it locally or hold it to one provider.") + Text("Add a host above, or tap traffic below to route it locally or hold it to one provider.") .font(themeManager.currentTheme.secondaryBodyFont) .foregroundColor(themeManager.currentTheme.textFaintColor) .listRowBackground(Color.clear) @@ -320,6 +323,61 @@ struct SplitRulesView: View { return values } + /** + * Adds a rule for a host that has not been seen in the activity below -- + * the only way to write a rule for traffic that has not happened yet. + * + * Disabled until the store has a list it can vouch for: creating against + * a list this process has not seen would replace the extension's saved + * rules rather than join them (see `BlockActionsStore.createRule`). That + * is one connect away, and the note below says so. + */ + @ViewBuilder + private var addRuleRow: some View { + VStack(alignment: .leading, spacing: 4) { + Button(action: { + editorTarget = EditorTarget( + id: Self.newRuleTargetId, + candidates: [], + selected: [], + ruleId: nil, + // route-locally is the only mode that always takes effect + // on its own: a merge replaces an existing route override + // when the incoming one is local, so a first hand-written + // rule in another mode could silently do nothing + mode: .excluded + ) + }) { + HStack(spacing: 8) { + Image(systemName: "plus.circle.fill") + .foregroundColor( + blockActionsStore.canCreateRule + ? .urGreen + : themeManager.currentTheme.textFaintColor + ) + Text("Add a rule") + .font(themeManager.currentTheme.bodyFont) + .foregroundColor( + blockActionsStore.canCreateRule + ? themeManager.currentTheme.textColor + : themeManager.currentTheme.textFaintColor + ) + Spacer() + } + .contentShape(Rectangle()) + } + .buttonStyle(.plain) + .disabled(!blockActionsStore.canCreateRule) + + if !blockActionsStore.canCreateRule { + Text("Connect once to load the rules you already have.") + .font(themeManager.currentTheme.secondaryBodyFont) + .foregroundColor(themeManager.currentTheme.textFaintColor) + } + } + .listRowBackground(Color.clear) + } + private func sectionHeader(_ text: LocalizedStringKey) -> some View { Text(text) .font(themeManager.currentTheme.secondaryBodyFont) @@ -620,6 +678,11 @@ struct SplitRuleEditorView: View { @State private var selection: Set @State private var mode: SplitRuleMode + /// Hosts typed here, newest first, ahead of the ones this rule was opened + /// with. Kept separate from `candidates` so the list is a plain function + /// of both and there is no state to keep in step. + @State private var addedHosts: [String] = [] + @State private var newHost: String = "" init(candidates: [String], initialSelection: Set, ruleId: String?, initialMode: SplitRuleMode) { self.candidates = candidates @@ -632,6 +695,26 @@ struct SplitRuleEditorView: View { ruleId != nil } + private var editableCandidates: [String] { + addedHosts + candidates + } + + private var validation: SplitRuleHostValidation { + SplitRuleHostInput.validate(newHost, existing: editableCandidates) + } + + /// Adds the typed host and selects it: a value typed by hand is one the + /// user wants in the rule, so making them tick it as well is a step that + /// only ever costs them the rule silently doing nothing. + private func addTypedHost() { + guard let host = validation.normalized else { + return + } + addedHosts.insert(host, at: 0) + selection.insert(host) + newHost = "" + } + var body: some View { VStack(alignment: .leading, spacing: 0) { @@ -675,7 +758,9 @@ struct SplitRuleEditorView: View { .padding(.horizontal) List { - ForEach(candidates, id: \.self) { host in + hostEntryRow + + ForEach(editableCandidates, id: \.self) { host in HStack { Text(host) .font(themeManager.currentTheme.bodyFont) @@ -709,7 +794,7 @@ struct SplitRuleEditorView: View { UrButton( text: isEditing ? "Update" : "Create", action: { - let hosts = candidates.filter { selection.contains($0) } + let hosts = editableCandidates.filter { selection.contains($0) } if let ruleId = ruleId { blockActionsStore.updateRule(id: ruleId, hosts: hosts, mode: mode) } else { @@ -743,6 +828,55 @@ struct SplitRuleEditorView: View { * one exclusive mode choice: a radio-style circle with the mode title * and what it does */ + /** + * Type a host, a wildcard or an IP range into the rule. + * + * The matcher has no error channel -- a value it cannot parse is filed as + * an exact host name and simply never matches -- so the grammar is + * checked here and the reason is said out loud. See `SplitRuleHostInput`. + */ + @ViewBuilder + private var hostEntryRow: some View { + let validation = self.validation + VStack(alignment: .leading, spacing: 4) { + HStack(spacing: 8) { + TextField("example.com, *.example.com, 10.0.0.0/8", text: $newHost) + .font(themeManager.currentTheme.bodyFont) + .foregroundColor(themeManager.currentTheme.textColor) + .autocorrectionDisabled() + #if os(iOS) + .keyboardType(.URL) + .textInputAutocapitalization(.never) + #endif + .onSubmit { addTypedHost() } + + Button(action: { addTypedHost() }) { + Image(systemName: "plus.circle.fill") + .foregroundColor( + validation.isAccepted + ? .urGreen + : themeManager.currentTheme.textFaintColor + ) + } + .buttonStyle(.plain) + .disabled(!validation.isAccepted) + } + + // silent while the field is empty: a rejection reason before + // anything has been typed reads as an error the user caused + if let error = validation.error { + Text(SplitRuleHostInput.message(for: error)) + .font(themeManager.currentTheme.secondaryBodyFont) + .foregroundColor(themeManager.currentTheme.dangerColor) + } else if let note = validation.note { + Text(String(format: String(localized: "Saved as %@"), note)) + .font(themeManager.currentTheme.secondaryBodyFont) + .foregroundColor(themeManager.currentTheme.textMutedColor) + } + } + .listRowBackground(Color.clear) + } + private func modeRow( _ rowMode: SplitRuleMode, title: LocalizedStringKey, diff --git a/app/networkTests/SplitRuleHostInputTests.swift b/app/networkTests/SplitRuleHostInputTests.swift new file mode 100644 index 00000000..633138b4 --- /dev/null +++ b/app/networkTests/SplitRuleHostInputTests.swift @@ -0,0 +1,129 @@ +// +// SplitRuleHostInputTests.swift +// networkTests +// +// The grammar behind the hand-typed split rule host. +// +// These matter more than most input validation because the failure is +// silent. The Go matcher files anything it cannot parse as an exact host +// name and there is no error channel anywhere on the write path, so an +// accepted-but-wrong value becomes a rule that is created, persisted, +// mirrored, counted on the connect card and matched never. Every case below +// that asserts a REJECTION is guarding that, not tidiness. +// + +import Testing +import Foundation +@testable import URnetwork + +struct SplitRuleHostInputTests { + + @Test func plainNamesAreAccepted() { + #expect(SplitRuleHostInput.validate("example.com").normalized == "example.com") + #expect(SplitRuleHostInput.validate("a1366.dscapi6.akamai.net").normalized == "a1366.dscapi6.akamai.net") + #expect(SplitRuleHostInput.validate("my-host.example.co.uk").normalized == "my-host.example.co.uk") + } + + @Test func inputIsTrimmedAndLowercased() { + #expect(SplitRuleHostInput.validate(" Example.COM ").normalized == "example.com") + } + + /// The matcher lowercases and compares bytes, and a resolved name arrives + /// as punycode, so a unicode name could only ever be a dead rule. + @Test func unicodeNamesAreRefused() { + #expect(SplitRuleHostInput.validate("münchen.de").error == .notAscii) + } + + /// Parses in Go as an exact host, but nothing resolves to it, so it would + /// be a rule that does nothing. + @Test func singleLabelNamesAreRefused() { + #expect(SplitRuleHostInput.validate("localhost").error == .badName) + #expect(SplitRuleHostInput.validate("router").error == .badName) + } + + @Test func malformedNamesAreRefused() { + #expect(SplitRuleHostInput.validate("example..com").error == .badName) + #expect(SplitRuleHostInput.validate("-example.com").error == .badName) + #expect(SplitRuleHostInput.validate("example-.com").error == .badName) + #expect(SplitRuleHostInput.validate("example.com.").error == .badName) + #expect(SplitRuleHostInput.validate("exa mple.com").error == .badName) + } + + @Test func wildcardsAreAccepted() { + #expect(SplitRuleHostInput.validate("*.example.com").normalized == "*.example.com") + #expect(SplitRuleHostInput.validate("**.example.com").normalized == "**.example.com") + } + + @Test func wildcardsNeedAName() { + #expect(SplitRuleHostInput.validate("*.").error == .badWildcard) + #expect(SplitRuleHostInput.validate("**.").error == .badWildcard) + #expect(SplitRuleHostInput.validate("*.com").error == .badWildcard) + } + + @Test func addressesAreAccepted() { + #expect(SplitRuleHostInput.validate("1.2.3.4").normalized == "1.2.3.4") + #expect(SplitRuleHostInput.validate("2001:db8::1").normalized == "2001:db8::1") + } + + /// The matcher unmaps an ipv4-mapped ipv6 address before keying on it, so + /// storing the mapped form would key something the matcher never looks up. + @Test func mappedAddressesAreUnmapped() { + #expect(SplitRuleHostInput.validate("::ffff:1.2.3.4").normalized == "1.2.3.4") + } + + /// Go masks a prefix before matching, so an unmasked one is silently + /// rewritten there. Doing it here means the chip shows what is in force. + @Test func rangesAreMaskedToTheirNetwork() { + let validation = SplitRuleHostInput.validate("192.168.1.42/24") + #expect(validation.normalized == "192.168.1.0/24") + #expect(validation.note == "192.168.1.0/24") + } + + @Test func alreadyMaskedRangesCarryNoNote() { + let validation = SplitRuleHostInput.validate("10.0.0.0/8") + #expect(validation.normalized == "10.0.0.0/8") + #expect(validation.note == nil) + } + + @Test func malformedRangesAreRefused() { + #expect(SplitRuleHostInput.validate("10.0.0.0/").error == .badRange) + #expect(SplitRuleHostInput.validate("10.0.0.0/33").error == .badRange) + #expect(SplitRuleHostInput.validate("example.com/24").error == .badRange) + #expect(SplitRuleHostInput.validate("2001:db8::/129").error == .badRange) + } + + @Test func anEmptyFieldIsNeitherAcceptedNorAnError() { + let validation = SplitRuleHostInput.validate(" ") + #expect(validation.normalized == nil) + #expect(validation.error == nil) + #expect(!validation.isAccepted) + } + + @Test func duplicatesAreRefused() { + #expect(SplitRuleHostInput.validate("example.com", existing: ["example.com"]).error == .duplicate) + // the dedupe is against the NORMALIZED form, not the typed one + #expect(SplitRuleHostInput.validate("EXAMPLE.com", existing: ["example.com"]).error == .duplicate) + } + + /// The rule row collapses a bare name into a wildcard that already covers + /// it, so adding both would show one chip where two values were typed. + @Test func valuesAlreadyCoveredByAWildcardAreRefused() { + #expect(SplitRuleHostInput.validate("a.example.com", existing: ["*.example.com"]).error + == .covered(by: "*.example.com")) + #expect(SplitRuleHostInput.validate("example.com", existing: ["**.example.com"]).error + == .covered(by: "**.example.com")) + // *. is subdomains only, so the bare base is not covered by it + #expect(SplitRuleHostInput.validate("example.com", existing: ["*.example.com"]).isAccepted) + // an unrelated name is not covered + #expect(SplitRuleHostInput.validate("example.org", existing: ["*.example.com"]).isAccepted) + } + + @Test func everyRejectionHasSomethingToSay() { + let errors: [SplitRuleHostError] = [ + .notAscii, .badName, .badWildcard, .badRange, .duplicate, .covered(by: "*.example.com"), + ] + for error in errors { + #expect(!SplitRuleHostInput.message(for: error).isEmpty) + } + } +} From 06fee857fbd148fd9d9c6948c15fe280ebc69429 Mon Sep 17 00:00:00 2001 From: Ryanmello07 <67509637+Ryanmello07@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:52:40 -0700 Subject: [PATCH 2/2] fix the trap when a split rule range ends inside a byte `UInt8(0xff << (8 - keep))` builds the mask in Int: `0xff` is an Int literal, so for every prefix length that is not a whole number of bytes the shift produces 510 to 32640 and the conversion traps. The field is validated on every keystroke, so this was not a corner case -- typing "10.0.0.0/24" crashed the app at the "2", on the way to a length the tests did cover. The two range cases that existed, /8 and /24, are both byte-aligned, so `keep` was always 8, `0xff << 0` fit, and the suite passed straight over it. Build the mask in UInt8 throughout, and cover it two ways: the specific non-aligned lengths with their expected network addresses, and a sweep of every length 0...32 and 0...128 asserting only that the validator returns rather than traps. The sweep is the one that generalises -- the user types through every prefix length on the way to the one they want. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AWV4MaF9JBcQppSX9UxATa --- .../Shared/Utilities/SplitRuleHostInput.swift | 9 ++++++-- .../SplitRuleHostInputTests.swift | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/app/network/Shared/Utilities/SplitRuleHostInput.swift b/app/network/Shared/Utilities/SplitRuleHostInput.swift index c404f2c9..d7517a56 100644 --- a/app/network/Shared/Utilities/SplitRuleHostInput.swift +++ b/app/network/Shared/Utilities/SplitRuleHostInput.swift @@ -184,8 +184,13 @@ enum SplitRuleHostInput { if bits <= bitsBefore { bytes[index] = 0 } else if bits < bitsBefore + 8 { - let keep = bits - bitsBefore - bytes[index] &= UInt8(0xff << (8 - keep)) + // keep is 1...7 here: this byte is the one the prefix ends + // inside. The mask is built in UInt8 throughout -- `0xff` is + // an Int literal, so shifting it left and converting back + // traps for every prefix length that is not a whole number + // of bytes, which is most of them. + let dropped = UInt8(8 - (bits - bitsBefore)) + bytes[index] &= ~((UInt8(1) << dropped) &- 1) } } return Data(bytes) diff --git a/app/networkTests/SplitRuleHostInputTests.swift b/app/networkTests/SplitRuleHostInputTests.swift index 633138b4..65b75974 100644 --- a/app/networkTests/SplitRuleHostInputTests.swift +++ b/app/networkTests/SplitRuleHostInputTests.swift @@ -79,6 +79,28 @@ struct SplitRuleHostInputTests { #expect(validation.note == "192.168.1.0/24") } + /// Prefix lengths that end inside a byte. Every case here traps rather + /// than fails when the mask is built in Int, and the field is validated on + /// every keystroke -- so typing "10.0.0.0/24" crashed the app at the "2", + /// on its way to a length the earlier tests did cover. + @Test func rangesThatEndInsideAByteAreMasked() { + #expect(SplitRuleHostInput.validate("10.1.2.3/12").normalized == "10.0.0.0/12") + #expect(SplitRuleHostInput.validate("192.168.1.130/25").normalized == "192.168.1.128/25") + #expect(SplitRuleHostInput.validate("192.168.1.42/2").normalized == "192.0.0.0/2") + #expect(SplitRuleHostInput.validate("2001:db8::1/36").normalized == "2001:db8::/36") + } + + /// Every prefix length must be survivable, because the user types through + /// all of them one keystroke at a time. + @Test func everyPrefixLengthIsSurvivable() { + for bits in 0...32 { + _ = SplitRuleHostInput.validate("192.168.1.42/\(bits)") + } + for bits in 0...128 { + _ = SplitRuleHostInput.validate("2001:db8::1/\(bits)") + } + } + @Test func alreadyMaskedRangesCarryNoNote() { let validation = SplitRuleHostInput.validate("10.0.0.0/8") #expect(validation.normalized == "10.0.0.0/8")