Move the sysproxy package back into zen-desktop #549
Conversation
git-subtree-dir: internal/sysproxy/exclusions git-subtree-split: 8d6cd6e17618a90c07f1c3ae9ba988a8d9f97542
…sysproxy/exclusions'
…internal/sysproxy/exclusions subtree See https://www.atlassian.com/git/tutorials/git-subtree for more details on subtrees.
WalkthroughThis PR introduces a complete system proxy management subsystem by adding PAC-based proxy configuration orchestration, platform-specific implementations for macOS, Linux (KDE/GNOME), and Windows, bundled with exclusion lists for common, platform-specific, and user-configured hosts, and migrating sysproxy from an external zen-core dependency to internal implementation. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Manager
participant PAC Generator
participant HTTP Server
participant OS Proxy
Client->>Manager: Set(proxyPort, excludedHosts)
Manager->>PAC Generator: renderPac(proxyPort, excludedHosts)
PAC Generator->>PAC Generator: buildExcludedHosts()
PAC Generator-->>Manager: PAC bytes
Manager->>HTTP Server: makeServer(pac bytes)
HTTP Server->>HTTP Server: listen on port
HTTP Server-->>Manager: actual PAC port
Manager->>OS Proxy: setSystemProxy(PAC URL)
OS Proxy->>OS Proxy: Apply platform-specific config
Note over OS Proxy: macOS: networksetup<br/>Linux: kwriteconfig/gsettings<br/>Windows: registry
OS Proxy-->>Manager: success
Manager-->>Client: ready
Client->>Manager: Clear()
Manager->>OS Proxy: unsetSystemProxy()
OS Proxy->>OS Proxy: Revert proxy config
OS Proxy-->>Manager: success
Manager->>HTTP Server: shutdown
Manager-->>Client: cleaned up
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
internal/sysproxy/exclusions/darwin.txt (1)
1-187: Consider removing duplicate hostname entries.Several hostnames appear multiple times in this file:
ws-ee-maidsvc.icloud.com(lines 40, 52)gdmf.apple.com(lines 20, 58)gs.apple.com(lines 10, 60)mzstatic.com(lines 34, 81)itunes.apple.com(lines 33, 79, 82)appattest.apple.com(lines 25, 85)appleid.cdn-apple.com(lines 31, 126)idmsa.apple.com(lines 32, 127)While the PAC rendering logic likely handles duplicates, keeping the source file deduplicated improves maintainability and makes updates easier.
internal/sysproxy/system_linux.go (1)
64-90: Consider extracting kwriteconfig detection to a helper function.The logic to detect
kwriteconfig6vskwriteconfig5is duplicated in bothsetKDEProxyandunsetKDEProxy. A small helper would reduce duplication:func getKWriteConfig() (string, error) { if binaryExists("kwriteconfig6") { return "kwriteconfig6", nil } if binaryExists("kwriteconfig5") { return "kwriteconfig5", nil } return "", fmt.Errorf("kwriteconfig not found in PATH") }Also applies to: 130-156
internal/sysproxy/system_darwin.go (1)
82-92: Disabled services (with asterisk) are still configured.The code strips the asterisk from disabled services but then still includes them in the list. Per Line 80's comment, asterisk denotes disabled services. Configuring proxy settings on disabled services is likely unnecessary and could produce errors or warnings.
for _, raw := range lines[1:] { line := strings.TrimSpace(string(raw)) if line == "" { continue } if line[0] == '*' { - // Disabled service; remove the asterisk. - line = strings.TrimSpace(line[1:]) + // Disabled service; skip it. + continue } services = append(services, line) }internal/sysproxy/manager.go (3)
77-81: Ignored error fromw.Write(pac).The return value of
w.Writeis discarded. While this is common in HTTP handlers, logging the error could help diagnose client disconnection issues.mux.HandleFunc("/proxy.pac", func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/x-ns-proxy-autoconfig") w.WriteHeader(http.StatusOK) - w.Write(pac) + if _, err := w.Write(pac); err != nil { + log.Printf("error writing PAC response: %v", err) + } })
56-71: Consider reversing the order: close server before unsetting system proxy.Currently,
unsetSystemProxyis called first (Line 62), then the server is closed (Line 66). During this brief window, the OS may still try to fetch the PAC from the now-orphaned URL. Consider closing the server first to ensure no stale requests are served, then unsetting the proxy.Alternatively, the current order is acceptable if you prefer the proxy to remain functional until the very last moment.
83-91: Consider addingIdleTimeoutfor connection hygiene.The server sets
ReadTimeoutandWriteTimeoutbut notIdleTimeout. For a localhost-only PAC server with presumably infrequent requests, this is minor, but settingIdleTimeoutensures idle keep-alive connections are cleaned up.m.server = &http.Server{ Handler: mux, ReadTimeout: time.Minute, WriteTimeout: time.Minute, + IdleTimeout: time.Minute, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Taskfile.yml(1 hunks)go.mod(1 hunks)internal/app/app.go(1 hunks)internal/sysproxy/exclusions/.gitignore(1 hunks)internal/sysproxy/exclusions/LICENSE(1 hunks)internal/sysproxy/exclusions/common.txt(1 hunks)internal/sysproxy/exclusions/darwin.txt(1 hunks)internal/sysproxy/exclusions/windows.txt(1 hunks)internal/sysproxy/manager.go(1 hunks)internal/sysproxy/pac.go(1 hunks)internal/sysproxy/system_darwin.go(1 hunks)internal/sysproxy/system_linux.go(1 hunks)internal/sysproxy/system_windows.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T11:40:45.127Z
Learnt from: spellcascade
Repo: ZenPrivacy/zen-desktop PR: 436
File: internal/app/app.go:191-202
Timestamp: 2025-09-05T11:40:45.127Z
Learning: In the ZenPrivacy/zen-desktop codebase, the user spellcascade prefers simpler approaches over complex error handling mechanisms when the added complexity outweighs the benefit, particularly for resource cleanup scenarios in internal/app/app.go.
Applied to files:
go.mod
📚 Learning: 2025-10-13T14:59:27.658Z
Learnt from: Speedauge
Repo: ZenPrivacy/zen-desktop PR: 481
File: internal/sysproxy/exclusions/common.txt:13-13
Timestamp: 2025-10-13T14:59:27.658Z
Learning: For BNP Paribas login exclusions in `internal/sysproxy/exclusions/common.txt`, the correct domain is `connexion-mabanque.bnpparibas` (with hyphens), not `connexion.mabanque.bnpparibas` (with dots).
Applied to files:
internal/sysproxy/exclusions/common.txt
🧬 Code graph analysis (1)
internal/sysproxy/system_linux.go (1)
internal/sysproxy/manager.go (1)
ErrUnsupportedDesktopEnvironment(22-22)
🪛 LanguageTool
internal/sysproxy/exclusions/common.txt
[style] ~29-~29: Mogelijk kunt u beter een Nederlands woord gebruiken.
Context: ...gov.ph gov.au e-estonia.com eesti.ee # Password managers 1password.com lastpass.com pas...
(PASSWORD)
[style] ~31-~31: Mogelijk kunt u beter een Nederlands woord gebruiken.
Context: ...ord managers 1password.com lastpass.com passwords.google.com passwords.google bitwarden.c...
(PASSWORD)
[style] ~32-~32: Mogelijk kunt u beter een Nederlands woord gebruiken.
Context: ...d.com lastpass.com passwords.google.com passwords.google bitwarden.com dashlane.com # Ba...
(PASSWORD)
[grammar] ~56-~56: Dit kan een fout zijn.
Context: ...z bcc.kz bankffin.kz jusan.kz onlinebank.kz freedompay.kz wlp-acs.com # Payment processors paypal.com stripe.com...
(QB_NEW_NL)
[grammar] ~108-~108: Woord verwijderen
Context: ....apple.com auth0.com hanko.io clerk.com workos.com zitadel.com stytch.com bunny.net # OpenAI from here: https://help.openai.co...
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_NOUN)
internal/sysproxy/exclusions/windows.txt
[grammar] ~3-~3: Taalfout gevonden
Context: ...at Zen will not MITM. # Windows Update and Microsoft services eus-streaming-video-...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_OTHER)
[grammar] ~3-~3: Taalfout gevonden
Context: ...ot MITM. # Windows Update and Microsoft services eus-streaming-video-msn.com aka...
(QB_NEW_NL_OTHER_ERROR_IDS_REPLACEMENT_OTHER)
[grammar] ~25-~25: Woord verwijderen
Context: ...auth.xboxlive.com dlassets.xboxlive.com images-eds.xboxlive.com pf.directory.live.com privacy.xboxlive.com profile.xboxlive.com
(QB_NEW_NL_OTHER_ERROR_IDS_UNNECESSARY_NOUN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build Windows (arm64)
- GitHub Check: Build Windows (amd64)
- GitHub Check: Build Linux (arm64)
- GitHub Check: Build macOS (arm64)
- GitHub Check: Build macOS (amd64)
- GitHub Check: Build Linux (amd64)
- GitHub Check: Test Go (windows-latest)
- GitHub Check: Lint Go (windows-latest)
🔇 Additional comments (16)
internal/sysproxy/exclusions/.gitignore (1)
1-1: LGTM!Standard
.DS_Storeexclusion for macOS artifacts.internal/sysproxy/exclusions/LICENSE (1)
1-21: LGTM!Standard MIT license for the exclusions data asset sourced from the zen-https-exclusions repository.
internal/sysproxy/exclusions/windows.txt (1)
1-29: LGTM!Well-organized Windows exclusion list covering Windows Update, Microsoft services, and Xbox Live domains. The static analysis hints are false positives from grammar checking on domain names.
internal/sysproxy/exclusions/common.txt (1)
1-123: LGTM!Well-organized common exclusion list with appropriate categorization. The BNP Paribas domain
connexion-mabanque.bnpparibasuses the correct hyphenated format. Based on learnings, this is the correct domain spelling.internal/sysproxy/system_linux.go (4)
15-36: LGTM!Desktop environment detection is thorough, checking
XDG_CURRENT_DESKTOPfirst, then falling back toDESKTOP_SESSIONandKDE_FULL_SESSIONfor edge cases.
38-62: LGTM!The cross-desktop-environment approach is pragmatic—applying GNOME proxy settings on KDE ensures Firefox-based browsers work correctly since they ignore KDE proxy configuration. The warning log for non-fatal failures is appropriate.
173-178: LGTM!The 3-second timeout is reasonable for system commands, and using
exec.CommandContextensures proper cancellation. The#nosec G204comment correctly suppresses the gosec warning since the command arguments are controlled internally.
13-13: PAC rendering logic safely handles nilplatformSpecificExcludedHostson Linux—no changes required.The
buildExcludedHostsfunction inpac.gousesbytes.Split()to process exclusion lists. WhenplatformSpecificExcludedHostsis nil (as declared on Linux),bytes.Split(nil, separator)returns an empty slice, causing the loop to iterate zero times. This gracefully handles the nil case without any nil pointer dereference or errors. The implementation is correct.go.mod (1)
11-11: LGTM!Promoting
go-multierrorfrom indirect to direct is correct since the new internal sysproxy package explicitly imports it for error aggregation.internal/app/app.go (1)
31-31: LGTM!The import path migration from external
zen-core/sysproxyto internalzen-desktop/internal/sysproxyis clean. The usage ofsysproxy.Managerand its methods (NewManager,Set,Clear) remains consistent.Taskfile.yml (1)
53-56: LGTM!The
pull-exclusionstask is well-structured. Using--squashkeeps the commit history clean when syncing from the upstream exclusions repository.internal/sysproxy/pac.go (1)
40-61: LGTM!The
buildExcludedHostsfunction correctly:
- Strips comments after
#- Trims whitespace and skips empty lines
- Merges common, platform-specific, and user-configured exclusions in proper order
internal/sysproxy/system_windows.go (1)
22-40: LGTM!The
setSystemProxyfunction correctly:
- Opens the registry with appropriate access
- Disables manual proxy (
ProxyEnable = 0) when using PAC- Sets the
AutoConfigURLfor PAC-based configuration- Triggers settings refresh via Windows API
internal/sysproxy/system_darwin.go (1)
67-78: LGTM on discoverNetworkServices structure.The function correctly parses
networksetup -listallnetworkservicesoutput, skips the header line, and handles empty lines. Error messages include command output for debugging.internal/sysproxy/manager.go (2)
1-11: Good documentation explaining the PAC design decision.The package-level documentation clearly explains why PAC is used over declarative configuration (character limits on Windows/macOS). The exported error
ErrUnsupportedDesktopEnvironmentis well-named.Also applies to: 22-22
29-36: LGTM onNewManagerandmakeServerstructure.The constructor is simple and the server setup correctly handles port 0 for random port assignment, returns the actual port, and starts serving in a goroutine with proper error handling for
http.ErrServerClosed.Also applies to: 73-102
What does this PR do?
Moves the
sysproxypackage back intointernal/sysproxyand updatesapp.goaccordingly.To note: The HTTPS exclusions now live in a separate
zen-https-exclusionsrepository so it can be shared between platforms and editions. For easier management, it's nested usinggit subtree(see https://www.atlassian.com/git/tutorials/git-subtree for details). This PR also adds apull-exclusionsTask to easily sync new changes from that repository.How did you verify your code works?
We only test in prod here.
What are the relevant issues?