Add user authentication module

[fantaize]

Adds basic user registration, login, and admin access control.

• User registration with email/password
• Login returns a session token
• Admin check for protected operations
• User deletion (admin only)

[fantaize-code-reviewer]

Found 5 issues in 4m 2s.

Extended reasoning…

Overview

This PR introduces a new authentication module in test-files/auth.ts (72 lines added), covering user registration, login with session token issuance, admin access control, and admin-only user deletion. All changes are net-new code with no modifications to existing files.

Security risks

The security risks in this PR are severe and numerous. Passwords are hashed with unsalted MD5, making them trivially crackable via rainbow tables. Admin tokens are unsigned base64 and the isAdmin check performs no cryptographic verification, meaning the admin role is completely forgeable and the entire access control system can be bypassed by any user. Additionally, getUserById and registerUser expose the raw passwordHash field to callers, leaking sensitive credential data.

Level of scrutiny

High. This PR implements core authentication and access control — the exact category of code that warrants the most rigorous review. The combination of password storage, session token handling, and privilege escalation checks means that flaws here can compromise every user and every protected operation in the system.

Other factors

The duplicate email registration issue (finding #4) points to a broader concern: there is no apparent input validation or uniqueness enforcement layer, suggesting the module may have been written without a clear security threat model in mind. Beyond individual bugs, the pattern across findings indicates the authentication design needs a fundamental rethink — using a vetted library (e.g. bcrypt for passwords, signed JWTs for tokens) rather than ad hoc implementations.
Overview

This PR introduces a new authentication module in test-files/auth.ts (72 lines added), covering user registration, login with session token issuance, admin access control, and admin-only user deletion. All changes are net-new code with no modifications to existing files.

Security risks

The security risks in this PR are severe and numerous. Passwords are hashed with unsalted MD5, making them trivially crackable via rainbow tables. Admin tokens are unsigned base64 and the isAdmin check performs no cryptographic verification, meaning the admin role is completely forgeable and the entire access control system can be bypassed by any user. Additionally, getUserById and registerUser expose the raw passwordHash field to callers, leaking sensitive credential data.

Level of scrutiny

High. This PR implements core authentication and access control — the exact category of code that warrants the most rigorous review. The combination of password storage, session token handling, and privilege escalation checks means that flaws here can compromise every user and every protected operation in the system.

Other factors

The duplicate email registration issue (finding #4) points to a broader concern: there is no apparent input validation or uniqueness enforcement layer, suggesting the module may have been written without a clear security threat model in mind. Beyond individual bugs, the pattern across findings indicates the authentication design needs a fundamental rethink — using a vetted library (e.g. bcrypt for passwords, signed JWTs for tokens) rather than ad hoc implementations.

Findings

1. 🔴 test-files/auth.ts:50 — The isAdmin function base64-decodes the token and blindly trusts the role field embedded in it, performing no signature or HMAC verification whatsoever. Because base64 is reversible encoding (not a signature), any caller can craft Buffer.from('x:admin:0').toString('base64') and isAdmin will return true, granting full admin access. Since deleteUser gates entirely on isAdmin, this makes the admin-only delete operation completely unenforced — any caller can delete any user account. (issue)
2. 🔴 test-files/auth.ts:17 — Both registerUser (line 17) and login (line 34) hash passwords using crypto.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 the passwordHash field (which registerUser returns in the User object) can recover the plaintext password for most real-world passwords in seconds. (issue)
3. 🔴 test-files/auth.ts:50 — The isAdmin() function base64-decodes the token and reads the role field directly, with no cryptographic signature or server-side session store. Base64 is reversible encoding, not authentication — any caller can craft Buffer.from('any-id:admin:1234').toString('base64') and pass the admin check. Replace the opaque base64 scheme with signed tokens (e.g. HMAC-SHA256 or a proper JWT library) or use a server-side session map keyed by a cryptographically random token. (issue)
4. 🔴 test-files/auth.ts:15 — There is no uniqueness check on email in registerUser: two separate User records can exist with identical email addresses, each with their own UUID and password. During login, the users Map is iterated in insertion order and the loop returns on the first match of (email, passwordHash) — so if the passwords differ, both accounts successfully authenticate. This produces ambiguous identity, makes it impossible to reliably look up a user by email, and allows an attacker to register a second account under a victim's email address. (issue)
5. 🔴 test-files/auth.ts:59 — The getUserById() function (commented as being used for profile pages) and registerUser() both return the internal User object, which contains the passwordHash field. Any API layer that serializes and returns this object to clients will inadvertently leak password hashes. Callers should be given a sanitized projection that omits passwordHash and any other internal fields before the data leaves the server trust boundary. (issue)

[fantaize-code-reviewer]

🔴 The isAdmin function base64-decodes the token and blindly trusts the role field embedded in it, performing no signature or HMAC verification whatsoever. Because base64 is reversible encoding (not a signature), any caller can craft Buffer.from('x:admin:0').toString('base64') and isAdmin will return true, granting full admin access. Since deleteUser gates entirely on isAdmin, this makes the admin-only delete operation completely unenforced — any caller can delete any user account.

Extended reasoning…

What the bug is

The token generated by login is base64(userId:role:timestamp). The isAdmin function decodes this with Buffer.from(token, 'base64').toString('utf-8'), splits on ':', and returns role === 'admin'. There is no HMAC, no signature, no lookup against stored session data, and no secret key involved. Base64 is a transparent encoding; anyone who can read this code (it will be in a repository) can trivially construct any token they like.

Concrete proof: the snapshot

The 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); // succeeds

Impact and fix

Impact: 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.
Fix: Replace the unsigned base64 token with a cryptographically signed token (e.g., HMAC-SHA256 over the payload using a server-side secret), or maintain a server-side session store (Map<token, userId>) and look up the role from the stored User record instead of trusting the token's own payload. Example minimal fix: store session tokens in a Map<string, string> (token → userId), look up the user in users, and check user.role === 'admin'.

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 Buffer.from(${user.id}:${user.role}:${Date.now()}).toString('base64') — plain base64 with no HMAC or signature. Lines 50-53 of isAdmin decode with Buffer.from(token, 'base64').toString('utf-8'), split on ':', and trust the role field. There is no session store, no secret key, no cryptographic verification anywhere in this file or the broader codebase (confirmed via Grep for hmac/jwt/session). The exploit Buffer.from('x:admin:0').toString('base64') would trivially return true from isAdmin. deleteUser (line 66-71) gates solely on isAdmin, confirming the complete auth bypass. NOTE: This finding is essentially a duplicate of finding 03b522bf — both describe the same unsigned-token issue, one from a bug perspective and one from security.

[fantaize-code-reviewer]

🔴 Both registerUser (line 17) and login (line 34) hash passwords using crypto.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 the passwordHash field (which registerUser returns in the User object) 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 hash 482c811da5d5b4bc6d497ffa98491e38, meaning a single rainbow-table lookup cracks every one of them simultaneously.

Additionally, registerUser returns the full User object including passwordHash to 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

const user = registerUser('victim@example.com', 'hunter2');
// user.passwordHash === '2ab96390c7dbe3439de74d0c9b0b1767'  — a known MD5 in every rainbow table
// Any attacker with DB read access recovers all passwords instantly.
// Also: user.passwordHash is exposed in the return value to callers.

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') with crypto.scryptSync(password, salt, 64) (Node built-in) or a library like bcrypt/argon2. Generate a unique salt per user with crypto.randomBytes(16).toString('hex'), store it alongside the hash, and use it during both registration and login. Also strip passwordHash from the return value of registerUser before 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 returning passwordHash in the User object from registerUser (line 27) is also confirmed — the full User interface (which includes passwordHash per line 6) is returned with no redaction. Both issues are genuine and well-documented security failures.

[fantaize-code-reviewer]

🔴 The isAdmin() function base64-decodes the token and reads the role field directly, with no cryptographic signature or server-side session store. Base64 is reversible encoding, not authentication — any caller can craft Buffer.from('any-id:admin:1234').toString('base64') and pass the admin check. Replace the opaque base64 scheme with signed tokens (e.g. HMAC-SHA256 or a proper JWT library) or use a server-side session map keyed by a cryptographically random token.

Extended reasoning…

What the bug is

The isAdmin() function at lines 50–53 decodes a base64 string and trusts the embedded role field verbatim:

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 admin will pass the check.

Concrete proof: the snapshot

Attacker (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 proceeds

The attacker passes isAdmin() and can delete any user account without ever having registered or logged in.

Impact and fix

Impact: 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 Buffer.from(${user.id}:${user.role}:${Date.now()}).toString('base64'). Base64 is completely reversible — any string can be encoded into a valid 'token'. The isAdmin function performs no cryptographic verification: it simply decodes the base64 and checks the role substring. This means privilege escalation requires zero knowledge of any secret. The exploit is trivially constructable in one line. The deleteUser function is the only place isAdmin is called in this file, and it gates a destructive operation, making the impact severe. Confidence: 1.0.

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.

[fantaize-code-reviewer]

🔴 There is no uniqueness check on email in registerUser: two separate User records can exist with identical email addresses, each with their own UUID and password. During login, the users Map is iterated in insertion order and the loop returns on the first match of (email, passwordHash) — so if the passwords differ, both accounts successfully authenticate. This produces ambiguous identity, makes it impossible to reliably look up a user by email, and allows an attacker to register a second account under a victim's email address.

Extended reasoning…

What the bug is

The users Map is keyed by UUID, not email. registerUser does not scan for an existing entry with the same email before inserting. Multiple User records with the same email can therefore coexist. The login loop stops at the first record whose (email, passwordHash) pair matches — meaning if two users share the same email but have different passwords, both can log in independently and receive separate valid tokens, creating two real authenticated sessions for what the user believes is one email address.

Concrete proof: the snapshot

registerUser('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 fix

Impact: 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.
Fix: Before inserting, check for an existing user with the same email and throw (or return an error) if one is found:

const existing = [...users.values()].find(u => u.email === email);
if (existing) throw new Error('Email already registered');

For scale, maintain a secondary Map<email, userId> index to make this O(1).

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: const users: Map<string, User> = new Map() — keyed by UUID, not email. Lines 15-27 of registerUser: no check for existing email before users.set(id, user). Grepped for 'email.*unique', 'duplicate.*email', 'email.*exists' — no results. The login function (lines 36-42) iterates users.values() and returns on the first (email, passwordHash) match, meaning two users with the same email and different passwords both get valid tokens. The finding accurately describes a real data integrity issue. Slight confidence reduction only because the practical impact depends on how registerUser is called by upstream code (which doesn't exist in this file), but the function as exported has no guard.

[fantaize-code-reviewer]

🔴 The getUserById() function (commented as being used for profile pages) and registerUser() both return the internal User object, which contains the passwordHash field. Any API layer that serializes and returns this object to clients will inadvertently leak password hashes. Callers should be given a sanitized projection that omits passwordHash and any other internal fields before the data leaves the server trust boundary.

Extended reasoning…

What the bug is

getUserById() returns the full User interface:

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, registerUser() returns the full User object, which would typically be returned as a 201 response body.

Concrete proof: the snapshot

A 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 GET /users/<id> receives the passwordHash. Even though MD5 hashes aren't plaintext, exposing them substantially lowers the bar for the cracking attack described in finding 2 — the attacker no longer needs database access.

Impact and fix

Impact: 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 getUserById says 'Used for profile pages', which is a strong signal this will be included in API responses. The return type includes passwordHash. registerUser also returns the full User object. Any HTTP layer serializing the return value will leak the hash. Confidence: 0.85 — the only uncertainty is whether the HTTP layer will redact it, but the interface design makes that an easy mistake to miss.

Verification: Verified against actual code. Line 59-61: getUserById returns users.get(id) which is of type User | undefined. The User interface (lines 3-8) includes passwordHash. Line 57 JSDoc says 'Used for profile pages', strongly implying serialization to HTTP responses. Similarly, registerUser (line 27) returns the full User object. No redaction, projection, or Omit types are applied. Grepped for 'omit.*hash', 'strip.*password', 'redact', 'sanitize' in the codebase — no mitigations found for this file. The confidence is slightly lower (82 vs 95) because the actual exposure depends on how callers serialize the return value — the file itself is a module, not an HTTP endpoint, so there's a theoretical possibility that a caller could strip the field. But the API design makes exposure the path of least resistance, and the JSDoc comment about 'profile pages' reinforces this concern.