fix: honor OS-level PAC configuration in system proxy mode#7766
Draft
pooja-bruno wants to merge 1 commit intousebruno:mainfrom
Draft
fix: honor OS-level PAC configuration in system proxy mode#7766pooja-bruno wants to merge 1 commit intousebruno:mainfrom
pooja-bruno wants to merge 1 commit intousebruno:mainfrom
Conversation
pooja-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughThis PR adds PAC (Proxy Auto-Config) URL support across the proxy system. It extends platform-specific proxy detectors to extract PAC URLs, updates type definitions to include Changes
Sequence Diagram(s)sequenceDiagram
actor Client as Request Handler
participant Detector as Platform Detector<br/>(Windows/macOS/Linux)
participant Resolver as PAC Resolver<br/>(resolveAgentsFromPac)
participant Agent as HTTP/HTTPS Agent
participant Target as Target Server
Client->>Detector: Detect system proxy config
Detector->>Detector: Extract pac_url from OS
Detector-->>Client: Return ProxyConfiguration{pac_url}
Client->>Resolver: Check proxyMode='system' with pac_url
alt PAC URL Present
Resolver->>Resolver: Resolve PAC directives for request URL
Resolver->>Resolver: Parse first directive (PROXY/SOCKS/DIRECT)
Resolver->>Agent: Set httpAgent or httpsAgent based on directive
Resolver-->>Client: Return agents
else No PAC URL
Client->>Client: Fall back to http_proxy/https_proxy logic
end
Client->>Agent: Use configured agent
Agent->>Target: Route request (direct or via proxy)
Target-->>Agent: Response
Agent-->>Client: Return response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/bruno-electron/src/utils/proxy-util.js (1)
107-134: Inconsistent agent assignment compared tohttp-https-agents.ts.This helper sets both
httpAgentandhttpsAgentregardless of request protocol, while the equivalentresolveAgentsFromPacinhttp-https-agents.ts(lines 424-437) selectively sets only the needed agent based onisHttpsRequest. This could cause unnecessary agent creation.Consider aligning the behavior by passing
isHttpsRequestand conditionally setting only the required agent.♻️ Suggested approach
-async function resolveAgentsFromPac({ pacSource, requestUrl, requestConfig, tlsOptions, httpsAgentRequestFields, timeline, disableCache, hostname }) { +async function resolveAgentsFromPac({ pacSource, requestUrl, requestConfig, tlsOptions, httpsAgentRequestFields, timeline, disableCache, hostname, isHttpsRequest }) { const resolver = await getPacResolver({ pacSource, httpsAgentRequestFields }); const directives = await resolver.resolve(requestUrl); if (!directives || !directives.length) { return null; } const first = directives[0]; if (/^(PROXY|HTTPS?)\s+/i.test(first)) { const parts = first.split(/\s+/); const keyword = parts[0].toUpperCase(); const hostPort = parts[1]; const scheme = keyword === 'HTTPS' ? 'https' : 'http'; const proxyUri = `${scheme}://${hostPort}`; - requestConfig.httpAgent = getOrCreateHttpAgent({ AgentClass: HttpProxyAgent, options: { keepAlive: true }, proxyUri, timeline, disableCache, hostname }); - requestConfig.httpsAgent = getOrCreateHttpsAgent({ AgentClass: PatchedHttpsProxyAgent, options: tlsOptions, proxyUri, timeline, disableCache, hostname }); + if (isHttpsRequest) { + requestConfig.httpsAgent = getOrCreateHttpsAgent({ AgentClass: PatchedHttpsProxyAgent, options: tlsOptions, proxyUri, timeline, disableCache, hostname }); + } else { + requestConfig.httpAgent = getOrCreateHttpAgent({ AgentClass: HttpProxyAgent, options: { keepAlive: true }, proxyUri, timeline, disableCache, hostname }); + } } else if (/^SOCKS/i.test(first)) { // ... similar change for SOCKS } return directives; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/utils/proxy-util.js` around lines 107 - 134, The resolveAgentsFromPac function currently assigns both requestConfig.httpAgent and requestConfig.httpsAgent unconditionally, creating unnecessary agents; update resolveAgentsFromPac to accept an isHttpsRequest boolean (like in http-https-agents.ts) and only call getOrCreateHttpAgent when !isHttpsRequest and only call getOrCreateHttpsAgent/appropriate Agent when isHttpsRequest, preserving the existing proxyUri/scheme/proto logic and using the same AgentClass and options (HttpProxyAgent, PatchedHttpsProxyAgent, SocksProxyAgent) so you avoid instantiating the unused agent.packages/bruno-requests/src/network/system-proxy/utils/linux.ts (1)
90-95: Consider addingpac_url: nullto manual proxy returns for type consistency.The PAC code paths return
{ pac_url, ... }but the manual proxy returns here (and ingetKDEProxy,parseProxyFromContent) omitpac_url. This inconsistency may cause downstream code to handleundefinedvsnulldifferently.♻️ Suggested fix
return { http_proxy, https_proxy, no_proxy: normalizeNoProxy(rawNoProxy), + pac_url: null, source: 'linux-system' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-requests/src/network/system-proxy/utils/linux.ts` around lines 90 - 95, The manual proxy return objects lack a pac_url field causing type inconsistency; update the manual-return branches in the linux-system code (the return currently including http_proxy, https_proxy, no_proxy, source: 'linux-system') as well as corresponding manual-return spots in getKDEProxy and parseProxyFromContent to include pac_url: null so all proxy result objects always have a pac_url property (PAC code paths should continue to return the actual pac_url).packages/bruno-requests/src/network/system-proxy/utils/windows.ts (1)
220-226: Consider addingpac_url: nullfor consistency with the PAC code path.Same observation as the Linux file:
parseProxyStringreturns aProxyConfigurationwithoutpac_url, creating a shape inconsistency with configurations that include it.♻️ Suggested fix
return { http_proxy, https_proxy, no_proxy: bypassList && bypassList !== '(none)' ? normalizeNoProxy(bypassList) : null, + pac_url: null, source: 'windows-system' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-requests/src/network/system-proxy/utils/windows.ts` around lines 220 - 226, The returned ProxyConfiguration from the Windows system parser (the object returned in the function that uses http_proxy, https_proxy and normalizeNoProxy) is missing pac_url which causes shape inconsistency with the PAC code path; update the returned object in the Windows parser (the same place parseProxyString/normalizeNoProxy are used) to include pac_url: null so ProxyConfiguration always has pac_url present.tests/proxy/system-pac/system-pac-proxy.spec.ts (1)
25-35: Consider try/finally pattern for system state cleanup.If
startServers()succeeds but a test fails mid-execution,disableSystemPac()still runs viaafterAll. However, ifenableSystemPac()throws (e.g., permission denied), the system may be left in an inconsistent state. Consider wrappingenableSystemPaccalls with their own error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/proxy/system-pac/system-pac-proxy.spec.ts` around lines 25 - 35, The before/after hooks should ensure enableSystemPac failures can't leave the OS proxy set; wrap calls to enableSystemPac (and any system-modifying setup) in a try/finally so disableSystemPac always runs on error: modify the setup where startServers() and enableSystemPac() are invoked (referencing startServers, enableSystemPac, disableSystemPac, stopServers) to catch exceptions from enableSystemPac and call disableSystemPac in a finally block, and ensure stopServers is awaited in the cleanup path as well so servers are stopped whether setup succeeds or fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts`:
- Around line 48-49: Add a positive PAC-enabled test in macos.spec.ts that
exercises the new PAC extraction path: create a test case that stubs/mocks the
macOS system proxy output to include a PAC URL and assert that the parsed result
(the object under test that currently exposes pac_url) returns that non-null URL
(not null) and retains source: 'macos-system'; locate the spec in
packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts and add the
test alongside the existing cases that currently assert pac_url: null so it
covers the PAC-enabled branch of the parsing function that produces pac_url.
In `@tests/proxy/system-pac/system-pac-proxy.spec.ts`:
- Around line 12-20: The test hardcodes NETWORK_SERVICE = 'Wi-Fi' and calls
macOS-only networksetup without platform guarding; update the test to first skip
on non-Darwin platforms (use a platform check and test.skip) and make
NETWORK_SERVICE dynamic by detecting an active service or accepting the service
name from environment/utility instead of hardcoding; locate the NETWORK_SERVICE
constant and the helper functions enableSystemPac and disableSystemPac and
change them to either query available services (e.g., via networksetup
-listallnetworkservices) or accept a resolved service name, and ensure the
entire test is wrapped with a Darwin-only guard so networksetup is never invoked
on other OSes.
---
Nitpick comments:
In `@packages/bruno-electron/src/utils/proxy-util.js`:
- Around line 107-134: The resolveAgentsFromPac function currently assigns both
requestConfig.httpAgent and requestConfig.httpsAgent unconditionally, creating
unnecessary agents; update resolveAgentsFromPac to accept an isHttpsRequest
boolean (like in http-https-agents.ts) and only call getOrCreateHttpAgent when
!isHttpsRequest and only call getOrCreateHttpsAgent/appropriate Agent when
isHttpsRequest, preserving the existing proxyUri/scheme/proto logic and using
the same AgentClass and options (HttpProxyAgent, PatchedHttpsProxyAgent,
SocksProxyAgent) so you avoid instantiating the unused agent.
In `@packages/bruno-requests/src/network/system-proxy/utils/linux.ts`:
- Around line 90-95: The manual proxy return objects lack a pac_url field
causing type inconsistency; update the manual-return branches in the
linux-system code (the return currently including http_proxy, https_proxy,
no_proxy, source: 'linux-system') as well as corresponding manual-return spots
in getKDEProxy and parseProxyFromContent to include pac_url: null so all proxy
result objects always have a pac_url property (PAC code paths should continue to
return the actual pac_url).
In `@packages/bruno-requests/src/network/system-proxy/utils/windows.ts`:
- Around line 220-226: The returned ProxyConfiguration from the Windows system
parser (the object returned in the function that uses http_proxy, https_proxy
and normalizeNoProxy) is missing pac_url which causes shape inconsistency with
the PAC code path; update the returned object in the Windows parser (the same
place parseProxyString/normalizeNoProxy are used) to include pac_url: null so
ProxyConfiguration always has pac_url present.
In `@tests/proxy/system-pac/system-pac-proxy.spec.ts`:
- Around line 25-35: The before/after hooks should ensure enableSystemPac
failures can't leave the OS proxy set; wrap calls to enableSystemPac (and any
system-modifying setup) in a try/finally so disableSystemPac always runs on
error: modify the setup where startServers() and enableSystemPac() are invoked
(referencing startServers, enableSystemPac, disableSystemPac, stopServers) to
catch exceptions from enableSystemPac and call disableSystemPac in a finally
block, and ensure stopServers is awaited in the cleanup path as well so servers
are stopped whether setup succeeds or fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a06975e5-0a99-46af-8e95-afd91be54a47
📒 Files selected for processing (16)
packages/bruno-electron/src/ipc/network/cert-utils.jspackages/bruno-electron/src/store/system-proxy.jspackages/bruno-electron/src/utils/proxy-util.jspackages/bruno-requests/src/network/system-proxy/index.spec.jspackages/bruno-requests/src/network/system-proxy/index.tspackages/bruno-requests/src/network/system-proxy/types.tspackages/bruno-requests/src/network/system-proxy/utils/linux.tspackages/bruno-requests/src/network/system-proxy/utils/macos.spec.tspackages/bruno-requests/src/network/system-proxy/utils/macos.tspackages/bruno-requests/src/network/system-proxy/utils/windows.tspackages/bruno-requests/src/utils/http-https-agents.tstests/proxy/system-pac/fixtures/collection/bruno.jsontests/proxy/system-pac/fixtures/collection/direct.brutests/proxy/system-pac/fixtures/collection/proxied.brutests/proxy/system-pac/init-user-data/preferences.jsontests/proxy/system-pac/system-pac-proxy.spec.ts
Comment on lines
+48
to
49
| pac_url: null, | ||
| source: 'macos-system' |
Contributor
There was a problem hiding this comment.
Add one positive PAC-enabled test case.
Current updates assert pac_url: null, but the new PAC extraction path is not directly asserted with a non-null URL scenario.
Proposed test addition
describe('scutil proxy detection', () => {
+ it('should detect PAC URL when PAC is enabled', async () => {
+ const scutilOutput = `<dictionary> {
+ ProxyAutoConfigEnable : 1
+ ProxyAutoConfigURLString : http://proxy.usebruno.com/proxy.pac
+ HTTPEnable : 0
+ HTTPSEnable : 0
+}`;
+
+ mockExecFile.mockResolvedValueOnce({ stdout: scutilOutput, stderr: '' });
+
+ const result = await detector.detect();
+
+ expect(result).toEqual({
+ http_proxy: null,
+ https_proxy: null,
+ no_proxy: null,
+ pac_url: 'http://proxy.usebruno.com/proxy.pac',
+ source: 'macos-system'
+ });
+ });As per coding guidelines **/*.{test,spec}.{js,jsx,ts,tsx}: Add tests for any new functionality or meaningful changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts` around
lines 48 - 49, Add a positive PAC-enabled test in macos.spec.ts that exercises
the new PAC extraction path: create a test case that stubs/mocks the macOS
system proxy output to include a PAC URL and assert that the parsed result (the
object under test that currently exposes pac_url) returns that non-null URL (not
null) and retains source: 'macos-system'; locate the spec in
packages/bruno-requests/src/network/system-proxy/utils/macos.spec.ts and add the
test alongside the existing cases that currently assert pac_url: null so it
covers the PAC-enabled branch of the parsing function that produces pac_url.
Comment on lines
+12
to
+20
| const NETWORK_SERVICE = 'Wi-Fi'; | ||
|
|
||
| function enableSystemPac(pacUrl: string) { | ||
| execSync(`networksetup -setautoproxyurl "${NETWORK_SERVICE}" "${pacUrl}"`); | ||
| } | ||
|
|
||
| function disableSystemPac() { | ||
| execSync(`networksetup -setautoproxystate "${NETWORK_SERVICE}" off`); | ||
| } |
Contributor
There was a problem hiding this comment.
Hardcoded network service and missing platform guard.
NETWORK_SERVICE = 'Wi-Fi'won't work on macOS machines using Ethernet, Thunderbolt Bridge, or other network interfaces.- This test uses
networksetupwhich is macOS-only, but there's notest.skipfor non-Darwin platforms.
🛠️ Suggested improvements
+import * as os from 'os';
+
+// Skip on non-macOS platforms
+const isMacOS = os.platform() === 'darwin';
+
+/**
+ * Get the primary network service. Falls back to Wi-Fi if detection fails.
+ */
+function getPrimaryNetworkService(): string {
+ try {
+ // Get the device for the default route, then find its service name
+ const output = execSync('networksetup -listallnetworkservices', { encoding: 'utf8' });
+ const services = output.split('\n').filter((s) => s && !s.startsWith('*'));
+ // Prefer Wi-Fi or Ethernet
+ return services.find((s) => /wi-fi|ethernet/i.test(s)) || services[0] || 'Wi-Fi';
+ } catch {
+ return 'Wi-Fi';
+ }
+}
+
-const NETWORK_SERVICE = 'Wi-Fi';
+const NETWORK_SERVICE = isMacOS ? getPrimaryNetworkService() : 'Wi-Fi';
test.describe('System Proxy with PAC', () => {
+ test.skip(!isMacOS, 'System PAC tests require macOS networksetup');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/proxy/system-pac/system-pac-proxy.spec.ts` around lines 12 - 20, The
test hardcodes NETWORK_SERVICE = 'Wi-Fi' and calls macOS-only networksetup
without platform guarding; update the test to first skip on non-Darwin platforms
(use a platform check and test.skip) and make NETWORK_SERVICE dynamic by
detecting an active service or accepting the service name from
environment/utility instead of hardcoding; locate the NETWORK_SERVICE constant
and the helper functions enableSystemPac and disableSystemPac and change them to
either query available services (e.g., via networksetup -listallnetworkservices)
or accept a resolved service name, and ensure the entire test is wrapped with a
Darwin-only guard so networksetup is never invoked on other OSes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Bruno supports PAC proxy resolution when explicitly configured in preferences, but in System proxy mode, OS-level PAC configuration is silently ignored.
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
Tests