-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(security): harden server-side SSRF egress filtering across plugins #41962
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,6 +146,149 @@ public void isBlockedIpAddressClass_doesNotMatchOtherHosts(String host) { | |
| "Did not expect " + host + " to be recognized as a blocked address class"); | ||
| } | ||
|
|
||
| // GHSA-7wfp-6c63-99gq / GHSA-66gg-xpjf-p92v / GHSA-4fjr-w826-cpwm: | ||
| // Link-local (incl. non-listed IMDS ranges), any-local, multicast, and IPv6 ULA addresses have | ||
| // no legitimate datasource use, so they must be blocked on every deployment — not only under | ||
| // IN_DOCKER as before. isAlwaysBlockedAddressClass is deployment-independent. | ||
| @ParameterizedTest | ||
| @ValueSource( | ||
| strings = { | ||
| // Link-local IPv4 (169.254.0.0/16) beyond the explicitly listed metadata IPs | ||
| "169.254.1.1", | ||
| "169.254.0.1", | ||
| // Link-local IPv6 | ||
| "fe80::1", | ||
| // Any-local | ||
| "0.0.0.0", | ||
| "::", | ||
| // Multicast | ||
| "224.0.0.1", | ||
| "239.255.255.250", | ||
| "ff02::1", | ||
| // IPv6 ULA (fc00::/7) | ||
| "fc00::1", | ||
| "fd00::1", | ||
| }) | ||
| public void isAlwaysBlockedAddressClass_recognizesNonRoutableClasses(String host) { | ||
| assertTrue( | ||
| WebClientUtils.isAlwaysBlockedAddressClass(host), | ||
| "Expected " + host + " to be an always-blocked address class"); | ||
| } | ||
|
|
||
| // Loopback and RFC1918 are NOT in the always-blocked set: loopback stays reachable on non-Docker | ||
| // deployments (and localhost-backed datasources / MockWebServer tests keep working), and RFC1918 | ||
| // stays reachable so self-hosted internal datasources are unaffected. No regression by default. | ||
| @ParameterizedTest | ||
| @ValueSource(strings = {"127.0.0.1", "::1", "10.0.0.1", "192.168.1.1", "172.16.0.1", "1.1.1.1", "8.8.8.8"}) | ||
| public void isAlwaysBlockedAddressClass_doesNotMatchLoopbackPrivateOrPublic(String host) { | ||
| assertFalse( | ||
| WebClientUtils.isAlwaysBlockedAddressClass(host), | ||
| "Did not expect " + host + " to be an always-blocked address class"); | ||
| } | ||
|
|
||
| // The HTTP egress enforcement points (isDisallowedAndFail / requestFilterFn) must reject the | ||
| // always-blocked classes even when IN_DOCKER is unset (the historical gap this batch closes). | ||
| @ParameterizedTest | ||
| @ValueSource(strings = {"169.254.1.1", "fe80::1", "0.0.0.0", "224.0.0.1", "fc00::1"}) | ||
| public void isDisallowedAndFail_blocksAlwaysBlockedClassesRegardlessOfDocker(String host) { | ||
| assertTrue( | ||
| WebClientUtils.isDisallowedAndFail(host, null), | ||
| "Expected " + host + " to be blocked on all deployments"); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @ValueSource( | ||
| strings = { | ||
| "http://169.254.1.1/latest/meta-data", | ||
| "http://[fe80::1]/", | ||
| "http://0.0.0.0/", | ||
| "http://224.0.0.1/", | ||
| }) | ||
| public void requestFilterFn_blocksAlwaysBlockedClassesRegardlessOfDocker(String url) throws Exception { | ||
| StepVerifier.create(invokeRequestFilterFn(url)) | ||
| .expectErrorSatisfies(throwable -> { | ||
| assertTrue(throwable instanceof UnknownHostException); | ||
| assertEquals(WebClientUtils.HOST_NOT_ALLOWED, throwable.getMessage()); | ||
| }) | ||
| .verify(); | ||
| } | ||
|
|
||
| // Default posture (no IN_DOCKER, strict-egress flag off): loopback and RFC1918 remain reachable | ||
| // so existing self-hosted internal datasources and local dev keep working (no regression). | ||
| @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)"); | ||
| } | ||
|
Comment on lines
+218
to
+224
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 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 🤖 Prompt for AI Agents |
||
|
|
||
| // resolveForDatasource is the datasource-facing resolver used by non-HTTP plugins that connect | ||
| // outside the WebClient pipeline (e.g. SMTP via JavaMail). It blocks the cloud-metadata denylist | ||
| // and the always-blocked address classes on every deployment, but allows loopback and RFC1918 by | ||
| // default so self-hosted mail servers on private networks (and Testcontainers hosts that resolve | ||
| // to loopback on some Docker setups) keep working. It returns the resolved address so callers can | ||
| // pin the connection and defeat DNS-rebinding. | ||
| @ParameterizedTest | ||
| @ValueSource(strings = {"169.254.169.254", "169.254.1.1", "0.0.0.0", "224.0.0.1", "metadata.google.internal"}) | ||
| public void resolveForDatasource_blocksMetadataAndAlwaysBlockedClasses(String host) { | ||
| assertTrue( | ||
| WebClientUtils.resolveForDatasource(host).isEmpty(), | ||
| "Expected datasource host " + host + " to be blocked"); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @ValueSource(strings = {"127.0.0.1", "10.0.0.1", "192.168.1.1", "172.16.0.1"}) | ||
| public void resolveForDatasource_allowsLoopbackAndPrivateByDefault(String host) { | ||
| assertTrue( | ||
| WebClientUtils.resolveForDatasource(host).isPresent(), | ||
| "Expected datasource host " + host + " to be allowed by default"); | ||
| } | ||
|
|
||
| @Test | ||
| public void resolveForDatasource_blocksNullAndEmpty() { | ||
| assertTrue(WebClientUtils.resolveForDatasource(null).isEmpty()); | ||
| assertTrue(WebClientUtils.resolveForDatasource(" ").isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| public void resolveForDatasource_returnsResolvedAddressForPublicHost() { | ||
| Optional<InetAddress> result = WebClientUtils.resolveForDatasource("smtp.gmail.com"); | ||
| assertTrue(result.isPresent()); | ||
| } | ||
|
|
||
| // Strict egress mode (APPSMITH_SSRF_BLOCK_PRIVATE_ADDRESS=true) additionally blocks loopback and | ||
| // RFC1918 on all deployments. Exercised through the pure overload so no process env is mutated. | ||
| @Test | ||
| public void isBlockedResolvedAddress_strictMode_blocksLoopbackAndPrivate() throws Exception { | ||
| assertTrue(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("127.0.0.1"), false, true)); | ||
| assertTrue(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("10.0.0.1"), false, true)); | ||
| assertTrue(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("192.168.1.1"), false, true)); | ||
| assertTrue(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("172.16.0.1"), false, true)); | ||
| } | ||
|
|
||
| @Test | ||
| public void isBlockedResolvedAddress_defaultMode_allowsLoopbackAndPrivate() throws Exception { | ||
| assertFalse(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("127.0.0.1"), false, false)); | ||
| assertFalse(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("10.0.0.1"), false, false)); | ||
| assertFalse(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("192.168.1.1"), false, false)); | ||
| } | ||
|
|
||
| @Test | ||
| public void isBlockedResolvedAddress_dockerMode_blocksLoopbackButNotPrivate() throws Exception { | ||
| // In Docker, loopback is blocked but RFC1918 stays reachable (sibling containers/services). | ||
| assertTrue(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("127.0.0.1"), true, false)); | ||
| assertFalse(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("10.0.0.1"), true, false)); | ||
| } | ||
|
|
||
| @Test | ||
| public void isBlockedResolvedAddress_alwaysBlocksNonRoutableClassesInEveryMode() throws Exception { | ||
| assertTrue(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("169.254.1.1"), false, false)); | ||
| assertTrue(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("fe80::1"), false, false)); | ||
| assertTrue(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("224.0.0.1"), false, false)); | ||
| assertTrue(WebClientUtils.isBlockedResolvedAddress(InetAddress.getByName("fc00::1"), false, false)); | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private Mono<ClientRequest> invokeRequestFilterFn(String url) throws Exception { | ||
| final Method method = WebClientUtils.class.getDeclaredMethod("requestFilterFn", ClientRequest.class); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
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:
SMTP still needs IP pinning or post-connect validation
SmtpPlugin.datasourceCreatevalidatesendpoint.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 resolvedInetAddress, or validate the remote peer afterconnect().🤖 Prompt for AI Agents
Source: Learnings