feat: implement SSRF guard with request configuration and add tests#1826
Conversation
|
Warning Review limit reached
More reviews will be available in 12 minutes and 46 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements SSRF protection by extracting forbidden address validation into a shared helper, creating guarded DNS resolution with Axios agent wiring for SaaS mode, and applying SSRF-safe request configuration to table action and Slack integrations. Test coverage verifies guard rejection of forbidden IPs and config behavior across environments. ChangesSSRF Protection Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
Implements an SSRF mitigation for outbound HTTP(S) calls in SaaS mode by introducing a DNS lookup guard and a reusable Axios request configuration, then wiring it into existing outbound request paths.
Changes:
- Added
ssrfGuardLookup+getSsrfSafeRequestConfig()to enforce forbidden-address blocking via custom HTTP(S) agents (SaaS-only, non-test). - Centralized forbidden-address checking into a shared
isForbiddenAddress()helper and re-exported it for compatibility. - Updated outbound Axios calls (Slack webhook + table action HTTP callback) to use the SSRF-safe request config; added AVA unit tests for the guard/config.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/unit-tests/ssrf-safe-http.test.ts | Adds unit tests for SSRF lookup blocking/allowing behavior and SaaS/test gating for request config. |
| backend/src/helpers/validators/ssrf-safe-http.ts | Introduces SSRF-safe Axios request config and guarded DNS lookup used by custom agents. |
| backend/src/helpers/validators/is-forbidden-address.ts | Extracts and centralizes forbidden-address detection logic. |
| backend/src/helpers/slack/action-slack-post-message.ts | Applies SSRF-safe request config to Slack webhook POST. |
| backend/src/entities/table-actions/table-actions-module/table-action-activation.service.ts | Applies SSRF-safe request config to outbound table action HTTP POST. |
| backend/src/entities/connection/utils/is-host-allowed.ts | Switches to shared isForbiddenAddress implementation and re-exports it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import test from 'ava'; | ||
| import { getSsrfSafeRequestConfig, ssrfGuardLookup } from '../../../src/helpers/validators/ssrf-safe-http.js'; | ||
|
|
||
| // The SSRF guard is only meant to engage in SaaS mode (self-hosted owns its network). isSaaS()/ |
| import test from 'ava'; | ||
| import { getSsrfSafeRequestConfig, ssrfGuardLookup } from '../../../src/helpers/validators/ssrf-safe-http.js'; |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/entities/table-actions/table-actions-module/table-action-activation.service.ts (1)
217-242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve Axios/SSRF-guard transport error details when no response exists
In SaaS,
getSsrfSafeRequestConfig()installs a custom agent lookup (ssrfGuardLookup) that can fail during DNS/SSRF checks beforeaxios.postreceives any HTTP response. Sinceresultis only assigned after theawait axios.post(...)succeeds, thecatchblock often falls back toresult?.data..., losing the real Axios error message and returning a generic “An error occurred”. Build the message fromerror.response?.data(when present) and otherwise fromerror.message.🔧 Proposed fix
} catch (error) { if (axios.isAxiosError(error)) { + const responseData = error.response?.data; const errorMessage = - result?.data?.error || - result?.data?.message || - result?.data?.errorMessage || - result?.data?.response || + responseData?.error || + responseData?.message || + responseData?.errorMessage || + responseData?.response || + error.message || 'An error occurred'; const responseStatus = error.response?.status || result?.status || 500; throw new HttpException( { message: errorMessage, }, responseStatus, ); } throw error; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/entities/table-actions/table-actions-module/table-action-activation.service.ts` around lines 217 - 242, The catch block in table-action-activation.service.ts currently reads error details from the local result variable which is only set on success; update the axios error handling in the catch of the axios.post call so that when axios.isAxiosError(error) you first extract data from error.response?.data (checking .error, .message, .errorMessage, .response) and if none exist fall back to error.message, and determine status from error.response?.status or default 500; then throw the HttpException using that constructed message and status (preserving SSRF/agent errors from getSsrfSafeRequestConfig()/ssrfGuardLookup by using error.message when no response exists).
🧹 Nitpick comments (1)
backend/test/ava-tests/unit-tests/ssrf-safe-http.test.ts (1)
10-33: ⚡ Quick winUse arrow helpers and an interface for the lookup result type.
On Line 10 and Line 23, switch helper declarations to
constarrow functions and extract the inline object shape into aninterfaceto match repository TS rules.Proposed refactor
+interface LookupResult { + address: string; + family: number; +} + -function setEnv(key: 'NODE_ENV' | 'IS_SAAS', value: string | undefined): void { +const setEnv = (key: 'NODE_ENV' | 'IS_SAAS', value: string | undefined): void => { if (value === undefined) { delete process.env[key]; } else { process.env[key] = value; } -} +}; -function lookup(host: string): Promise<{ address: string; family: number }> { +const lookup = (host: string): Promise<LookupResult> => { return new Promise((resolve, reject) => { ssrfGuardLookup(host, {}, (err, address, family) => { if (err) { reject(err); } else { resolve({ address: address as string, family: family ?? 0 }); } }); }); -} +};As per coding guidelines, “Prefer arrow functions over function declarations” and “Use interfaces for object shapes and type for unions and primitives”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/unit-tests/ssrf-safe-http.test.ts` around lines 10 - 33, Convert the function declarations setEnv and lookup into const arrow functions and introduce an interface (e.g., LookupResult) for the lookup return shape instead of an inline object type; specifically, change "function setEnv(...)" to "const setEnv = (...): void => { ... }" and "function lookup(host: string): Promise<...>" to "const lookup = (host: string): Promise<LookupResult> => { ... }", define "interface LookupResult { address: string; family: number }" and update the lookup resolve to return that interface type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/helpers/validators/ssrf-safe-http.ts`:
- Around line 39-43: The SSRF-safe axios config returned from the function
currently sets httpAgent/httpsAgent and timeout but doesn't disable axios'
environment proxy handling; add proxy: false to the returned config object so
axios won't use HTTP_PROXY/HTTPS_PROXY and bypass the
ssrfSafeHttpAgent/ssrfSafeHttpsAgent checks (update the return where httpAgent:
ssrfSafeHttpAgent, httpsAgent: ssrfSafeHttpsAgent, maxRedirects: 0, timeout:
SSRF_REQUEST_TIMEOUT_MS are set to include proxy: false).
---
Outside diff comments:
In
`@backend/src/entities/table-actions/table-actions-module/table-action-activation.service.ts`:
- Around line 217-242: The catch block in table-action-activation.service.ts
currently reads error details from the local result variable which is only set
on success; update the axios error handling in the catch of the axios.post call
so that when axios.isAxiosError(error) you first extract data from
error.response?.data (checking .error, .message, .errorMessage, .response) and
if none exist fall back to error.message, and determine status from
error.response?.status or default 500; then throw the HttpException using that
constructed message and status (preserving SSRF/agent errors from
getSsrfSafeRequestConfig()/ssrfGuardLookup by using error.message when no
response exists).
---
Nitpick comments:
In `@backend/test/ava-tests/unit-tests/ssrf-safe-http.test.ts`:
- Around line 10-33: Convert the function declarations setEnv and lookup into
const arrow functions and introduce an interface (e.g., LookupResult) for the
lookup return shape instead of an inline object type; specifically, change
"function setEnv(...)" to "const setEnv = (...): void => { ... }" and "function
lookup(host: string): Promise<...>" to "const lookup = (host: string):
Promise<LookupResult> => { ... }", define "interface LookupResult { address:
string; family: number }" and update the lookup resolve to return that interface
type.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6655085f-fa75-48d2-9687-83ba6784c5c9
📒 Files selected for processing (6)
backend/src/entities/connection/utils/is-host-allowed.tsbackend/src/entities/table-actions/table-actions-module/table-action-activation.service.tsbackend/src/helpers/slack/action-slack-post-message.tsbackend/src/helpers/validators/is-forbidden-address.tsbackend/src/helpers/validators/ssrf-safe-http.tsbackend/test/ava-tests/unit-tests/ssrf-safe-http.test.ts
| return { | ||
| httpAgent: ssrfSafeHttpAgent, | ||
| httpsAgent: ssrfSafeHttpsAgent, | ||
| maxRedirects: 0, | ||
| timeout: SSRF_REQUEST_TIMEOUT_MS, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Axios in Node.js honor HTTP_PROXY / HTTPS_PROXY environment variables unless AxiosRequestConfig.proxy is explicitly set to false?
💡 Result:
Yes. In Axios for Node.js, HTTP(S)_PROXY environment variables are honored by Axios’s own proxy logic unless you explicitly disable proxying by setting AxiosRequestConfig.proxy to false. Axios documents that setting proxy to false “disable[s] proxies, ignoring environment variables.”[1][2] In practice, Axios’s HTTP adapter applies proxy settings derived from the environment; and the Axios issue discussions note that Node’s built-in proxy handling may also be involved in some setups, but they still describe proxy: false as the switch that disables Axios’s proxy behavior.[3][4] So, if you want Axios to ignore HTTP_PROXY/HTTPS_PROXY/NO_PROXY, you must set proxy: false in the request config (or corresponding Axios instance/defaults).
Citations:
- 1: https://www.mintlify.com/axios/axios/advanced/request-config
- 2: https://github.com/axios/axios-docs/blob/master/posts/en/req_config.md
- 3: Proxy environment variables ignored axios/axios#6321
- 4: axios native proxy detection collides with native (NODE_USE_ENV_PROXY) NodeJS proxy handling axios/axios#7299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the SSRF-safe request config helper
rg -n "getSsrfSafeRequestConfig|ssrfSafeHttpAgent|ssrfSafeHttpsAgent|ssrfGuardLookup" backend/src/helpers/validators/ssrf-safe-http.ts
echo "----"
sed -n '1,200p' backend/src/helpers/validators/ssrf-safe-http.ts
echo "==== Usage sites ===="
# Find usages of getSsrfSafeRequestConfig
rg -n "getSsrfSafeRequestConfig\(" backend/src
echo "----"
# Show surrounding context where used
rg -n "getSsrfSafeRequestConfig\(" backend/src -n -C 3
echo "==== Axios proxy config elsewhere ===="
# Search for proxy configuration or env proxy usage
rg -n "axios.*proxy|AxiosRequestConfig.*proxy|proxy:\s*false|HTTP_PROXY|HTTPS_PROXY|NO_PROXY" backend/src -SRepository: rocket-admin/rocketadmin
Length of output: 4342
🌐 Web query:
Axios for Node.js: what does setting AxiosRequestConfig.proxy=false do regarding HTTP(S)_PROXY env vars and NO_PROXY?
💡 Result:
Setting AxiosRequestConfig.proxy to false disables Axios's internal proxy processing logic [1][2]. When this value is explicitly set, Axios ignores the HTTP_PROXY, HTTPS_PROXY, and NO_PROXY environment variables that it would otherwise automatically detect and apply to the request [1][2][3]. By setting proxy: false, you effectively opt out of Axios's automatic proxy resolution mechanism [1][2]. This is the standard way to ensure that your request is made directly (or via any custom agent you explicitly configure) without interference from environment-based proxy configurations that Axios would normally inspect [1][4]. It is important to note the following context: 1. Scope of Effect: This setting specifically targets Axios's own proxy handling [2]. It does not prevent other mechanisms, such as Node.js native proxy support (e.g., if NODE_USE_ENV_PROXY is enabled in modern Node.js versions) or other libraries from potentially applying proxies, though it prevents Axios from injecting its own proxy configuration or overriding request details based on environment variables [5][6]. 2. Custom Agents: If you need to manage your own proxy logic (e.g., using an https-proxy-agent), you should set proxy: false and provide your custom httpAgent or httpsAgent configuration [1][4]. 3. Troubleshooting: In some configurations, if you are not using environment variables but are seeing issues, ensuring proxy: false is set can help confirm that Axios is not unintentionally attempting to resolve a proxy [7]. Conversely, if you observe that a proxy is still being used despite setting proxy: false, it may be due to Node.js's own internal proxy handling (if enabled) or a custom agent implementation that includes its own proxy logic [5][8][9].
Citations:
- 1: https://github.com/axios/axios-docs/blob/master/posts/en/req_config.md
- 2: Adding a way to disable all proxy processing axios/axios#691
- 3: https://brightdata.com/blog/how-tos/axios-proxy
- 4: Add Proxy support axios/axios#1679
- 5: axios native proxy detection collides with native (NODE_USE_ENV_PROXY) NodeJS proxy handling axios/axios#7299
- 6: fix: skip Axios proxy logic when NODE_USE_ENV_PROXY is enabled on sup… axios/axios#7300
- 7: Axios v1.8.2 failing with
proxy: falseaxios config axios/axios#6941 - 8: fix: https data in cleartext to proxy axios/axios#10858
- 9: Proxy environment variables ignored axios/axios#6321
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the SSRF-safe request config helper
rg -n "getSsrfSafeRequestConfig|ssrfSafeHttpAgent|ssrfSafeHttpsAgent|ssrfGuardLookup" backend/src/helpers/validators/ssrf-safe-http.ts
echo "----"
sed -n '1,200p' backend/src/helpers/validators/ssrf-safe-http.ts
echo "==== Usage sites ===="
# Find usages of getSsrfSafeRequestConfig
rg -n "getSsrfSafeRequestConfig\(" backend/src
echo "----"
# Show surrounding context where used
rg -n "getSsrfSafeRequestConfig\(" backend/src -n -C 3
echo "==== Axios proxy config elsewhere ===="
# Search for proxy configuration or env proxy usage
rg -n "axios.*proxy|AxiosRequestConfig.*proxy|proxy:\s*false|HTTP_PROXY|HTTPS_PROXY|NO_PROXY" backend/src -SRepository: rocket-admin/rocketadmin
Length of output: 4342
🌐 Web query:
Axios for Node.js: what does setting AxiosRequestConfig.proxy=false do regarding HTTP(S)_PROXY env vars and NO_PROXY?
💡 Result:
In Axios for Node.js, setting AxiosRequestConfig.proxy = false instructs Axios to disable its internal proxy resolution logic for that specific request [1][2]. Specifically, this configuration has the following effects: 1. Ignores Environment Variables: It causes Axios to ignore standard proxy environment variables, including HTTP_PROXY, HTTPS_PROXY, and NO_PROXY [1][2]. 2. Disables Internal Proxy Logic: By setting this, you explicitly instruct Axios to skip its internal code responsible for parsing these environment variables and configuring the request to route through a proxy [3][4]. 3. Direct Connection: Because the internal proxy logic is bypassed, the request will typically attempt to connect directly to the destination URL [2]. Important Considerations: * Interaction with Node.js Native Proxy Support: In modern Node.js versions (e.g., when NODE_USE_ENV_PROXY is enabled), Node.js may handle proxying natively [5][3]. When proxy: false is set in Axios, you are disabling only the Axios-specific proxy logic; if your Node.js environment is configured to handle proxies automatically, that behavior may still persist independently of Axios [3][4]. * Potential for Conflicts: Users have reported that in certain configurations—particularly when custom httpAgent or httpsAgent instances are provided—setting proxy: false can occasionally lead to unexpected connection errors (e.g., ENOTFOUND or other socket-related issues) if the underlying agent expectations are not perfectly aligned with a direct connection [6]. * Not a Global Setting: This is a request-level configuration. If you wish to disable proxying for all requests, you must apply it to your Axios instance defaults or to every individual request configuration object [7].
Citations:
- 1: https://github.com/axios/axios-docs/blob/master/posts/en/req_config.md
- 2: Adding a way to disable all proxy processing axios/axios#691
- 3: axios native proxy detection collides with native (NODE_USE_ENV_PROXY) NodeJS proxy handling axios/axios#7299
- 4: fix: skip Axios proxy logic when NODE_USE_ENV_PROXY is enabled on sup… axios/axios#7300
- 5: https://www.npmjs.com/package/proxy-from-env
- 6: Axios v1.8.2 failing with
proxy: falseaxios config axios/axios#6941 - 7: https://github.com/axios/axios/blob/e9965bfafc82d8b42765705061b9ebe2d5532493/index.d.ts
Disable Axios environment proxy handling in SSRF-safe config
Axios can still use HTTP_PROXY/HTTPS_PROXY; with only a guarded httpAgent/httpsAgent, the guard validates the proxy host DNS but the proxy resolves/connects to the final destination, bypassing the SSRF check. Set proxy: false in the returned config.
🔒 Proposed fix
return {
httpAgent: ssrfSafeHttpAgent,
httpsAgent: ssrfSafeHttpsAgent,
+ proxy: false,
maxRedirects: 0,
timeout: SSRF_REQUEST_TIMEOUT_MS,
};📝 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.
| return { | |
| httpAgent: ssrfSafeHttpAgent, | |
| httpsAgent: ssrfSafeHttpsAgent, | |
| maxRedirects: 0, | |
| timeout: SSRF_REQUEST_TIMEOUT_MS, | |
| return { | |
| httpAgent: ssrfSafeHttpAgent, | |
| httpsAgent: ssrfSafeHttpsAgent, | |
| proxy: false, | |
| maxRedirects: 0, | |
| timeout: SSRF_REQUEST_TIMEOUT_MS, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/helpers/validators/ssrf-safe-http.ts` around lines 39 - 43, The
SSRF-safe axios config returned from the function currently sets
httpAgent/httpsAgent and timeout but doesn't disable axios' environment proxy
handling; add proxy: false to the returned config object so axios won't use
HTTP_PROXY/HTTPS_PROXY and bypass the ssrfSafeHttpAgent/ssrfSafeHttpsAgent
checks (update the return where httpAgent: ssrfSafeHttpAgent, httpsAgent:
ssrfSafeHttpsAgent, maxRedirects: 0, timeout: SSRF_REQUEST_TIMEOUT_MS are set to
include proxy: false).
…g for non-SaaS environments
…tadmin into backend_fixes
Summary by CodeRabbit