fix: use hostname for SMTP TLS verification while pinning the connection to the SSRF-validated address - #42060
Conversation
…ted address The SSRF fix for GHSA-vvxf-f8q9-86gh made sendTestEmail connect to the DNS-resolved IP. TLS certificate identity checks run against the connect host, and server certificates carry hostnames, so every TLS mode failed with "SSLHandshakeException: No subject alternative names matching IP address" while plaintext SMTP kept working (APP-15713). buildMailSender now sets the sender host to the user-entered hostname — restoring correct SNI and certificate identity checking — while pinning the TCP connection to the RestrictedHostFilter-validated address through a custom mail.smtp.socketFactory. Angus Mail asks the factory for an unconnected socket and connects it to (host, port) itself; the pinned socket ignores the re-resolved endpoint address, so the DNS-rebinding TOCTOU the security fix closed stays closed. The implicit-SSL path on port 465 is pinned via Angus's plain-socketFactory fallback, and fails closed (handshake-first identity check against the hostname) should that fallback ever stop applying. mail.smtp.socketFactory.fallback is set to false explicitly so a factory failure can never silently unpin. Fixes #42024 Linear: https://linear.app/appsmith/issue/APP-15713 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
Bug
WalkthroughSMTP test-email sender construction now preserves the configured hostname for TLS while pinning connections to the SSRF-validated address. ChangesSMTP connection pinning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant sendTestEmail
participant RestrictedHostFilter
participant buildMailSender
participant JavaMailSenderImpl
sendTestEmail->>RestrictedHostFilter: resolveIfAllowed SMTP hostname
RestrictedHostFilter-->>sendTestEmail: return validated address
sendTestEmail->>buildMailSender: provide request and resolved address
buildMailSender->>JavaMailSenderImpl: configure hostname and pinned socket factory
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 |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/30324969679. |
|
Deploy-Preview-URL: https://ce-42060.dp.appsmith.com |
![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
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java (2)
844-859: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOnly a read timeout is set — no connect timeout.
mail.smtp.timeoutbounds the read timeout, butmail.smtp.connectiontimeout(andmail.smtp.writetimeout) aren't set. If the pinned address is reachable but silently drops packets (firewalled host, blackholed route), the connect phase can block far longer than 7s, tying up the calling thread on this synchronoustestConnection()/send()path.♻️ Proposed fix
props.put("mail.smtp.timeout", 7000); // 7 seconds + props.put("mail.smtp.connectiontimeout", 7000); // 7 seconds + props.put("mail.smtp.writetimeout", 7000); // 7 seconds🤖 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-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java` around lines 844 - 859, Update the SMTP properties setup in the EnvManagerCEImpl mail configuration to set connection and write timeouts alongside the existing mail.smtp.timeout, using the same 7-second bound. Preserve the current protocol, TLS, socket-factory, and fallback settings.
846-849: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSecurity Misconfiguration (CWE-297)
Reachability: Internal
Set
mail.smtp.ssl.checkserveridentityexplicitly.This path relies on TLS server-ID validation, but the fallback comment still describes the library default as false. Explicitly setting it to
"true"removes reliance on a default that changed between JavaMail/Jakarta Mail and Angus Mail.🤖 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-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java` around lines 846 - 849, Update the SMTP properties setup near pinnedSocketFactory to explicitly set mail.smtp.ssl.checkserveridentity to "true", alongside the existing socketFactory and fallback properties, ensuring TLS server identity validation does not depend on library defaults.
🤖 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-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java`:
- Around line 861-868: Remove the unconditional `props.put("mail.debug",
"true")` from the mail configuration in `EnvManagerCEImpl`; keep SMTP debug
logging disabled by default so `sendTestEmail` cannot emit authentication
credentials, while preserving the existing authentication property setup.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java`:
- Around line 844-859: Update the SMTP properties setup in the EnvManagerCEImpl
mail configuration to set connection and write timeouts alongside the existing
mail.smtp.timeout, using the same 7-second bound. Preserve the current protocol,
TLS, socket-factory, and fallback settings.
- Around line 846-849: Update the SMTP properties setup near pinnedSocketFactory
to explicitly set mail.smtp.ssl.checkserveridentity to "true", alongside the
existing socketFactory and fallback properties, ensuring TLS server identity
validation does not depend on library defaults.
🪄 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 Plus
Run ID: 97d394f9-cde8-4a06-935f-127eced54dd8
📒 Files selected for processing (2)
app/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
…d mail.debug Per CodeRabbit review: - Set mail.smtp.connectiontimeout and mail.smtp.writetimeout (7s) so a blackholed connect can't hold the calling thread indefinitely (the existing mail.smtp.timeout only bounds reads). - Set mail.smtp.ssl.checkserveridentity=true explicitly instead of relying on the library default, which differed across JavaMail/Angus Mail generations. - Gate mail.debug on the logger's DEBUG level instead of hardcoding true. Note: Angus already suppresses the AUTH exchange in the debug trace by default (mail.debug.auth=false), so this gates transcript noise (banners, recipient, username), not the password. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/30400420076. |
|
Deploy-Preview-URL: https://ce-42060.dp.appsmith.com |
2 participants
Description
Problem (APP-15713 / #42024): SMTP test emails from Admin Settings fail whenever TLS is enabled — STARTTLS on 587/2525 and implicit SSL on 465 — with:
Plaintext SMTP (e.g. 2525 with TLS off) works, which matches the reporter's port matrix exactly.
Root cause: the SSRF fix for GHSA-vvxf-f8q9-86gh (#41666) made
EnvManagerCEImpl.sendTestEmailconnect to the DNS-resolved IP (setHost(resolvedAddress.getHostAddress())) to close a DNS-rebinding TOCTOU. But JavaMail performs SNI and certificate identity checking against the connect host, and server certificates carry hostnames, not IPs — so every TLS handshake fails.Fix:
buildMailSendernow sets the sender host to the user-entered hostname (restoring correct SNI + certificate identity checking) while pinning the TCP connection to theRestrictedHostFilter.resolveIfAllowed-validated address via a custommail.smtp.socketFactory. Angus Mail asks the factory for an unconnected socket and connects it to(host, port)itself; the pinned socket ignores the re-resolved endpoint address and connects to the validated one. Both security properties of the original fix are preserved:mail.smtp.ssl.socketFactoryconfigured, Angus falls back to the plainmail.smtp.socketFactoryfor the TCP connect and layers TLS over the already-connected socket (verified against Angus Mail 2.0.5SocketFetcherbytecode). If that fallback ever stops applying, the path still fails closed — it is handshake-first withmail.smtp.ssl.checkserveridentityon by default, so a rebound DNS answer dies at certificate verification before any SMTP dialogue.mail.smtp.socketFactory.fallback=falseis set explicitly so a factory failure can never silently fall back to an unpinned socket.The superuser-only authorization, port allowlist, and
RestrictedHostFiltervalidation are unchanged and still run before any socket is built.Tests: 6 new unit tests in
EnvManagerTest— hostname-based host for TLS (fails on the old IP-connecting code), 465 implicit-SSL props, STARTTLS-disabled props, auth on/off props, and socket-factory pinning (a factory socket pointed at an unroutable TEST-NET endpoint must land on the validated listener, for both the unconnected-then-connect path JavaMail uses and the pre-connected overloads).mvn test -Dtest=EnvManagerTest: 29/29 pass.Maintainer note: the pinning relies on Angus Mail's socket-factory calling convention (documented in the
buildMailSenderjavadoc). When upgrading the mail library, re-verify thatSocketFetcherstill consultsmail.smtp.socketFactoryand connects the factory's unconnected socket itself.Manual verification
Verified on a live deploy preview of this PR via Admin Settings → Email → "Send test email", against real providers:
169.254.169.254(SSRF guard)Additionally verified off-DP by driving the real Angus Mail stack through
buildMailSenderagainst livesmtp.gmail.com: the 587-STARTTLS and 465-implicit-SSL handshakes succeed with this fix, and rebuilding the sender the pre-fix way (host = resolved IP) reproduces the exact reported failure (Could not convert socket to TLS) — same machine, same server, hostname-vs-IP being the only variable.Note for anyone re-testing: Mailtrap's sandbox does not speak implicit TLS on port 465 (it greets in plaintext on that port), so 465 must be verified with a provider that does, e.g. Brevo or Gmail. That failure mode is a provider quirk, not this code path.
Fixes #42024
Linear: https://linear.app/appsmith/issue/APP-15713
Automation
/ok-to-test tags="@tag.All"
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/30640681435
Commit: d5301eb
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 31 Jul 2026 16:05:29 UTC