feat: add certs and proxy config for bruno-cli oauth2 requests#6423
feat: add certs and proxy config for bruno-cli oauth2 requests#6423bijin-bruno merged 1 commit intousebruno:mainfrom
certs and proxy config for bruno-cli oauth2 requests#6423Conversation
WalkthroughAdds recursive object interpolation and expands interpolation sources (collection, folder, request, runtime, process env). Introduces a TLS/proxy-aware HTTP(S) agent factory and wires custom axios instances into OAuth2 token retrieval. Also adds proxy-agent deps and a new npm script for the bruno-requests workspace. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Runner
participant Interp as Interpolation Engine
participant Agent as HTTP Agent Factory
participant OAuth2 as OAuth2 Handler
participant Token as Token Endpoint
CLI->>Interp: buildCombinedVars(collection, folder, request, runtime, processEnv)
Interp-->>CLI: combinedVars
CLI->>Interp: interpolateString(accessTokenUrl, combinedVars)
Interp-->>CLI: resolvedAccessTokenUrl
CLI->>Agent: getHttpHttpsAgents(resolvedAccessTokenUrl, collectionPath, clientCerts, proxyConfig)
Agent->>Agent: resolve proxy mode, load certs, aggregate CAs
Agent-->>CLI: { httpAgent, httpsAgent }
CLI->>OAuth2: getOAuth2Token(oauth2Config, axiosInstanceWithAgents)
OAuth2->>Token: POST token request (via custom axios instance)
Token-->>OAuth2: token response
OAuth2-->>CLI: token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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 |
certs and proxy config for bruno-cli oauth2 requests
| /** | ||
| * recursively interpolating all string values in a object | ||
| */ | ||
| const interpolateObject = (obj, interpolationOptions) => { |
There was a problem hiding this comment.
Can be moved to common
6b3d6ee to
0345a99
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/bruno-cli/src/runner/run-single-request.js`:
- Around line 489-496: The call to getHttpHttpsAgents uses the wrong parameter
name: it passes globalLevelProxy but the function expects systemProxyConfig;
update the argument key in the call that constructs the agents (the object
passed to getHttpHttpsAgents near oauth2RequestUrl, collectionPath,
options/tlsOptions, clientCertificates/interpolatedClientCertificates, and
collectionLevelProxy/interpolatedProxyConfig) to use systemProxyConfig:
getSystemProxyEnvVariables() so the system proxy configuration is actually
forwarded for OAuth2 requests.
In `@packages/bruno-common/src/interpolate/index.ts`:
- Around line 132-156: The current interpolateObject implementation uses a
persistent WeakSet named seen inside walk which falsely treats shared but
non-cyclic references as cycles; update the traversal to use a recursion stack
(or add/remove from seen around recursion) so you mark an object as visited when
entering (seen.add or stack.push) and remove it when leaving (seen.delete or
stack.pop), e.g. wrap recursive visits in a try/finally so Array.isArray and
isPlainObject branches call walk on children while ensuring the mark is removed
after mapping; keep the circular-detection error thrown only when the object is
already on the current recursion stack inside walk.
In `@packages/bruno-requests/src/utils/http-https-agents.ts`:
- Around line 295-303: The branch handling collectionProxyEnabled === 'global'
never assigns the system proxy, so proxyConfig stays empty and proxyMode always
falls back to 'off'; update that branch to assign proxyConfig =
systemProxyConfig || {} (using the existing systemProxyConfig param) and then
set proxyMode = get(proxyConfig, 'mode', 'off') as ProxyMode so the
global/system proxy settings are actually applied (refer to
collectionProxyConfig, collectionProxyEnabled, proxyConfig, proxyMode, and
systemProxyConfig).
- Around line 316-325: The TLS options object tlsOptions currently sets
minVersion: 'TLSv1', which permits deprecated TLS 1.0; update the minVersion in
the TlsOptions initialization (where tlsOptions is composed from
httpsAgentRequestFields and certsConfig) to 'TLSv1.2' (or higher) to enforce
modern TLS (preferably TLSv1.3 when available) while keeping the other fields
(secureProtocol, rejectUnauthorized) unchanged; ensure this change is made in
the tlsOptions definition referenced in http-https-agents.ts so the created
agents use the stricter minimum version.
♻️ Duplicate comments (1)
packages/bruno-cli/src/runner/interpolate-string.js (1)
57-63: Acknowledged: duplication with other packages.This wrapper provides CLI-specific variable assembly before calling the common utility. The pattern is consistent with similar wrappers in
bruno-electronandbruno-js.
| // Ensure TLS options are properly set | ||
| const tlsOptions: TlsOptions = { | ||
| ...httpsAgentRequestFields, | ||
| ...certsConfig, | ||
| // Enable all secure protocols by default | ||
| secureProtocol: undefined, | ||
| // Allow Node.js to choose the protocol | ||
| minVersion: 'TLSv1', | ||
| rejectUnauthorized: httpsAgentRequestFields.rejectUnauthorized !== undefined ? httpsAgentRequestFields.rejectUnauthorized : true | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js recommended minimum TLS version HTTPS agent 2025 best practices
💡 Result:
Short answer
- Minimum: don’t accept TLS < 1.2; require at least TLS 1.2. Prefer TLS 1.3 wherever possible. [1][2][3]
Why
- Node.js defaults to TLSv1.2 minimum and documents that versions < TLS1.2 are not recommended. You can override per-connection with minVersion/maxVersion. [1]
- Industry guidance (OWASP, Mozilla, ASVS) says prefer TLS 1.3 and support TLS 1.2 only for interoperability; disable older protocol versions and weak ciphers. [2][3]
How to enforce in Node.js (HTTPS agent)
-
Create an https.Agent with minVersion (and optionally maxVersion) and a strong cipher/curve policy. Example:
const agent = new https.Agent({ minVersion: 'TLSv1.2', maxVersion: 'TLSv1.3' }); -
Also harden cipher suites / curves, keep OpenSSL/Node updated, enable TLS_FALLBACK_SCSV/OCSP stapling / HSTS, and monitor client compatibility. [1][2][3]
Sources
- Node.js TLS docs (minVersion/defaults). [1]
- OWASP Transport Layer Security cheat sheet / ASVS guidance. [2]
- Mozilla Server-Side TLS recommendations (prefer TLS 1.3, allow TLS 1.2 for compatibility). [3]
🏁 Script executed:
# Check the context and purpose of this file
head -50 packages/bruno-requests/src/utils/http-https-agents.ts
# Also check if there are any comments about legacy support
grep -n "legacy\|TLS\|version\|deprecated" packages/bruno-requests/src/utils/http-https-agents.tsRepository: usebruno/bruno
Length of output: 1437
Security concern: TLS 1.0 is deprecated and below industry standards.
Line 323 sets minVersion: 'TLSv1', allowing TLS 1.0 connections. Current industry guidance (Node.js, OWASP, Mozilla) recommends TLS 1.2 as the minimum, with TLS 1.3 preferred. Node.js documentation explicitly states that versions below TLS 1.2 are not recommended.
Change to minVersion: 'TLSv1.2' to meet current security standards.
🤖 Prompt for AI Agents
In `@packages/bruno-requests/src/utils/http-https-agents.ts` around lines 316 -
325, The TLS options object tlsOptions currently sets minVersion: 'TLSv1', which
permits deprecated TLS 1.0; update the minVersion in the TlsOptions
initialization (where tlsOptions is composed from httpsAgentRequestFields and
certsConfig) to 'TLSv1.2' (or higher) to enforce modern TLS (preferably TLSv1.3
when available) while keeping the other fields (secureProtocol,
rejectUnauthorized) unchanged; ensure this change is made in the tlsOptions
definition referenced in http-https-agents.ts so the created agents use the
stricter minimum version.
787c814 to
0aab3a9
Compare
0aab3a9 to
e61e057
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/interpolate-string.js (1)
4-46: AddglobalEnvironmentVariablestobuildCombinedVarsparameters.
packages/bruno-jsandpackages/bruno-electronboth includeglobalEnvironmentVariablesin their variable precedence order, but the CLI implementation omits it. This inconsistency could cause interpolation behavior to differ between the desktop app and CLI runner. UpdatebuildCombinedVarsto accept and mergeglobalEnvironmentVariablesas the lowest-precedence source, matching the other implementations.
♻️ Duplicate comments (1)
packages/bruno-requests/src/utils/http-https-agents.ts (1)
326-334: Raise minimum TLS version to TLSv1.2+.Line 332 sets
minVersion: 'TLSv1', which permits deprecated TLS 1.0. Please raise it to TLS 1.2 or higher.🔒 Proposed fix
- minVersion: 'TLSv1', + minVersion: 'TLSv1.2',Node.js https.Agent minVersion recommended minimum TLS version (TLS 1.2+ guidance)
🧹 Nitpick comments (1)
packages/bruno-cli/src/runner/interpolate-string.js (1)
57-63: Consider expanding the JSDoc.The comment describes intent but omits parameter and return type documentation.
📝 Suggested JSDoc improvement
/** - * recursively interpolating all string values in a object + * Recursively interpolates all string values in an object. + * `@param` {unknown} obj - The object to interpolate + * `@param` {Object} interpolationOptions - Variable sources for interpolation + * `@returns` {unknown} - The interpolated object */ const interpolateObject = (obj, interpolationOptions) => {
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
✏️ Tip: You can customize this high-level summary in your review settings.