feat: add network proxy settings (HTTP/SOCKS5)#163
Merged
Conversation
Add proxy configuration in settings UI (Electron client and backend-connected browser modes). Users can enable HTTP or SOCKS5 proxy, configure host/port and optional auth credentials. Proxy applies to all external requests (GitHub API, AI providers, WebDAV) but not to backend communication. - Add ProxyConfig type and Zustand store persistence (migration v5→v6) - Add NetworkPanel UI with toggle, type selection, host/port, auth, test, save - Add Electron IPC bridge (preload.js + session.setProxy with bypass rules) - Replace fetch with axios in backend proxyService for proxy support - Add proxy config API endpoints (GET/PUT/POST test) in backend routes - All proxyRequest calls pass proxyConfig for consistent proxy behavior Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end proxy support: types, axios-based proxy client, settings API with TCP test, generated Electron main/preload IPC and persistence, renderer electron API, persisted store updates, and a NetworkPanel UI for configuring/testing proxies. ChangesNetwork Proxy Configuration
Sequence DiagramssequenceDiagram
participant Renderer
participant Preload as preload.js (contextBridge)
participant Main as main.js (ipcMain)
participant FS as userData/proxy-config.json
participant Session as webContents.session.setProxy
Renderer->>Preload: window.electronAPI.setProxy(config)
Preload->>Main: ipcRenderer.invoke('set-proxy', config)
Main->>FS: write proxy-config.json
Main->>Session: session.setProxy(proxyRules)
Session-->>Renderer: proxy rules applied
Renderer->>Preload: window.electronAPI.testProxy(config)
Preload->>Main: ipcRenderer.invoke('test-proxy', config)
Main-->>Preload: { success, error? }
Preload-->>Renderer: result
sequenceDiagram
participant Caller
participant proxyRequest
participant axios
participant UpstreamProxy
participant TargetServer
Caller->>proxyRequest: ProxyRequestOptions (url, method, proxyConfig)
proxyRequest->>axios: AxiosRequestConfig (url, method, proxy, timeout)
alt ProxyConfig enabled
axios->>UpstreamProxy: route through proxy
UpstreamProxy->>TargetServer: forward request
TargetServer->>UpstreamProxy: response
UpstreamProxy->>axios: proxied response
else Direct connection
axios->>TargetServer: direct request
TargetServer->>axios: response
end
axios->>proxyRequest: AxiosResponse
proxyRequest->>Caller: ProxyResponse (status, headers, data)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-desktop.yml:
- Around line 388-390: The saveProxyConfig function currently writes the entire
proxy config (including username/password) to PROXY_CONFIG_PATH
(proxy-config.json) in cleartext; change it to persist only non-secret fields
(host, port, protocol, bypass list, etc.) and remove username/password before
writing, and instead store credentials using a secure OS keychain (e.g., keytar)
via dedicated helpers like saveProxyCredentials/getProxyCredentials (use
PROXY_CONFIG_PATH for the config and a stable service/account naming scheme) and
update any loadProxyConfig/load logic to fetch credentials from the keychain at
runtime.
- Around line 415-433: The current ipcMain.handle('test-proxy', ...) only checks
raw TCP connectivity (socket.connect) which can return false positives; update
the handler identified by ipcMain.handle('test-proxy') to perform an actual
protocol-level probe based on config.type (e.g., send an HTTP CONNECT or a
simple HTTP GET via the proxy for HTTP/HTTPS, and perform a SOCKS5 handshake for
SOCKS proxies) and validate authentication by including credentials from config
and verifying a successful proxy response (HTTP 200/CONNECT success or SOCKS5
success reply) instead of treating socket.connect success as proof of a working
proxy.
In `@server/src/routes/proxy.ts`:
- Around line 403-428: The current router.post('/api/settings/proxy/test') only
tests raw TCP and ignores the proxy type and credentials; update this handler to
perform protocol-specific checks using the extracted type (and username/password
if provided): for type 'http' perform an HTTP CONNECT (or send a simple GET
through the proxy) to a known endpoint and validate an expected HTTP response;
for 'socks5' perform a SOCKS5 handshake (use a small socks client or implement
the minimal greeting/auth/CONNECT flow) and verify successful CONNECT response;
ensure you still fall back to a TCP check only when type is missing but return a
warning, properly handle timeouts/cleanup (socket.destroy), surface
credential/auth errors, and include the variables host, port, type, username,
password in the logic so success is only returned when the actual proxy
handshake completes.
- Around line 369-374: The PUT handler currently replaces proxy_config when the
incoming body omits the masked password (parsed.password === ''), which deletes
the stored secret and also stores new passwords unencrypted; change the update
logic in the proxy save/update flow to: when parsing row.value (parsed) and
handling parsed.password, if the incoming request provides an empty or missing
password leave the existing secret from the stored row.value (do not overwrite
proxy_config) and keep parsed.hasPassword = true; when a non-empty new password
is provided, encrypt it before persisting and store the encrypted value into
settings/proxy_config (matching other credential flows), and ensure GET still
returns password: '' and hasPassword flag. Locate symbols: parsed,
parsed.password, parsed.hasPassword, proxy_config, settings and the PUT/update
code that reads/writes row.value and apply these changes.
In `@server/src/services/proxyService.ts`:
- Around line 78-84: The current axiosConfig only sets axiosConfig.proxy when
proxyConfig?.enabled is true, leaving it unset and allowing Axios to fall back
to env HTTP_PROXY/HTTPS_PROXY; explicitly set axiosConfig.proxy = false when
proxyConfig?.enabled is falsy so the app-level proxy flag wins. Update the
construction for axiosConfig (the AxiosRequestConfig object used in
proxyService.ts) so it conditionally assigns either the proxy object when
proxyConfig?.enabled is true or assigns false when disabled, ensuring no
env-proxy fallback.
- Around line 97-103: The proxy block currently only sets
axiosConfig.proxy.protocol/host/port and never wires proxy auth or handles
SOCKS5 correctly; update it to (1) if proxyConfig.username or
proxyConfig.password exist, set axiosConfig.proxy.auth = { username:
proxyConfig.username, password: proxyConfig.password } when using axios built-in
HTTP proxy, and (2) if proxyConfig.type === 'socks5', do not set
axiosConfig.proxy.protocol = 'socks5' — instead create a SOCKS agent (e.g.
SocksProxyAgent) using proxyConfig.host/port and credentials, set
axiosConfig.httpAgent and axiosConfig.httpsAgent to that agent and set
axiosConfig.proxy = false so Axios uses the custom agent; keep the existing
host/port checks and use the symbols axiosConfig.proxy, proxyConfig.type,
proxyConfig.host, proxyConfig.port, proxyConfig.username, proxyConfig.password
to locate and modify the code in proxyService.ts.
In `@src/components/settings/NetworkPanel.tsx`:
- Around line 108-113: The switch control in NetworkPanel (the button using
role="switch" that reads form.enabled and calls setForm) and the icon-only
password-visibility button lack accessible names; add explicit accessible labels
(aria-label or aria-labelledby) to both controls, e.g., for the network switch
add aria-label like "Enable network" (and keep aria-checked tied to
form.enabled) and for the password toggle add an aria-label that updates with
state (e.g., "Show password" / "Hide password") or use aria-pressed to reflect
the current visibility state; ensure the labels reference the same state
variables (form.enabled, and your showPassword/togglePasswordVisibility state or
handler) so assistive tech receives correct live status.
- Around line 250-253: The Save button currently only checks saving and
hasChanges; update the UI and handler to block saving when proxy fields are
invalid: add a derived boolean (e.g., isProxyValid) that checks when
proxy.enabled is true then proxy.host is non-empty and proxy.port is a valid
port number (1-65535), use that boolean to extend the button's disabled
condition (in the component where the button JSX lives) and also perform the
same validation at the start of handleSave to early-return and surface
validation errors if proxy is enabled but invalid; reference the existing
handleSave function, the saving and hasChanges state/props, and the
proxy.{enabled,host,port} fields when implementing this.
- Around line 39-61: Currently the code calls setProxyConfig(form) before
syncing to remote and swallows backend errors, causing local state drift; change
the flow so you attempt remote applies first (call electronProxy.setProxy(form)
and the backend PUT to '/api/settings/proxy' using backendApiSecret) and only
call setProxyConfig(form) if remote syncs succeed, or if you must optimistically
set local state keep the original config and on any remote failure revert by
calling setProxyConfig(originalForm) and surface the error; reference
setProxyConfig, isElectron(), electronProxy.setProxy, backend.isAvailable,
backendApiSecret and the fetch('/api/settings/proxy', { method: 'PUT', ... })
call to locate where to reorder or add the rollback logic.
In `@src/store/useAppStore.ts`:
- Line 1493: The state currently persists proxyConfig (leading to stored proxy
credentials); update the persistence logic in useAppStore to exclude proxyConfig
from the persisted snapshot (keep proxyConfig in memory only) by using the
persist middleware's partialize/serialize options or by moving sensitive fields
(proxyConfig or proxyCredentials/proxyAuth) to a non-persisted part of the
store; ensure the key names referenced are proxyConfig and useAppStore so you
remove/omit proxyConfig from the persisted payload while leaving the rest of the
store unchanged.
- Around line 531-537: The current rehydration for proxyConfig only checks for a
boolean enabled and can let corrupted fields through; update the IIFE that
computes proxyConfig to validate the full shape from safePersisted: check that
(safePersisted as Record<string, unknown>).proxyConfig is an object and that its
properties match ProxyConfig (enabled is boolean, type is one of the allowed
literal values e.g. 'http' | 'socks', host is a string, and port is a finite
number); only return the casted import('../types').ProxyConfig when all checks
pass, otherwise return the default { enabled: false, type: 'http', host: '',
port: 7890 } to avoid passing invalid values downstream.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b1bc7ef-b55d-45cc-b3af-c0771b950010
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.github/workflows/build-desktop.ymlserver/package.jsonserver/src/routes/proxy.tsserver/src/services/proxyService.tssrc/components/settings/GeneralPanel.tsxsrc/components/settings/NetworkPanel.tsxsrc/services/electronProxy.tssrc/store/useAppStore.tssrc/types/index.ts
…idation - Fix workflow YAML syntax: replace template literal with string concatenation for preload.js generation (backtick broke YAML parsing) - Fix proxyService: use socks-proxy-agent for SOCKS5, set proxy.auth for HTTP proxy credentials, set proxy=false when disabled to prevent env fallback - Fix NetworkPanel: add aria-label on toggle switch and password button, add form validation (host required, port 1-65535), remote-first save order - Fix store: validate full ProxyConfig shape in normalizePersistedState - Fix proxy.ts PUT: preserve existing password when request omits it - Add socks-proxy-agent dependency for SOCKS5 support Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-desktop.yml (1)
391-405:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Electron proxy auth (username/password are ignored)
In
.github/workflows/build-desktop.yml(applyProxy),session.setProxy()is configured only withtype/host/portviaproxyRules, andconfig.username/config.passwordare never used. The repo also has no Electronsession/webContentsloginhandler to respond to proxy authentication challenges, so proxy-authenticated requests from the renderer won’t succeed. Additionally, the backend proxy implementation supports HTTP proxy auth, but SOCKS5 auth credentials are not wired (server/src/services/proxyService.tsusessocks5://host:portonly).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-desktop.yml around lines 391 - 405, applyProxy currently builds proxyRules without using config.username/config.password and there is no login handler to respond to proxy auth challenges; update applyProxy to include credentials in the proxy URL when present (e.g. socks5://user:pass@host:port or http://user:pass@host:port) before calling mainWindow.webContents.session.setProxy, and register a session 'login' event handler (mainWindow.webContents.session or app.on as appropriate) to supply config.username/config.password for proxy auth challenges; also ensure server/src/services/proxyService.ts accepts and forwards SOCKS5 credentials (use socks5://user:pass@host:port when wiring proxyService) so credentials are consistently applied end-to-end.
♻️ Duplicate comments (4)
server/src/services/proxyService.ts (1)
98-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass SOCKS5 credentials into the agent.
Line 101 builds the SOCKS URL from
host/portonly, soproxyConfig.username/passwordare ignored in the SOCKS5 path. Authenticated SOCKS5 proxies will still fail even though the sharedProxyConfigcontract supports credentials.Suggested fix
if (proxyConfig.type === 'socks5') { // SOCKS5: axios 不原生支持，使用 socks-proxy-agent const { SocksProxyAgent } = await import('socks-proxy-agent'); - const socksUrl = `socks5://${proxyConfig.host}:${proxyConfig.port}`; + const auth = + proxyConfig.username || proxyConfig.password + ? `${encodeURIComponent(proxyConfig.username ?? '')}:${encodeURIComponent(proxyConfig.password ?? '')}@` + : ''; + const socksUrl = `socks5://${auth}${proxyConfig.host}:${proxyConfig.port}`; const agent = new SocksProxyAgent(socksUrl); axiosConfig.httpAgent = agent; axiosConfig.httpsAgent = agent;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/services/proxyService.ts` around lines 98 - 105, The SOCKS5 branch ignores proxy credentials; update the logic in proxyService (where proxyConfig is handled and SocksProxyAgent is instantiated) to pass username/password to the agent instead of only host:port. Build the socks URL including credentials when present (e.g., socks5://username:password@host:port) or pass an options object with user/pass to the SocksProxyAgent constructor, and ensure axiosConfig.httpAgent/httpsAgent are set and axiosConfig.proxy remains false so authenticated SOCKS5 proxies work.server/src/routes/proxy.ts (1)
399-407:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not persist the proxy password in plaintext.
This path still writes
configToStore.passwordverbatim intosettings, unlike the other credential flows in this file that store encrypted values and decrypt on read. That leaves the proxy secret readable at rest.Suggested direction
-import { decrypt } from '../services/crypto.js'; +import { decrypt, encrypt } from '../services/crypto.js'; ... - if (password) { - configToStore.password = password; + if (password) { + configToStore.password = encrypt(password, config.encryptionKey); } else if (storedPassword) { configToStore.password = storedPassword; }Also decrypt
parsed.passwordinsidegetProxyConfig()before returning it toproxyRequest().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/routes/proxy.ts` around lines 399 - 407, The code currently writes configToStore.password in plaintext to the settings DB; instead, follow the existing credential flow used elsewhere in this file: when setting configToStore, encrypt the password before assigning it (use the same encryption helper used for other credentials in this file) and store the encrypted value in the INSERT/REPLACE into settings; additionally, in getProxyConfig() decrypt parsed.password before returning it so proxyRequest() receives the plaintext secret (and ensure you only decrypt when a password exists). Reference configToStore, the db.prepare('INSERT OR REPLACE INTO settings (key, value) VALUES (?, ?)') call, getProxyConfig(), parsed.password, and proxyRequest() when making the changes.src/store/useAppStore.ts (1)
531-545:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersisting
proxyConfigwholesale leaks secrets and doesn't rehydrate them anyway.
partializestoresstate.proxyConfigverbatim, butnormalizePersistedStaterebuilds onlyenabled/type/host/port. That means any savedusername/passwordstill land in IndexedDB, then disappear from the live store after reload. Persist only the non-secret proxy fields, or keep credentials in a non-persisted slice.Also applies to: 1419-1503
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/useAppStore.ts` around lines 531 - 545, The persisted proxyConfig currently stores the whole state.proxyConfig (including credentials) but normalizePersistedState only rehydrates enabled/type/host/port, causing secrets to be written to IndexedDB then discarded; update the persistence layer (the partialize function that writes state.proxyConfig) to only persist the non-secret fields {enabled, type, host, port} and/or exclude username/password entirely, and ensure proxyConfig reconstruction in normalizePersistedState and any code using safePersisted consistently expects only those four fields (references: proxyConfig, state.proxyConfig, partialize, normalizePersistedState, safePersisted)..github/workflows/build-desktop.yml (1)
388-390:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't write proxy credentials to
userDatain cleartext.
saveProxyConfigserializes the full renderer-supplied object intoproxy-config.json, which includes proxy usernames/passwords when auth is enabled. Persist only non-secret fields here and move credentials to OS-backed secure storage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-desktop.yml around lines 388 - 390, The saveProxyConfig function is currently serializing the full renderer-supplied object (including credentials) to PROXY_CONFIG_PATH; change it to persist only non-secret fields (e.g., host, port, protocol, bypass list, exclude patterns) by filtering out username/password/auth tokens before JSON.stringify, and store any credentials separately using the OS-backed secure storage API used by the app (e.g., keytar/Keychain/Windows Credential Manager) under a stable key tied to the saved config; update the code paths that read proxy settings to reconstruct the full config by reading the non-secret JSON from saveProxyConfig and fetching credentials from secure storage via the existing secret store APIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/settings/NetworkPanel.tsx`:
- Around line 41-67: Before starting a new save attempt clear any previous test
result to avoid stale error banners: call setTestResult(null) (or the
component's empty-state value) immediately when initiating the save (e.g., right
before setSaving(true) or at the top of the try block) so that functions like
isElectron(), electronProxy.setProxy(form), backend.isAvailable branch, the
fetch('/api/settings/proxy') call, and the subsequent setProxyConfig(form) won't
leave a prior failure visible; keep existing error handling that calls
setTestResult(...) on catch unchanged.
- Around line 43-62: The code updates Electron first then the backend, causing
inconsistent state if the backend PUT (fetch('/api/settings/proxy')) fails;
change the flow in NetworkPanel.tsx to persist to the backend before calling
electronProxy.setProxy (use backend.isAvailable and backendApiSecret as
currently used), and only call electronProxy.setProxy(form) after the backend
responds ok; alternatively, if you prefer to keep the current order, call
electronProxy.getProxy (or otherwise capture the previous Electron proxy state)
before setting it and, if the backend save throws, revert by calling
electronProxy.setProxy(previousState) to ensure atomicity.
---
Outside diff comments:
In @.github/workflows/build-desktop.yml:
- Around line 391-405: applyProxy currently builds proxyRules without using
config.username/config.password and there is no login handler to respond to
proxy auth challenges; update applyProxy to include credentials in the proxy URL
when present (e.g. socks5://user:pass@host:port or http://user:pass@host:port)
before calling mainWindow.webContents.session.setProxy, and register a session
'login' event handler (mainWindow.webContents.session or app.on as appropriate)
to supply config.username/config.password for proxy auth challenges; also ensure
server/src/services/proxyService.ts accepts and forwards SOCKS5 credentials (use
socks5://user:pass@host:port when wiring proxyService) so credentials are
consistently applied end-to-end.
---
Duplicate comments:
In @.github/workflows/build-desktop.yml:
- Around line 388-390: The saveProxyConfig function is currently serializing the
full renderer-supplied object (including credentials) to PROXY_CONFIG_PATH;
change it to persist only non-secret fields (e.g., host, port, protocol, bypass
list, exclude patterns) by filtering out username/password/auth tokens before
JSON.stringify, and store any credentials separately using the OS-backed secure
storage API used by the app (e.g., keytar/Keychain/Windows Credential Manager)
under a stable key tied to the saved config; update the code paths that read
proxy settings to reconstruct the full config by reading the non-secret JSON
from saveProxyConfig and fetching credentials from secure storage via the
existing secret store APIs.
In `@server/src/routes/proxy.ts`:
- Around line 399-407: The code currently writes configToStore.password in
plaintext to the settings DB; instead, follow the existing credential flow used
elsewhere in this file: when setting configToStore, encrypt the password before
assigning it (use the same encryption helper used for other credentials in this
file) and store the encrypted value in the INSERT/REPLACE into settings;
additionally, in getProxyConfig() decrypt parsed.password before returning it so
proxyRequest() receives the plaintext secret (and ensure you only decrypt when a
password exists). Reference configToStore, the db.prepare('INSERT OR REPLACE
INTO settings (key, value) VALUES (?, ?)') call, getProxyConfig(),
parsed.password, and proxyRequest() when making the changes.
In `@server/src/services/proxyService.ts`:
- Around line 98-105: The SOCKS5 branch ignores proxy credentials; update the
logic in proxyService (where proxyConfig is handled and SocksProxyAgent is
instantiated) to pass username/password to the agent instead of only host:port.
Build the socks URL including credentials when present (e.g.,
socks5://username:password@host:port) or pass an options object with user/pass
to the SocksProxyAgent constructor, and ensure axiosConfig.httpAgent/httpsAgent
are set and axiosConfig.proxy remains false so authenticated SOCKS5 proxies
work.
In `@src/store/useAppStore.ts`:
- Around line 531-545: The persisted proxyConfig currently stores the whole
state.proxyConfig (including credentials) but normalizePersistedState only
rehydrates enabled/type/host/port, causing secrets to be written to IndexedDB
then discarded; update the persistence layer (the partialize function that
writes state.proxyConfig) to only persist the non-secret fields {enabled, type,
host, port} and/or exclude username/password entirely, and ensure proxyConfig
reconstruction in normalizePersistedState and any code using safePersisted
consistently expects only those four fields (references: proxyConfig,
state.proxyConfig, partialize, normalizePersistedState, safePersisted).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c48097b-7b5d-4aa4-b677-024dec48aa70
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/build-desktop.ymlserver/package.jsonserver/src/routes/proxy.tsserver/src/services/proxyService.tssrc/components/settings/NetworkPanel.tsxsrc/store/useAppStore.ts
✅ Files skipped from review due to trivial changes (1)
- server/package.json
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ges" This reverts commit 8b0d30e.
…omicity - Exclude proxy password from Zustand persistence (IndexedDB) for security - Clear stale testResult on new save attempt - Rollback Electron proxy state if backend sync fails (atomic save) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closed
Store proxy password as password_encrypted using the same encrypt/decrypt flow as GitHub tokens, AI API keys, and WebDAV passwords. GET handler masks the encrypted value and exposes only hasPassword flag. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/src/routes/proxy.ts (1)
425-448:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
/api/settings/proxy/teststill reports false positives.This handler still only checks whether something is listening on
host:port; it never validates an HTTP proxy response or a SOCKS5 handshake, and it ignores credentials entirely. A wrong protocol, bad auth, or even a non-proxy daemon can still return{ success: true }, so the UI’s “Test connection” result is not trustworthy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/routes/proxy.ts` around lines 425 - 448, The current test handler (the block reading req.body and creating a raw net.Socket with socket.connect) only checks that something is listening on host:port and must be replaced with protocol-aware validation: inspect req.body.type (e.g., "http" or "socks5") and for HTTP proxies perform an actual HTTP proxy check (open TCP to host:port, send a CONNECT request to a known external host or send a simple GET via the proxy and validate a proper HTTP/1.1 proxy response and proxy auth if credentials present), and for SOCKS5 perform the SOCKS5 handshake (method negotiation, USERNAME/PASSWORD if credentials supplied, then attempt a CONNECT command and verify the reply code). Keep using a timed TCP socket but validate protocol responses and return success only on correct proxy responses; ensure errors and auth failures are included in the returned error string instead of treating any open port as success.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/routes/proxy.ts`:
- Around line 377-382: The API currently treats any falsy password the same, so
add an explicit clear-vs-omit distinction: keep the GET behavior
(parsed.hasPassword true if parsed.password_encrypted) but do not treat an empty
string as "keep"; in the PUT handler (the update path around lines referenced)
check req.body.hasOwnProperty('password') — if true and password === '' then
clear the stored ciphertext (set parsed.password_encrypted/existingEncrypted to
null/undefined), if true and password is non-empty encrypt and replace, and if
password is omitted (no ownProperty) preserve the existingEncrypted; update the
code that currently deletes parsed.password_encrypted and parsed.password to
follow this logic so only omission preserves ciphertext and an explicit empty
string clears it.
- Around line 15-18: The current catch swallowing failures from
decrypt(parsed.password_encrypted, config.encryptionKey) sets parsed.password =
'' which silently strips auth; instead treat a decryption error as an invalid
proxy config: in the catch block for decrypt(...) log the error (including the
exception) and propagate failure by marking the proxy config invalid or throwing
an error so the request is rejected (e.g., return a 4xx and disable/skip proxy
usage). Update the catch around decrypt to stop normal processing when
decryption fails rather than assigning an empty parsed.password.
---
Duplicate comments:
In `@server/src/routes/proxy.ts`:
- Around line 425-448: The current test handler (the block reading req.body and
creating a raw net.Socket with socket.connect) only checks that something is
listening on host:port and must be replaced with protocol-aware validation:
inspect req.body.type (e.g., "http" or "socks5") and for HTTP proxies perform an
actual HTTP proxy check (open TCP to host:port, send a CONNECT request to a
known external host or send a simple GET via the proxy and validate a proper
HTTP/1.1 proxy response and proxy auth if credentials present), and for SOCKS5
perform the SOCKS5 handshake (method negotiation, USERNAME/PASSWORD if
credentials supplied, then attempt a CONNECT command and verify the reply code).
Keep using a timed TCP socket but validate protocol responses and return success
only on correct proxy responses; ensure errors and auth failures are included in
the returned error string instead of treating any open port as success.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e03003d1-ee85-4380-8210-fdafdcd360e6
📒 Files selected for processing (1)
server/src/routes/proxy.ts
The 'direct://' string was missing an escaped closing quote, causing the generated main.js to have a syntax error. Changed from \\'direct://' to \\'direct://\\' to properly escape both quotes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-desktop.yml (1)
391-406:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProxy authentication credentials are ignored.
The
applyProxyfunction builds the proxy URL fromhostandportonly, ignoringusernameandpassword. Electron'ssession.setProxysupports credentials in the URL (e.g.,http://user:pass@host:port), but they are never included here. Authenticated proxies will fail silently.🔧 Proposed fix to include credentials
'async function applyProxy(config) {\\n' + ' if (!mainWindow || mainWindow.isDestroyed()) return;\\n' + ' if (config.enabled && config.host && config.port) {\\n' + -' const proxyUrl = config.type === \\'socks5\\'\\n' + -' ? \\'socks5://\\' + config.host + \\':\\' + config.port\\n' + -' : \\'http://\\' + config.host + \\':\\' + config.port;\\n' + +' let auth = \\'\\';\\n' + +' if (config.username) {\\n' + +' auth = config.password\\n' + +' ? encodeURIComponent(config.username) + \\':\\' + encodeURIComponent(config.password) + \\'@\\'\\n' + +' : encodeURIComponent(config.username) + \\'@\\';\\n' + +' }\\n' + +' const proxyUrl = config.type === \\'socks5\\'\\n' + +' ? \\'socks5://\\' + auth + config.host + \\':\\' + config.port\\n' + +' : \\'http://\\' + auth + config.host + \\':\\' + config.port;\\n' +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-desktop.yml around lines 391 - 406, The applyProxy function currently builds proxyUrl from only host and port and ignores username/password, causing failures with authenticated proxies; update applyProxy to include credentials when present by URL-encoding config.username and config.password and prepending "user:pass@" (omit if username empty) to the host portion for both HTTP and SOCKS5 cases, then pass that proxyUrl to mainWindow.webContents.session.setProxy (keep the existing proxyBypassRules and direct fallback behavior); reference the applyProxy function and mainWindow.webContents.session.setProxy to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/build-desktop.yml:
- Around line 391-406: The applyProxy function currently builds proxyUrl from
only host and port and ignores username/password, causing failures with
authenticated proxies; update applyProxy to include credentials when present by
URL-encoding config.username and config.password and prepending "user:pass@"
(omit if username empty) to the host portion for both HTTP and SOCKS5 cases,
then pass that proxyUrl to mainWindow.webContents.session.setProxy (keep the
existing proxyBypassRules and direct fallback behavior); reference the
applyProxy function and mainWindow.webContents.session.setProxy to locate and
modify the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be859158-7458-4592-b000-ef40d6865efe
📒 Files selected for processing (1)
.github/workflows/build-desktop.yml
- getProxyConfig: remove try/catch around decrypt, let decrypt failure propagate so proxy config is treated as invalid rather than silently stripping auth - PUT handler: distinguish between omitted password (preserve existing), empty password (clear stored secret), and new password (encrypt & store) using 'password' in req.body check Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The applyProxy function was ignoring username/password when building the proxy URL. Authenticated proxies would fail silently. Now includes URL-encoded credentials in the proxy URL when username is provided. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace TCP-only port check with actual protocol handshake: - SOCKS5: send greeting (v5), validate server response, support username/password auth negotiation - HTTP: send CONNECT request, validate 200 response, detect 407 auth required Both Electron IPC handler and backend endpoint upgraded. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move NetworkPanel from GeneralPanel to a new "Network" tab in the settings sidebar. The tab is only visible when running in Electron or when a backend is connected (same visibility logic as before). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Summary
session.setProxywith bypass rules for localhostfetchwithaxiosin backend proxyService for native proxy supportChanges
ProxyConfiginterface andProxyTypetosrc/types/index.tssetProxyConfigaction, persistence, and migration v5→v6 inuseAppStore.tsNetworkPanel.tsxwith toggle, type radio, host/port inputs, collapsible auth, test button, save buttonpreload.jswith IPC bridge; main process handlesset-proxy/get-proxy/test-proxyIPC calls; appliessession.setProxywithproxyBypassRules: '<local>;localhost;127.0.0.1'proxyService.tsusesaxioswithproxyconfig option;proxy.tsroutes passproxyConfigto allproxyRequestcalls; new/api/settings/proxyendpointsisElectron() || backend.isAvailable(hidden in pure browser mode)Backward Compatibility
enabled: false— existing users unaffectedproxyConfigfield with safe defaultssession.setProxycallproxyConfigparameter is optional, defaults to no proxyaxiosdependency only used in proxy path, existing behavior unchangedTest plan
npx tsc --noEmitpasses for both frontend and server🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Desktop