feat: custom CA certificate support for enterprise proxy SSL#1917
feat: custom CA certificate support for enterprise proxy SSL#1917hiboute wants to merge 14 commits intoAndyMik90:developfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
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 support for a custom CA certificate: settings/UI to pick a CA file, IPC and preload wiring for file selection, threads the CA path into profile service calls and Anthropic fetch, and propagates Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as GeneralSettings
participant Preload as Preload/API
participant Main as MainProcess
participant Service as ProfileService
participant SDK as AnthropicSDK
participant Agent as AgentProcess
User->>UI: Click "Browse" / set CA path
UI->>Preload: selectFile(filters?)
Preload->>Main: IPC DIALOG_SELECT_FILE
Main->>Main: openFile dialog -> selected path
Main-->>Preload: selected file path
Preload-->>UI: selected file path
UI->>Main: request testConnection/discoverModels
Main->>Main: readSettingsFile() -> customCACertPath
Main->>Service: testConnection(..., caCertPath)
Service->>Service: buildCustomFetch(caCertPath) -> https.Agent with CA
Service->>SDK: init client / call with custom fetch
SDK->>SDK: TLS handshake using provided CA
Agent->>Agent: read settings -> set NODE_EXTRA_CA_CERTS in env
Agent->>Agent: spawn subprocess with augmented env
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's compatibility with enterprise network environments by introducing comprehensive support for custom CA certificates. It addresses the challenge of SSL connections through corporate proxies and self-hosted services, ensuring that both API profile connection tests and agent subprocesses can successfully establish secure communications using user-provided CA certificates. The changes include UI additions, environment variable injection, and a custom Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Code Review
This pull request adds support for custom CA certificates, a valuable feature for users behind corporate proxies. However, the current implementation allows the renderer process to specify arbitrary file paths for the CA certificate without validation, introducing risks of path traversal, Denial of Service, and Man-in-the-Middle attacks. Additionally, the code could benefit from improved maintainability, reduced duplication, and enhanced robustness and error handling, particularly concerning file reading and the custom fetch implementation, to prevent silent failures.
- Extract buildCustomFetch() helper to eliminate duplicated cert-reading logic in testConnection and discoverModels - Extract getCustomCaCertPath() helper in profile-handlers to remove duplicated readSettingsFile calls - Fix body handling in createFetchWithCA: handle string, Uint8Array, and URLSearchParams correctly instead of blindly calling String() Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
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 `@apps/frontend/src/main/agent/agent-process.ts`:
- Around line 232-237: The code forwards appSettingsForCert.customCACertPath
directly into certEnv['NODE_EXTRA_CA_CERTS'], which can pass relative or
non-existent paths to subprocesses; update the logic that reads customCACertPath
(after readSettingsFile) to normalize and validate the path before exporting:
use path.resolve to convert relative paths to an absolute path, verify the file
exists and is a regular file (e.g., with fs.existsSync / fs.statSync), and only
set certEnv['NODE_EXTRA_CA_CERTS'] when validation succeeds; if validation
fails, omit the env var and emit a clear log/warn message so subprocesses don’t
receive an invalid path (apply the same fix to the other occurrence that sets
NODE_EXTRA_CA_CERTS around the referenced lines).
In `@apps/frontend/src/main/ipc-handlers/profile-handlers.ts`:
- Around line 218-220: The settings lookup for custom CA cert is duplicated —
extract a small helper like getCustomCACertPath that calls readSettingsFile(),
casts to Partial<AppSettings> and returns the customCACertPath (or undefined);
replace the repeated logic in both ipc handlers in profile-handlers.ts (where
caCertPath is read at the top and again around line 308) with calls to
getCustomCACertPath() to centralize behavior and avoid drift.
In `@apps/frontend/src/main/services/profile/profile-service.ts`:
- Around line 460-467: The code currently swallows errors when reading a custom
CA (the fs.readFileSync in the block that sets customFetch via
createFetchWithCA), which hides misconfiguration; change the catch to rethrow or
return a rejected Error that includes the certificate path and original error
(e.g., throw new Error(`Failed to read CA at ${caCertPath}: ${err.message}`)) so
callers of the profile service can surface the certificate-path error instead of
continuing without the CA; apply the same fix to the other identical block
around the createFetchWithCA usage (the occurrence referenced at lines ~638-645
/ ~642) so both locations propagate a clear, descriptive error containing the
path and original error details.
- Around line 26-77: createFetchWithCA currently misparses Request inputs,
forces HTTPS only, and ignores abort signals; fix it by (1) deriving the URL
string from Request.url when input is a Request object (use Request.url instead
of toString) and merge signals/body from the Request and init (prefer explicit
Request properties), (2) choose the HTTP client based on url.protocol (use http
for "http:" and https for "https:") and set the default port to Number(url.port)
|| (url.protocol === 'http:' ? 80 : 443) when building the request options, and
(3) honor AbortSignal from the Request or init by rejecting immediately if
already aborted and by wiring a signal listener that calls req.abort() (or
req.destroy()) and rejects the promise with an AbortError/DOMException, and
remove the listener on completion; update references in the function
createFetchWithCA, the local variables urlStr/url, init?.signal, and the
https.request call to use the selected client and proper port.
In `@apps/frontend/src/renderer/components/settings/GeneralSettings.tsx`:
- Around line 373-375: Replace the hardcoded label 'Certificates' passed to
window.electronAPI.selectFile in GeneralSettings.tsx with a translated key via
react-i18next (use the t(...) from useTranslation) e.g.
t('settings.certificates'), ensuring you import/use useTranslation in that
component; then add the corresponding "settings.certificates" entry to both
locale JSON files (en/*.json and fr/*.json) with proper translations so the
file-filter label is localized while keeping the extensions array unchanged.
In `@apps/frontend/src/shared/i18n/locales/fr/settings.json`:
- Line 247: Update the French translation value for the key
"customCACertPathDescription" to correct the typo: replace "proxys d'entreprise"
with "proxies d'entreprise" so the string reads "Chemin vers un fichier de
certificat CA (.pem/.crt) pour les connexions SSL (ex: Zscaler, proxies
d'entreprise)"; ensure only the wording is changed and JSON syntax remains
valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d613d5c7-5309-4b10-af6a-8f5c65ea060e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (14)
apps/backend/core/auth.pyapps/frontend/src/main/agent/agent-process.tsapps/frontend/src/main/ipc-handlers/profile-handlers.test.tsapps/frontend/src/main/ipc-handlers/profile-handlers.tsapps/frontend/src/main/ipc-handlers/settings-handlers.tsapps/frontend/src/main/services/profile/profile-service.tsapps/frontend/src/preload/api/project-api.tsapps/frontend/src/renderer/components/settings/GeneralSettings.tsxapps/frontend/src/renderer/lib/mocks/project-mock.tsapps/frontend/src/shared/constants/ipc.tsapps/frontend/src/shared/i18n/locales/en/settings.jsonapps/frontend/src/shared/i18n/locales/fr/settings.jsonapps/frontend/src/shared/types/ipc.tsapps/frontend/src/shared/types/settings.ts
apps/frontend/src/renderer/components/settings/GeneralSettings.tsx
Outdated
Show resolved
Hide resolved
- createFetchWithCA: use Request.url instead of toString(), support HTTP URLs alongside HTTPS, wire AbortSignal to req.destroy() for proper cancellation - buildCustomFetch: throw with a clear message on cert read failure instead of silently falling back; both testConnection and discoverModels now surface cert errors with a descriptive message - agent-process: resolve cert path to absolute and check existsSync before setting NODE_EXTRA_CA_CERTS, warn if file not found - GeneralSettings: move hardcoded 'Certificates' file-filter label to i18n key (en + fr) - fr/settings.json: fix typo proxys → proxies Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/frontend/src/main/services/profile/profile-service.ts (2)
93-101:⚠️ Potential issue | 🟠 MajorDo not silently ignore custom CA read failures.
Swallowing file-read errors here hides misconfiguration and produces misleading downstream “network” failures. If
caCertPathis set and unreadable, this should fail fast with a clear certificate-path error.🔧 Proposed fix
function buildCustomFetch(caCertPath?: string): typeof globalThis.fetch | undefined { if (!caCertPath) return undefined; try { const ca = fs.readFileSync(caCertPath); return createFetchWithCA(ca); - } catch { - // If the cert file can't be read (missing/unreadable), fall back to default TLS - return undefined; + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + throw new Error(`Failed to read custom CA certificate at "${caCertPath}": ${reason}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/main/services/profile/profile-service.ts` around lines 93 - 101, When buildCustomFetch is given a caCertPath but fs.readFileSync fails, do not swallow the error; instead surface a clear failure: catch the error in buildCustomFetch and throw a new Error (or rethrow) that includes the caCertPath and the original error message so callers see a clear "certificate path unreadable" error; keep the existing behavior of returning undefined only when caCertPath is not provided, and use createFetchWithCA after successful read.
9-11:⚠️ Potential issue | 🔴 CriticalFix custom fetch semantics for
Requestinput, protocol handling, and cancellation.Line [31] uses
Request.toString(), Lines [48-52] forcehttps.requestwith default443, and Lines [73-85] never wire an abort listener. This can break SDK behavior and cancellation, and it regresseshttp://endpoints when custom CA is enabled.#!/bin/bash set -euo pipefail python - <<'PY' from pathlib import Path p = Path("apps/frontend/src/main/services/profile/profile-service.ts") t = p.read_text() start = t.find("function createFetchWithCA") end = t.find("function buildCustomFetch") segment = t[start:end] checks = { "uses Request.toString()": ".toString()" in segment, "forces https.request only": "https.request" in segment and "http.request" not in segment, "hardcodes default port 443": "Number(url.port) || 443" in segment, "has AbortSignal listener wiring": ("addEventListener('abort'" in segment) or ('addEventListener(\"abort\"' in segment), } for k, v in checks.items(): print(f"{k}: {v}") PY🔧 Proposed fix
+import http from 'node:http'; import https from 'node:https'; @@ function createFetchWithCA(ca: Buffer): typeof globalThis.fetch { const agent = new https.Agent({ ca }); return async (input, init): Promise<Response> => { - const urlStr = typeof input === 'string' ? input : (input as URL | Request).toString(); + const requestInput = input instanceof Request ? input : undefined; + const urlStr = + typeof input === 'string' + ? input + : input instanceof URL + ? input.toString() + : input.url; const url = new URL(urlStr); - const method = init?.method ?? 'GET'; + const method = init?.method ?? requestInput?.method ?? 'GET'; @@ - const initHeaders = init?.headers; + const initHeaders = init?.headers ?? requestInput?.headers; @@ + const signal = init?.signal ?? requestInput?.signal; return new Promise<Response>((resolve, reject) => { - const req = https.request( + const requestFn = url.protocol === 'http:' ? http.request : https.request; + const req = requestFn( { hostname: url.hostname, - port: Number(url.port) || 443, + port: Number(url.port) || (url.protocol === 'http:' ? 80 : 443), path: url.pathname + url.search, method, headers: rawHeaders, - agent, + agent: url.protocol === 'https:' ? agent : undefined, }, @@ ); req.on('error', reject); + if (signal?.aborted) { + reject(new DOMException('The operation was aborted.', 'AbortError')); + req.destroy(); + return; + } + const onAbort = () => req.destroy(new DOMException('The operation was aborted.', 'AbortError')); + signal?.addEventListener('abort', onAbort, { once: true }); + req.on('close', () => signal?.removeEventListener('abort', onAbort));Also applies to: 31-56, 73-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/main/services/profile/profile-service.ts` around lines 9 - 11, The custom fetch helpers createFetchWithCA/buildCustomFetch mishandle Request inputs, force https and port 443, and never wire up AbortSignal; update createFetchWithCA/buildCustomFetch to accept a Request or URL/string properly (use request instanceof Request ? request.url : input or access request.url instead of Request.toString()), choose the node client by inspecting the parsed URL protocol and use http.request when protocol === 'http:' and https.request when protocol === 'https:', compute port with Number(url.port) || (url.protocol === 'https:' ? 443 : 80) (not hardcode 443), and wire the AbortSignal by listening to signal.addEventListener('abort') (or signal.onabort) to call req.destroy()/req.abort and reject the fetch with an AbortError so cancellations propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/frontend/src/main/services/profile/profile-service.ts`:
- Around line 93-101: When buildCustomFetch is given a caCertPath but
fs.readFileSync fails, do not swallow the error; instead surface a clear
failure: catch the error in buildCustomFetch and throw a new Error (or rethrow)
that includes the caCertPath and the original error message so callers see a
clear "certificate path unreadable" error; keep the existing behavior of
returning undefined only when caCertPath is not provided, and use
createFetchWithCA after successful read.
- Around line 9-11: The custom fetch helpers createFetchWithCA/buildCustomFetch
mishandle Request inputs, force https and port 443, and never wire up
AbortSignal; update createFetchWithCA/buildCustomFetch to accept a Request or
URL/string properly (use request instanceof Request ? request.url : input or
access request.url instead of Request.toString()), choose the node client by
inspecting the parsed URL protocol and use http.request when protocol ===
'http:' and https.request when protocol === 'https:', compute port with
Number(url.port) || (url.protocol === 'https:' ? 443 : 80) (not hardcode 443),
and wire the AbortSignal by listening to signal.addEventListener('abort') (or
signal.onabort) to call req.destroy()/req.abort and reject the fetch with an
AbortError so cancellations propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c6df2a5-b0d6-4a43-854d-2819cf6cdfe0
📒 Files selected for processing (2)
apps/frontend/src/main/ipc-handlers/profile-handlers.tsapps/frontend/src/main/services/profile/profile-service.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend/src/main/agent/agent-process.ts`:
- Around line 234-236: readSettingsFile() returns untyped JSON and the code
calls customCACertPath.trim() without ensuring it's a string; this can throw if
settings were corrupted. Fix by guarding the type before trimming: check that
appSettingsForCert?.customCACertPath is a string (e.g., typeof === 'string') and
only then call .trim() and assign to configuredCertPath; otherwise treat as
undefined/null. Update the logic around readSettingsFile, AppSettings,
customCACertPath and configuredCertPath to handle non-string values safely and
ensure TypeScript strict type checks pass.
In `@apps/frontend/src/renderer/components/settings/GeneralSettings.tsx`:
- Around line 372-379: The onClick handler for the browse button currently
awaits window.electronAPI.selectFile and can yield an unhandled rejection; wrap
the await call in a try/catch so failures don’t propagate and settings remain
unchanged. Specifically, in the anonymous async onClick function (the handler
that calls window.electronAPI.selectFile with the PEM/CRT/CER filter), catch any
thrown error and simply return or optionally log it, and only call
onSettingsChange({ ...settings, customCACertPath: result }) when result is
valid; ensure settings and customCACertPath are untouched on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 098e7ead-e49d-4915-8314-d74d3d4903b0
📒 Files selected for processing (5)
apps/frontend/src/main/agent/agent-process.tsapps/frontend/src/main/services/profile/profile-service.tsapps/frontend/src/renderer/components/settings/GeneralSettings.tsxapps/frontend/src/shared/i18n/locales/en/settings.jsonapps/frontend/src/shared/i18n/locales/fr/settings.json
Our change to agent-process.ts added a readSettingsFile() call for the
custom CA cert path. readSettingsFile() calls app.getPath('userData')
which was not mocked in agent-process.test.ts, causing 24 test failures.
Added vi.mock('../settings-utils') matching the pattern used in
profile-handlers.test.ts.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
- agent-process.ts: add typeof check before .trim() to avoid throwing if customCACertPath is a non-string value in corrupted settings - GeneralSettings.tsx: wrap selectFile IPC call in try/catch to prevent unhandled promise rejection if file dialog fails Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend/src/main/agent/agent-process.ts`:
- Around line 238-244: Resolve the configuredCertPath deterministically using
Electron packaging context instead of process.cwd(): when app.isPackaged is
true, resolve relative paths against process.resourcesPath (otherwise use
process.cwd() or the existing behavior), then verify the resolved path exists
and is a regular file (use fs.statSync(...).isFile()) before setting
certEnv['NODE_EXTRA_CA_CERTS']; update the resolution logic around variables
configuredCertPath and resolvedCertPath and replace the existsSync check with a
file-type check so only a real file is injected.
In `@apps/frontend/src/renderer/components/settings/GeneralSettings.tsx`:
- Line 5: Replace the relative import of Button in GeneralSettings.tsx with the
frontend path alias: change the import of Button (currently from '../ui/button')
to use the configured alias (e.g. '@components/ui/button') so it follows the
apps/frontend tsconfig convention; update the import statement in
GeneralSettings.tsx to reference '@components/ui/button' and ensure any named
export matches the Button symbol used in the file.
- Around line 372-379: The click handler captures a stale settings snapshot and
can overwrite concurrent changes; update the prop contract and usage: change
GeneralSettingsProps to type onSettingsChange: (settings: AppSettings | ((prev:
AppSettings) => AppSettings)) => void, then call onSettingsChange with the
functional updater (onSettingsChange(prev => ({ ...prev, customCACertPath:
result }))) inside the async onClick handler, and ensure the parent (where
setSettings is passed from, e.g., AppSettings.tsx) accepts and forwards updater
functions (or wraps setSettings to handle both object and function forms) so
TypeScript and runtime behavior remain correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e410c7e6-c52c-41de-983f-23dcfe4e7ca7
📒 Files selected for processing (2)
apps/frontend/src/main/agent/agent-process.tsapps/frontend/src/renderer/components/settings/GeneralSettings.tsx
Relative paths are unreliable in packaged Electron apps because process.cwd() varies between dev and production runtimes. Also add statSync().isFile() check to avoid injecting a directory or symlink target as NODE_EXTRA_CA_CERTS. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…t path statSync() can throw EACCES if the process lacks read permissions on the file, which would crash agent process initialization. Wrap the existsSync/statSync block in try-catch so permission errors are logged as warnings and gracefully skipped instead of propagating. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Summary
.pem/.crtfile for SSL connections through corporate proxies (e.g. Zscaler, self-hosted LiteLLM instances with custom TLS)NODE_EXTRA_CA_CERTSfrom that setting into agent subprocesses (Claude CLI, backend agent) so builds work end-to-end behind a custom CAhttps.Agent(via a customfetchpassed to the Anthropic SDK) when a CA cert path is configured, so the connection test no longer fails with "Network error" against custom-CA endpointsDIALOG_SELECT_FILEIPC channelRoot cause of the connection test failure
The Anthropic SDK runs in the Electron main process and uses Node's default TLS trust store. Setting
NODE_EXTRA_CA_CERTSat runtime viaprocess.envhas no effect (Node reads it from the environment at startup). The fix injects a customfetchfunction backed byhttps.Agent({ ca })into the SDK client, which routes all requests through an agent that trusts the custom CA.Tested
Auto-Claude-2.7.6-darwin-arm64.dmg)Test plan
.pemCA cert path in Settings > Paths > Custom CA CertificateSummary by CodeRabbit
New Features
Tests
Chores