Open
Conversation
Allow routing unblocked traffic through an external HTTP or SOCKS5 proxy. This enables using Zen alongside VPNs and corporate proxies. Changes: - Add ExternalProxyConfig to config with getter/setter methods - Wire external proxy config to proxy.NewProxy() via ProxyOption - Add ExternalProxyInput settings component (protocol, host, port, auth) - Add en-US i18n keys for external proxy settings Note: requires corresponding zen-core changes (proxy.Dialer interface, WithExternalProxy option, httpConnectDialer) to function.
WalkthroughAdds external proxy configuration end-to-end: UI component, i18n entries, frontend bindings, backend config types and methods, integration into proxy startup, and small runtime and dependency updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/SettingsManager/ExternalProxyInput/index.tsx`:
- Around line 19-24: The current defaultConfig (ExternalProxyConfig) uses
host='' and port=0 and the component/handler that flips enabled (calls
SetExternalProxy) can persist these invalid defaults; add a simple validator
(e.g., isValidExternalProxyConfig or isValidHostPort) that checks host is
non-empty/non-whitespace and port is within 1-65535, and use it wherever
SetExternalProxy is invoked (the enable toggle handler and any save/submit paths
in the ExternalProxyInput component) to short-circuit saving and keep
enabled=false if validation fails; update defaultConfig usage so toggling to
enabled first requires passing the validator before calling SetExternalProxy.
- Around line 54-57: The toast currently interpolates the raw caught value `ex`
(in the catch block that calls AppToaster.show with t('externalProxy.saveError',
{ error: ex })), which can render as “[object Object]”; normalize the error to a
string first (e.g. compute const errMsg = ex instanceof Error ? ex.message :
typeof ex === 'object' ? JSON.stringify(ex) : String(ex)) or use a small helper
like getErrorMessage(ex), then call AppToaster.show with
t('externalProxy.saveError', { error: errMsg }) so the toast displays a readable
message; update the catch(ex) block around AppToaster.show accordingly.
- Around line 36-49: The useEffect async IIFE calling GetExternalProxy can throw
and leave loading true; wrap the await GetExternalProxy() call in a try/catch
(inside the same async IIFE) and in the catch set a safe default state (e.g.,
enabled:false, protocol:'socks5', host:'', port:0, username:'', password:'') or
preserve prior values, then ensure loading is set to false in a finally block
(or after both try/catch) before returning; update the code paths around
GetExternalProxy, setState, and loading to guarantee loading:false is always
applied even on failure.
In `@frontend/wailsjs/go/cfg/Config.d.ts`:
- Line 16: Update the TypeScript declarations to reflect Go pointer nullability:
change the return type of GetExternalProxy to Promise<cfg.ExternalProxyConfig |
null> and change the parameter type of SetExternalProxy to accept
cfg.ExternalProxyConfig | null so callers can pass/receive null when the backend
returns or expects a nil pointer; update the declarations for the functions
named GetExternalProxy and SetExternalProxy accordingly.
In `@internal/cfg/config.go`:
- Around line 73-74: The config struct currently persists plaintext proxy
credentials via the Username and Password fields; remove these persisted fields
and instead store only a non-sensitive reference (e.g., ProxyCredRef or
ProxyCredentialID) in the config model, and implement lookup functions (e.g.,
GetProxyCredentials or FetchProxyCredentials) that retrieve the actual
username/password from the OS keychain/credential store at runtime; update any
code that reads/writes config to stop serializing Username/Password and to use
the new reference + keychain retrieval when a proxy needs credentials.
- Around line 466-472: GetExternalProxy currently returns the internal pointer
(c.Proxy.ExternalProxy) exposing mutable state; change it to return a copy (deep
copy or value copy) of ExternalProxyConfig so callers cannot mutate internal
config, and similarly when storing an external pointer (the setter that writes
into c.Proxy.ExternalProxy) make a defensive copy into the config under the
write lock instead of assigning the caller-owned pointer. Ensure you keep the
existing c.RLock/c.RUnlock and use c.Lock/c.Unlock in the setter, and reference
the symbols GetExternalProxy and the field Proxy.ExternalProxy (and the
corresponding setter method) when implementing the copy.
- Around line 475-483: The SetExternalProxy method currently writes any
ExternalProxyConfig without validation; update Config.SetExternalProxy to
validate epc before assigning and saving: check epc.Enabled (if true) then
ensure epc.Protocol is one of "http" or "socks5", epc.Host is non-empty, and
epc.Port is within 1..65535; if validation fails return a descriptive error and
do not call c.Save(); keep the existing c.Lock()/defer c.Unlock() behavior and
only set c.Proxy.ExternalProxy and call c.Save() after validation succeeds.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
frontend/src/SettingsManager/ExternalProxyInput/index.tsxfrontend/src/SettingsManager/index.tsxfrontend/src/i18n/locales/en-US.jsonfrontend/wailsjs/go/cfg/Config.d.tsfrontend/wailsjs/go/cfg/Config.jsfrontend/wailsjs/go/models.tsfrontend/wailsjs/runtime/runtime.jsgo.modinternal/app/app.gointernal/cfg/config.go
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T12:45:14.572Z
Learnt from: anfragment
Repo: ZenPrivacy/zen-desktop PR: 434
File: extended-css/Taskfile.yml:7-11
Timestamp: 2025-09-08T12:45:14.572Z
Learning: In Wails framework development, dev mode doesn't automatically reload when go:embed files change, so developers may need to touch source files (like main.go) to trigger a rebuild during development.
Applied to files:
go.mod
🧬 Code graph analysis (5)
frontend/wailsjs/go/cfg/Config.d.ts (3)
frontend/wailsjs/go/cfg/Config.js (2)
GetExternalProxy(25-27)SetExternalProxy(109-111)frontend/wailsjs/go/models.ts (1)
ExternalProxyConfig(8-29)internal/cfg/config.go (1)
ExternalProxyConfig(68-75)
frontend/wailsjs/go/models.ts (1)
internal/cfg/config.go (1)
ExternalProxyConfig(68-75)
internal/app/app.go (3)
frontend/wailsjs/go/cfg/Config.js (2)
GetExternalProxy(25-27)GetPort(53-55)frontend/wailsjs/go/models.ts (1)
ExternalProxyConfig(8-29)internal/cfg/config.go (1)
ExternalProxyConfig(68-75)
internal/cfg/config.go (2)
frontend/wailsjs/go/models.ts (1)
ExternalProxyConfig(8-29)frontend/wailsjs/go/cfg/Config.js (2)
GetExternalProxy(25-27)SetExternalProxy(109-111)
frontend/src/SettingsManager/index.tsx (1)
frontend/src/SettingsManager/ExternalProxyInput/index.tsx (1)
ExternalProxyInput(28-161)
🔇 Additional comments (7)
frontend/wailsjs/runtime/runtime.js (1)
51-53: Good addition: runtime wrapper stays consistent.
EventsOffAll()follows the existing wrapper pattern and cleanly exposes the global event unsubscription call.go.mod (1)
12-12: Dependency updates look good.No blockers identified in the Wails/WebView2 version bumps.
Also applies to: 58-58
frontend/src/i18n/locales/en-US.json (1)
75-86: Localization block is complete and aligned with the UI fields.internal/app/app.go (1)
219-230: External proxy option wiring in startup path is clean and well-scoped.frontend/src/SettingsManager/index.tsx (1)
18-18: Integration point is appropriate (Advanced section) and consistent with existing settings structure.Also applies to: 77-77
frontend/wailsjs/go/models.ts (1)
8-29: Generated model mapping aligns with backendExternalProxyConfigfields.frontend/src/SettingsManager/ExternalProxyInput/index.tsx (1)
82-95: Good guard against live reconfiguration while proxy is active.Disabling the toggle when
isProxyRunningis true plus surfacing a tooltip is a solid UX/safety choice.
|
|
||
| export function GetCAInstalled():Promise<boolean>; | ||
|
|
||
| export function GetExternalProxy():Promise<cfg.ExternalProxyConfig>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Step 1: Verify Go method nullability (pointer return) ==="
if rg -nP 'func\s+\(c\s+\*Config\)\s+GetExternalProxy\(\)\s+\*ExternalProxyConfig' internal/cfg/config.go; then
echo "✓ Found GetExternalProxy() returning *ExternalProxyConfig"
else
echo "! Pattern not found, searching for GetExternalProxy with broader pattern:"
rg 'GetExternalProxy' internal/cfg/config.go -A 2 || echo "No matches in config.go"
fi
echo ""
echo "=== Step 2: Verify current TS declaration ==="
if [ -f frontend/wailsjs/go/cfg/Config.d.ts ]; then
echo "File exists. Checking lines 16 and 58:"
sed -n '16p;58p' frontend/wailsjs/go/cfg/Config.d.ts
echo ""
echo "Full context (lines 10-65):"
sed -n '10,65p' frontend/wailsjs/go/cfg/Config.d.ts
else
echo "! File not found: frontend/wailsjs/go/cfg/Config.d.ts"
fi
echo ""
echo "=== Step 3: Check frontend call sites ==="
if rg 'GetExternalProxy' frontend/src -t ts -B 2 -A 3; then
echo "Found usage in frontend"
else
echo "! No matches found in frontend/src"
fiRepository: ZenPrivacy/zen-desktop
Length of output: 3351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check other GetX methods returning pointers in Go ==="
rg 'func\s+\(c\s+\*Config\)\s+Get.*\(\)\s+\*' internal/cfg/config.go | head -10
echo ""
echo "=== Check SetExternalProxy backend signature ==="
rg 'func\s+\(c\s+\*Config\)\s+SetExternalProxy' internal/cfg/config.go -A 5
echo ""
echo "=== Check if other pointer returns are nullable in d.ts ==="
echo "Example: GetFilterLists returns Array, GetIgnoredHosts returns Array - check for pointer types:"
grep -nP 'export function.*Promise.*\|.*null' frontend/wailsjs/go/cfg/Config.d.ts || echo "No nullable types found in declarations"Repository: ZenPrivacy/zen-desktop
Length of output: 589
Type declarations should reflect nullable pointer semantics from Go backend.
Line 16 declares Promise<cfg.ExternalProxyConfig> (non-nullable), but GetExternalProxy() returns a Go pointer that can be nil. Similarly, line 58 declares SetExternalProxy(arg1:cfg.ExternalProxyConfig) (non-nullable), but the backend accepts a pointer that can be nil. The frontend code already compensates with optional chaining (config?.enabled ?? false), revealing the type contract gap. Update declarations to Promise<cfg.ExternalProxyConfig | null> and SetExternalProxy(arg1:cfg.ExternalProxyConfig | null) respectively, or confirm Wails nullability mapping support.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/wailsjs/go/cfg/Config.d.ts` at line 16, Update the TypeScript
declarations to reflect Go pointer nullability: change the return type of
GetExternalProxy to Promise<cfg.ExternalProxyConfig | null> and change the
parameter type of SetExternalProxy to accept cfg.ExternalProxyConfig | null so
callers can pass/receive null when the backend returns or expects a nil pointer;
update the declarations for the functions named GetExternalProxy and
SetExternalProxy accordingly.
Comment on lines
+73
to
+74
| Username string `json:"username,omitempty"` | ||
| Password string `json:"password,omitempty"` |
There was a problem hiding this comment.
Do not persist proxy credentials in plaintext config.
Lines 73-74 add credentials to the persisted config model. That weakens local credential security significantly. Prefer OS keychain/credential store and persist only a reference in config.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cfg/config.go` around lines 73 - 74, The config struct currently
persists plaintext proxy credentials via the Username and Password fields;
remove these persisted fields and instead store only a non-sensitive reference
(e.g., ProxyCredRef or ProxyCredentialID) in the config model, and implement
lookup functions (e.g., GetProxyCredentials or FetchProxyCredentials) that
retrieve the actual username/password from the OS keychain/credential store at
runtime; update any code that reads/writes config to stop serializing
Username/Password and to use the new reference + keychain retrieval when a proxy
needs credentials.
- Validate protocol/host/port in SetExternalProxy when enabled - Return/store defensive copies to prevent shared mutable state - Handle GetExternalProxy load failure with try/catch - Normalize error text in save toast to avoid [object Object] - Gate save behind host/port validation when proxy is enabled
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/cfg/config.go (2)
482-500:⚠️ Potential issue | 🟠 MajorTrim/normalize external proxy fields before validation and persistence.
host == ""allows whitespace-only host values, and protocol casing/whitespace are not normalized. This can persist unusable config and fail later at runtime.Suggested hardening
func (c *Config) SetExternalProxy(epc *ExternalProxyConfig) error { - if epc != nil && epc.Enabled { - if epc.Protocol != "http" && epc.Protocol != "socks5" { + if epc != nil { + epcCopy := *epc + epcCopy.Protocol = strings.ToLower(strings.TrimSpace(epcCopy.Protocol)) + epcCopy.Host = strings.TrimSpace(epcCopy.Host) + + if epcCopy.Enabled { + if epcCopy.Protocol != "http" && epcCopy.Protocol != "socks5" { + return fmt.Errorf("protocol must be \"http\" or \"socks5\"") + } + if epcCopy.Host == "" { + return fmt.Errorf("host must not be empty") + } + if epcCopy.Port < 1 || epcCopy.Port > 65535 { + return fmt.Errorf("port must be between 1 and 65535") + } + } + + c.Lock() + defer c.Unlock() + c.Proxy.ExternalProxy = &epcCopy + if err := c.Save(); err != nil { + log.Printf("failed to save config: %v", err) + return err + } + return nil + } - if epc.Host == "" { - return fmt.Errorf("host must not be empty") - } - if epc.Port < 1 || epc.Port > 65535 { - return fmt.Errorf("port must be between 1 and 65535") - } - } - c.Lock() defer c.Unlock() - - // Store a defensive copy to avoid shared mutable state. - if epc != nil { - copy := *epc - c.Proxy.ExternalProxy = © - } else { - c.Proxy.ExternalProxy = nil - } + c.Proxy.ExternalProxy = nil if err := c.Save(); err != nil { log.Printf("failed to save config: %v", err) return err } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cfg/config.go` around lines 482 - 500, The validation and persistence of ExternalProxy (epc) should trim/normalize fields before checking and before storing so whitespace or casing don't allow invalid configs; update the logic around epc (check and assignment near epc.Protocol, epc.Host, epc.Port and where c.Proxy.ExternalProxy = ©) to first create a trimmed/lowercased copy (e.g., trim spaces from Host and strings.ToLower for Protocol), then validate protocol against "http"/"socks5" and validate that trimmed Host is non-empty and Port in 1..65535, and finally store the normalized defensive copy in c.Proxy.ExternalProxy instead of the original object.
73-74:⚠️ Potential issue | 🟠 MajorDo not persist proxy credentials in plaintext config.
Username/Passwordare serialized intoconfig.json, which weakens local secret-at-rest security. Prefer storing credentials in OS keychain/credential manager and persisting only a reference ID in config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/cfg/config.go`:
- Around line 482-500: The validation and persistence of ExternalProxy (epc)
should trim/normalize fields before checking and before storing so whitespace or
casing don't allow invalid configs; update the logic around epc (check and
assignment near epc.Protocol, epc.Host, epc.Port and where c.Proxy.ExternalProxy
= ©) to first create a trimmed/lowercased copy (e.g., trim spaces from Host
and strings.ToLower for Protocol), then validate protocol against
"http"/"socks5" and validate that trimmed Host is non-empty and Port in
1..65535, and finally store the normalized defensive copy in
c.Proxy.ExternalProxy instead of the original object.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/SettingsManager/ExternalProxyInput/index.tsxinternal/cfg/config.go
📜 Review details
🔇 Additional comments (3)
frontend/src/SettingsManager/ExternalProxyInput/index.tsx (2)
36-53: Initial config load failure path is now safe.Good fix:
loadingis cleared in the error path, so controls don’t remain permanently disabled ifGetExternalProxy()fails.
69-83: Persistence gating for enabled configs looks correct.Nice guard here—enabled configs are only persisted when host/port are valid, which avoids saving obviously broken values.
internal/cfg/config.go (1)
466-477: Defensive copy inGetExternalProxy()is a solid fix.Returning a copy instead of internal pointer state is the right move for encapsulation and race-safety.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR adds support for routing Zen's unblocked traffic through an external HTTP or SOCKS5 proxy, enabling Zen to work alongside VPNs, corporate proxies, and tools like Tor.
Architecture:
Client → Zen (filter/block) → External Proxy → Internet
Changes:
Backend (internal/):
cfg/config.go
— Added
ExternalProxyConfig
struct with protocol, host, port, and optional username/password fields. Added
GetExternalProxy()
/
SetExternalProxy()
methods following existing getter/setter patterns.
app/app.go
—
StartProxy()
now reads the external proxy config and passes proxy.WithExternalProxy() option to proxy.NewProxy().
Frontend (frontend/src/):
New SettingsManager/ExternalProxyInput/ component — enable toggle, protocol dropdown (HTTP/SOCKS5), host, port, username, and password fields. Follows existing patterns (useState, useEffect, useDebouncedCallback, disabled when proxy is running).
Added to the Advanced section in
SettingsManager/index.tsx
.
Added externalProxy.* i18n keys to
en-US.json
.
Note: This PR requires corresponding changes to zen-core — adding a proxy.Dialer interface field,
WithExternalProxy()
functional option, and
httpConnectDialer
for HTTP CONNECT proxies. SOCKS5 support uses the existing golang.org/x/net/proxy dependency.
How did you verify your code works?
Go backend builds cleanly (go build ./...)
Frontend compiles with no TypeScript errors
App launches in dev mode and the External Proxy settings render correctly in Settings → Advanced
Unit tests for
httpConnectDialer
pass: tested HTTP CONNECT tunneling, Basic auth (rejection + success), and
WithExternalProxy()
option application
Regression: verified the app starts and functions normally with no external proxy configured
What are the relevant issues?
Closes #609