feat: add CIDR/subnet support in VPN Only mode#34
Conversation
Allow users to enter CIDR ranges (e.g., 192.168.1.0/24) in VPN Only mode, routing entire subnets through VPN instead of only individual domain-resolved IPs. This addresses the limitation reported in #33 where users with IKEv2 VPNs could not route their home LAN subnet. Changes: - Add isCIDR flag to DomainEntry with backward-compatible decoding - Detect CIDR input in addInverseDomain, bypass cleanDomain - Route CIDRs as network routes (isNetwork: true) in all code paths: initial apply, cached apply, DNS refresh, toggle, and live add - Skip CIDR entries from DNS resolution and hosts file management - Reject /0 mask to prevent conflict with catch-all routes - Reject leading-zero IP octets (octal interpretation mismatch) - Add CIDR badge in domain list UI, update placeholder text - Add 39 new tests for CIDR validation, Codable, and classification - Bump helper version to 1.5.0 (validation behavior change) Closes #33
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 51 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request adds support for CIDR notation and IP range entries in VPN bypass routing, enabling users to specify entire subnets (e.g., Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Helper/Info.plist`:
- Around line 11-14: The helper version is hardcoded in
Sources/HelperProtocol.swift (variable helperVersion) and must be replaced with
a runtime read from the helper bundle's Info.plist CFBundleShortVersionString;
update the code that sets helperVersion to fetch Bundle(for: <type>?) or
Bundle(identifier: "<helper bundle id>") and read object(forInfoDictionaryKey:
"CFBundleShortVersionString") (with a safe fallback) instead of the literal
"1.5.0" so the value stays synchronized with the Helper/Info.plist bump.
In `@Sources/HelperProtocol.swift`:
- Line 81: Replace the hardcoded static let helperVersion = "1.5.0" with a
computed property that reads CFBundleShortVersionString from the helper's bundle
at runtime (e.g., use Bundle(for: HelperProtocol.self) or the appropriate bundle
for the helper target), returning that string and providing a sensible fallback
(like "0.0.0") if the key is missing; update the symbol helperVersion in
HelperProtocol to be a computed String property that fetches Info.plist's
CFBundleShortVersionString instead of a hardcoded value.
In `@Sources/RouteManager.swift`:
- Around line 2566-2577: In addInverseDomain(_:), avoid passing strings that
look like CIDRs (contain "/") into cleanDomain; instead check if the trimmed
input contains "/" and if so validate with isValidCIDR and reject (return) when
isValidCIDR is false; otherwise if contains no "/", call cleanDomain as before
and guard against empty result—use the existing symbols addInverseDomain,
isValidCIDR, and cleanDomain to implement this branch.
In `@Tests/VPNBypassTests/VPNBypassTests.swift`:
- Around line 302-330: Replace the local test-only DomainEntry struct with the
production type so the test validates actual decoding logic: remove the nested
struct in DomainEntryCodableTests and use RouteManager.DomainEntry (or the real
DomainEntry type from the app) instead; ensure the test target imports the app
module (e.g. add `@testable` import YourAppModule) and construct/encode/decode
RouteManager.DomainEntry instances (or add a small test-only initializer/wrapper
if needed) so the test asserts the production DomainEntry's init(from:) handles
decodeIfPresent(.isCIDR) and keys correctly.
- Around line 202-211: The CIDR validation must consistently reject a /0 mask;
update the isValidCIDR function so the mask range is explicitly 1...32 (ensure
the guard uses mask >= 1 && mask <= 32) and mirror this same check where CIDR
parsing/validation is duplicated (the similar helper around the other CIDR
handling block) so default routes (/0) are treated as invalid for VPN-only
input.
- Line 689: The test currently uses a hardcoded string literal ("version") which
makes the assertion meaningless; change the test to read the helper’s real
version (either by referencing the production constant
HelperConstants.helperVersion or by loading the helper bundle’s
CFBundleShortVersionString) instead of the literal, e.g., import the app module
with `@testable` to access HelperConstants or load the helper bundle plist and
compare that value to the code-under-test; update the variable named version in
VPNBypassTests.swift to obtain the runtime version from
HelperConstants.helperVersion or from the helper bundle’s
CFBundleShortVersionString before asserting.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7852820b-25d7-448d-9213-e9ae2e89770f
📒 Files selected for processing (7)
Helper/HelperTool.swiftHelper/Info.plistSources/HelperProtocol.swiftSources/MenuBarViews.swiftSources/RouteManager.swiftSources/SettingsView.swiftTests/VPNBypassTests/VPNBypassTests.swift
|
|
||
| struct HelperConstants { | ||
| static let helperVersion = "1.4.0" | ||
| static let helperVersion = "1.5.0" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Read the helper version from the bundle, not a hardcoded string.
This can drift from Info.plist and break helper readiness/upgrade checks.
Proposed fix
struct HelperConstants {
- static let helperVersion = "1.5.0"
+ static var helperVersion: String {
+ Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String ?? "Unknown"
+ }
static let bundleID = "com.geiserx.vpnbypass.helper"As per coding guidelines, **/*.swift: Do NOT hardcode version strings in Swift code. The app must read the version from the bundle's CFBundleShortVersionString at runtime.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static let helperVersion = "1.5.0" | |
| struct HelperConstants { | |
| static var helperVersion: String { | |
| Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String ?? "Unknown" | |
| } | |
| static let bundleID = "com.geiserx.vpnbypass.helper" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/HelperProtocol.swift` at line 81, Replace the hardcoded static let
helperVersion = "1.5.0" with a computed property that reads
CFBundleShortVersionString from the helper's bundle at runtime (e.g., use
Bundle(for: HelperProtocol.self) or the appropriate bundle for the helper
target), returning that string and providing a sensible fallback (like "0.0.0")
if the key is missing; update the symbol helperVersion in HelperProtocol to be a
computed String property that fetches Info.plist's CFBundleShortVersionString
instead of a hardcoded value.
There was a problem hiding this comment.
Not applicable — the helper is a standalone swiftc-compiled binary running as a launchd daemon at /Library/PrivilegedHelperTools/. Bundle.main won't have an Info.plist to read from. The hardcoded constant is intentional — it's shared between the app (which checks it via XPC getVersion) and the helper (which reports it). Both sides need to agree on the version string at compile time.
There was a problem hiding this comment.
@GeiserX, that's a valid and important architectural distinction — acknowledged. A bare swiftc-compiled launchd daemon has no bundle, so Bundle.main is indeed the wrong approach here. The hardcoded constant as a shared compile-time contract between the app and helper over XPC is the correct pattern for this setup. I'll retract the suggestion.
✏️ Learnings added
Learnt from: GeiserX
Repo: GeiserX/VPN-Bypass PR: 34
File: Sources/HelperProtocol.swift:81-81
Timestamp: 2026-04-19T22:30:16.255Z
Learning: In the VPN-Bypass project, `HelperConstants.helperVersion` in `Sources/HelperProtocol.swift` is intentionally hardcoded. The privileged helper is a standalone `swiftc`-compiled binary (not an app bundle) running as a launchd daemon at `/Library/PrivilegedHelperTools/`. `Bundle.main` has no Info.plist in that context, so reading `CFBundleShortVersionString` at runtime is not feasible. The constant is a compile-time contract shared between the main app (which checks it via XPC `getVersion`) and the helper (which reports it). Do NOT suggest replacing it with a bundle read.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: GeiserX/VPN-Bypass PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-16T15:10:58.933Z
Learning: Applies to **/*.swift : Helper readiness must be authoritative everywhere in Swift code—startup, refresh, reroute, DNS refresh, and hosts file updates must all refuse to proceed when the helper is not verified ready
Learnt from: CR
Repo: GeiserX/VPN-Bypass PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-16T15:10:58.933Z
Learning: The privileged helper is auto-installed on first launch and auto-updated on version mismatch. Do not reintroduce helperless fallbacks (direct `/sbin/route`, AppleScript) that bypass the helper—all route-mutating operations require the helper.
Learnt from: CR
Repo: GeiserX/VPN-Bypass PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-16T15:10:58.933Z
Learning: Applies to **/*.swift : Do NOT hardcode version strings in Swift code. The app must read the version from the bundle's `CFBundleShortVersionString` at runtime.
Learnt from: CR
Repo: GeiserX/VPN-Bypass PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-16T15:10:58.933Z
Learning: Applies to **/*.swift : SMAppService.register() does NOT replace existing helper binaries—when updating, always route to the legacy AppleScript path that does actual `cp` + `launchctl bootout/bootstrap`
Learnt from: CR
Repo: GeiserX/VPN-Bypass PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-16T15:10:58.933Z
Learning: Applies to **/*.swift : On macOS Sonoma+, users can disable VPN Bypass's background item in System Settings → General → Login Items & Extensions, which blocks the helper daemon. When disabled, the app will re-prompt for admin on every launch. Detect this via `SMAppService.daemon().status` and show a helpful error message instead of re-prompting.
Learnt from: CR
Repo: GeiserX/VPN-Bypass PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-16T15:10:58.933Z
Learning: Ship universal binaries for both the main app and privileged helper (arm64 + x86_64). Use `swift build --arch arm64 --arch x86_64` for the main app (outputs to `.build/apple/Products/Release`), and separate arch builds plus `lipo` for the helper.
Learnt from: CR
Repo: GeiserX/VPN-Bypass PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-16T15:10:58.933Z
Learning: Applies to **/*.swift : Some VPNs route via interface link (e.g., Cisco Secure Client sets default route to `link#N`) instead of IP gateway. Handle this via `iface:<interface>` convention: use `route add -host <dest> -interface utunX` instead of IP gateway. See VPN Only mode implementation.
| /// Tests for DomainEntry encoding/decoding, especially backward compatibility with isCIDR field. | ||
| final class DomainEntryCodableTests: XCTestCase { | ||
|
|
||
| // Minimal reimplementation of DomainEntry for Codable testing | ||
| struct DomainEntry: Codable, Identifiable, Equatable { | ||
| let id: UUID | ||
| var domain: String | ||
| var enabled: Bool | ||
| var resolvedIP: String? | ||
| var lastResolved: Date? | ||
| var isCIDR: Bool | ||
|
|
||
| init(domain: String, enabled: Bool = true, isCIDR: Bool = false) { | ||
| self.id = UUID() | ||
| self.domain = domain | ||
| self.enabled = enabled | ||
| self.isCIDR = isCIDR | ||
| } | ||
|
|
||
| init(from decoder: Decoder) throws { | ||
| let container = try decoder.container(keyedBy: CodingKeys.self) | ||
| id = try container.decode(UUID.self, forKey: .id) | ||
| domain = try container.decode(String.self, forKey: .domain) | ||
| enabled = try container.decode(Bool.self, forKey: .enabled) | ||
| resolvedIP = try container.decodeIfPresent(String.self, forKey: .resolvedIP) | ||
| lastResolved = try container.decodeIfPresent(Date.self, forKey: .lastResolved) | ||
| isCIDR = try container.decodeIfPresent(Bool.self, forKey: .isCIDR) ?? false | ||
| } | ||
| } |
There was a problem hiding this comment.
Test the production DomainEntry, not a duplicate struct.
This local reimplementation can pass even if RouteManager.DomainEntry forgets decodeIfPresent(.isCIDR) or encodes different keys. Use the real production type via the test target instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/VPNBypassTests/VPNBypassTests.swift` around lines 302 - 330, Replace
the local test-only DomainEntry struct with the production type so the test
validates actual decoding logic: remove the nested struct in
DomainEntryCodableTests and use RouteManager.DomainEntry (or the real
DomainEntry type from the app) instead; ensure the test target imports the app
module (e.g. add `@testable` import YourAppModule) and construct/encode/decode
RouteManager.DomainEntry instances (or add a small test-only initializer/wrapper
if needed) so the test asserts the production DomainEntry's init(from:) handles
decodeIfPresent(.isCIDR) and keys correctly.
There was a problem hiding this comment.
Acknowledged but not actionable — the test target is standalone with no dependency on the main VPNBypass executable target (see Package.swift). Adding @testable import would require restructuring the target graph to extract DomainEntry into a shared library target, which is out of scope for this feature PR. The local reimplementation mirrors the production struct and validates the same Codable contract.
There was a problem hiding this comment.
@GeiserX, understood — that's a valid constraint. With a standalone test target and no shared library layer, the local reimplementation is the practical approach. I'll note this for future reviews.
✏️ Learnings added
Learnt from: GeiserX
Repo: GeiserX/VPN-Bypass PR: 34
File: Tests/VPNBypassTests/VPNBypassTests.swift:302-330
Timestamp: 2026-04-19T22:30:14.596Z
Learning: In GeiserX/VPN-Bypass, the test target (`Tests/VPNBypassTests`) is standalone with no `testable import` dependency on the main `VPNBypass` executable target. Restructuring the target graph (e.g., extracting types into a shared library) is considered out of scope. Local reimplementations of production types (e.g., `DomainEntry`) in test files are intentional and acceptable given this constraint. Do not flag them as duplicates or suggest `testable import` as a fix.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: GeiserX/VPN-Bypass PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-16T15:10:58.933Z
Learning: Applies to **/*.swift : Some VPNs route via interface link (e.g., Cisco Secure Client sets default route to `link#N`) instead of IP gateway. Handle this via `iface:<interface>` convention: use `route add -host <dest> -interface utunX` instead of IP gateway. See VPN Only mode implementation.
- Reject slash-containing input that fails CIDR validation instead of falling through to cleanDomain (which strips the slash and adds the bare IP as a domain entry) - Align test isValidCIDR to reject /0 mask (matching production code), while keeping isValidDestination (helper validation) allowing /0
1 participant
Summary
192.168.1.0/24) to VPN Only mode entries, enabling users to route entire subnets through VPNCloses #33
Changes
DomainEntrygainsisCIDRflag (backward-compatible Codable). CIDR input bypassescleanDomain()and DNS resolution, routing directly viaisNetwork: true. Toggle re-enable fixed for CIDR entries. DNS refresh skips CIDRs. Hosts file skips CIDRs.isValidIP()rejects leading zeros (octal mismatch prevention)How it works
192.168.1.0/24in VPN Only domain inputisValidCIDR()validates format (rejects /0 to avoid catch-all conflict)isCIDR: true, bypasses DNS resolution pipeline-netflag (pre-existing infrastructure)Test plan
192.168.1.0/24in VPN Only mode, verify route appears innetstat -rnSummary by CodeRabbit
New Features
Bug Fixes
Chores