fix(security): block full loopback range in SSRF protection#41694
Open
fix(security): block full loopback range in SSRF protection#41694
Conversation
…tion The SSRF protection in WebClientUtils only blocked 127.0.0.1 by exact string match, allowing addresses like 127.0.0.2 to bypass the filter and reach internal services (e.g. supervisord on port 9001). Add isBlockedByAddressType() helper that uses InetAddress type checks (isLoopbackAddress, isLinkLocalAddress, isAnyLocalAddress, isMulticastAddress, IPv6 ULA) and integrate it into requestFilterFn, isDisallowedAndFail, and resolveIfAllowed. This provides defense-in-depth at all three SSRF check layers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
WalkthroughIntroduces a mutable static Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Failed server tests
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java (2)
101-111: Consider adding0.0.0.0for consistency.The
requestFilterFntest includes0.0.0.0, but this test doesn't. While0.0.0.0is tested elsewhere (line 57), adding it here would make the coverage symmetric across all three SSRF layers.Suggested addition
`@ValueSource`( strings = { "127.0.0.2", "127.0.1.1", "127.255.255.254", + "0.0.0.0", })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java` around lines 101 - 111, Add 0.0.0.0 to the parameterized inputs for the test method testIsDisallowedAndFailBlocksFullLoopbackRange so the WebClientUtils.isDisallowedAndFail coverage matches other SSRF tests; update the `@ValueSource` strings in that test to include "0.0.0.0" alongside the existing loopback addresses to ensure symmetry with requestFilterFn and the earlier test that already covers 0.0.0.0.
84-123: Consider adding an explicit IPv6 loopback test for completeness.IPv6 loopback (
::1) is implicitly covered byInetAddress.isLoopbackAddress()inisBlockedByAddressType(), which blocks all loopback addresses regardless of theDISALLOWED_HOSTSlist. However, adding an explicit test with::1would match the thoroughness of the IPv4 loopback coverage and provide more immediate verification of the IPv6 path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java` around lines 84 - 123, Add explicit IPv6 loopback ("::1") test cases to the WebClientUtilsTest suite: create tests that call invokeRequestFilterFn("http://[::1]:9001/RPC2") and assert it yields UnknownHostException with message WebClientUtils.HOST_NOT_ALLOWED, call WebClientUtils.isDisallowedAndFail("::1", null) and assert true, and call WebClientUtils.resolveIfAllowed("::1") and assert the result is empty; place these alongside the existing IPv4 loopback tests so the IPv6 loopback path in WebClientUtils.isBlockedByAddressType() is explicitly validated.app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java (1)
313-326: Minor inefficiency: host is normalized twice.
canonicalHostis already normalized at line 315, butisHostDisallowednormalizes it again internally. Not a bug since normalization is idempotent, but slightly wasteful.Consider either:
- Passing raw
hosttoisHostDisallowed(preferred - simpler call site), or- Creating an overload that accepts pre-normalized input
Low priority given this is a security fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java` around lines 313 - 326, The code normalizes host into canonicalHost via normalizeHostForComparison but then calls isHostDisallowed(canonicalHost) which causes a second normalization inside isHostDisallowed; to avoid the duplicate work, call isHostDisallowed(host) from the WebClientUtils method (keeping the existing internal normalization in isHostDisallowed) or alternatively add an overload like isHostDisallowedNormalized(String canonicalHost) that assumes already-normalized input and call that here; update the call site around canonicalHost and remove the redundant normalization usage accordingly while keeping the existing error handling around normalizeHostForComparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.java`:
- Around line 313-326: The code normalizes host into canonicalHost via
normalizeHostForComparison but then calls isHostDisallowed(canonicalHost) which
causes a second normalization inside isHostDisallowed; to avoid the duplicate
work, call isHostDisallowed(host) from the WebClientUtils method (keeping the
existing internal normalization in isHostDisallowed) or alternatively add an
overload like isHostDisallowedNormalized(String canonicalHost) that assumes
already-normalized input and call that here; update the call site around
canonicalHost and remove the redundant normalization usage accordingly while
keeping the existing error handling around normalizeHostForComparison.
In
`@app/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java`:
- Around line 101-111: Add 0.0.0.0 to the parameterized inputs for the test
method testIsDisallowedAndFailBlocksFullLoopbackRange so the
WebClientUtils.isDisallowedAndFail coverage matches other SSRF tests; update the
`@ValueSource` strings in that test to include "0.0.0.0" alongside the existing
loopback addresses to ensure symmetry with requestFilterFn and the earlier test
that already covers 0.0.0.0.
- Around line 84-123: Add explicit IPv6 loopback ("::1") test cases to the
WebClientUtilsTest suite: create tests that call
invokeRequestFilterFn("http://[::1]:9001/RPC2") and assert it yields
UnknownHostException with message WebClientUtils.HOST_NOT_ALLOWED, call
WebClientUtils.isDisallowedAndFail("::1", null) and assert true, and call
WebClientUtils.resolveIfAllowed("::1") and assert the result is empty; place
these alongside the existing IPv4 loopback tests so the IPv6 loopback path in
WebClientUtils.isBlockedByAddressType() is explicitly validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97386ccf-aa9e-4ead-aea0-d2889e6afa50
📒 Files selected for processing (2)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/WebClientUtils.javaapp/server/appsmith-interfaces/src/test/java/com/appsmith/util/WebClientUtilsTest.java
The isBlockedByAddressType check was blocking loopback addresses unconditionally, but loopback should only be blocked inside Docker (matching the existing DISALLOWED_HOSTS policy). This broke OAuth2ClientCredentialsTest which uses MockWebServer on localhost. - Extract IN_DOCKER into a static field, gate loopback check on it - resolveIfAllowed (SMTP path) always blocks loopback regardless - Tests enable Docker mode via reflection to verify loopback blocking Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The SSRF protection in
WebClientUtilsonly blocked127.0.0.1via exact string match in theDISALLOWED_HOSTSset. On Linux, the entire127.0.0.0/8subnet routes to localhost, so addresses like127.0.0.2bypassed the filter and could reach internal services bound to127.0.0.1(e.g., supervisord XML-RPC on port 9001).Root cause
requestFilterFn(used by REST API plugin) andNameResolveronly checked againstDISALLOWED_HOSTS— a string set containing only127.0.0.1resolveIfAllowed(used for SMTP) already usedInetAddress.isLoopbackAddress()which covers the full/8range, but the HTTP path did notFix
isBlockedByAddressType(InetAddress)helper that checksisLoopbackAddress(),isLinkLocalAddress(),isAnyLocalAddress(),isMulticastAddress(), and IPv6 ULA (fc00::/7)requestFilterFn(pre-request filter),isDisallowedAndFail(DNS resolver), andresolveIfAllowed(SMTP path) — defense in depth at all three layersresolveIfAllowedto reuse the shared helper, eliminating code duplicationTests
testRequestFilterFnRejectsFullLoopbackRange— verifies127.0.0.2,127.0.1.1,127.255.255.254,0.0.0.0are rejected at the request filter leveltestIsDisallowedAndFailBlocksFullLoopbackRange— verifies the DNS resolver layer blocks the full rangeresolveIfAllowed_blocksFullLoopbackRange— verifies the SMTP path continues to block the full rangeFixes GHSA RCE via Supervisord XML-RPC — Incomplete Loopback Blocklist Allows 127.0.0.2 Bypass
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/24096557010
Commit: c53c881
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 07 Apr 2026 19:23:17 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests