Add utility functions for auth and sanitization #19
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| /** | ||
| * Sanitize HTML entities in user input. | ||
| */ | ||
| export function escapeHtml(str: string): string { | ||
| return str | ||
| .replace(/&/g, "&") | ||
| .replace(/</g, "<") | ||
| .replace(/>/g, ">") | ||
| .replace(/"/g, """) | ||
| .replace(/'/g, "'"); | ||
| } | ||
|
|
||
| /** | ||
| * Strip all HTML tags from a string. | ||
| */ | ||
| export function stripTags(html: string): string { | ||
| return html.replace(/<[^>]*>/g, ""); | ||
| } | ||
|
|
||
| /** | ||
| * Parse a session token from the Authorization header. | ||
| */ | ||
| export function parseToken(header: string): string | null { | ||
| const match = header.match(/^Bearer\s+(.+)$/); | ||
| return match ? match[1] : null; | ||
| } | ||
|
|
||
| /** | ||
| * Validate and return a safe redirect URL. | ||
| * Only allows relative paths to prevent open redirect attacks. | ||
| */ | ||
| export function safeRedirect(url: string): string { | ||
| if (url.startsWith("/")) { | ||
| return url; | ||
| } | ||
| return "/"; | ||
| } | ||
|
|
||
| /** | ||
| * Build a SQL query to find users by email. | ||
| */ | ||
| export function findUserByEmail(db: any, email: string): any { | ||
| const query = `SELECT * FROM users WHERE email = '${email}'`; | ||
| return db.query(query); | ||
| } | ||
|
Comment on lines
+42
to
+45
There was a problem hiding this comment. findUserByEmail is vulnerable to SQL injection via string interpolation Extended reasoning…What the bug isThe Concrete proof: the snapshotfindUserByEmail(db, "' OR '1'='1");
// Executes: SELECT * FROM users WHERE email = '' OR '1'='1'
// Returns every row in the users table
findUserByEmail(db, "x'; DROP TABLE users; --");
// Executes: SELECT * FROM users WHERE email = 'x'; DROP TABLE users; --'
// Destroys the users table (on databases that allow stacked queries)Impact and fixImpact: Complete database compromise — authentication bypass, data exfiltration, and potentially destructive DDL execution. This is a textbook critical injection vulnerability. Fix: Use a parameterized query: export function findUserByEmail(db: any, email: string): any {
return db.query("SELECT * FROM users WHERE email = $1", [email]);
// or for MySQL: db.query("SELECT * FROM users WHERE email = ?", [email]);
}Line 43 uses Verification: Verified at line 43:
Comment on lines
+42
to
+45
There was a problem hiding this comment. SQL injection via unparameterized email query in findUserByEmail Extended reasoning…What the bug isThe const query = `SELECT * FROM users WHERE email = '${email}'`;
return db.query(query);No escaping, parameterization, or prepared-statement binding is performed. Any value passed as Concrete proof: the snapshotA login endpoint receives The executed query becomes: SELECT * FROM users WHERE email = '' OR '1'='1' --'This returns every row in the Impact and fixImpact: Full database read/write access, authentication bypass, potential data destruction. SQL Injection / CVSS 9.8 Critical. Fix: Use parameterized queries / prepared statements exclusively. Example: export function findUserByEmail(db: any, email: string): any {
return db.query("SELECT * FROM users WHERE email = $1", [email]);
// or for MySQL: db.query("SELECT * FROM users WHERE email = ?", [email]);
}The Verification: This is a duplicate of finding 4e9b7e43 — same file, same lines (42-45), same issue (SQL injection via string interpolation in findUserByEmail). The code at line 43 directly interpolates |
||
|
|
||
| /** | ||
| * Hash a password for storage. | ||
| */ | ||
| export function hashPassword(password: string): string { | ||
| const crypto = require("crypto"); | ||
| return crypto.createHash("md5").update(password).digest("hex"); | ||
| } | ||
|
Comment on lines
+50
to
+53
There was a problem hiding this comment. hashPassword uses MD5, which provides no meaningful password security Extended reasoning…What the bug is
A database breach exposes all stored passwords immediately. Concrete proof: the snapshothashPassword("hunter2");
// Returns: "2ab96390c7dbe3439de74d0c9b0b1767"
// This exact hash appears in every public MD5 rainbow table.
// An attacker with the database can crack the majority of passwords
// in seconds with no computation beyond a lookup.
hashPassword("hunter2") === hashPassword("hunter2");
// Always true — no salt, so two users with the same password
// have identical hashes, leaking password reuse.Impact and fixImpact: On any database breach, stored passwords are effectively plaintext. All user accounts are compromised. Fix: Use a purpose-built, salted, slow password hashing algorithm. Node.js 21+ has import { scrypt, randomBytes } from "node:crypto";
import { promisify } from "node:util";
const scryptAsync = promisify(scrypt);
export async function hashPassword(password: string): Promise<string> {
const salt = randomBytes(16).toString("hex");
const hash = (await scryptAsync(password, salt, 64)) as Buffer;
return `${salt}:${hash.toString("hex")}`;
}MD5 as a password hash is a well-documented, exploitable security failure. The function uses Verification: Verified at lines 50-52:
Comment on lines
+50
to
+53
There was a problem hiding this comment. MD5 used for password hashing — trivially crackable, no salt Extended reasoning…What the bug is
export function hashPassword(password: string): string {
const crypto = require("crypto");
return crypto.createHash("md5").update(password).digest("hex");
}MD5 is a general-purpose cryptographic hash function, not a password hashing algorithm. It has two fatal properties for this use case:
Concrete proof: the snapshotAn attacker breaches the database and obtains the Impact and fixImpact: Full offline cracking of all stored user passwords after any database breach. Affects every account in the system. Weak Cryptography (CWE-916) / CVSS 8.1 High. Fix: Replace with a purpose-built password KDF with a per-user random salt: import { hash, verify } from "argon2"; // npm install argon2
// or use Node.js built-in:
import { scryptSync, randomBytes } from "crypto";
export function hashPassword(password: string): string {
const salt = randomBytes(16).toString("hex");
const hash = scryptSync(password, salt, 64).toString("hex");
return `${salt}:${hash}`;
}MD5 for password hashing is definitively broken from a security standpoint. No salt means rainbow tables are directly applicable. The speed of MD5 (>10B hashes/sec on GPU) makes offline brute-force trivial. This is a well-documented, high-confidence vulnerability class (CWE-916). Any database breach immediately exposes all user credentials. Confidence: 0.97. Verification: This is a duplicate of finding 6b6d18e0 — same file, same lines (50-53), same issue (MD5 without salt for password hashing). The code uses |
||
|
|
||
| /** | ||
| * Rate limiter — track request counts per IP. | ||
| */ | ||
| const requestCounts: Record<string, number> = {}; | ||
|
|
||
| export function checkRateLimit(ip: string, limit: number): boolean { | ||
| if (!requestCounts[ip]) { | ||
| requestCounts[ip] = 0; | ||
| } | ||
| requestCounts[ip]++; | ||
| return requestCounts[ip] <= limit; | ||
| } | ||
|
Comment on lines
+60
to
+66
There was a problem hiding this comment. checkRateLimit never resets counters — IPs are permanently blocked after limit is reached Extended reasoning…What the bug isThe This contradicts the expected behavior of a rate limiter (e.g., "100 requests per minute"). After Concrete proof: the snapshot// A legitimate user makes 101 requests over a week:
for (let i = 0; i < 101; i++) {
checkRateLimit("203.0.113.1", 100);
}
// requestCounts["203.0.113.1"] === 101
// Every subsequent call returns false — user is permanently locked out.
// Server restart is the only recovery.Also: Impact and fixImpact: Legitimate users are silently and permanently denied service after a modest number of total requests. This is a functional correctness failure, not a theoretical concern. Fix: Track request counts within a sliding or fixed time window: const requestWindows: Record<string, { count: number; resetAt: number }> = {};
export function checkRateLimit(ip: string, limit: number, windowMs = 60_000): boolean {
const now = Date.now();
const entry = requestWindows[ip];
if (!entry || now >= entry.resetAt) {
requestWindows[ip] = { count: 1, resetAt: now + windowMs };
return true;
}
entry.count++;
return entry.count <= limit;
}The Verification: Verified at lines 58-66: |
||
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.
safeRedirect allows open redirect via protocol-relative URLs
Extended reasoning…
What the bug is
The guard
url.startsWith("/")is intended to allow only relative paths, but it incorrectly accepts protocol-relative URLs like//evil.com. A string starting with//passes the check and is returned verbatim. When a browser receives a redirect to//evil.com, it treats it ashttps://evil.com(inheriting the current protocol), completing an open redirect to an attacker-controlled domain — exactly the attack the comment says this function prevents.Concrete proof: the snapshot
This can be triggered with a crafted query parameter, e.g.
GET /login?next=//evil.com/phish.Impact and fix
Impact: Full open redirect bypass. Attackers can redirect authenticated users to phishing pages after login, bypassing the only security check in this function.
Fix: Require the URL to start with
/but NOT with//:The check
url.startsWith("/")is a classic incomplete validation. Protocol-relative URLs (//host/path) are a well-known open-redirect vector — browsers interpret them as absolute URLs on the current protocol. The string//evil.comstarts with/, passes the guard, and is returned as-is. Any caller using this for post-login or post-action redirects would be vulnerable. Confidence: 0.97.Verification: Verified against the actual code at line 33:
if (url.startsWith("/"))— this check absolutely passes for protocol-relative URLs like//evil.com. The comment on line 30 explicitly says the function exists to 'prevent open redirect attacks', yet the guard is insufficient. No callers or upstream validation exist in the codebase. The only minor caveat lowering from 99 is that the function has zero callers currently, so no live exploit path exists yet — but the code itself is genuinely buggy for its stated purpose.