fix(security): block Redis datasource from internal Appsmith Redis (GHSA-qhfj-g87x-m39w)#41921
Open
wyattwalter wants to merge 12 commits into
Open
fix(security): block Redis datasource from internal Appsmith Redis (GHSA-qhfj-g87x-m39w)#41921wyattwalter wants to merge 12 commits into
wyattwalter wants to merge 12 commits into
Conversation
…HSA-qhfj-g87x-m39w) The Redis datasource plugin opened a Jedis connection to a user-controlled host/port with no IP-class filtering, so a user with datasource-create permission could point a Redis datasource at the internal Appsmith Redis. The existing HTTP-side filter doesn't cover Redis — Jedis uses the JVM's default resolver and bypasses the Reactor Netty hook. Three changes: 1. Block Redis datasources from reaching the internal Appsmith Redis or non-routable addresses. RedisPlugin.datasourceCreate now consults a shared RestrictedHostFilter — blocks hostname / IP overlap with APPSMITH_REDIS_URL and APPSMITH_REDIS_GIT_URL, plus loopback / link-local / cloud-metadata. Enforcement is connection-time only; PluginExecutor.validateDatasource is documented format-only, so host-policy belongs on the connection path. UX: Save succeeds, Test Datasource rejects with "Host not allowed.". 2. Make SSRF protections always-on; rename IN_DOCKER to APPSMITH_DISABLE_SSRF_FILTER. The previous filter gated loopback / link-local on IN_DOCKER=1, which the official Docker image always sets, so users on documented deployment paths were already getting this protection. Closes the gap for unofficial distributions (self- built images, bare-metal, etc.) and replaces a poorly-named variable. The most likely place to hit this is a dev machine running the server directly against http://localhost:* — set APPSMITH_DISABLE_SSRF_FILTER=true to allow it. 3. Extract RestrictedHostFilter from WebClientUtils. Pure refactor — the host-filter logic had already grown several non-HTTP call sites.
Contributor
|
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:
WalkthroughIntroduces ChangesSSRF Filter Extraction and Plugin Wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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: 5
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/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java (1)
262-290: 🔒 Security & Privacy | 🟠 MajorPrevent DNS rebinding between assertHostAllowed and socket creation.
The hostname is validated in
assertHostAllowedbeforeJedisPoolconstruction, but the actual TCP connection occurs later whengetResource()is called. An attacker-controlled hostname that passes validation could resolve to an internal IP address at connection time. Since Jedis doesn't support pre-resolved IP injection, add post-connect validation immediately aftergetResource()to verify the remote address matches the checked hostname, or document the deliberate TOCTOU acceptance with risk justification.🤖 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/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java` around lines 262 - 290, The hostname is validated in `assertHostAllowed()` before JedisPool creation, but the actual TCP connection occurs later when `getResource()` is called on the JedisPool instance. An attacker-controlled hostname could resolve to a different IP address between validation and connection time. Locate all places where `getResource()` is called on the JedisPool and add post-connection validation immediately after to verify that the remote peer address matches the hostname that was validated in `assertHostAllowed()`, or alternatively document the deliberate Time-Of-Check-Time-Of-Use acceptance with clear risk justification if you decide to accept this DNS rebinding risk.Source: Learnings
🧹 Nitpick comments (2)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/RestrictedHostFilter.java (2)
338-355: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional:
resolveInternalRedisIpsre-resolves on everyisHostBlockedcall.Each Redis connection attempt triggers a fresh DNS lookup per internal Redis host. The internal hosts are effectively static, so a short-TTL cache (or one-time resolve with periodic refresh) would trim avoidable lookups on the connection path. Not urgent given this runs on
boundedElastic.🤖 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/RestrictedHostFilter.java` around lines 338 - 355, The resolveInternalRedisIps method performs DNS lookups repeatedly on every isHostBlocked call, which is inefficient since Redis hosts are static. Implement caching for the DNS resolution results by storing the resolved IPs in a cached field that is computed once during initialization or on first use, rather than recalculating them on each method invocation. Consider using a memoization approach or a class-level cache that stores the result of resolveInternalRedisIps so that subsequent calls to isHostBlocked can reuse the cached IP set instead of triggering fresh DNS lookups.
165-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize internal-Redis hosts the same way you compare them.
computeInternalRedisHosts/addInternalRedisHostFromEnv/setInternalRedisHostsForTestingstore hosts astrim().toLowerCase(Locale.ROOT)only, butisHostBlocked/isLiteralBlockedcompare against the fully canonicalized form (normalizeHostForComparison*— decorator-stripped, IP-normalized). Two mismatches fall out:
- An IPv6 redis URL yields a bracketed
uri.getHost()like[::1]; the literal compare misses ([::1]vs canonical0.0.0.1), andresolveInternalRedisIpsthen callsInetAddress.getAllByName("[::1]"), which throws and is silently skipped — so the IP-overlap path misses it too.- A trailing-dot or mixed-case literal host slips through the literal compare for the same reason.
Routing the stored values through
normalizeHostForComparisonQuietlykeeps both sides symmetric.♻️ Normalize on store
try { final URI uri = URI.create(url.trim()); final String host = uri.getHost(); if (StringUtils.hasText(host)) { - hosts.add(host.trim().toLowerCase(Locale.ROOT)); + hosts.add(normalizeHostForComparisonQuietly(host)); } } catch (IllegalArgumentException e) {final Set<String> normalized = new HashSet<>(hosts.length); for (String host : hosts) { if (StringUtils.hasText(host)) { - normalized.add(host.trim().toLowerCase(Locale.ROOT)); + normalized.add(normalizeHostForComparisonQuietly(host)); } }🤖 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/RestrictedHostFilter.java` around lines 165 - 201, The internal Redis host storage methods (computeInternalRedisHosts, addInternalRedisHostFromEnv, and setInternalRedisHostsForTesting) normalize hosts using only trim().toLowerCase(Locale.ROOT), but the comparison methods (isHostBlocked, isLiteralBlocked) use fully canonicalized normalization via normalizeHostForComparison*. This mismatch causes IPv6 addresses in brackets and other edge cases to fail comparisons. Apply normalizeHostForComparisonQuietly to all host values before storing them in the set across all three methods to ensure symmetric normalization between storage and comparison.
🤖 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/RestrictedHostFilter.java`:
- Around line 300-336: The isHostBlocked method validates a hostname once and
returns a boolean, but the caller then passes the original hostname string to
JedisPool, which re-resolves it independently, creating a DNS-rebinding window
where a hostile resolver can return different IPs between the two resolutions.
To fix this, modify the isHostBlocked method to return the resolved InetAddress
array instead of just a boolean, then update the caller in RedisPlugin's
assertHostAllowed to use the pre-resolved InetAddress when creating the Jedis
connection pool instead of the original hostname string. This ensures the
validated IP addresses are used and prevents re-resolution by Jedis.
In
`@app/server/appsmith-interfaces/src/test/java/com/appsmith/util/RestrictedHostFilterTest.java`:
- Around line 90-109: The tests resolveIfAllowed_allowsLegitimateSmtpHosts,
resolveIfAllowed_returnsResolvedAddress, and other test methods rely on real
public DNS names that require network resolution, making them flaky in
restricted environments. Replace the public SMTP hostnames in the `@ValueSource`
and test method calls with localhost or 127.0.0.1, which are guaranteed to
resolve without external dependencies. Additionally, update the IPv4-only regex
pattern in resolveIfAllowed_returnsResolvedAddress that matches
\d+\.\d+\.\d+\.\d+ to support both IPv4 and IPv6 address formats, or adjust the
assertion to be more flexible for loopback addresses.
In
`@app/server/appsmith-plugins/elasticSearchPlugin/src/test/java/com/external/plugins/ElasticSearchPluginTest.java`:
- Around line 541-546: The MockWebServer instance created in the redirect test
is started but never shut down, causing thread and socket leaks. Add a
shutdown() call for the mockWebServer in the finally block where SSRF state is
being reset to ensure proper cleanup. This applies to both redirect test cases
mentioned in the comment (the one at lines 541-546 and the similar one around
lines 614-618).
In
`@app/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.java`:
- Around line 2012-2018: The test is creating a new MockWebServer instance and
assigning it to the mockEndpoint variable, which overwrites the instance created
in the setUp() `@BeforeEach` method. This causes the original server to leak since
tearDown() only closes the replacement instance. Instead of reassigning
mockEndpoint, create a separate local MockWebServer variable with a different
name (for example, mockRedirectEndpoint) to hold this test-specific redirect
response server, and ensure both servers are properly managed and closed in the
test.
- Around line 1883-1885: The GCP metadata test method that follows this comment
block is not enforcing SSRF filter verification like the other
testDenyInstanceMetadata* tests. Wrap the GCP metadata test body with
setSsrfFilterDisabledForTesting(false) at the beginning to ensure the SSRF
filter is enabled for this test, and then modify the assertion to specifically
check for the "Host not allowed." error message instead of just checking for
generic failure. This ensures the test properly verifies filter blocking
behavior rather than relying on generic network/DNS failures that could mask
filter regressions.
---
Outside diff comments:
In
`@app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java`:
- Around line 262-290: The hostname is validated in `assertHostAllowed()` before
JedisPool creation, but the actual TCP connection occurs later when
`getResource()` is called on the JedisPool instance. An attacker-controlled
hostname could resolve to a different IP address between validation and
connection time. Locate all places where `getResource()` is called on the
JedisPool and add post-connection validation immediately after to verify that
the remote peer address matches the hostname that was validated in
`assertHostAllowed()`, or alternatively document the deliberate
Time-Of-Check-Time-Of-Use acceptance with clear risk justification if you decide
to accept this DNS rebinding risk.
---
Nitpick comments:
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/util/RestrictedHostFilter.java`:
- Around line 338-355: The resolveInternalRedisIps method performs DNS lookups
repeatedly on every isHostBlocked call, which is inefficient since Redis hosts
are static. Implement caching for the DNS resolution results by storing the
resolved IPs in a cached field that is computed once during initialization or on
first use, rather than recalculating them on each method invocation. Consider
using a memoization approach or a class-level cache that stores the result of
resolveInternalRedisIps so that subsequent calls to isHostBlocked can reuse the
cached IP set instead of triggering fresh DNS lookups.
- Around line 165-201: The internal Redis host storage methods
(computeInternalRedisHosts, addInternalRedisHostFromEnv, and
setInternalRedisHostsForTesting) normalize hosts using only
trim().toLowerCase(Locale.ROOT), but the comparison methods (isHostBlocked,
isLiteralBlocked) use fully canonicalized normalization via
normalizeHostForComparison*. This mismatch causes IPv6 addresses in brackets and
other edge cases to fail comparisons. Apply normalizeHostForComparisonQuietly to
all host values before storing them in the set across all three methods to
ensure symmetric normalization between storage and comparison.
🪄 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: dfce919e-7c16-4f1c-9527-2d9559075821
📒 Files selected for processing (14)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/RestrictedHostFilter.javaapp/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.javaapp/server/appsmith-interfaces/src/test/java/com/appsmith/external/connections/OAuth2ClientCredentialsTest.javaapp/server/appsmith-interfaces/src/test/java/com/appsmith/util/RestrictedHostFilterTest.javaapp/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.javaapp/server/appsmith-plugins/elasticSearchPlugin/src/main/java/org/apache/http/impl/nio/client/HttpAsyncClientBuilder.javaapp/server/appsmith-plugins/elasticSearchPlugin/src/test/java/com/external/plugins/ElasticSearchPluginTest.javaapp/server/appsmith-plugins/graphqlPlugin/src/test/java/com/external/plugins/GraphQLPluginTest.javaapp/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.javaapp/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.javaapp/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.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.javaapp/server/pom.xml
Six follow-ups from the CodeRabbit review on the CE PR — none change behavior beyond closing existing edge-case gaps and tightening tests. Will collapse into the squash-merged commit. - RestrictedHostFilter: route stored internal-Redis hosts through normalizeHostForComparisonQuietly so the storage form matches the compare-side canonicalization. Closes an IPv6-bracketed-URL bypass (e.g. redis://[::1]:6379 was stored as "[::1]" but lookups compared to "0.0.0.1"). - RedisPlugin.assertHostAllowed: document the deliberate TOCTOU acceptance between isHostBlocked and Jedis's own re-resolution. - RestApiPluginTest.testDenyInstanceMetadataGcp: wrap in setSsrfFilterDisabledForTesting(false) and assert "Host not allowed." so a network failure to metadata.google.internal can't mask a filter regression. Was the one outlier left unwrapped when the AWS variants were strengthened. - RestApiPluginTest.testDenyInstanceMetadataAwsWithRedirect: use a local MockWebServer rather than reassigning the @beforeeach `mockEndpoint` field; the reassignment leaked the original (tearDown only shuts down whatever's in the field at AfterEach time). - ElasticSearchPluginTest.itShouldRejectGetTo*AndRedirect: add mockWebServer.shutdown() in finally so the per-test MockWebServer instances don't leak threads / sockets across the suite. - RestrictedHostFilterTest.resolveIfAllowed_returnsResolvedAddress: use the literal IP 1.1.1.1 and assert exact equality, instead of smtp.gmail.com + an IPv4-only regex that would fail on IPv6-preferred CI environments.
Pure dependency bump, no code changes. Jedis 3.3.0 (2020) predates the JedisSocketFactory injection API needed to close the DNS-rebinding TOCTOU in the Redis SSRF host filter (next commit). 5.2.0 keeps the Java 8 baseline and every API the plugin uses is unchanged: JedisPool, getResource, sendCommand, ping, JedisPoolConfig, Protocol.Command, SafeEncoder, JedisException all compile as-is (verified locally). Jedis is used only by redisPlugin; the server's own Redis state runs on Lettuce, so this bump is plugin-isolated (PF4J classloader + shaded jar). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28055924025. |
|
Deploy-Preview-URL: https://ce-41921.dp.appsmith.com |
Failed server tests
|
The Jedis 5.x client reports an unresolvable host with a different exception message than 3.x did, so the assertion pinned to "Failed connecting to <host>:<port>" no longer matches. Assert the behavioral contract instead — an invalid host yields an unsuccessful DatasourceTestResult carrying a non-empty error — which is robust across driver versions. Necessitated by the Jedis 3.3.0 to 5.2.0 upgrade. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lution socket factory The create-time pre-check (RestrictedHostFilter.isHostBlocked) and Jedis's own connect-time resolution were two independent DNS lookups — a hostile resolver could return an allowed IP to the pre-check and the internal Redis IP to the driver (DNS rebinding). Now requires Jedis 5.2.0's socket-factory injection (prior commits). - Add RestrictedHostFilter.firstAllowedRedisAddress(host, resolved): pure policy check over an already-resolved address set, returning the validated address to connect to (or empty if blocked). Caller owns the single DNS resolution. - Add RestrictedHostJedisSocketFactory: resolves once at connect time, validates via the above, and connects the socket directly to the pinned IP so the driver never re-resolves. For rediss:// the plaintext socket connects to the pinned IP while the original hostname is used for SNI / cert verification; connect + TLS logic mirrors DefaultJedisSocketFactory. - Wire it into RedisPlugin.datasourceCreate via JedisPool(poolConfig, socketFactory, clientConfig), deriving the client config from the URI exactly as Jedis's own URI constructor does. The isHostBlocked pre-check stays as create-time UX/defense-in-depth. - Test that the factory rejects a denylisted host at connect time. See GHSA-qhfj-g87x-m39w. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28057260205. |
|
Deploy-Preview-URL: https://ce-41921.dp.appsmith.com |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/RestrictedHostFilter.java (1)
384-414: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeny-logic is a near-duplicate of
isHostBlocked— extract a shared helper to avoid SSRF drift.The block-checking core here (canonical denylist +
isBlockedIpAddressClass, internal-Redis hostname match, then the per-addressresolveInternalRedisIps/matchesBlockedAddressClassloop) is copy-pasted fromisHostBlocked(Lines 311-339). In security-sensitive code that's a maintenance hazard: a future tweak to one path can silently leave the other open. The method itself is correct and fail-closed — this is purely about keeping the two gates in lockstep.♻️ Extract a shared resolved-set check
+ private static boolean isResolvedSetBlocked(String canonicalHost, InetAddress[] addresses) { + if (DISALLOWED_HOSTS.contains(canonicalHost) || isBlockedIpAddressClass(canonicalHost)) { + return true; + } + final Set<String> redisHosts = internalRedisHosts; + if (redisHosts.contains(canonicalHost)) { + return true; + } + final Set<String> internalRedisIps = resolveInternalRedisIps(redisHosts); + for (InetAddress addr : addresses) { + final String addrString = normalizeHostForComparisonQuietly(addr.getHostAddress()); + if (DISALLOWED_HOSTS.contains(addrString) + || matchesBlockedAddressClass(addr) + || internalRedisIps.contains(addrString)) { + return true; + } + } + return false; + }if (ssrfFilterDisabled || isExplicitlyAllowedForTesting(host)) { return Optional.of(resolvedAddresses[0]); } - final String canonicalHost = normalizeHostForComparisonQuietly(host); - if (DISALLOWED_HOSTS.contains(canonicalHost) || isBlockedIpAddressClass(canonicalHost)) { - return Optional.empty(); - } - - final Set<String> redisHosts = internalRedisHosts; - if (redisHosts.contains(canonicalHost)) { - return Optional.empty(); - } - - final Set<String> internalRedisIps = resolveInternalRedisIps(redisHosts); - for (InetAddress addr : resolvedAddresses) { - final String addrString = normalizeHostForComparisonQuietly(addr.getHostAddress()); - if (DISALLOWED_HOSTS.contains(addrString) - || matchesBlockedAddressClass(addr) - || internalRedisIps.contains(addrString)) { - return Optional.empty(); - } - } - - return Optional.of(resolvedAddresses[0]); + return isResolvedSetBlocked(canonicalHost, resolvedAddresses) + ? Optional.empty() + : Optional.of(resolvedAddresses[0]);
isHostBlockedcan then resolve and delegate to the same helper.🤖 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/RestrictedHostFilter.java` around lines 384 - 414, Extract the common deny-logic that is duplicated between firstAllowedRedisAddress and isHostBlocked into a shared helper method. The helper should encapsulate the core checks: canonical host validation against DISALLOWED_HOSTS, IP address class validation using isBlockedIpAddressClass, internal Redis hostname matching against internalRedisHosts, and the per-address loop that checks each InetAddress against DISALLOWED_HOSTS, matchesBlockedAddressClass, and resolveInternalRedisIps. Replace the duplicate logic blocks in both firstAllowedRedisAddress and isHostBlocked to call this shared helper, ensuring both methods use identical deny-list evaluation logic to prevent SSRF security gaps.
🤖 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-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java`:
- Around line 289-298: The DefaultJedisClientConfig.builder() chain that
constructs clientConfig is missing the hostnameVerifier configuration, which
disables TLS hostname verification for rediss:// datasources. Add a
.hostnameVerifier() method call to the builder chain (before the .build() call)
to enable proper TLS certificate hostname verification and prevent attacks using
mismatched certificates on pinned IPs.
---
Nitpick comments:
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/util/RestrictedHostFilter.java`:
- Around line 384-414: Extract the common deny-logic that is duplicated between
firstAllowedRedisAddress and isHostBlocked into a shared helper method. The
helper should encapsulate the core checks: canonical host validation against
DISALLOWED_HOSTS, IP address class validation using isBlockedIpAddressClass,
internal Redis hostname matching against internalRedisHosts, and the per-address
loop that checks each InetAddress against DISALLOWED_HOSTS,
matchesBlockedAddressClass, and resolveInternalRedisIps. Replace the duplicate
logic blocks in both firstAllowedRedisAddress and isHostBlocked to call this
shared helper, ensuring both methods use identical deny-list evaluation logic to
prevent SSRF security gaps.
🪄 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: 7cc48cd9-b20d-4f66-b68d-58a11a351800
📒 Files selected for processing (4)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/RestrictedHostFilter.javaapp/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.javaapp/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RestrictedHostJedisSocketFactory.javaapp/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java
Comment on lines
+289
to
+298
| JedisClientConfig clientConfig = DefaultJedisClientConfig.builder() | ||
| .connectionTimeoutMillis(timeout) | ||
| .socketTimeoutMillis(timeout) | ||
| .user(JedisURIHelper.getUser(uri)) | ||
| .password(JedisURIHelper.getPassword(uri)) | ||
| .database(JedisURIHelper.getDBIndex(uri)) | ||
| .ssl(JedisURIHelper.isRedisSSLScheme(uri)) | ||
| .build(); | ||
| JedisSocketFactory socketFactory = | ||
| new RestrictedHostJedisSocketFactory(hostAndPort, clientConfig); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
Does Jedis JedisPool(poolConfig, URI, timeout) perform TLS hostname verification by default for rediss scheme
💡 Result:
Yes, Jedis now performs TLS hostname verification by default for rediss:// schemes, provided you are using Jedis version 7.5.0 or later [1][2]. In versions prior to 7.5.0, TLS hostname verification was not enforced by default [3][1]. Starting with Jedis 7.5.0, the client was updated to enforce hostname verification automatically when SSL is enabled (which occurs when using the rediss scheme or explicitly setting ssl(true)) [1][2]. Specifically, the library now sets the endpoint identification algorithm to HTTPS (SSLParameters.setEndpointIdentificationAlgorithm("HTTPS")) by default, ensuring that the certificate's Common Name (CN) or Subject Alternative Name (SAN) matches the host being connected to [4][2]. If this verification fails, the connection attempt will result in an SSL handshake error [4][1]. If you need to bypass this verification (e.g., for legacy environments where certificate hostnames do not match the connection endpoints), you can disable it by explicitly providing custom SSLParameters with a null or empty endpoint identification algorithm [4][2]. Additionally, newer versions of Jedis (as of the 7.x series) encourage the use of the SslOptions API for more explicit configuration of TLS verification modes [4][5][1].
Citations:
- 1: https://github.com/redis/jedis/releases/tag/v7.5.0
- 2: redis/jedis@v7.4.1...v7.5.0
- 3: SSL connections should support hostname verification out of the box redis/jedis#3270
- 4: https://redis.github.io/jedis/migration-guides/v7-to-v8/
- 5: https://redis.io/docs/latest/develop/clients/jedis/connect/
TLS cert hostname verification is effectively off.
clientConfig omits hostnameVerifier, so for rediss:// datasources the verification check at line 106 of RestrictedHostJedisSocketFactory never executes. Since Jedis 5.2.0 (in use here) does not enforce TLS hostname verification by default, this means an attacker presenting any valid-but-mismatched certificate to the pinned IP would not be caught.
This is not a regression from the pre-PR URI-based JedisPool constructor, which also lacked hostname verification with Jedis 5.2.0. However, if hostname verification should be enforced, add .hostnameVerifier() to the DefaultJedisClientConfig.builder().
🤖 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/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java`
around lines 289 - 298, The DefaultJedisClientConfig.builder() chain that
constructs clientConfig is missing the hostnameVerifier configuration, which
disables TLS hostname verification for rediss:// datasources. Add a
.hostnameVerifier() method call to the builder chain (before the .build() call)
to enable proper TLS certificate hostname verification and prevent attacks using
mismatched certificates on pinned IPs.
…dRedisAddress Both methods evaluated the deny list independently (canonical denylist + IP-class check + internal-Redis literal, then a per-resolved-address loop). Extract isCanonicalHostBlocked and isAnyResolvedAddressBlocked so the two share one evaluation and cannot drift into an SSRF gap. Behavior-preserving: isHostBlocked keeps its canonical short-circuit before DNS and its unresolvable-host-returns-false contract. RestrictedHostFilterTest (87) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28060123190. |
|
Deploy-Preview-URL: https://ce-41921.dp.appsmith.com |
…datasources Jedis 5.2.0 does not verify the certificate hostname by default (only the trust chain), so a MITM presenting any CA-trusted-but-mismatched cert on the pinned IP would be accepted. Set the endpoint identification algorithm to HTTPS in the socket factory so the cert SAN/CN is checked against the configured hostname (we connect to the pinned IP but pass the hostname to the TLS layer, so verification runs against the hostname). Defaults to HTTPS only when a caller hasn't set its own endpoint-identification policy. Low blast radius: rediss:// support was added 2026-03-03 (#41587) and is barely adopted. Chain validation was already on, so self-signed certs already failed; this newly rejects only CA-trusted-but-mismatched certs and IP-addressed rediss:// (certs rarely carry IP SANs). No opt-out env var yet — add one if a real datasource needs a mismatched/IP cert. Addresses the CodeRabbit "TLS hostname verification effectively off" finding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28072374766. |
|
Deploy-Preview-URL: https://ce-41921.dp.appsmith.com |
…n instead of a generic error testDatasource read error.getCause().getMessage(), but the socket factory's connection-time rejection throws a causeless JedisConnectionException (message "Host not allowed."), so getCause() was null and the call NPE'd — the user saw a generic "Couldn't establish a connection" instead of the intended message. This is the DNS-rebinding path: the host passes the create-time pre-check (resolves to an allowed IP) but the socket factory's own resolution lands on a blocked IP and refuses. Fall back to the error's own message when there's no cause (also fixes the latent NPE on the no-PONG path). Add a log line in the factory so the rejection is visible server-side. Verified on the deploy preview with rbndr.us flipping 172.31.4.163 <-> 127.0.0.1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28108451594. |
|
Deploy-Preview-URL: https://ce-41921.dp.appsmith.com |
The connect-time rejection logs (RestrictedHostJedisSocketFactory), but the create-time pre-check (assertHostAllowed) — which catches the common case of a datasource pointed at a disallowed host — threw silently. Log it at WARN; the log framework tags the line with the requesting userEmail / orgId / traceId, so a cluster of these is a useful reconnaissance signal. No server-side trace otherwise, since the block is returned as a graceful HTTP 200 DatasourceTestResult. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28113194173. |
|
Deploy-Preview-URL: https://ce-41921.dp.appsmith.com |
subrata71
reviewed
Jun 25, 2026
|
|
||
| private static Set<String> computeInternalRedisHosts() { | ||
| final Set<String> hosts = new HashSet<>(); | ||
| addInternalRedisHostFromEnv(hosts, "APPSMITH_REDIS_URL"); |
Collaborator
There was a problem hiding this comment.
What if Redis is configured using application.properties e.g. appsmith.redis.url. In that case it remains fail-open. Please correct me if I am wrong.
Contributor
Author
There was a problem hiding this comment.
Excellent point. I think this is addressed now.
| * connected to. Honors the {@code APPSMITH_DISABLE_SSRF_FILTER} kill-switch and the test | ||
| * allowlist, in which case the first resolved address is returned without policy checks. | ||
| */ | ||
| public static Optional<InetAddress> firstAllowedRedisAddress(String host, InetAddress[] resolvedAddresses) { |
Collaborator
There was a problem hiding this comment.
QQ: Do we need to loop through all the addresses? Could it be possible if the first resolved IP is IPV6 but the actual redis is listening on IPV4 address?
Contributor
Author
There was a problem hiding this comment.
Yep, adjusted that. Good catch.
…ved URL, not just the env var RestrictedHostFilter built its internal-Redis set only from the APPSMITH_REDIS_URL / APPSMITH_REDIS_GIT_URL environment variables at static init. But the app binds appsmith.redis.url via Spring, which can also resolve from application.properties, a -D system property, or a profile. An operator configuring Redis that way (no env var) left the internal-Redis set empty, so a datasource could be pointed at the in-cluster Redis — fail-open. (Loopback and cloud-metadata literals were still blocked, but an RFC1918 address or a service hostname was not.) RedisConfig now registers the resolved appsmith.redis.url / appsmith.redis.git.url with the filter at startup (@PostConstruct, before any datasource test), unioning with the env-derived seed so configuring through either channel blocks both. The filter is shared with the redisPlugin (appsmith-interfaces is provided-scope to plugins, so it's parent-loaded), so the pushed state is visible at the connection path. RestrictedHostFilterTest covers the new registration and union semantics. See GHSA-qhfj-g87x-m39w. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28193528930. |
|
Deploy-Preview-URL: https://ce-41921.dp.appsmith.com |
…st the first RestrictedHostJedisSocketFactory connected to only the first resolved address. For a dual-stack host whose leading record is an unreachable IPv6 (AAAA first, but Redis listens on IPv4 / the client has no IPv6 route), that dropped the connection even though a reachable IPv4 was in the resolved set — a resilience regression versus stock DefaultJedisSocketFactory, which tries each address in turn. Now iterate the resolved addresses and connect to the first reachable one. Every address was already validated by the single firstAllowedRedisAddress() call (present only if none is blocked), so failover does not widen the SSRF surface and the single-resolution guarantee is unchanged. Raised in PR review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28194282012. |
|
Deploy-Preview-URL: https://ce-41921.dp.appsmith.com |
Both block log lines recorded only the host, so a hostname that resolved to
loopback / cloud-metadata looked identical to one typed as an internal IP
literally. Add the resolved address(es) for forensics:
- Connection-time (RestrictedHostJedisSocketFactory) logs its own already-
validated resolution — exact, no extra lookup.
- Create-time pre-check (RedisPlugin) uses a new log-only helper,
RestrictedHostFilter.describeResolvedAddresses, which does a best-effort
lookup ("unresolved" when it can't resolve). It is point-in-time and not a
security decision, since isHostBlocked only returns a boolean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28199055848. |
|
Deploy-Preview-URL: https://ce-41921.dp.appsmith.com |
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
2 participants
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.
Summary
Mitigates GHSA-qhfj-g87x-m39w. The Redis datasource plugin opened a Jedis connection to a user-controlled host/port with no IP-class filtering, so a user with datasource-create permission could point a Redis datasource at the internal Appsmith Redis. The existing HTTP-side filter doesn't cover Redis — Jedis uses the JVM's default resolver and bypasses the Reactor Netty hook.
This PR also closes a DNS-rebinding TOCTOU on the same path and enforces TLS certificate hostname verification for
rediss://datasources (items 4 and 5 below).Changes
Block Redis datasources from reaching the internal Appsmith Redis.
RedisPlugin.datasourceCreatenow consults a sharedRestrictedHostFilter. Blocks literal hostname / IP matches againstAPPSMITH_REDIS_URLandAPPSMITH_REDIS_GIT_URL, plus loopback / link-local / cloud-metadata literals. Enforcement is connection-time only —PluginExecutor.validateDatasourceis documented format-only, so host-policy belongs on the connection path. UX: Save succeeds, "Test Datasource" rejects withHost not allowed..Make SSRF protections always-on; rename
IN_DOCKERtoAPPSMITH_DISABLE_SSRF_FILTER. The previous filter gated the loopback / link-local block onIN_DOCKER=1, which Appsmith's official Docker image always sets — so users on documented deployment paths were already getting this protection. This change closes the gap for unofficial distributions (self-built images, bare-metal, etc.) and replaces a poorly-named variable with one that actually describes what it does. The most likely place to hit this is a development machine running the server directly againsthttp://localhost:*— setAPPSMITH_DISABLE_SSRF_FILTER=trueto allow it.Extract
RestrictedHostFilterfromWebClientUtils. Pure refactor — the host-filter logic had already grown several non-HTTP call sites and the class name had outgrown its scope.Close the DNS-rebinding TOCTOU on the Redis path (upgrade Jedis 3.3.0 → 5.2.0 + pin the resolved IP). The create-time pre-check in (1) and Jedis's own connection-time lookup resolved the hostname independently, so a hostile resolver could return an allowed IP to the check and the internal Redis IP to the driver. Jedis 5.2.0 (up from 3.3.0, which predated the API) allows injecting a
JedisSocketFactory; the newRestrictedHostJedisSocketFactoryresolves once at connect time, validates every resolved address, and connects directly to the pinned IP — so the driver never re-resolves and the window is closed. The create-time check stays as fast Save/Test UX and now logs the block at WARN, tagged with the requesting user/org/trace (a useful reconnaissance signal). Jedis is plugin-isolated (PF4J + shaded); the server's own Redis runs on Lettuce and is untouched.Enforce TLS certificate hostname verification for
rediss://datasources. Jedis 5.2.0 validates the certificate trust chain but does not verify that the cert's SAN/CN matches the host (it only does so by default in 7.5.0+), so a man-in-the-middle presenting any CA-trusted-but-mismatched cert on the pinned IP would have been accepted. The socket factory now sets the endpoint identification algorithm toHTTPS, verifying against the configured hostname (we connect to the pinned IP but hand the original hostname to the TLS layer for SNI and verification). This was a pre-existing gap, not introduced here; closing it now is low-risk because Redis TLS support is recent (feat: Redis datasource: add TLS (SSL mode) support in backend + datasource UI #41587, ~2026-03) and barely adopted, and chain validation was already on — so only CA-trusted-but-mismatched certs and IP-addressedrediss://(certs rarely carry IP SANs) are newly rejected.Test plan
RedisPluginTest(socketFactory_blockedHost_failsAtConnectTime). A live end-to-end rebinding flip needs a changing resolver and was not exercised on the DP (rbndr.us is unreachable from AWS).rediss://user (Appsmith internal) connects via a hostname matching its cert, now that hostname verification is enforced.🤖 Generated with Claude Code
Warning
Tests have not run on the HEAD bfef59b yet
Thu, 25 Jun 2026 20:39:48 UTC
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests