Skip to content

Stop leaking other provider settings #3357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 9, 2025
Merged

Stop leaking other provider settings #3357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a71884a
Stop leaking other provider settings
jr May 7, 2025
9a6f783
Also filter out leaked properties on export
jr May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/core/config/ProviderSettingsManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import { ExtensionContext } from "vscode"
import { z, ZodError } from "zod"

import { providerSettingsSchema, ApiConfigMeta } from "../../schemas"
import { providerSettingsSchema, ApiConfigMeta, providerSettingsSchemaDiscriminated } from "../../schemas"
import { Mode, modes } from "../../shared/modes"
import { telemetryService } from "../../services/telemetry/TelemetryService"

const providerSettingsWithIdSchema = providerSettingsSchema.extend({ id: z.string().optional() })
const discriminatedProviderSettingsWithIdSchema = providerSettingsSchemaDiscriminated.and(
z.object({ id: z.string().optional() }),
)

type ProviderSettingsWithId = z.infer<typeof providerSettingsWithIdSchema>

Expand Down Expand Up @@ -250,7 +253,11 @@ export class ProviderSettingsManager {
const providerProfiles = await this.load()
// Preserve the existing ID if this is an update to an existing config.
const existingId = providerProfiles.apiConfigs[name]?.id
providerProfiles.apiConfigs[name] = { ...config, id: config.id || existingId || this.generateId() }
const id = config.id || existingId || this.generateId()

// Filter out settings from other providers.
const filteredConfig = providerSettingsSchemaDiscriminated.parse(config)
providerProfiles.apiConfigs[name] = { ...filteredConfig, id }
await this.store(providerProfiles)
})
} catch (error) {
Expand Down Expand Up @@ -381,7 +388,15 @@ export class ProviderSettingsManager {

public async export() {
try {
return await this.lock(async () => providerProfilesSchema.parse(await this.load()))
return await this.lock(async () => {
const profiles = providerProfilesSchema.parse(await this.load())
const configs = profiles.apiConfigs
for (const name in configs) {
// Avoid leaking properties from other providers.
configs[name] = discriminatedProviderSettingsWithIdSchema.parse(configs[name])
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a config fails the discriminated schema parse, the entire export will throw. Consider logging or including the provider name in the error to aid debugging if one config is malformed.

This comment was generated because it violated a code review rule: mrule_OR1S8PRRHcvbdFib.

Copy link
Collaborator Author

@jr jr May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it already passed providerProfilesSchema.parse if this fails it suggests a bug in the schemas. Ideally we'd either figure out a way to have the discriminated union baked into the main schema or else find a way to assert that they describe the same properties, but that feels like too large of a reach at this point.

}
return profiles
})
} catch (error) {
throw new Error(`Failed to export provider profiles: ${error}`)
}
Expand Down
61 changes: 54 additions & 7 deletions src/core/config/__tests__/ProviderSettingsManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,58 @@ describe("ProviderSettingsManager", () => {
},
}

expect(mockSecrets.store).toHaveBeenCalledWith(
"roo_cline_config_api_config",
JSON.stringify(expectedConfig, null, 2),
expect(mockSecrets.store.mock.calls[0][0]).toEqual("roo_cline_config_api_config")
expect(storedConfig).toEqual(expectedConfig)
})

it("should only save provider relevant settings", async () => {
mockSecrets.get.mockResolvedValue(
JSON.stringify({
currentApiConfigName: "default",
apiConfigs: {
default: {},
},
modeApiConfigs: {
code: "default",
architect: "default",
ask: "default",
},
}),
)

const newConfig: ProviderSettings = {
apiProvider: "anthropic",
apiKey: "test-key",
}
const newConfigWithExtra: ProviderSettings = {
...newConfig,
openRouterApiKey: "another-key",
}

await providerSettingsManager.saveConfig("test", newConfigWithExtra)

// Get the actual stored config to check the generated ID
const storedConfig = JSON.parse(mockSecrets.store.mock.lastCall[1])
const testConfigId = storedConfig.apiConfigs.test.id

const expectedConfig = {
currentApiConfigName: "default",
apiConfigs: {
default: {},
test: {
...newConfig,
id: testConfigId,
},
},
modeApiConfigs: {
code: "default",
architect: "default",
ask: "default",
},
}

expect(mockSecrets.store.mock.calls[0][0]).toEqual("roo_cline_config_api_config")
expect(storedConfig).toEqual(expectedConfig)
})

it("should update existing config", async () => {
Expand Down Expand Up @@ -291,10 +339,9 @@ describe("ProviderSettingsManager", () => {
},
}

expect(mockSecrets.store).toHaveBeenCalledWith(
"roo_cline_config_api_config",
JSON.stringify(expectedConfig, null, 2),
)
const storedConfig = JSON.parse(mockSecrets.store.mock.lastCall[1])
expect(mockSecrets.store.mock.lastCall[0]).toEqual("roo_cline_config_api_config")
expect(storedConfig).toEqual(expectedConfig)
})

it("should throw error if secrets storage fails", async () => {
Expand Down
6 changes: 3 additions & 3 deletions src/exports/roo-code.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,22 +121,22 @@ type ProviderSettings = {
unboundModelId?: string | undefined
requestyApiKey?: string | undefined
requestyModelId?: string | undefined
fakeAi?: unknown | undefined
xaiApiKey?: string | undefined
groqApiKey?: string | undefined
chutesApiKey?: string | undefined
litellmBaseUrl?: string | undefined
litellmApiKey?: string | undefined
litellmModelId?: string | undefined
modelMaxTokens?: number | undefined
modelMaxThinkingTokens?: number | undefined
includeMaxTokens?: boolean | undefined
reasoningEffort?: ("low" | "medium" | "high") | undefined
promptCachingDisabled?: boolean | undefined
diffEnabled?: boolean | undefined
fuzzyMatchThreshold?: number | undefined
modelTemperature?: (number | null) | undefined
rateLimitSeconds?: number | undefined
fakeAi?: unknown | undefined
modelMaxTokens?: number | undefined
modelMaxThinkingTokens?: number | undefined
}

type GlobalSettings = {
Expand Down
6 changes: 3 additions & 3 deletions src/exports/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,22 +122,22 @@ type ProviderSettings = {
unboundModelId?: string | undefined
requestyApiKey?: string | undefined
requestyModelId?: string | undefined
fakeAi?: unknown | undefined
xaiApiKey?: string | undefined
groqApiKey?: string | undefined
chutesApiKey?: string | undefined
litellmBaseUrl?: string | undefined
litellmApiKey?: string | undefined
litellmModelId?: string | undefined
modelMaxTokens?: number | undefined
modelMaxThinkingTokens?: number | undefined
includeMaxTokens?: boolean | undefined
reasoningEffort?: ("low" | "medium" | "high") | undefined
promptCachingDisabled?: boolean | undefined
diffEnabled?: boolean | undefined
fuzzyMatchThreshold?: number | undefined
modelTemperature?: (number | null) | undefined
rateLimitSeconds?: number | undefined
fakeAi?: unknown | undefined
modelMaxTokens?: number | undefined
modelMaxThinkingTokens?: number | undefined
}

export type { ProviderSettings }
Expand Down
Loading