fix(auth): validate Origin header against APPSMITH_BASE_URL #41426
Conversation
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
Contributor
WalkthroughNew admin setting Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
Note over Client,Service: Forgot Password / Email Verification Flow
Client->>Controller: POST request (with origin)
Controller->>Service: Call flow method
rect rgb(220, 240, 245)
Note over Service: validateBaseUrl()
Service->>Service: Check if APPSMITH_BASE_URL configured
alt URL configured
Service->>Service: Validate origin matches APPSMITH_BASE_URL
alt Mismatch
Service-->>Controller: Return error
else Match
Service->>Service: Proceed to token generation
end
else URL not configured
Service->>Service: Skip validation, proceed
end
end
Service->>Service: Process token generation
Service-->>Controller: Return success/result
Controller-->>Client: HTTP 200 response
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/cypress/locators/AdminsSettings.js(1 hunks)app/client/src/ce/pages/AdminSettings/config/configuration.tsx(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java(4 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.java
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/**/**.*
⚙️ CodeRabbit configuration file
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
Files:
app/client/cypress/locators/AdminsSettings.js
🧠 Learnings (6)
📓 Common learnings
Learnt from: sharat87
Repo: appsmithorg/appsmith PR: 29585
File: deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs:8-8
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The `APPSMITH_CUSTOM_DOMAIN` environment variable is not being deleted and should remain in use as per the current codebase configuration.
Learnt from: sharat87
Repo: appsmithorg/appsmith PR: 29585
File: deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs:8-8
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `APPSMITH_CUSTOM_DOMAIN` environment variable is not being deleted and should remain in use as per the current codebase configuration.
📚 Learning: 2024-07-24T08:29:41.208Z
Learnt from: abhishek-bandameedi
Repo: appsmithorg/appsmith PR: 35133
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBox2_spec.js:0-0
Timestamp: 2024-07-24T08:29:41.208Z
Learning: In Cypress tests for the Appsmith project, ensure the use of locator variables for selectors and include multiple assertions for comprehensive testing.
Applied to files:
app/client/cypress/locators/AdminsSettings.js
📚 Learning: 2024-10-08T15:32:34.115Z
Learnt from: sagar-qa007
Repo: appsmithorg/appsmith PR: 34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-07-16T06:44:55.118Z
Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep`, and other related sleep functions in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.
Applied to files:
app/client/cypress/locators/AdminsSettings.js
📚 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/services/ce/UserServiceCEImpl.java
📚 Learning: 2024-07-26T21:12:57.228Z
Learnt from: nidhi-nair
Repo: appsmithorg/appsmith PR: 29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:24-24
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The change from `private` to `protected` for the `applicationService` field in `ApplicationJsLibServiceCEImpl` class was confirmed to be intentional by the user nidhi-nair for subclass access.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
📚 Learning: 2024-12-25T06:50:40.623Z
Learnt from: trishaanand
Repo: appsmithorg/appsmith PR: 38359
File: app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java:78-82
Timestamp: 2024-12-25T06:50:40.623Z
Learning: In `CustomOAuth2UserServiceCEImpl`, the second repository call for finding users by case-insensitive email is needed because older users might have uppercase or mixed-case emails, while new users have emails stored in lowercase.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
🧬 Code graph analysis (1)
app/client/src/ce/pages/AdminSettings/config/configuration.tsx (1)
app/client/src/ce/pages/AdminSettings/config/types.ts (2)
Setting(54-108)SettingCategories(123-141)
⏰ 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). (10)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (5)
app/client/cypress/locators/AdminsSettings.js (1)
83-83: LGTM!The locator follows the established pattern and uses an attribute selector as required.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (4)
220-225: Validation integration looks good.The validation is correctly positioned before token generation, ensuring the security check happens early in the flow. The defer wrapper ensures proper reactive chain execution.
However, ensure the URL normalization issue in
validateBaseUrl(Line 124-138) is addressed.
227-282: Good refactoring - extracted helper improves maintainability.Extracting the token generation logic into a private helper method makes the code cleaner and separates the validation concern from the business logic.
851-856: Validation integration looks good.Consistent with the forgot password flow - validation happens before processing. The defer wrapper ensures proper reactive chain execution.
However, ensure the URL normalization issue in
validateBaseUrl(Line 124-138) is addressed.
858-931: Good refactoring - extracted helper improves maintainability.Extracting the email verification logic into a private helper method maintains consistency with the forgot password flow and improves code organization.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
Show resolved
Hide resolved
Failed server tests
|
sharat87
approved these changes
Nov 28, 2025
This was referenced Nov 28, 2025
subrata71
added a commit
to appsmithorg/appsmith-docs
that referenced
this pull request
Nov 28, 2025
## Description - Add new Security section to environment-variables.md - Document APPSMITH_BASE_URL for Origin header validation - Explain protection against account takeover attacks - Include configuration options (Admin Settings UI and env var) - Add backward compatibility notes and recommendations This addresses the account takeover vulnerability by documenting the Origin header validation feature for password reset and email verification requests. Related PRs: appsmithorg/appsmith-ee#8448 appsmithorg/appsmith#41426 ## Pull request type Check the appropriate box: - [ ] Review Fixes - [ ] Documentation Overhaul - [ ] Feature/Story - Link one or more Engineering Tickets * - [ ] A-Force - [ ] Error in documentation - [ ] Maintenance ## Documentation tickets Link to one or more documentation tickets: - ## Checklist From the below options, select the ones that are applicable: - [ ] Checked for Grammarly suggestions. - [ ] Adhered to the writing checklist. - [ ] Adhered to the media checklist. - [ ] Verified and updated cross-references or added redirect rules. - [ ] Tested the redirect rules on deploy preview. - [ ] Validated the modifications made to the content on the deploy preview. - [ ] Validated the CSS modifications on different screen sizes.
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.
Description
Prevent account takeover by validating Origin header in password reset and email verification endpoints against configured APPSMITH_BASE_URL.
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/8448
Fixes https://github.com/appsmithorg/appsmith/security/advisories/GHSA-7hf5-mc28-xmcv
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/19726406804
Commit: 3764386
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 27 Nov 2025 07:03:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.