diff --git a/packages/loopover-engine/src/tenant-config.ts b/packages/loopover-engine/src/tenant-config.ts index 9334f1781..3afc9fe9d 100644 --- a/packages/loopover-engine/src/tenant-config.ts +++ b/packages/loopover-engine/src/tenant-config.ts @@ -42,6 +42,13 @@ export const DEFAULT_TENANT_CONFIG: TenantConfig = { * is copied on every call — so mutating one tenant's config can never affect another's. An override with an * unrecognized autonomy level falls back to the default level rather than trusting arbitrary input. */ +// #9614: normalize any numeric input to a non-negative integer (a non-finite or negative value becomes 0) -- +// the exact body used in tenant-quota.ts:45-47 / loop-consumption.ts / worktree-pool.ts (the #5828 +// finiteNonNegativeInt discipline), so maxConcurrentLoops can never resolve NaN, fractional, negative, or Infinity. +function finiteNonNegativeInt(value: number): number { + return Number.isFinite(value) ? Math.max(0, Math.floor(value)) : 0; +} + export function resolveTenantConfig(overrides: TenantConfigOverrides = {}): TenantConfig { const base = DEFAULT_TENANT_CONFIG; const autonomyLevel = @@ -52,9 +59,17 @@ export function resolveTenantConfig(overrides: TenantConfigOverrides = {}): Tena return { autonomyLevel, preferences: { - maxConcurrentLoops: prefs.maxConcurrentLoops ?? base.preferences.maxConcurrentLoops, + // #9614: normalize an EXPLICITLY-supplied override (finiteNonNegativeInt); an absent override still + // inherits DEFAULT_TENANT_CONFIG's 1, since `??` alone let -5/0.5/NaN/Infinity pass through verbatim. + maxConcurrentLoops: + prefs.maxConcurrentLoops !== undefined ? finiteNonNegativeInt(prefs.maxConcurrentLoops) : base.preferences.maxConcurrentLoops, pauseOnFailure: prefs.pauseOnFailure ?? base.preferences.pauseOnFailure, - allowedActionClasses: [...(prefs.allowedActionClasses ?? base.preferences.allowedActionClasses)], + // #9614: drop non-string entries from an override rather than copying them through, mirroring how an + // unrecognized autonomyLevel is refused above -- untrusted input must not smuggle a non-string action class in. + allowedActionClasses: + prefs.allowedActionClasses !== undefined + ? prefs.allowedActionClasses.filter((entry): entry is string => typeof entry === "string") + : [...base.preferences.allowedActionClasses], }, }; } @@ -74,10 +89,33 @@ export function setTenantConfig( tenantId: string, overrides: TenantConfigOverrides = {}, ): TenantConfigStore { - return Object.freeze({ ...store, [tenantId]: resolveTenantConfig(overrides) }); + // #9614: deep-freeze the written entry. Object.freeze is shallow, so freeze each level (the config, its + // preferences, and the allowedActionClasses array) -- otherwise a caller holding a reference to a nested + // value could still mutate the stored config. + const config = resolveTenantConfig(overrides); + Object.freeze(config.preferences.allowedActionClasses); + Object.freeze(config.preferences); + Object.freeze(config); + return Object.freeze({ ...store, [tenantId]: config }); +} + +// #9614: a fresh deep copy sharing no mutable reference with the stored (frozen-but-shallow) config, so a +// caller that mutates the result can never rewrite the tenant's stored preferences/allowedActionClasses. +function cloneTenantConfig(config: TenantConfig): TenantConfig { + return { + autonomyLevel: config.autonomyLevel, + preferences: { + maxConcurrentLoops: config.preferences.maxConcurrentLoops, + pauseOnFailure: config.preferences.pauseOnFailure, + allowedActionClasses: [...config.preferences.allowedActionClasses], + }, + }; } -/** Read a tenant's effective config, falling back to a fresh copy of the defaults when they've set none. */ +/** Read a tenant's effective config as a fresh, caller-owned copy. The returned config -- and its nested + * `preferences` and `allowedActionClasses` -- shares no reference with the store on EITHER path, so mutating + * it never affects the stored config; a tenant with none set gets a fresh copy of the defaults. */ export function getTenantConfig(store: TenantConfigStore, tenantId: string): TenantConfig { - return store[tenantId] ?? resolveTenantConfig(); + const stored = store[tenantId]; + return stored !== undefined ? cloneTenantConfig(stored) : resolveTenantConfig(); } diff --git a/test/unit/tenant-config.test.ts b/test/unit/tenant-config.test.ts index d81eed161..4779d394f 100644 --- a/test/unit/tenant-config.test.ts +++ b/test/unit/tenant-config.test.ts @@ -68,3 +68,46 @@ describe("tenant config store (#4787)", () => { expect(DEFAULT_TENANT_CONFIG.preferences.allowedActionClasses).not.toContain("delete_repo"); }); }); + +describe("tenant-config isolation + normalization (#9614)", () => { + it("normalizes an EXPLICIT maxConcurrentLoops override with finiteNonNegativeInt", () => { + const norm = (v: number) => resolveTenantConfig({ preferences: { maxConcurrentLoops: v } }).preferences.maxConcurrentLoops; + expect(norm(-5)).toBe(0); // negative -> 0 + expect(norm(0.5)).toBe(0); // fractional -> floor -> 0 + expect(norm(Number.NaN)).toBe(0); // non-finite -> 0 + expect(norm(Number.POSITIVE_INFINITY)).toBe(0); // non-finite -> 0 + expect(norm(3)).toBe(3); // finite positive int passes through + }); + + it("an ABSENT maxConcurrentLoops override still inherits DEFAULT_TENANT_CONFIG's 1", () => { + // pauseOnFailure supplied so the preferences override object is present but maxConcurrentLoops is not. + expect(resolveTenantConfig({ preferences: { pauseOnFailure: false } }).preferences.maxConcurrentLoops).toBe(1); + }); + + it("drops non-string entries from an allowedActionClasses override rather than copying them through", () => { + const cfg = resolveTenantConfig({ + preferences: { allowedActionClasses: ["open_pr", 42, null, "comment"] as unknown as string[] }, + }); + expect(cfg.preferences.allowedActionClasses).toEqual(["open_pr", "comment"]); + }); + + it("getTenantConfig returns a caller-owned deep copy on the HIT path — mutating it never reaches the store", () => { + const store = setTenantConfig(EMPTY_TENANT_CONFIG_STORE, "acme", { preferences: { allowedActionClasses: ["open_pr"] } }); + const first = getTenantConfig(store, "acme"); + (first.preferences.allowedActionClasses as string[]).push("merge_pr"); + first.preferences.maxConcurrentLoops = 99; + first.autonomyLevel = "auto"; + const second = getTenantConfig(store, "acme"); + expect(second.preferences.allowedActionClasses).toEqual(["open_pr"]); + expect(second.preferences.maxConcurrentLoops).toBe(1); + expect(second.autonomyLevel).toBe("suggest"); + }); + + it("setTenantConfig deep-freezes the stored entry (config, its preferences, and the action-class array)", () => { + const store = setTenantConfig(EMPTY_TENANT_CONFIG_STORE, "acme", {}); + const stored = store["acme"]!; + expect(Object.isFrozen(stored)).toBe(true); + expect(Object.isFrozen(stored.preferences)).toBe(true); + expect(Object.isFrozen(stored.preferences.allowedActionClasses)).toBe(true); + }); +});