fix(security): harden server-side SSRF egress filtering across plugins#41962
fix(security): harden server-side SSRF egress filtering across plugins#41962subrata71 wants to merge 1 commit into
Conversation
Consolidates five HIGH SSRF advisories that share one root cause — Appsmith's centralized outbound-request filter (WebClientUtils) is either gated behind IN_DOCKER or bypassed entirely by some plugins. - WebClientUtils: apply link-local (incl. the full 169.254.0.0/16 IMDS range), any-local, multicast and IPv6 ULA blocking on ALL deployments, not only under IN_DOCKER. Cloud-metadata denylist stays always-on. (GHSA-7wfp-6c63-99gq; also closes the loopback-on-non-Docker gap in GHSA-66gg-xpjf-p92v and GHSA-4fjr-w826-cpwm.) - Add opt-in strict egress mode APPSMITH_SSRF_BLOCK_PRIVATE_ADDRESS=true that additionally blocks loopback and RFC1918 on all deployments. Default off, so self-hosted internal datasources are not broken on upgrade. (GHSA-4fjr-w826-cpwm) - Route the AI plugins (OpenAI/Anthropic/Google AI/Appsmith AI) and the Google Sheets trigger path through WebClientUtils.builder() so the SSRF filter is applied consistently. (GHSA-h4wh-59p8-vqff) - Validate the SMTP plugin destination host via the new WebClientUtils.resolveForDatasource() before building the JavaMail session, rejecting metadata/link-local targets while allowing legitimate private mail servers. (GHSA-72m2-f9xp-wg9h) RFC1918 stays reachable by default so existing self-hosted internal datasources keep working. Adds regression tests in WebClientUtilsTest and SmtpPluginTest.
WalkthroughWebClientUtils introduces a strict egress mode (APPSMITH_SSRF_BLOCK_PRIVATE_ADDRESS) and refactors address-class blocking into shared helpers (isBlockedForEgress, isBlockedResolvedAddress) used by both HTTP egress checks and datasource resolution. Multiple plugins switch WebClient construction to WebClientUtils.builder, and SmtpPlugin validates datasource hosts against SSRF rules. ChangesSSRF Egress Filtering
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java (1)
204-224: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftPin the SMTP connection to the resolved address
resolveForDatasource()returns anInetAddress, but this code drops it and still configures JavaMail from the raw host string. That leaves a DNS-rebinding window between the guard and the actual SMTP connect/send. Thread the resolved address through the transport setup instead of re-resolving the hostname later.🤖 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 `@app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java` around lines 204 - 224, The SMTP setup in SmtpPlugin currently validates the host with WebClientUtils.resolveForDatasource() but then still configures JavaMail using the raw endpoint.getHost(), leaving a DNS-rebinding gap. Thread the resolved InetAddress from the guard through the transport/session setup and use that resolved address for the SMTP connection in the SmtpPlugin flow, so the same vetted destination is used instead of re-resolving the hostname later.Source: Learnings
🧹 Nitpick comments (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java (1)
262-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-duplicate of
resolveIfAllowed.
resolveForDatasourceandresolveIfAllowed(Line 221) differ only in the per-address predicate (isBlockedResolvedAddressvsmatchesBlockedAddressClass). Consider extracting the shared normalize/resolve/loop skeleton and parameterizing the predicate to avoid drift between the two resolvers.🤖 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 `@app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java` around lines 262 - 288, `resolveForDatasource` is nearly the same as `resolveIfAllowed`, with only the per-address block check differing. Extract the shared normalize/resolve/iterate skeleton into a common helper in `WebClientUtils`, and pass in the address predicate so `resolveForDatasource` uses `isBlockedResolvedAddress` while `resolveIfAllowed` uses `matchesBlockedAddressClass`. Keep the existing `normalizeHostForComparisonQuietly`, `InetAddress.getAllByName`, and `DISALLOWED_HOSTS` handling centralized to prevent future drift.
🤖 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
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java`:
- Around line 262-288: SMTP hostname validation is still bypassable because
`SmtpPlugin.datasourceCreate` only checks `endpoint.getHost()` before JavaMail
connects. Update the SMTP connection flow to either pin the session/transport to
the resolved `InetAddress` returned by `WebClientUtils.resolveForDatasource` or
add post-connect verification of the remote peer after `connect()`, so the
actual connected address is checked against the DNS-rebinding guard.
In
`@app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java`:
- Around line 218-224: The parameterized test in WebClientUtilsTest mixes
loopback and RFC1918 hosts in a case that depends on the runtime IN_DOCKER flag,
so remove the 127.0.0.1 row from
isDisallowedAndFail_allowsLoopbackAndPrivateByDefault and keep only the
deterministic non-Docker-gated private ranges. Leave the separate pure-overload
coverage for isDisallowedAndFail and resolveForDatasource intact so the test
stays stable regardless of environment.
---
Outside diff comments:
In
`@app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java`:
- Around line 204-224: The SMTP setup in SmtpPlugin currently validates the host
with WebClientUtils.resolveForDatasource() but then still configures JavaMail
using the raw endpoint.getHost(), leaving a DNS-rebinding gap. Thread the
resolved InetAddress from the guard through the transport/session setup and use
that resolved address for the SMTP connection in the SmtpPlugin flow, so the
same vetted destination is used instead of re-resolving the hostname later.
---
Nitpick comments:
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java`:
- Around line 262-288: `resolveForDatasource` is nearly the same as
`resolveIfAllowed`, with only the per-address block check differing. Extract the
shared normalize/resolve/iterate skeleton into a common helper in
`WebClientUtils`, and pass in the address predicate so `resolveForDatasource`
uses `isBlockedResolvedAddress` while `resolveIfAllowed` uses
`matchesBlockedAddressClass`. Keep the existing
`normalizeHostForComparisonQuietly`, `InetAddress.getAllByName`, and
`DISALLOWED_HOSTS` handling centralized to prevent future drift.
🪄 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: 4bfa7f2e-d533-4b60-96be-50e850818978
📒 Files selected for processing (10)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.javaapp/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.javaapp/server/appsmith-plugins/anthropicPlugin/src/main/java/com/external/plugins/utils/RequestUtils.javaapp/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/utils/RequestUtils.javaapp/server/appsmith-plugins/googleAiPlugin/src/main/java/com/external/plugins/utils/RequestUtils.javaapp/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.javaapp/server/appsmith-plugins/openAiPlugin/src/main/java/com/external/plugins/utils/RequestUtils.javaapp/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.javaapp/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/exceptions/SMTPErrorMessages.javaapp/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java
| public static Optional<InetAddress> resolveForDatasource(String host) { | ||
| if (!StringUtils.hasText(host)) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| final String canonicalHost = normalizeHostForComparisonQuietly(host); | ||
| if (DISALLOWED_HOSTS.contains(canonicalHost)) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| final InetAddress[] resolved; | ||
| try { | ||
| resolved = InetAddress.getAllByName(host); | ||
| } catch (UnknownHostException e) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| final boolean inDocker = "1".equals(System.getenv("IN_DOCKER")); | ||
| for (InetAddress addr : resolved) { | ||
| if (DISALLOWED_HOSTS.contains(normalizeHostForComparisonQuietly(addr.getHostAddress())) | ||
| || isBlockedResolvedAddress(addr, inDocker, BLOCK_PRIVATE_ADDRESSES)) { | ||
| return Optional.empty(); | ||
| } | ||
| } | ||
|
|
||
| return Optional.of(resolved[0]); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How does SmtpPlugin build the JavaMail session after resolveForDatasource? Does it pin the IP?
fd 'SmtpPlugin.java' --exec sed -n '190,320p' {}Repository: appsmithorg/appsmith
Length of output: 6707
🌐 Web query:
JavaMail Transport.connect host DNS resolution re-resolves on connect mail.smtp.host ip pinning
💡 Result:
In Jakarta Mail (formerly JavaMail), the Transport.connect method does not explicitly pin IP addresses, nor does it inherently cache DNS lookups in a way that prevents re-resolution [1][2]. The resolution of the mail.smtp.host property is handled by the underlying Java networking stack (specifically java.net.InetAddress) each time a new connection is established [1][3]. Key technical points regarding DNS resolution and IP pinning in this context include: 1. DNS Resolution Behavior: When you call transport.connect, Jakarta Mail triggers a socket connection attempt to the host specified in the mail.smtp.host property (or passed as an argument) [4][5]. This relies on the standard Java networking library to resolve the hostname to an IP address [1][2]. If the hostname resolves to multiple IP addresses, the behavior depends on the JVM's underlying implementation of InetAddress.getByName or getAllByName, which typically follows the OS-level DNS resolution rules and potential local JVM-level caching [1]. 2. Lack of Native IP Pinning: Jakarta Mail does not implement its own internal DNS caching layer or IP pinning mechanism [1]. Because the library relies on the Java runtime's networking facilities, it does not "pin" to a specific IP address across multiple sessions unless explicitly configured to do so by the network environment or JVM system properties [1][3]. 3. DNS Timeouts and Performance: DNS resolution is performed synchronously. If the DNS server is unresponsive or slow, the connect call can appear to hang, as JavaMail timeouts generally cover only the socket connection, read, and write operations, not the initial DNS lookup [3]. 4. Workarounds for Resilience: Because Jakarta Mail does not natively handle DNS-based load balancing (such as iterating through multiple IP addresses from an MX record or a round-robin DNS entry upon connection failure), developers often implement custom retry logic [1][2]. This involves resolving the hostname manually via InetAddress.getAllByName, shuffling the resulting array of IP addresses, and attempting to connect to each address sequentially if the previous attempt fails [1]. In summary, JavaMail performs a fresh DNS lookup through the standard Java networking stack at the time of connection [1][3]. If you require behavior like "IP pinning" (sticking to one IP) or conversely "DNS failover" (trying all IPs), these must be managed at the application layer or via JVM configuration [1][2].
Citations:
- 1: Loadbalancing and Failover support for SMTP transport jakartaee/mail-api#365
- 2: DNS MX record not used for mailserver lookup and failback jakartaee/mail-api#561
- 3: https://www.daniweb.com/programming/software-development/threads/496611/tranport-send-hangs
- 4: https://github.com/javaee/javamail/blob/master/mail/src/main/java/com/sun/mail/smtp/SMTPTransport.java
- 5: https://javaee.github.io/javamail/docs/api/com/sun/mail/smtp/SMTPTransport.html
SMTP still needs IP pinning or post-connect validation
SmtpPlugin.datasourceCreate validates endpoint.getHost() up front, but JavaMail still connects using the hostname, so the SMTP transport can re-resolve it at connect time and bypass the DNS-rebinding guard. Pin the session to the resolved InetAddress, or validate the remote peer after connect().
🤖 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
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java`
around lines 262 - 288, SMTP hostname validation is still bypassable because
`SmtpPlugin.datasourceCreate` only checks `endpoint.getHost()` before JavaMail
connects. Update the SMTP connection flow to either pin the session/transport to
the resolved `InetAddress` returned by `WebClientUtils.resolveForDatasource` or
add post-connect verification of the remote peer after `connect()`, so the
actual connected address is checked against the DNS-rebinding guard.
Source: Learnings
| @ParameterizedTest | ||
| @ValueSource(strings = {"127.0.0.1", "10.0.0.1", "192.168.1.1", "172.16.0.1"}) | ||
| public void isDisallowedAndFail_allowsLoopbackAndPrivateByDefault(String host) { | ||
| assertFalse( | ||
| WebClientUtils.isDisallowedAndFail(host, null), | ||
| "Expected " + host + " to be allowed by default (no IN_DOCKER, strict egress off)"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does the CI/build env set IN_DOCKER for server test runs?
rg -n 'IN_DOCKER' --iglob '*.yml' --iglob '*.yaml' --iglob 'Dockerfile*' --iglob '*.sh'Repository: appsmithorg/appsmith
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate IN_DOCKER usage ---'
rg -n 'IN_DOCKER' app/server || true
echo
echo '--- inspect WebClientUtilsTest around the flagged lines ---'
sed -n '180,260p' app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java
echo
echo '--- inspect WebClientUtils implementation ---'
fd -a 'WebClientUtils.java' app/serverRepository: appsmithorg/appsmith
Length of output: 6630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -a 'WebClientUtils.java' app/server | head -n 1)"
echo "FILE=$file"
echo
ast-grep outline "$file" --view expanded || true
echo
sed -n '1,260p' "$file"Repository: appsmithorg/appsmith
Length of output: 13740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java"
echo '--- isDisallowedAndFail / resolveForDatasource / egress checks ---'
sed -n '250,420p' "$file"Repository: appsmithorg/appsmith
Length of output: 7944
Drop the loopback rows from the live-env tests
isDisallowedAndFail and resolveForDatasource both read IN_DOCKER at runtime, so the 127.0.0.1 cases flip under IN_DOCKER=1 while the RFC1918 rows stay deterministic. Keep the pure overload coverage below and leave only the non-Docker-gated cases here.
🤖 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
`@app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java`
around lines 218 - 224, The parameterized test in WebClientUtilsTest mixes
loopback and RFC1918 hosts in a case that depends on the runtime IN_DOCKER flag,
so remove the 127.0.0.1 row from
isDisallowedAndFail_allowsLoopbackAndPrivateByDefault and keep only the
deterministic non-Docker-gated private ranges. Leave the separate pure-overload
coverage for isDisallowedAndFail and resolveForDatasource intact so the test
stays stable regardless of environment.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
1 participant
Summary
Consolidates five HIGH severity SSRF advisories that share a single root cause into one coherent change. Appsmith's centralized outbound-request filter (
WebClientUtils) was either gated behindIN_DOCKER(so non-Docker deployments lost address-class filtering) or bypassed entirely by a few plugins that built raw clients.Advisories addressed:
GHSA-7wfp-6c63-99gq(keystone),GHSA-4fjr-w826-cpwm,GHSA-66gg-xpjf-p92v,GHSA-h4wh-59p8-vqff,GHSA-72m2-f9xp-wg9h.Changes
WebClientUtils— deployment-independent address-class filtering. Link-local (including the full169.254.0.0/16IMDS range and IPv6fe80::/10), any-local, multicast and IPv6 ULA (fc00::/7) are now blocked on every deployment, not just underIN_DOCKER. The cloud-metadata denylist remains always-on. This is the keystone that also closes the loopback-on-non-Docker residual in the ElasticSearch and REST advisories.APPSMITH_SSRF_BLOCK_PRIVATE_ADDRESS=trueadditionally blocks loopback and RFC1918 private ranges on all deployments. Default off, so self-hosted deployments that legitimately connect datasources to internal/private hosts are not broken on upgrade.WebClientUtils.builder()instead of a rawWebClient.builder(), so the SSRF filter applies uniformly. These target fixed provider hosts, so legitimate traffic is unchanged.WebClientUtils.resolveForDatasource()before the session is built — rejecting metadata/link-local targets while allowing legitimate private mail servers by default.Why this is regression-safe
IN_DOCKER.Test plan
WebClientUtilsTest— 93 tests pass (added coverage for always-blocked classes regardless ofIN_DOCKER, default-allow of loopback/RFC1918, strict-mode blocking via the pureisBlockedResolvedAddressoverload, andresolveForDatasource).SmtpPluginTest— added tests thatdatasourceCreaterejects cloud-metadata and link-local hosts and allows private hosts by default (test module compiles; integration tests require Docker).appsmith-interfacescompile;spotless:checkclean.release.Automation
/ok-to-test tags="@tag.All"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/28863593712
Commit: bab5d44
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 07 Jul 2026 12:44:13 UTC
Summary by CodeRabbit
New Features
Bug Fixes