fix: remove proxy for api client requests & resolve localhost requests #276
Conversation
WalkthroughAdds IPv6/IPv4 probing and dynamic localhost resolution for requests targeting localhost. Introduces helpers Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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 |
| const lookup = await createLocalhostLookup(port); | ||
|
|
||
| requestConfig.httpAgent = new http.Agent({ lookup }); | ||
| requestConfig.httpsAgent = new https.Agent({ lookup }); |
There was a problem hiding this comment.
why add it to https agent? is it needed there as well?
There was a problem hiding this comment.
Not needed though, do local setups use https://localhost ?
There was a problem hiding this comment.
you are right, no harm in keeping it
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/actions/getProxiedAxios.ts`:
- Around line 97-115: The request interceptor crashes on relative URLs because
new URL(requestUrl) throws; update instance.interceptors.request.use to guard
URL parsing by using requestConfig.baseURL as the fallback (i.e., try new
URL(requestUrl, requestConfig.baseURL)) and wrap construction in a try/catch
that returns requestConfig on parse failure; only proceed to check hostname/port
and call createLocalhostLookup when URL construction succeeds, and keep setting
requestConfig.httpAgent/httpsAgent as before.
| instance.interceptors.request.use(async (requestConfig) => { | ||
| const { url: requestUrl } = requestConfig; | ||
|
|
||
| if (!requestUrl) { | ||
| return requestConfig; | ||
| } | ||
|
|
||
| const url = new URL(requestUrl); | ||
| const { hostname, port: urlPort, protocol } = url; | ||
|
|
||
| if (hostname === "localhost" || hostname === LOCAL_IPV6 || hostname === LOCAL_IPV4) { | ||
| // convert string port to integer | ||
| const port = urlPort ? parseInt(urlPort, 10) : protocol === "https:" ? 443 : 80; | ||
|
|
||
| const lookup = await createLocalhostLookup(port); | ||
|
|
||
| requestConfig.httpAgent = new http.Agent({ lookup }); | ||
| requestConfig.httpsAgent = new https.Agent({ lookup }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the file to see the context and current implementation
cat -n src/main/actions/getProxiedAxios.ts | head -130 | tail -50Repository: requestly/requestly-desktop-app
Length of output: 1847
🏁 Script executed:
node <<'EOF'
// Test URL constructor with relative paths
console.log("Test 1: Absolute URL");
try {
const url1 = new URL("https://example.com/path");
console.log("✓ Success:", url1.href);
} catch (e) {
console.log("✗ Error:", e.message);
}
console.log("\nTest 2: Relative URL without base");
try {
const url2 = new URL("/path");
console.log("✓ Success:", url2.href);
} catch (e) {
console.log("✗ Error:", e.message);
}
console.log("\nTest 3: Relative URL with base parameter");
try {
const url3 = new URL("/path", "https://example.com");
console.log("✓ Success:", url3.href);
} catch (e) {
console.log("✗ Error:", e.message);
}
console.log("\nTest 4: Simple relative path");
try {
const url4 = new URL("api/endpoint");
console.log("✓ Success:", url4.href);
} catch (e) {
console.log("✗ Error:", e.message);
}
console.log("\nTest 5: Simple relative path with base");
try {
const url5 = new URL("api/endpoint", "https://example.com");
console.log("✓ Success:", url5.href);
} catch (e) {
console.log("✗ Error:", e.message);
}
EOFRepository: requestly/requestly-desktop-app
Length of output: 400
🏁 Script executed:
# Search for axios creation with baseURL
rg "axios\.create|baseURL" --type ts --type js -A 3 -B 1Repository: requestly/requestly-desktop-app
Length of output: 703
🏁 Script executed:
# Search for usage of getProxiedAxios
rg "getProxiedAxios|createOrUpdateAxiosInstance" --type ts --type js -B 2 -A 5Repository: requestly/requestly-desktop-app
Length of output: 3612
🏁 Script executed:
# Read makeApiClientRequest to see how axios is used
cat -n src/main/actions/makeApiClientRequest.js | head -120 | tail -80Repository: requestly/requestly-desktop-app
Length of output: 2929
🏁 Script executed:
# Read the start of makeApiClientRequest to see the function signature and URL source
cat -n src/main/actions/makeApiClientRequest.js | head -40Repository: requestly/requestly-desktop-app
Length of output: 1599
Guard URL parsing against relative url values.
new URL(requestUrl) throws for relative paths, which will crash the interceptor and prevent requests from being sent. This is particularly important for applications using axios with baseURL or for handling edge cases with relative URLs. Extract baseURL from requestConfig and use it as a fallback parameter to the URL constructor, returning early if parsing fails.
🛠️ Proposed fix
- instance.interceptors.request.use(async (requestConfig) => {
- const { url: requestUrl } = requestConfig;
+ instance.interceptors.request.use(async (requestConfig) => {
+ const { url: requestUrl, baseURL } = requestConfig;
if (!requestUrl) {
return requestConfig;
}
- const url = new URL(requestUrl);
+ let url: URL;
+ try {
+ url = baseURL ? new URL(requestUrl, baseURL) : new URL(requestUrl);
+ } catch {
+ return requestConfig;
+ }
const { hostname, port: urlPort, protocol } = url;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| instance.interceptors.request.use(async (requestConfig) => { | |
| const { url: requestUrl } = requestConfig; | |
| if (!requestUrl) { | |
| return requestConfig; | |
| } | |
| const url = new URL(requestUrl); | |
| const { hostname, port: urlPort, protocol } = url; | |
| if (hostname === "localhost" || hostname === LOCAL_IPV6 || hostname === LOCAL_IPV4) { | |
| // convert string port to integer | |
| const port = urlPort ? parseInt(urlPort, 10) : protocol === "https:" ? 443 : 80; | |
| const lookup = await createLocalhostLookup(port); | |
| requestConfig.httpAgent = new http.Agent({ lookup }); | |
| requestConfig.httpsAgent = new https.Agent({ lookup }); | |
| } | |
| instance.interceptors.request.use(async (requestConfig) => { | |
| const { url: requestUrl, baseURL } = requestConfig; | |
| if (!requestUrl) { | |
| return requestConfig; | |
| } | |
| let url: URL; | |
| try { | |
| url = baseURL ? new URL(requestUrl, baseURL) : new URL(requestUrl); | |
| } catch { | |
| return requestConfig; | |
| } | |
| const { hostname, port: urlPort, protocol } = url; | |
| if (hostname === "localhost" || hostname === LOCAL_IPV6 || hostname === LOCAL_IPV4) { | |
| // convert string port to integer | |
| const port = urlPort ? parseInt(urlPort, 10) : protocol === "https:" ? 443 : 80; | |
| const lookup = await createLocalhostLookup(port); | |
| requestConfig.httpAgent = new http.Agent({ lookup }); | |
| requestConfig.httpsAgent = new https.Agent({ lookup }); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/actions/getProxiedAxios.ts` around lines 97 - 115, The request
interceptor crashes on relative URLs because new URL(requestUrl) throws; update
instance.interceptors.request.use to guard URL parsing by using
requestConfig.baseURL as the fallback (i.e., try new URL(requestUrl,
requestConfig.baseURL)) and wrap construction in a try/catch that returns
requestConfig on parse failure; only proceed to check hostname/port and call
createLocalhostLookup when URL construction succeeds, and keep setting
requestConfig.httpAgent/httpsAgent as before.
There was a problem hiding this comment.
can you explain this by some examples ?
There was a problem hiding this comment.
@Aarushsr12, sure! Let me explain with concrete examples:
The Problem
The new URL() constructor requires an absolute URL (with protocol). If you pass a relative URL, it throws an error:
// ✅ Works - absolute URL
new URL('https://api.example.com/users')
// Result: { hostname: 'api.example.com', ... }
// ❌ Crashes - relative URL
new URL('/api/users')
// Throws: TypeError: Invalid URL
// ❌ Crashes - relative path
new URL('api/users')
// Throws: TypeError: Invalid URLWhy This Matters for Axios
When using axios, you can configure requests in different ways:
// Scenario 1: Absolute URL - works fine
axios.get('https://api.example.com/users')
// Scenario 2: Using baseURL with relative path - CRASHES with current code
const client = axios.create({ baseURL: 'https://api.example.com' })
client.get('/users') // requestUrl = '/users' → new URL('/users') throws!
// Scenario 3: Just a path - CRASHES with current code
axios.get('/api/users')The Fix
By using baseURL as the second parameter to the URL constructor, we can safely parse relative URLs:
// Before (crashes):
const url = new URL('/users') // ❌ Throws
// After (works):
const url = new URL('/users', 'https://api.example.com') // ✅ Returns full URL
// The try-catch ensures any parsing errors don't break the request:
try {
url = baseURL ? new URL(requestUrl, baseURL) : new URL(requestUrl);
} catch {
return requestConfig; // Skip localhost handling, let axios handle it normally
}Practical Impact
Without the fix, if your API client code uses:
const apiClient = getProxiedAxios();
apiClient.defaults.baseURL = 'https://api.requestly.io';
apiClient.get('/users'); // This would crash the interceptorWith the fix, the interceptor gracefully handles it by constructing the full URL and checking if it's localhost before applying the custom DNS lookup.
Does this clarify the issue?
URL Port was required for customLookup, hence used
Interceptorsof axios APIcheck doc for reference https://axios-http.com/docs/interceptors
For non-localhost requests, axios default dns resolution will internally it, we do not custom lookup for them
How to test
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.