diff --git a/changelog.d/fixes/11101-reject-silent-validation.md b/changelog.d/fixes/11101-reject-silent-validation.md new file mode 100644 index 0000000000..04b2a67d5a --- /dev/null +++ b/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)) diff --git a/src/app/api/providers/[id]/route.ts b/src/app/api/providers/[id]/route.ts index 562dad0744..4f588571b8 100644 --- a/src/app/api/providers/[id]/route.ts +++ b/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 { diff --git a/src/lib/db/providers.ts b/src/lib/db/providers.ts index 3b02933e11..fbab6817fd 100644 --- a/src/lib/db/providers.ts +++ b/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); diff --git a/src/lib/db/providers/columns.ts b/src/lib/db/providers/columns.ts index b32f653f7c..f06f948cd2 100644 --- a/src/lib/db/providers/columns.ts +++ b/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 | 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 | 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 = {}; for (const [key, v] of Object.entries(value as Record)) { - 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 | 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 = {}; for (const [key, v] of Object.entries(value as Record)) { + 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 { diff --git a/src/shared/validation/helpers.ts b/src/shared/validation/helpers.ts index 4e486c7287..c811d6d4bf 100644 --- a/src/shared/validation/helpers.ts +++ b/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( 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 } + : {}), })), }, }; diff --git a/src/shared/validation/schemas/provider.ts b/src/shared/validation/schemas/provider.ts index b3272f70fb..4cfe7afff4 100644 --- a/src/shared/validation/schemas/provider.ts +++ b/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(), diff --git a/tests/unit/columns-validation.test.ts b/tests/unit/columns-validation.test.ts new file mode 100644 index 0000000000..41f70cddeb --- /dev/null +++ b/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 }); +}); diff --git a/tests/unit/db-providers-split.test.ts b/tests/unit/db-providers-split.test.ts index 220544ba2d..284a5193c8 100644 --- a/tests/unit/db-providers-split.test.ts +++ b/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"], + }); }); }); diff --git a/tests/unit/provider-connections-quota-threshold.test.ts b/tests/unit/provider-connections-quota-threshold.test.ts index 5caaeea369..e7812557e9 100644 --- a/tests/unit/provider-connections-quota-threshold.test.ts +++ b/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", () => { diff --git a/tests/unit/providers-patch-400.test.ts b/tests/unit/providers-patch-400.test.ts new file mode 100644 index 0000000000..8ec1cbdf3b --- /dev/null +++ b/tests/unit/providers-patch-400.test.ts @@ -0,0 +1,42 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { updateProviderConnectionSchema } from "@/shared/validation/schemas/provider"; + +test("PATCH rateLimitOverrides {rpm:\"60\"} coerces to a valid number", () => { + const r = updateProviderConnectionSchema.safeParse({ rateLimitOverrides: { rpm: "60" } }); + assert.equal(r.success, true); +}); + +test("unknown key in rateLimitOverrides is rejected (no silent drop)", () => { + const r = updateProviderConnectionSchema.safeParse({ rateLimitOverrides: { rpm: 10, foo: 1 } }); + assert.equal(r.success, false); + const flaggedFoo = r.error!.issues.some( + (i) => i.path.includes("foo") || (i as { keys?: string[] }).keys?.includes("foo") || i.message.includes("foo") + ); + assert.ok( + flaggedFoo, + `expected an issue flagging "foo", got: ${JSON.stringify(r.error!.issues)}` + ); +}); + +test("quotaWindowThresholds key longer than 64 chars is rejected", () => { + const r = updateProviderConnectionSchema.safeParse({ + quotaWindowThresholds: { ["a".repeat(65)]: 50 }, + }); + assert.equal(r.success, false); +}); + +test("empty string rate limit value is rejected (coerce \"\"→0 trap)", () => { + const r = updateProviderConnectionSchema.safeParse({ rateLimitOverrides: { rpm: "" } }); + assert.equal(r.success, false); +}); + +test("non-numeric rate limit value is rejected", () => { + const r = updateProviderConnectionSchema.safeParse({ rateLimitOverrides: { rpm: "60abc" } }); + assert.equal(r.success, false); +}); + +test("quotaWindowThresholds value outside 0-100 is rejected", () => { + const r = updateProviderConnectionSchema.safeParse({ quotaWindowThresholds: { win: 101 } }); + assert.equal(r.success, false); +});