Skip to content

Add utility functions for auth and sanitization#18

Open
fantaize wants to merge 1 commit intomasterfrom
test/format-check
Open

Add utility functions for auth and sanitization#18
fantaize wants to merge 1 commit intomasterfrom
test/format-check

Conversation

@fantaize
Copy link
Copy Markdown
Owner

Adds helper utilities for:

  • HTML sanitization (escapeHtml, stripTags)
  • Auth token parsing
  • Safe redirect validation
  • User lookup by email
  • Password hashing
  • Rate limiting

These will be used by the API routes.

@fantaize
Copy link
Copy Markdown
Owner Author

/review

@fantaize fantaize closed this Mar 12, 2026
Comment on lines +43 to +46
export function findUserByEmail(db: any, email: string): any {
const query = `SELECT * FROM users WHERE email = '${email}'`;
return db.query(query);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SQL injection via string interpolation in findUserByEmail

Extended reasoning…

What the bug is

The email parameter is concatenated directly into a SQL query string using a template literal, with no escaping or parameterization. Any value containing a single-quote (') can break out of the string literal and inject arbitrary SQL.

Concrete proof: the snapshot

Calling findUserByEmail(db, "' OR '1'='1") produces:

SELECT * FROM users WHERE email = '' OR '1'='1'

This returns every row in the users table, completely bypassing authentication. A more destructive payload like "'; DROP TABLE users; --" could destroy data. Because the PR says this function will be used by API routes, the email value is almost certainly attacker-controlled.

Impact and fix

Impact: 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 SELECT * FROM users WHERE email = '${email}' directly embeds user-controlled data into a SQL string. The function accepts an email: string parameter that comes from outside (API routes per PR description). Single-quote in the email breaks out of the SQL string literal. This is a textbook SQL injection with 100% confidence. There is no upstream validation or escaping shown anywhere in the codebase.

Verification: Verified by reading src/utils/sanitize.ts line 44: const query = \SELECT * FROM users WHERE email = '${email}'`. This is textbook SQL injection via string interpolation. The emailparameter is directly embedded into the SQL string with no escaping or parameterization. There are no callers yet (no imports of sanitize.ts found anywhere), so there is no upstream validation. The function acceptsdb: any`, so no ORM protections apply. The finding is correct in every detail.

Comment on lines +32 to +38
export function safeRedirect(url: string): string {
// Allow any URL that starts with /
if (url.startsWith("/")) {
return url;
}
return "/";
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Open redirect bypass via protocol-relative URLs in safeRedirect

Extended reasoning…

What the bug is

The guard url.startsWith("/") is intended to restrict redirects to relative paths, but it also passes //evil.com — a protocol-relative URL — because it starts with /. Browsers resolve //evil.com as https://evil.com (or http://evil.com), making this a fully exploitable open redirect despite the stated security intent.

Concrete proof: the snapshot

safeRedirect("//attacker.com/phishing") // returns "//attacker.com/phishing"

A browser receiving Location: //attacker.com/phishing in a redirect response navigates to https://attacker.com/phishing. The function's JSDoc explicitly states it "only allows relative paths to prevent open redirect attacks," making the gap between intent and implementation clear.

Impact and fix

Impact: Open redirect exploitable for phishing, OAuth token hijacking, and credential theft. Attackers craft URLs like https://app.com/login?next=//evil.com which, after login, redirect the victim off-site.

Fix: Check that the URL does NOT start with // and does NOT contain a scheme, or use URL parsing:

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 startsWith("/") check passes //host because // does start with /. Protocol-relative URLs are interpreted by browsers as absolute URLs with the current page's scheme. The comment in the code states the explicit goal is preventing open redirects, confirming that //evil.com is an unintended bypass. Confidence 1.0 — this is a well-documented and commonly exploited bypass.

Verification: Verified by reading src/utils/sanitize.ts lines 32-38. The check is url.startsWith("/") which indeed passes //evil.com since // starts with /. Protocol-relative URLs like //evil.com are resolved by browsers as https://evil.com (or http:// depending on current scheme). The JSDoc on line 30 explicitly states the intent is to 'prevent open redirect attacks', confirming this is a gap. No additional URL validation exists anywhere in the codebase. This is a well-known bypass pattern.

Comment on lines +51 to +54
export function hashPassword(password: string): string {
const crypto = require("crypto");
return crypto.createHash("md5").update(password).digest("hex");
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MD5 used for password hashing — trivially crackable, no salt

Extended reasoning…

What the bug is

hashPassword uses crypto.createHash('md5') to hash passwords before storage. MD5 is a general-purpose digest algorithm, not a password-hashing function. It is (1) unsalted, making every user with the same password store the same hash, (2) extremely fast, enabling billions of guesses per second on commodity hardware, and (3) thoroughly broken by precomputed rainbow tables that cover essentially all common passwords.

Concrete proof: the snapshot

hashPassword("password123")
// => "482c811da5d5b4bc6d497ffa98491e38"

This exact hash appears in public rainbow table databases. Any attacker who dumps the users table immediately recovers plaintext passwords for the vast majority of users without any computation.

Impact and fix

Impact: 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 bcrypt, scrypt, or argon2:

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 hashPassword and is described as being for storage. The lack of salt means identical passwords produce identical hashes. Confidence 1.0 — this will cause real-world credential exposure in any breach.

Verification: Verified by reading src/utils/sanitize.ts line 53: crypto.createHash("md5").update(password).digest("hex"). MD5 is unsalted and computationally trivial to brute-force (billions of hashes/second on modern GPUs). The function is named hashPassword and documented as 'Hash a password for storage', confirming this is intended for credential storage. No salt is added, meaning identical passwords produce identical hashes. No other password hashing mechanism exists in the codebase. MD5 has been considered unacceptable for password hashing by OWASP and NIST for well over a decade.

Comment on lines +59 to +67
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;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rate limiter has no time window — counts never reset, permanently blocking legitimate users

Extended reasoning…

What the bug is

requestCounts is a module-level object that accumulates counts indefinitely. There is no timestamp, TTL, sliding window, or reset mechanism. Once an IP's count reaches the limit, every subsequent request from that IP is rejected — forever, until the server process restarts. This is not a rate limiter; it is a one-time quota that permanently bans IPs.

Concrete proof: the snapshot

With checkRateLimit(ip, 100) and a rate limit meant to be "100 requests per minute":

  • A legitimate user sends 100 requests in minute 1 — all pass.
  • In minute 2, the first request returns false and the user is permanently blocked.
  • requestCounts[ip] is now 101 and will only grow.

There is no code path that ever decrements or clears entries in requestCounts.

Impact and fix

Impact: 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 requestCounts map is initialized at module load and is never cleared. The checkRateLimit function only increments and checks, never resets. There is no expiry, no TTL, no time-window logic anywhere in the file or visible in the codebase. Once count > limit, the function returns false for that IP on every future call. This is a definite logic error — a rate limiter without a window is not a rate limiter. Confidence 0.99.

Verification: Verified by reading src/utils/sanitize.ts lines 59-67. The requestCounts object is module-level (line 59) and is only ever incremented (line 65). There is no code path that resets, decrements, or deletes entries. No timer, TTL, sliding window, or cleanup mechanism exists anywhere in the file or codebase. Once requestCounts[ip] exceeds limit, checkRateLimit returns false for that IP permanently until the process restarts. This is functionally a lifetime quota, not a rate limiter. The finding's analysis is accurate.

Comment on lines +32 to +38
export function safeRedirect(url: string): string {
// Allow any URL that starts with /
if (url.startsWith("/")) {
return url;
}
return "/";
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Open redirect bypass via protocol-relative URL (//evil.com starts with /)

Extended reasoning…

What the bug is

safeRedirect is documented to "only allow relative paths to prevent open redirect attacks", but its check is incomplete:

if (url.startsWith("/")) {
  return url;
}

A protocol-relative URL like //evil.com starts with /, passes the guard, and is returned unchanged. When a browser or framework executes res.redirect(safeRedirect(url)), it will redirect the user to https://evil.com (the scheme is inherited from the current page). This is a well-documented, commonly exploited bypass of exactly this pattern.

Concrete proof: the snapshot

Attacker crafts a phishing link:

https://app.example.com/logout?next=//evil.com/fake-login

Route calls res.redirect(safeRedirect(req.query.next)). safeRedirect("//evil.com/fake-login") returns "//evil.com/fake-login" (starts with /). User is redirected to https://evil.com/fake-login, a credential-harvesting page styled to look like the real login form. The browser URL bar shows evil.com.

Impact and fix

Impact: 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 / but not with //, and additionally reject backslash-based bypasses:

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 new URL(url, "https://example.com") and verify the resulting origin matches your own.

The //evil.com bypass for startsWith("/") checks is extremely well-known and has been exploited in the wild across many frameworks. The comment in the code explicitly claims protection against open redirects, making this a direct security regression. Protocol-relative URLs are valid in HTTP redirect responses and all major browsers follow them. Confidence: 0.92.

Verification: This is a duplicate of finding 6c0772b1. Both describe the exact same issue: safeRedirect on lines 32-38 passes //evil.com because url.startsWith("/") matches protocol-relative URLs. The analysis is correct — verified by reading the actual code. The //evil.com bypass is a classic, well-documented open redirect technique. The code's JSDoc comment on line 30 explicitly states the security intent ('prevent open redirect attacks'), confirming the gap. No mitigations exist elsewhere. However, since this is a duplicate of finding 6c0772b1, one of them is redundant.

* Strip all HTML tags from a string.
*/
export function stripTags(html: string): string {
return html.replace(/<[^>]*>/g, "");
Copy link
Copy Markdown

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 /<[^>]*>/g uses [^>] 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 the href value, causing the regex to match only <a href="> as the "tag" and leave "> as residual content in the output.

Concrete proof: the snapshot

stripTags('<a href=">" onclick="alert(1)">click</a>')
// Regex matches: <a href=">"
// Remaining:     " onclick=\"alert(1)\">click</a>
// Actual output: '" onclick="alert(1)">click'  
// ↑ onclick handler survives in the output

If the result is embedded in an HTML context, the onclick attribute executes.

Impact and fix

Impact: If stripTags is 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-html or DOMPurify on 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 > inside href=">" 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)">click in 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.

@fantaize fantaize reopened this Mar 12, 2026
Comment on lines +43 to +46
export function findUserByEmail(db: any, email: string): any {
const query = `SELECT * FROM users WHERE email = '${email}'`;
return db.query(query);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SQL injection via unparameterized email in findUserByEmail

Extended reasoning…

What the bug is

The email argument is directly interpolated into the SQL string using a template literal on line 44:

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 snapshot

Calling findUserByEmail(db, "' OR '1'='1' --") produces:

SELECT * FROM users WHERE email = '' OR '1'='1' --'

This returns every row in the users table. A more destructive payload like '; DROP TABLE users; -- would execute a second statement entirely, dropping the table if the database driver allows stacked queries.

Impact and fix

Impact: Full database compromise. An attacker controlling the email value (e.g., via a login form) can read all user records, bypass authentication, or destroy data.

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 email into a SQL string via template literal: SELECT * FROM users WHERE email = '${email}'. The db parameter is typed as any with no ORM or parameterization. Grep confirms no callers exist yet, but the function itself is a textbook SQL injection. There is no upstream validation, no escaping function applied to email, no ORM in package.json dependencies, and no middleware wrapping this function. The finding's proof-of-concept (' OR '1'='1' --) is trivially correct.

Comment on lines +51 to +54
export function hashPassword(password: string): string {
const crypto = require("crypto");
return crypto.createHash("md5").update(password).digest("hex");
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MD5 used for password hashing — cryptographically broken, no salt

Extended reasoning…

What the bug is

hashPassword uses crypto.createHash('md5') to hash passwords for storage:

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:

  1. Speed: MD5 hashes billions of inputs per second on commodity hardware, making brute-force trivial.
  2. No salt: identical passwords produce identical hashes, enabling precomputed rainbow table attacks.

Concrete proof: the snapshot

Calling hashPassword("password123") always returns "482c811da5d5b4bc6d497ffa98491e38" — the same value every time, for every user. An attacker who obtains the users table can instantly crack common passwords via rainbow tables (billions of pre-computed MD5 hashes are freely available online) and see which users share the same password.

Impact and fix

Impact: 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 bcrypt package is the standard Node.js choice:

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 crypto.createHash('md5').update(password).digest('hex') — a plain, unsalted MD5 hash. MD5 is a fast general-purpose digest, not a password hashing function. No salt is generated or applied anywhere in the function. No bcrypt, scrypt, or argon2 library exists in package.json dependencies. The function is named hashPassword and the comment says 'Hash a password for storage', confirming this is intended for credential storage. Identical passwords will always produce identical hashes, enabling rainbow table attacks. This is a well-established cryptographic weakness.

Comment on lines +32 to +38
export function safeRedirect(url: string): string {
// Allow any URL that starts with /
if (url.startsWith("/")) {
return url;
}
return "/";
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

safeRedirect allows protocol-relative open redirects via // prefix

Extended reasoning…

What the bug is

The redirect guard checks only that the URL starts with /:

if (url.startsWith("/")) {
  return url;
}

This does not prevent open redirects because //evil.com starts with / and passes the check. Browsers (and HTTP clients) interpret //evil.com as a protocol-relative URL — equivalent to https://evil.com in an HTTPS context. The function's stated purpose is to "prevent open redirect attacks", but this input bypasses it completely.

Concrete proof: the snapshot

safeRedirect("//evil.com/phishing") // returns "//evil.com/phishing"

When a route then does res.redirect(safeRedirect(req.query.next)), an attacker crafts:

https://yourapp.com/login?next=//evil.com/phishing

The user is redirected to https://evil.com/phishing after login, enabling credential phishing.

Impact and fix

Impact: Open redirect exploitable for phishing attacks. Passes security review because the guard looks correct.

Fix: Reject any URL where the path component starts with //, or use URL parsing to enforce the absence of a host:

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 //host/path form of URL is a well-documented open redirect bypass that exploits the HTML/HTTP protocol-relative URL specification. It starts with / so it passes startsWith("/"), but browsers treat it as an absolute URL with the current protocol. This is CVE-class vulnerability with a trivially demonstrable proof of concept. The function's own comment says it prevents open redirects, confirming this is unintended behavior. Confidence: 0.95.

Verification: Verified genuine. Lines 34-35 check url.startsWith('/') and return the URL. In JavaScript, '//evil.com'.startsWith('/') evaluates to true, so safeRedirect('//evil.com') returns '//evil.com'. Browsers interpret //evil.com as a protocol-relative URL (equivalent to https://evil.com on HTTPS pages). The function's own JSDoc says 'Only allows relative paths to prevent open redirect attacks' — but //evil.com is not a relative path. Grep confirms no additional redirect validation exists anywhere in the codebase. No middleware intercepts redirects. This is a well-documented open redirect bypass pattern (OWASP).

Comment on lines +32 to +38
export function safeRedirect(url: string): string {
// Allow any URL that starts with /
if (url.startsWith("/")) {
return url;
}
return "/";
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Open redirect bypass via protocol-relative URL (//evil.com passes startsWith("/") check)

Extended reasoning…

What the bug is

safeRedirect intends to prevent open redirects by allowing only relative paths, but its implementation only checks that the URL begins with /:

export function safeRedirect(url: string): string {
  // Allow any URL that starts with /
  if (url.startsWith("/")) {
    return url;
  }
  return "/";
}

A protocol-relative URL such as //evil.com starts with /, passes the guard, and is returned as-is. When this value is used in an HTTP Location: header (e.g., res.redirect(safeRedirect(req.query.next))), browsers interpret //evil.com as https://evil.com — a redirect to a fully external, attacker-controlled domain.

Concrete proof: the snapshot

Attacker crafts a phishing link:

https://legitimate-app.com/login?next=//evil.com/steal-creds

After the user authenticates, the server calls safeRedirect("//evil.com/steal-creds"). The check "//evil.com/steal-creds".startsWith("/") returns true. The function returns "//evil.com/steal-creds". The browser follows the Location: //evil.com/steal-creds redirect to the attacker's site.

Impact and fix

Impact: 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 // or contains a scheme, and optionally validate against a whitelist of allowed path prefixes:

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 //evil.com protocol-relative URL bypass of startsWith("/") guards is a well-documented, commonly exploited open redirect pattern (listed explicitly in OWASP's open redirect cheat sheet). The function comment even says 'Only allows relative paths to prevent open redirect attacks', confirming the intent is to block external redirects — but the implementation fails to do so. Confidence is high (>0.9) because the bypass is syntactically trivial and browsers unambiguously treat //host as an absolute URL.

Verification: Verified genuine, but this is a duplicate of finding 7e4f5743. Both findings describe the exact same bug: safeRedirect allows protocol-relative URLs like //evil.com to bypass the startsWith('/') check. The code, the proof-of-concept, and the fix are identical. The analysis is correct — //evil.com passes the guard and browsers treat it as an absolute URL. One finding is from bug-finder and this one is from security-auditor, but they are the same issue.

Comment on lines +61 to +67
export function checkRateLimit(ip: string, limit: number): boolean {
if (!requestCounts[ip]) {
requestCounts[ip] = 0;
}
requestCounts[ip]++;
return requestCounts[ip] <= limit;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

checkRateLimit has no time window — permanently bans IPs after limit requests

Extended reasoning…

What the bug is

requestCounts is a module-level object that accumulates request counts forever, with no expiry, no TTL, and no time-window reset:

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 limit total requests across the entire lifetime of the server process, it is permanently blocked. The counter is never decremented or reset. This is not rate limiting — it is a permanent block after N total lifetime requests.

Concrete proof: the snapshot

With limit = 100, a user who makes 100 requests to the API over their first session is permanently locked out. The next day, checkRateLimit("1.2.3.4", 100) returns false for every call from that IP forever, until the server restarts.

Impact and fix

Impact: 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 requestCounts[ip] <= limit becomes permanently false once the count exceeds limit. There is no timestamp, no TTL, no cron job, and no decay. This will block every IP after limit total lifetime requests, which for any real-world traffic will happen within hours or days. Confidence: 0.92.

Verification: Verified genuine. The module-level requestCounts (line 59) is a plain Record<string, number> that only ever increments (line 65) and never resets, expires, or decays. There is no setInterval, no TTL, no timestamp, no sliding window, and no external reset mechanism anywhere in the file or codebase (confirmed via Grep). Once requestCounts[ip] exceeds limit, the function permanently returns false for that IP. The function is labeled 'Rate limiter' but implements a lifetime counter, not a rate limiter. While no callers currently exist, the implementation is fundamentally broken for its stated purpose. Slightly lower confidence only because one could argue this is an intentionally simple counter (though the naming strongly suggests otherwise).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant