From f28709ea4fc1af189485b4fd28af94593d3bd9da Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Fri, 5 Dec 2025 10:28:11 -0700 Subject: [PATCH] fix: sanitize removed/invalid API providers to prevent infinite loop When a user has an API provider selected that is later removed from the extension (e.g., 'glama'), the schema validation repeatedly fails causing telemetry spam and potential infinite loops. This fix: - Adds sanitizeProviderConfig() in ProviderSettingsManager to reset invalid apiProvider values during load while preserving the rest of the profile - Adds sanitizeProviderValues() in ContextProxy to prevent repeated validation errors when getProviderSettings() is called - Adds migrateInvalidApiProvider() migration to clean up invalid values from storage during initialization The profile is preserved with apiProvider reset to undefined so the user can select a new valid provider instead of losing their entire configuration. --- src/core/config/ContextProxy.ts | 52 ++++++++++++- src/core/config/ProviderSettingsManager.ts | 32 +++++++- .../config/__tests__/ContextProxy.spec.ts | 75 +++++++++++++++++++ .../__tests__/ProviderSettingsManager.spec.ts | 69 +++++++++++++++-- 4 files changed, 219 insertions(+), 9 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index ab952da9491..d570ccc7c67 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -15,6 +15,7 @@ import { providerSettingsSchema, globalSettingsSchema, isSecretStateKey, + isProviderName, } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" @@ -88,9 +89,33 @@ export class ContextProxy { // Migration: Check for old nested image generation settings and migrate them await this.migrateImageGenerationSettings() + // Migration: Sanitize invalid/removed API providers + await this.migrateInvalidApiProvider() + this._isInitialized = true } + /** + * Migrates invalid/removed apiProvider values by clearing them from storage. + * This handles cases where a user had a provider selected that was later removed + * from the extension (e.g., "glama"). + */ + private async migrateInvalidApiProvider() { + try { + const apiProvider = this.stateCache.apiProvider + if (apiProvider !== undefined && !isProviderName(apiProvider)) { + logger.info(`[ContextProxy] Found invalid provider "${apiProvider}" in storage - clearing it`) + // Clear the invalid provider from both cache and storage + this.stateCache.apiProvider = undefined + await this.originalContext.globalState.update("apiProvider", undefined) + } + } catch (error) { + logger.error( + `Error during invalid API provider migration: ${error instanceof Error ? error.message : String(error)}`, + ) + } + } + /** * Migrates old nested openRouterImageGenerationSettings to the new flattened structure */ @@ -266,15 +291,38 @@ export class ContextProxy { public getProviderSettings(): ProviderSettings { const values = this.getValues() + // Sanitize invalid/removed apiProvider values before parsing + // This handles cases where a user had a provider selected that was later removed + // from the extension (e.g., "glama"). We sanitize here to avoid repeated + // schema validation errors that can cause infinite loops in telemetry. + const sanitizedValues = this.sanitizeProviderValues(values) + try { - return providerSettingsSchema.parse(values) + return providerSettingsSchema.parse(sanitizedValues) } catch (error) { if (error instanceof ZodError) { TelemetryService.instance.captureSchemaValidationError({ schemaName: "ProviderSettings", error }) } - return PROVIDER_SETTINGS_KEYS.reduce((acc, key) => ({ ...acc, [key]: values[key] }), {} as ProviderSettings) + return PROVIDER_SETTINGS_KEYS.reduce( + (acc, key) => ({ ...acc, [key]: sanitizedValues[key] }), + {} as ProviderSettings, + ) + } + } + + /** + * Sanitizes provider values by resetting invalid/removed apiProvider values. + * This prevents schema validation errors for removed providers. + */ + private sanitizeProviderValues(values: RooCodeSettings): RooCodeSettings { + if (values.apiProvider !== undefined && !isProviderName(values.apiProvider)) { + logger.info(`[ContextProxy] Sanitizing invalid provider "${values.apiProvider}" - resetting to undefined`) + // Return a new values object without the invalid apiProvider + const { apiProvider, ...restValues } = values + return restValues as RooCodeSettings } + return values } public async setProviderSettings(values: ProviderSettings) { diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index ea3e18f3ef7..43017882fae 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -11,6 +11,7 @@ import { DEFAULT_CONSECUTIVE_MISTAKE_LIMIT, getModelId, type ProviderName, + isProviderName, } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" @@ -598,7 +599,10 @@ export class ProviderSettingsManager { const apiConfigs = Object.entries(providerProfiles.apiConfigs).reduce( (acc, [key, apiConfig]) => { - const result = providerSettingsWithIdSchema.safeParse(apiConfig) + // First, sanitize invalid apiProvider values before parsing + // This handles removed providers (like "glama") gracefully + const sanitizedConfig = this.sanitizeProviderConfig(apiConfig) + const result = providerSettingsWithIdSchema.safeParse(sanitizedConfig) return result.success ? { ...acc, [key]: result.data } : acc }, {} as Record, @@ -622,6 +626,32 @@ export class ProviderSettingsManager { } } + /** + * Sanitizes a provider config by resetting invalid/removed apiProvider values. + * This handles cases where a user had a provider selected that was later removed + * from the extension (e.g., "glama"). + */ + private sanitizeProviderConfig(apiConfig: unknown): unknown { + if (typeof apiConfig !== "object" || apiConfig === null) { + return apiConfig + } + + const config = apiConfig as Record + + // Check if apiProvider is set and if it's still valid + if (config.apiProvider !== undefined && !isProviderName(config.apiProvider)) { + console.log( + `[ProviderSettingsManager] Sanitizing invalid provider "${config.apiProvider}" - resetting to undefined`, + ) + // Return a new config object without the invalid apiProvider + // This effectively resets the profile so the user can select a valid provider + const { apiProvider, ...restConfig } = config + return restConfig + } + + return apiConfig + } + private async store(providerProfiles: ProviderProfiles) { try { await this.context.secrets.store(this.secretsKey, JSON.stringify(providerProfiles, null, 2)) diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 58dae7e24e5..49e706b1818 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -428,4 +428,79 @@ describe("ContextProxy", () => { expect(initializeSpy).toHaveBeenCalledTimes(1) }) }) + + describe("invalid apiProvider migration", () => { + it("should clear invalid apiProvider from storage during initialization", async () => { + // Reset and create a new proxy with invalid provider in state + vi.clearAllMocks() + mockGlobalState.get.mockImplementation((key: string) => { + if (key === "apiProvider") { + return "invalid-removed-provider" // Invalid/removed provider + } + return undefined + }) + + const proxyWithInvalidProvider = new ContextProxy(mockContext) + await proxyWithInvalidProvider.initialize() + + // Should have cleared the invalid apiProvider + expect(mockGlobalState.update).toHaveBeenCalledWith("apiProvider", undefined) + }) + + it("should not modify valid apiProvider during initialization", async () => { + // Reset and create a new proxy with valid provider in state + vi.clearAllMocks() + mockGlobalState.get.mockImplementation((key: string) => { + if (key === "apiProvider") { + return "anthropic" // Valid provider + } + return undefined + }) + + const proxyWithValidProvider = new ContextProxy(mockContext) + await proxyWithValidProvider.initialize() + + // Should NOT have called update for apiProvider (it's valid) + const updateCalls = mockGlobalState.update.mock.calls + const apiProviderUpdateCalls = updateCalls.filter((call: any[]) => call[0] === "apiProvider") + expect(apiProviderUpdateCalls.length).toBe(0) + }) + }) + + describe("getProviderSettings", () => { + it("should sanitize invalid apiProvider before parsing", async () => { + // Set an invalid provider in state + await proxy.updateGlobalState("apiProvider", "invalid-removed-provider" as any) + await proxy.updateGlobalState("apiModelId", "some-model") + + const settings = proxy.getProviderSettings() + + // The invalid apiProvider should be sanitized (removed) + expect(settings.apiProvider).toBeUndefined() + // Other settings should still be present + expect(settings.apiModelId).toBe("some-model") + }) + + it("should pass through valid apiProvider", async () => { + // Set a valid provider in state + await proxy.updateGlobalState("apiProvider", "anthropic") + await proxy.updateGlobalState("apiModelId", "claude-3-opus-20240229") + + const settings = proxy.getProviderSettings() + + // Valid provider should be returned + expect(settings.apiProvider).toBe("anthropic") + expect(settings.apiModelId).toBe("claude-3-opus-20240229") + }) + + it("should handle undefined apiProvider gracefully", async () => { + // Ensure no provider is set + await proxy.updateGlobalState("apiProvider", undefined) + + const settings = proxy.getProviderSettings() + + // Should not throw and should return undefined + expect(settings.apiProvider).toBeUndefined() + }) + }) }) diff --git a/src/core/config/__tests__/ProviderSettingsManager.spec.ts b/src/core/config/__tests__/ProviderSettingsManager.spec.ts index b710dc6cca8..d8dd62cad9b 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.spec.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.spec.ts @@ -705,7 +705,55 @@ describe("ProviderSettingsManager", () => { ) }) - it("should remove invalid profiles during load", async () => { + it("should sanitize invalid/removed providers by resetting apiProvider to undefined", async () => { + // This tests the fix for the infinite loop issue when a provider is removed + const configWithRemovedProvider = { + currentApiConfigName: "valid", + apiConfigs: { + valid: { + apiProvider: "anthropic", + apiKey: "valid-key", + apiModelId: "claude-3-opus-20240229", + id: "valid-id", + }, + removedProvider: { + // Provider that was removed from the extension (e.g., "invalid-removed-provider") + id: "removed-id", + apiProvider: "invalid-removed-provider", + apiKey: "some-key", + apiModelId: "some-model", + }, + }, + migrations: { + rateLimitSecondsMigrated: true, + diffSettingsMigrated: true, + openAiHeadersMigrated: true, + consecutiveMistakeLimitMigrated: true, + todoListEnabledMigrated: true, + }, + } + + mockSecrets.get.mockResolvedValue(JSON.stringify(configWithRemovedProvider)) + + await providerSettingsManager.initialize() + + const storeCalls = mockSecrets.store.mock.calls + expect(storeCalls.length).toBeGreaterThan(0) + const finalStoredConfigJson = storeCalls[storeCalls.length - 1][1] + + const storedConfig = JSON.parse(finalStoredConfigJson) + // The valid provider should be untouched + expect(storedConfig.apiConfigs.valid).toBeDefined() + expect(storedConfig.apiConfigs.valid.apiProvider).toBe("anthropic") + + // The config with the removed provider should have its apiProvider reset to undefined + // but still be present (not filtered out entirely) + expect(storedConfig.apiConfigs.removedProvider).toBeDefined() + expect(storedConfig.apiConfigs.removedProvider.apiProvider).toBeUndefined() + expect(storedConfig.apiConfigs.removedProvider.id).toBe("removed-id") + }) + + it("should sanitize invalid providers and remove non-object profiles during load", async () => { const invalidConfig = { currentApiConfigName: "valid", apiConfigs: { @@ -715,12 +763,12 @@ describe("ProviderSettingsManager", () => { apiModelId: "claude-3-opus-20240229", rateLimitSeconds: 0, }, - invalid: { - // Invalid API provider. + invalidProvider: { + // Invalid API provider - should be sanitized (kept but apiProvider reset to undefined) id: "x.ai", apiProvider: "x.ai", }, - // Incorrect type. + // Incorrect type - should be completely removed anotherInvalid: "not an object", }, migrations: { @@ -737,10 +785,19 @@ describe("ProviderSettingsManager", () => { const finalStoredConfigJson = storeCalls[storeCalls.length - 1][1] const storedConfig = JSON.parse(finalStoredConfigJson) + // Valid config should be untouched expect(storedConfig.apiConfigs.valid).toBeDefined() - expect(storedConfig.apiConfigs.invalid).toBeUndefined() + expect(storedConfig.apiConfigs.valid.apiProvider).toBe("anthropic") + + // Invalid provider config should be sanitized - kept but apiProvider reset to undefined + expect(storedConfig.apiConfigs.invalidProvider).toBeDefined() + expect(storedConfig.apiConfigs.invalidProvider.apiProvider).toBeUndefined() + expect(storedConfig.apiConfigs.invalidProvider.id).toBe("x.ai") + + // Non-object config should be completely removed expect(storedConfig.apiConfigs.anotherInvalid).toBeUndefined() - expect(Object.keys(storedConfig.apiConfigs)).toEqual(["valid"]) + + expect(Object.keys(storedConfig.apiConfigs)).toEqual(["valid", "invalidProvider"]) expect(storedConfig.currentApiConfigName).toBe("valid") }) })