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/modules/auth/auth.service.ts b/src/modules/auth/auth.service.ts index dda809c..ffec0e0 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/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/src/server.ts b/src/server.ts index 543cd11..e1395e6 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"; import { checkServiceHealth } from "./utils/service-health.js"; // Versioned route modules @@ -283,7 +283,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", @@ -291,7 +301,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/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/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/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/auth/challenge-lockout.test.ts b/tests/unit/auth/challenge-lockout.test.ts new file mode 100644 index 0000000..8c7c820 --- /dev/null +++ b/tests/unit/auth/challenge-lockout.test.ts @@ -0,0 +1,68 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import * as StellarSdk from "@stellar/stellar-sdk"; + +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), +})); +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"; + +// 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/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); + }); +}); 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(), }, })); 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"), + ); + }); + }); +}); 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(); + }); +}); 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(); + }); +});