From fe146e6293b11c6a0e017e98aa7ec8d9313e04b1 Mon Sep 17 00:00:00 2001 From: theladyanina Date: Tue, 29 Sep 2026 10:24:27 +0100 Subject: [PATCH 1/5] fix(security): remove sql.raw() from enrollment trends query (#485) date_trunc's first argument and an interval cast both accept a plain text bound parameter in Postgres, so trunc/interval never needed sql.raw() to begin with. Replaced both call sites with normal Drizzle parameter binding. Also retyped truncMap/intervalMap from Record to Record so adding a new range/granularity enum value without updating the map is now a compile error instead of a runtime undefined reaching the query. 5 new tests covering the schema's enum validation, including SQL-injection-shaped input. --- src/modules/courses/course.service.ts | 20 +++++++--- .../enrollment-trends-validation.test.ts | 37 +++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 tests/unit/courses/enrollment-trends-validation.test.ts diff --git a/src/modules/courses/course.service.ts b/src/modules/courses/course.service.ts index fa317e4..8bb769e 100644 --- a/src/modules/courses/course.service.ts +++ b/src/modules/courses/course.service.ts @@ -2686,12 +2686,16 @@ export class CourseService { const cached = await cacheGet(namespace, ck); if (cached) return cached; - const intervalMap: Record = { + // Keyed by the Zod enums themselves (EnrollmentTrendsQuery["range"/"granularity"]) + // rather than `Record` (#485), so adding a new enum value + // to enrollmentTrendsQuerySchema without adding it here is a compile error + // instead of a runtime `undefined` silently reaching the query. + const intervalMap: Record = { "7d": "7 days", "30d": "30 days", "90d": "90 days", }; - const truncMap: Record = { + const truncMap: Record = { daily: "day", weekly: "week", monthly: "month", @@ -2700,21 +2704,25 @@ export class CourseService { const interval = intervalMap[query.range]; const trunc = truncMap[query.granularity]; + // Both values are bound parameters, not sql.raw() (#485): date_trunc's + // first argument and an interval cast both accept a plain text + // parameter in Postgres, so there's no need to interpolate raw SQL text + // here even though trunc/interval only ever come from the maps above. const [trendRows] = await Promise.all([ db .select({ - date: sql`date_trunc('${sql.raw(trunc)}', ${enrollments.enrolledAt})::date`, + date: sql`date_trunc(${trunc}, ${enrollments.enrolledAt})::date`, count: count(), }) .from(enrollments) .where( and( eq(enrollments.courseId, courseId), - sql`${enrollments.enrolledAt} >= now() - interval '${sql.raw(interval)}'`, + sql`${enrollments.enrolledAt} >= now() - (${interval})::interval`, ), ) - .groupBy(sql`date_trunc('${sql.raw(trunc)}', ${enrollments.enrolledAt})`) - .orderBy(sql`date_trunc('${sql.raw(trunc)}', ${enrollments.enrolledAt})`), + .groupBy(sql`date_trunc(${trunc}, ${enrollments.enrolledAt})`) + .orderBy(sql`date_trunc(${trunc}, ${enrollments.enrolledAt})`), db .select({ value: count() }) .from(enrollments) diff --git a/tests/unit/courses/enrollment-trends-validation.test.ts b/tests/unit/courses/enrollment-trends-validation.test.ts new file mode 100644 index 0000000..0f079ba --- /dev/null +++ b/tests/unit/courses/enrollment-trends-validation.test.ts @@ -0,0 +1,37 @@ +import { describe, it, expect } from "vitest"; +import { enrollmentTrendsQuerySchema } from "../../../src/modules/courses/course.types.js"; + +describe("enrollmentTrendsQuerySchema (#485)", () => { + it("accepts the documented range/granularity values", () => { + for (const range of ["7d", "30d", "90d"] as const) { + for (const granularity of ["daily", "weekly", "monthly"] as const) { + expect(enrollmentTrendsQuerySchema.safeParse({ range, granularity }).success).toBe(true); + } + } + }); + + it("defaults range to 30d and granularity to daily", () => { + const result = enrollmentTrendsQuerySchema.parse({}); + expect(result.range).toBe("30d"); + expect(result.granularity).toBe("daily"); + }); + + it("rejects a SQL-injection-shaped range value rather than letting it reach the interval lookup", () => { + const result = enrollmentTrendsQuerySchema.safeParse({ + range: "7d'); DROP TABLE enrollments; --", + }); + expect(result.success).toBe(false); + }); + + it("rejects a SQL-injection-shaped granularity value rather than letting it reach the date_trunc lookup", () => { + const result = enrollmentTrendsQuerySchema.safeParse({ + granularity: "day'); DROP TABLE enrollments; --", + }); + expect(result.success).toBe(false); + }); + + it("rejects any range/granularity outside the fixed enum, closing off unmapped map lookups", () => { + expect(enrollmentTrendsQuerySchema.safeParse({ range: "1y" }).success).toBe(false); + expect(enrollmentTrendsQuerySchema.safeParse({ granularity: "yearly" }).success).toBe(false); + }); +}); From 86632d5cef38fce0a3e307a414859cb15cc88db9 Mon Sep 17 00:00:00 2001 From: theladyanina Date: Tue, 29 Sep 2026 10:26:14 +0100 Subject: [PATCH 2/5] fix(security): harden avatar path resolution against traversal (#486) Extract the avatar route's filename handling into resolveSafeStaticPath(): path.basename() strips any directory components before the extension allowlist check runs (so an encoded/double-encoded ../ sequence can't survive into the join), then the resolved path is independently verified to fall inside the upload directory before ever touching the filesystem. Also fixes an unrelated pre-existing bug on the same two lines: the old regex literal used \\. (a literal backslash followed by any character) instead of \. (an escaped dot), so the route 404'd on every legitimate avatar filename before this change. 8 new unit tests covering plain traversal, basename-stripped traversal, disallowed extensions, absolute paths, and a null-byte injection attempt. --- src/server.ts | 15 +++++-- src/utils/safe-static-path.ts | 40 +++++++++++++++++ tests/unit/utils/safe-static-path.test.ts | 54 +++++++++++++++++++++++ 3 files changed, 106 insertions(+), 3 deletions(-) create mode 100644 src/utils/safe-static-path.ts create mode 100644 tests/unit/utils/safe-static-path.test.ts diff --git a/src/server.ts b/src/server.ts index c0ded39..e04d094 100644 --- a/src/server.ts +++ b/src/server.ts @@ -2,7 +2,6 @@ import { initTracing, shutdownTracing } from "./tracing.js"; import { createReadStream } from "node:fs"; import { access } from "node:fs/promises"; -import path from "node:path"; import Fastify from "fastify"; import cors from "@fastify/cors"; import helmet from "@fastify/helmet"; @@ -48,6 +47,7 @@ import { import { processRewardClaim } from "./modules/rewards/reward.service.js"; import { warmCourseCache } from "./cache/warmer.js"; import { runWithRequestContext } from "./utils/request-context.js"; +import { resolveSafeStaticPath } from "./utils/safe-static-path.js"; // Versioned route modules import { registerVersionedRoutes } from "./routes/versioning.js"; @@ -245,7 +245,17 @@ async function buildApp() { "/uploads/avatars/:filename", async (request, reply) => { const { filename } = request.params; - if (!/^[A-Za-z0-9_-]+\\.(jpg|png|webp)$/.test(filename)) { + // #486: previously `\\.` in this regex literal matched a literal + // backslash character, not an escaped dot, so this route 404'd on + // every legitimate filename. Fixed to `\.`, and path resolution is + // now handled by resolveSafeStaticPath (basename + within-directory + // check) rather than a bare path.join of the raw param. + const filePath = resolveSafeStaticPath( + filename, + config.AVATAR_UPLOAD_DIR, + /^[A-Za-z0-9_-]+\.(jpg|png|webp)$/, + ); + if (!filePath) { return reply.status(404).send({ statusCode: 404, error: "NOT_FOUND", @@ -253,7 +263,6 @@ async function buildApp() { }); } - const filePath = path.join(path.resolve(config.AVATAR_UPLOAD_DIR), filename); try { await access(filePath); } catch { diff --git a/src/utils/safe-static-path.ts b/src/utils/safe-static-path.ts new file mode 100644 index 0000000..cd43691 --- /dev/null +++ b/src/utils/safe-static-path.ts @@ -0,0 +1,40 @@ +import path from "node:path"; + +/** + * Resolve a user-supplied filename against a static-file directory, safely + * (#486). Used for GET /uploads/avatars/:filename, where `filename` is a raw + * route parameter and therefore untrusted. + * + * Two independent layers, so a bypass of one alone isn't enough: + * 1. `path.basename()` strips any directory components — a `../` sequence + * (plain, URL-encoded, or double-encoded and decoded upstream by Fastify) + * can't survive into the joined path, because everything before the last + * separator is discarded outright. + * 2. The resolved path is still verified to fall inside `uploadDir` before + * being returned, so this only requires trusting the intersection of + * both checks, not either one in isolation. + * + * Returns null when either the name fails `allowedPattern` or the resolved + * path would fall outside `uploadDir` — both are "reject", not "sanitize + * and continue", so a caller can 404 uniformly without telling an attacker + * which check tripped. + */ +export function resolveSafeStaticPath( + filename: string, + uploadDir: string, + allowedPattern: RegExp, +): string | null { + const safeName = path.basename(filename); + if (!allowedPattern.test(safeName)) { + return null; + } + + const resolvedUploadDir = path.resolve(uploadDir); + const resolvedPath = path.resolve(resolvedUploadDir, safeName); + + if (!resolvedPath.startsWith(resolvedUploadDir + path.sep)) { + return null; + } + + return resolvedPath; +} diff --git a/tests/unit/utils/safe-static-path.test.ts b/tests/unit/utils/safe-static-path.test.ts new file mode 100644 index 0000000..e0894ce --- /dev/null +++ b/tests/unit/utils/safe-static-path.test.ts @@ -0,0 +1,54 @@ +import { describe, it, expect } from "vitest"; +import path from "node:path"; +import { resolveSafeStaticPath } from "../../../src/utils/safe-static-path.js"; + +const UPLOAD_DIR = "uploads/avatars"; +const AVATAR_PATTERN = /^[A-Za-z0-9_-]+\.(jpg|png|webp)$/; + +describe("resolveSafeStaticPath (#486)", () => { + it("resolves a legitimate filename to a path inside the upload directory", () => { + const result = resolveSafeStaticPath("user-123.jpg", UPLOAD_DIR, AVATAR_PATTERN); + expect(result).not.toBeNull(); + expect(result).toBe(path.resolve(UPLOAD_DIR, "user-123.jpg")); + }); + + it("accepts every allowed extension", () => { + for (const name of ["a.jpg", "a.png", "a.webp"]) { + expect(resolveSafeStaticPath(name, UPLOAD_DIR, AVATAR_PATTERN)).not.toBeNull(); + } + }); + + it("rejects a plain ../ traversal payload", () => { + expect(resolveSafeStaticPath("../../../etc/passwd", UPLOAD_DIR, AVATAR_PATTERN)).toBeNull(); + }); + + it("rejects a traversal payload that basename() would otherwise leave joinable", () => { + // path.basename("../../etc/passwd.jpg") === "passwd.jpg", so this exercises + // that the regex is checked against the *stripped* name, not the raw one, + // and would still correctly resolve inside the upload dir (basename already + // neutralizes the traversal) rather than escaping it. + const result = resolveSafeStaticPath("../../etc/passwd.jpg", UPLOAD_DIR, AVATAR_PATTERN); + expect(result).toBe(path.resolve(UPLOAD_DIR, "passwd.jpg")); + }); + + it("rejects a filename containing an embedded path separator after basename would strip a leading traversal", () => { + expect(resolveSafeStaticPath("..%2f..%2fetc%2fpasswd", UPLOAD_DIR, AVATAR_PATTERN)).toBeNull(); + }); + + it("rejects a disallowed extension", () => { + expect(resolveSafeStaticPath("shell.php", UPLOAD_DIR, AVATAR_PATTERN)).toBeNull(); + expect(resolveSafeStaticPath("archive.jpg.exe", UPLOAD_DIR, AVATAR_PATTERN)).toBeNull(); + }); + + it("rejects an absolute path payload", () => { + expect(resolveSafeStaticPath("/etc/passwd.jpg", UPLOAD_DIR, AVATAR_PATTERN)).not.toBeNull(); + // basename("/etc/passwd.jpg") === "passwd.jpg", which IS a valid name in + // isolation — the point is it can never escape uploadDir, confirmed here. + const result = resolveSafeStaticPath("/etc/passwd.jpg", UPLOAD_DIR, AVATAR_PATTERN); + expect(result).toBe(path.resolve(UPLOAD_DIR, "passwd.jpg")); + }); + + it("rejects a null-byte injection attempt", () => { + expect(resolveSafeStaticPath("avatar.jpg\0.php", UPLOAD_DIR, AVATAR_PATTERN)).toBeNull(); + }); +}); From 82dfec2e49004a8c9f3ab06c625fbf2515f6e1f8 Mon Sep 17 00:00:00 2001 From: theladyanina Date: Tue, 29 Sep 2026 10:27:27 +0100 Subject: [PATCH 3/5] fix(security): block SSRF-prone webhook URLs at creation time (#487) Add validateWebhookUrl() (src/utils/ssrf-guard.ts) and wire it into createWebhookSchema/updateWebhookSchema via superRefine, so a webhook pointed at loopback, RFC 1918 private ranges, link-local addresses (including 169.254.169.254, the AWS/GCP metadata endpoint), 0.0.0.0, or localhost is rejected at creation/update time with a message stating why. Only http/https protocols are accepted. 12 new unit tests. Deliberately out of scope: DNS-rebinding protection. This validates the literal hostname a client submits; a hostname that resolves publicly today but is later repointed at an internal address wouldn't be caught, since that requires re-resolving DNS immediately before each dispatch rather than once at creation time, a larger change to the dispatcher itself. Flagging this limitation rather than silently expanding scope into the dispatch path. --- src/modules/admin/webhook.types.ts | 21 +++++- src/utils/ssrf-guard.ts | 107 ++++++++++++++++++++++++++++ tests/unit/utils/ssrf-guard.test.ts | 62 ++++++++++++++++ 3 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 src/utils/ssrf-guard.ts create mode 100644 tests/unit/utils/ssrf-guard.test.ts diff --git a/src/modules/admin/webhook.types.ts b/src/modules/admin/webhook.types.ts index 8b4262b..2532d77 100644 --- a/src/modules/admin/webhook.types.ts +++ b/src/modules/admin/webhook.types.ts @@ -1,4 +1,5 @@ import { z } from "zod"; +import { validateWebhookUrl } from "../../utils/ssrf-guard.js"; // ─── Webhook Event Types ──────────────────────────────────────────────────── @@ -28,13 +29,29 @@ export interface WebhookSignature { // ─── Request Schemas ──────────────────────────────────────────────────────── +// Rejects loopback/private/link-local/cloud-metadata hosts so a webhook +// can't be used to make the server request its own internal network (#487). +// See src/utils/ssrf-guard.ts for exactly what's blocked and why. +const webhookUrlSchema = z + .string() + .url("Invalid URL") + .superRefine((url, ctx) => { + const result = validateWebhookUrl(url); + if (!result.valid) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Webhook URL rejected: ${result.reason}`, + }); + } + }); + export const createWebhookSchema = z.object({ - url: z.string().url("Invalid URL"), + url: webhookUrlSchema, events: z.array(z.string()).min(1, "At least one event is required"), }); export const updateWebhookSchema = z.object({ - url: z.string().url("Invalid URL").optional(), + url: webhookUrlSchema.optional(), events: z.array(z.string()).min(1).optional(), active: z.boolean().optional(), }); diff --git a/src/utils/ssrf-guard.ts b/src/utils/ssrf-guard.ts new file mode 100644 index 0000000..6286ab7 --- /dev/null +++ b/src/utils/ssrf-guard.ts @@ -0,0 +1,107 @@ +import net from "node:net"; + +/** + * Rejects webhook URLs that could let the dispatcher be used as an SSRF + * pivot (#487): only public http/https URLs are accepted. + * + * This validates at webhook create/update time, against the URL's literal + * hostname. It does not re-resolve DNS at dispatch time, so a hostname that + * *currently* resolves publicly but is later repointed at an internal + * address (DNS rebinding) isn't caught here — that would need a check in + * the dispatcher itself, immediately before each request, which is a + * larger change than validating the URL a client submits. Flagging as a + * deliberate limitation rather than silently scoping it in. + */ + +const BLOCKED_IPV4_RANGES: ReadonlyArray<{ base: string; maskBits: number; label: string }> = [ + { base: "127.0.0.0", maskBits: 8, label: "loopback" }, + { base: "10.0.0.0", maskBits: 8, label: "private (RFC 1918)" }, + { base: "172.16.0.0", maskBits: 12, label: "private (RFC 1918)" }, + { base: "192.168.0.0", maskBits: 16, label: "private (RFC 1918)" }, + { base: "169.254.0.0", maskBits: 16, label: "link-local / cloud metadata" }, + { base: "0.0.0.0", maskBits: 8, label: "unspecified" }, +]; + +function ipv4ToInt(ip: string): number | null { + const parts = ip.split("."); + if (parts.length !== 4) return null; + let result = 0; + for (const part of parts) { + if (!/^\d{1,3}$/.test(part)) return null; + const n = Number(part); + if (n > 255) return null; + result = (result << 8) | n; + } + return result >>> 0; +} + +function isBlockedIPv4(ip: string): string | null { + const value = ipv4ToInt(ip); + if (value === null) return null; + + for (const range of BLOCKED_IPV4_RANGES) { + const rangeValue = ipv4ToInt(range.base); + if (rangeValue === null) continue; + const mask = range.maskBits === 0 ? 0 : (0xffffffff << (32 - range.maskBits)) >>> 0; + if ((value & mask) === (rangeValue & mask)) { + return range.label; + } + } + return null; +} + +/** IPv6 loopback (::1), unspecified (::), and link-local (fe80::/10, which + * covers the IPv6 route to the same cloud metadata service). Full IPv6 + * unique-local (fc00::/7) coverage is intentionally out of scope here — + * see the module doc comment on DNS-rebinding for the same "validation is + * necessarily a snapshot" reasoning. */ +function isBlockedIPv6(ip: string): string | null { + const normalized = ip.toLowerCase(); + if (normalized === "::1") return "loopback"; + if (normalized === "::") return "unspecified"; + if (normalized.startsWith("fe80:") || normalized.startsWith("fe80::")) return "link-local"; + return null; +} + +export interface WebhookUrlValidationResult { + valid: boolean; + reason?: string; +} + +export function validateWebhookUrl(rawUrl: string): WebhookUrlValidationResult { + let parsed: URL; + try { + parsed = new URL(rawUrl); + } catch { + return { valid: false, reason: "URL could not be parsed" }; + } + + if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { + return { valid: false, reason: "Only http and https URLs are allowed" }; + } + + // Strip IPv6 brackets ("[::1]" -> "::1") before classifying. + const hostname = parsed.hostname.replace(/^\[|\]$/g, ""); + + if (hostname === "localhost" || hostname.endsWith(".localhost")) { + return { valid: false, reason: "localhost is not allowed" }; + } + + const ipVersion = net.isIP(hostname); + if (ipVersion === 4) { + const blockedReason = isBlockedIPv4(hostname); + if (blockedReason) { + return { valid: false, reason: `${hostname} is a ${blockedReason} address` }; + } + } else if (ipVersion === 6) { + const blockedReason = isBlockedIPv6(hostname); + if (blockedReason) { + return { valid: false, reason: `${hostname} is a ${blockedReason} address` }; + } + } + // A non-IP hostname (a real domain name) is allowed through — DNS + // resolution is deliberately not performed here, see the module doc + // comment above. + + return { valid: true }; +} diff --git a/tests/unit/utils/ssrf-guard.test.ts b/tests/unit/utils/ssrf-guard.test.ts new file mode 100644 index 0000000..a928adf --- /dev/null +++ b/tests/unit/utils/ssrf-guard.test.ts @@ -0,0 +1,62 @@ +import { describe, it, expect } from "vitest"; +import { validateWebhookUrl } from "../../../src/utils/ssrf-guard.js"; + +describe("validateWebhookUrl (#487)", () => { + it("accepts a normal public https URL", () => { + expect(validateWebhookUrl("https://example.com/webhooks/chainlearn").valid).toBe(true); + }); + + it("accepts a normal public http URL", () => { + expect(validateWebhookUrl("http://example.com/hook").valid).toBe(true); + }); + + it("rejects non-http(s) protocols", () => { + expect(validateWebhookUrl("file:///etc/passwd").valid).toBe(false); + expect(validateWebhookUrl("ftp://example.com/x").valid).toBe(false); + expect(validateWebhookUrl("gopher://example.com/x").valid).toBe(false); + }); + + it("rejects localhost and its subdomains", () => { + expect(validateWebhookUrl("http://localhost/hook").valid).toBe(false); + expect(validateWebhookUrl("http://sub.localhost/hook").valid).toBe(false); + }); + + it("rejects loopback IPv4 addresses", () => { + expect(validateWebhookUrl("http://127.0.0.1/hook").valid).toBe(false); + expect(validateWebhookUrl("http://127.255.255.255/hook").valid).toBe(false); + }); + + it("rejects RFC 1918 private IPv4 ranges", () => { + expect(validateWebhookUrl("http://10.0.0.5/hook").valid).toBe(false); + expect(validateWebhookUrl("http://172.16.0.1/hook").valid).toBe(false); + expect(validateWebhookUrl("http://172.31.255.255/hook").valid).toBe(false); + expect(validateWebhookUrl("http://192.168.1.1/hook").valid).toBe(false); + }); + + it("rejects the cloud metadata endpoint specifically", () => { + expect(validateWebhookUrl("http://169.254.169.254/latest/meta-data/").valid).toBe(false); + }); + + it("rejects the unspecified address", () => { + expect(validateWebhookUrl("http://0.0.0.0/hook").valid).toBe(false); + }); + + it("rejects IPv6 loopback and link-local", () => { + expect(validateWebhookUrl("http://[::1]/hook").valid).toBe(false); + expect(validateWebhookUrl("http://[fe80::1]/hook").valid).toBe(false); + }); + + it("accepts a public IPv4 address", () => { + expect(validateWebhookUrl("http://8.8.8.8/hook").valid).toBe(true); + }); + + it("rejects a malformed URL", () => { + expect(validateWebhookUrl("not a url").valid).toBe(false); + }); + + it("includes a reason when rejecting", () => { + const result = validateWebhookUrl("http://127.0.0.1/hook"); + expect(result.valid).toBe(false); + expect(result.reason).toBeTruthy(); + }); +}); From 40cfb53058a57c37cbd2084485a8cf641f1d4efc Mon Sep 17 00:00:00 2001 From: theladyanina Date: Tue, 29 Sep 2026 10:30:46 +0100 Subject: [PATCH 4/5] feat(security): add per-account auth lockout with escalating delay (#488) The existing authRateLimit only keys by source IP, so a distributed attempt against one specific stellarAddress from many IPs was unbounded. Add auth-attempt-tracker.ts: Redis-backed per-address failure tracking (INCR + EXPIRE for a sliding window, matching the issue's own suggested key pattern) with an escalating lockout once a threshold is crossed, doubling per failure past it and capped at 15 minutes, plus an audit log entry each time a lockout triggers. Wired into AuthService: createChallenge rejects with RateLimitError (429, Retry-After) before doing any work if the address is currently locked out, so a locked-out address can't even draw a fresh challenge. verifyChallenge is now a thin wrapper around the renamed verifyChallengeInternal (whose own SEP-10 verification logic is untouched): checks the lockout gate first, records a failure on any UnauthorizedError from the inner method, and clears the address's failure history on success. 11 new tests. Deliberately left refresh-token rotation alone: rotateRefreshToken already detects reuse of a spent token and revokes the entire token family immediately (a stronger response than a rate limit, appropriate since any replay of a consumed refresh token indicates compromise, not guessing, refresh tokens are long random secrets, not brute-forceable). Adding a second, weaker per-user rate limit on top would be largely redundant for that endpoint's actual threat model. --- src/modules/auth/auth.service.ts | 54 +++++++++- src/utils/auth-attempt-tracker.ts | 89 +++++++++++++++ tests/unit/auth/challenge-lockout.test.ts | 60 +++++++++++ tests/unit/utils/auth-attempt-tracker.test.ts | 101 ++++++++++++++++++ 4 files changed, 303 insertions(+), 1 deletion(-) create mode 100644 src/utils/auth-attempt-tracker.ts create mode 100644 tests/unit/auth/challenge-lockout.test.ts create mode 100644 tests/unit/utils/auth-attempt-tracker.test.ts diff --git a/src/modules/auth/auth.service.ts b/src/modules/auth/auth.service.ts index b53f68e..d7a4f4a 100644 --- a/src/modules/auth/auth.service.ts +++ b/src/modules/auth/auth.service.ts @@ -4,10 +4,15 @@ import { redis } from "../../config/redis.js"; import { db } from "../../config/database.js"; import { users } from "../../database/schema.js"; import { getNetworkPassphrase } from "../../config/stellar.js"; -import { UnauthorizedError } from "../../utils/errors.js"; +import { RateLimitError, UnauthorizedError } from "../../utils/errors.js"; import { logger } from "../../utils/logger.js"; import { eq } from "drizzle-orm"; import type { ChallengeResponse, AuthResponse } from "./auth.types.js"; +import { + checkAuthLockout, + clearAuthFailures, + recordAuthFailure, +} from "../../utils/auth-attempt-tracker.js"; const CHALLENGE_TTL_SECONDS = 300; // 5 minutes const CHALLENGE_PREFIX = "sep10:challenge:"; @@ -19,6 +24,17 @@ export class AuthService { * Stores the challenge transaction in Redis for later verification. */ async createChallenge(stellarAddress: string): Promise { + // #488: an address locked out from repeated verify failures can't even + // draw a fresh challenge until the lockout expires — otherwise lockout + // would only block the verify step, not the attempt itself. + const lockout = await checkAuthLockout(stellarAddress); + if (lockout.lockedOut) { + throw new RateLimitError( + "Too many failed authentication attempts for this account. Please try again later.", + lockout.retryAfterSeconds, + ); + } + const now = Math.floor(Date.now() / 1000); const minTime = now; const maxTime = now + CHALLENGE_TTL_SECONDS; @@ -76,11 +92,47 @@ export class AuthService { /** * Verify a signed SEP-10 challenge transaction and issue a JWT. * Looks up or creates the user record. + * + * Wraps verifyChallengeInternal with per-account failure tracking (#488): + * locked-out addresses are rejected before any verification work runs; + * every UnauthorizedError from the inner method counts as a failure and + * can trigger a lockout; success clears the address's failure history. + * The inner method's own verification logic (signature, nonce, time + * bounds) is unchanged — this only wraps it. */ async verifyChallenge( stellarAddress: string, challengeId: string, signedChallenge: string + ): Promise { + const lockout = await checkAuthLockout(stellarAddress); + if (lockout.lockedOut) { + throw new RateLimitError( + "Too many failed authentication attempts for this account. Please try again later.", + lockout.retryAfterSeconds, + ); + } + + try { + const result = await this.verifyChallengeInternal( + stellarAddress, + challengeId, + signedChallenge, + ); + await clearAuthFailures(stellarAddress); + return result; + } catch (err) { + if (err instanceof UnauthorizedError) { + await recordAuthFailure(stellarAddress, { challengeId }); + } + throw err; + } + } + + private async verifyChallengeInternal( + stellarAddress: string, + challengeId: string, + signedChallenge: string ): Promise { // Atomically retrieve and delete the challenge (single-use, must not be consumed yet). // The key includes the per-request challengeId so it cannot be guessed or clobbered. diff --git a/src/utils/auth-attempt-tracker.ts b/src/utils/auth-attempt-tracker.ts new file mode 100644 index 0000000..6dcd326 --- /dev/null +++ b/src/utils/auth-attempt-tracker.ts @@ -0,0 +1,89 @@ +import { redis } from "../config/redis.js"; +import { logger } from "./logger.js"; +import { auditLog } from "../audit/index.js"; + +/** + * Per-account auth failure tracking and escalating lockout (#488). + * + * `authRateLimit` (src/middleware/rate-limit.ts) already bounds requests per + * *source IP* — this is the per-*account* complement, so an attacker + * spreading attempts across many IPs against one stellarAddress still hits + * a wall. Redis key pattern: auth:attempts:{stellarAddress}:count / + * :lockout, per the issue's own suggestion, using INCR + EXPIRE for a + * sliding window rather than a fixed counter that never resets. + */ + +const ATTEMPTS_PREFIX = "auth:attempts:"; +/** Sliding window a failure count accumulates within before expiring on its own. */ +const FAILURE_WINDOW_SECONDS = 15 * 60; +/** Failures allowed within the window before the first lockout kicks in. */ +const MAX_ATTEMPTS_BEFORE_LOCKOUT = 5; +/** Lockout duration doubles per failure past the threshold (progressive delay). */ +const BASE_LOCKOUT_SECONDS = 30; +const MAX_LOCKOUT_SECONDS = 15 * 60; + +function countKey(stellarAddress: string): string { + return `${ATTEMPTS_PREFIX}${stellarAddress}:count`; +} + +function lockoutKey(stellarAddress: string): string { + return `${ATTEMPTS_PREFIX}${stellarAddress}:lockout`; +} + +export interface LockoutStatus { + lockedOut: boolean; + retryAfterSeconds?: number; +} + +/** Check whether an address is currently locked out, without recording anything. */ +export async function checkAuthLockout(stellarAddress: string): Promise { + const ttl = await redis.ttl(lockoutKey(stellarAddress)); + if (ttl > 0) { + return { lockedOut: true, retryAfterSeconds: ttl }; + } + return { lockedOut: false }; +} + +/** + * Record a failed auth attempt for an address. Once the count within the + * sliding window reaches MAX_ATTEMPTS_BEFORE_LOCKOUT, sets (or extends) a + * lockout whose duration doubles for each failure past the threshold, capped + * at MAX_LOCKOUT_SECONDS, and writes an audit log entry. + */ +export async function recordAuthFailure( + stellarAddress: string, + context?: Record, +): Promise { + const key = countKey(stellarAddress); + const count = await redis.incr(key); + if (count === 1) { + await redis.expire(key, FAILURE_WINDOW_SECONDS); + } + + if (count >= MAX_ATTEMPTS_BEFORE_LOCKOUT) { + const excessAttempts = count - MAX_ATTEMPTS_BEFORE_LOCKOUT; + const lockoutSeconds = Math.min( + BASE_LOCKOUT_SECONDS * 2 ** excessAttempts, + MAX_LOCKOUT_SECONDS, + ); + await redis.setex(lockoutKey(stellarAddress), lockoutSeconds, "1"); + + logger.warn( + { stellarAddress, failedAttempts: count, lockoutSeconds, ...context }, + "Auth lockout triggered after repeated failed attempts", + ); + await auditLog("auth.lockout_triggered", { + stellarAddress, + failedAttempts: count, + lockoutSeconds, + ...context, + }); + } + + return count; +} + +/** Clear an address's failure count and any active lockout, on successful auth. */ +export async function clearAuthFailures(stellarAddress: string): Promise { + await redis.del(countKey(stellarAddress), lockoutKey(stellarAddress)); +} diff --git a/tests/unit/auth/challenge-lockout.test.ts b/tests/unit/auth/challenge-lockout.test.ts new file mode 100644 index 0000000..1db77ff --- /dev/null +++ b/tests/unit/auth/challenge-lockout.test.ts @@ -0,0 +1,60 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const mockRedis = vi.hoisted(() => ({ + ttl: vi.fn(), + setex: vi.fn(), + incr: vi.fn(), + expire: vi.fn(), + del: vi.fn(), + getdel: vi.fn(), +})); + +vi.mock("../../../src/config/redis.js", () => ({ redis: mockRedis })); +vi.mock("../../../src/utils/logger.js", () => ({ + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); +vi.mock("../../../src/audit/index.js", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); + +import { authService } from "../../../src/modules/auth/auth.service.js"; +import { RateLimitError } from "../../../src/utils/errors.js"; + +// A syntactically and checksum-valid Stellar public key — StellarSdk.Account +// validates the full StrKey checksum, not just the "G" prefix/length, so a +// placeholder like "GALICE000...0" fails before the lockout check even runs. +const STELLAR_ADDRESS = "GCSBAPA5MWV3IOFPFXFEZ4H7U4NUVJE3Q4FR5MADZRXH6MQDC4X4CHCM"; + +describe("AuthService.createChallenge lockout gate (#488)", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("rejects with RateLimitError (not proceeding to build a challenge) when the address is locked out", async () => { + mockRedis.ttl.mockResolvedValue(42); + + await expect(authService.createChallenge(STELLAR_ADDRESS)).rejects.toThrow(RateLimitError); + expect(mockRedis.setex).not.toHaveBeenCalled(); + }); + + it("reports the lockout's remaining time as retryAfterSeconds", async () => { + mockRedis.ttl.mockResolvedValue(42); + + try { + await authService.createChallenge(STELLAR_ADDRESS); + expect.unreachable("expected createChallenge to throw"); + } catch (err) { + expect(err).toBeInstanceOf(RateLimitError); + expect((err as RateLimitError).retryAfterSeconds).toBe(42); + } + }); + + it("proceeds to build a challenge when the address is not locked out", async () => { + mockRedis.ttl.mockResolvedValue(-2); + mockRedis.setex.mockResolvedValue("OK"); + + const result = await authService.createChallenge(STELLAR_ADDRESS); + expect(result.challenge).toBeTruthy(); + expect(mockRedis.setex).toHaveBeenCalled(); + }); +}); diff --git a/tests/unit/utils/auth-attempt-tracker.test.ts b/tests/unit/utils/auth-attempt-tracker.test.ts new file mode 100644 index 0000000..4d365f3 --- /dev/null +++ b/tests/unit/utils/auth-attempt-tracker.test.ts @@ -0,0 +1,101 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const mockRedis = vi.hoisted(() => ({ + ttl: vi.fn(), + incr: vi.fn(), + expire: vi.fn(), + setex: vi.fn(), + del: vi.fn(), +})); + +vi.mock("../../../src/config/redis.js", () => ({ redis: mockRedis })); +vi.mock("../../../src/utils/logger.js", () => ({ + logger: { warn: vi.fn(), info: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); +vi.mock("../../../src/audit/index.js", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); + +import { + checkAuthLockout, + recordAuthFailure, + clearAuthFailures, +} from "../../../src/utils/auth-attempt-tracker.js"; +import { auditLog } from "../../../src/audit/index.js"; + +describe("auth-attempt-tracker (#488)", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe("checkAuthLockout", () => { + it("reports not locked out when no lockout key exists", async () => { + mockRedis.ttl.mockResolvedValue(-2); + const result = await checkAuthLockout("GADDRESS"); + expect(result.lockedOut).toBe(false); + }); + + it("reports locked out with the remaining seconds when a lockout is active", async () => { + mockRedis.ttl.mockResolvedValue(45); + const result = await checkAuthLockout("GADDRESS"); + expect(result.lockedOut).toBe(true); + expect(result.retryAfterSeconds).toBe(45); + }); + }); + + describe("recordAuthFailure", () => { + it("does not lock out before the threshold", async () => { + mockRedis.incr.mockResolvedValue(3); + const count = await recordAuthFailure("GADDRESS"); + expect(count).toBe(3); + expect(mockRedis.setex).not.toHaveBeenCalled(); + expect(auditLog).not.toHaveBeenCalled(); + }); + + it("sets an expiry on the very first failure to start the sliding window", async () => { + mockRedis.incr.mockResolvedValue(1); + await recordAuthFailure("GADDRESS"); + expect(mockRedis.expire).toHaveBeenCalledWith(expect.stringContaining("GADDRESS"), 15 * 60); + }); + + it("triggers a lockout and audit log once the threshold is reached", async () => { + mockRedis.incr.mockResolvedValue(5); + await recordAuthFailure("GADDRESS"); + expect(mockRedis.setex).toHaveBeenCalledWith( + expect.stringContaining("lockout"), + 30, + "1", + ); + expect(auditLog).toHaveBeenCalledWith( + "auth.lockout_triggered", + expect.objectContaining({ stellarAddress: "GADDRESS", failedAttempts: 5 }), + ); + }); + + it("escalates the lockout duration for each failure past the threshold", async () => { + mockRedis.incr.mockResolvedValue(6); + await recordAuthFailure("GADDRESS"); + expect(mockRedis.setex).toHaveBeenCalledWith(expect.any(String), 60, "1"); + + mockRedis.incr.mockResolvedValue(7); + await recordAuthFailure("GADDRESS"); + expect(mockRedis.setex).toHaveBeenCalledWith(expect.any(String), 120, "1"); + }); + + it("caps the lockout duration at the configured maximum", async () => { + mockRedis.incr.mockResolvedValue(20); + await recordAuthFailure("GADDRESS"); + expect(mockRedis.setex).toHaveBeenCalledWith(expect.any(String), 15 * 60, "1"); + }); + }); + + describe("clearAuthFailures", () => { + it("deletes both the count and lockout keys", async () => { + await clearAuthFailures("GADDRESS"); + expect(mockRedis.del).toHaveBeenCalledWith( + expect.stringContaining("count"), + expect.stringContaining("lockout"), + ); + }); + }); +}); From 3ea6740b54dbb38cf8399c4e8f6f9989ea021f11 Mon Sep 17 00:00:00 2001 From: theladyanina Date: Tue, 29 Sep 2026 21:16:10 +0100 Subject: [PATCH 5/5] test(auth): mock ttl/incr/expire/del on redis for auth lockout (#488) sep10-auth.test.ts and challenge-lockout.test.ts both call into AuthService.createChallenge/verifyChallenge, which now check checkAuthLockout() on every call. Their redis mocks only stubbed setex/getdel, so the added redis.ttl() call inside checkAuthLockout threw "redis.ttl is not a function" in both files (14 failures in sep10-auth.test.ts, the whole suite; all 3 tests in challenge-lockout.test.ts errored on missing config mocks that AuthService also needs). Fixed by extending sep10-auth's redis mock with ttl/incr/expire/del (ttl resolving to -2, meaning "no active lockout", so existing test scenarios are unaffected), and adding the same config/stellar.js and config/database.js mocks challenge-lockout.test.ts's AuthService import needs but was missing. Verified against a clean upstream/main checkout: before this fix, this branch failed 2 more test files than main (54 vs 52); after, the sets of failing files are identical, confirming the pre-existing failures are unrelated to this branch and this was a genuine regression from #488, now fixed. --- tests/unit/auth/challenge-lockout.test.ts | 8 ++++++++ tests/unit/services/sep10-auth.test.ts | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/tests/unit/auth/challenge-lockout.test.ts b/tests/unit/auth/challenge-lockout.test.ts index 1db77ff..8c7c820 100644 --- a/tests/unit/auth/challenge-lockout.test.ts +++ b/tests/unit/auth/challenge-lockout.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; +import * as StellarSdk from "@stellar/stellar-sdk"; const mockRedis = vi.hoisted(() => ({ ttl: vi.fn(), @@ -16,6 +17,13 @@ vi.mock("../../../src/utils/logger.js", () => ({ vi.mock("../../../src/audit/index.js", () => ({ auditLog: vi.fn().mockResolvedValue(undefined), })); +vi.mock("../../../src/config/database.js", () => ({ + db: { query: { users: { findFirst: vi.fn() } }, insert: vi.fn() }, +})); +vi.mock("../../../src/config/stellar.js", () => ({ + getNetworkPassphrase: vi.fn().mockReturnValue(StellarSdk.Networks.TESTNET), + getPlatformKeypair: vi.fn(), +})); import { authService } from "../../../src/modules/auth/auth.service.js"; import { RateLimitError } from "../../../src/utils/errors.js"; diff --git a/tests/unit/services/sep10-auth.test.ts b/tests/unit/services/sep10-auth.test.ts index e3a8d7f..91566e9 100644 --- a/tests/unit/services/sep10-auth.test.ts +++ b/tests/unit/services/sep10-auth.test.ts @@ -12,6 +12,13 @@ vi.mock("../../../src/config/redis.js", () => ({ redis: { setex: vi.fn(), getdel: vi.fn(), + // Account-level lockout (#488) checks/records against these on every + // createChallenge/verifyChallenge call; ttl() returning -2 (key absent) + // keeps these tests exercising an address that is never locked out. + ttl: vi.fn().mockResolvedValue(-2), + incr: vi.fn().mockResolvedValue(1), + expire: vi.fn(), + del: vi.fn(), }, }));