fix: system proxy resolver logic updates
#6273
Conversation
lohit-bruno
requested review from
bijin-bruno,
helloanoop and
naman-bruno
as code owners
WalkthroughAdds a cross-platform async system proxy resolver (with caching, timeouts, and env fallback), exposes getSystemProxy(), wires an IPC handler, adds Redux thunk and read-only SystemProxy UI, updates Electron/CLI proxy flows to use the resolver, and introduces platform detectors with tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as React UI
participant Redux as Redux Store
participant Renderer as Renderer (ipcRenderer)
participant Main as Electron Main
participant Detector as System Proxy Detector
UI->>Redux: dispatch getSystemProxyVariables()
Redux->>Renderer: ipcRenderer.invoke('renderer:get-system-proxy-variables')
Renderer->>Main: IPC invoke
Main->>Detector: getSystemProxy()
Detector->>Detector: detectByPlatform() or env fallback (with timeout/cache)
Detector-->>Main: ProxyConfiguration
Main-->>Renderer: return ProxyConfiguration
Renderer-->>Redux: resolve promise
Redux->>Redux: updateSystemProxyVariables(config)
Redux-->>UI: state updated
UI->>UI: render SystemProxy component (read-only)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bruno-cli/src/runner/run-single-request.js (2)
306-313: DuplicategetSystemProxy()call detected.
getSystemProxy()is called here at line 306 and again at line 353. This function performs OS-level detection (spawningscutilon macOS, reading registry on Windows, etc.), which is unnecessarily expensive to run twice.Store the result and reuse it:
+ let systemProxy = null; + if (noproxy) { // If noproxy flag is set, don't use any proxy proxyMode = 'off'; } else if (collectionProxyEnabled === true) { // If collection proxy is enabled, use it proxyConfig = collectionProxyConfig; proxyMode = 'on'; } else if (collectionProxyEnabled === 'global') { // If collection proxy is set to 'global', use system proxy - const systemProxy = await getSystemProxy(); - const { http_proxy, https_proxy } = systemProxy; + systemProxy = await getSystemProxy(); + const { http_proxy, https_proxy } = systemProxy; if (http_proxy?.length || https_proxy?.length) { proxyMode = 'system'; }Then at line 353, reuse
systemProxyinstead of callinggetSystemProxy()again.
351-389: Refactor to reuse system proxy result and improve error handling.The current implementation has two issues:
- Duplicate call to
getSystemProxy()(see previous comment)- Inner throws at lines 365 and 378 are caught by the outer catch at line 385, which silently falls back to no proxy. This hides configuration errors from the user.
} else if (proxyMode === 'system') { - try { - const systemProxy = await getSystemProxy(); - const { http_proxy, https_proxy, no_proxy } = systemProxy; + const { http_proxy, https_proxy, no_proxy } = systemProxy || {}; const shouldUseSystemProxy = shouldUseProxy(request.url, no_proxy || ''); const parsedUrl = parseUrl(request.url); const isHttpsRequest = parsedUrl.protocol === 'https:'; + if (shouldUseSystemProxy) { - try { - if (http_proxy?.length && !isHttpsRequest) { + if (http_proxy?.length && !isHttpsRequest) { + try { new URL(http_proxy); request.httpAgent = new HttpProxyAgent(http_proxy); + } catch (error) { + console.log(chalk.yellow('Warning: Invalid system http_proxy URL, skipping proxy')); } - } catch (error) { - throw new Error('Invalid system http_proxy'); } - try { - if (https_proxy?.length && isHttpsRequest) { + if (https_proxy?.length && isHttpsRequest) { + try { new URL(https_proxy); - request.httpsAgent = new PatchedHttpsProxyAgent(https_proxy, + request.httpsAgent = new PatchedHttpsProxyAgent( + https_proxy, Object.keys(httpsAgentRequestFields).length > 0 ? { ...httpsAgentRequestFields } : undefined); - } else { - request.httpsAgent = new https.Agent({ - ...httpsAgentRequestFields - }); + } catch (error) { + console.log(chalk.yellow('Warning: Invalid system https_proxy URL, using direct connection')); + request.httpsAgent = new https.Agent({ ...httpsAgentRequestFields }); } - } catch (error) { - throw new Error('Invalid system https_proxy'); + } else if (isHttpsRequest) { + request.httpsAgent = new https.Agent({ ...httpsAgentRequestFields }); } } else { - request.httpsAgent = new https.Agent({ - ...httpsAgentRequestFields - }); + request.httpsAgent = new https.Agent({ ...httpsAgentRequestFields }); } - } catch (error) { - request.httpsAgent = new https.Agent({ - ...httpsAgentRequestFields - }); - } } else if (Object.keys(httpsAgentRequestFields).length > 0) {
🧹 Nitpick comments (10)
packages/bruno-app/src/components/Preferences/ProxySettings/index.js (1)
161-165: Minor: Consider removing redundant wrapper div.The SystemProxy component already has its own wrapper with styling. The outer div at line 162 adds
mb-3 pt-1 text-muted system-proxy-settingsbut the inner component also usessystem-proxy-settings. This may cause style conflicts or redundancy.{formik?.values?.mode === 'system' ? ( - <div className="mb-3 pt-1 text-muted system-proxy-settings"> - <SystemProxy /> - </div> + <SystemProxy /> ) : null}packages/bruno-requests/src/network/system-proxy/utils/windows.ts (2)
118-160: Significant code duplication betweengetSystemProxyEnvironmentandgetUserEnvironmentProxy.Both methods parse registry output identically. Consider extracting a shared helper:
+ private parseEnvironmentProxyFromRegistry(stdout: string): ProxyConfiguration | null { + const lines = stdout.split('\n'); + let http_proxy: string | null = null; + let https_proxy: string | null = null; + let no_proxy: string | null = null; + + for (const line of lines) { + const trimmedLine = line.trim(); + if (trimmedLine.includes('HTTP_PROXY') && trimmedLine.includes('REG_SZ')) { + const match = trimmedLine.match(/HTTP_PROXY\s+REG_SZ\s+(.+)/); + if (match) http_proxy = normalizeProxyUrl(match[1].trim()); + } + if (trimmedLine.includes('HTTPS_PROXY') && trimmedLine.includes('REG_SZ')) { + const match = trimmedLine.match(/HTTPS_PROXY\s+REG_SZ\s+(.+)/); + if (match) https_proxy = normalizeProxyUrl(match[1].trim(), 'https'); + } + if (trimmedLine.includes('NO_PROXY') && trimmedLine.includes('REG_SZ')) { + const match = trimmedLine.match(/NO_PROXY\s+REG_SZ\s+(.+)/); + if (match) no_proxy = normalizeNoProxy(match[1].trim()); + } + } + + if (http_proxy || https_proxy) { + return { http_proxy, https_proxy, no_proxy, source: 'windows-system' }; + } + return null; + }Also applies to: 162-204
34-44: Silent error swallowing may hide configuration issues.Each detection method catches errors and continues, but the outer catch (lines 47-49) will also rethrow after all methods fail. Consider logging individual method failures for debugging:
} catch (error) { // Continue to next method if this one fails + // console.debug(`Detection method failed: ${error instanceof Error ? error.message : String(error)}`); continue; }This is optional; just flagging for visibility during troubleshooting.
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js (3)
36-39: Nested ternary is hard to read.Consider using an object lookup:
+const sourceLabels = { + 'environment': 'Environment Variables', + 'windows-system': 'Windows System Settings', + 'macos-system': 'macOS System Settings', + 'linux-system': 'Linux System Settings' +}; + <div> - {source === 'environment' ? 'Environment Variables' - : source === 'windows-system' ? 'Windows System Settings' - : source === 'macos-system' ? 'macOS System Settings' - : source === 'linux-system' ? 'Linux System Settings' : source} + {sourceLabels[source] || source} </div>
10-12: Consider adding loading/error feedback.The component dispatches
getSystemProxyVariablesbut doesn't track loading state. Users may see stale or empty values briefly. If the IPC call fails, there's no error indication.
14-69: Unnecessary fragment wrapper.The
<></>wrapper is not needed since the component returns a single<div>element.return ( - <> <div className="mb-3 text-muted system-proxy-settings space-y-4"> ... </div> - </> );packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts (1)
161-168: Fragile mock setup depends on implicit call order.The combination of
mockReturnValue(false)followed by twomockReturnValueOnce(true)relies on knowing exact internal call order ofexistsSync. If the implementation changes call order, this test breaks silently.Consider being explicit:
- // Mock all previous methods to fail - mockExistsSync.mockReturnValue(false); - - // Mock systemd conf directory to exist - mockExistsSync.mockReturnValueOnce(true); - - // Mock systemd proxy file to exist - mockExistsSync.mockReturnValueOnce(true); + mockExistsSync.mockImplementation((path: string) => { + if (path === '/etc/environment') return false; + if (path === '/etc/systemd/system.conf.d') return true; + if (path.includes('proxy.conf')) return true; + return false; + });packages/bruno-app/src/providers/ReduxStore/slices/app.js (1)
201-210: Minor promise chaining issue.The
.then((variables) => { dispatch(...) }).then(resolve)chain resolves withundefinedsince the dispatch callback returns nothing. This works but is slightly confusing.export const getSystemProxyVariables = () => (dispatch, getState) => { return new Promise((resolve, reject) => { const { ipcRenderer } = window; ipcRenderer.invoke('renderer:get-system-proxy-variables') .then((variables) => { dispatch(updateSystemProxyVariables(variables)); + resolve(variables); - }) - .then(resolve).catch(reject); + }).catch(reject); }); };packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
175-243: Consider simplifying the parsing loop.The nested loops with multiple regex patterns per variable could be simplified. However, the current implementation is correct and readable enough.
One minor improvement: you could build a single combined regex pattern upfront instead of iterating per variable, but this is optional given the small, fixed list.
packages/bruno-requests/src/network/system-proxy/index.ts (1)
65-67:detectWithTimeoutis a pass-through wrapper.This method just calls
detectByPlatform— thecommandTimeoutis passed to individual detectors. The method name is slightly misleading since no additional timeout wrapper exists here.Consider either:
- Removing this wrapper and calling
detectByPlatformdirectly, or- Adding an actual
Promise.racetimeout if you want a hard cap beyond individual command timeouts
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js(1 hunks)packages/bruno-app/src/components/Preferences/ProxySettings/index.js(2 hunks)packages/bruno-app/src/components/ResponsePane/index.js(1 hunks)packages/bruno-app/src/providers/App/useIpcEvents.js(1 hunks)packages/bruno-app/src/providers/ReduxStore/slices/app.js(4 hunks)packages/bruno-cli/src/runner/run-single-request.js(4 hunks)packages/bruno-cli/src/utils/proxy-util.js(1 hunks)packages/bruno-electron/src/ipc/network/cert-utils.js(2 hunks)packages/bruno-electron/src/ipc/preferences.js(2 hunks)packages/bruno-electron/src/store/preferences.js(2 hunks)packages/bruno-electron/src/utils/proxy-util.js(5 hunks)packages/bruno-requests/src/index.ts(1 hunks)packages/bruno-requests/src/network/index.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/index.spec.js(1 hunks)packages/bruno-requests/src/network/system-proxy/index.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/types.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/common.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/linux.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/macos.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/windows.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-requests/src/network/system-proxy/types.tspackages/bruno-app/src/components/ResponsePane/index.jspackages/bruno-electron/src/ipc/network/cert-utils.jspackages/bruno-requests/src/network/system-proxy/utils/macos.spec.tspackages/bruno-requests/src/network/system-proxy/utils/common.tspackages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-requests/src/index.tspackages/bruno-requests/src/network/index.tspackages/bruno-electron/src/ipc/preferences.jspackages/bruno-requests/src/network/system-proxy/utils/linux.tspackages/bruno-requests/src/network/system-proxy/utils/windows.tspackages/bruno-requests/src/network/system-proxy/index.spec.jspackages/bruno-requests/src/network/system-proxy/utils/macos.tspackages/bruno-requests/src/network/system-proxy/utils/linux.spec.tspackages/bruno-requests/src/network/system-proxy/index.tspackages/bruno-requests/src/network/system-proxy/utils/windows.spec.tspackages/bruno-cli/src/utils/proxy-util.jspackages/bruno-electron/src/utils/proxy-util.jspackages/bruno-app/src/components/Preferences/ProxySettings/index.jspackages/bruno-requests/src/network/system-proxy/utils/common.spec.tspackages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-electron/src/store/preferences.jspackages/bruno-app/src/providers/ReduxStore/slices/app.js
🧬 Code graph analysis (13)
packages/bruno-app/src/components/ResponsePane/index.js (6)
packages/bruno-app/src/components/FolderSettings/index.js (1)
getTabClassname(69-73)packages/bruno-app/src/components/CollectionSettings/index.js (1)
getTabClassname(83-87)packages/bruno-app/src/components/RequestPane/HttpRequestPane/index.js (2)
getTabClassname(83-87)selectTab(27-34)packages/bruno-app/src/components/ResponsePane/StatusCode/index.js (1)
getTabClassname(8-16)packages/bruno-app/src/components/RunnerResults/ResponsePane/index.js (2)
getTabClassname(83-87)selectTab(36-36)packages/bruno-app/src/components/RequestPane/GraphQLRequestPane/index.js (2)
getTabClassname(123-127)selectTab(54-61)
packages/bruno-electron/src/ipc/network/cert-utils.js (5)
packages/bruno-requests/src/index.ts (1)
getSystemProxy(10-10)packages/bruno-requests/src/network/index.ts (1)
getSystemProxy(3-3)packages/bruno-requests/src/network/system-proxy/index.ts (2)
getSystemProxy(22-44)getSystemProxy(106-120)packages/bruno-electron/src/utils/proxy-util.js (1)
proxyConfig(379-379)packages/bruno-app/src/components/CollectionSettings/index.js (1)
proxyConfig(42-42)
packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts (1)
packages/bruno-requests/src/network/system-proxy/utils/macos.ts (1)
MacOSProxyDetector(8-122)
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js (1)
packages/bruno-app/src/providers/ReduxStore/slices/app.js (2)
getSystemProxyVariables(201-210)getSystemProxyVariables(201-210)
packages/bruno-requests/src/network/system-proxy/utils/linux.ts (2)
packages/bruno-requests/src/network/system-proxy/types.ts (2)
ProxyDetector(8-10)ProxyConfiguration(1-6)packages/bruno-requests/src/network/system-proxy/utils/common.ts (2)
normalizeProxyUrl(13-23)normalizeNoProxy(30-40)
packages/bruno-requests/src/network/system-proxy/utils/windows.ts (3)
packages/bruno-requests/src/network/system-proxy/types.ts (2)
ProxyDetector(8-10)ProxyConfiguration(1-6)packages/bruno-requests/src/network/system-proxy/index.ts (1)
ProxyConfiguration(122-122)packages/bruno-requests/src/network/system-proxy/utils/common.ts (2)
normalizeProxyUrl(13-23)normalizeNoProxy(30-40)
packages/bruno-requests/src/network/system-proxy/index.spec.js (4)
packages/bruno-requests/src/network/system-proxy/index.ts (3)
SystemProxyDetector(8-102)getSystemProxy(22-44)getSystemProxy(106-120)packages/bruno-requests/src/network/system-proxy/utils/windows.ts (1)
WindowsProxyDetector(16-235)packages/bruno-requests/src/network/system-proxy/utils/macos.ts (1)
MacOSProxyDetector(8-122)packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
LinuxProxyDetector(24-244)
packages/bruno-requests/src/network/system-proxy/utils/macos.ts (2)
packages/bruno-requests/src/network/system-proxy/types.ts (2)
ProxyDetector(8-10)ProxyConfiguration(1-6)packages/bruno-requests/src/network/system-proxy/utils/common.ts (2)
normalizeProxyUrl(13-23)normalizeNoProxy(30-40)
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts (1)
packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
LinuxProxyDetector(24-244)
packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts (1)
packages/bruno-requests/src/network/system-proxy/utils/windows.ts (1)
WindowsProxyDetector(16-235)
packages/bruno-electron/src/utils/proxy-util.js (2)
packages/bruno-cli/src/runner/run-single-request.js (4)
proxyUri(324-324)proxyConfig(292-292)parsedUrl(356-356)parseUrl(3-3)packages/bruno-cli/src/utils/proxy-util.js (3)
shouldUseProxy(16-63)parsedUrl(26-26)parseUrl(1-1)
packages/bruno-app/src/components/Preferences/ProxySettings/index.js (1)
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js (1)
SystemProxy(5-70)
packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts (1)
packages/bruno-requests/src/network/system-proxy/utils/common.ts (2)
normalizeProxyUrl(13-23)normalizeNoProxy(30-40)
🪛 ast-grep (0.40.0)
packages/bruno-requests/src/network/system-proxy/utils/linux.ts
[warning] 208-208: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^export\\s+${varName}\\s*=\\s*(.+)$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 209-209: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${varName}\\s*=\\s*(.+)$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.1.2)
packages/bruno-electron/src/utils/proxy-util.js
[error] 88-103: This constructor has code paths that return without calling super().
If this is intentional, add an explicit throw statement in unsupported paths.
(lint/correctness/noUnreachableSuper)
⏰ 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). (6)
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Windows
🔇 Additional comments (35)
packages/bruno-requests/src/network/system-proxy/types.ts (1)
1-15: System proxy type definitions look consistent and usable.
ProxyConfiguration,ProxyDetector, andSystemProxyDetectorOptionsalign well with the new detection flow and env-style proxy fields.packages/bruno-electron/src/utils/proxy-util.js (2)
337-340: System proxy handling and HTTP proxy agent options look coherent.
httpProxyAgentOptionswithkeepAlive: trueand the newTimelineHttpProxyAgentwiring for both manual (proxyMode === 'on') and system proxies integrate cleanly with the existing timeline agent pattern.- In system mode, splitting behavior by protocol (
http_proxyfor non‑HTTPS,https_proxyfor HTTPS) and validating vianew URL(...)before constructing agents gives clear, actionable error messages when system proxy values are malformed.- Fallback to a non‑proxy
https.Agentwhen system proxies are disabled byno_proxyor missing also looks reasonable.Also applies to: 365-368, 379-407
86-105: Derived agent constructor should always callsuper(and fail fast ifproxyis missing).The subclass returned by
createTimelineHttpAgentClassonly callssuperwhenoptions?.proxyis set andBaseAgentClass.name === 'HttpProxyAgent'. Other code paths fall through without callingsuper, which violates class constructor requirements and can lead to runtime errors if the factory is used differently in the future.Given that a proxy URI is always required here, restructure to:
- Unconditionally destructure
proxyand options- Throw if
proxyis missing- Always call
super(proxyUri, httpProxyAgentOptions)before accessingthis- Remove the fragile
BaseAgentClass.namecheck-function createTimelineHttpAgentClass(BaseAgentClass) { - return class extends BaseAgentClass { - constructor(options, timeline) { - // For proxy agents, the first argument is the proxy URI and the second is options - if (options?.proxy) { - if (BaseAgentClass.name === 'HttpProxyAgent') { - const { proxy: proxyUri, httpProxyAgentOptions } = options; - super(proxyUri, httpProxyAgentOptions); - this.timeline = Array.isArray(timeline) ? timeline : []; - // Log the proxy details - this.timeline.push({ - timestamp: new Date(), - type: 'info', - message: `Using proxy: ${proxyUri}` - }); - } - } - } - }; -} +function createTimelineHttpAgentClass(BaseAgentClass) { + return class extends BaseAgentClass { + constructor(options, timeline) { + const { proxy: proxyUri, httpProxyAgentOptions } = options || {}; + + if (!proxyUri) { + throw new Error('TimelineHttpProxyAgent requires a "proxy" option'); + } + + // For proxy agents, the first argument is the proxy URI and the second is options + super(proxyUri, httpProxyAgentOptions); + + this.timeline = Array.isArray(timeline) ? timeline : []; + this.timeline.push({ + timestamp: new Date(), + type: 'info', + message: `Using proxy: ${proxyUri}` + }); + } + }; +}Ensure all call sites pass
options.proxyand test against your Node/Electron versions.packages/bruno-app/src/components/ResponsePane/index.js (1)
160-165: Addingdata-testidon the Headers tab is a clean, non-invasive test hook.Nice way to make the Headers tab easy to target in tests without affecting runtime behavior.
packages/bruno-requests/src/index.ts (1)
8-10: Re-exportinggetSystemProxyfrom the package entrypoint looks correct.This cleanly surfaces the new system proxy API alongside existing exports without breaking the current public surface.
packages/bruno-electron/src/ipc/network/cert-utils.js (1)
4-4: Good integration of sharedgetSystemProxyinto Electron’s proxy resolution.For collections using the global proxy config, resolving
proxyModeto'system'and then delegating toawait getSystemProxy()centralizes system proxy detection in the requests layer and keeps this IPC helper focused on wiring.Also applies to: 105-122
packages/bruno-requests/src/network/index.ts (1)
1-3: Network module export ofgetSystemProxyis consistent with the new API surface.This keeps the network entrypoint aligned with the package entrypoint and downstream Electron usage.
packages/bruno-electron/src/store/preferences.js (1)
30-30: Defaulting global proxy mode to'system'is consistent and backward-tolerant.Changing the default mode to
'system'in bothdefaultPreferencesandgetGlobalProxyConfigaligns behavior for new installs while still allowing existing saved preferences to override it through the merge logic.Also applies to: 195-196
packages/bruno-cli/src/utils/proxy-util.js (1)
82-85: Verify no remaining imports ofgetSystemProxyEnvVariablesexist in the codebase.The module now exports only
shouldUseProxyandPatchedHttpsProxyAgent. Ensure thatgetSystemProxyEnvVariablesis not imported or used anywhere else in the project before merging.packages/bruno-app/src/components/Preferences/ProxySettings/index.js (1)
11-11: LGTM on the import.Clean import of the new SystemProxy component. The explicit
/indexsuffix is consistent with the codebase style.packages/bruno-app/src/providers/App/useIpcEvents.js (1)
2-6: LGTM on the import cleanup.Removal of
updateSystemProxyEnvVariablesis consistent with the architectural shift to on-demand IPC-based proxy detection. The new flow correctly fetches system proxy data via thegetSystemProxyVariablesthunk when the SystemProxy component mounts.packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts (2)
1-25: Good test coverage for normalizeProxyUrl.Tests cover the key scenarios: missing protocol, existing protocols, empty strings, SOCKS variants, and URLs without ports. The assertions align correctly with the implementation in
common.ts.
27-64: Comprehensive tests for normalizeNoProxy.Excellent coverage of delimiter variations, whitespace handling, and edge cases. The complex patterns test at line 61-63 properly validates CIDR, wildcards, and
<local>patterns.packages/bruno-electron/src/ipc/preferences.js (2)
6-6: LGTM on the import.Clean import of
getSystemProxyfrom the@usebruno/requestspackage.
51-55: Consider adding error handling for robustness.The handler works but doesn't catch errors from
getSystemProxy(). If the system proxy detection fails (e.g.,scutilcommand fails on macOS), the rejection propagates to the renderer. Ensure the Redux thunk handles this gracefully.packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts (3)
1-22: LGTM on the test setup.Clean mock setup for
child_process.execandutil.promisify. ThebeforeEachcorrectly resets mocks between tests.
24-128: Solid coverage for proxy detection scenarios.Tests properly validate HTTP/HTTPS combinations, default port fallbacks, and enabled/disabled states. Assertions match the expected
ProxyConfigurationshape.
235-247: Good error handling coverage.Tests correctly verify that command failures and invalid output formats throw appropriate errors.
packages/bruno-cli/src/runner/run-single-request.js (2)
3-3: LGTM on the import.Using Node's built-in
url.parsefor protocol detection.
19-20: LGTM on the updated imports.Clean imports for the new proxy utilities and
getSystemProxyfrom@usebruno/requests.packages/bruno-requests/src/network/system-proxy/utils/windows.ts (1)
8-14: Good use of an enum to restrict commands.This approach mitigates command injection by whitelisting allowed shell commands. Clean pattern.
packages/bruno-requests/src/network/system-proxy/utils/common.ts (1)
1-40: Clean utility functions with good documentation.Both helpers are well-documented, handle edge cases appropriately, and follow consistent patterns. The regex for protocol detection is correct and the no_proxy normalization handles multiple delimiter formats.
packages/bruno-requests/src/network/system-proxy/index.spec.js (1)
37-78: Good test coverage across detection scenarios.Tests cover environment variable priority, platform routing, error fallbacks, caching behavior, and the public
getSystemProxyfunction. Solid integration test suite.Also applies to: 80-116, 118-154, 156-195, 197-210
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts (2)
40-126: Good coverage for gsettings scenarios.Tests cover manual proxy, identical HTTP/HTTPS proxies, non-manual mode handling, and empty ignore lists. Well-structured mocking of sequential gsettings calls.
186-223: Error handling tests are comprehensive.Covers gsettings unavailable, missing schema, and no configuration found scenarios. All properly assert the expected error message.
packages/bruno-app/src/providers/ReduxStore/slices/app.js (1)
46-47: LGTM on the renamed state field.Consistent rename from
systemProxyEnvVariablestosystemProxyVariables— makes sense given it now captures system-level proxy config, not just environment variables.packages/bruno-requests/src/network/system-proxy/utils/macos.ts (1)
88-121: LGTM on proxy configuration builder.Good handling of HTTP/HTTPS proxies with appropriate default ports and proper normalization. The
<local>append forExcludeSimpleHostnamesis a nice touch.packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts (3)
1-22: LGTM on test setup.Clean mocking of
node:child_processandnode:util. ThebeforeEachproperly resets mocks between tests.
24-84: Solid coverage for Internet Options detection.Good test cases for single proxy, protocol-specific, and fallback scenarios. The mock chaining for fallback behavior is well structured.
231-377: Thorough edge case coverage.Tests for existing prefixes, empty overrides, disabled proxies, decimal values, and complex bypass lists provide good confidence in parsing robustness.
packages/bruno-requests/src/network/system-proxy/utils/linux.ts (3)
10-22: Good use of enum for command allowlisting.Prevents command injection by constraining shell commands to known-safe values.
59-92: LGTM on GNOME gsettings detection.Properly checks for manual mode, parses host/port pairs, and handles the gsettings array format for ignore-hosts.
206-223: Static analysis warning is a false positive.The
varNamecomes from the hardcodedproxyVarsarray (line 176), not user input. No ReDoS risk here.packages/bruno-requests/src/network/system-proxy/index.ts (2)
8-44: Well-designed caching and concurrency control.The
activeDetectionpromise coalescing prevents redundant concurrent detections. Cache TTL of 60s is reasonable for proxy settings that rarely change.
106-120: LGTM on the exportedgetSystemProxyfunction.Environment variables taking precedence over system detection is a sensible default — allows users to override system proxy easily.
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts
Outdated
Show resolved
Hide resolved
lohit-bruno
force-pushed
the
system_proxy_updates
branch
from
5f22e87 to
73ef5b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/bruno-requests/src/network/system-proxy/index.ts (1)
97-101: Race condition: clearing activeDetection during in-flight detection.Line 100 sets
activeDetection = null, which can cause duplicate detections ifclearCache()is called while a detection is in progress. The nextgetSystemProxy()call (line 29) won't find the active detection and will start a new one.Choose one approach:
Option 1 (recommended): Preserve in-flight detection
clearCache(): void { this.cachedResult = null; this.cacheTimestamp = 0; - this.activeDetection = null; + // Keep activeDetection to avoid duplicate in-flight requests }Option 2: Implement force-refresh semantics
-clearCache(): void { +async clearCache(): Promise<void> { + // Wait for any in-flight detection to complete before clearing + if (this.activeDetection) { + await this.activeDetection.catch(() => {}); + } this.cachedResult = null; this.cacheTimestamp = 0; this.activeDetection = null; }
🧹 Nitpick comments (4)
packages/bruno-electron/src/ipc/network/cert-utils.js (1)
116-119: Use strict equality.Line 116 uses loose equality (
==) instead of strict equality (===).Apply this diff:
- if (proxyMode == 'system') { + if (proxyMode === 'system') {packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
206-223: Static analysis flag is a false positive, but pattern pre-compilation would improve performance.The static analyzer warns about RegExp construction from variables (lines 208-209), but
varNamecomes from the hardcodedproxyVarsarray (line 176), not user input, so there's no ReDoS risk.For better performance, consider pre-compiling the patterns:
+ // Pre-compiled patterns for known proxy variables + private readonly PROXY_PATTERNS = [ + 'http_proxy', 'https_proxy', 'no_proxy', 'all_proxy', + 'HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY', 'ALL_PROXY' + ].flatMap((varName) => [ + { regex: new RegExp(`^export\\s+${varName}\\s*=\\s*(.+)$`, 'i'), varName }, + { regex: new RegExp(`^${varName}\\s*=\\s*(.+)$`, 'i'), varName } + ]); + private parseProxyFromContent(content: string): ProxyConfiguration | null { - const proxyVars = ['http_proxy', 'https_proxy', 'no_proxy', 'all_proxy', 'HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY', 'ALL_PROXY']; const proxies: Record<string, string> = {}; const lines = content.split('\n'); for (const line of lines) { const trimmedLine = line.trim(); // Skip comments and empty lines if (!trimmedLine || trimmedLine.startsWith('#')) { continue; } // ... systemd handling ... - // Handle different formats: VAR=value, export VAR=value, VAR="value", etc. - for (const varName of proxyVars) { - const patterns = [ - new RegExp(`^export\\s+${varName}\\s*=\\s*(.+)$`, 'i'), - new RegExp(`^${varName}\\s*=\\s*(.+)$`, 'i') - ]; - - for (const pattern of patterns) { - const match = trimmedLine.match(pattern); - if (match) { - let value = match[1].trim(); - // Remove surrounding quotes - value = value.replace(/^["']|["']$/g, ''); - proxies[varName.toLowerCase()] = value; - break; - } + for (const { regex, varName } of this.PROXY_PATTERNS) { + const match = trimmedLine.match(regex); + if (match) { + let value = match[1].trim(); + value = value.replace(/^["']|["']$/g, ''); + proxies[varName.toLowerCase()] = value; + break; } } }packages/bruno-electron/src/utils/proxy-util.js (1)
379-407: System proxy selection & validation behaviour is clearer; watch expectations around env formatsThe updated
proxyMode === 'system'logic:
- Destructures
{ http_proxy, https_proxy, no_proxy }fromproxyConfig || {}and usesshouldUseProxy(requestConfig.url, no_proxy || '')to respectno_proxy.- Differentiates HTTP vs HTTPS requests via
parseUrl(requestConfig.url)andparsedUrl.protocol === 'https:', usinghttp_proxyonly for non‑HTTPS andhttps_proxyonly for HTTPS.- Validates each configured system proxy with
new URL(...)and wraps failures in explicit error messages:Invalid system http_proxy "…"/Invalid system https_proxy "…", which is a nice debugging improvement.- Reuses the same timeline‑enabled agents as the manual proxy mode, so HTTP/HTTPS system proxies now benefit from the same logging.
Two things to keep in mind:
- This behaviour assumes
http_proxy/https_proxyare full URLs acceptable tonew URL(...)(including a scheme). If your system‑proxy detection can emit barehost:portvalues, those will now be rejected; confirm that upstream always normalizes to proper URLs.- Because the HTTP/system proxy agents push
Using proxy: ${proxyUri}into the timeline, any credentials embedded inhttp_proxy/https_proxywill end up in diagnostic output. If that’s a concern for your users, consider redacting the userinfo segment before logging.Functionally this change looks sound; the main follow‑up is just to verify the expected shape of the system proxy values fed into
proxyConfig.packages/bruno-cli/src/runner/run-single-request.js (1)
3-3: Prefernew URL()over legacyurl.parsefor protocol detectionYou import
parseUrljust to readparsedUrl.protocol, while elsewhere in this file you already use the globalURLconstructor. For consistency and to avoid the legacyurlAPI, you can drop the import and do:-const parseUrl = require('url').parse;and later:
- const parsedUrl = parseUrl(request.url); - const isHttpsRequest = parsedUrl.protocol === 'https:'; + const parsedUrl = new URL(request.url); + const isHttpsRequest = parsedUrl.protocol === 'https:';This keeps everything on the WHATWG URL path and removes an extra dependency.
Also applies to: 356-357
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js(1 hunks)packages/bruno-app/src/components/Preferences/ProxySettings/index.js(2 hunks)packages/bruno-app/src/components/ResponsePane/index.js(1 hunks)packages/bruno-app/src/providers/App/useIpcEvents.js(1 hunks)packages/bruno-app/src/providers/ReduxStore/slices/app.js(4 hunks)packages/bruno-cli/src/runner/run-single-request.js(4 hunks)packages/bruno-cli/src/utils/proxy-util.js(1 hunks)packages/bruno-electron/src/ipc/network/cert-utils.js(2 hunks)packages/bruno-electron/src/ipc/preferences.js(2 hunks)packages/bruno-electron/src/store/preferences.js(2 hunks)packages/bruno-electron/src/utils/proxy-util.js(5 hunks)packages/bruno-requests/src/index.ts(1 hunks)packages/bruno-requests/src/network/index.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/index.spec.js(1 hunks)packages/bruno-requests/src/network/system-proxy/index.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/types.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/common.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/linux.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/macos.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/windows.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/bruno-app/src/components/ResponsePane/index.js
- packages/bruno-requests/src/network/system-proxy/index.spec.js
- packages/bruno-app/src/components/Preferences/ProxySettings/index.js
- packages/bruno-app/src/providers/ReduxStore/slices/app.js
- packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts
- packages/bruno-requests/src/network/system-proxy/utils/common.ts
- packages/bruno-requests/src/index.ts
- packages/bruno-requests/src/network/system-proxy/types.ts
- packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js
- packages/bruno-requests/src/network/system-proxy/utils/macos.ts
- packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts
- packages/bruno-electron/src/ipc/preferences.js
- packages/bruno-requests/src/network/index.ts
- packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-requests/src/network/system-proxy/utils/common.spec.tspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-cli/src/utils/proxy-util.jspackages/bruno-requests/src/network/system-proxy/utils/windows.tspackages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-requests/src/network/system-proxy/utils/linux.tspackages/bruno-electron/src/utils/proxy-util.jspackages/bruno-requests/src/network/system-proxy/index.tspackages/bruno-electron/src/store/preferences.jspackages/bruno-electron/src/ipc/network/cert-utils.js
🧬 Code graph analysis (2)
packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts (1)
packages/bruno-requests/src/network/system-proxy/utils/common.ts (2)
normalizeProxyUrl(13-23)normalizeNoProxy(30-40)
packages/bruno-requests/src/network/system-proxy/index.ts (7)
packages/bruno-requests/src/network/system-proxy/types.ts (2)
ProxyConfiguration(1-6)SystemProxyDetectorOptions(12-15)packages/bruno-requests/src/network/system-proxy/utils/windows.ts (1)
WindowsProxyDetector(16-235)packages/bruno-requests/src/network/system-proxy/utils/macos.ts (1)
MacOSProxyDetector(8-118)packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
LinuxProxyDetector(24-244)packages/bruno-requests/src/network/system-proxy/utils/common.ts (2)
normalizeProxyUrl(13-23)normalizeNoProxy(30-40)packages/bruno-requests/src/index.ts (1)
getSystemProxy(10-10)packages/bruno-requests/src/network/index.ts (1)
getSystemProxy(3-3)
🪛 ast-grep (0.40.0)
packages/bruno-requests/src/network/system-proxy/utils/linux.ts
[warning] 208-208: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^export\\s+${varName}\\s*=\\s*(.+)$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 209-209: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${varName}\\s*=\\s*(.+)$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.1.2)
packages/bruno-electron/src/utils/proxy-util.js
[error] 88-103: This constructor has code paths that return without calling super().
If this is intentional, add an explicit throw statement in unsupported paths.
(lint/correctness/noUnreachableSuper)
⏰ 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). (6)
- GitHub Check: Unit Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (12)
packages/bruno-cli/src/utils/proxy-util.js (1)
82-85: Export surface change looks good; confirm no remaining callers of removed helper
module.exportsnow only exposesshouldUseProxyandPatchedHttpsProxyAgent, which matches the PR's move toward centralized system-proxy handling elsewhere. No issues with the remaining exports.Please confirm that any prior consumers of the removed helper (e.g.,
getSystemProxyEnvVariables) have been updated or removed across the repo so there are no lingering import errors.packages/bruno-app/src/providers/App/useIpcEvents.js (1)
2-6: Import ofupdatePreferencesis consistent and necessaryThe added
updatePreferencesnamed import matches its usage in the'main:load-preferences'handler below and aligns with existing import style; no issues from this change in this file.packages/bruno-electron/src/store/preferences.js (1)
30-30: LGTM - Default proxy mode updated to 'system'.The default proxy mode change from 'off' to 'system' is consistent across both
defaultPreferences(line 30) andgetGlobalProxyConfig(line 195). This aligns with the PR's system proxy detection feature.Also applies to: 195-195
packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts (1)
1-64: LGTM - Comprehensive test coverage.The test suite thoroughly covers edge cases for both
normalizeProxyUrlandnormalizeNoProxyutilities, including empty inputs, protocol handling, delimiter normalization, and whitespace handling.packages/bruno-requests/src/network/system-proxy/utils/windows.ts (2)
9-50: LGTM - Well-structured detection with command injection prevention.The
AllowedWindowsCommandsenum effectively prevents command injection, and the cascading fallback strategy (Internet Settings → WinHTTP → System/User environment) provides robust detection across different Windows configurations.
147-154: Past review comment addressed.The normalization for environment-based proxy URLs has been implemented as suggested in the previous review. The code now applies
normalizeProxyUrlandnormalizeNoProxyto registry-derived values.packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
11-57: LGTM - Robust Linux proxy detection.The command injection prevention via
AllowedLinuxCommandsenum and the cascading detection strategy (GNOME GSettings → KDE → environment files → systemd) provide comprehensive coverage across different Linux desktop environments and configurations.packages/bruno-requests/src/network/system-proxy/index.ts (2)
22-44: LGTM - Effective caching and concurrency control.The implementation prevents duplicate concurrent detections by checking
activeDetection(lines 29-31) and properly clears it in thefinallyblock (line 42) after caching the result.
106-120: LGTM - Sensible environment variable precedence and error handling.The function prioritizes environment variables (lines 107-113) over system detection, which is correct behavior. The try-catch fallback (lines 115-119) ensures a valid
ProxyConfigurationis always returned, even if system detection fails.packages/bruno-electron/src/utils/proxy-util.js (2)
337-340: CentralizedhttpProxyAgentOptionsdefinition looks goodPulling the HTTP proxy agent options (currently
keepAlive: true) into a sharedhttpProxyAgentOptionsconstant keeps thesetupProxyAgentsbranches consistent and makes it easier to tune behaviour in one place. No issues from a logic/readability standpoint.
365-368: WiringTimelineHttpProxyAgentfor explicit HTTP proxies is consistent with the new helperThe
proxyMode === 'on'HTTP branch now instantiatesTimelineHttpProxyAgentwith{ proxy: proxyUri, httpProxyAgentOptions }, which matches the expected shape of the newcreateTimelineHttpAgentClasshelper and cleanly separates HTTP proxy handling from the TLS-awareTimelineHttpsProxyAgent.Once the constructor of
createTimelineHttpAgentClassis tightened up as suggested above, this call site should be safe and future-proof.packages/bruno-cli/src/runner/run-single-request.js (1)
291-293: Harden system proxy handling: avoid hard failures on detection errors and surface invalid proxy configTwo coupled concerns here:
- Unprotected
getSystemProxy()during mode selection can abort the whole requestWhen
collectionProxyEnabled === 'global'you callgetSystemProxy()outside a try/catch. If this call rejects (e.g., platform-specific detector fails), the error bubbles to the outermostrunSingleRequesttry/catch and the request is returned as an overall error instead of just falling back to direct connectivity. That's a behavioral regression relative to the "best-effort" nature of system proxies.
- Invalid system proxies are silently ignored
Inside the
proxyMode === 'system'branch, thrown errors like'Invalid system http_proxy'are immediately swallowed by the catch block at Line 385, which only setsrequest.httpsAgentwithout logging or rethrowing. Users won't see why system proxy was skipped.
getSystemProxy()is called twiceYou first call it while deciding
proxyMode, then again inside thesystembranch. This duplication makes error handling unnecessarily complex.Suggested direction
- Fetch
systemProxyonce at initialization and reuse it throughout.- Wrap the initial detection in a local try/catch so a detector failure simply leaves
proxyMode = 'off'.- Log invalid proxies instead of silently ignoring them.
Relevant sections: lines 305–311 (proxy mode selection), 352–385 (system proxy handling).
lohit-bruno
force-pushed
the
system_proxy_updates
branch
from
73ef5b7 to
c23eb0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-electron/src/utils/proxy-util.js (1)
136-141: Same credential redaction needed here.The
createTimelineAgentClassalso logs proxy URIs at line 140. Apply the same credential redaction approach to avoid leaking secrets.
♻️ Duplicate comments (1)
packages/bruno-electron/src/utils/proxy-util.js (1)
99-104: Redact credentials from proxy URI before logging.The proxy URI may contain embedded credentials (e.g.,
http://user:[email protected]:8080). Logging the full URI leaks secrets into the timeline. Consider parsing and redacting userinfo before logging.+ // Redact credentials from proxy URI for logging + let safeProxyUri = proxyUri; + try { + const parsed = new URL(proxyUri); + if (parsed.username || parsed.password) { + parsed.username = parsed.username ? '***' : ''; + parsed.password = parsed.password ? '***' : ''; + safeProxyUri = parsed.toString(); + } + } catch (e) { + // If parsing fails, use a generic message + safeProxyUri = '[proxy]'; + } this.timeline.push({ timestamp: new Date(), type: 'info', - message: `Using proxy: ${proxyUri}` + message: `Using proxy: ${safeProxyUri}` });Based on past review comments, this concern was previously flagged.
🧹 Nitpick comments (7)
packages/bruno-electron/src/utils/proxy-util.js (1)
400-400: Minor formatting: add space after comma.Per coding guidelines, maintain consistent spacing around spread operators.
- { proxy: https_proxy,...tlsOptions }, + { proxy: https_proxy, ...tlsOptions },packages/bruno-app/src/components/Preferences/ProxySettings/index.js (1)
11-11: Consider simplifying the import path.The
/indexsuffix is unnecessary since ES modules resolve directory imports toindex.jsby default.-import SystemProxy from './SystemProxy/index'; +import SystemProxy from './SystemProxy';packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js (2)
10-12: Consider adding loading/error state handling.The component dispatches the IPC call on mount but doesn't handle loading or error states. If the IPC call is slow or fails, users see stale or no data without feedback.
+const [isLoading, setIsLoading] = useState(true); + useEffect(() => { - dispatch(getSystemProxyVariables()); + dispatch(getSystemProxyVariables()) + .finally(() => setIsLoading(false)); }, [dispatch]);You'd also need to import
useStateand conditionally render a loading indicator.
36-39: Nested ternary is hard to read.Consider using a mapping object or switch statement for source label resolution.
+const SOURCE_LABELS = { + 'environment': 'Environment Variables', + 'windows-system': 'Windows System Settings', + 'macos-system': 'macOS System Settings', + 'linux-system': 'Linux System Settings' +}; -{source === 'environment' ? 'Environment Variables' - : source === 'windows-system' ? 'Windows System Settings' - : source === 'macos-system' ? 'macOS System Settings' - : source === 'linux-system' ? 'Linux System Settings' : source} +{SOURCE_LABELS[source] || source}packages/bruno-requests/src/network/system-proxy/utils/windows.ts (1)
131-145: Use case-insensitive matching for registry proxy environment variable names.Windows registry stores environment variable names case-insensitively but case-preservingly, so users may set
http_proxy,Http_Proxy, orHTTP_PROXYwhen persisting these values. The current string matching only catches uppercase variants. Apply case-insensitive matching consistently:-if (trimmedLine.includes('HTTP_PROXY') && trimmedLine.includes('REG_SZ')) { - const match = trimmedLine.match(/HTTP_PROXY\s+REG_SZ\s+(.+)/); +if (trimmedLine.toUpperCase().includes('HTTP_PROXY') && trimmedLine.includes('REG_SZ')) { + const match = trimmedLine.match(/HTTP_PROXY\s+REG_SZ\s+(.+)/i);Apply the same pattern to
HTTPS_PROXYandNO_PROXYchecks in bothgetSystemProxyEnvironmentandgetUserEnvironmentProxy.packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
206-223: Static analysis ReDoS warning is a false positive.The
proxyVarsarray is a hardcoded constant containing safe alphanumeric strings with underscores. No user input flows into the regex construction.However, creating
RegExpobjects inside nested loops is inefficient. Consider pre-compiling the patterns:+// Pre-compile patterns for proxy variable detection +const PROXY_VAR_PATTERNS = ['http_proxy', 'https_proxy', 'no_proxy', 'all_proxy', 'HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY', 'ALL_PROXY'] + .flatMap((varName) => [ + { varName: varName.toLowerCase(), pattern: new RegExp(`^export\\s+${varName}\\s*=\\s*(.+)$`, 'i') }, + { varName: varName.toLowerCase(), pattern: new RegExp(`^${varName}\\s*=\\s*(.+)$`, 'i') } + ]); private parseProxyFromContent(content: string): ProxyConfiguration | null { - const proxyVars = ['http_proxy', 'https_proxy', 'no_proxy', 'all_proxy', 'HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY', 'ALL_PROXY']; const proxies: Record<string, string> = {}; // ... for (const line of lines) { // ... systemd handling ... - for (const varName of proxyVars) { - const patterns = [ - new RegExp(`^export\\s+${varName}\\s*=\\s*(.+)$`, 'i'), - new RegExp(`^${varName}\\s*=\\s*(.+)$`, 'i') - ]; - for (const pattern of patterns) { - const match = trimmedLine.match(pattern); - if (match) { - // ... - } - } - } + for (const { varName, pattern } of PROXY_VAR_PATTERNS) { + const match = trimmedLine.match(pattern); + if (match) { + let value = match[1].trim(); + value = value.replace(/^["']|["']$/g, ''); + proxies[varName] = value; + break; + } + } } }packages/bruno-requests/src/network/system-proxy/utils/macos.ts (1)
101-109: Replace<local>with explicit bypass entries for cross-platform compatibility.The
<local>token is not a standard proxy bypass syntax—it's a Windows/IE-specific convention. macOS itself doesn't recognize it, and downstream consumers (curl, Docker, other platforms, or even Bruno's own proxy configuration serialization) won't interpret it correctly. Use explicit portable entries instead:localhost,127.0.0.1,::1, and any internal domain patterns. This ensures the bypass list works consistently across platforms.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js(1 hunks)packages/bruno-app/src/components/Preferences/ProxySettings/index.js(2 hunks)packages/bruno-app/src/components/ResponsePane/index.js(1 hunks)packages/bruno-app/src/providers/App/useIpcEvents.js(1 hunks)packages/bruno-app/src/providers/ReduxStore/slices/app.js(4 hunks)packages/bruno-cli/src/runner/run-single-request.js(4 hunks)packages/bruno-cli/src/utils/proxy-util.js(1 hunks)packages/bruno-electron/src/ipc/network/cert-utils.js(2 hunks)packages/bruno-electron/src/ipc/preferences.js(2 hunks)packages/bruno-electron/src/store/preferences.js(2 hunks)packages/bruno-electron/src/utils/proxy-util.js(5 hunks)packages/bruno-requests/src/index.ts(1 hunks)packages/bruno-requests/src/network/index.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/index.spec.js(1 hunks)packages/bruno-requests/src/network/system-proxy/index.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/types.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/common.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/linux.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/macos.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/windows.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/bruno-electron/src/store/preferences.js
- packages/bruno-requests/src/network/system-proxy/index.spec.js
- packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts
- packages/bruno-requests/src/network/index.ts
- packages/bruno-electron/src/ipc/preferences.js
- packages/bruno-requests/src/network/system-proxy/index.ts
- packages/bruno-requests/src/index.ts
- packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts
- packages/bruno-requests/src/network/system-proxy/utils/common.ts
- packages/bruno-cli/src/runner/run-single-request.js
- packages/bruno-app/src/providers/App/useIpcEvents.js
- packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/ResponsePane/index.jspackages/bruno-cli/src/utils/proxy-util.jspackages/bruno-app/src/components/Preferences/ProxySettings/index.jspackages/bruno-requests/src/network/system-proxy/utils/macos.spec.tspackages/bruno-requests/src/network/system-proxy/utils/macos.tspackages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.jspackages/bruno-electron/src/utils/proxy-util.jspackages/bruno-app/src/providers/ReduxStore/slices/app.jspackages/bruno-requests/src/network/system-proxy/utils/linux.tspackages/bruno-electron/src/ipc/network/cert-utils.jspackages/bruno-requests/src/network/system-proxy/types.tspackages/bruno-requests/src/network/system-proxy/utils/windows.ts
🧬 Code graph analysis (8)
packages/bruno-app/src/components/Preferences/ProxySettings/index.js (1)
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js (1)
SystemProxy(5-70)
packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts (1)
packages/bruno-requests/src/network/system-proxy/utils/macos.ts (1)
MacOSProxyDetector(8-118)
packages/bruno-requests/src/network/system-proxy/utils/macos.ts (2)
packages/bruno-requests/src/network/system-proxy/types.ts (2)
ProxyDetector(8-10)ProxyConfiguration(1-6)packages/bruno-requests/src/network/system-proxy/utils/common.ts (2)
normalizeProxyUrl(13-23)normalizeNoProxy(30-40)
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js (1)
packages/bruno-app/src/providers/ReduxStore/slices/app.js (2)
getSystemProxyVariables(201-210)getSystemProxyVariables(201-210)
packages/bruno-electron/src/utils/proxy-util.js (1)
packages/bruno-cli/src/utils/proxy-util.js (3)
shouldUseProxy(16-63)parsedUrl(26-26)parseUrl(1-1)
packages/bruno-app/src/providers/ReduxStore/slices/app.js (2)
packages/bruno-app/src/utils/network/ws-event-listeners.js (1)
window(8-8)packages/bruno-app/src/providers/App/ConfirmAppClose/index.js (1)
window(7-7)
packages/bruno-requests/src/network/system-proxy/utils/linux.ts (2)
packages/bruno-requests/src/network/system-proxy/types.ts (2)
ProxyDetector(8-10)ProxyConfiguration(1-6)packages/bruno-requests/src/network/system-proxy/utils/common.ts (2)
normalizeProxyUrl(13-23)normalizeNoProxy(30-40)
packages/bruno-electron/src/ipc/network/cert-utils.js (3)
packages/bruno-requests/src/network/system-proxy/index.ts (2)
getSystemProxy(22-44)getSystemProxy(106-120)packages/bruno-electron/src/utils/proxy-util.js (1)
proxyConfig(381-381)packages/bruno-app/src/components/CollectionSettings/index.js (1)
proxyConfig(42-42)
🪛 ast-grep (0.40.0)
packages/bruno-requests/src/network/system-proxy/utils/linux.ts
[warning] 208-208: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^export\\s+${varName}\\s*=\\s*(.+)$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 209-209: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${varName}\\s*=\\s*(.+)$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (6)
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
🔇 Additional comments (25)
packages/bruno-electron/src/utils/proxy-util.js (3)
339-341: LGTM!The
httpProxyAgentOptionswithkeepAlive: trueis appropriate for connection reuse.
367-369: LGTM!The integration of
TimelineHttpProxyAgentfor HTTP proxy handling is consistent with the HTTPS proxy pattern.
381-413: System proxy logic looks correct.The routing logic correctly:
- Uses
http_proxyfor non-HTTPS requests- Uses
https_proxyfor HTTPS requests- Falls back to a standard HTTPS agent when no proxy applies
One note: ensure upstream callers provide
proxyConfigwith the expected shape ({ http_proxy, https_proxy, no_proxy }). The code already defends against missingproxyConfigwith theproxyConfig || {}pattern, but verify callers consistently provide the required properties.packages/bruno-electron/src/ipc/network/cert-utils.js (2)
4-4: Verification ofgetSystemProxyAPI import could not be completed.The import addition appears structurally sound, but verification of the
getSystemProxyAPI export and ProxyConfiguration type signature could not be performed due to repository access limitations. To ensure correctness, manually verify:
- The
getSystemProxyfunction is properly exported from@usebruno/requests- The returned ProxyConfiguration structure matches expectations in consuming code (especially proxy-util.js which destructures
http_proxy,https_proxy,no_proxy)- Error handling from
getSystemProxy()aligns with the application's proxy fallback strategy
116-119: Use strict equality for string comparison.Line 116 should use strict equality (
===) instead of loose equality (==). This follows JavaScript best practices and prevents unintended type coercion. Apply the following change:- if (proxyMode == 'system') { + if (proxyMode === 'system') { const systemProxyConfig = await getSystemProxy(); proxyConfig = systemProxyConfig; }Verify that
getSystemProxy()returns an object with the standard Node.js proxy environment variable fields:http_proxy,https_proxy, andno_proxy.packages/bruno-app/src/components/ResponsePane/index.js (1)
162-162: LGTM!Good addition of
data-testidfor testing purposes. Clean and non-intrusive.packages/bruno-app/src/components/Preferences/ProxySettings/index.js (1)
161-164: LGTM!Clean integration of the SystemProxy component. The conditional rendering based on
formik.values.mode === 'system'is appropriate.packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js (1)
14-68: LGTM on the component structure.The read-only display with fallbacks (
|| '-') and clear user guidance about OS documentation is well done.packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts (3)
1-22: LGTM on test setup.Clean mocking of
child_processandutilmodules. ThebeforeEachproperly resets mocks between tests.
207-227: Test now properly validates malformed input handling.The input at line 214 (
HTTPSProxy proxy.usebruno.com) lacks the colon separator, correctly testing graceful degradation. The expectation thathttps_proxyisnullwhilehttp_proxyis parsed confirms the parser handles partial malformation.
230-242: LGTM on error handling tests.Good coverage of both command failure and invalid output scenarios.
packages/bruno-requests/src/network/system-proxy/utils/windows.ts (4)
9-14: Good security practice with command allowlist.Using an enum to constrain shell commands prevents injection vulnerabilities. Clean approach.
25-44: LGTM on detection strategy waterfall.The ordered fallback through four detection methods with graceful error handling is robust. Each method failing silently allows the next to attempt detection.
206-234: LGTM onparseProxyStringimplementation.Correctly handles both protocol-specific (
http=proxy1:8080;https=proxy2:8080) and single-proxy formats. Normalization is properly applied.
221-226: Verify single-proxy format for HTTPS.When a single proxy is used for all protocols, both
http_proxyandhttps_proxyget the same normalized URL. However,normalizeProxyUrl(proxyServer)defaults tohttp://scheme. Downstream consumers expectinghttps://forhttps_proxymay behave unexpectedly.Is it intentional that
https_proxyuseshttp://scheme when a single proxy is configured? Some proxy clients require matching schemes.packages/bruno-cli/src/utils/proxy-util.js (1)
82-85: Verify no remaining usages of removedgetSystemProxyEnvVariables.The export was removed from module.exports. Confirm no callers remain in the CLI package to avoid runtime errors.
packages/bruno-app/src/providers/ReduxStore/slices/app.js (2)
46-47: State shape update looks correct.The rename from
systemProxyEnvVariablestosystemProxyVariablesaligns with the broader refactor for system proxy detection.
93-95: Reducer rename is consistent.
updateSystemProxyVariablescorrectly updates the renamed state field.packages/bruno-requests/src/network/system-proxy/utils/macos.ts (2)
8-22: Clean implementation of macOS proxy detection.The detector correctly uses
scutil --proxy, applies reasonable timeouts, and handles the dictionary output format. The use ofnormalizeProxyUrlandnormalizeNoProxyfrom common utilities ensures consistency.
41-82: Parsing logic handles scutil output correctly.The recursive dictionary and array parsing covers typical
scutil --proxyoutput. One minor observation: ifHTTPPortorHTTPSPortare explicitly set to0in the config, the fallback to default ports (80/443) would override that. This is likely acceptable since port 0 is invalid for proxies.packages/bruno-requests/src/network/system-proxy/utils/linux.ts (4)
10-22: Good security practice with AllowedLinuxCommands enum.Using an enum of pre-defined commands prevents command injection vulnerabilities. This is a solid pattern for system command execution.
32-56: Detection strategy chain is well-designed.The fallback approach (GNOME → KDE → /etc/environment → systemd) covers major Linux environments. Swallowing errors per-method and continuing to the next is appropriate here.
59-91: GSettings parsing handles gsettings output quirks correctly.The single-quote stripping on line 72 and array bracket handling on line 81 properly account for gsettings' output format.
146-173: Systemd proxy detection covers common config locations.Reading from
/etc/systemd/system.conf.d/with fallback files is a reasonable approach. Note that this won't detect proxies set viasystemctl set-environment, but that's acceptable for a file-based detection strategy.packages/bruno-requests/src/network/system-proxy/types.ts (1)
1-15: Type definitions are well-structured.Clear interfaces for
ProxyConfiguration,ProxyDetector, andSystemProxyDetectorOptions. Thesourceunion type properly distinguishes between environment and platform-specific sources.The question about
SystemProxyDetectorOptionsdefiningcacheTimeoutandcommandTimeoutwhileProxyDetector.detect()usestimeoutMsshould be verified against the implementation to confirm these options are wired correctly and understand whether they serve different purposes (e.g., cache vs. command execution timeout) or if there's a naming inconsistency that needs alignment.
lohit-bruno
force-pushed
the
system_proxy_updates
branch
from
c23eb0b to
5f3416b
Compare
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)
packages/bruno-cli/src/runner/run-single-request.js (1)
304-313: Refactor to avoid duplicate system proxy fetch.The system proxy is fetched twice:
- Line 306: To determine if system proxy should be used
- Line 353: To actually apply the system proxy configuration
This is inefficient and could lead to inconsistencies if the system proxy changes between calls. Cache the result from the first call and reuse it.
Apply this diff to cache and reuse the system proxy:
let proxyMode = 'off'; let proxyConfig = {}; + let systemProxyConfig = null; const collectionProxyConfig = get(brunoConfig, 'proxy', {}); const collectionProxyEnabled = get(collectionProxyConfig, 'enabled', false); if (noproxy) { // If noproxy flag is set, don't use any proxy proxyMode = 'off'; } else if (collectionProxyEnabled === true) { // If collection proxy is enabled, use it proxyConfig = collectionProxyConfig; proxyMode = 'on'; } else if (collectionProxyEnabled === 'global') { // If collection proxy is set to 'global', use system proxy - const systemProxy = await getSystemProxy(); - const { http_proxy, https_proxy } = systemProxy; + systemProxyConfig = await getSystemProxy(); + const { http_proxy, https_proxy } = systemProxyConfig; if (http_proxy?.length || https_proxy?.length) { proxyMode = 'system'; } } else { proxyMode = 'off'; } // ... existing code ... } else if (proxyMode === 'system') { try { - const systemProxy = await getSystemProxy(); - const { http_proxy, https_proxy, no_proxy } = systemProxy; + const { http_proxy, https_proxy, no_proxy } = systemProxyConfig; const shouldUseSystemProxy = shouldUseProxy(request.url, no_proxy || '');Also applies to: 351-389
♻️ Duplicate comments (2)
packages/bruno-requests/src/network/system-proxy/index.ts (1)
97-101: Avoid clearingactiveDetectioninsideclearCacheto preserve dedup semanticsIf
clearCache()is called while a detection is in progress, settingactiveDetection = nullallows the nextgetSystemProxy()call to start a second detection instead of reusing the in‑flight one. That defeats the in-flight dedup behavior and can cause duplicate work.Since
clearCache()cannot actually cancel the existing Promise, it’s safer to leaveactiveDetectionuntouched here and only reset the cached value and timestamp:clearCache(): void { this.cachedResult = null; this.cacheTimestamp = 0; - this.activeDetection = null; }If you really need “force refresh even if one is in flight”, I’d make that an explicit API (e.g.
refresh()) and document the semantics.packages/bruno-electron/src/utils/proxy-util.js (1)
98-105: Avoid logging or throwing full proxy URIs that may contain credentialsRight now the timeline entries and error messages include the raw proxy URI. If the proxy URI contains
user:pass@host, this will leak credentials into logs/timelines and possibly the UI.Consider redacting userinfo before logging or including it in error text. One way to do this:
class PatchedHttpsProxyAgent extends HttpsProxyAgent { constructor(proxy, opts) { super(proxy, opts); this.constructorOpts = opts; } } +function redactProxyUri(uri) { + try { + const url = new URL(uri); + if (url.username || url.password) { + url.username = '***'; + url.password = '***'; + } + return url.toString(); + } catch { + return uri; + } +} + function createTimelineHttpAgentClass(BaseAgentClass) { @@ - this.timeline.push({ - timestamp: new Date(), - type: 'info', - message: `Using proxy: ${proxyUri}` - }); + this.timeline.push({ + timestamp: new Date(), + type: 'info', + message: `Using proxy: ${redactProxyUri(proxyUri)}` + });- this.timeline.push({ - timestamp: new Date(), - type: 'info', - message: `Using proxy: ${proxyUri}`, - }); + this.timeline.push({ + timestamp: new Date(), + type: 'info', + message: `Using proxy: ${redactProxyUri(proxyUri)}`, + });- throw new Error(`Invalid system http_proxy "${http_proxy}": ${error.message}`); + throw new Error(`Invalid system http_proxy "${redactProxyUri(http_proxy)}": ${error.message}`);- throw new Error(`Invalid system https_proxy "${https_proxy}": ${error.message}`); + throw new Error(`Invalid system https_proxy "${redactProxyUri(https_proxy)}": ${error.message}`);Also applies to: 137-141, 393-393, 408-408
🧹 Nitpick comments (8)
packages/bruno-cli/src/runner/run-single-request.js (1)
360-379: Consider simplifying nested try-catch blocks.The nested try-catch blocks for http_proxy and https_proxy validation add complexity. While the logic is correct, consider extracting proxy validation to a helper function for better readability.
Example helper function:
const validateAndSetProxy = (proxyUrl, proxyType, agentType, agentOptions) => { try { if (proxyUrl?.length) { new URL(proxyUrl); if (agentType === 'http') { request.httpAgent = new HttpProxyAgent(proxyUrl); } else { request.httpsAgent = new PatchedHttpsProxyAgent(proxyUrl, agentOptions); } return true; } return false; } catch (error) { throw new Error(`Invalid system ${proxyType}_proxy`); } };packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js (1)
13-18: MoveSOURCE_LABELSoutside the component.This constant is recreated on every render. Since it never changes, define it at module scope for better performance.
+const SOURCE_LABELS = { + 'environment': 'Environment Variables', + 'windows-system': 'Windows System Settings', + 'macos-system': 'macOS System Settings', + 'linux-system': 'Linux System Settings' +}; + const SystemProxy = () => { const dispatch = useDispatch(); const systemProxyVariables = useSelector((state) => state.app.systemProxyVariables); const { source, http_proxy, https_proxy, no_proxy } = systemProxyVariables || {}; const [isFetching, setIsFetching] = useState(true); const [error, setError] = useState(null); - - const SOURCE_LABELS = { - 'environment': 'Environment Variables', - 'windows-system': 'Windows System Settings', - 'macos-system': 'macOS System Settings', - 'linux-system': 'Linux System Settings' - };packages/bruno-requests/src/network/system-proxy/utils/macos.ts (1)
41-82: Consider edge case: array terminator detection.The array parsing loop at line 62 terminates on any line containing
}. If an array element itself contains}(unlikely but possible in edge cases), parsing would terminate early.This is a minor edge case since proxy exception lists typically contain hostnames/IPs, but worth noting.
packages/bruno-requests/src/network/system-proxy/types.ts (1)
1-6: Consider simplifying nullability syntax.The fields use both
?(optional) and| null. This is redundant—?already allowsundefined. If you want to explicitly allownullvalues (distinct from missing keys), the current approach is fine. Otherwise, simplify:export interface ProxyConfiguration { - http_proxy?: string | null; - https_proxy?: string | null; - no_proxy?: string | null; + http_proxy?: string; + https_proxy?: string; + no_proxy?: string; source: 'environment' | 'windows-system' | 'macos-system' | 'linux-system'; }packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
153-180: Hardcoded systemd file paths may miss configurations.The detector only checks
proxy.confandenvironment.confin/etc/systemd/system.conf.d/. Some distributions or users may use different filenames.Consider using
readdirto scan all.conffiles in the directory:- const systemdFiles = ['proxy.conf', 'environment.conf']; + const { readdir } = require('node:fs/promises'); + const files = await readdir(systemdConfDir); + const systemdFiles = files.filter((f) => f.endsWith('.conf'));packages/bruno-requests/src/network/system-proxy/index.ts (2)
16-20: Prefer??over||for numeric option defaultsUsing
||means an explicit0forcacheTimeoutorcommandTimeoutis treated as “unset” and replaced with the default, which makes it impossible to intentionally disable caching/timeouts with0.You can make the intent clearer and allow
0as a valid value by switching to nullish coalescing:constructor(options: SystemProxyDetectorOptions = {}) { this.platform = platform(); - this.cacheTimeout = options.cacheTimeout || 60000; // 60 seconds default - this.commandTimeout = options.commandTimeout || 10000; // 10 seconds default + this.cacheTimeout = options.cacheTimeout ?? 60000; // 60 seconds default + this.commandTimeout = options.commandTimeout ?? 10000; // 10 seconds default }
65-67:detectWithTimeoutis currently a pass‑through abstractionRight now
detectWithTimeoutjust forwards todetectByPlatformand adds only an extra stack frame. Given the guideline to avoid single-line abstractions, either:
- Inline it in
detectSystemProxy:- const result = await this.detectWithTimeout(); + const result = await this.detectByPlatform();and drop
detectWithTimeout, or
- Add actual timeout behavior here (e.g.
Promise.racewith a timer) and a short JSDoc explaining that it owns the timeout semantics.packages/bruno-electron/src/utils/proxy-util.js (1)
86-96: Optional: simplifycreateTimelineHttpAgentClassoptions shapeThe current pattern:
- Call sites pass
{ proxy, httpProxyAgentOptions }.- The factory immediately unpacks
{ proxy: proxyUri, httpProxyAgentOptions }and forwardshttpProxyAgentOptionstosuper.Functionally this is fine, but you could make the HTTP factory mirror
createTimelineAgentClassand accept{ proxy, ...agentOptions }, merging inkeepAlive: trueinternally. That would:
- Remove the extra
httpProxyAgentOptionswrapper, and- Keep the “options” contract consistent across the timeline agent factories.
Purely a readability/API-tidy suggestion; not a blocker.
Also applies to: 339-341, 367-370, 387-391
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js(1 hunks)packages/bruno-app/src/components/Preferences/ProxySettings/index.js(2 hunks)packages/bruno-app/src/components/ResponsePane/index.js(1 hunks)packages/bruno-app/src/providers/App/useIpcEvents.js(1 hunks)packages/bruno-app/src/providers/ReduxStore/slices/app.js(4 hunks)packages/bruno-cli/src/runner/run-single-request.js(4 hunks)packages/bruno-cli/src/utils/proxy-util.js(1 hunks)packages/bruno-electron/src/ipc/network/cert-utils.js(2 hunks)packages/bruno-electron/src/ipc/preferences.js(2 hunks)packages/bruno-electron/src/store/preferences.js(2 hunks)packages/bruno-electron/src/utils/proxy-util.js(4 hunks)packages/bruno-requests/src/index.ts(1 hunks)packages/bruno-requests/src/network/index.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/index.spec.js(1 hunks)packages/bruno-requests/src/network/system-proxy/index.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/types.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/common.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/linux.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/macos.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/windows.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts
- packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts
- packages/bruno-cli/src/utils/proxy-util.js
- packages/bruno-app/src/providers/App/useIpcEvents.js
- packages/bruno-app/src/providers/ReduxStore/slices/app.js
- packages/bruno-app/src/components/ResponsePane/index.js
- packages/bruno-requests/src/network/system-proxy/index.spec.js
- packages/bruno-requests/src/network/system-proxy/utils/common.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-electron/src/store/preferences.jspackages/bruno-app/src/components/Preferences/ProxySettings/index.jspackages/bruno-requests/src/network/system-proxy/utils/windows.tspackages/bruno-requests/src/network/system-proxy/index.tspackages/bruno-requests/src/network/system-proxy/types.tspackages/bruno-electron/src/ipc/network/cert-utils.jspackages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-requests/src/network/system-proxy/utils/windows.spec.tspackages/bruno-electron/src/utils/proxy-util.jspackages/bruno-requests/src/network/system-proxy/utils/linux.spec.tspackages/bruno-electron/src/ipc/preferences.jspackages/bruno-requests/src/network/index.tspackages/bruno-requests/src/network/system-proxy/utils/macos.tspackages/bruno-requests/src/network/system-proxy/utils/linux.tspackages/bruno-requests/src/index.ts
🧠 Learnings (1)
📚 Learning: 2025-12-02T07:26:37.751Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-02T07:26:37.751Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Applied to files:
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts
🧬 Code graph analysis (8)
packages/bruno-electron/src/store/preferences.js (1)
packages/bruno-cli/src/runner/prepare-request.js (1)
get(1-1)
packages/bruno-requests/src/network/system-proxy/types.ts (1)
packages/bruno-requests/src/network/system-proxy/index.ts (1)
ProxyConfiguration(122-122)
packages/bruno-electron/src/ipc/network/cert-utils.js (3)
packages/bruno-requests/src/index.ts (1)
getSystemProxy(10-10)packages/bruno-requests/src/network/index.ts (1)
getSystemProxy(3-3)packages/bruno-requests/src/network/system-proxy/index.ts (2)
getSystemProxy(22-44)getSystemProxy(106-120)
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js (1)
packages/bruno-app/src/providers/ReduxStore/slices/app.js (2)
getSystemProxyVariables(201-211)getSystemProxyVariables(201-211)
packages/bruno-electron/src/utils/proxy-util.js (1)
packages/bruno-cli/src/utils/proxy-util.js (3)
shouldUseProxy(16-63)parsedUrl(26-26)parseUrl(1-1)
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts (2)
packages/bruno-requests/src/network/system-proxy/index.spec.js (1)
detector(11-11)packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
LinuxProxyDetector(31-244)
packages/bruno-electron/src/ipc/preferences.js (2)
packages/bruno-requests/src/index.ts (1)
getSystemProxy(10-10)packages/bruno-requests/src/network/system-proxy/index.ts (2)
getSystemProxy(22-44)getSystemProxy(106-120)
packages/bruno-requests/src/network/system-proxy/utils/macos.ts (2)
packages/bruno-requests/src/network/system-proxy/types.ts (2)
ProxyDetector(8-10)ProxyConfiguration(1-6)packages/bruno-requests/src/network/system-proxy/utils/common.ts (2)
normalizeProxyUrl(13-23)normalizeNoProxy(30-40)
🪛 ast-grep (0.40.0)
packages/bruno-requests/src/network/system-proxy/utils/linux.ts
[warning] 26-26: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^export\\s+${varName}\\s*=\\s*(.+)$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 27-27: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${varName}\\s*=\\s*(.+)$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (6)
- GitHub Check: CLI Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
- GitHub Check: Unit Tests
🔇 Additional comments (24)
packages/bruno-electron/src/store/preferences.js (1)
30-30: LGTM!The default proxy mode change to 'system' is consistent across both the defaultPreferences object and the getGlobalProxyConfig fallback. The validation schema already supports this value.
Also applies to: 195-195
packages/bruno-requests/src/network/index.ts (1)
1-3: LGTM!Clean export structure for the network module's public API.
packages/bruno-app/src/components/Preferences/ProxySettings/index.js (1)
11-11: LGTM!Clean refactoring that delegates system proxy rendering to a dedicated component, improving modularity and maintainability.
Also applies to: 163-163
packages/bruno-requests/src/index.ts (1)
8-10: LGTM!The new
getSystemProxyexport expands the public API surface appropriately for the system proxy feature.packages/bruno-electron/src/ipc/preferences.js (1)
52-55: LGTM!Clean on-demand IPC handler for system proxy data. The async implementation is correct, and this approach is more efficient than the previous eager-loading pattern.
packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts (1)
1-400: LGTM!Comprehensive test suite with excellent coverage of Windows proxy detection scenarios:
- Multiple detection sources (Registry, WinHTTP, Environment variables)
- Edge cases (protocol prefixes, empty bypass lists, disabled proxies)
- Error handling (access denied, malformed output)
The test organization is clear and the mock setup is appropriate for testing system-level operations.
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js (2)
20-25: LGTM on the data fetch pattern.The useEffect properly dispatches, handles errors, and cleans up loading state via
.finally(). Thedispatchdependency is correctly included.
27-89: Clean UI implementation with proper error and loading states.The component correctly handles all display states: loading spinner, error banner, and the read-only proxy values. The fallback to
'-'for empty values is user-friendly.packages/bruno-requests/src/network/system-proxy/utils/windows.ts (4)
8-14: Good use of command allowlist to prevent injection.The enum constrains shell commands to a fixed set, mitigating command injection risks.
25-50: Detection cascade is well structured.The ordered fallback approach ensures robust detection across different Windows proxy configurations. Error handling correctly continues to the next method on failure.
147-154: Normalization applied correctly after previous review feedback.The environment registry methods now properly normalize URLs via
normalizeProxyUrlandnormalizeNoProxy, ensuring consistent output format.Also applies to: 191-198
222-227: The suggestion is not necessary; using the same proxy address for both HTTP and HTTPS traffic is standard behavior.When a single proxy is configured in Windows Internet Options, the same proxy address (host:port) is applied to both HTTP and HTTPS traffic. The protocol prefix (
http=,https=) indicates the traffic type, not a different proxy server. Setting bothhttp_proxyandhttps_proxyto the same normalized URL is correct and aligns with Windows proxy conventions.Likely an incorrect or invalid review comment.
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts (2)
1-38: Comprehensive mock setup for Linux detector tests.The mocking strategy correctly isolates the detector from system dependencies. Using
jest.clearAllMocks()inbeforeEachensures clean state between tests.
93-99: Stray comment removed as per previous review.The
// ?debug marker flagged in the past review is no longer present.packages/bruno-requests/src/network/system-proxy/utils/macos.ts (2)
71-75: Dead code branches removed as per previous review.The boolean parsing branches for
'1'and'0'that were unreachable have been cleaned up. The logic now correctly parses integers and falls back to string values.
84-117: Solid proxy configuration building logic.The method correctly:
- Checks enable flags before using proxy values
- Applies sensible port defaults (80/443)
- Handles ExceptionsList arrays
- Appends
<local>for ExcludeSimpleHostnames- Normalizes output via helper functions
packages/bruno-requests/src/network/system-proxy/types.ts (1)
8-15: Well-defined interfaces for the detector pattern.The
ProxyDetectorinterface provides a clean contract for platform-specific implementations.SystemProxyDetectorOptionsoffers sensible configuration points.packages/bruno-requests/src/network/system-proxy/utils/linux.ts (5)
24-29: Static analysis ReDoS warning is a false positive.The
varNamevalues come from a hardcoded array of known proxy variable names, not user input. The patterns are safe.
10-22: Good command allowlist with stderr suppression.The enum constrains commands and redirects stderr to
/dev/nullto handle missing tools gracefully.
66-99: Well-implemented gsettings detection.Correctly checks for
'manual'mode before reading proxy values. The quote stripping and ignore-hosts parsing are handled properly.
101-139: KDE proxy detection with clear type documentation.The ProxyType value comments are helpful. Only manual configuration (type
1) is handled, which is appropriate for deterministic proxy settings.
182-243: Robust content parsing with multiple format support.The
parseProxyFromContentmethod handles:
- Shell variable formats (
VAR=value,export VAR=value)- Quoted values (single and double)
- Systemd directives (
Environment=,DefaultEnvironment=)ALL_PROXYfallbackThe implementation is thorough.
packages/bruno-requests/src/network/system-proxy/index.ts (1)
104-120: Environment‑first singleton API looks solidThe exported
getSystemProxycorrectly:
- Reads and normalizes env proxy variables once
- Short‑circuits to env when
http_proxy/https_proxy(orall_proxy) are present- Falls back to the shared
SystemProxyDetectorand degrades to env again on errorThis is a good, resilient shape for callers in CLI/Electron contexts.
packages/bruno-electron/src/utils/proxy-util.js (1)
381-385: Confirm invariants forrequestConfig.urlandproxyConfigin system proxy modeThis branch assumes:
requestConfig.urlis always a non-empty string (used directly inparseUrl(requestConfig.url)), andproxyConfigcarries lowercase keyshttp_proxy,https_proxy,no_proxy.If those invariants ever change (e.g.
urlmissing/undefined, or keys normalized differently), this block can either throw earlier than expected or silently skip the system proxy.If that’s not already enforced at the call site, consider a light guard here (early-return if
!requestConfig.url) or normalizingproxyConfigkeys once upstream.Also applies to: 387-391, 396-408
lohit-bruno
force-pushed
the
system_proxy_updates
branch
from
5f3416b to
2c17f1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts (1)
154-182: Mock ordering may cause flaky behavior.The
mockReturnValue(false)at line 161 sets the default, but execution order matters:getEnvironmentFileProxychecks/etc/environmentbeforegetSystemdProxychecks the systemd directory. ThemockReturnValueOncecalls at lines 164 and 167 may not align with actual call order.Use path-based mock or explicit sequential ordering:
- // Mock all previous methods to fail - mockExistsSync.mockReturnValue(false); - - // Mock systemd conf directory to exist - mockExistsSync.mockReturnValueOnce(true); - - // Mock systemd proxy file to exist - mockExistsSync.mockReturnValueOnce(true); + // Mock /etc/environment to not exist, then systemd dir and file to exist + mockExistsSync + .mockReturnValueOnce(false) // /etc/environment + .mockReturnValueOnce(true) // /etc/systemd/system.conf.d + .mockReturnValueOnce(true); // proxy.conf filepackages/bruno-electron/src/utils/proxy-util.js (1)
86-107: Redact credentials from proxy URI before logging.The
super()issue from the previous review has been resolved—the constructor now either throws beforesuper()(which is valid) or callssuper()before accessingthis.However, the credential leakage issue remains: Line 103 logs the full
proxyUri, which may contain credentials in the formathttp://user:[email protected]. Redact the userinfo portion before adding it to the timeline.Apply this diff to redact credentials:
+ // Redact credentials from proxy URI for logging + const redactedProxyUri = proxyUri.replace(/\/\/([^:]+):([^@]+)@/, '//$1:***@'); + this.timeline = Array.isArray(timeline) ? timeline : []; // Log the proxy details this.timeline.push({ timestamp: new Date(), type: 'info', - message: `Using proxy: ${proxyUri}` + message: `Using proxy: ${redactedProxyUri}` });
🧹 Nitpick comments (1)
packages/bruno-requests/src/network/system-proxy/index.ts (1)
58-69: Consider instantiating platform resolvers once.New resolver instances are created on each detection call. For better efficiency, instantiate them once during construction and reuse.
export class SystemProxyResolver { private platform: string; private commandTimeout: number; + private windowsResolver?: WindowsProxyResolver; + private macosResolver?: MacOSProxyResolver; + private linuxResolver?: LinuxProxyResolver; private activeDetection: Promise<ProxyConfiguration> | null = null; constructor(options: SystemProxyResolverOptions = {}) { this.platform = platform(); this.commandTimeout = options.commandTimeout || 10000; + // Initialize platform-specific resolver + if (this.platform === 'win32') { + this.windowsResolver = new WindowsProxyResolver(); + } else if (this.platform === 'darwin') { + this.macosResolver = new MacOSProxyResolver(); + } else if (this.platform === 'linux') { + this.linuxResolver = new LinuxProxyResolver(); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js(1 hunks)packages/bruno-app/src/components/Preferences/ProxySettings/index.js(2 hunks)packages/bruno-app/src/components/ResponsePane/index.js(1 hunks)packages/bruno-app/src/providers/App/useIpcEvents.js(1 hunks)packages/bruno-app/src/providers/ReduxStore/slices/app.js(4 hunks)packages/bruno-cli/src/runner/run-single-request.js(3 hunks)packages/bruno-cli/src/utils/proxy-util.js(1 hunks)packages/bruno-electron/src/ipc/network/cert-utils.js(2 hunks)packages/bruno-electron/src/ipc/preferences.js(2 hunks)packages/bruno-electron/src/store/preferences.js(2 hunks)packages/bruno-electron/src/utils/proxy-util.js(4 hunks)packages/bruno-requests/src/index.ts(1 hunks)packages/bruno-requests/src/network/index.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/index.spec.js(1 hunks)packages/bruno-requests/src/network/system-proxy/index.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/types.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/common.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/linux.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/macos.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts(1 hunks)packages/bruno-requests/src/network/system-proxy/utils/windows.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/bruno-app/src/components/ResponsePane/index.js
- packages/bruno-electron/src/ipc/network/cert-utils.js
- packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts
- packages/bruno-app/src/components/Preferences/ProxySettings/SystemProxy/index.js
- packages/bruno-requests/src/index.ts
- packages/bruno-cli/src/runner/run-single-request.js
- packages/bruno-requests/src/network/system-proxy/types.ts
- packages/bruno-cli/src/utils/proxy-util.js
- packages/bruno-requests/src/network/index.ts
- packages/bruno-requests/src/network/system-proxy/utils/common.spec.ts
- packages/bruno-requests/src/network/system-proxy/utils/common.ts
- packages/bruno-app/src/providers/ReduxStore/slices/app.js
- packages/bruno-app/src/providers/App/useIpcEvents.js
- packages/bruno-app/src/components/Preferences/ProxySettings/index.js
- packages/bruno-electron/src/store/preferences.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation, never tabs
Use single quotes for strings instead of double quotes
Always add semicolons at the end of statements
No trailing commas in code
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line with a minimum of 2 elements for multiline formatting
No newlines inside function parentheses
Space before and after the arrow in arrow functions:() => {}
No space between function name and parentheses:func()notfunc ()
Semicolons should go at the end of the line, not on a new line
Function names should be concise and descriptive
Add JSDoc comments to provide details to abstractions
Avoid single-line abstractions where all that is being done is increasing the call stack with one additional function
Add meaningful comments to explain complex code flow instead of obvious comments
Files:
packages/bruno-requests/src/network/system-proxy/utils/windows.tspackages/bruno-requests/src/network/system-proxy/utils/linux.spec.tspackages/bruno-electron/src/ipc/preferences.jspackages/bruno-requests/src/network/system-proxy/utils/macos.tspackages/bruno-requests/src/network/system-proxy/utils/linux.tspackages/bruno-requests/src/network/system-proxy/index.spec.jspackages/bruno-electron/src/utils/proxy-util.jspackages/bruno-requests/src/network/system-proxy/index.tspackages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Add tests for any new functionality or meaningful changes; update corresponding tests when code is added, removed, or significantly modified
Files:
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.tspackages/bruno-requests/src/network/system-proxy/index.spec.jspackages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-12-03T08:09:57.124Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-03T08:09:57.124Z
Learning: Applies to **/*.{test,spec}.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes; update corresponding tests when code is added, removed, or significantly modified
Applied to files:
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.tspackages/bruno-requests/src/network/system-proxy/index.spec.jspackages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts
🧬 Code graph analysis (3)
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts (1)
packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
LinuxProxyResolver(31-245)
packages/bruno-electron/src/utils/proxy-util.js (1)
packages/bruno-cli/src/utils/proxy-util.js (2)
shouldUseProxy(16-63)parseUrl(1-1)
packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts (1)
packages/bruno-requests/src/network/system-proxy/utils/windows.ts (1)
WindowsProxyResolver(16-236)
🪛 ast-grep (0.40.0)
packages/bruno-requests/src/network/system-proxy/utils/linux.ts
[warning] 26-26: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^export\\s+${varName}\\s*=\\s*(.+)$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 27-27: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${varName}\\s*=\\s*(.+)$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (6)
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Windows
🔇 Additional comments (39)
packages/bruno-requests/src/network/system-proxy/utils/linux.spec.ts (3)
1-38: LGTM! Test setup is well-structured.Mocks are correctly configured for all required dependencies, and the
beforeEachproperly initializes the detector and clears mocks between tests.
40-126: LGTM! gsettings test coverage is thorough.Tests properly cover manual proxy detection, identical proxies, non-manual mode fallback, and empty ignore hosts scenarios.
185-222: LGTM! Error handling tests are comprehensive.Covers gsettings unavailable, schema not installed, and no configuration found scenarios.
packages/bruno-requests/src/network/system-proxy/utils/windows.spec.ts (5)
1-23: LGTM! Clean test setup.Mock configuration and
beforeEachinitialization are correct.
24-84: LGTM! Registry detection tests are thorough.Covers single proxy, protocol-specific proxy, and fallback to WinHTTP scenarios.
86-157: LGTM! WinHTTP detection coverage is solid.Tests direct access handling, single/protocol-specific proxies, and "(none)" bypass list correctly.
159-229: LGTM! Environment detection tests are complete.Covers system-wide and user-specific proxy environment variables with proper fallback chain.
231-399: LGTM! Edge cases and error handling are comprehensive.Good coverage of prefix handling, decimal ProxyEnable values, malformed output, and complex bypass lists.
packages/bruno-electron/src/ipc/preferences.js (1)
6-7: LGTM! Import added correctly.packages/bruno-requests/src/network/system-proxy/utils/windows.ts (7)
1-14: LGTM! Clean setup with injection prevention.Using an enum for allowed commands is a good security practice.
16-50: LGTM! Detection flow is well-structured.Ordered fallback chain with proper error handling at each step.
52-92: LGTM! Registry parsing handles edge cases correctly.Supports both hex (
0x1) and decimal (1) ProxyEnable values.
94-116: LGTM! WinHTTP detection is robust.Correctly identifies "Direct access" as no-proxy scenario.
118-160: LGTM! System environment detection with proper normalization.The past review issue regarding missing normalization has been addressed.
162-204: LGTM! User environment detection mirrors system environment logic.Consistent implementation with proper normalization.
206-236: LGTM! Proxy string parsing handles both formats correctly.Protocol-specific (
http=...;https=...) and single proxy formats are properly handled.packages/bruno-requests/src/network/system-proxy/index.spec.js (4)
1-35: LGTM! Test setup and teardown are correct.Timer cleanup order is now proper:
runOnlyPendingTimers()executes pending timers beforeclearAllTimers()removes them.
37-78: LGTM! Environment variable tests are thorough.Covers lowercase/uppercase priority, fallback behavior, and null handling.
80-116: LGTM! Platform routing tests verify correct detector delegation.Each platform test properly mocks
os.platform()and verifies the correct resolver is invoked.
118-169: LGTM! Error handling and fallback tests are comprehensive.Covers detection failure fallback and timeout graceful handling.
packages/bruno-requests/src/network/system-proxy/utils/macos.ts (4)
1-22: LGTM! Clean setup with proper timeout handling.Uses
scutil --proxyfor macOS proxy detection with configurable timeout.
24-39: LGTM! Output parsing is robust.Validates input, finds dictionary section, and delegates to parsing helpers.
41-82: LGTM! Configuration parsing handles all value types.Arrays, integers, and strings are correctly parsed from scutil output.
84-118: LGTM! Proxy configuration builder is correct.Properly handles HTTP/HTTPS enables, default ports, ExceptionsList, and ExcludeSimpleHostnames.
packages/bruno-requests/src/network/system-proxy/utils/linux.ts (6)
1-29: LGTM! Safe command enumeration and pattern precompilation.The static analysis ReDoS warning on lines 26-27 is a false positive—
varNamecomes from a hardcoded array of trusted proxy variable names (http_proxy,https_proxy, etc.), not user input. The patterns are compiled once at module load time.
31-64: LGTM! Detection flow with proper fallback chain.Tries gsettings → KDE → /etc/environment → systemd in order, returning on first success.
66-99: LGTM! gsettings parsing handles GNOME proxy settings correctly.Properly checks for manual mode and cleans quoted values.
101-139: LGTM! KDE proxy detection is well-documented.ProxyType value meanings are documented, and only manual configuration (type 1) is handled.
141-181: LGTM! File-based proxy detection is robust.Both
/etc/environmentand systemd configuration directory are checked with proper null guards.
183-245: LGTM! Content parsing handles multiple formats.Supports shell
VAR=value,export VAR=value, and systemdEnvironment=directives with proper quote removal. ALL_PROXY fallback is correctly implemented.packages/bruno-requests/src/network/system-proxy/index.ts (6)
1-6: LGTM!Imports are well-organized and follow the coding guidelines.
8-16: LGTM!Class structure and constructor are clean. The 10-second default timeout is reasonable for system proxy detection.
18-33: LGTM!In-flight deduplication is correctly implemented. The
finallyblock ensuresactiveDetectionis cleared after the promise settles, preventing duplicate concurrent detections.
71-84: LGTM!Environment variable handling is comprehensive with proper fallback chain and normalization.
87-103: LGTM!The singleton pattern and environment variable precedence are appropriate. Environment variables correctly take precedence over system detection, matching expected behavior for proxy configuration in most CLI tools.
105-105: LGTM!Type re-export for convenience.
packages/bruno-electron/src/utils/proxy-util.js (3)
339-341: LGTM—keepAlive for HTTP proxy agents.The
httpProxyAgentOptionswithkeepAlive: trueimproves connection reuse for HTTP proxy agents.
380-391: System proxy protocol detection looks correct.The logic correctly routes
http_proxyto non-HTTPS requests (Line 387:!isHttpsRequest) andhttps_proxyto HTTPS requests (Line 396), aligning with standard proxy conventions. URL validation withnew URL()is appropriate.
396-406: HTTPS proxy setup properly uses PatchedHttpsProxyAgent.The conditional application of
https_proxyonly for HTTPS requests (Line 396:isHttpsRequest) and the use ofPatchedHttpsProxyAgentwithtlsOptionsensure TLS settings are correctly propagated. The fallback to a regular HTTPS agent (lines 403-406) is appropriate.
|
|
||
| ipcMain.handle('renderer:get-system-proxy-variables', async () => { | ||
| const systemProxyConfig = await getSystemProxy(); | ||
| return systemProxyConfig; | ||
| }); |
There was a problem hiding this comment.
Add error handling to prevent unhandled rejection.
If getSystemProxy() throws (e.g., detection fails on all platforms), the renderer receives an unhandled promise rejection. Wrap in try/catch to return a graceful fallback or propagate a meaningful error.
ipcMain.handle('renderer:get-system-proxy-variables', async () => {
- const systemProxyConfig = await getSystemProxy();
- return systemProxyConfig;
+ try {
+ const systemProxyConfig = await getSystemProxy();
+ return systemProxyConfig;
+ } catch (error) {
+ console.error('Failed to get system proxy:', error);
+ return {
+ http_proxy: null,
+ https_proxy: null,
+ no_proxy: null,
+ source: 'environment'
+ };
+ }
});🤖 Prompt for AI Agents
In packages/bruno-electron/src/ipc/preferences.js around lines 51 to 55, the
ipcMain handler calls getSystemProxy() without error handling which can produce
an unhandled rejection; wrap the await call in a try/catch, log or process the
error inside the catch, and then either return a safe fallback (e.g., null or an
empty config) or throw a meaningful Error that the renderer can handle so the
promise always resolves or rejects with a controlled value and no unhandled
rejection occurs.
| } catch (error) { | ||
| throw new Error('Invalid system http_proxy'); | ||
| throw new Error(`Invalid system http_proxy "${http_proxy}": ${error.message}`); | ||
| } |
There was a problem hiding this comment.
Redact credentials from proxy URIs in error messages.
Including the raw proxy URI values (http_proxy, https_proxy) in error messages risks leaking credentials if they are embedded in the URI format http://user:[email protected]. Error messages may appear in logs or user-facing dialogs.
Redact the userinfo before interpolating:
+ const redactedHttpProxy = http_proxy ? http_proxy.replace(/\/\/([^:]+):([^@]+)@/, '//$1:***@') : http_proxy;
} catch (error) {
- throw new Error(`Invalid system http_proxy "${http_proxy}": ${error.message}`);
+ throw new Error(`Invalid system http_proxy "${redactedHttpProxy}": ${error.message}`);
}Apply the same pattern for https_proxy at Line 408:
+ const redactedHttpsProxy = https_proxy ? https_proxy.replace(/\/\/([^:]+):([^@]+)@/, '//$1:***@') : https_proxy;
} catch (error) {
- throw new Error(`Invalid system https_proxy "${https_proxy}": ${error.message}`);
+ throw new Error(`Invalid system https_proxy "${redactedHttpsProxy}": ${error.message}`);
}Also applies to: 407-409
🤖 Prompt for AI Agents
In packages/bruno-electron/src/utils/proxy-util.js around lines 392-394 and
407-409, the code interpolates raw proxy URIs into error messages which can leak
credentials; parse each proxy string with the URL constructor (or equivalent),
detect username/password in the URL, replace the userinfo with a redaction (e.g.
remove username/password or set them to "[REDACTED]"), then interpolate the
sanitized URL into the thrown Error (and keep including error.message); if URL
parsing fails, fall back to a safe placeholder like "<invalid-proxy>" so no raw
credentials are logged.
| private async detectSystemProxy(): Promise<ProxyConfiguration> { | ||
| const startTime = Date.now(); | ||
|
|
||
| try { | ||
| const result = await this.detectWithTimeout(); | ||
|
|
||
| // Log slow detections | ||
| const detectionTime = Date.now() - startTime; | ||
| if (detectionTime > 5000) { | ||
| console.warn(`System proxy detection took ${detectionTime}ms`); | ||
| } | ||
|
|
||
| return result; | ||
| } catch (error) { | ||
| console.warn(`System proxy detection failed after ${Date.now() - startTime}ms:`, error instanceof Error ? error.message : String(error)); | ||
| return this.getEnvironmentVariables(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fallback might hide detection failures.
The catch block silently falls back to environment variables, which could mask code bugs or configuration issues in platform-specific detectors. Consider logging the error type or adding telemetry to track detection failures.
That said, the fallback behavior may be intentional for resilience. If so, document the design choice.
🤖 Prompt for AI Agents
In packages/bruno-requests/src/network/system-proxy/index.ts around lines 35–52,
the catch block quietly falls back to environment variables which can hide
detector failures; update the catch to log the error type and details (e.g.,
error.name and error.message and/or stack) and emit a telemetry/event hook (or
call a provided metrics reporter) with the failure reason and duration so
failures are observable, and add a short inline comment or JSDoc above
detectSystemProxy explaining that the fallback is intentional for resilience (or
change behavior if it should instead surface errors).
| private async detectWithTimeout(): Promise<ProxyConfiguration> { | ||
| return await this.detectByPlatform(); | ||
| } |
There was a problem hiding this comment.
Misleading method name - no timeout logic implemented.
detectWithTimeout doesn't implement any timeout handling; it simply calls detectByPlatform. The timeout is actually handled by individual platform resolvers. Either implement a proper timeout wrapper here or remove this unnecessary indirection.
Consider:
-private async detectWithTimeout(): Promise<ProxyConfiguration> {
- return await this.detectByPlatform();
-}
-
private async detectSystemProxy(): Promise<ProxyConfiguration> {
const startTime = Date.now();
try {
- const result = await this.detectWithTimeout();
+ const result = await this.detectByPlatform();Or implement actual timeout logic:
private async detectWithTimeout(): Promise<ProxyConfiguration> {
return Promise.race([
this.detectByPlatform(),
new Promise<ProxyConfiguration>((_, reject) =>
setTimeout(() => reject(new Error('Detection timeout')), this.commandTimeout)
)
]);
}🤖 Prompt for AI Agents
In packages/bruno-requests/src/network/system-proxy/index.ts around lines 54-56,
the method detectWithTimeout is misleading because it declares a timeout
behavior but only calls detectByPlatform; replace the indirection with a real
timeout wrapper: return a Promise.race between this.detectByPlatform() and a
timeout Promise that rejects with a clear Error (e.g., "Detection timeout")
after this.commandTimeout milliseconds, making sure types match
(Promise<ProxyConfiguration>) and the timeout Promise rejects so callers receive
the failure; alternatively, if you prefer not to implement timeout here, remove
this method and call detectByPlatform() directly where used to avoid confusion.
system proxy detection logic updatessystem proxy resolver logic updates
2 participants
jira
Description
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.