web security audit fixes

2026-06-02 10:30:42 -04:00
parent 36b087ae92
commit ab0d4857db
26 changed files with 1527 additions and 289 deletions
--- a/web/src/server/api/routers/billing.test.ts
+++ b/web/src/server/api/routers/billing.test.ts
@@ -224,7 +224,7 @@ describe("billing.createCheckoutSession", () => {
    const api = createCaller(makeUser());
    const result = await api.createCheckoutSession({
      priceId: "price_basic",
-      returnUrl: "https://example.com/return",
+      returnUrl: "https://app.kordant.com/return",
    }) as { clientSecret: string; sessionId: string };

    expect(result.clientSecret).toBe("cs_123_secret");
@@ -240,7 +240,7 @@ describe("billing.createCheckoutSession", () => {
    const api = createCaller(makeUser());
    await api.createCheckoutSession({
      priceId: "price_plus",
-      returnUrl: "https://example.com/return",
+      returnUrl: "https://app.kordant.com/return",
    });

    expect(mockChangeSubscriptionTier).toHaveBeenCalledWith("sub_stripe_1", "price_plus");
@@ -257,7 +257,7 @@ describe("billing.createTrialSubscription", () => {

    const api = createCaller(makeUser());
    const result = await api.createTrialSubscription({
-      returnUrl: "https://example.com/return",
+      returnUrl: "https://app.kordant.com/return",
    });

    expect(result.sessionId).toBe("session_trial");
@@ -270,7 +270,7 @@ describe("billing.createTrialSubscription", () => {

    const api = createCaller(makeUser());
    await expect(api.createTrialSubscription({
-      returnUrl: "https://example.com/return",
+      returnUrl: "https://app.kordant.com/return",
    })).rejects.toThrow(TRPCError);
  });
 });
@@ -304,7 +304,7 @@ describe("billing.createPortalSession", () => {

    const api = createCaller(makeUser());
    const result = await api.createPortalSession({
-      returnUrl: "https://example.com/return",
+      returnUrl: "https://app.kordant.com/return",
    });

    expect(result.url).toBe("https://billing.stripe.com/portal/session_456");
@@ -312,7 +312,7 @@ describe("billing.createPortalSession", () => {

  it("throws NOT_FOUND when user has no stripeCustomerId", async () => {
    const api = createCaller(makeUser({ stripeCustomerId: null }));
-    await expect(api.createPortalSession({ returnUrl: "https://example.com/return" })).rejects.toThrow(TRPCError);
+    await expect(api.createPortalSession({ returnUrl: "https://app.kordant.com/return" })).rejects.toThrow(TRPCError);
  });
 });

--- a/web/src/server/api/utils.test.ts
+++ b/web/src/server/api/utils.test.ts
@@ -1,85 +1,273 @@
 import { describe, it, expect } from "vitest";

 /**
- * Mirrors the SENSITIVE_PROCEDURES Set from utils.ts
+ * Mirrors the PROCEDURE_TIERS mapping from utils.ts
+ *
+ * Categories:
+ *   sensitive  (3/hr) — auth operations
+ *   expensive  (5/hr) — external API calls / ML inference
+ *   memory    (10/hr) — memory-heavy ML processing
 */
-const SENSITIVE_PROCEDURES = new Set([
-  "user.login",
-  "user.signup",
-  "user.forgotPassword",
-  "user.resetPassword",
-  "darkwatch.runScan",
-  "darkwatch.runFullScan",
-  "voiceprint.analyzeAudio",
-  "voiceprint.createEnrollment",
-]);
+const PROCEDURE_TIERS: Record<string, string> = {
+  // Auth operations — 3/hr
+  "user.login": "sensitive",
+  "user.signup": "sensitive",
+  "user.forgotPassword": "sensitive",
+  "user.resetPassword": "sensitive",

-function getRateLimitTier(path: string, userRole: string | null, hasUser: boolean): "sensitive" | "authenticated" | "public" | "admin" {
+  // Darkwatch — 5/hr (expensive external API calls: HIBP, SecurityTrails, Censys, Shodan)
+  "darkwatch.runScan": "expensive",
+  "darkwatch.runFullScan": "expensive",
+
+  // VoicePrint — 10/hr (ML analysis, 300MB+ memory per request)
+  "voiceprint.analyzeAudio": "memory",
+  "voiceprint.analyzeCallRecording": "memory",
+  "voiceprint.createEnrollment": "memory",
+  "voiceprint.enrollAdditionalSample": "memory",
+
+  // SpamShield — 5/hr (ML model inference)
+  "spamshield.classifySMS": "expensive",
+  "spamshield.classifyCall": "expensive",
+
+  // HomeTitle — 5/hr (county website scraping)
+  "hometitle.runScan": "expensive",
+
+  // RemoveBrokers — 5/hr (broker website scraping)
+  "removebrokers.scanForListings": "expensive",
+};
+
+function getRateLimitTier(
+  path: string,
+  userRole: string | null,
+  hasUser: boolean,
+): string {
  if (userRole === "admin") return "admin";
-  if (SENSITIVE_PROCEDURES.has(path)) return "sensitive";
-  return hasUser ? "authenticated" : "public";
+  return PROCEDURE_TIERS[path] ?? (hasUser ? "authenticated" : "public");
 }

-describe("Rate limiter exact matching", () => {
-  describe("sensitive procedures", () => {
-    it("matches auth procedures", () => {
+describe("Rate limiter tiered exact matching", () => {
+  // -----------------------------------------------------------------------
+  // sensitive tier (3/hr) — auth operations
+  // -----------------------------------------------------------------------
+  describe("sensitive tier — auth operations (3/hr)", () => {
+    it("matches user.login", () => {
      expect(getRateLimitTier("user.login", null, true)).toBe("sensitive");
+    });
+
+    it("matches user.signup", () => {
      expect(getRateLimitTier("user.signup", null, true)).toBe("sensitive");
+    });
+
+    it("matches user.forgotPassword", () => {
      expect(getRateLimitTier("user.forgotPassword", null, true)).toBe("sensitive");
+    });
+
+    it("matches user.resetPassword", () => {
      expect(getRateLimitTier("user.resetPassword", null, true)).toBe("sensitive");
    });
-
-    it("matches darkwatch procedures", () => {
-      expect(getRateLimitTier("darkwatch.runScan", null, true)).toBe("sensitive");
-      expect(getRateLimitTier("darkwatch.runFullScan", null, true)).toBe("sensitive");
-    });
-
-    it("matches voiceprint procedures", () => {
-      expect(getRateLimitTier("voiceprint.analyzeAudio", null, true)).toBe("sensitive");
-      expect(getRateLimitTier("voiceprint.createEnrollment", null, true)).toBe("sensitive");
-    });
  });

-  describe("non-sensitive procedures", () => {
-    it("returns authenticated tier for normal procedures", () => {
+  // -----------------------------------------------------------------------
+  // expensive tier (5/hr) — external API calls / ML inference
+  // -----------------------------------------------------------------------
+  describe("expensive tier — external API operations (5/hr)", () => {
+    it("matches darkwatch.runScan", () => {
+      expect(getRateLimitTier("darkwatch.runScan", null, true)).toBe("expensive");
+    });
+
+    it("matches darkwatch.runFullScan", () => {
+      expect(getRateLimitTier("darkwatch.runFullScan", null, true)).toBe("expensive");
+    });
+
+    it("matches spamshield.classifySMS", () => {
+      expect(getRateLimitTier("spamshield.classifySMS", null, true)).toBe("expensive");
+    });
+
+    it("matches spamshield.classifyCall", () => {
+      expect(getRateLimitTier("spamshield.classifyCall", null, true)).toBe("expensive");
+    });
+
+    it("matches hometitle.runScan", () => {
+      expect(getRateLimitTier("hometitle.runScan", null, true)).toBe("expensive");
+    });
+
+    it("matches removebrokers.scanForListings", () => {
+      expect(getRateLimitTier("removebrokers.scanForListings", null, true)).toBe("expensive");
+    });
+  });
+
+  // -----------------------------------------------------------------------
+  // memory tier (10/hr) — memory-heavy ML processing
+  // -----------------------------------------------------------------------
+  describe("memory tier — ML analysis (10/hr)", () => {
+    it("matches voiceprint.analyzeAudio", () => {
+      expect(getRateLimitTier("voiceprint.analyzeAudio", null, true)).toBe("memory");
+    });
+
+    it("matches voiceprint.analyzeCallRecording", () => {
+      expect(getRateLimitTier("voiceprint.analyzeCallRecording", null, true)).toBe("memory");
+    });
+
+    it("matches voiceprint.createEnrollment", () => {
+      expect(getRateLimitTier("voiceprint.createEnrollment", null, true)).toBe("memory");
+    });
+
+    it("matches voiceprint.enrollAdditionalSample", () => {
+      expect(getRateLimitTier("voiceprint.enrollAdditionalSample", null, true)).toBe("memory");
+    });
+  });
+
+  // -----------------------------------------------------------------------
+  // Non-sensitive procedures — default tiers
+  // -----------------------------------------------------------------------
+  describe("default tier fallback", () => {
+    it("returns authenticated tier for normal procedures when user is logged in", () => {
      expect(getRateLimitTier("blog.bySlug", null, true)).toBe("authenticated");
      expect(getRateLimitTier("correlation.search", null, true)).toBe("authenticated");
-      expect(getRateLimitTier("spamshield.analyze", null, true)).toBe("authenticated");
+      expect(getRateLimitTier("spamshield.getRules", null, true)).toBe("authenticated");
+      expect(getRateLimitTier("billing.getInvoices", null, true)).toBe("authenticated");
    });

    it("returns public tier for unauthenticated users", () => {
      expect(getRateLimitTier("blog.bySlug", null, false)).toBe("public");
+      expect(getRateLimitTier("spamshield.modelInfo", null, false)).toBe("public");
    });

    it("returns admin tier for admin users regardless of procedure", () => {
      expect(getRateLimitTier("user.login", "admin", true)).toBe("admin");
      expect(getRateLimitTier("darkwatch.runScan", "admin", true)).toBe("admin");
      expect(getRateLimitTier("voiceprint.analyzeAudio", "admin", true)).toBe("admin");
+      expect(getRateLimitTier("darkwatch.nonexistent", "admin", true)).toBe("admin");
    });
  });

+  // -----------------------------------------------------------------------
+  // Substring bypass prevention
+  // -----------------------------------------------------------------------
  describe("substring bypass prevention", () => {
-    it("does not match substring attacks on auth procedures", () => {
-      // These should NOT be sensitive (substring match would incorrectly flag them)
-      expect(getRateLimitTier("user.loginLike", null, true)).toBe("authenticated");
-      expect(getRateLimitTier("user.signupPage", null, true)).toBe("authenticated");
-      expect(getRateLimitTier("user.loginResetPassword", null, true)).toBe("authenticated");
+    describe("auth procedures", () => {
+      it("does not match login variant suffixes", () => {
+        expect(getRateLimitTier("user.loginLike", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("user.loginPage", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("user.logins", null, true)).toBe("authenticated");
+      });
+
+      it("does not match signup variant suffixes", () => {
+        expect(getRateLimitTier("user.signupPage", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("user.signups", null, true)).toBe("authenticated");
+      });
+
+      it("does not match concatenated procedure names", () => {
+        expect(getRateLimitTier("user.loginResetPassword", null, true)).toBe("authenticated");
+      });
    });

-    it("does not match substring attacks on darkwatch", () => {
-      expect(getRateLimitTier("darkwatch.runScanLike", null, true)).toBe("authenticated");
-      expect(getRateLimitTier("darkwatch.runScanHistory", null, true)).toBe("authenticated");
+    describe("darkwatch procedures", () => {
+      it("does not match suffix attacks", () => {
+        expect(getRateLimitTier("darkwatch.runScanLike", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("darkwatch.runScanHistory", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("darkwatch.runScanner", null, true)).toBe("authenticated");
+      });
+
+      it("does not match prefix attacks", () => {
+        expect(getRateLimitTier("notdarkwatch.runScan", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("predarkwatch.runScan", null, true)).toBe("authenticated");
+      });
+
+      it("does not match different method on same namespace", () => {
+        expect(getRateLimitTier("darkwatch.notrunScan", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("darkwatch.getScanStatus", null, true)).toBe("authenticated");
+      });
    });

-    it("does not match substring attacks on voiceprint", () => {
-      expect(getRateLimitTier("voiceprint.analyzeAudioPlayer", null, true)).toBe("authenticated");
-      expect(getRateLimitTier("voiceprint.createEnrollmentPage", null, true)).toBe("authenticated");
+    describe("voiceprint procedures", () => {
+      it("does not match suffix attacks", () => {
+        expect(getRateLimitTier("voiceprint.analyzeAudioPlayer", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("voiceprint.analyzeAudioFile", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("voiceprint.createEnrollmentPage", null, true)).toBe("authenticated");
+      });
+
+      it("does not match partial path segments", () => {
+        expect(getRateLimitTier("voiceprint.analyze", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("voiceprint.create", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("voiceprint.enroll", null, true)).toBe("authenticated");
+      });
    });

-    it("does not match partial path segments", () => {
-      expect(getRateLimitTier("notdarkwatch.runScan", null, true)).toBe("authenticated");
-      expect(getRateLimitTier("darkwatch.notrunScan", null, true)).toBe("authenticated");
-      expect(getRateLimitTier("voiceprint.analyze", null, true)).toBe("authenticated");
+    describe("spamshield procedures", () => {
+      it("does not match suffix attacks", () => {
+        expect(getRateLimitTier("spamshield.classifySMSSpam", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("spamshield.classifyCallLog", null, true)).toBe("authenticated");
+      });
+
+      it("does not match different method on same namespace", () => {
+        expect(getRateLimitTier("spamshield.getRules", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("spamshield.createRule", null, true)).toBe("authenticated");
+      });
+    });
+
+    describe("hometitle procedures", () => {
+      it("does not match suffix attacks", () => {
+        expect(getRateLimitTier("hometitle.runScanNow", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("hometitle.runScanner", null, true)).toBe("authenticated");
+      });
+
+      it("does not match different method on same namespace", () => {
+        expect(getRateLimitTier("hometitle.getProperties", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("hometitle.addProperty", null, true)).toBe("authenticated");
+      });
+    });
+
+    describe("removebrokers procedures", () => {
+      it("does not match suffix attacks", () => {
+        expect(getRateLimitTier("removebrokers.scanForListingsNow", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("removebrokers.scanForListingsBatch", null, true)).toBe("authenticated");
+      });
+
+      it("does not match different method on same namespace", () => {
+        expect(getRateLimitTier("removebrokers.getBrokerRegistry", null, true)).toBe("authenticated");
+        expect(getRateLimitTier("removebrokers.createRemovalRequest", null, true)).toBe("authenticated");
+      });
+    });
+  });
+
+  // -----------------------------------------------------------------------
+  // Edge cases
+  // -----------------------------------------------------------------------
+  describe("edge cases", () => {
+    it("handles empty path gracefully", () => {
+      expect(getRateLimitTier("", null, true)).toBe("authenticated");
+    });
+
+    it("handles unknown paths gracefully", () => {
+      expect(getRateLimitTier("completely.unknown.procedure", null, true)).toBe("authenticated");
+    });
+
+    it("handles paths with dots in unexpected places", () => {
+      expect(getRateLimitTier(".darkwatch.runScan", null, true)).toBe("authenticated");
+      expect(getRateLimitTier("darkwatch..runScan", null, true)).toBe("authenticated");
+    });
+  });
+
+  // -----------------------------------------------------------------------
+  // Tier configuration verification
+  // -----------------------------------------------------------------------
+  describe("tier configuration", () => {
+    it("all mapped tiers are valid rate limit tier keys", () => {
+      const validTiers = new Set(["sensitive", "expensive", "memory"]);
+      for (const tier of Object.values(PROCEDURE_TIERS)) {
+        expect(validTiers.has(tier)).toBe(true);
+      }
+    });
+
+    it("every sensitive procedure has a defined tier", () => {
+      // If a procedure is listed, it must have a valid tier entry
+      const allMappedProcedures = Object.keys(PROCEDURE_TIERS);
+      expect(allMappedProcedures.length).toBeGreaterThan(0);
+      for (const proc of allMappedProcedures) {
+        expect(PROCEDURE_TIERS[proc]).toBeDefined();
+        expect(["sensitive", "expensive", "memory"]).toContain(PROCEDURE_TIERS[proc]);
+      }
    });
  });
 });
--- a/web/src/server/api/utils.ts
+++ b/web/src/server/api/utils.ts
@@ -32,23 +32,49 @@ const isAdmin = t.middleware(({ ctx, next }) => {

 export const adminProcedure = t.procedure.use(isAdmin);

+/**
+ * Tiered procedure-to-rate-limit mapping.
+ *
+ * Exact procedure path matching (not substring) to prevent bypass.
+ * Categories:
+ *   sensitive  (3/hr) — auth operations
+ *   expensive  (5/hr) — external API calls / ML inference
+ *   memory    (10/hr) — memory-heavy ML processing
+ */
+const PROCEDURE_TIERS: Record<string, keyof typeof import("~/server/lib/ratelimit").rateLimitTiers> = {
+  // Auth operations — 3/hr
+  "user.login": "sensitive",
+  "user.signup": "sensitive",
+  "user.forgotPassword": "sensitive",
+  "user.resetPassword": "sensitive",
+
+  // Darkwatch — 5/hr (expensive external API calls: HIBP, SecurityTrails, Censys, Shodan)
+  "darkwatch.runScan": "expensive",
+  "darkwatch.runFullScan": "expensive",
+
+  // VoicePrint — 10/hr (ML analysis, 300MB+ memory per request)
+  "voiceprint.analyzeAudio": "memory",
+  "voiceprint.analyzeCallRecording": "memory",
+  "voiceprint.createEnrollment": "memory",
+  "voiceprint.enrollAdditionalSample": "memory",
+
+  // SpamShield — 5/hr (ML model inference)
+  "spamshield.classifySMS": "expensive",
+  "spamshield.classifyCall": "expensive",
+
+  // HomeTitle — 5/hr (county website scraping)
+  "hometitle.runScan": "expensive",
+
+  // RemoveBrokers — 5/hr (broker website scraping)
+  "removebrokers.scanForListings": "expensive",
+};
+
 const isRateLimited = t.middleware(async ({ ctx, next, path }) => {
  const identifier = ctx.user?.id ?? ctx.apiKey ?? "anonymous";
  const tier = ctx.user?.role === "admin" ? "admin" : ctx.user ? "authenticated" : "public";

-  // Sensitive operations get stricter limits (exact match to prevent bypass)
-  const SENSITIVE_PROCEDURES = new Set([
-    "user.login",
-    "user.signup",
-    "user.forgotPassword",
-    "user.resetPassword",
-    "darkwatch.runScan",
-    "darkwatch.runFullScan",
-    "voiceprint.analyzeAudio",
-    "voiceprint.createEnrollment",
-  ]);
-
-  const effectiveTier = SENSITIVE_PROCEDURES.has(path) ? "sensitive" : tier;
+  // Look up procedure-specific tier, falling back to the default for the user
+  const effectiveTier = PROCEDURE_TIERS[path] ?? tier;

  await checkRateLimitOrThrow(identifier, effectiveTier);
  return next();