Skip to content

fix(socket): find a domain by the link that added it, not refuse it as a CIDR - #155

Merged
GeiserX merged 3 commits into
mainfrom
fix/socket-link-remove-enable
Oct 2, 2026
Merged

GeiserX merged 3 commits into
mainfrom
fix/socket-link-remove-enable

Conversation

@GeiserX

@GeiserX GeiserX commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner

vpnb domain.add domain=example.com/page saves example.com on the Bypass list. Removing, enabling or disabling it with that same value answered invalid_args: malformed CIDR. The cause was ClassicControl.parseTarget, which refused any / without :// in front as a broken IP range, while domain.add goes through RouteManager.checkDomainInput and keeps the host.

parseTarget now runs checkDomainInput once per list it searches, so add and lookup follow one rule:

  • Bypass list. A link (https://example.com/page, example.com/page, Example.com:8080/a?b=c) finds example.com. An IP range is refused with the bypass list holds domain names, not a CIDR. The old code looked it up and answered not_found.
  • list=vpnOnly. A value with a / must be a CIDR. Anything else is refused, as domain.add refuses it.
  • No list=. A link is looked up on both lists by its host. A domain on both lists still answers that domain is on both lists; add list=bypass or list=vpnOnly, and a domain only on VPN Only is still found, as before. A range is looked up only on VPN Only. If every list refuses the value, the VPN Only reason comes back (malformed, or /0 and /1).

A malformed range like 10.0.0.0/33 is still refused on every list and never trimmed to the host entry 10.0.0.0. Nothing here writes a route, and the GUI does not change. The GUI removes and toggles by entry, so it never parses a typed value. One GUI add path does skip checkDomainInput. In Custom mode the dropdown quick-add calls addDomainRuleToDirect, which runs plain cleanDomain, so a typed 10.0.0.0/24 still becomes a domain rule for the host 10.0.0.0. That was there before this PR and is left for its own fix.

One change in behaviour to push back on if you disagree: domain.rm domain=https://example.com list=vpnOnly now returns invalid_args. It used to find example.com. That is what domain.add says for the same value on that list. Without list= the same value behaves as it did on main.

Tests. Five new cases in ClassicControlTests: a bare host, an https link, a schemeless link with a path, and a link with a port and query, through add, disable, enable and rm on the Bypass list with and without list=; links on VPN Only; a link with no list= against a domain on both lists and one only on VPN Only; a real CIDR on each list; a malformed range through every verb on every list.

  • With origin/main's ClassicControl.swift they fail: testBypassLookupTakesEveryValueAddTakes, testCIDRLookupOnEachList and testVPNOnlyLookupRefusesALinkWithASlashLikeAdd go red (example.com/page list=bypass: ... "malformed CIDR ...").
  • Changing the check to always use .bypass sends five tests red, among them testMalformedRangeIsRefusedByEveryVerbOnEveryList and the existing testVPNOnlyDomainEnableDisable.
  • Dropping the no-list= host lookup on VPN Only sends testLinkWithoutListSearchesBothListsByHost red: domain.disable https://example.com returns ok instead of the both-lists answer, and with the entry only on VPN Only it returns not_found.
  • Full suite on the Mac mini after merging main: Executed 1467 tests, with 10 tests skipped and 0 failures.

Docs: the verb notes in docs/usage.md and an [Unreleased] Fixed entry in docs/CHANGELOG.md. Socket error text is English only like the rest of it, so there are no new strings and no screenshots. The MCP server's remove_domain and set_domain_enabled pass the value through unchanged and get the new answers without a change on their side.

…dd took

domain.add saves example.com/page as example.com on the Bypass list through
RouteManager.checkDomainInput, but parseTarget refused the same value as a
malformed CIDR because it treated any slash without a scheme as a broken IP
range. parseTarget now reads the value with checkDomainInput for each list it
searches, so the lookup and the add follow one rule.
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: GeiserX/VPN-Bypass/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 97c31505-108b-49c3-8a8f-3ddf907cff24

📥 Commits

Reviewing files that changed from the base of the PR and between 282285d and b78b868.

📒 Files selected for processing (4)
  • Sources/VPNBypassCore/ClassicControl.swift
  • Tests/VPNBypassTests/ClassicControlTests.swift
  • docs/CHANGELOG.md
  • docs/usage.md

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

Domain lookups now validate raw input separately for each selected list and use each list’s accepted value as its lookup key. Tests cover host, URL, path, and CIDR inputs across lookup verbs. The usage documentation and changelog describe these rules.

Changes

Domain lookup

Layer / File(s) Summary
Parse and match domain inputs
Sources/VPNBypassCore/ClassicControl.swift
parseTarget validates the raw domain for each selected list and keeps accepted cleaned values as list-specific lookup keys. findDomain uses the key for the current list.
Verify and document lookup behavior
Tests/VPNBypassTests/ClassicControlTests.swift, docs/CHANGELOG.md, docs/usage.md
Tests cover domain and CIDR inputs across lookup verbs. The changelog and usage documentation describe list-specific parsing and validation.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to b78b8

This change makes domain removal, enable and disable accept the same pasted links as domain add on the Bypass list, and it refuses malformed ranges. It has tests and documentation, and no merge-blocking risk is evident.

Security Architecture Review

Security architecture risk: 🔵 Low · up to b78b8

Domain-management commands now follow each list’s existing acceptance rules. Same-user access checks and list-scoped selection remain intact, and no new security concern was identified in the reviewed path. Failure-recovery and deployment coverage remain limited.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The reviewed path affects the app’s Bypass and VPN Only configuration and their associated local routes and optional hosts-file updates. Newly accepted Bypass link syntax names hosts already addressable through domain-management commands rather than granting a new category of mutation authority.

Trust Boundaries and Controls

  • observed — The existing socket boundary checks peer identity with getpeereid and rejects peers whose UID differs from the app’s UID before handing the connection to request processing. The changed parser operates behind that gate.
  • observed — Explicit list selection limits both validation and matching. Without a list, matching searches only accepting lists and rejects multiple matches instead of choosing one implicitly.

Resilience and Maintainability Implications

  • observed — Existing removal awaits a cleanup task when one is returned, but can persist removal with route cleanup deferred. Existing toggles persist the enabled state before asynchronous route work and use epoch checks to clean up stale route additions. Successful command responses therefore are not a general atomic guarantee of completed routing changes; this PR leaves those mechanisms unchanged.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files. (2 skipped: 2 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the socket lookup fix and the malformed CIDR issue. It is concise and directly matches the primary change.
Description check ✅ Passed The description provides a detailed summary, behavioral changes, test coverage, reported results, documentation updates, and known scope limits. It does not reproduce the template headings or checkbox…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

… lists

Reading a link by the VPN Only add rule dropped that list from the search
when no list= was given, so https://example.com acted on the Bypass entry of
a domain on both lists, and missed one only on VPN Only. Without list=, a
link is now looked up on VPN Only by its host too.
@GeiserX
GeiserX merged commit 40433ab into main Oct 2, 2026
5 checks passed
@GeiserX
GeiserX deleted the fix/socket-link-remove-enable branch October 2, 2026 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant