From 4305ef8690e2ccc24c5feab276470f1f52a80ddb Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Wed, 24 Jun 2026 00:04:22 -0300 Subject: [PATCH] =?UTF-8?q?fix(quota):=20policy=20inv=C3=A1lida=20n=C3=A3o?= =?UTF-8?q?=20vaza=20allow=20+=20guard=20connectionIds=20vazio=20[Fase=203?= =?UTF-8?q?=20#10]=20(#4901)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integrated into release/v3.8.36 --- src/lib/db/quotaPools.ts | 16 ++++- src/lib/quota/fairShare.ts | 26 ++++++++- tests/unit/db-quota-pools.test.ts | 59 +++++++++++++++++++ tests/unit/quota-combos-sync.test.ts | 87 ++++++++++++++++++++++++++++ tests/unit/quota-fair-share.test.ts | 46 +++++++++++++++ 5 files changed, 230 insertions(+), 4 deletions(-) diff --git a/src/lib/db/quotaPools.ts b/src/lib/db/quotaPools.ts index 14b9032e3a..93cf86264d 100644 --- a/src/lib/db/quotaPools.ts +++ b/src/lib/db/quotaPools.ts @@ -144,11 +144,25 @@ interface AllocationRow { policy: string; } +const VALID_POLICIES: ReadonlySet = new Set(["hard", "soft", "burst"]); + +/** + * Fail-safe policy normalization at the DB read boundary. A column value outside + * `hard | soft | burst` (corrupted/legacy row, or a value inserted while the + * table CHECK constraint was bypassed) is coerced to the most restrictive + * policy, `hard`, instead of being trusted via a raw `as Policy` cast. This + * prevents an unknown policy from reaching the fair-share engine, where it would + * otherwise be a silent fail-OPEN (issue #10). + */ +function normalizePolicy(value: string): Policy { + return VALID_POLICIES.has(value) ? (value as Policy) : "hard"; +} + function rowToAllocation(row: AllocationRow): PoolAllocation { const alloc: PoolAllocation = { apiKeyId: row.api_key_id, weight: row.weight, - policy: row.policy as Policy, + policy: normalizePolicy(row.policy), }; if (row.cap_value != null) alloc.capValue = row.cap_value; if (row.cap_unit != null) alloc.capUnit = row.cap_unit as QuotaUnit; diff --git a/src/lib/quota/fairShare.ts b/src/lib/quota/fairShare.ts index 4da7fcb3fa..238dea45d0 100644 --- a/src/lib/quota/fairShare.ts +++ b/src/lib/quota/fairShare.ts @@ -61,6 +61,19 @@ function dimensionKeyString(key: FairShareDimension["key"]): string { return `${key.poolId}:${key.unit}:${key.window}`; } +const KNOWN_POLICIES: ReadonlySet = new Set(["hard", "soft", "burst"]); + +/** + * Fail-safe policy normalization. Any value outside the known + * `hard | soft | burst` set (e.g. a corrupted DB row that reached this engine + * through an unchecked `row.policy as Policy` cast) is treated as the most + * restrictive policy, `hard`. This closes a fail-OPEN hole: an unknown policy + * used to fall through every `switch` case and return a silent `allow`. + */ +function normalizePolicy(policy: Policy): Policy { + return KNOWN_POLICIES.has(policy) ? policy : "hard"; +} + // --------------------------------------------------------------------------- // Core algorithm // --------------------------------------------------------------------------- @@ -77,6 +90,10 @@ export function decideFairShare(input: FairShareInput): FairShareDecision { return { kind: "allow", reason: "ok" }; } + // Fail-safe: an unknown/corrupted policy is treated as `hard` (most + // restrictive) so it can never silently bypass fair-share enforcement. + const effectivePolicy = normalizePolicy(allocation.policy); + let anyPenalized = false; for (const dim of dimensions) { @@ -97,7 +114,7 @@ export function decideFairShare(input: FairShareInput): FairShareDecision { // If the pool's global limit is already reached AND this key's request // would exceed it (burst mode without borrow room), block as "global-saturated". if (dim.consumedTotal >= dim.limit) { - if (allocation.policy !== "burst") { + if (effectivePolicy !== "burst") { return { kind: "block", reason: "global-saturated" }; } // burst also blocked when no room at all @@ -108,7 +125,9 @@ export function decideFairShare(input: FairShareInput): FairShareDecision { if (isStrict) { // ── Strict mode ──────────────────────────────────────────────────── - switch (allocation.policy) { + // effectivePolicy is normalized (unknown → hard), so these cases are + // exhaustive and an unknown policy is enforced as hard. + switch (effectivePolicy) { case "hard": // Hard: block once consumed >= fair_share if (consumed >= fairShare) { @@ -131,7 +150,8 @@ export function decideFairShare(input: FairShareInput): FairShareDecision { } else { // ── Generous mode ────────────────────────────────────────────────── // There is slack — allow borrowing up to the global limit. - switch (allocation.policy) { + // effectivePolicy is normalized (unknown → hard). + switch (effectivePolicy) { case "hard": // Hard in generous mode: allow if global limit not reached AND // the key is within global limit (which we know because diff --git a/tests/unit/db-quota-pools.test.ts b/tests/unit/db-quota-pools.test.ts index 12575f2e24..670ab67f8f 100644 --- a/tests/unit/db-quota-pools.test.ts +++ b/tests/unit/db-quota-pools.test.ts @@ -20,6 +20,7 @@ process.env.DATA_DIR = TEST_DATA_DIR; const core = await import("../../src/lib/db/core.ts"); const poolsDb = await import("../../src/lib/db/quotaPools.ts"); +const { getDbInstance } = core; async function resetStorage() { core.resetDbInstance(); @@ -260,3 +261,61 @@ test("allocation stores optional capValue and capUnit correctly", () => { assert.equal(alloc.capValue, 1000); assert.equal(alloc.capUnit, "requests"); }); + +// --------------------------------------------------------------------------- +// Guard A (issue #10): corrupted/unknown policy in the DB must be normalized to +// the most restrictive policy ('hard') at the read boundary — never trusted via +// `row.policy as Policy`. A garbage policy reaching the fair-share engine would +// fall through every switch case and silently ALLOW (fail-OPEN). +// +// The schema has CHECK (policy IN ('hard','soft','burst')), so we bypass it with +// PRAGMA ignore_check_constraints to simulate a legacy/corrupted row. +// --------------------------------------------------------------------------- + +test("rowToAllocation normalizes an unknown DB policy to 'hard' (Guard A)", () => { + const pool = poolsDb.createPool({ + connectionId: "c-guardA", + name: "Corrupt Policy Pool", + allocations: [{ apiKeyId: "k-corrupt", weight: 100, policy: "soft" }], + }); + + // Inject a corrupted policy directly, bypassing the CHECK constraint. + const db = getDbInstance() as unknown as { + pragma: (s: string) => unknown; + prepare: (sql: string) => { run: (...p: unknown[]) => unknown }; + }; + db.pragma("ignore_check_constraints = ON"); + db.prepare("UPDATE quota_allocations SET policy = ? WHERE pool_id = ? AND api_key_id = ?").run( + "bogus-policy", + pool.id, + "k-corrupt" + ); + db.pragma("ignore_check_constraints = OFF"); + + // Read through the domain module — the unknown policy must become 'hard'. + const found = poolsDb.getPool(pool.id)!; + const alloc = found.allocations.find((a) => a.apiKeyId === "k-corrupt")!; + assert.equal(alloc.policy, "hard", "unknown DB policy must be normalized to 'hard'"); + + // Same expectation via listAllocationsForApiKey (the other read path). + const list = poolsDb.listAllocationsForApiKey("k-corrupt"); + assert.equal(list.length, 1); + assert.equal(list[0].allocation.policy, "hard"); +}); + +test("rowToAllocation preserves valid policies unchanged (Guard A regression)", () => { + const pool = poolsDb.createPool({ + connectionId: "c-guardA-valid", + name: "Valid Policy Pool", + allocations: [ + { apiKeyId: "k-hard", weight: 34, policy: "hard" }, + { apiKeyId: "k-soft", weight: 33, policy: "soft" }, + { apiKeyId: "k-burst", weight: 33, policy: "burst" }, + ], + }); + + const found = poolsDb.getPool(pool.id)!; + assert.equal(found.allocations.find((a) => a.apiKeyId === "k-hard")!.policy, "hard"); + assert.equal(found.allocations.find((a) => a.apiKeyId === "k-soft")!.policy, "soft"); + assert.equal(found.allocations.find((a) => a.apiKeyId === "k-burst")!.policy, "burst"); +}); diff --git a/tests/unit/quota-combos-sync.test.ts b/tests/unit/quota-combos-sync.test.ts index 0d3c74c0f6..a0a1b5b502 100644 --- a/tests/unit/quota-combos-sync.test.ts +++ b/tests/unit/quota-combos-sync.test.ts @@ -332,3 +332,90 @@ test("removeQuotaCombosForPool: unknown pool id — no throw", async () => { "removeQuotaCombosForPool with unknown poolId should not throw" ); }); + +// --------------------------------------------------------------------------- +// Guard B (issue #10): a pool whose connections no longer resolve (empty/dangling +// connection list) must NOT prune/delete the group's existing combos. Pruning is +// provider-scoped, and with no resolvable connection the provider is unknown — so +// syncQuotaCombos returns early (poolProvider === undefined) BEFORE the prune loop. +// This proves the `if (!poolProvider) return` guard covers the empty-connection +// path: a transient connection-resolution failure cannot wipe a group's combos. +// --------------------------------------------------------------------------- + +test("syncQuotaCombos: pool with no resolvable connection does NOT prune existing combos (Guard B)", async () => { + // 1. Seed a glm connection + pool and mint combos. + const conn = await providersDb.createProviderConnection({ + provider: "glm", + authType: "apikey", + name: "quota-combos-guardB", + apiKey: "sk-test-glm-guardb", + }); + const connId = (conn as Record).id as string; + const pool = poolsDb.createPool({ connectionId: connId, name: "GuardBPool" }); + + await syncQuotaCombos(pool.id); + const before = await listQuotaCombos(); + assert.ok(before.length > 0, "expected combos after initial sync"); + const beforeNames = new Set(before.map((c) => c.name)); + + // 2. Delete the provider connection while the pool still references it. + // The join row (quota_pool_connections) remains → getConnectionIds returns a + // dangling id whose getProviderConnectionById() resolves to null. This is the + // "connections do not resolve" scenario. + const removed = await providersDb.deleteProviderConnection(connId); + assert.equal(removed, true, "connection should be deleted"); + assert.equal( + await providersDb.getProviderConnectionById(connId), + null, + "connection must no longer resolve" + ); + + // 3. Re-sync. With no resolvable connection, poolProvider is undefined → the + // guard returns before pruning. Existing combos must be untouched. + await syncQuotaCombos(pool.id); + + const after = await listQuotaCombos(); + assert.equal( + after.length, + before.length, + "combos must NOT be pruned when the pool has no resolvable connection" + ); + for (const name of beforeNames) { + const stillThere = after.some((c) => c.name === name); + assert.ok(stillThere, `combo was wrongly pruned: ${name}`); + } +}); + +test("syncQuotaCombos: pool whose join table is emptied (truly no connectionIds) does NOT prune combos (Guard B)", async () => { + // Variant of Guard B where the join table itself is empty AND the primary + // connection is gone — connectionIds falls back to [pool.connectionId], which + // also fails to resolve. Still must not prune. + const conn = await providersDb.createProviderConnection({ + provider: "glm", + authType: "apikey", + name: "quota-combos-guardB-empty", + apiKey: "sk-test-glm-guardb-empty", + }); + const connId = (conn as Record).id as string; + const pool = poolsDb.createPool({ connectionId: connId, name: "GuardBEmptyPool" }); + + await syncQuotaCombos(pool.id); + const before = await listQuotaCombos(); + assert.ok(before.length > 0, "expected combos after initial sync"); + + // Empty the join table for this pool AND delete the connection row. + const db = core.getDbInstance() as unknown as { + prepare: (sql: string) => { run: (...p: unknown[]) => unknown }; + }; + db.prepare("DELETE FROM quota_pool_connections WHERE pool_id = ?").run(pool.id); + await providersDb.deleteProviderConnection(connId); + + await syncQuotaCombos(pool.id); + + const after = await listQuotaCombos(); + assert.equal( + after.length, + before.length, + "combos must survive when the pool has an empty/unresolvable connection set" + ); +}); diff --git a/tests/unit/quota-fair-share.test.ts b/tests/unit/quota-fair-share.test.ts index 37efde61b7..904dd3a859 100644 --- a/tests/unit/quota-fair-share.test.ts +++ b/tests/unit/quota-fair-share.test.ts @@ -201,3 +201,49 @@ test("fairShare: empty dimensions → allow:ok", () => { assert.equal(result.kind, "allow"); assert.equal(result.reason, "ok"); }); + +// ─── Guard A: unknown/garbage policy must fail SAFE (treated as hard) ───────── +// Defensive guard for issue #10: a policy value outside hard|soft|burst (e.g. a +// corrupted DB row read through `row.policy as Policy`) previously fell through +// every switch case in decideFairShare and returned a silent `allow` +// (fail-OPEN). It must be treated as the most restrictive policy (hard) so an +// unknown policy can never bypass fair-share enforcement. + +test("fairShare: GUARD-A strict mode, unknown policy over fair_share → block:fair-share (treated as hard)", () => { + // globalUsedPercent=0.6 >= 0.5 → strict; consumed=600 > fair_share=500. + // policy is garbage ("bogus") — must behave like hard and BLOCK, not allow. + const result = decideFairShare({ + dimensions: [dim({ limit: 1000, consumedTotal: 700, globalUsedPercent: 0.6 })], + // Cast through unknown to bypass the Policy type — simulates a corrupted row. + allocation: { weight: 50, policy: "bogus" as unknown as "hard" }, + consumedByThisKey: { "pool1:tokens:hourly": 600 }, + saturationThreshold: THRESHOLD, + }); + assert.equal(result.kind, "block"); + assert.equal(result.reason, "fair-share"); +}); + +test("fairShare: GUARD-A strict mode, unknown policy UNDER fair_share → allow (hard semantics still allow under share)", () => { + // Treated as hard: under fair_share is allowed even in strict mode. + const result = decideFairShare({ + dimensions: [dim({ limit: 1000, consumedTotal: 700, globalUsedPercent: 0.7 })], + allocation: { weight: 50, policy: "garbage" as unknown as "hard" }, + consumedByThisKey: { "pool1:tokens:hourly": 300 }, + saturationThreshold: THRESHOLD, + }); + assert.equal(result.kind, "allow"); +}); + +test("fairShare: GUARD-A generous mode, unknown policy → hard semantics (no soft penalize / no burst leniency)", () => { + // Generous mode (globalUsedPercent=0.3), consumed=600 > fair_share=500, but + // consumedTotal=600 < 1000 so hard allows borrowing. Must NOT be flagged as + // penalized (that would be soft) and must NOT block (key < global limit). + const result = decideFairShare({ + dimensions: [dim({ limit: 1000, consumedTotal: 600, globalUsedPercent: 0.3 })], + allocation: { weight: 50, policy: "weird" as unknown as "hard" }, + consumedByThisKey: { "pool1:tokens:hourly": 600 }, + saturationThreshold: THRESHOLD, + }); + assert.equal(result.kind, "allow"); + assert.equal(result.penalized, undefined, "unknown policy must not get soft penalize semantics"); +});