EPMRPP-109127 || Manage proxy settings directly - #235
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a proxy helper module and integrates proxy-aware agents into RestClient and OAuthInterceptor (token requests), extends tests for proxy/no-proxy behavior, updates types and README, and adds proxy runtime dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant RestClient
participant OAuth as OAuthInterceptor
participant ProxyHelper
participant Axios
participant TokenEndpoint
Client->>RestClient: request(url, restClientConfig)
RestClient->>ProxyHelper: getProxyAgentForUrl(url, restClientConfig)
alt proxy applies
ProxyHelper->>ProxyHelper: createProxyAgents -> Http/HttpsProxyAgent
else bypass or no proxy
ProxyHelper->>ProxyHelper: createProxyAgents -> default Agent
end
ProxyHelper-->>RestClient: agent(s)
RestClient->>OAuth: ensure token (passes restClientConfig)
OAuth->>ProxyHelper: getProxyAgentForUrl(tokenUrl, restClientConfig)
ProxyHelper-->>OAuth: tokenAgent
OAuth->>Axios: post(tokenUrl, body, { httpsAgent: tokenAgent, headers })
Axios->>TokenEndpoint: network request (via proxy if agent is proxy)
TokenEndpoint-->>Axios: token response
Axios-->>OAuth: token
OAuth-->>RestClient: authenticated request proceeds
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
__tests__/oauth.spec.js (2)
313-345: Excellent test coverage for proxy configuration.The test properly validates that the OAuth interceptor uses proxy agents for token requests when proxy configuration is provided.
Remove the unused
paramsvariable at line 338:- const [url, params, config] = axios.post.mock.calls[0]; + const [url, , config] = axios.post.mock.calls[0];As per static analysis hints.
347-382: Excellent test coverage for noProxy bypass.The test properly validates that requests bypass the proxy when the target URL matches the
noProxyconfiguration, using a default agent instead of a proxy agent.Remove the unused
paramsvariable at line 373:- const [url, params, config] = axios.post.mock.calls[0]; + const [url, , config] = axios.post.mock.calls[0];As per static analysis hints.
lib/rest.js (1)
43-43: Proxy integration is correctly implemented; consider adding error handling for URL parsing.The implementation properly passes
restClientConfigtoOAuthInterceptor, computes proxy agents per-request, and spreads them into axios options.However, both
getProxyConfig(line 60 in proxyHelper.js) andcreateProxyAgents(line 125) callnew URL(url)without error handling. If an invalid URL reaches these functions, a TypeError will bubble up. Consider wrapping the URL parsing in a try-catch to handle malformed URLs gracefully, especially at the call site in rest.js line 67.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
__tests__/oauth.spec.js(3 hunks)__tests__/proxyHelper.spec.js(1 hunks)lib/oauth.js(4 hunks)lib/proxyHelper.js(1 hunks)lib/rest.js(4 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
lib/rest.js (4)
__tests__/proxyHelper.spec.js (3)
require(1-1)require(2-2)require(3-8)lib/oauth.js (1)
require(2-2)lib/proxyHelper.js (3)
require(1-1)require(2-2)require(3-3)__tests__/rest.spec.js (1)
options(8-20)
lib/oauth.js (2)
lib/proxyHelper.js (3)
require(1-1)require(2-2)require(3-3)lib/rest.js (1)
require(7-7)
lib/proxyHelper.js (2)
lib/oauth.js (1)
require(2-2)lib/rest.js (3)
require(7-7)http(3-3)https(4-4)
__tests__/proxyHelper.spec.js (3)
lib/oauth.js (1)
require(2-2)lib/proxyHelper.js (4)
require(1-1)require(2-2)require(3-3)noProxy(65-65)lib/rest.js (1)
require(7-7)
__tests__/oauth.spec.js (3)
lib/oauth.js (2)
require(2-2)axios(1-1)lib/proxyHelper.js (3)
require(1-1)require(2-2)require(3-3)lib/rest.js (3)
require(7-7)OAuthInterceptor(6-6)axios(1-1)
🪛 GitHub Actions: CI-pipeline
__tests__/oauth.spec.js
[error] 3-3: https-proxy-agent import should occur before import of ../lib/oauth import/order
🪛 GitHub Check: test (16)
__tests__/oauth.spec.js
[failure] 3-3:
https-proxy-agent import should occur before import of ../lib/oauth
[warning] 373-373:
'params' is assigned a value but never used
[warning] 338-338:
'params' is assigned a value but never used
🪛 GitHub Check: test (18)
__tests__/oauth.spec.js
[failure] 3-3:
https-proxy-agent import should occur before import of ../lib/oauth
[warning] 373-373:
'params' is assigned a value but never used
[warning] 338-338:
'params' is assigned a value but never used
🪛 GitHub Check: test (20)
__tests__/oauth.spec.js
[failure] 3-3:
https-proxy-agent import should occur before import of ../lib/oauth
[warning] 373-373:
'params' is assigned a value but never used
[warning] 338-338:
'params' is assigned a value but never used
🪛 GitHub Check: test (22)
__tests__/oauth.spec.js
[failure] 3-3:
https-proxy-agent import should occur before import of ../lib/oauth
[warning] 373-373:
'params' is assigned a value but never used
[warning] 338-338:
'params' is assigned a value but never used
🔇 Additional comments (11)
package.json (1)
31-34: LGTM! Proxy dependencies are appropriate.The addition of
http-proxy-agent,https-proxy-agent, andproxy-from-envprovides the necessary infrastructure for centralized proxy handling. These are standard, well-maintained libraries with appropriate version constraints.lib/rest.js (2)
7-7: LGTM! Clean integration of proxy helper.The import of
getProxyAgentForUrlproperly integrates the centralized proxy handling.
20-20: LGTM! Proper configuration key handling.Adding
'proxy'and'noProxy'toSKIPPED_REST_CONFIG_KEYSis correct since these are handled per-request throughgetProxyAgentForUrlrather than as global axios configuration.lib/oauth.js (3)
2-2: LGTM! Clean proxy support initialization.The integration properly:
- Imports the proxy helper function
- Documents the
restClientConfigparameter in JSDoc- Provides a sensible default empty object
Also applies to: 21-21, 31-31
162-162: LGTM! Clean proxy agent application.The proxy agents are properly merged into the axios request configuration using the spread operator.
151-157: Proxy credential exposure is well-mitigated; confirm this aligns with your security policy.The code correctly guards all proxy URL logging with
if (restClientConfig.debug)checks. Proxy credentials will only appear in logs if the developer explicitly enablesrestClientConfig.debug = true—settingdebug: trueat the OAuth level does not automatically expose proxy credentials. This is a secure design choice.Verify this explicit opt-in approach meets your organization's security requirements.
__tests__/oauth.spec.js (1)
51-52: LGTM! Test assertions updated for proxy support.The assertions now correctly verify:
- The complete headers object including
Content-Type- Presence of
httpsAgent(now includes default agent even without explicit proxy)__tests__/proxyHelper.spec.js (1)
1-277: LGTM! Comprehensive test coverage for proxy helper.The test suite provides excellent coverage:
shouldBypassProxy: wildcards, subdomains, exact matches, case-insensitivity, multiple entries, invalid URLsgetProxyConfig: config vs environment precedence, noProxy handling, various proxy formats with/without authcreateProxyAgents: appropriate agent creation for HTTP/HTTPS, noProxy bypassgetProxyAgentForUrl: integration scenarios with environment variablesProper environment isolation with
beforeEach/afterAllensures test independence.lib/proxyHelper.js (3)
13-50: LGTM! Robust proxy bypass logic.The
shouldBypassProxyfunction correctly handles:
- Wildcard
*to bypass all- Leading dot patterns (
.example.com) for subdomain-only matching- Domain suffix matching for both exact and subdomain matches
- Case-insensitive comparison
- Graceful handling of invalid URLs
124-166: LGTM! Well-designed agent creation logic.The function correctly:
- Returns default agents when no proxy is configured or URL is bypassed, preventing axios from falling back to environment variables
- Creates
HttpsProxyAgentfor HTTPS URLs andHttpProxyAgentfor HTTP URLs- Includes helpful debug logging
The comment at lines 134-135 clearly explains the important behavior of returning default agents to ensure
noProxyentries truly bypass the proxy.
175-184: LGTM! Clean module interface.The wrapper function and exports provide a clear, well-organized public API for the proxy helper functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (2)
241-241: Clarify hostname matching behavior.The description states "Exact hostname:
example.com- matchesexample.comandsub.example.com", which seems contradictory. If it's "exact" matching but also matches subdomains, consider rephrasing for clarity. Perhaps: "Domain:example.com- matchesexample.comand all subdomains" or clarify that this is the expected behavior.
299-309: Add language identifier to fenced code block.The debug output example at line 299 should have a language identifier for consistency and proper rendering. Consider using
textorlog.-``` +```text [ProxyHelper] getProxyConfig called: URL: https://login.microsoftonline.com/oauth2/v2.0/tokenindex.d.ts (1)
32-55: Consider stronger typing for protocol field.The
protocolfield is currently typed asstring, but based on the README documentation (line 187), only'http'or'https'are valid values. Consider using a union type for better type safety:export interface ProxyConfig { /** * Protocol for the proxy (http or https). */ - protocol?: string; + protocol?: 'http' | 'https'; /** * Proxy host.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)__tests__/proxyHelper.spec.js(1 hunks)index.d.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/proxyHelper.spec.js
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~315-~315: To form a complete sentence, be sure to include a subject.
Context: ...ring | object | Proxy configuration. Can befalse` (disable), URL string, or co...
(MISSING_IT_THERE)
[grammar] ~326-~326: Ensure spelling is correct
Context: ...xy configuration based on the target URL 2. noProxy checking: URLs matching noProxy pat...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
299-299: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
README.md (2)
122-122: Documentation enhancement looks good.The updated description clearly explains
restClientConfigcapabilities including the new proxy support, with a helpful link to the detailed proxy configuration section.
169-331: Excellent proxy documentation.This comprehensive proxy configuration section covers all essential scenarios with clear examples:
- Multiple configuration methods (object, string, environment variables)
- noProxy bypass patterns
- OAuth integration
- Advanced scenarios and debugging
The documentation will be very helpful for users implementing proxy support.
index.d.ts (3)
57-99: Well-designed RestClientConfig interface.The interface comprehensively covers all proxy and REST client configuration options:
proxyunion type (false | string | ProxyConfig) supports all documented configuration methodsnoProxyfor bypass patternsretryconfiguration with flexible typing- Index signature allows additional axios options
- All fields appropriately optional
The types align well with the documentation and provide good developer experience.
128-144: Helpful proxy configuration example.The JSDoc example clearly demonstrates how to use the new proxy configuration types in practice, including both the proxy object structure and noProxy bypass patterns. This will help developers understand the API quickly.
156-156: Excellent type safety improvement.Changing
restClientConfigfromRecord<string, unknown>to the strongly-typedRestClientConfiginterface significantly improves the developer experience by:
- Providing IntelliSense for proxy, noProxy, timeout, retry, and other options
- Enabling compile-time type checking
- Making the API more discoverable
- Maintaining backward compatibility through the index signature
This is a great enhancement to the public API.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
__tests__/oauth.spec.js (1)
2-3: Fix import order violation.External package imports must come before internal module imports per the project's linting rules.
Apply this diff:
const axios = require('axios'); -const { HttpsProxyAgent } = require('https-proxy-agent'); const OAuthInterceptor = require('../lib/oauth'); +const { HttpsProxyAgent } = require('https-proxy-agent');As per coding guidelines (ESLint
import/orderrule).
🧹 Nitpick comments (2)
lib/oauth.js (1)
151-170: Consider simplifying the agent precedence logic.The current implementation is correct but slightly convoluted. Custom agents take precedence, but the logic could be more straightforward.
Consider this alternative for better clarity:
- // Get proxy agent for token endpoint - // Only apply if custom agents are not explicitly provided - const hasCustomAgents = this.restClientConfig.httpsAgent || this.restClientConfig.httpAgent; - const proxyAgents = hasCustomAgents - ? {} - : getProxyAgentForUrl(this.tokenEndpoint, this.restClientConfig); - - if (this.debug && Object.keys(proxyAgents).length > 0) { - this.logDebug(`Making token request to ${this.tokenEndpoint} with proxy agent`); - } - const response = await axios.post(this.tokenEndpoint, params, { headers: { 'Content-Type': 'application/x-www-form-urlencoded', }, - ...proxyAgents, - // Custom agents from restClientConfig (if provided) take precedence - ...(this.restClientConfig.httpsAgent && { httpsAgent: this.restClientConfig.httpsAgent }), - ...(this.restClientConfig.httpAgent && { httpAgent: this.restClientConfig.httpAgent }), + // Custom agents take precedence over proxy-derived agents + ...(this.restClientConfig.httpsAgent || this.restClientConfig.httpAgent + ? { + ...(this.restClientConfig.httpsAgent && { httpsAgent: this.restClientConfig.httpsAgent }), + ...(this.restClientConfig.httpAgent && { httpAgent: this.restClientConfig.httpAgent }), + } + : getProxyAgentForUrl(this.tokenEndpoint, this.restClientConfig)), });However, the current implementation is functional and may be preferred for readability.
lib/rest.js (1)
67-79: LGTM, but consider adding a clarifying comment.The proxy agent precedence logic is correct but intricate:
- Custom agents from
restClientConfig.httpsAgent/httpAgentprevent proxy agents- Per-request
optionscan override both proxy and custom agentsConsider adding a comment to clarify the precedence:
request(method, url, data, options = {}) { // Only apply proxy agents if custom agents are not explicitly provided - // Priority: explicit httpsAgent/httpAgent > proxy config > default + // Precedence: per-request options > proxy agents > instance-level custom agents const hasCustomAgents = 'httpsAgent' in this.restClientConfig || 'httpAgent' in this.restClientConfig; const proxyAgents = hasCustomAgents ? {} : getProxyAgentForUrl(url, this.restClientConfig);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
__tests__/oauth.spec.js(4 hunks)lib/oauth.js(4 hunks)lib/rest.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
lib/oauth.js (2)
lib/rest.js (1)
require(7-7)lib/proxyHelper.js (3)
require(1-1)require(2-2)require(3-3)
lib/rest.js (4)
lib/oauth.js (1)
require(2-2)lib/proxyHelper.js (3)
require(1-1)require(2-2)require(3-3)__tests__/proxyHelper.spec.js (3)
require(1-1)require(2-2)require(3-8)__tests__/rest.spec.js (1)
options(8-20)
__tests__/oauth.spec.js (2)
lib/oauth.js (1)
require(2-2)lib/proxyHelper.js (3)
require(1-1)require(2-2)require(3-3)
🪛 GitHub Check: test (14)
__tests__/oauth.spec.js
[warning] 376-376:
'params' is assigned a value but never used
[warning] 341-341:
'params' is assigned a value but never used
🪛 GitHub Check: test (16)
__tests__/oauth.spec.js
[warning] 376-376:
'params' is assigned a value but never used
[warning] 341-341:
'params' is assigned a value but never used
🪛 GitHub Check: test (18)
__tests__/oauth.spec.js
[warning] 376-376:
'params' is assigned a value but never used
[warning] 341-341:
'params' is assigned a value but never used
🪛 GitHub Check: test (20)
__tests__/oauth.spec.js
[warning] 376-376:
'params' is assigned a value but never used
[warning] 341-341:
'params' is assigned a value but never used
🪛 GitHub Check: test (22)
__tests__/oauth.spec.js
[warning] 376-376:
'params' is assigned a value but never used
[warning] 341-341:
'params' is assigned a value but never used
🔇 Additional comments (6)
lib/oauth.js (1)
2-2: LGTM!Import is correctly added to support proxy handling in token requests.
lib/rest.js (3)
7-7: LGTM!Import correctly added for proxy support in REST requests.
20-20: LGTM!Correctly excludes
proxyandnoProxyfrom being passed directly to axios, as they're transformed into agents viagetProxyAgentForUrl.
40-44: Verify debug flag is respected.The RestClient passes
debug: this.debugat line 42, but the OAuthInterceptor constructor (in oauth.js line 31) only readsconfig.restClientConfig.debug. This means the top-leveldebugflag won't be used by the OAuth interceptor.This is addressed in the review comment on oauth.js lines 30-31.
__tests__/oauth.spec.js (2)
51-52: LGTM!Test correctly verifies that an
httpsAgentis attached to token requests, aligning with the new proxy-aware behavior.
155-158: Add test coverage for top-level debug flag.The test now validates
restClientConfig.debug, but doesn't verify that top-levelconfig.debugstill works. Given that the constructor documentation (oauth.js line 20) and RestClient (rest.js line 42) both passdebugat the top level, there should be test coverage for both approaches.Consider adding a test case:
it('logs debug messages when debug is passed at top level', () => { const consoleSpy = jest.spyOn(console, 'log').mockImplementation(); const oauthInterceptor = new OAuthInterceptor({ ...baseConfig, debug: true, }); oauthInterceptor.logDebug('message', { foo: 'bar' }); expect(consoleSpy).toHaveBeenCalledWith('[OAuth] message', { foo: 'bar' }); consoleSpy.mockRestore(); });This would catch the issue identified in oauth.js lines 30-31.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Type Definitions
Chores