fix: account takeover vulnerability #41398
Conversation
WalkthroughThis PR introduces host-based URL derivation infrastructure by adding a new HostUrlHelper utility and integrating it into UserController endpoints. The helper resolves client-facing base URLs from request context, replacing direct Origin header usage in password reset and email verification flows. Changes
Sequence DiagramsequenceDiagram
participant Client
participant UserControllerCE
participant HostUrlHelper
participant ServerRequest as Request Context
Client->>UserControllerCE: POST forgotPasswordRequest
UserControllerCE->>HostUrlHelper: getClientFacingBaseUrl(originHeader, exchange)
HostUrlHelper->>ServerRequest: Check configured domain (appsmith.domain)
alt Configured domain exists
HostUrlHelper->>ServerRequest: sanitizeConfiguredDomain()
HostUrlHelper-->>HostUrlHelper: Normalize & prefix scheme
else No configured domain
HostUrlHelper->>ServerRequest: getHostUrl(request)
HostUrlHelper->>HostUrlHelper: Try OAuth2 query params
HostUrlHelper->>HostUrlHelper: Try RFC 7239 Forwarded header
HostUrlHelper->>HostUrlHelper: Try X-Forwarded-\\* headers
HostUrlHelper->>HostUrlHelper: Fall back to Host header
end
HostUrlHelper-->>UserControllerCE: baseUrl (or empty string)
UserControllerCE-->>Client: ResponseDTO
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/HostUrlHelperCE.java (1)
82-94: Forwarded header parsing is fragile but safe.The simple string split approach (line 87) for parsing the RFC 7239
Forwardedheader may fail on edge cases or malformed headers. However, the try-catch wrapper ensures it fails safely and falls through to other header sources.Consider using a more robust parsing approach or a library designed for RFC 7239 parsing if this becomes an issue in production.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/UserController.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/HostUrlHelper.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/HostUrlHelperCE.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-04T05:51:15.138Z
Learnt from: abhvsn
Repo: appsmithorg/appsmith PR: 36596
File: app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java:475-475
Timestamp: 2024-10-04T05:51:15.138Z
Learning: In `app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java`, the variable `newsletterSignedUpUserEmail` is always expected to be non-null because the user's email is always present in the user object, so an additional null check is unnecessary.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/UserController.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.java
🧬 Code graph analysis (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/HostUrlHelper.java (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/HostUrlHelperCE.java (1)
Slf4j(14-187)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/HostUrlHelperCE.java (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/HostUrlHelper.java (1)
Slf4j(7-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (9)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/HostUrlHelper.java (1)
1-9: LGTM!Clean wrapper class following the CE/EE pattern. The
@Componentannotation enables Spring DI for the parent class's@Valuefields.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/UserController.java (1)
21-38: LGTM!Standard dependency injection pattern. The
HostUrlHelperis properly wired through the constructor and forwarded to the parent class.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.java (3)
51-51: LGTM!The
HostUrlHelperis properly declared as a final field for constructor injection via Lombok's@RequiredArgsConstructor.
86-103: Excellent security improvement!The method now derives the client-facing URL via
HostUrlHelperinstead of trusting theOriginheader directly. This prevents account takeover attacks where an attacker could spoof the Origin header to redirect password reset emails to malicious domains.The Origin header is now only used for logging discrepancies, which is the correct approach.
187-200: Excellent security improvement!Consistent with the
forgotPasswordRequestfix, this method now derives URLs viaHostUrlHelperinstead of directly using theOriginheader. This prevents the same class of account takeover vulnerability in the email verification flow.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/HostUrlHelperCE.java (4)
26-65: Excellent security design!This method implements the core vulnerability fix by deliberately ignoring the
Originheader for URL derivation, using it only for logging discrepancies. The priority order (operator config → request metadata) ensures operators can enforce canonical URLs while still supporting dynamic environments.The clear documentation and safe fallback (empty string) are well-designed.
96-106: LGTM!Standard and secure handling of reverse proxy headers.
X-Forwarded-*headers are operator-controlled (set by the proxy/load balancer), making them trustworthy sources for host derivation.
108-115: LGTM!The fallback to the standard
Hostheader is appropriate. In properly configured environments (especially behind proxies), this header reflects the actual host used to reach the server.
167-186: LGTM! Secure defaults.The method properly sanitizes configured domains and defaults to
httpswhen the scheme cannot be determined (line 182), which is the secure choice.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/HostUrlHelperCE.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/HostUrlHelperCE.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/HostUrlHelperCE.java
Show resolved
Hide resolved
Failed server tests
|
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Closing in favor of these |
1 participant
Key changes
Slack thread
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
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/19491671380
Commit: ad9ae53
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 19 Nov 2025 07:15:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit