feat: block SMTP connections to private/reserved network addresses#1480
feat: block SMTP connections to private/reserved network addresses#1480N2D4 wants to merge 5 commits into
Conversation
Add SSRF protection for custom email server configs. Before connecting to a user-configured SMTP host, resolve its IP via public DNS and reject any address in private, loopback, link-local, CGNAT, multicast, or other reserved ranges. Shared/platform-managed configs are exempt. - Add isPrivateOrReservedIp() to stack-shared (IPv4 + IPv6 + IPv4-mapped) - Add ensureSmtpHostNotPrivateNetwork() in emails-low-level.tsx - Returns a clear PRIVATE_NETWORK error type with user-facing message Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds IPv4/IPv6 parsing and a public-DNS-backed resolver that validates SMTP targets to block private/reserved addresses; integrates this as ChangesSSRF Protection for SMTP Email Sending
Sequence Diagram(s)sequenceDiagram
participant BackendApp
participant PublicDNS as PublicDNS(1.1.1.1/8.8.8.8)
participant SMTPServer
BackendApp->>PublicDNS: resolve A/AAAA for hostname
PublicDNS-->>BackendApp: return addresses or DNS error
BackendApp->>BackendApp: isPrivateOrReservedIp(addresses) checks
BackendApp->>SMTPServer: connect to resolvedHost (IP) with TLS servername
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR adds SSRF protection for custom SMTP configurations by resolving the configured host via public DNS resolvers (Cloudflare 1.1.1.1 and Google 8.8.8.8) and rejecting any address that falls in a private or reserved range, along with a shared
Confidence Score: 3/5The SSRF protection has two exploitable holes that could allow a crafted SMTP host to reach internal infrastructure in containerized deployments. The apps/backend/src/lib/emails-low-level.tsx — the DNS-check logic around the empty-result return and the resolver used for the actual connection both need attention.
|
| Filename | Overview |
|---|---|
| apps/backend/src/lib/emails-low-level.tsx | Adds SSRF protection before SMTP connections for custom configs; two bypasses exist: empty public-DNS result silently allows internal hostnames through, and the check uses different DNS resolvers than nodemailer does at connection time. |
| packages/stack-shared/src/utils/ips.tsx | Adds isPrivateOrReservedIp covering all major IPv4 and IPv6 reserved ranges with inline tests; IPv4-compatible IPv6 addresses (::x.x.x.x) are not blocked, which is a minor gap since that format is deprecated. |
Sequence Diagram
sequenceDiagram
participant Caller
participant ensureSmtpHostNotPrivateNetwork
participant PublicDNS as Public DNS (1.1.1.1 / 8.8.8.8)
participant isPrivateOrReservedIp
participant Nodemailer
participant SystemDNS as System DNS (container/internal)
participant SMTPServer
Caller->>ensureSmtpHostNotPrivateNetwork: host (string)
alt host is raw IP literal
ensureSmtpHostNotPrivateNetwork->>isPrivateOrReservedIp: check IP directly
isPrivateOrReservedIp-->>ensureSmtpHostNotPrivateNetwork: true/false
alt is private
ensureSmtpHostNotPrivateNetwork-->>Caller: throw StackAssertionError
end
else host is a hostname
ensureSmtpHostNotPrivateNetwork->>PublicDNS: resolve4(host) + resolve6(host)
PublicDNS-->>ensureSmtpHostNotPrivateNetwork: IP list (or empty on NXDOMAIN)
alt allAddrs is empty ⚠️ bypass
ensureSmtpHostNotPrivateNetwork-->>Caller: return (no check!)
else addresses returned
loop each resolved IP
ensureSmtpHostNotPrivateNetwork->>isPrivateOrReservedIp: check addr
isPrivateOrReservedIp-->>ensureSmtpHostNotPrivateNetwork: true/false
alt is private
ensureSmtpHostNotPrivateNetwork-->>Caller: throw StackAssertionError
end
end
ensureSmtpHostNotPrivateNetwork-->>Caller: ok
end
end
Caller->>Nodemailer: sendMail (uses system DNS ⚠️ different resolver)
Nodemailer->>SystemDNS: resolve host
SystemDNS-->>Nodemailer: IP (may differ from public DNS result)
Nodemailer->>SMTPServer: TCP connect
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/backend/src/lib/emails-low-level.tsx:67-71
**SSRF bypass via empty public-DNS result**
When both `resolver.resolve4` and `resolver.resolve6` return empty results, the function returns without blocking. This is exploitable in containerized deployments (Docker, Kubernetes) where internal service hostnames (`postgres`, `redis`, `api`, `metadata.google.internal`, etc.) are resolvable via the container's internal DNS but invisible to 1.1.1.1/8.8.8.8. An attacker with a custom SMTP config can set the host to one of these names: the public resolvers return NXDOMAIN → `allAddrs.length === 0` → check passes → nodemailer uses the system resolver and connects to the private IP.
### Issue 2 of 3
apps/backend/src/lib/emails-low-level.tsx:59-65
**Split-horizon DNS / DNS rebinding TOCTOU**
The check resolves the hostname via Cloudflare (1.1.1.1) and Google (8.8.8.8), but nodemailer uses whatever DNS the system is configured with. In a split-horizon setup, the same hostname can legitimately resolve to a public IP via the public resolvers while resolving to a private IP via the internal resolver — the check passes while the actual connection goes to the internal host. Additionally, an attacker who controls their domain's authoritative DNS can set a very short TTL, have the first query (used here) return a public IP, then flip to a private IP for the second query that nodemailer issues, exploiting the TOCTOU window.
### Issue 3 of 3
packages/stack-shared/src/utils/ips.tsx:78-109
**IPv4-compatible IPv6 addresses (`::IPv4`) not blocked**
The function correctly handles IPv4-*mapped* addresses (`::ffff:x.x.x.x`, groups[5] = 0xffff), but not the deprecated IPv4-*compatible* form (`::x.x.x.x`, groups[5] = 0). `parseIpv6Groups("::192.168.1.1")` produces `[0,0,0,0,0,0,0xc0a8,0x0101]`: groups[5] is `0`, not `0xffff`, so the IPv4-embedded check is skipped and the function returns `false` despite mapping to a private address. While IPv4-compatible addresses are deprecated and most modern OS stacks won't route them, some systems still process them, and the `ipRegex` library does accept them as valid IPv6 addresses.
Reviews (1): Last reviewed commit: "feat: block SMTP connections to private/..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (allAddrs.length === 0) { | ||
| // DNS returned no records — the host doesn't exist. | ||
| // Let nodemailer's own DNS timeout / EDNS handling deal with this. | ||
| return; | ||
| } |
There was a problem hiding this comment.
When both resolver.resolve4 and resolver.resolve6 return empty results, the function returns without blocking. This is exploitable in containerized deployments (Docker, Kubernetes) where internal service hostnames (postgres, redis, api, metadata.google.internal, etc.) are resolvable via the container's internal DNS but invisible to 1.1.1.1/8.8.8.8. An attacker with a custom SMTP config can set the host to one of these names: the public resolvers return NXDOMAIN → allAddrs.length === 0 → check passes → nodemailer uses the system resolver and connects to the private IP.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/lib/emails-low-level.tsx
Line: 67-71
Comment:
**SSRF bypass via empty public-DNS result**
When both `resolver.resolve4` and `resolver.resolve6` return empty results, the function returns without blocking. This is exploitable in containerized deployments (Docker, Kubernetes) where internal service hostnames (`postgres`, `redis`, `api`, `metadata.google.internal`, etc.) are resolvable via the container's internal DNS but invisible to 1.1.1.1/8.8.8.8. An attacker with a custom SMTP config can set the host to one of these names: the public resolvers return NXDOMAIN → `allAddrs.length === 0` → check passes → nodemailer uses the system resolver and connects to the private IP.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Fixed in 37b8955 — public DNS returning no records now throws instead of allowing through.
| const resolver = new dns.Resolver(); | ||
| resolver.setServers(["1.1.1.1", "8.8.8.8"]); | ||
| const [ipv4Addrs, ipv6Addrs] = await Promise.all([ | ||
| resolver.resolve4(host).catch(() => [] as string[]), | ||
| resolver.resolve6(host).catch(() => [] as string[]), | ||
| ]); | ||
| const allAddrs = [...ipv4Addrs, ...ipv6Addrs]; |
There was a problem hiding this comment.
The check resolves the hostname via Cloudflare (1.1.1.1) and Google (8.8.8.8), but nodemailer uses whatever DNS the system is configured with. In a split-horizon setup, the same hostname can legitimately resolve to a public IP via the public resolvers while resolving to a private IP via the internal resolver — the check passes while the actual connection goes to the internal host. Additionally, an attacker who controls their domain's authoritative DNS can set a very short TTL, have the first query (used here) return a public IP, then flip to a private IP for the second query that nodemailer issues, exploiting the TOCTOU window.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/lib/emails-low-level.tsx
Line: 59-65
Comment:
**Split-horizon DNS / DNS rebinding TOCTOU**
The check resolves the hostname via Cloudflare (1.1.1.1) and Google (8.8.8.8), but nodemailer uses whatever DNS the system is configured with. In a split-horizon setup, the same hostname can legitimately resolve to a public IP via the public resolvers while resolving to a private IP via the internal resolver — the check passes while the actual connection goes to the internal host. Additionally, an attacker who controls their domain's authoritative DNS can set a very short TTL, have the first query (used here) return a public IP, then flip to a private IP for the second query that nodemailer issues, exploiting the TOCTOU window.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 37b8955 — resolveAndValidateSmtpHost now returns the resolved IP which is passed directly to nodemailer's host (with tls.servername set to the original hostname for SNI). Since nodemailer skips its own DNS resolution when host is already an IP, the TOCTOU window is eliminated.
| function isPrivateOrReservedIpv6(groups: number[]): boolean { | ||
| // :: (unspecified) — all zeros | ||
| if (groups.every(g => g === 0)) return true; | ||
|
|
||
| // ::1 (loopback) | ||
| if (groups.slice(0, 7).every(g => g === 0) && groups[7] === 1) return true; | ||
|
|
||
| // ::ffff:0:0/96 — IPv4-mapped; check the embedded IPv4 part | ||
| if (groups.slice(0, 5).every(g => g === 0) && groups[5] === 0xffff) { | ||
| const a = (groups[6]! >> 8) & 0xff; | ||
| const b = groups[6]! & 0xff; | ||
| const c = (groups[7]! >> 8) & 0xff; | ||
| const d = groups[7]! & 0xff; | ||
| return isPrivateOrReservedIpv4([a, b, c, d]); | ||
| } | ||
|
|
||
| // fc00::/7 — unique local address (ULA) | ||
| if ((groups[0]! & 0xfe00) === 0xfc00) return true; | ||
|
|
||
| // fe80::/10 — link-local | ||
| if ((groups[0]! & 0xffc0) === 0xfe80) return true; | ||
|
|
||
| // ff00::/8 — multicast | ||
| if ((groups[0]! & 0xff00) === 0xff00) return true; | ||
|
|
||
| // 100::/64 — discard prefix | ||
| if (groups[0] === 0x0100 && groups.slice(1, 4).every(g => g === 0)) return true; | ||
|
|
||
| // 2001:db8::/32 — documentation | ||
| if (groups[0] === 0x2001 && groups[1] === 0x0db8) return true; | ||
|
|
||
| return false; |
There was a problem hiding this comment.
::IPv4) not blocked
The function correctly handles IPv4-mapped addresses (::ffff:x.x.x.x, groups[5] = 0xffff), but not the deprecated IPv4-compatible form (::x.x.x.x, groups[5] = 0). parseIpv6Groups("::192.168.1.1") produces [0,0,0,0,0,0,0xc0a8,0x0101]: groups[5] is 0, not 0xffff, so the IPv4-embedded check is skipped and the function returns false despite mapping to a private address. While IPv4-compatible addresses are deprecated and most modern OS stacks won't route them, some systems still process them, and the ipRegex library does accept them as valid IPv6 addresses.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/stack-shared/src/utils/ips.tsx
Line: 78-109
Comment:
**IPv4-compatible IPv6 addresses (`::IPv4`) not blocked**
The function correctly handles IPv4-*mapped* addresses (`::ffff:x.x.x.x`, groups[5] = 0xffff), but not the deprecated IPv4-*compatible* form (`::x.x.x.x`, groups[5] = 0). `parseIpv6Groups("::192.168.1.1")` produces `[0,0,0,0,0,0,0xc0a8,0x0101]`: groups[5] is `0`, not `0xffff`, so the IPv4-embedded check is skipped and the function returns `false` despite mapping to a private address. While IPv4-compatible addresses are deprecated and most modern OS stacks won't route them, some systems still process them, and the `ipRegex` library does accept them as valid IPv6 addresses.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 37b8955 — the IPv4-embedded check now also fires when groups[5] === 0 (IPv4-compatible), not just 0xffff (IPv4-mapped). Added tests for ::192.168.1.1, ::10.0.0.1, ::127.0.0.1, and ::8.8.8.8.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
- Block when public DNS returns no records (prevents internal-only hostname bypass) - Pass resolved IP directly to nodemailer with TLS servername for SNI (eliminates split-horizon DNS / TOCTOU rebinding gap) - Handle IPv4-compatible IPv6 addresses (::x.x.x.x) in addition to IPv4-mapped (::ffff:x.x.x.x) Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.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: 2
🤖 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 `@apps/backend/src/lib/emails-low-level.tsx`:
- Around line 67-71: The early-return when allAddrs.length === 0 allows
nodemailer to fall back to system DNS and enables split-horizon SSRF; change
this branch in emails-low-level.tsx so that when allAddrs is empty you throw a
clear error (or at minimum log a high‑severity warning and abort sending)
instead of returning, and ensure the mail-sending path (nodemailer usage) is
configured to use the same public resolvers if you must allow empty public
responses; update the conditional that checks allAddrs (the block that currently
returns) to explicitly error-out and include context about DNS resolution and
the target host to prevent silent fallback to system DNS.
In `@packages/stack-shared/src/utils/ips.tsx`:
- Around line 78-92: The IPv4-compatible IPv6 form (::a.b.c.d) isn't handled:
update isPrivateOrReservedIpv6 to add a branch that detects when the first 6
16-bit groups are zero (groups.slice(0,6).every(g => g === 0)) and then extract
the embedded IPv4 bytes from groups[6] and groups[7] (same byte extraction used
for the ::ffff branch) and pass [a,b,c,d] into isPrivateOrReservedIpv4; add
tests calling isPrivateOrReservedIp("::192.168.1.1"),
isPrivateOrReservedIp("::127.0.0.1") (expect true) and
isPrivateOrReservedIp("::8.8.8.8") (expect false) to cover the new behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6c4d109-a4cc-44e3-a485-687d6c27bf46
📒 Files selected for processing (2)
apps/backend/src/lib/emails-low-level.tsxpackages/stack-shared/src/utils/ips.tsx
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)
apps/backend/src/lib/emails-low-level.tsx (1)
72-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix DNS failure classification/retryability in SMTP host resolution
resolve4/resolve6failures are converted into[]via.catch(() => [])(so transient DNS/lookup errors are indistinguishable from “no public records”), and then the caller maps any thrown error fromresolveAndValidateSmtpHost()toResult.error({ errorType: 'PRIVATE_NETWORK', canRetry: false, ... }). This will misreport lookup failures as “private/reserved” and make them non-retryable when they should likely be retryable or at least classified as DNS/lookup issues.🤖 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 `@apps/backend/src/lib/emails-low-level.tsx` around lines 72 - 75, The current resolver.resolve4/resolve6 calls inside resolveAndValidateSmtpHost swallow all errors by using .catch(() => []), which makes transient DNS lookup failures indistinguishable from "no records" and causes the caller to map every thrown error to a non-retryable PRIVATE_NETWORK Result.error; change the logic so DNS resolution errors are not converted into an empty-array silently: remove the blanket .catch(() => []) and instead let resolution errors propagate (or catch and rethrow as a distinct DNS error type), only treat the case where both resolve4/resolve6 return empty arrays as the "no public records / private network" branch, and update the error-to-Result mapping so DNS/lookup errors produce a retryable or DNS-specific error result while PRIVATE_NETWORK remains reserved for validated private/reserved-address outcomes; locate symbols resolver.resolve4, resolver.resolve6, and resolveAndValidateSmtpHost to implement this behavior.
🧹 Nitpick comments (1)
apps/backend/src/lib/emails-low-level.tsx (1)
97-99: ⚡ Quick winReplace the non-null assertion on
allAddrs[0].This invariant is already enforced immediately above, so please keep it explicit instead of relying on
!. The repo convention here isallAddrs[0] ?? throwErr(...), which keeps the failure mode intact if this branch gets rearranged later.As per coding guidelines "Use defensive coding with
?? throwErr(...)over non-null assertions and fallback values; avoid silent fallbacks".🤖 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 `@apps/backend/src/lib/emails-low-level.tsx` around lines 97 - 99, Replace the non-null assertion on allAddrs[0] with the repo defensive pattern using the throw helper: return { resolvedHost: allAddrs[0] ?? throwErr("..."), servername: host }; specifically update the expression that builds the returned object (resolvedHost) in the same block where allAddrs is validated so it uses allAddrs[0] ?? throwErr(...) instead of allAddrs[0]!, referencing the existing throwErr helper to preserve the explicit failure mode if the invariant is ever violated.
🤖 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 `@apps/backend/src/lib/emails-low-level.tsx`:
- Around line 162-163: Remove the unused/incorrect SMTP client identifier by
dropping the transport option `name: smtpServername` and keep only `tls: {
servername: smtpServername }`; update the transport creation where `smtpHost`
and `smtpServername` are used (the object that currently contains `host:
smtpHost, ...(smtpServername ? { name: smtpServername, tls: { servername:
smtpServername } } : {})`) to instead conditionally include only
`tls.servername` when `smtpServername` is present so `name` is not set on the
Nodemailer transport.
---
Outside diff comments:
In `@apps/backend/src/lib/emails-low-level.tsx`:
- Around line 72-75: The current resolver.resolve4/resolve6 calls inside
resolveAndValidateSmtpHost swallow all errors by using .catch(() => []), which
makes transient DNS lookup failures indistinguishable from "no records" and
causes the caller to map every thrown error to a non-retryable PRIVATE_NETWORK
Result.error; change the logic so DNS resolution errors are not converted into
an empty-array silently: remove the blanket .catch(() => []) and instead let
resolution errors propagate (or catch and rethrow as a distinct DNS error type),
only treat the case where both resolve4/resolve6 return empty arrays as the "no
public records / private network" branch, and update the error-to-Result mapping
so DNS/lookup errors produce a retryable or DNS-specific error result while
PRIVATE_NETWORK remains reserved for validated private/reserved-address
outcomes; locate symbols resolver.resolve4, resolver.resolve6, and
resolveAndValidateSmtpHost to implement this behavior.
---
Nitpick comments:
In `@apps/backend/src/lib/emails-low-level.tsx`:
- Around line 97-99: Replace the non-null assertion on allAddrs[0] with the repo
defensive pattern using the throw helper: return { resolvedHost: allAddrs[0] ??
throwErr("..."), servername: host }; specifically update the expression that
builds the returned object (resolvedHost) in the same block where allAddrs is
validated so it uses allAddrs[0] ?? throwErr(...) instead of allAddrs[0]!,
referencing the existing throwErr helper to preserve the explicit failure mode
if the invariant is ever violated.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f36590e-890e-45a6-918a-677d6179e5c6
📒 Files selected for processing (2)
apps/backend/src/lib/emails-low-level.tsxpackages/stack-shared/src/utils/ips.tsx
- Distinguish transient DNS errors from NXDOMAIN/NODATA (retryable vs non-retryable) - Replace non-null assertion with ?? throwErr() defensive pattern - Remove unnecessary 'name' from nodemailer transport (keep only tls.servername) - Skip SSRF validation in dev/test environments (Inbucket uses localhost) Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/backend/src/lib/emails-low-level.tsx (1)
85-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an explicit timeout/abort around the public-DNS lookups.
These queries now sit on the send path, but this
Promise.allhas no local timeout or cancellation. If1.1.1.1/8.8.8.8stop responding, the request can hang well past the SMTP transport timeouts because those only apply after DNS resolution completes. Bound this with a resolver cancel / timeout wrapper so the retry logic can take over.Node.js dns.promises Resolver resolve4 resolve6 timeout cancel resolver.cancel official docs🤖 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 `@apps/backend/src/lib/emails-low-level.tsx` around lines 85 - 88, The DNS lookups using resolver.resolve4/resolve6 need a local timeout/abort so they can't hang; implement a small helper like resolveWithTimeout(resolver, methodName, host, ms) that starts the resolver call and also starts a timer which calls resolver.cancel() and rejects after ms, clears the timer on completion, then replace the Promise.all calls with Promise.all([resolveWithTimeout(resolver,'resolve4',host, DNS_TIMEOUT).catch(catchNoRecords), resolveWithTimeout(resolver,'resolve6',host, DNS_TIMEOUT).catch(catchNoRecords)]) so resolver.cancel() is invoked on timeout and the retry/transport logic can proceed. Ensure the helper forwards the original error if it occurs before timeout and cleans up timers to avoid leaks.
🤖 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 `@apps/backend/src/lib/emails-low-level.tsx`:
- Around line 163-178: Narrow the catch around resolveAndValidateSmtpHost():
only handle the expected DNS/resolution failures (e.g., the helper's known error
classes or DNS error codes such as ENOTFOUND, EAI_AGAIN, ETIMEDOUT, SERVFAIL)
and the existing StackAssertionError case, map those to the appropriate
Result.error responses (keeping the PRIVATE_NETWORK message for
StackAssertionError and using a retryable DNS_ERROR only for genuine transient
DNS errors), and rethrow any other unexpected exceptions so they fail loudly
instead of being masked as DNS failures; update the "no-public-DNS" branch to
return the more accurate user message returned by the helper rather than the
generic DNS message.
---
Duplicate comments:
In `@apps/backend/src/lib/emails-low-level.tsx`:
- Around line 85-88: The DNS lookups using resolver.resolve4/resolve6 need a
local timeout/abort so they can't hang; implement a small helper like
resolveWithTimeout(resolver, methodName, host, ms) that starts the resolver call
and also starts a timer which calls resolver.cancel() and rejects after ms,
clears the timer on completion, then replace the Promise.all calls with
Promise.all([resolveWithTimeout(resolver,'resolve4',host,
DNS_TIMEOUT).catch(catchNoRecords), resolveWithTimeout(resolver,'resolve6',host,
DNS_TIMEOUT).catch(catchNoRecords)]) so resolver.cancel() is invoked on timeout
and the retry/transport logic can proceed. Ensure the helper forwards the
original error if it occurs before timeout and cleans up timers to avoid leaks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ab2e163-bf05-440a-bda0-18f024a8b15b
📒 Files selected for processing (1)
apps/backend/src/lib/emails-low-level.tsx
- Add 7s timeout with resolver.cancel() around DNS lookups - Narrow catch to only StackAssertionError + known DNS error codes - Rethrow unexpected errors instead of masking as DNS_ERROR - Pass through StackAssertionError message for accurate user feedback Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
1 participant
SSRF protection for custom email server configurations. Before connecting to a user-configured SMTP host, resolves its IP addresses via public DNS (1.1.1.1, 8.8.8.8) and rejects any that fall in private, loopback, link-local, CGNAT, multicast, or other reserved ranges.
isPrivateOrReservedIp()in stack-shared (IPv4 + IPv6 + IPv4-mapped IPv6)ensureSmtpHostNotPrivateNetwork()in emails-low-level.tsxPRIVATE_NETWORKerror typeLink to Devin session: https://app.devin.ai/sessions/ba27568e90e44a57a856ba78455235b3
Requested by: @N2D4
Summary by CodeRabbit
Bug Fixes
Tests