Add utility functions for auth and sanitization #18
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /** | ||
| * 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 { | ||
| // Allow any URL that starts with / | ||
| if (url.startsWith("/")) { | ||
| return url; | ||
| } | ||
| return "/"; | ||
| } | ||
|
Comment on lines
+32
to
+38
There was a problem hiding this comment. Open redirect bypass via protocol-relative URLs in safeRedirect Extended reasoning…What the bug isThe guard Concrete proof: the snapshotsafeRedirect("//attacker.com/phishing") // returns "//attacker.com/phishing"A browser receiving Impact and fixImpact: Open redirect exploitable for phishing, OAuth token hijacking, and credential theft. Attackers craft URLs like Fix: Check that the URL does NOT start with export function safeRedirect(url: string): string {
// Reject protocol-relative URLs (//host) and anything with a scheme
if (url.startsWith("/") && !url.startsWith("//")) {
return url;
}
return "/";
}The Verification: Verified by reading src/utils/sanitize.ts lines 32-38. The check is
Comment on lines
+32
to
+38
There was a problem hiding this comment. Open redirect bypass via protocol-relative URL (//evil.com starts with /) Extended reasoning…What the bug is
if (url.startsWith("/")) {
return url;
}A protocol-relative URL like Concrete proof: the snapshotAttacker crafts a phishing link: Route calls Impact and fixImpact: Open redirect enabling phishing, session-token theft via Referer header, OAuth redirect abuse. CWE-601 / CVSS 6.1 Medium. Fix: Require the URL to start with export function safeRedirect(url: string): string {
// Reject protocol-relative (//host) and backslash variants (/\host)
if (url.startsWith("/") && !url.startsWith("//") && !url.startsWith("/\\")) {
return url;
}
return "/";
}For stronger guarantees, parse with The Verification: This is a duplicate of finding 6c0772b1. Both describe the exact same issue:
Comment on lines
+32
to
+38
There was a problem hiding this comment. safeRedirect allows protocol-relative open redirects via // prefix Extended reasoning…What the bug isThe redirect guard checks only that the URL starts with if (url.startsWith("/")) {
return url;
}This does not prevent open redirects because Concrete proof: the snapshotsafeRedirect("//evil.com/phishing") // returns "//evil.com/phishing"When a route then does The user is redirected to Impact and fixImpact: Open redirect exploitable for phishing attacks. Passes security review because the guard looks correct. Fix: Reject any URL where the path component starts with export function safeRedirect(url: string): string {
// Reject protocol-relative URLs (//host) and anything with a scheme
if (url.startsWith("/") && !url.startsWith("//")) {
return url;
}
return "/";
}The Verification: Verified genuine. Lines 34-35 check
Comment on lines
+32
to
+38
There was a problem hiding this comment. Open redirect bypass via protocol-relative URL (//evil.com passes startsWith("/") check) Extended reasoning…What the bug is
export function safeRedirect(url: string): string {
// Allow any URL that starts with /
if (url.startsWith("/")) {
return url;
}
return "/";
}A protocol-relative URL such as Concrete proof: the snapshotAttacker crafts a phishing link: After the user authenticates, the server calls Impact and fixImpact: Open redirect enabling phishing and credential theft. OWASP A01 / CWE-601. Medium severity because it requires user interaction but is trivially exploitable in login flows. Fix: Reject any URL that begins with export function safeRedirect(url: string): string {
// Reject protocol-relative URLs (//host) and anything with a scheme
if (url.startsWith("/") && !url.startsWith("//")) {
return url;
}
return "/";
}The Verification: Verified genuine, but this is a duplicate of finding 7e4f5743. Both findings describe the exact same bug: |
||
|
|
||
| /** | ||
| * 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
+43
to
+46
There was a problem hiding this comment. SQL injection via string interpolation in findUserByEmail Extended reasoning…What the bug isThe Concrete proof: the snapshotCalling SELECT * FROM users WHERE email = '' OR '1'='1'This returns every row in the Impact and fixImpact: Full SQL injection. Attackers can dump the entire database, bypass authentication, or (depending on DB driver privileges) destroy data or achieve remote code execution. Fix: Use a parameterized query / prepared statement: 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 template literal Verification: Verified by reading src/utils/sanitize.ts line 44:
Comment on lines
+43
to
+46
There was a problem hiding this comment. SQL injection via unparameterized email in findUserByEmail Extended reasoning…What the bug isThe const query = `SELECT * FROM users WHERE email = '${email}'`;This is a textbook SQL injection vulnerability. There is no escaping, parameterization, or prepared statement of any kind. Concrete proof: the snapshotCalling SELECT * FROM users WHERE email = '' OR '1'='1' --'This returns every row in the Impact and fixImpact: Full database compromise. An attacker controlling the Fix: Use a parameterized query / prepared statement: 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]);
}Direct string interpolation of user-controlled input into SQL is the canonical SQL injection pattern. There is no sanitization upstream (the function accepts raw caller-supplied strings), no escaping downstream, and no parameterization. The db.query call on line 45 executes whatever string is built on line 44. Confidence: 0.99. Verification: Verified genuine. Line 44 of src/utils/sanitize.ts 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
+51
to
+54
There was a problem hiding this comment. MD5 used for password hashing — trivially crackable, no salt Extended reasoning…What the bug is
Concrete proof: the snapshothashPassword("password123")
// => "482c811da5d5b4bc6d497ffa98491e38"This exact hash appears in public rainbow table databases. Any attacker who dumps the Impact and fixImpact: If the database is ever breached (including via the SQL injection bug above), all user passwords are instantly recoverable. This also compromises users on other sites where they reuse the same password. Fix: Use a password-specific KDF with a built-in salt, such as import bcrypt from "bcrypt";
export async function hashPassword(password: string): Promise<string> {
return bcrypt.hash(password, 12);
}MD5 has been considered cryptographically broken for password storage for over a decade. It is listed in OWASP and NIST guidance as unacceptable for this purpose. The function is named Verification: Verified by reading src/utils/sanitize.ts line 53:
Comment on lines
+51
to
+54
There was a problem hiding this comment. MD5 used for password hashing — cryptographically broken, no salt Extended reasoning…What the bug is
return crypto.createHash("md5").update(password).digest("hex");MD5 is a general-purpose message digest, not a password hashing function. It has two fatal flaws for this use case:
Concrete proof: the snapshotCalling Impact and fixImpact: If the database is ever breached (common in SQL-injection scenarios — see the other finding), all user passwords are effectively plaintext. This also enables credential-stuffing attacks across other services. Fix: Use a purpose-built password hashing function with a built-in salt. The import bcrypt from "bcrypt";
export async function hashPassword(password: string): Promise<string> {
return bcrypt.hash(password, 12); // cost factor 12
}MD5 has been considered cryptographically broken for password storage since at least the mid-2000s. It produces a fixed 128-bit output with no salt and executes in nanoseconds. Every major security authority (OWASP, NIST) explicitly prohibits MD5 for password hashing. The absence of salt means the same password always hashes to the same value, making rainbow table attacks trivially effective. Confidence: 0.99. Verification: Verified genuine. Line 53 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
+59
to
+67
There was a problem hiding this comment. Rate limiter has no time window — counts never reset, permanently blocking legitimate users Extended reasoning…What the bug is
Concrete proof: the snapshotWith
There is no code path that ever decrements or clears entries in Impact and fixImpact: All users who legitimately hit the rate limit are permanently blocked until the server restarts. In a production system this causes complete denial of service for active users. The longer the server runs without a restart, the more users accumulate in the permanent block list. Fix: Store counts with timestamps and only count requests within the desired window: const requestCounts: Record<string, { count: number; windowStart: number }> = {};
export function checkRateLimit(ip: string, limit: number, windowMs = 60_000): boolean {
const now = Date.now();
const entry = requestCounts[ip];
if (!entry || now - entry.windowStart > windowMs) {
requestCounts[ip] = { count: 1, windowStart: now };
return true;
}
entry.count++;
return entry.count <= limit;
}The Verification: Verified by reading src/utils/sanitize.ts lines 59-67. The
Comment on lines
+61
to
+67
There was a problem hiding this comment. checkRateLimit has no time window — permanently bans IPs after limit requests Extended reasoning…What the bug is
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;
}Once an IP accumulates Concrete proof: the snapshotWith Impact and fixImpact: All legitimate users will eventually be permanently denied service. In production with a limit of e.g. 1000 req/hour, after ~1000 legitimate requests, every user is blocked forever — effectively a self-inflicted DoS. Fix: Store timestamps or use a sliding-window approach: const requestWindows: Record<string, { count: number; windowStart: number }> = {};
export function checkRateLimit(ip: string, limit: number, windowMs = 60_000): boolean {
const now = Date.now();
const entry = requestWindows[ip];
if (!entry || now - entry.windowStart > windowMs) {
requestWindows[ip] = { count: 1, windowStart: now };
return true;
}
entry.count++;
return entry.count <= limit;
}The counter increments on every call and is never reset or expired. The condition Verification: Verified genuine. The module-level |
||
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.
stripTags regex fails on > inside quoted attribute values, enabling tag-stripping bypass
Extended reasoning…
What the bug is
The regex
/<[^>]*>/guses[^>]to match tag contents, which stops at the first>character. It does not account for>characters that appear inside quoted attribute values. A tag like<a href=">">contains a>inside thehrefvalue, causing the regex to match only<a href=">as the "tag" and leave">as residual content in the output.Concrete proof: the snapshot
If the result is embedded in an HTML context, the
onclickattribute executes.Impact and fix
Impact: If
stripTagsis used as an XSS-prevention layer before rendering user content in HTML, this bypass allows script injection. The severity depends on how the API routes consume this function.Fix: Use a proper HTML parsing library (e.g.,
sanitize-htmlorDOMPurifyon the server) rather than a regex. If a regex must be used, the pattern needs to handle quoted strings inside tags, which is non-trivial.The
[^>]character class does not understand quoted strings. A>insidehref=">"terminates the match early. This is a known regex-based HTML parsing limitation. The concrete input<a href=">" onclick="alert(1)">produces output containing the onclick handler. Confidence 0.90 — the trigger is specific but realistic in user-generated content.Verification: Verified by reading src/utils/sanitize.ts line 17:
/<[^>]*>/g. The regex[^>]*stops at the first>character, even one inside a quoted attribute. The finding's example<a href=">" onclick="alert(1)">click</a>would indeed cause the regex to match<a href=">as the first 'tag', leaving" onclick="alert(1)">clickin the output after subsequent matches remove</a>. This is a real limitation of regex-based HTML stripping. However, the severity is somewhat context-dependent: this function is not yet called anywhere, and the exploit requires an attacker to craft specific HTML with>inside quoted attributes. The finding is technically correct but less likely to be exploited in practice than the SQL injection or open redirect. The function stripTags is a utility — if it's used as a security boundary (the only XSS defense), this is a genuine vulnerability. If it's used for display purposes alongside escapeHtml, it's less critical.