fix(security): prevent super user creation race condition (GHSA-9wcp-79g5-5c3c)#41681
Merged
fix(security): prevent super user creation race condition (GHSA-9wcp-79g5-5c3c)#41681
Conversation
The /api/v1/users/super endpoint had a TOCTOU race condition where concurrent requests during initial setup could bypass the single-admin restriction. The isUsersEmpty() check and user creation were non-atomic, allowing multiple requests to pass the check before any user was created. This fix introduces an atomic sentinel document claim using MongoDB's findAndModify with upsert. Only the first request to insert the sentinel succeeds; all concurrent requests see the existing document and are rejected. The isUsersEmpty() check is retained as defense-in-depth. The sentinel approach works on both standalone MongoDB and replica sets, requires no external dependencies (Redis, distributed locks), and has zero race window since MongoDB guarantees document-level atomicity on findAndModify. CVSS 3.1: AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:H/A:H — 8.1 (HIGH)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds MongoDB-backed coordination for single-winner super-user creation: repository methods to atomically claim and release a creation slot, service delegations, signup flow gated on the claim (with release on failure), a migration to seed locks for existing installs, and tests for sequential and concurrent claims. Changes
Sequence DiagramsequenceDiagram
actor Client
participant UserSignup as UserSignupService
participant UserSvc as UserService
participant UserRepo as UserRepository
participant MongoDB
Client->>UserSignup: signupAndLoginSuper(request)
UserSignup->>UserSvc: claimSuperUserCreationSlot()
UserSvc->>UserRepo: claimSuperUserCreationSlot()
UserRepo->>MongoDB: findAndModify(filter {_id:"super-user-setup"}, upsert=true)
alt First claim -> document created
MongoDB-->>UserRepo: inserted document
UserRepo-->>UserSvc: true
UserSvc-->>UserSignup: true
UserSignup->>UserSvc: isUsersEmpty()
alt empty -> proceed
UserSignup-->>Client: create & login super user
else not empty -> fail
UserSignup->>UserSvc: releaseSuperUserCreationSlot()
UserSvc->>UserRepo: releaseSuperUserCreationSlot()
UserRepo->>MongoDB: remove lock document
UserRepo-->>UserSvc: void
UserSignup-->>Client: error UNAUTHORIZED_ACCESS
end
else Document exists -> claim lost
MongoDB-->>UserRepo: existing document
UserRepo-->>UserSvc: false
UserSvc-->>UserSignup: false
UserSignup-->>Client: error UNAUTHORIZED_ACCESS
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java (2)
175-178: Remove unnecessary fully-qualified class name.
Fluxis already imported on line 13. Use the simple name for consistency.♻️ Suggested fix
- List<Boolean> results = reactor.core.publisher.Flux.merge(claimMonos) + List<Boolean> results = Flux.merge(claimMonos) .collectList() .block();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java` around lines 175 - 178, In CustomUserRepositoryCEImplTest, replace the unnecessary fully-qualified reference reactor.core.publisher.Flux.merge with the simple imported Flux.merge (leave the claimMonos variable and collectList().block() logic unchanged) so the test uses the already-imported Flux type consistently; update the line using Flux.merge(claimMonos) instead of the fully-qualified name.
192-198: Consider using@BeforeEach/@AfterEachfor cleanup.Calling
dropSuperUserSetupLockCollection()at the start and end of each test is repetitive. Moving cleanup to lifecycle methods would reduce duplication and ensure consistent state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java` around lines 192 - 198, Move the repeated dropSuperUserSetupLockCollection() calls into JUnit lifecycle methods to ensure consistent cleanup: add a `@BeforeEach` method in CustomUserRepositoryCEImplTest that calls dropSuperUserSetupLockCollection() to ensure a clean state before each test, and optionally add an `@AfterEach` method that calls dropSuperUserSetupLockCollection() to guarantee teardown after each test; reference the existing private helper dropSuperUserSetupLockCollection() and invoke it from these annotated methods rather than duplicating calls in individual tests.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java (2)
33-34: Prefer constructor injection over field injection.Field injection with
@Autowiredmakes the class harder to test and hides dependencies. Consider constructor injection for consistency with Spring best practices.♻️ Suggested refactor
- `@Autowired` - private ReactiveMongoOperations reactiveMongoOperations; + private final ReactiveMongoOperations reactiveMongoOperations; + + public CustomUserRepositoryCEImpl(ReactiveMongoOperations reactiveMongoOperations) { + this.reactiveMongoOperations = reactiveMongoOperations; + }Note: You may need to adjust if this class already has a constructor or relies on proxy behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java` around lines 33 - 34, Replace the field-injected ReactiveMongoOperations in CustomUserRepositoryCEImpl with constructor injection: remove the `@Autowired` annotation on the reactiveMongoOperations field, add a constructor that accepts a ReactiveMongoOperations parameter and assigns it to the reactiveMongoOperations field (or make the field final and assign in the constructor), and update any existing constructors if present to include this dependency so the repository's dependency is explicit and testable.
74-77: Consider storingclaimedAtas a native Date type.Storing
Instant.now().toString()as a String works but loses native date querying/indexing benefits. For future debugging or auditing, a proper date type is preferable.♻️ Suggested change
- .setOnInsert("claimedAt", Instant.now().toString()); + .setOnInsert("claimedAt", new java.util.Date());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java` around lines 74 - 77, Replace storing claimedAt as a String by inserting a native Date object: change the Update construction that calls setOnInsert("claimedAt", Instant.now().toString()) to setOnInsert("claimedAt", Date.from(Instant.now())) (or new Date()), ensuring java.util.Date is imported; this preserves SUPER_USER_SETUP_LOCK_ID semantics and uses the same Update update and setOnInsert("claimedAt", ...) call site so claimedAt becomes a proper date type for querying/indexing.app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java (1)
192-216: Good defense-in-depth test.Verifies that even if the slot is claimed, the
isUsersEmpty()check still gates access when users already exist.Consider adding a happy-path test that mocks both
claimSuperUserCreationSlot()→trueandisUsersEmpty()→trueto verify successful super user creation proceeds correctly. This would complete the test matrix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java` around lines 192 - 216, Add a happy-path test in UserSignupTest that mocks userService.claimSuperUserCreationSlot() to return Mono.just(true) and userService.isUsersEmpty() to return Mono.just(true), constructs a UserSignupRequestDTO (same fields as the failing test), calls userSignup.signupAndLoginSuper(request, "http://localhost", exchange) and verifies via StepVerifier that the Mono emits a User with expected properties (email/name/state/isEnabled) and completes successfully; ensure you reference the same symbols (UserSignupRequestDTO, userService.claimSuperUserCreationSlot, userService.isUsersEmpty, userSignup.signupAndLoginSuper, StepVerifier) and assert the created user's fields match the request.
🤖 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-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java`:
- Around line 33-34: Replace the field-injected ReactiveMongoOperations in
CustomUserRepositoryCEImpl with constructor injection: remove the `@Autowired`
annotation on the reactiveMongoOperations field, add a constructor that accepts
a ReactiveMongoOperations parameter and assigns it to the
reactiveMongoOperations field (or make the field final and assign in the
constructor), and update any existing constructors if present to include this
dependency so the repository's dependency is explicit and testable.
- Around line 74-77: Replace storing claimedAt as a String by inserting a native
Date object: change the Update construction that calls setOnInsert("claimedAt",
Instant.now().toString()) to setOnInsert("claimedAt", Date.from(Instant.now()))
(or new Date()), ensuring java.util.Date is imported; this preserves
SUPER_USER_SETUP_LOCK_ID semantics and uses the same Update update and
setOnInsert("claimedAt", ...) call site so claimedAt becomes a proper date type
for querying/indexing.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java`:
- Around line 175-178: In CustomUserRepositoryCEImplTest, replace the
unnecessary fully-qualified reference reactor.core.publisher.Flux.merge with the
simple imported Flux.merge (leave the claimMonos variable and
collectList().block() logic unchanged) so the test uses the already-imported
Flux type consistently; update the line using Flux.merge(claimMonos) instead of
the fully-qualified name.
- Around line 192-198: Move the repeated dropSuperUserSetupLockCollection()
calls into JUnit lifecycle methods to ensure consistent cleanup: add a
`@BeforeEach` method in CustomUserRepositoryCEImplTest that calls
dropSuperUserSetupLockCollection() to ensure a clean state before each test, and
optionally add an `@AfterEach` method that calls
dropSuperUserSetupLockCollection() to guarantee teardown after each test;
reference the existing private helper dropSuperUserSetupLockCollection() and
invoke it from these annotated methods rather than duplicating calls in
individual tests.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java`:
- Around line 192-216: Add a happy-path test in UserSignupTest that mocks
userService.claimSuperUserCreationSlot() to return Mono.just(true) and
userService.isUsersEmpty() to return Mono.just(true), constructs a
UserSignupRequestDTO (same fields as the failing test), calls
userSignup.signupAndLoginSuper(request, "http://localhost", exchange) and
verifies via StepVerifier that the Mono emits a User with expected properties
(email/name/state/isEnabled) and completes successfully; ensure you reference
the same symbols (UserSignupRequestDTO, userService.claimSuperUserCreationSlot,
userService.isUsersEmpty, userSignup.signupAndLoginSuper, StepVerifier) and
assert the created user's fields match the request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f4dbb86-8bf3-472e-8a9f-cc47f8be2234
📒 Files selected for processing (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java (1)
65-66: Prevent sentinel-seeding errors from short-circuiting later startup steps.At Line 66,
seedSuperUserSetupLockIfNeeded()is in the main chain without local fallback. If it errors, license validation and org feature-flag cache refresh are skipped in this run. Consider isolating this step with a localonErrorResumeso startup-critical post-steps still execute.Proposed patch
// Prefill the server cache with anonymous user permission group ids. .then(cacheableRepositoryHelper.preFillAnonymousUserPermissionGroupIdsCache()) // Seed the super user setup sentinel for upgraded instances that already have users. - .then(userService.seedSuperUserSetupLockIfNeeded()) + .then(userService.seedSuperUserSetupLockIfNeeded().onErrorResume(errorSignal -> { + log.warn("Super user setup sentinel seeding failed; continuing startup: {}", errorSignal.getMessage()); + return Mono.empty(); + })) // Cold publisher to wait for upstream execution to complete as we have dependency on the instance // registration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java` around lines 65 - 66, The call to userService.seedSuperUserSetupLockIfNeeded() in InstanceConfig is currently in the main reactive chain and can short-circuit subsequent startup tasks if it errors; update that step to handle errors locally by adding a reactive fallback (e.g., onErrorResume) around seedSuperUserSetupLockIfNeeded() so any exception is logged/ignored for this step but does not propagate, ensuring the subsequent license validation and org feature-flag cache refresh steps still run; locate the chain that calls seedSuperUserSetupLockIfNeeded() and wrap only that Mono/Flux with onErrorResume to swallow or log the error and continue.
🤖 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-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java`:
- Around line 65-66: The call to userService.seedSuperUserSetupLockIfNeeded() in
InstanceConfig is currently in the main reactive chain and can short-circuit
subsequent startup tasks if it errors; update that step to handle errors locally
by adding a reactive fallback (e.g., onErrorResume) around
seedSuperUserSetupLockIfNeeded() so any exception is logged/ignored for this
step but does not propagate, ensuring the subsequent license validation and org
feature-flag cache refresh steps still run; locate the chain that calls
seedSuperUserSetupLockIfNeeded() and wrap only that Mono/Flux with onErrorResume
to swallow or log the error and continue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be6cff87-de86-4ce8-8d9f-240cc1d309f0
📒 Files selected for processing (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java
…gration Adds a Mongock migration (075) that runs once during upgrade. For existing instances that already have non-system users (meaning the super admin setup completed before this fix), the migration inserts the sentinel document into the superUserSetupLock collection. This ensures the /api/v1/users/super endpoint is properly guarded from the first moment the upgraded instance starts serving requests. For fresh instances with no users, the migration is a no-op — the sentinel will be atomically claimed by the first legitimate setup request via the findAndModify mechanism in the first commit.
subrata71
force-pushed
the
fix/super-user-race-condition
branch
from
17e9058 to
b3e2486
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java (3)
36-37: Consider adding a comment explaining the intentionally empty rollback.The empty rollback is correct (removing the sentinel would re-expose the race condition), but a brief inline comment would clarify the intent for future maintainers.
📝 Suggested comment
`@RollbackExecution` -public void rollbackExecution() {} +public void rollbackExecution() { + // Intentionally empty: removing the sentinel would re-expose the TOCTOU vulnerability +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java` around lines 36 - 37, Add a brief inline comment inside the rollbackExecution method of the Migration075SeedSuperUserSetupLock class explaining that the method is intentionally empty and why (removing the sentinel would re-expose the race condition), so future maintainers understand the intent; reference the `@RollbackExecution-annotated` rollbackExecution method and document that no rollback is performed on purpose to preserve the sentinel protection.
65-65: Consider defensive error handling for the insert.The plain
insert()relies on Mongock's migration locking to prevent concurrent execution. For defense-in-depth (consistent with the PR's approach), you could catchDuplicateKeyExceptionand log/skip gracefully rather than failing the migration.This is unlikely to matter in practice since Mongock serializes migrations, but it would align with the application code's defensive style (see
claimSuperUserCreationSlot()which handles this case explicitly).🛡️ Optional defensive wrapper
-mongoTemplate.insert(sentinel, SUPER_USER_SETUP_COLLECTION); -log.info("Seeded super user setup sentinel for existing instance with {} non-system user(s).", - nonSystemUserCount); +try { + mongoTemplate.insert(sentinel, SUPER_USER_SETUP_COLLECTION); + log.info("Seeded super user setup sentinel for existing instance with {} non-system user(s).", + nonSystemUserCount); +} catch (org.springframework.dao.DuplicateKeyException e) { + log.info("Sentinel was inserted concurrently. Skipping."); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java` at line 65, The insert of the sentinel document in Migration075SeedSuperUserSetupLock currently calls mongoTemplate.insert(sentinel, SUPER_USER_SETUP_COLLECTION) without handling DuplicateKeyException; update the migration method to wrap that insert in a try/catch that specifically catches org.springframework.dao.DuplicateKeyException (or the Mongo duplicate key equivalent), and on that exception log a debug/info message and skip/return gracefully so the migration does not fail if the sentinel already exists, keeping behavior consistent with claimSuperUserCreationSlot().
24-24: Emptyauthorfield.The
authorfield is blank. Consider adding the PR author's username or team identifier for traceability in Mongock's changelog history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java` at line 24, The `@ChangeUnit` annotation on the Migration075SeedSuperUserSetupLock class has an empty author attribute; update the annotation (ChangeUnit(order = "075", id = "seed-super-user-setup-lock", author = "")) to include a non-empty identifier (e.g., your GitHub username or team name) so Mongock changelogs have traceability—edit the author value directly in the Migration075SeedSuperUserSetupLock class annotation.
🤖 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-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java`:
- Around line 36-37: Add a brief inline comment inside the rollbackExecution
method of the Migration075SeedSuperUserSetupLock class explaining that the
method is intentionally empty and why (removing the sentinel would re-expose the
race condition), so future maintainers understand the intent; reference the
`@RollbackExecution-annotated` rollbackExecution method and document that no
rollback is performed on purpose to preserve the sentinel protection.
- Line 65: The insert of the sentinel document in
Migration075SeedSuperUserSetupLock currently calls
mongoTemplate.insert(sentinel, SUPER_USER_SETUP_COLLECTION) without handling
DuplicateKeyException; update the migration method to wrap that insert in a
try/catch that specifically catches
org.springframework.dao.DuplicateKeyException (or the Mongo duplicate key
equivalent), and on that exception log a debug/info message and skip/return
gracefully so the migration does not fail if the sentinel already exists,
keeping behavior consistent with claimSuperUserCreationSlot().
- Line 24: The `@ChangeUnit` annotation on the Migration075SeedSuperUserSetupLock
class has an empty author attribute; update the annotation (ChangeUnit(order =
"075", id = "seed-super-user-setup-lock", author = "")) to include a non-empty
identifier (e.g., your GitHub username or team name) so Mongock changelogs have
traceability—edit the author value directly in the
Migration075SeedSuperUserSetupLock class annotation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bab08d29-3611-4426-8009-643e583f4e7d
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java
Failed server tests
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java (1)
169-177: Consider extracting duplicatedUserSignupRequestDTOconstruction.The request setup is repeated in both tests. A small helper would reduce maintenance overhead and keep test intent focused.
Refactor sketch
+ private UserSignupRequestDTO createSuperSignupRequest() { + UserSignupRequestDTO request = new UserSignupRequestDTO(); + request.setEmail("admin@test.com"); + request.setPassword("ValidP@ssw0rd!"); + request.setName("Admin"); + request.setSource(LoginSource.FORM); + request.setState(UserState.ACTIVATED); + request.setIsEnabled(true); + request.setAllowCollectingAnonymousData(false); + return request; + } ... - UserSignupRequestDTO request = new UserSignupRequestDTO(); - request.setEmail("admin@test.com"); - request.setPassword("ValidP@ssw0rd!"); - request.setName("Admin"); - request.setSource(LoginSource.FORM); - request.setState(UserState.ACTIVATED); - request.setIsEnabled(true); - request.setAllowCollectingAnonymousData(false); + UserSignupRequestDTO request = createSuperSignupRequest(); ... - UserSignupRequestDTO request = new UserSignupRequestDTO(); - request.setEmail("admin@test.com"); - request.setPassword("ValidP@ssw0rd!"); - request.setName("Admin"); - request.setSource(LoginSource.FORM); - request.setState(UserState.ACTIVATED); - request.setIsEnabled(true); - request.setAllowCollectingAnonymousData(false); + UserSignupRequestDTO request = createSuperSignupRequest();Also applies to: 195-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java` around lines 169 - 177, Several tests duplicate construction of UserSignupRequestDTO; extract that block into a reusable helper (e.g., a private method in UserSignupTest such as buildDefaultUserSignupRequest or createDefaultSignupRequest) that returns a populated UserSignupRequestDTO with Email, Password, Name, Source (LoginSource.FORM), State (UserState.ACTIVATED), isEnabled(true) and allowCollectingAnonymousData(false); replace the inline construction in both test methods with calls to this helper so tests read cleaner and changes to defaults are maintained in one place.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java (1)
24-24: Consider filling in the author field.The
authorfield is empty. While not a functional issue, populating it improves traceability in the migration history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java` at line 24, The `@ChangeUnit` annotation on class Migration075SeedSuperUserSetupLock has an empty author value; update the annotation's author attribute (the string in `@ChangeUnit`(order = "075", id = "seed-super-user-setup-lock", author = "")) to a non-empty identifier (e.g., your name or handle) so the migration record includes traceability information for the Migration075SeedSuperUserSetupLock change unit.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java (2)
33-34: Consider constructor injection for better testability.Field injection with
@Autowiredworks, but constructor injection is generally preferred for easier unit testing and clearer dependency declaration. If this class follows an existing pattern in the codebase, feel free to keep as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java` around lines 33 - 34, Replace field injection of ReactiveMongoOperations in CustomUserRepositoryCEImpl with constructor injection: remove the `@Autowired` private ReactiveMongoOperations reactiveMongoOperations field and add a constructor that accepts ReactiveMongoOperations (and other dependencies if present) and assigns it to a final field, updating usages to the new final field to improve testability and make dependencies explicit.
74-77: RedundantsetOnInsertfor_id.Line 75's
setOnInsert("_id", ...)is unnecessary. MongoDB automatically uses the_idfrom the query filter during upsert. No harm done, just a minor cleanup opportunity.♻️ Optional cleanup
Update update = new Update() - .setOnInsert("_id", SUPER_USER_SETUP_LOCK_ID) .setOnInsert("status", "CLAIMED") .setOnInsert("claimedAt", Instant.now().toString());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java` around lines 74 - 77, The Update builder is calling setOnInsert("_id", SUPER_USER_SETUP_LOCK_ID) which is redundant because MongoDB uses the query filter's _id for upserts; remove the call to setOnInsert("_id", ...) from the Update construction (leave the other setOnInsert calls for "status" and "claimedAt" intact) — locate the Update variable (Update update = new Update() ...) and drop the setOnInsert referencing SUPER_USER_SETUP_LOCK_ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java`:
- Around line 41-63: The exists() then insert() sequence in
Migration075SeedSuperUserSetupLock is racy: when multiple instances run the
migration they can both pass mongoTemplate.exists(...) for
SUPER_USER_SETUP_LOCK_ID and one insert will throw DuplicateKeyException; wrap
the mongoTemplate.insert(...) call (or replace with an atomic upsert) in a
try/catch that catches DuplicateKeyException
(org.springframework.dao.DuplicateKeyException or
com.mongodb.DuplicateKeyException), treat it as a benign race (log an info/debug
message referencing SUPER_USER_SETUP_LOCK_ID and "migration-075") and return
without failing the migration so concurrent starts succeed gracefully.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java`:
- Around line 190-212: The test lacks an interaction assertion to confirm the
claimed-slot path reached userService.isUsersEmpty(); after the
StepVerifier.verify() add a Mockito.verify(userService).isUsersEmpty() (or
verify(userService, times(1)).isUsersEmpty()) to assert the method was invoked
during signupAndLoginSuper_WhenSlotClaimedButUsersExist_RaisesUnauthorized,
ensuring the branch was actually executed.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java`:
- Line 24: The `@ChangeUnit` annotation on class
Migration075SeedSuperUserSetupLock has an empty author value; update the
annotation's author attribute (the string in `@ChangeUnit`(order = "075", id =
"seed-super-user-setup-lock", author = "")) to a non-empty identifier (e.g.,
your name or handle) so the migration record includes traceability information
for the Migration075SeedSuperUserSetupLock change unit.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java`:
- Around line 33-34: Replace field injection of ReactiveMongoOperations in
CustomUserRepositoryCEImpl with constructor injection: remove the `@Autowired`
private ReactiveMongoOperations reactiveMongoOperations field and add a
constructor that accepts ReactiveMongoOperations (and other dependencies if
present) and assigns it to a final field, updating usages to the new final field
to improve testability and make dependencies explicit.
- Around line 74-77: The Update builder is calling setOnInsert("_id",
SUPER_USER_SETUP_LOCK_ID) which is redundant because MongoDB uses the query
filter's _id for upserts; remove the call to setOnInsert("_id", ...) from the
Update construction (leave the other setOnInsert calls for "status" and
"claimedAt" intact) — locate the Update variable (Update update = new Update()
...) and drop the setOnInsert referencing SUPER_USER_SETUP_LOCK_ID.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java`:
- Around line 169-177: Several tests duplicate construction of
UserSignupRequestDTO; extract that block into a reusable helper (e.g., a private
method in UserSignupTest such as buildDefaultUserSignupRequest or
createDefaultSignupRequest) that returns a populated UserSignupRequestDTO with
Email, Password, Name, Source (LoginSource.FORM), State (UserState.ACTIVATED),
isEnabled(true) and allowCollectingAnonymousData(false); replace the inline
construction in both test methods with calls to this helper so tests read
cleaner and changes to defaults are maintained in one place.
🪄 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: 604eb891-7729-4b0a-af4a-ed206b5241a7
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java
✅ Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java
Collaborator
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/23837048906. |
|
Deploy-Preview-URL: https://ce-41681.dp.appsmith.com |
subrata71
changed the title
sondermanish
reviewed
Apr 1, 2026
| if (!Boolean.TRUE.equals(claimed)) { | ||
| return Mono.error(new AppsmithException(AppsmithError.UNAUTHORIZED_ACCESS)); | ||
| } | ||
| return userService.isUsersEmpty(); |
Contributor
There was a problem hiding this comment.
what happens to the claim when further step fails? i.e. does the claim expire?
sondermanish
reviewed
Apr 1, 2026
| Query query = new Query(Criteria.where("_id").is(SUPER_USER_SETUP_LOCK_ID)); | ||
| Update update = new Update() | ||
| .setOnInsert("_id", SUPER_USER_SETUP_LOCK_ID) | ||
| .setOnInsert("status", "CLAIMED") |
Contributor
There was a problem hiding this comment.
Is this claim ephemeral?
Collaborator
Author
There was a problem hiding this comment.
No but it's released on failure. Once setup succeeds, it stays permanently
sondermanish
reviewed
Apr 1, 2026
| @ChangeUnit(order = "075", id = "seed-super-user-setup-lock", author = "") | ||
| public class Migration075SeedSuperUserSetupLock { | ||
|
|
||
| private static final String SUPER_USER_SETUP_COLLECTION = "superUserSetupLock"; |
Contributor
There was a problem hiding this comment.
nit: this could come from a central place. not to be minded of
2 tasks
- Add releaseSuperUserCreationSlot() to release the sentinel if super user creation fails after claiming the slot (e.g., password validation error, DB write failure). This prevents the instance from being bricked by a permanently claimed but unused sentinel. - Extract createAndSetupSuperUser() private method to cleanly scope the onErrorResume handler that triggers the release. - Catch DuplicateKeyException in Migration075 for defense-in-depth against concurrent migration execution. - Add verify(userService).isUsersEmpty() interaction assertion in test. - Fix fully-qualified Flux reference in concurrent test. Addresses review comments from @sondermanish and @coderabbitai.
Contributor
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/solutions/ce/UserSignupCEImpl.java (1)
303-385:⚠️ Potential issue | 🟠 MajorReleasing the slot here still leaves bootstrap unrecoverable.
signupAndLogin(...)persists the first user beforemakeInstanceAdministrator(...)runs. If elevation fails, Lines 383-385 only remove the sentinel; the next attempt still failsisUsersEmpty(), so the instance can end up with no Instance Administrator and no retry path. Please make admin elevation part of the initial create flow, or compensate by deleting the created user before releasing the slot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java` around lines 303 - 385, The current flow persists a user in signupAndLogin(...) before calling userUtils.makeInstanceAdministrator(...), so if elevation fails the onErrorResume only calls userService.releaseSuperUserCreationSlot() leaving a created, non-admin user and making bootstrap unrecoverable; fix by ensuring the created user is removed before releasing the slot (e.g., invoke userService.deleteById(...) or userService.delete(...) for the user created by signupAndLogin() and wait for that Mono to complete, then call userService.releaseSuperUserCreationSlot()), or alternatively move the makeInstanceAdministrator(...) call into the initial create/login flow (signupAndLogin or its helper) so user creation+elevation are atomic.
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java (1)
134-197: Make the sentinel cleanup fixture-scoped and narrower.If one assertion fails, the trailing cleanup never runs, and
dropCollection(...)is wider than these tests need. Prefer removing just the fixed lock document in@BeforeEach/@AfterEach; that avoids the broad catch and reduces cross-test interference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java` around lines 134 - 197, Replace the broad dropSuperUserSetupLockCollection() that drops the whole collection with a narrower per-test remove of the sentinel lock document: add `@BeforeEach` and `@AfterEach` methods that run reactiveMongoOperations.remove(Query.query(Criteria.where("<uniqueLockField>").is("<uniqueLockValue>")), "superUserSetupLock").block() (use the actual lock document field/value used by claimSuperUserCreationSlot()), remove or stop calling the old dropSuperUserSetupLockCollection() in the tests (and delete that helper), and ensure tests (claimSuperUserCreationSlot_*) only delete that single lock document so cleanup runs even if an assertion fails and avoids dropping the collection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java`:
- Around line 303-385: The current flow persists a user in signupAndLogin(...)
before calling userUtils.makeInstanceAdministrator(...), so if elevation fails
the onErrorResume only calls userService.releaseSuperUserCreationSlot() leaving
a created, non-admin user and making bootstrap unrecoverable; fix by ensuring
the created user is removed before releasing the slot (e.g., invoke
userService.deleteById(...) or userService.delete(...) for the user created by
signupAndLogin() and wait for that Mono to complete, then call
userService.releaseSuperUserCreationSlot()), or alternatively move the
makeInstanceAdministrator(...) call into the initial create/login flow
(signupAndLogin or its helper) so user creation+elevation are atomic.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java`:
- Around line 134-197: Replace the broad dropSuperUserSetupLockCollection() that
drops the whole collection with a narrower per-test remove of the sentinel lock
document: add `@BeforeEach` and `@AfterEach` methods that run
reactiveMongoOperations.remove(Query.query(Criteria.where("<uniqueLockField>").is("<uniqueLockValue>")),
"superUserSetupLock").block() (use the actual lock document field/value used by
claimSuperUserCreationSlot()), remove or stop calling the old
dropSuperUserSetupLockCollection() in the tests (and delete that helper), and
ensure tests (claimSuperUserCreationSlot_*) only delete that single lock
document so cleanup runs even if an assertion fails and avoids dropping the
collection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47fa39eb-0a6c-436c-995d-ea8d5ef7a25d
📒 Files selected for processing (8)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/solutions/UserSignupTest.java
✅ Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075SeedSuperUserSetupLock.java
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java
sondermanish
approved these changes
Apr 2, 2026
Failed server tests
|
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
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
TL;DR: Fix a critical TOCTOU race condition in the
/api/v1/users/superendpoint that allowed concurrent requests to bypass the single-admin restriction during initial Appsmith setup, enabling multiple unauthorized Instance Administrators.Root Cause
The
signupAndLoginSuper()method inUserSignupCEImpl.javaperformed a non-atomic check-then-act sequence:isUsersEmpty()queries MongoDB — returnstrueif no users existsignupAndLogin()creates the user +makeInstanceAdministrator()grants adminIn the reactive WebFlux environment, concurrent requests all saw an empty database and all proceeded to create admin users.
Fixes https://linear.app/appsmith/issue/APP-15067/security-high-super-user-creation-race-condition-allows-multiple
Fixes https://github.com/appsmithorg/appsmith/security/advisories/GHSA-9wcp-79g5-5c3c
Fix
Introduces an atomic sentinel document claim using MongoDB's
findAndModifywithupsert=true:superUserSetupLockcollection with a fixed_iddocumentfindAndModifywithupsertis atomic at the document level — only the first request to upsert winsisUsersEmpty()check is retained as defense-in-depthWhy this approach:
Security Impact
Files Changed
CustomUserRepositoryCE.javaclaimSuperUserCreationSlot()interface methodCustomUserRepositoryCEImpl.javafindAndModifyUserServiceCE.javaclaimSuperUserCreationSlot()in service interfaceUserServiceCEImpl.javaUserSignupCEImpl.javaisUsersEmpty()checkCustomUserRepositoryCEImplTest.javaUserSignupTest.javaAutomation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/23942098312
Commit: 2f7328a
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 03 Apr 2026 11:03:13 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Migrations