fix(auth): prevent concurrent signup race conditions and lost updates#552
Open
Namraa310806 wants to merge 2 commits into
Open
fix(auth): prevent concurrent signup race conditions and lost updates#552Namraa310806 wants to merge 2 commits into
Namraa310806 wants to merge 2 commits into
Conversation
|
@Namraa310806 is attempting to deploy a commit to the firefistisdead's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Warning Review limit reached
More reviews will be available in 52 minutes and 35 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds ChangesConcurrent-safe signup with file locking
Sequence Diagram(s)sequenceDiagram
participant Client
participant signup as signup handler
participant getUsersLocked
participant saveUsersLocked
participant users.json
Client->>signup: POST /api/auth/signup (email, password)
signup->>signup: validate fields & password strength
signup->>getUsersLocked: acquire lock, read users.json
getUsersLocked->>users.json: readFileSync + JSON.parse
getUsersLocked-->>signup: users[], releaseFn
alt email already exists
signup->>getUsersLocked: releaseFn()
signup-->>Client: 400 User already exists
else new user
signup->>signup: bcrypt.hash(password)
signup->>saveUsersLocked: users.push(newUser), write + release
saveUsersLocked->>users.json: write via tmp, rename
saveUsersLocked->>getUsersLocked: releaseFn()
signup-->>Client: 201 + JWT token
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
backend
![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: 3
🧹 Nitpick comments (1)
src/controllers/authController.js (1)
2-2: ⚡ Quick winRemove unused symbols flagged by CI (
fsPromises,saveUsers).Line 2 and Lines 22-24 are now dead after the locked helper migration, and they keep lint warnings active.
Proposed cleanup
-const fsPromises = require("fs/promises"); @@ -const saveUsers = (users) => { - fs.writeFileSync(usersFile, JSON.stringify(users, null, 2)); -};Also applies to: 22-24
🤖 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 `@src/controllers/authController.js` at line 2, Remove the unused fsPromises import statement from line 2 and delete the saveUsers function definition from lines 22-24. These symbols became dead code after the locked helper migration and are causing lint warnings. Ensure no other code references these symbols before deletion.Sources: Linters/SAST tools, Pipeline failures
🤖 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 `@server.test.js`:
- Around line 988-1014: The test "POST /api/auth/signup - concurrent
registrations prevent lost updates" currently only verifies HTTP response status
codes and token presence, but does not verify that all three users were actually
persisted in the database. A lost-update bug could still pass this test by
returning 201 for all concurrent requests while failing to store some users.
After the Promise.all(promises) completes and you verify the successCount, add
post-signup verification by attempting to login each of the three users with
their respective email and password credentials. Verify that each login attempt
succeeds to confirm all three accounts were actually persisted, ensuring the
test truly catches lost-update bugs rather than just checking surface-level
response codes.
- Around line 946-1050: The signup tests (POST /api/auth/signup - single user
registration succeeds, POST /api/auth/signup - duplicate email is rejected, POST
/api/auth/signup - concurrent registrations prevent lost updates, POST
/api/auth/signup - validates password strength, POST /api/auth/signup - rejects
missing email or password) are writing directly to the real src/data/users.json
file and not cleaning up afterward, causing state to persist across test runs.
To fix this, implement test isolation by either mocking the user data store with
an in-memory implementation specific to each test, using a temporary test
fixture that gets restored after each test, or implementing setup/teardown hooks
to clear or reset the users.json file before and after each test runs. Ensure
that each test operates on isolated state that does not affect subsequent test
runs or the committed repository state.
In `@src/controllers/authController.js`:
- Around line 43-45: The empty catch block at line 45 that follows the
fs.unlinkSync(tempFile) call is violating the ESLint no-empty rule and is hiding
potential cleanup failures. Replace the empty catch block with proper error
handling by either logging the error that occurred during the file deletion
attempt (using a logger at an appropriate level like warn or debug since this is
cleanup code) or adding an explanatory comment if the error is intentionally
ignored. This will satisfy the linter and provide visibility into cleanup
failures for debugging purposes.
---
Nitpick comments:
In `@src/controllers/authController.js`:
- Line 2: Remove the unused fsPromises import statement from line 2 and delete
the saveUsers function definition from lines 22-24. These symbols became dead
code after the locked helper migration and are causing lint warnings. Ensure no
other code references these symbols before deletion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6cadb328-991e-440d-a208-fe9e282f4a8e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonserver.test.jssrc/controllers/authController.jssrc/data/users.json
Comment on lines
+946
to
+1050
| test("POST /api/auth/signup - single user registration succeeds", async () => { | ||
| const testEmail = `test-single-${Date.now()}@example.com`; | ||
| const res = await fetch(`${baseUrl}/api/auth/signup`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| email: testEmail, | ||
| password: "Test@1234", | ||
| }), | ||
| }); | ||
| assert.equal(res.status, 201); | ||
| const data = await res.json(); | ||
| assert.ok(data.token); | ||
| assert.equal(data.message, "Signup successful"); | ||
| }); | ||
|
|
||
| test("POST /api/auth/signup - duplicate email is rejected", async () => { | ||
| const testEmail = `test-dup-${Date.now()}@example.com`; | ||
|
|
||
| const firstRes = await fetch(`${baseUrl}/api/auth/signup`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| email: testEmail, | ||
| password: "Test@1234", | ||
| }), | ||
| }); | ||
| assert.equal(firstRes.status, 201); | ||
|
|
||
| const secondRes = await fetch(`${baseUrl}/api/auth/signup`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| email: testEmail, | ||
| password: "Different@1234", | ||
| }), | ||
| }); | ||
| assert.equal(secondRes.status, 400); | ||
| const data = await secondRes.json(); | ||
| assert.equal(data.message, "User already exists"); | ||
| }); | ||
|
|
||
| test("POST /api/auth/signup - concurrent registrations prevent lost updates", async () => { | ||
| const timestamp = Date.now(); | ||
| const emails = [ | ||
| `concurrent-1-${timestamp}@example.com`, | ||
| `concurrent-2-${timestamp}@example.com`, | ||
| `concurrent-3-${timestamp}@example.com`, | ||
| ]; | ||
|
|
||
| const promises = emails.map((email) => | ||
| fetch(`${baseUrl}/api/auth/signup`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| email, | ||
| password: "Test@1234", | ||
| }), | ||
| }) | ||
| ); | ||
|
|
||
| const results = await Promise.all(promises); | ||
|
|
||
| const successCount = results.filter((r) => r.status === 201).length; | ||
| assert.equal(successCount, 3, "All concurrent registrations should succeed"); | ||
|
|
||
| const data = await results[0].json(); | ||
| assert.ok(data.token); | ||
| }); | ||
|
|
||
| test("POST /api/auth/signup - validates password strength", async () => { | ||
| const testEmail = `test-weak-${Date.now()}@example.com`; | ||
| const res = await fetch(`${baseUrl}/api/auth/signup`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| email: testEmail, | ||
| password: "weak", | ||
| }), | ||
| }); | ||
| assert.equal(res.status, 400); | ||
| const data = await res.json(); | ||
| assert.match(data.message, /Password must contain/); | ||
| }); | ||
|
|
||
| test("POST /api/auth/signup - rejects missing email or password", async () => { | ||
| const res1 = await fetch(`${baseUrl}/api/auth/signup`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| password: "Test@1234", | ||
| }), | ||
| }); | ||
| assert.equal(res1.status, 400); | ||
|
|
||
| const res2 = await fetch(`${baseUrl}/api/auth/signup`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| email: "test@example.com", | ||
| }), | ||
| }); | ||
| assert.equal(res2.status, 400); | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
Signup tests are non-hermetic and mutate the committed auth datastore.
These tests write directly to the real src/data/users.json and never restore it, which causes cross-run state bleed and fixture churn (already visible in this PR’s users.json delta).
One practical isolation pattern
+const fs = require("node:fs");
+const path = require("node:path");
+const usersFile = path.join(__dirname, "src/data/users.json");
+let usersSnapshot;
+
+before(() => {
+ usersSnapshot = fs.readFileSync(usersFile, "utf8");
+});
+
+after(() => {
+ fs.writeFileSync(usersFile, usersSnapshot, "utf8");
+});🤖 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 `@server.test.js` around lines 946 - 1050, The signup tests (POST
/api/auth/signup - single user registration succeeds, POST /api/auth/signup -
duplicate email is rejected, POST /api/auth/signup - concurrent registrations
prevent lost updates, POST /api/auth/signup - validates password strength, POST
/api/auth/signup - rejects missing email or password) are writing directly to
the real src/data/users.json file and not cleaning up afterward, causing state
to persist across test runs. To fix this, implement test isolation by either
mocking the user data store with an in-memory implementation specific to each
test, using a temporary test fixture that gets restored after each test, or
implementing setup/teardown hooks to clear or reset the users.json file before
and after each test runs. Ensure that each test operates on isolated state that
does not affect subsequent test runs or the committed repository state.
Comment on lines
+988
to
+1014
| test("POST /api/auth/signup - concurrent registrations prevent lost updates", async () => { | ||
| const timestamp = Date.now(); | ||
| const emails = [ | ||
| `concurrent-1-${timestamp}@example.com`, | ||
| `concurrent-2-${timestamp}@example.com`, | ||
| `concurrent-3-${timestamp}@example.com`, | ||
| ]; | ||
|
|
||
| const promises = emails.map((email) => | ||
| fetch(`${baseUrl}/api/auth/signup`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| email, | ||
| password: "Test@1234", | ||
| }), | ||
| }) | ||
| ); | ||
|
|
||
| const results = await Promise.all(promises); | ||
|
|
||
| const successCount = results.filter((r) => r.status === 201).length; | ||
| assert.equal(successCount, 3, "All concurrent registrations should succeed"); | ||
|
|
||
| const data = await results[0].json(); | ||
| assert.ok(data.token); | ||
| }); |
Contributor
There was a problem hiding this comment.
The concurrent-signup test does not verify persisted outcomes.
Right now it only checks response status/token. A lost-update bug can still return 201 for all requests while dropping one or more writes. Add post-signup verification (e.g., login each created user).
Strengthen the assertion
const results = await Promise.all(promises);
const successCount = results.filter((r) => r.status === 201).length;
assert.equal(successCount, 3, "All concurrent registrations should succeed");
- const data = await results[0].json();
- assert.ok(data.token);
+ const loginResults = await Promise.all(
+ emails.map((email) =>
+ fetch(`${baseUrl}/api/auth/login`, {
+ method: "POST",
+ headers: { "Content-Type": "application/json" },
+ body: JSON.stringify({ email, password: "Test@1234" }),
+ }),
+ ),
+ );
+ assert.equal(
+ loginResults.filter((r) => r.status === 200).length,
+ 3,
+ "Each concurrently created user must be persisted and able to log in",
+ );🤖 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 `@server.test.js` around lines 988 - 1014, The test "POST /api/auth/signup -
concurrent registrations prevent lost updates" currently only verifies HTTP
response status codes and token presence, but does not verify that all three
users were actually persisted in the database. A lost-update bug could still
pass this test by returning 201 for all concurrent requests while failing to
store some users. After the Promise.all(promises) completes and you verify the
successCount, add post-signup verification by attempting to login each of the
three users with their respective email and password credentials. Verify that
each login attempt succeeds to confirm all three accounts were actually
persisted, ensuring the test truly catches lost-update bugs rather than just
checking surface-level response codes.
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
This PR fixes a race condition in the user registration flow that could cause lost updates and inconsistent user data during concurrent signup requests.
Previously, the authentication system used a read-modify-write pattern on
users.jsonwithout synchronization. Multiple simultaneous registration requests could read the same file state, perform independent modifications, and overwrite each other's changes when writing back to disk.This update introduces file locking, atomic writes, and concurrency-safe registration handling to ensure reliable user persistence under concurrent workloads.
Changes Made
Concurrency-Safe User Registration
proper-lockfile.users.jsonat a time.Duplicate User Validation Under Lock
Moved duplicate-email validation inside the protected critical section.
This ensures:
Atomic File Writes
Implemented atomic write operations using:
Benefits:
Lock Cleanup & Error Handling
Added safe lock release logic for:
This prevents stale locks and improves recovery behavior.
Regression Test Coverage
Added concurrent registration tests covering:
Reliability Impact
This change improves:
The application can now safely process concurrent signup requests without risking lost registrations or corrupted user data.
Security Impact
This change reduces the risk of:
Files Modified
Verification Checklist
Related Issue
Fixes #503
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Chores