GRO-2294: Route Optimization security hardening (geocode-batch limit cap + redact settings secret) (#193)

src/__tests__/geocodeBatchLimit.test.ts

@@ -0,0 +1,89 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { Hono } from "hono";
+
+// ─── Mocks ──────────────────────────────────────────────────────────────────
+// GRO-2294: the POST /clients/geocode-batch handler must clamp ?limit to the
+// documented maximum (500) before invoking the geocoding service. We mock the
+// service to capture the exact limit the route forwards.
+
+const geocodeUngeocodedClients = vi.fn(async () => ({
+  totalRemaining: 0,
+  processed: 0,
+  geocoded: 0,
+  failed: 0,
+  remaining: 0,
+}));
+
+vi.mock("../services/clientGeocoding.js", () => ({
+  geocodeUngeocodedClients,
+  geocodeClient: vi.fn(),
+  resolveClientGeocodingProvider: vi.fn(),
+}));
+
+vi.mock("@groombook/db", () => {
+  const tableProxy = (name: string) =>
+    new Proxy(
+      { _name: name },
+      { get: (_t, p) => (p === "_name" ? name : { table: name, column: p }) }
+    );
+  return {
+    getDb: () => ({}),
+    clients: tableProxy("clients"),
+    appointments: tableProxy("appointments"),
+    and: vi.fn(),
+    eq: vi.fn(),
+    or: vi.fn(),
+    exists: vi.fn(),
+  };
+});
+
+const { clientsRouter } = await import("../routes/clients.js");
+
+const app = new Hono();
+app.route("/clients", clientsRouter);
+
+function postBatch(query: string) {
+  return app.request(`/clients/geocode-batch${query}`, { method: "POST" });
+}
+
+describe("POST /clients/geocode-batch — ?limit cap (GRO-2294)", () => {
+  beforeEach(() => {
+    geocodeUngeocodedClients.mockClear();
+  });
+
+  it("defaults to 50 when no ?limit is supplied", async () => {
+    const res = await postBatch("");
+    expect(res.status).toBe(200);
+    expect(geocodeUngeocodedClients).toHaveBeenCalledWith(expect.anything(), 50);
+  });
+
+  it("passes through a value within the cap", async () => {
+    const res = await postBatch("?limit=120");
+    expect(res.status).toBe(200);
+    expect(geocodeUngeocodedClients).toHaveBeenCalledWith(expect.anything(), 120);
+  });
+
+  it("clamps an over-cap value to 500", async () => {
+    const res = await postBatch("?limit=100000");
+    expect(res.status).toBe(200);
+    expect(geocodeUngeocodedClients).toHaveBeenCalledWith(expect.anything(), 500);
+  });
+
+  it("floors a fractional value before clamping", async () => {
+    const res = await postBatch("?limit=49.9");
+    expect(res.status).toBe(200);
+    expect(geocodeUngeocodedClients).toHaveBeenCalledWith(expect.anything(), 49);
+  });
+
+  it("rejects a non-positive limit with 400", async () => {
+    const res = await postBatch("?limit=0");
+    expect(res.status).toBe(400);
+    expect(geocodeUngeocodedClients).not.toHaveBeenCalled();
+  });
+
+  it("rejects a non-numeric limit with 400", async () => {
+    const res = await postBatch("?limit=abc");
+    expect(res.status).toBe(400);
+    expect(geocodeUngeocodedClients).not.toHaveBeenCalled();
+  });
+});

src/__tests__/settings.test.ts

@@ -0,0 +1,91 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { Hono } from "hono";
+
+// ─── Mocks ──────────────────────────────────────────────────────────────────
+// GRO-2294: GET /api/admin/settings must not return the encrypted
+// googleMapsApiKey ciphertext, on either the existing-row or auto-create branch.
+
+let selectRows: Record<string, unknown>[] = [];
+let insertReturning: Record<string, unknown>[] = [];
+
+function makeChainable(data: unknown[]): unknown {
+  const arr = [...data];
+  const chain = new Proxy(arr, {
+    get(target, prop) {
+      if (prop === "where" || prop === "orderBy" || prop === "limit") {
+        return () => chain;
+      }
+      // @ts-expect-error proxy passthrough
+      return target[prop];
+    },
+  });
+  return chain;
+}
+
+vi.mock("@groombook/db", () => {
+  const businessSettings = new Proxy(
+    { _name: "business_settings" },
+    { get: (_t, p) => (p === "_name" ? "business_settings" : { column: p }) }
+  );
+  return {
+    getDb: () => ({
+      select: () => ({ from: () => makeChainable(selectRows) }),
+      insert: () => ({
+        values: () => ({ returning: () => insertReturning }),
+      }),
+    }),
+    businessSettings,
+    eq: vi.fn(),
+  };
+});
+
+vi.mock("../lib/s3.js", () => ({
+  getPresignedUploadUrl: vi.fn(),
+  deleteObject: vi.fn(),
+  putObject: vi.fn(),
+  getObject: vi.fn(),
+}));
+
+const { settingsRouter } = await import("../routes/settings.js");
+
+const app = new Hono();
+app.route("/settings", settingsRouter);
+
+const FULL_ROW = {
+  id: "settings-uuid-1",
+  businessName: "GroomBook",
+  primaryColor: "#4f8a6f",
+  accentColor: "#8b7355",
+  routeOptimizationProvider: "google",
+  googleMapsApiKey: "ENCRYPTED::super-secret-ciphertext",
+  createdAt: new Date(),
+  updatedAt: new Date(),
+};
+
+describe("GET /settings — googleMapsApiKey redaction (GRO-2294)", () => {
+  beforeEach(() => {
+    selectRows = [];
+    insertReturning = [];
+  });
+
+  it("omits googleMapsApiKey from an existing settings row", async () => {
+    selectRows = [{ ...FULL_ROW }];
+    const res = await app.request("/settings", { method: "GET" });
+    expect(res.status).toBe(200);
+    const body = (await res.json()) as Record<string, unknown>;
+    expect(body).not.toHaveProperty("googleMapsApiKey");
+    // Non-secret fields are still returned.
+    expect(body.businessName).toBe("GroomBook");
+    expect(body.routeOptimizationProvider).toBe("google");
+  });
+
+  it("omits googleMapsApiKey from the auto-create branch", async () => {
+    selectRows = [];
+    insertReturning = [{ ...FULL_ROW, id: "settings-uuid-new" }];
+    const res = await app.request("/settings", { method: "GET" });
+    expect(res.status).toBe(200);
+    const body = (await res.json()) as Record<string, unknown>;
+    expect(body).not.toHaveProperty("googleMapsApiKey");
+    expect(body.id).toBe("settings-uuid-new");
+  });
+});