Skip to content

fix(auth): prevent concurrent signup race conditions and lost updates #552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Namraa310806 wants to merge 2 commits into
base: master
Choose a base branch
from
+294 −11
Open

fix(auth): prevent concurrent signup race conditions and lost updates #552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1380ac
fix(auth): prevent concurrent signup race conditions and lost updates
Namraa310806 Jun 15, 2026
73de0dd
Fixing Backend test cases
Namraa310806 Jun 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"helmet": "^8.1.0",
"jsonwebtoken": "^9.0.3",
"multer": "^2.0.2",
"proper-lockfile": "^4.1.2",
"rate-limit-redis": "^4.2.3",
"react-hot-toast": "^2.6.0",
"redis": "^4.7.1",
Expand Down
1 change: 0 additions & 1 deletion server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ const {
summarizeSchema,
summarizeCredentialSchema,
sessionsLookupSchema,
knowledgeGapsSchema,
MAX_QUESTION_LENGTH,
} = require("./validators/schemas");
const { clientIpFromRequest } = require("./security/ip");
Expand Down
105 changes: 105 additions & 0 deletions server.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,111 @@ describe("route error responses", () => {
);
});

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);
});
Comment on lines +988 to +1014

@coderabbitai coderabbitai Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.


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);
});

Comment on lines +946 to +1050

@coderabbitai coderabbitai Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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.

test("successful upload response does not include a url field", async () => {
const originalPostForm = axios.postForm;
const originalPost = axios.post;
Expand Down
57 changes: 47 additions & 10 deletions src/controllers/authController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const fs = require("fs");
const path = require("path");
const bcrypt = require("bcryptjs");
const jwt = require("jsonwebtoken");
const lockfile = require("proper-lockfile");

const { validatePassword } = require("../utils/passwordValidator");

Expand All @@ -17,15 +18,38 @@ const getUsers = () => {
return JSON.parse(fs.readFileSync(usersFile));
};

const saveUsers = (users) => {
fs.writeFileSync(usersFile, JSON.stringify(users, null, 2));
const getUsersLocked = async () => {
const release = await lockfile.lock(usersFile, { retries: 3, stale: 5000 });
try {
const data = fs.readFileSync(usersFile, "utf8");
return { users: JSON.parse(data), release };
} catch (error) {
await release();
throw error;
}
};

const saveUsersLocked = async (users) => {
const tempFile = `${usersFile}.tmp`;
try {
fs.writeFileSync(tempFile, JSON.stringify(users, null, 2), "utf8");
fs.renameSync(tempFile, usersFile);
} catch (error) {
try {
fs.unlinkSync(tempFile);
} catch {
// Ignore cleanup errors - temp file might not exist
}
throw error;
}
};

const normalizeEmail = (email) => {
return email ? String(email).trim().toLowerCase() : email;
};

exports.signup = async (req, res) => {
let release = null;
try {
let { email, password } = req.body;

Expand All @@ -34,7 +58,7 @@ exports.signup = async (req, res) => {
message: "Email and password are required",
});
}

email = normalizeEmail(email);

const validation = validatePassword(password);
Expand All @@ -45,29 +69,34 @@ exports.signup = async (req, res) => {
});
}

const users = getUsers();
const hashedPassword = await bcrypt.hash(
password,
10
);

const { users, release: lockRelease } = await getUsersLocked();
release = lockRelease;

const existingUser = users.find(
(u) => u.email === email
);

if (existingUser) {
await release();
release = null;
return res.status(400).json({
message: "User already exists",
});
}

const hashedPassword = await bcrypt.hash(
password,
10
);

users.push({
email,
password: hashedPassword,
});

saveUsers(users);
await saveUsersLocked(users);
await release();
release = null;

const token = jwt.sign({ email }, SECRET, { expiresIn: "7d" });

Expand All @@ -76,6 +105,14 @@ exports.signup = async (req, res) => {
message: "Signup successful",
});
} catch (error) {
console.error("[signup] error:", error);
if (release) {
try {
await release();
} catch (releaseError) {
console.error("[signup] lock release error:", releaseError);
}
}
res.status(500).json({
message: "Server error",
});
Expand Down
Loading
Loading