fix(server): prevent open redirect via forged Origin header on login#41995
Open
subrata71 wants to merge 2 commits into
Open
fix(server): prevent open redirect via forged Origin header on login#41995subrata71 wants to merge 2 commits into
subrata71 wants to merge 2 commits into
Conversation
The Origin header was used as both the redirect URL builder and the trust anchor in isSafeRedirectUrl(), allowing an attacker who forges the Origin to bypass validation (evil.com == evil.com always passes). Introduce getTrustedOrigin() which cross-checks the Origin header against the request's Host / X-Forwarded-Host before trusting it. All four call sites that read Origin now use this helper. APP-15347
Contributor
WalkthroughRedirect handling now validates the ChangesRedirect hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 2
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/RedirectHelperOpenRedirectTest.java (1)
506-521: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test to verify IPv6 Origin validation against the request host.
The existing
testIPv6LocalhostMatchdoes not set aHostorX-Forwarded-Hostheader, meaning it bypasses the new host-comparison logic ingetTrustedOrigin. Consider adding a test that provides both an IPv6 Origin and an IPv6 Host header to ensure the bracket stripping logic works correctly and prevents future regressions.🧪 Proposed test
`@Test` void testIPv6OriginMatchesRequestHost() { HttpHeaders headers = new HttpHeaders(); headers.setOrigin("http://[::1]:8080"); headers.set("X-Forwarded-Host", "[::1]:8080"); assertTrue(RedirectHelper.isSafeRedirectUrl("http://[::1]:8080/applications", headers)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/RedirectHelperOpenRedirectTest.java` around lines 506 - 521, Add a test alongside testIPv6LocalhostMatch that sets an IPv6 Origin with brackets and port and provides the matching IPv6 X-Forwarded-Host header, then assert isSafeRedirectUrl accepts the corresponding redirect. Ensure the test exercises host comparison and IPv6 bracket stripping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedirectHelper.java`:
- Around line 138-141: Replace the removed StringUtils.isEmpty checks with
StringUtils.hasText in RedirectHelper at
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedirectHelper.java
lines 138-141, 202-203, and 440-441. Apply the change to trustedOrigin at the
anchor and final site, and to origin at the sibling site, preserving the
surrounding redirect logic.
- Around line 324-328: Update the origin host comparison in RedirectHelper to
remove surrounding IPv6 brackets from originHost before comparing it with
requestHost, matching the normalization used by isSafeRedirectUrl. Preserve the
existing null check and return origin only when the normalized hosts match
case-insensitively.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/RedirectHelperOpenRedirectTest.java`:
- Around line 506-521: Add a test alongside testIPv6LocalhostMatch that sets an
IPv6 Origin with brackets and port and provides the matching IPv6
X-Forwarded-Host header, then assert isSafeRedirectUrl accepts the corresponding
redirect. Ensure the test exercises host comparison and IPv6 bracket stripping.
🪄 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: 0d653edc-6bf8-42f7-921a-09d38a2fffad
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedirectHelper.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/RedirectHelperOpenRedirectTest.java
- Strip IPv6 brackets from originHost in getTrustedOrigin() before comparing against requestHost (which already strips them), matching the normalization in isSafeRedirectUrl's fallback path. - Replace deprecated StringUtils.isEmpty() with StringUtils.hasText() at the three call sites introduced in the prior commit. - Add IPv6 test with X-Forwarded-Host to exercise the bracket stripping path. APP-15347
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/RedirectHelperOpenRedirectTest.java`:
- Around line 523-531: The IPv6 redirect validation lacks a same-host,
different-port rejection case and currently may trust an Origin with an
incorrect port. Extend RedirectHelper.isSafeRedirectUrl validation and its
getTrustedOrigin flow to compare the effective origin port as well as the host,
then add a negative test using X-Forwarded-Host [::1]:8080 and Origin/redirect
port 9090 that asserts rejection while preserving the existing matching-port
acceptance.
🪄 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: 1c4c0330-3253-4ca6-a486-ce2df694b026
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedirectHelper.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/RedirectHelperOpenRedirectTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedirectHelper.java
Comment on lines
+523
to
+531
| @Test | ||
| void testIPv6OriginMatchesRequestHost() { | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.setOrigin("http://[::1]:8080"); | ||
| headers.set("X-Forwarded-Host", "[::1]:8080"); | ||
| assertTrue( | ||
| RedirectHelper.isSafeRedirectUrl("http://[::1]:8080/applications", headers), | ||
| "IPv6 Origin matching X-Forwarded-Host must be accepted"); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a same-host, different-port rejection test.
This only verifies the positive case. getTrustedOrigin() strips the request port and compares hosts only, then isSafeRedirectUrl() trusts the retained client-supplied Origin. With X-Forwarded-Host: [::1]:8080 and Origin: http://[::1]:9090, a redirect to port 9090 may be accepted. Add a negative test and make trusted-origin validation include effective port matching.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/RedirectHelperOpenRedirectTest.java`
around lines 523 - 531, The IPv6 redirect validation lacks a same-host,
different-port rejection case and currently may trust an Origin with an
incorrect port. Extend RedirectHelper.isSafeRedirectUrl validation and its
getTrustedOrigin flow to compare the effective origin port as well as the host,
then add a negative test using X-Forwarded-Host [::1]:8080 and Origin/redirect
port 9090 that asserts rejection while preserving the existing matching-port
acceptance.
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.
Summary
Fixes an open redirect vulnerability (APP-15347) where an attacker could forge the
OriginHTTP header onPOST /api/v1/loginto redirect authenticated users to an external domain.Root cause:
isSafeRedirectUrl()used the client-suppliedOriginheader as both the redirect URL builder and the trust anchor. A forgedOrigin: https://evil.compassed validation because the redirect host trivially matched the Origin host (evil.com == evil.com).Fix: Introduces
getTrustedOrigin()which cross-checks theOriginheader against the request'sHost/X-Forwarded-Host(set by the reverse proxy, not the client). All four call sites that previously read Origin directly now use this helper:isSafeRedirectUrl()— no longer uses forged Origin as the validation baselinefulfillRedirectUrl()— no longer prepends forged Origin to relative redirect pathssanitizeRedirectUrl()— no longer uses forged Origin in the fallback URLgetRedirectUrl()(fork-app path) — no longer uses forged Origin for URL constructionWhen no
Host/X-Forwarded-Hostis available (unusual environments without proxy headers), Origin is still trusted to preserve backward compatibility.Test plan
testForgedOriginHeaderIsBlockedWhenHostDiffers— confirms forged Origin is rejected and sanitized URL does not contain the attacker domainRedirectHelperOpenRedirectTesttests pass (0 failures, 0 errors)Linear ticket
https://linear.app/appsmith/issue/APP-15347/security-low-open-redirect-via-improper-validation-of-origin-header-on
Summary by CodeRabbit
Summary by CodeRabbit
Originheaders from enabling unsafe redirects, including in absolute-URL handling.Originscenarios and IPv6+port origin matching.Automation
/ok-to-test tags="@tag.All"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/29364494919
Commit: 2a35649
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 14 Jul 2026 21:45:29 UTC