SQL injection, path traversal, SSRF, and auth brute-force security fixes

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(),
 });

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<ChallengeResponse> {
+    // #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<AuthResponse> {
+    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<AuthResponse> {
     // 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.

src/modules/courses/course.service.ts

@@ -2686,12 +2686,16 @@ export class CourseService {
     const cached = await cacheGet<EnrollmentTrendsResult>(namespace, ck);
     if (cached) return cached;
 
-    const intervalMap: Record<string, string> = {
+    // Keyed by the Zod enums themselves (EnrollmentTrendsQuery["range"/"granularity"])
+    // rather than `Record<string, string>` (#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<EnrollmentTrendsQuery["range"], string> = {
       "7d": "7 days",
       "30d": "30 days",
       "90d": "90 days",
     };
-    const truncMap: Record<string, string> = {
+    const truncMap: Record<EnrollmentTrendsQuery["granularity"], string> = {
       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<string>`date_trunc('${sql.raw(trunc)}', ${enrollments.enrolledAt})::date`,
+          date: sql<string>`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)

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,15 +283,24 @@ 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",
           message: "Route not found",
         });
       }
 
-      const filePath = path.join(path.resolve(config.AVATAR_UPLOAD_DIR), filename);
       try {
         await access(filePath);
       } catch {

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<LockoutStatus> {
+  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<string, unknown>,
+): Promise<number> {
+  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<void> {
+  await redis.del(countKey(stellarAddress), lockoutKey(stellarAddress));
+}

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;
+}