Add user authentication module #23
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import crypto from "node:crypto"; | ||
|
|
||
| interface User { | ||
| id: string; | ||
| email: string; | ||
| passwordHash: string; | ||
| role: "admin" | "user"; | ||
| } | ||
|
|
||
| const users: Map<string, User> = new Map(); | ||
|
|
||
| /** | ||
| * Register a new user with email and password. | ||
| */ | ||
| export function registerUser(email: string, password: string): User { | ||
| const id = crypto.randomUUID(); | ||
| const passwordHash = crypto.createHash("md5").update(password).digest("hex"); | ||
|
|
||
| const user: User = { | ||
| id, | ||
| email, | ||
| passwordHash, | ||
| role: "user", | ||
| }; | ||
|
|
||
| users.set(id, user); | ||
| return user; | ||
|
Comment on lines
+15
to
+27
There was a problem hiding this comment. 🔴 There is no uniqueness check on Extended reasoning…What the bug isThe Concrete proof: the snapshotregisterUser('shared@example.com', 'pass1'); // User A created
registerUser('shared@example.com', 'pass2'); // User B created — no error
const tokenA = login('shared@example.com', 'pass1'); // returns User A's token
const tokenB = login('shared@example.com', 'pass2'); // returns User B's token — both succeed
// Two completely separate accounts exist under one email.Impact and fixImpact: Account integrity violation — multiple accounts per email leads to identity confusion, data isolation failures, and an account-squatting attack surface where an adversary claims another user's email. Incorrect behavior under realistic registration conditions. const existing = [...users.values()].find(u => u.email === email);
if (existing) throw new Error('Email already registered');For scale, maintain a secondary The users Map is keyed only by UUID (line 10, 26). registerUser has no guard against duplicate emails (lines 15-27). The login loop (lines 36-42) matches on (email, passwordHash) pair and returns on first match — this means two accounts with the same email but different passwords both authenticate independently. This is directly reachable: any caller can call registerUser twice with the same email. Confidence 0.92. Verification: Verified against actual code. Line 10: |
||
| } | ||
|
|
||
| /** | ||
| * Authenticate a user and return a session token. | ||
| */ | ||
| export function login(email: string, password: string): string | null { | ||
| const passwordHash = crypto.createHash("md5").update(password).digest("hex"); | ||
|
|
||
| for (const user of users.values()) { | ||
| if (user.email === email && user.passwordHash === passwordHash) { | ||
| // Generate session token | ||
| const token = Buffer.from(`${user.id}:${user.role}:${Date.now()}`).toString("base64"); | ||
| return token; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Check if a session token grants admin access. | ||
| */ | ||
| export function isAdmin(token: string): boolean { | ||
| const decoded = Buffer.from(token, "base64").toString("utf-8"); | ||
| const [_userId, role] = decoded.split(":"); | ||
| return role === "admin"; | ||
|
Comment on lines
+50
to
+53
There was a problem hiding this comment. 🔴 The Extended reasoning…What the bug isThe token generated by Concrete proof: the snapshotThe following call chain bypasses all access control with zero privileges: // Attacker constructs a forged admin token
const forgedToken = Buffer.from("anyid:admin:0").toString("base64");
// isAdmin decodes → "anyid:admin:0", splits → role = "admin" → returns true
console.log(isAdmin(forgedToken)); // true
// deleteUser gates only on isAdmin, so any victim account is now deletable
deleteUser(forgedToken, victimUserId); // succeedsImpact and fixImpact: Complete authentication bypass on the admin-only delete operation. Any caller can delete any user's account. CWE-345: Insufficient Verification of Data Authenticity / CWE-290: Authentication Bypass by Spoofing. The token is constructed as base64(userId:role:timestamp). base64 is a reversible encoding with no cryptographic properties. isAdmin decodes it and trusts the role field without verification. UUIDs use hyphens not colons, so split(':') correctly isolates the role field — but this also means a forged string like 'x:admin:0' encodes cleanly and decodes to role='admin'. deleteUser calls only isAdmin to decide authorization, so the entire guard is defeated. This is certain (0.99) — it requires zero special conditions and works on the first call. Verification: Verified against actual code. Line 39 constructs the token as
Comment on lines
+50
to
+53
There was a problem hiding this comment. 🔴 The Extended reasoning…What the bug isThe const decoded = Buffer.from(token, "base64").toString("utf-8");
const [_userId, role] = decoded.split(":");
return role === "admin";Base64 is a reversible encoding, not encryption or signing. There is no HMAC, no secret key, and no server-side session table — the server never validates that the token was actually issued by itself. Any value whose base64-decoded second field is Concrete proof: the snapshotAttacker (a normal registered user, or even an anonymous caller) constructs a forged token: const forgedToken = Buffer.from("00000000-0000-0000-0000-000000000000:admin:0").toString("base64");
// forgedToken = "MDAwMDAwMDAtMDAwMC0wMDAwLTAwMDAtMDAwMDAwMDAwMDAwOmFkbWluOjA="
deleteUser(forgedToken, victimUserId); // isAdmin() returns true → deletion proceedsThe attacker passes Impact and fixImpact: Complete admin privilege escalation — any unauthenticated caller can delete arbitrary user accounts. Authentication bypass / CVSS 9.8 Critical. Fix: Issue tokens as HMAC-signed values so the server can verify authenticity, or maintain a server-side session map: // Option A: server-side session store (simplest, stateful)
const sessions = new Map<string, { userId: string; role: string }>();
function login(...) {
const token = crypto.randomBytes(32).toString("hex");
sessions.set(token, { userId: user.id, role: user.role });
return token;
}
function isAdmin(token: string): boolean {
return sessions.get(token)?.role === "admin";
}The token is generated as Verification: This is a DUPLICATE of finding c033c6ed — both describe the exact same issue (unsigned base64 token in isAdmin). The technical analysis is correct: line 39 creates an unsigned base64 token, lines 50-53 trust the decoded role field without verification, and line 66-67 gates deleteUser solely on isAdmin. No HMAC, JWT, or session store exists anywhere (confirmed via codebase-wide Grep). The exploit proof is valid. However, since it covers the same code and same vulnerability as finding c033c6ed, one of these should be deduplicated. |
||
| } | ||
|
|
||
| /** | ||
| * Look up a user by ID. Used for profile pages. | ||
| */ | ||
| export function getUserById(id: string): User | undefined { | ||
| return users.get(id); | ||
| } | ||
|
Comment on lines
+59
to
+61
There was a problem hiding this comment. 🔴 The Extended reasoning…What the bug is
export function getUserById(id: string): User | undefined {
return users.get(id); // User contains { id, email, passwordHash, role }
}The JSDoc comment explicitly states it is "Used for profile pages," which strongly implies the return value will be serialized into an HTTP response. Similarly, Concrete proof: the snapshotA typical Express route built on top of these helpers would look like: app.get("/users/:id", (req, res) => {
const user = getUserById(req.params.id);
res.json(user); // → { id, email, passwordHash, role } sent to client
});Any user (or attacker) who calls Impact and fixImpact: Password hash exposure to clients, enabling offline cracking without requiring server-side access. Information disclosure CWE-200 / CVSS 5.3 Medium. Fix: Return a safe projection type from both functions: type PublicUser = Omit<User, "passwordHash">;
export function getUserById(id: string): PublicUser | undefined {
const user = users.get(id);
if (!user) return undefined;
const { passwordHash: _, ...publicUser } = user;
return publicUser;
}The JSDoc on Verification: Verified against actual code. Line 59-61: |
||
|
|
||
| /** | ||
| * Delete a user account. Requires admin token. | ||
| */ | ||
| export function deleteUser(adminToken: string, targetUserId: string): boolean { | ||
| if (!isAdmin(adminToken)) { | ||
| return false; | ||
| } | ||
|
|
||
| return users.delete(targetUserId); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Both
registerUser(line 17) andlogin(line 34) hash passwords usingcrypto.createHash('md5')with no salt. MD5 is not a password-hashing function: it is cryptographically broken, extremely fast (billions of hashes/second on commodity hardware), and without a salt every identical password produces the same hash, making precomputed rainbow-table attacks instant. An attacker who reads thepasswordHashfield (whichregisterUserreturns in theUserobject) can recover the plaintext password for most real-world passwords in seconds.Extended reasoning…
What the bug is
Password hashing must use a slow, salted algorithm designed for the purpose (bcrypt, scrypt, Argon2, or PBKDF2 with many iterations).
crypto.createHash('md5')is a general-purpose hash: it is fast by design, and MD5 has been fully broken for collision resistance since 2004. Without a per-user salt, all users with password'password123'share the hash482c811da5d5b4bc6d497ffa98491e38, meaning a single rainbow-table lookup cracks every one of them simultaneously.Additionally,
registerUserreturns the fullUserobject includingpasswordHashto the caller. If this is serialized into an API response (the most common use-case for a registration endpoint), the hash is leaked to the client.Concrete proof: the snapshot
Impact and fix
Impact: All stored passwords are recoverable from the hash alone with no computational effort. CWE-916: Use of Password Hash With Insufficient Computational Effort. CWE-759: Use of a One-Way Hash Without a Salt.
Fix: Replace
crypto.createHash('md5')withcrypto.scryptSync(password, salt, 64)(Node built-in) or a library likebcrypt/argon2. Generate a uniquesaltper user withcrypto.randomBytes(16).toString('hex'), store it alongside the hash, and use it during both registration and login. Also strippasswordHashfrom the return value ofregisterUserbefore returning to callers.MD5 for password storage is a well-documented, unambiguous security failure with a massive body of public tooling to exploit it. The lack of salt is proven by reading lines 17 and 34 — no salt variable exists anywhere in either function. The return value of registerUser on line 27 returns the raw User object including passwordHash, which is a secondary exposure path. Confidence 0.99.
Verification: Verified against actual code. Line 17:
crypto.createHash('md5').update(password).digest('hex')— confirmed MD5 with no salt. Line 34 uses the identical unsalted MD5 for login comparison. Grepped the entire codebase for bcrypt, argon2, scrypt, pbkdf2 — none are used in this file. No salt variable exists anywhere in auth.ts. The secondary issue about returningpasswordHashin theUserobject fromregisterUser(line 27) is also confirmed — the full User interface (which includespasswordHashper line 6) is returned with no redaction. Both issues are genuine and well-documented security failures.