fix(providers): reject silent validation degradation with 400 and rejected keys

changelog.d/fixes/11101-reject-silent-validation.md

@@ -0,0 +1 @@
+- **fix(providers):** Reject silent validation degradation on provider connection patch — unknown `rateLimitOverrides` keys (e.g. a typo'd `tpm`) and empty/non-numeric values now return `400` with the rejected key list instead of being silently dropped ([#11101](https://github.com/diegosouzapw/OmniRoute/pull/11101))

src/app/api/providers/[id]/route.ts

@@ -121,7 +121,17 @@ export async function PUT(request: Request, { params }: { params: Promise<{ id:
     const { id } = await params;
     const validation = validateBody(updateProviderConnectionSchema, rawBody);
     if (isValidationFailure(validation)) {
-      return NextResponse.json({ error: validation.error }, { status: 400 });
+      // never drop an operator's intent silently. Surface the rejected
+      // keys (field paths and unrecognized-key names) alongside the existing
+      // error envelope so clients and the UI can tell exactly what was refused.
+      const rejected = [
+        ...validation.error.details.map((d) => d.field).filter(Boolean),
+        ...validation.error.details.flatMap((d) => d.keys ?? []),
+      ];
+      return NextResponse.json(
+        { error: { ...validation.error, rejected } },
+        { status: 400 }
+      );
     }
     const body = validation.data;
     const {

src/lib/db/providers.ts

@@ -627,15 +627,26 @@ export async function createProviderConnection(data: JsonRecord) {
   // to no-overrides) keeps the field present on the returned object so the
   // UI can tell "field was read, no overrides" apart from "field absent."
   if ("quotaWindowThresholds" in connection) {
-    connection.quotaWindowThresholds = sanitizeQuotaWindowThresholds(
-      connection.quotaWindowThresholds
-    );
+    const result = sanitizeQuotaWindowThresholds(connection.quotaWindowThresholds);
+    if (result.rejected.length > 0) {
+      throw new Error(
+        `Refusing to persist quotaWindowThresholds with rejected keys: ${result.rejected.join(", ")}`
+      );
+    }
+    connection.quotaWindowThresholds = result.sanitized;
   }
 
   // Same sanitization for rateLimitOverrides — keep in-memory representation
-  // in sync with what gets persisted.
+  // in sync with what gets persisted. Reject (don't silently drop) invalid
+  // keys/values so a direct DB writer can't lose operator intent.
   if ("rateLimitOverrides" in connection) {
-    connection.rateLimitOverrides = sanitizeRateLimitOverrides(connection.rateLimitOverrides);
+    const result = sanitizeRateLimitOverrides(connection.rateLimitOverrides);
+    if (result.rejected.length > 0) {
+      throw new Error(
+        `Refusing to persist rateLimitOverrides with rejected keys: ${result.rejected.join(", ")}`
+      );
+    }
+    connection.rateLimitOverrides = result.sanitized;
   }
 
   _insertConnectionRow(db, encryptConnectionFields({ ...connection }));
@@ -849,13 +860,24 @@ export async function updateProviderConnection(id: string, data: JsonRecord) {
   // Mirror the sanitization the create path applies — keep the returned
   // object in lockstep with what we persist.
   if ("quotaWindowThresholds" in merged) {
-    const sanitized = sanitizeQuotaWindowThresholds(merged.quotaWindowThresholds);
+    const result = sanitizeQuotaWindowThresholds(merged.quotaWindowThresholds);
+    if (result.rejected.length > 0) {
+      throw new Error(
+        `Refusing to persist quotaWindowThresholds with rejected keys: ${result.rejected.join(", ")}`
+      );
+    }
     // For updates we always carry the key forward (even as null) so the read
-    // path surfaces the cleared state to callers that just patched it.
-    merged.quotaWindowThresholds = sanitized;
+    // path surfaces the cleared state to callers that merged it.
+    merged.quotaWindowThresholds = result.sanitized;
   }
   if ("rateLimitOverrides" in merged) {
-    merged.rateLimitOverrides = sanitizeRateLimitOverrides(merged.rateLimitOverrides);
+    const result = sanitizeRateLimitOverrides(merged.rateLimitOverrides);
+    if (result.rejected.length > 0) {
+      throw new Error(
+        `Refusing to persist rateLimitOverrides with rejected keys: ${result.rejected.join(", ")}`
+      );
+    }
+    merged.rateLimitOverrides = result.sanitized;
   }
   const existingRecord = toRecord(existing);
 

src/lib/db/providers/columns.ts

@@ -64,20 +64,37 @@ export function normalizeBooleanColumn(value: unknown, fallback: boolean): boole
   return fallback;
 }
 
+// Result of sanitizing a per-connection overrides/threshold map. `sanitized`
+// is the cleaned value (or null when it collapses to nothing); `rejected`
+// lists every key that was refused so callers can fail loudly
+// instead of silently dropping the operator's input.
+export type SanitizeResult = {
+  sanitized: Record<string, number> | null;
+  rejected: string[];
+};
+
 // Sanitize the per-connection rate limit overrides map: keep only known
-// fields with valid numeric values. Called once at each write-path boundary.
-export function sanitizeRateLimitOverrides(value: unknown): Record<string, number> | null {
-  if (value === null || value === undefined) return null;
-  if (typeof value !== "object" || Array.isArray(value)) return null;
+// fields with valid non-negative integer values. Called once at each
+// write-path boundary. Unknown keys and invalid values go into `rejected`
+// rather than being dropped in silence.
+export function sanitizeRateLimitOverrides(value: unknown): SanitizeResult {
+  if (value === null || value === undefined) return { sanitized: null, rejected: [] };
+  if (typeof value !== "object" || Array.isArray(value)) return { sanitized: null, rejected: [] };
   const allowedKeys = new Set(["rpm", "tpm", "tpd", "minTime", "maxConcurrent"]);
+  const rejected: string[] = [];
   const map: Record<string, number> = {};
   for (const [key, v] of Object.entries(value as Record<string, unknown>)) {
-    if (!allowedKeys.has(key)) continue;
+    if (!allowedKeys.has(key)) {
+      rejected.push(key);
+      continue;
+    }
     if (typeof v === "number" && Number.isInteger(v) && v >= 0) {
       map[key] = v;
+    } else {
+      rejected.push(key);
     }
   }
-  return Object.keys(map).length === 0 ? null : map;
+  return { sanitized: Object.keys(map).length === 0 ? null : map, rejected };
 }
 
 // Serialize an already-sanitized map for SQLite TEXT storage.
@@ -91,20 +108,29 @@ export function toRecord(value: unknown): JsonRecord {
   return value && typeof value === "object" ? (value as JsonRecord) : {};
 }
 
-// Sanitize the per-window threshold map: keep only 0-100 integer values.
-// Called once at each write-path boundary (createProviderConnection +
-// updateProviderConnection) so both the in-memory return and the persisted
-// row share the same shape. Serialization below trusts this output.
-export function sanitizeQuotaWindowThresholds(value: unknown): Record<string, number> | null {
-  if (value === null || value === undefined) return null;
-  if (typeof value !== "object" || Array.isArray(value)) return null;
+// Sanitize the per-window threshold map: keep only 0-100 integer values with
+// keys no longer than 64 chars. Called once at each write-path boundary
+// (createProviderConnection + updateProviderConnection) so both the in-memory
+// return and the persisted row share the same shape. Serialization below
+// trusts this output. Invalid keys/values go into `rejected` rather than being
+// dropped in silence.
+export function sanitizeQuotaWindowThresholds(value: unknown): SanitizeResult {
+  if (value === null || value === undefined) return { sanitized: null, rejected: [] };
+  if (typeof value !== "object" || Array.isArray(value)) return { sanitized: null, rejected: [] };
+  const rejected: string[] = [];
   const map: Record<string, number> = {};
   for (const [key, v] of Object.entries(value as Record<string, unknown>)) {
+    if (key.length > 64) {
+      rejected.push(key);
+      continue;
+    }
     if (typeof v === "number" && Number.isInteger(v) && v >= 0 && v <= 100) {
       map[key] = v;
+    } else {
+      rejected.push(key);
     }
   }
-  return Object.keys(map).length === 0 ? null : map;
+  return { sanitized: Object.keys(map).length === 0 ? null : map, rejected };
 }
 
 export function toStringOrNull(value: unknown): string | null {

src/shared/validation/helpers.ts

@@ -4,6 +4,10 @@ import { z } from "zod";
 type ValidationErrorDetail = {
   field: string;
   message: string;
+  // Present for `unrecognized_keys` issues: the unknown key names that were
+  // refused (e.g. a typo'd override key). Surfaced by callers so clients can
+  // tell exactly which keys were rejected.
+  keys?: string[];
 };
 
 type ValidationErrorPayload = {
@@ -45,6 +49,9 @@ export function validateBody<TSchema extends z.ZodTypeAny>(
       details: issues.map((e) => ({
         field: e.path.join("."),
         message: e.message,
+        ...(("keys" in e && (e as { keys?: string[] }).keys)
+          ? { keys: (e as { keys: string[] }).keys }
+          : {}),
       })),
     },
   };

src/shared/validation/schemas/provider.ts

@@ -420,6 +420,25 @@ export const providerNodeValidateSchema = z.object({
   modelId: z.string().trim().max(200).optional().or(z.literal("")),
 });
 
+// rate-limit override numeric fields must reject operator intent loss.
+// `z.coerce.number()` silently turns "" into 0 and "60abc" into NaN, which
+// would drop or distort the value instead of rejecting it. Preprocess first so
+// an empty/non-numeric string fails validation (surfaced as a 400), while still
+// coercing legit numeric strings like "60".
+function rateLimitOverrideNumber(max: number) {
+  return z.preprocess(
+    (raw) => {
+      if (typeof raw === "string") {
+        if (raw.trim() === "") return NaN;
+        const parsed = Number(raw);
+        return Number.isNaN(parsed) ? raw : parsed;
+      }
+      return raw;
+    },
+    z.coerce.number().int().min(0).max(max)
+  );
+}
+
 export const updateProviderConnectionSchema = z
   .object({
     name: z.string().max(200).optional(),
@@ -468,17 +487,24 @@ export const updateProviderConnectionSchema = z
     projectId: z.union([z.string(), z.null()]).optional(),
     // Per-connection rate limit overrides — overrides the global RequestQueueSettings
     // for this connection. Set to null to clear all overrides.
+    // Per-connection rate limit overrides — overrides the global
+    // RequestQueueSettings for this connection. Set to null to clear all
+    // overrides. `.strict()` rejects unknown keys (e.g. a typo'd `tmp`) with a
+    // 400 instead of silently stripping them: the operator's intent is
+    // never dropped without an error. `.nullable()` (rather than a
+    // `z.union([z.null(), …])`) keeps the `unrecognized_keys` issue at the top
+    // level so the rejected key name survives into the 400 response.
     rateLimitOverrides: z
-      .union([
-        z.null(),
-        z.object({
-          rpm: z.coerce.number().int().min(0).max(1_000_000).optional(),
-          tpm: z.coerce.number().int().min(0).max(100_000_000).optional(),
-          tpd: z.coerce.number().int().min(0).max(10_000_000_000).optional(),
-          minTime: z.coerce.number().int().min(0).max(60_000).optional(),
-          maxConcurrent: z.coerce.number().int().min(0).max(10_000).optional(),
-        }),
-      ])
+      .object({
+        rpm: rateLimitOverrideNumber(1_000_000).optional(),
+        tpm: rateLimitOverrideNumber(100_000_000).optional(),
+        tpd: rateLimitOverrideNumber(10_000_000_000).optional(),
+        minTime: rateLimitOverrideNumber(60_000).optional(),
+        maxConcurrent: rateLimitOverrideNumber(10_000).optional(),
+      })
+      .partial()
+      .strict()
+      .nullable()
       .optional(),
     proxyEnabled: z.boolean().optional(),
     perKeyProxyEnabled: z.boolean().optional(),

tests/unit/columns-validation.test.ts

@@ -0,0 +1,24 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import {
+  sanitizeRateLimitOverrides,
+  sanitizeQuotaWindowThresholds,
+} from "@/lib/db/providers/columns";
+
+test("sanitizeRateLimitOverrides surfaces rejected keys (blocking, not silent)", () => {
+  const r = sanitizeRateLimitOverrides({ rpm: 10, foo: 1, tpm: -1 });
+  assert.deepEqual(r.sanitized, { rpm: 10 });
+  assert.deepEqual(r.rejected.sort(), ["foo", "tpm"]);
+});
+
+test("sanitizeQuotaWindowThresholds surfaces key-too-long and out-of-range", () => {
+  const r = sanitizeQuotaWindowThresholds({ ["a".repeat(65)]: 50, win: 101 });
+  assert.ok(r.rejected.length >= 1);
+  assert.ok(r.rejected.includes("win"));
+});
+
+test("valid input yields no rejected keys", () => {
+  const r = sanitizeRateLimitOverrides({ rpm: 10, tpm: 20 });
+  assert.deepEqual(r.rejected, []);
+  assert.deepEqual(r.sanitized, { rpm: 10, tpm: 20 });
+});

tests/unit/db-providers-split.test.ts

@@ -52,26 +52,38 @@ describe("providers/columns — normalizeBooleanColumn", () => {
 });
 
 describe("providers/columns — sanitizeRateLimitOverrides", () => {
-  it("returns null for nullish / non-object / array input", () => {
-    assert.equal(sanitizeRateLimitOverrides(null), null);
-    assert.equal(sanitizeRateLimitOverrides(undefined), null);
-    assert.equal(sanitizeRateLimitOverrides("x"), null);
-    assert.equal(sanitizeRateLimitOverrides([1, 2]), null);
+  it("returns {sanitized:null,rejected:[]} for nullish / non-object / array input", () => {
+    assert.deepEqual(sanitizeRateLimitOverrides(null), { sanitized: null, rejected: [] });
+    assert.deepEqual(sanitizeRateLimitOverrides(undefined), { sanitized: null, rejected: [] });
+    assert.deepEqual(sanitizeRateLimitOverrides("x"), { sanitized: null, rejected: [] });
+    assert.deepEqual(sanitizeRateLimitOverrides([1, 2]), { sanitized: null, rejected: [] });
   });
-  it("keeps only allowed keys with non-negative integers", () => {
-    assert.deepEqual(sanitizeRateLimitOverrides({ rpm: 10, bogus: 5, tpm: -1 }), { rpm: 10 });
+  it("keeps only allowed keys with non-negative integers, reports the rest as rejected", () => {
+    assert.deepEqual(sanitizeRateLimitOverrides({ rpm: 10, bogus: 5, tpm: -1 }), {
+      sanitized: { rpm: 10 },
+      rejected: ["bogus", "tpm"],
+    });
   });
-  it("returns null when nothing valid remains", () => {
-    assert.equal(sanitizeRateLimitOverrides({ rpm: 1.5, nope: 3 }), null);
+  it("returns {sanitized:null} when nothing valid remains, with rejected keys", () => {
+    assert.deepEqual(sanitizeRateLimitOverrides({ rpm: 1.5, nope: 3 }), {
+      sanitized: null,
+      rejected: ["rpm", "nope"],
+    });
   });
 });
 
 describe("providers/columns — sanitizeQuotaWindowThresholds", () => {
-  it("keeps only 0-100 integers", () => {
-    assert.deepEqual(sanitizeQuotaWindowThresholds({ a: 50, b: 120, c: 0 }), { a: 50, c: 0 });
+  it("keeps only 0-100 integers, reports the rest as rejected", () => {
+    assert.deepEqual(sanitizeQuotaWindowThresholds({ a: 50, b: 120, c: 0 }), {
+      sanitized: { a: 50, c: 0 },
+      rejected: ["b"],
+    });
   });
-  it("returns null when empty", () => {
-    assert.equal(sanitizeQuotaWindowThresholds({ a: 200 }), null);
+  it("returns {sanitized:null} when nothing valid remains, with rejected keys", () => {
+    assert.deepEqual(sanitizeQuotaWindowThresholds({ a: 200 }), {
+      sanitized: null,
+      rejected: ["a"],
+    });
   });
 });
 

tests/unit/provider-connections-quota-threshold.test.ts

@@ -106,18 +106,36 @@ test("updateProviderConnection with explicit null clears the column entirely", a
   assert.ok(reread.quotaWindowThresholds === null || reread.quotaWindowThresholds === undefined);
 });
 
-test("DB serializer drops out-of-range values silently", async () => {
-  // The DB module sanitizes the map on the way in; values outside 0-100 or
-  // non-integers are pruned. This is a defense in depth — the Zod schema
-  // already rejects them at the API boundary, but the DB shouldn't trust.
-  const created = await providersDb.createProviderConnection({
-    provider: "codex",
-    authType: "apikey",
-    name: "Codex Sanitize",
-    apiKey: "sk-san",
-    quotaWindowThresholds: { window5h: 95, bogus: 999, fractional: 1.5 },
-  });
-  assert.deepEqual(created.quotaWindowThresholds, { window5h: 95 });
+test("DB serializer refuses out-of-range / invalid values instead of dropping silently", async () => {
+  // the DB module must refuse the write (throw) rather than
+  // silently prune invalid keys/values on the way in, so operator intent is
+  // never lost without an error. The Zod schema already rejects at the API
+  // boundary; this is defense in depth for direct DB writers (seed/scripts).
+  await assert.rejects(
+    () =>
+      providersDb.createProviderConnection({
+        provider: "codex",
+        authType: "apikey",
+        name: "Codex Sanitize",
+        apiKey: "sk-san",
+        quotaWindowThresholds: { window5h: 95, bogus: 999, fractional: 1.5 },
+      }),
+    /rejected keys/
+  );
+});
+
+test("DB serializer refuses unknown rate-limit override keys instead of dropping silently", async () => {
+  await assert.rejects(
+    () =>
+      providersDb.createProviderConnection({
+        provider: "codex",
+        authType: "apikey",
+        name: "Codex Sanitize RLO",
+        apiKey: "sk-san-2",
+        rateLimitOverrides: { rpm: 10, bogus: 999, tpm: -1 },
+      }),
+    /rejected keys/
+  );
 });
 
 test("updateProviderConnectionSchema accepts a valid window map", () => {