fix(security): block SSRF via send-test-email SMTP host validation (GHSA-vvxf-f8q9-86gh)#41666
Open
fix(security): block SSRF via send-test-email SMTP host validation (GHSA-vvxf-f8q9-86gh)#41666
Conversation
The sendTestEmail endpoint accepted attacker-controlled smtpHost/smtpPort and passed them directly to JavaMailSenderImpl, bypassing the WebClient IP_CHECK_FILTER. This allowed admin users to reach internal services, cloud metadata endpoints, and private networks via raw SMTP connections. - Add WebClientUtils.validateHostNotDisallowed() that checks hostnames against the existing denylist and, after DNS resolution, rejects loopback, link-local, site-local, and multicast addresses - Apply host validation in sendTestEmail before connecting - Sanitize error messages to prevent information leakage via error-based port scanning (CWE-209) - Add tests for blocked and allowed hosts Fixes https://linear.app/appsmith/issue/APP-15034 Advisory: GHSA-vvxf-f8q9-86gh
Contributor
WalkthroughAdds a new public API Changes
Sequence DiagramsequenceDiagram
participant Client
participant EnvManager as EnvManager<br/>(sendTestEmail)
participant WebClientUtils as WebClientUtils<br/>(resolveIfAllowed)
participant DNS as DNS/InetAddress
participant SMTP as SMTP Server
Client->>EnvManager: sendTestEmail(smtpHost)
EnvManager->>WebClientUtils: resolveIfAllowed(smtpHost)
WebClientUtils->>WebClientUtils: canonicalize & check DISALLOWED_HOSTS
alt Host is in disallowed list
WebClientUtils-->>EnvManager: Optional.empty()
else Host not pre-disallowed
WebClientUtils->>DNS: InetAddress.getAllByName(host)
DNS-->>WebClientUtils: resolved addresses[]
WebClientUtils->>WebClientUtils: check each address (ULA, loopback, link-local, any-local, multicast)
alt any address rejected
WebClientUtils-->>EnvManager: Optional.empty()
else all addresses allowed
WebClientUtils-->>EnvManager: Optional.of(resolved[0])
end
end
alt Validation failed
EnvManager-->>Client: AppsmithException("Invalid SMTP host.")
else Validation passed
EnvManager->>SMTP: connect/send using resolved host
SMTP-->>EnvManager: success or exception
alt exception
EnvManager-->>Client: GENERIC_BAD_REQUEST (SMTP_GENERIC_ERROR)
else
EnvManager-->>Client: success
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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📝 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 |
subrata71
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java (1)
79-84: Avoid live DNS dependency in this unit test.Line 82 performs real DNS resolution for external domains, which can make the test flaky in restricted CI environments. Prefer a deterministic resolver abstraction (mocked in tests) or move live-resolution checks to an integration-test suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java` around lines 79 - 84, The test validateHostNotDisallowed_allowsLegitimateSmtpHosts calls WebClientUtils.validateHostNotDisallowed which triggers real DNS resolution making the unit test flaky; modify the code under test to accept a DNS/resolver abstraction (or provide an overloaded method) and update this test to inject a deterministic mocked resolver that returns expected answers, or alternatively replace external hosts with deterministic local test hosts (e.g., localhost or a controlled test domain) and assert against those; locate the WebClientUtils.validateHostNotDisallowed method and the test validateHostNotDisallowed_allowsLegitimateSmtpHosts to implement the resolver injection and update the test to mock/stub resolution so no live DNS lookup occurs.app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java (1)
404-450: Consider parameterizing the blocked-host tests.Line 404-Line 450 repeats the same pattern 4 times. A single
@ParameterizedTestwith host values would keep this easier to extend.♻️ Refactor sketch
+ `@ParameterizedTest` + `@ValueSource`(strings = {"127.0.0.1", "169.254.169.254", "10.0.0.1", "localhost"}) + public void sendTestEmail_WhenBlockedHost_ThrowsException(String host) { + mockSuperUser(); + StepVerifier.create(envManager.sendTestEmail(buildDto(host))) + .expectErrorSatisfies(e -> { + assertThat(e).isInstanceOf(AppsmithException.class); + assertThat(e.getMessage()).contains("Invalid SMTP host"); + }) + .verify(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java` around lines 404 - 450, Replace the four near-duplicate tests in EnvManagerTest with a single parameterized test: create a new `@ParameterizedTest` method (e.g., sendTestEmail_WhenBlockedHost_ThrowsException) that calls mockSuperUser(), then uses StepVerifier.create(envManager.sendTestEmail(buildDto(host))) and the same expectErrorSatisfies assertions; supply the blocked host values ("127.0.0.1", "169.254.169.254", "10.0.0.1", "localhost") via `@ValueSource` or `@CsvSource` so the logic in sendTestEmail and buildDto is exercised once for each host. Ensure imports for the JUnit Jupiter parameterized annotations are added and remove the four original test methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java`:
- Around line 231-236: The validation in WebClientUtils currently relies on
InetAddress methods which miss IPv6 Unique Local Addresses (fc00::/7); update
the block in the method that checks addr (the addr variable and HOST_NOT_ALLOWED
return) to explicitly detect IPv6 ULA by: if addr is an instance of
java.net.Inet6Address, call addr.getAddress() and test the first byte with
((bytes[0] & 0xFE) == 0xFC) (this matches fc00::/7), and if true return
Optional.of(HOST_NOT_ALLOWED); keep the existing
loopback/link-local/site-local/any-local/multicast checks intact.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java`:
- Around line 793-801: The current validation via
WebClientUtils.validateHostNotDisallowed(requestDTO.getSmtpHost()) can be
bypassed by DNS rebinding because JavaMailSenderImpl (mailSender) reconnects
using the original hostname; fix by performing a single trusted DNS resolution
during validation and using that resolved InetAddress for the actual socket
connect (or supply a custom SocketFactory/SSLSocketFactory to JavaMailSenderImpl
that enforces/connects to the validated InetAddress) so the mail connection
cannot re-resolve to a different IP at connect time; update EnvManagerCEImpl to
use the resolved address returned from validateHostNotDisallowed (or a new
resolver helper) when configuring mailSender and ensure the SMTP EHLO/Host
header is set appropriately if SMTP requires the original hostname.
---
Nitpick comments:
In
`@app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java`:
- Around line 79-84: The test
validateHostNotDisallowed_allowsLegitimateSmtpHosts calls
WebClientUtils.validateHostNotDisallowed which triggers real DNS resolution
making the unit test flaky; modify the code under test to accept a DNS/resolver
abstraction (or provide an overloaded method) and update this test to inject a
deterministic mocked resolver that returns expected answers, or alternatively
replace external hosts with deterministic local test hosts (e.g., localhost or a
controlled test domain) and assert against those; locate the
WebClientUtils.validateHostNotDisallowed method and the test
validateHostNotDisallowed_allowsLegitimateSmtpHosts to implement the resolver
injection and update the test to mock/stub resolution so no live DNS lookup
occurs.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java`:
- Around line 404-450: Replace the four near-duplicate tests in EnvManagerTest
with a single parameterized test: create a new `@ParameterizedTest` method (e.g.,
sendTestEmail_WhenBlockedHost_ThrowsException) that calls mockSuperUser(), then
uses StepVerifier.create(envManager.sendTestEmail(buildDto(host))) and the same
expectErrorSatisfies assertions; supply the blocked host values ("127.0.0.1",
"169.254.169.254", "10.0.0.1", "localhost") via `@ValueSource` or `@CsvSource` so
the logic in sendTestEmail and buildDto is exercised once for each host. Ensure
imports for the JUnit Jupiter parameterized annotations are added and remove the
four original test methods.
🪄 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: 3abcb242-ae73-41d6-9588-ab5160e4e72a
📒 Files selected for processing (4)
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-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java
Outdated
Show resolved
Hide resolved
Address CodeRabbitAI review findings and Cypress test regression: 1. DNS rebinding TOCTOU (Critical): Rename validateHostNotDisallowed to resolveIfAllowed, returning Optional<InetAddress>. The caller now uses the resolved IP directly for mailSender.setHost(), eliminating the window between validation and connection where DNS could rebind. 2. IPv6 ULA gap (Major): Java's isSiteLocalAddress() only checks the deprecated fec0::/10 prefix, not fc00::/7 (Unique Local Addresses). Added explicit byte-level check: (firstByte & 0xFE) == 0xFC. 3. Cypress test failure: Removed isSiteLocalAddress() from the blocking criteria. The existing WebClient denylist intentionally allows RFC 1918 (10/8, 172.16/12, 192.168/16) because legitimate SMTP servers reside on private networks. host.docker.internal (used in CI) resolves to a site-local address and was incorrectly blocked. 4. Test cleanup: Parameterized duplicate EnvManager blocked-host tests into a single @ParameterizedTest. Updated WebClientUtilsTest for new API signature and added resolved-address assertion.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java (1)
406-417: Good SSRF regression coverage; consider expanding test vectors.The parameterized test covers the critical SSRF vectors (loopback, AWS metadata IP, localhost). The assertion correctly uses
contains()which handles the trailing period in the production message.Consider adding
0.0.0.0(any-local),168.63.129.16(Azure metadata), and100.100.100.200(Alibaba metadata) to align with the broader coverage inWebClientUtilsTest. This strengthens the integration-level guarantee.♻️ Suggested expanded test vectors
`@ParameterizedTest` -@ValueSource(strings = {"127.0.0.1", "169.254.169.254", "localhost"}) +@ValueSource(strings = {"127.0.0.1", "169.254.169.254", "localhost", "0.0.0.0", "168.63.129.16", "100.100.100.200"}) public void sendTestEmail_WhenBlockedHost_ThrowsException(String host) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java` around lines 406 - 417, The parameterized test sendTestEmail_WhenBlockedHost_ThrowsException currently only checks {"127.0.0.1", "169.254.169.254", "localhost"} and should be expanded to include additional metadata/any-local hosts to improve SSRF coverage; update the `@ValueSource` on the test (and keep assertions on envManager.sendTestEmail and buildDto unchanged) to add "0.0.0.0", "168.63.129.16", and "100.100.100.200" so the test covers Azure, Alibaba, and any-local vectors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java`:
- Around line 90-95: The test resolveIfAllowed_returnsResolvedAddress assumes
IPv4; update it so WebClientUtils.resolveIfAllowed("smtp.gmail.com") is asserted
present and the returned InetAddress (result.get().getHostAddress()) is
validated for either IPv4 or IPv6—for example keep the existing IPv4 regex OR
accept addresses containing ':' (IPv6) or use a standard InetAddress validation
method—so the assertion passes regardless of whether the resolver returns IPv4
or IPv6.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java`:
- Around line 406-417: The parameterized test
sendTestEmail_WhenBlockedHost_ThrowsException currently only checks
{"127.0.0.1", "169.254.169.254", "localhost"} and should be expanded to include
additional metadata/any-local hosts to improve SSRF coverage; update the
`@ValueSource` on the test (and keep assertions on envManager.sendTestEmail and
buildDto unchanged) to add "0.0.0.0", "168.63.129.16", and "100.100.100.200" so
the test covers Azure, Alibaba, and any-local vectors.
🪄 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: 8a5e2d63-b5c2-42eb-9345-11184c43d151
📒 Files selected for processing (4)
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-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java
- app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java
Comment on lines
+90
to
+95
| @Test | ||
| public void resolveIfAllowed_returnsResolvedAddress() { | ||
| Optional<InetAddress> result = WebClientUtils.resolveIfAllowed("smtp.gmail.com"); | ||
| assertTrue(result.isPresent()); | ||
| assertTrue(result.get().getHostAddress().matches("\\d+\\.\\d+\\.\\d+\\.\\d+")); | ||
| } |
Contributor
There was a problem hiding this comment.
IPv4-only regex may fail for IPv6-capable resolvers.
The regex \\d+\\.\\d+\\.\\d+\\.\\d+ assumes IPv4. If smtp.gmail.com resolves to an IPv6 address (Google supports both), this assertion fails.
🛡️ Proposed fix to handle both IPv4 and IPv6
`@Test`
public void resolveIfAllowed_returnsResolvedAddress() {
Optional<InetAddress> result = WebClientUtils.resolveIfAllowed("smtp.gmail.com");
assertTrue(result.isPresent());
- assertTrue(result.get().getHostAddress().matches("\\d+\\.\\d+\\.\\d+\\.\\d+"));
+ // Just verify we got a valid address string - could be IPv4 or IPv6
+ String hostAddress = result.get().getHostAddress();
+ assertTrue(hostAddress != null && !hostAddress.isEmpty(), "Expected non-empty host address");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java`
around lines 90 - 95, The test resolveIfAllowed_returnsResolvedAddress assumes
IPv4; update it so WebClientUtils.resolveIfAllowed("smtp.gmail.com") is asserted
present and the returned InetAddress (result.get().getHostAddress()) is
validated for either IPv4 or IPv6—for example keep the existing IPv4 regex OR
accept addresses containing ':' (IPv6) or use a standard InetAddress validation
method—so the assertion passes regardless of whether the resolver returns IPv4
or IPv6.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
TL;DR: The
POST /api/v1/admin/send-test-emailendpoint was vulnerable to SSRF (CWE-918) and error-based information disclosure (CWE-209). Attacker-controlledsmtpHost/smtpPortwere passed directly to JavaMail, bypassing theWebClientUtils.IP_CHECK_FILTERthat only protects HTTP requests. This fix adds host validation and sanitizes error messages.Root Cause
EnvManagerCEImpl.sendTestEmail()accepted user-controlled SMTP host/port and established raw TCP connections viaJavaMailSenderImplwithout any IP validation. The existingWebClientUtils.IP_CHECK_FILTERonly applies to Spring WebClient HTTP requests — a completely separate code path from JavaMail SMTP.Changes
WebClientUtils.validateHostNotDisallowed()— new reusable method that checks a hostname against the existing cloud-metadata denylist, resolves it via DNS, and rejects loopback, link-local, site-local, any-local, and multicast addresses.EnvManagerCEImpl.sendTestEmail()— calls the new validation before connecting. Error messages fromMessagingException/MailExceptionare no longer returned verbatim to the caller, preventing error-based port scanning.WebClientUtilsTestfor blocked hosts (private ranges, metadata IPs, localhost, unresolvable) and allowed hosts (legitimate SMTP servers). Added SSRF regression tests inEnvManagerTest.Fixes https://linear.app/appsmith/issue/APP-15034/ssrf-via-post-apiv1adminsend-test-email-javamail-bypasses-webclient-ip
Advisory: GHSA-vvxf-f8q9-86gh
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/23677660193
Commit: 524cc1b
Cypress dashboard.
Tags:
@tag.AllSpec:
Sat, 28 Mar 2026 08:17:19 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests