fix: SSO login redirect lands on /applications instead of original app URL#41820
fix: SSO login redirect lands on /applications instead of original app URL#41820sebastianiv21 wants to merge 5 commits into
Conversation
…p URL (#8815) PR #41550 made isSafeRedirectUrl reject any absolute redirect URL when the Origin header is absent. Browsers do not send Origin on top-level GET navigations, which is exactly how the frontend triggers the SSO entry URL (window.location.href = "/oauth2/authorization/...?redirectUrl=<absolute>"). The absolute redirectUrl was therefore replaced with /applications and baked into the OAuth2 state, so post-SSO the user always landed on /applications rather than the deep app link they opened. Relax the no-Origin path to accept same-host absolute URLs, validated against X-Forwarded-Host (preferred, for proxied envs) or Host. Hostname-only comparison since these headers carry no scheme/port. The open-redirect protection from #41550 is preserved — only same-host URLs become permissible when Origin is absent, and userinfo / protocol-relative / non-http(s) bypass attempts are still rejected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
WalkthroughThis PR tightens open-redirect checks by parsing redirect URIs, rejecting userinfo/malformed targets, enforcing Origin-based host/port matching when present, and adding an Origin-absent fallback that derives request host/port from X-Forwarded-Host/Host with IPv6 and port normalization. ChangesOrigin-absent redirect fallback validation
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers:
🚥 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 |
sebastianiv21
changed the title
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/25946297135. |
|
Deploy-Preview-URL: https://ce-41820.dp.appsmith.com |
subrata71
left a comment
subrata71
left a comment
There was a problem hiding this comment.
The changes look good to me. If you want to address those nitpicks then it would be great otherwise I can approve. But before you merge this can you please create a shadow EE PR and make sure all tests pass there as well?
| * a comma-separated list — the first entry is the outermost client-facing host), | ||
| * then falls back to the Host header. Returns null when neither is set. | ||
| */ | ||
| private static String extractRequestHost(HttpHeaders headers) { |
There was a problem hiding this comment.
Could you please check if there are already such util method available which does the same thing? Just trying to make sure we are not keeping duplicate logic scattered in different places.
There was a problem hiding this comment.
CustomServerOAuth2AuthorizationRequestResolverCE.extractHostFromRequest was doing essentially the same job (XFH comma-parsing + Host fallback) in the same SSO entry flow.
I promoted RedirectHelper.extractRequestHost(HttpHeaders) from private to public static and refactored the OAuth2 resolver to delegate.
The resolver still keeps request.getURI().getHost() as a last-resort fallback, so the only behavior change is the order between Host header and URI host when XFH is unset (which is a no-op in proxied envs since XFH is always set there). Let me know if the changes are ok.
- Add warn logs in the two URI parse catch blocks in isSafeRedirectUrl (one for redirectUrl, one for Origin header) so malformed inputs are visible in server logs instead of silently rejected. - Extract sanitizeForLog(String) to centralize the CR/LF strip + 200-char truncate previously inlined in sanitizeRedirectUrl; reuse it in the two new logs. - Promote RedirectHelper.extractRequestHost(HttpHeaders) from private to public static. Refactor CustomServerOAuth2AuthorizationRequestResolverCE .extractHostFromRequest to delegate to it, removing the duplicate XFH / Host header parsing. The request URI host is preserved as a last-resort fallback in the resolver. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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-server/src/main/java/com/appsmith/server/helpers/RedirectHelper.java (1)
246-256:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the request port in the Origin-absent validation path.
This branch now reduces the request side to hostname-only, so a request for
app.example.comwill also allowhttps://app.example.com:8443/...wheneverOriginis missing. That is a different origin, and it can bounce the browser to another service on the same host after login. Please carry the forwarded/host port through when it is present and compare it againstredirectUri.getPort()instead of dropping it unconditionally.Also applies to: 271-283
🤖 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/main/java/com/appsmith/server/helpers/RedirectHelper.java` around lines 246 - 256, The current Origin-absent path in isSameOrigin drops the request port by using extractRequestHost(httpHeaders) and comparing only hostname to redirectHost (after normalizing brackets), which permits different-origin requests on nonstandard ports; update isSameOrigin to preserve and parse the forwarded/request port from httpHeaders (e.g., Host or X-Forwarded-Host/Port) alongside extractRequestHost, build a request host:port representation, and compare the request port against redirectUri.getPort() (and hostname against normalizedRedirectHost) instead of ignoring the port; adjust the logic in the same block (and the similar block around lines 271-283) to treat IPv6 bracketed hosts the same but include port comparison when present.
🤖 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.
Outside diff comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedirectHelper.java`:
- Around line 246-256: The current Origin-absent path in isSameOrigin drops the
request port by using extractRequestHost(httpHeaders) and comparing only
hostname to redirectHost (after normalizing brackets), which permits
different-origin requests on nonstandard ports; update isSameOrigin to preserve
and parse the forwarded/request port from httpHeaders (e.g., Host or
X-Forwarded-Host/Port) alongside extractRequestHost, build a request host:port
representation, and compare the request port against redirectUri.getPort() (and
hostname against normalizedRedirectHost) instead of ignoring the port; adjust
the logic in the same block (and the similar block around lines 271-283) to
treat IPv6 bracketed hosts the same but include port comparison when present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6d5d0da-615d-494f-b69b-9dacd38c3ae8
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomServerOAuth2AuthorizationRequestResolverCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedirectHelper.java
Failed server tests
|
1 similar comment
Failed server tests
|
Addresses a same-host open-redirect vector flagged in review: the Origin-absent fallback in isSafeRedirectUrl compared hostnames only, so a request to app.example.com would also allow https://app.example.com:8443/... (a different service on the same host) after login. Now the fallback path also compares ports. The request port is derived from X-Forwarded-Port, a port embedded in X-Forwarded-Host, or the Host header (mirroring extractRequestHost's source precedence). Since these headers carry no scheme, an implicit request port is normalized to the default for the redirect URL's scheme via the existing normalizePort helper. Same-host redirects to a different port are rejected; default-vs-explicit-default ports still match. Adds tests for the new port comparison and updates the prior X-Forwarded-Host-with-port test to assert port matching. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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/main/java/com/appsmith/server/helpers/RedirectHelper.java`:
- Around line 317-339: The extractRequestPort method currently ignores
X-Forwarded-Port unless X-Forwarded-Host is present; change it to first check
and parse X-Forwarded-Port (headers.getFirst("X-Forwarded-Port")) independently
and return its parsed value if valid, then fall back to X-Forwarded-Host parsing
(using portOf(first) as now) and finally to headers.getHost() as before; ensure
you handle comma-separated lists and NumberFormatException the same way and
preserve the existing return values (-1 when unknown).
🪄 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: bd20574f-ebbd-4242-a835-02314de8e9f3
📒 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
Per review, extractRequestPort previously only consulted X-Forwarded-Port inside the X-Forwarded-Host branch. Proxies that preserve the Host header but convey the client-facing port solely via X-Forwarded-Port would have their port ignored, causing same-host redirects on non-default ports to be rejected in the Origin-absent flow. Read X-Forwarded-Port first (regardless of X-Forwarded-Host), then a port embedded in X-Forwarded-Host, then the Host header port. Adds a regression test for the Host + X-Forwarded-Port combination. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2 participants
Description
Fixes a regression introduced in #41550 where unauthenticated users opening a deep app link (e.g.
https://<host>/app/<app-name>/<page-id>?ssoTrigger=saml) land on/applicationsafter completing OIDC/SAML login instead of the originally requested URL.Root cause
The frontend triggers the SSO entry URL via
window.location.href = "/oauth2/authorization/keycloak?redirectUrl=<encoded absolute URL>"— a top-level GET navigation. Browsers do not send theOriginheader for top-level GET navigations, but theredirectUrlvalue is always absolute (it comes fromwindow.location.href).RedirectHelper.isSafeRedirectUrlreadshttpHeaders.getOrigin(), finds it empty, and returnsfalsefor any absolute URL.sanitizeRedirectUrlthen rewrites the URL to<origin>/applications, which gets baked into the OAuth2stateand used by the post-callback success handler — so the user always lands on/applications.Fix
Restructure
isSafeRedirectUrlso the early bypass-blocking checks (relative path, http(s) prefix, URI parse, userInfo, null host) happen first, then split into two host-comparison branches:X-Forwarded-Host(preferred, supports proxied envs and comma-separated lists) thenHost. Compare hostnames only (these headers carry no scheme/port). If neither header is present, reject — preserves the old strict behavior in the truly no-info case.The open-redirect protection from #41550 is preserved — only same-host URLs become permissible in the no-Origin path, and userinfo / protocol-relative / non-http(s) bypass attempts are still rejected.
Fixes appsmith-ee#8815
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/26175316808
Commit: d37c496
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 20 May 2026 21:54:33 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Test plan
RedirectHelperOpenRedirectTestcovering the new fallback path: Host / X-Forwarded-Host, XFH precedence, comma-separated XFH lists, port stripping, IPv6 brackets, userinfo + protocol-relative bypass attempts blocked./app/<app-name>/<page-id>?ssoTrigger=saml(not/applications)./oauth2/authorization/keycloak?redirectUrl=https://evil.com/phish, complete SSO, verify the browser lands on/applicationsand server logs containBlocked open redirect attempt to: https://evil.com/phish.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests