feat: add provider configuration export/import#1146
feat: add provider configuration export/import#1146NJX-njx wants to merge 1 commit intomoeru-ai:mainfrom
Conversation
- Add exportProviderConfigurations() and importProviderConfigurations() to useDataMaintenance composable - Add provider export/import UI card section in data settings page - Add i18n keys for provider export/import labels and status messages - Export format: provider-configurations:v1 with providers and addedProviders data - Import validates format and merges provider configs Closes moeru-ai#973
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances data management capabilities by introducing features for exporting and importing provider configurations. Users can now easily back up their provider settings or transfer them between instances, improving data portability and simplifying the setup of various AI providers. The changes provide a robust and user-friendly mechanism for managing sensitive configuration data through the application's settings. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Adds export/import support for provider configurations from the Data settings page to improve developer experience when resetting local data (Issue #973).
Changes:
- Added provider configuration export/import helpers in
useDataMaintenancewith aprovider-configurations:v1payload format. - Added a new Providers card in the Data settings page UI with Export/Import actions and a hidden file input.
- Added English i18n strings for the new Providers export/import UI and status messages.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
packages/stage-ui/src/composables/use-data-maintenance.ts |
Introduces provider config export/import logic and payload format checking. |
packages/stage-pages/src/pages/settings/data/index.vue |
Adds Providers export/import UI controls and wiring to the composable functions. |
packages/i18n/src/locales/en/settings.yaml |
Adds new labels/status messages for provider export/import and updates the Providers description text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function isProviderConfigPayload(payload: unknown): payload is { format: string, providers: Record<string, Record<string, unknown>>, addedProviders: Record<string, boolean> } { | ||
| if (!payload || typeof payload !== 'object') | ||
| return false | ||
| return (payload as { format?: string }).format === 'provider-configurations:v1' | ||
| && typeof (payload as any).providers === 'object' |
There was a problem hiding this comment.
isProviderConfigPayload only checks format and that providers is an object; null (and arrays) pass this check, which can then cause runtime errors in Object.entries(payload.providers) and unexpected imports. Strengthen the guard to ensure providers is a non-null plain object and validate addedProviders (or make it optional in the predicate) before iterating.
| function isProviderConfigPayload(payload: unknown): payload is { format: string, providers: Record<string, Record<string, unknown>>, addedProviders: Record<string, boolean> } { | |
| if (!payload || typeof payload !== 'object') | |
| return false | |
| return (payload as { format?: string }).format === 'provider-configurations:v1' | |
| && typeof (payload as any).providers === 'object' | |
| function isProviderConfigPayload(payload: unknown): payload is { format: string, providers: Record<string, Record<string, unknown>>, addedProviders?: Record<string, boolean> } { | |
| if (!payload || typeof payload !== 'object') | |
| return false | |
| const maybePayload = payload as { | |
| format?: string | |
| providers?: unknown | |
| addedProviders?: unknown | |
| } | |
| if (maybePayload.format !== 'provider-configurations:v1') | |
| return false | |
| const providers = maybePayload.providers | |
| if (!providers || typeof providers !== 'object' || Array.isArray(providers)) | |
| return false | |
| const addedProviders = maybePayload.addedProviders | |
| if (addedProviders !== undefined && (addedProviders === null || typeof addedProviders !== 'object' || Array.isArray(addedProviders))) | |
| return false | |
| return true |
| const currentProviders = providersStore.providers | ||
| for (const [id, config] of Object.entries(payload.providers)) { | ||
| if (config && typeof config === 'object') { | ||
| currentProviders[id] = config |
There was a problem hiding this comment.
importProviderConfigurations replaces each provider config object (currentProviders[id] = config). For forward compatibility (e.g., importing an older file that lacks newly added config fields), consider merging into the existing config (or defaults) rather than wholesale replacement, so missing fields don't get dropped.
| currentProviders[id] = config | |
| const existing = currentProviders[id] | |
| if (existing && typeof existing === 'object') { | |
| currentProviders[id] = { | |
| ...(existing as Record<string, unknown>), | |
| ...(config as Record<string, unknown>), | |
| } | |
| } | |
| else { | |
| currentProviders[id] = config | |
| } |
| function triggerProviderImportPicker() { | ||
| importError.value = '' | ||
| providerImportFileInput.value?.click() | ||
| } | ||
|
|
||
| function handleProviderImport(event: Event) { | ||
| const target = event.target as HTMLInputElement | ||
| const file = target.files?.[0] | ||
| if (!file) | ||
| return | ||
|
|
||
| file.text().then((raw) => { | ||
| const parsed = JSON.parse(raw) as Record<string, unknown> | ||
| importProviderConfigurations(parsed) | ||
| setStatus(t('settings.pages.data.status.providers_imported')) | ||
| importError.value = '' | ||
| }).catch((error) => { | ||
| console.error(error) | ||
| importError.value = t('settings.pages.data.status.providers_import_error') | ||
| setStatus(error instanceof Error ? error.message : String(error), 'error') | ||
| }).finally(() => { | ||
| target.value = '' | ||
| }) |
There was a problem hiding this comment.
Provider import/export reuses importError, but the only place that displays importError is inside the chat sessions card. As a result, provider import failures won't be shown next to the providers section (and may appear under the chat section instead). Use a separate providerImportError state and render it under the providers card, and clear that state in triggerProviderImportPicker/handleProviderImport.
| providers: | ||
| title: Providers | ||
| description: Reset all provider settings and credentials. | ||
| description: Export or import provider configurations and credentials. |
There was a problem hiding this comment.
settings.pages.data.sections.providers.description is used both for the new providers export/import card and for the danger-zone provider reset section (see settings page template). Updating it to an export/import description makes the danger-zone copy inaccurate. Introduce a new i18n key for the export/import card description (or a separate danger-zone description) so each section has correct text.
| description: Export or import provider configurations and credentials. | |
| description: Reset provider configurations and credentials. | |
| export_import_description: Export or import provider configurations and credentials. |
![gemini-code-assist[bot]](https://avatars.gh.lumenfield.work/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for exporting and importing provider configurations. The current implementation of the import process has security vulnerabilities related to prototype pollution and credential theft, and a critical bug in the import logic prevents configurations from being saved correctly. Additionally, there are suggestions to enhance correctness by completing a type guard and improving code consistency by refactoring to async/await. Addressing these points will ensure the feature is secure and robust.
| function importProviderConfigurations(payload: Record<string, unknown>) { | ||
| if (!isProviderConfigPayload(payload)) | ||
| throw new Error('Invalid provider configuration export format') | ||
|
|
||
| const currentProviders = providersStore.providers | ||
| for (const [id, config] of Object.entries(payload.providers)) { | ||
| if (config && typeof config === 'object') { | ||
| currentProviders[id] = config | ||
| } | ||
| } | ||
|
|
||
| if (payload.addedProviders) { | ||
| for (const [id, added] of Object.entries(payload.addedProviders)) { | ||
| if (added) { | ||
| providersStore.markProviderAdded(id) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The importProviderConfigurations function lacks proper validation of the imported JSON payload, leading to two main vulnerabilities: 1. Credential Theft: An attacker can craft a malicious JSON file that sets the baseUrl of a provider to an attacker-controlled server, potentially stealing API keys. 2. Prototype Pollution: The function iterates over the providers object in the payload and assigns its properties to the currentProviders object. If the payload contains a __proto__ key, it can lead to prototype pollution. Additionally, providersStore.providers returns a Vue Ref object. The code attempts to assign properties directly to this Ref object (currentProviders[id] = config) instead of its underlying .value, which will not update the stored data correctly. To remediate these issues, you should: - Validate that the provider IDs in the imported file are valid (exist in providerMetadata). - Explicitly check for and reject sensitive keys like __proto__, constructor, and prototype during the import process. - Ensure that when modifying currentProviders, you access its .value property (e.g., currentProviders.value[id] = config). - Consider warning the user before overwriting existing configurations.
function importProviderConfigurations(payload: Record<string, unknown>) {
if (!isProviderConfigPayload(payload))
throw new Error('Invalid provider configuration export format')
const currentProviders = providersStore.providers
for (const [id, config] of Object.entries(payload.providers)) {
if (id === '__proto__' || id === 'constructor' || id === 'prototype')
continue
if (!providersStore.providerMetadata[id])
continue
if (config && typeof config === 'object') {
currentProviders[id] = config
}
}
if (payload.addedProviders) {
for (const [id, added] of Object.entries(payload.addedProviders)) {
if (id === '__proto__' || id === 'constructor' || id === 'prototype')
continue
if (!providersStore.providerMetadata[id])
continue
if (added) {
providersStore.markProviderAdded(id)
}
}
}
}| function isProviderConfigPayload(payload: unknown): payload is { format: string, providers: Record<string, Record<string, unknown>>, addedProviders: Record<string, boolean> } { | ||
| if (!payload || typeof payload !== 'object') | ||
| return false | ||
| return (payload as { format?: string }).format === 'provider-configurations:v1' | ||
| && typeof (payload as any).providers === 'object' | ||
| } |
There was a problem hiding this comment.
The type guard isProviderConfigPayload is incomplete. Its type predicate payload is { ..., addedProviders: Record<string, boolean> } indicates that addedProviders should be present, but the function body does not validate its existence or type. This discrepancy can lead to incorrect type narrowing. Please add a check for addedProviders to ensure the type guard is accurate and reliable.
| function isProviderConfigPayload(payload: unknown): payload is { format: string, providers: Record<string, Record<string, unknown>>, addedProviders: Record<string, boolean> } { | |
| if (!payload || typeof payload !== 'object') | |
| return false | |
| return (payload as { format?: string }).format === 'provider-configurations:v1' | |
| && typeof (payload as any).providers === 'object' | |
| } | |
| function isProviderConfigPayload(payload: unknown): payload is { format: string, providers: Record<string, Record<string, unknown>>, addedProviders: Record<string, boolean> } { | |
| if (!payload || typeof payload !== 'object') | |
| return false | |
| return (payload as { format?: string }).format === 'provider-configurations:v1' | |
| && typeof (payload as any).providers === 'object' | |
| && typeof (payload as any).addedProviders === 'object' | |
| } |
| function handleProviderImport(event: Event) { | ||
| const target = event.target as HTMLInputElement | ||
| const file = target.files?.[0] | ||
| if (!file) | ||
| return | ||
|
|
||
| file.text().then((raw) => { | ||
| const parsed = JSON.parse(raw) as Record<string, unknown> | ||
| importProviderConfigurations(parsed) | ||
| setStatus(t('settings.pages.data.status.providers_imported')) | ||
| importError.value = '' | ||
| }).catch((error) => { | ||
| console.error(error) | ||
| importError.value = t('settings.pages.data.status.providers_import_error') | ||
| setStatus(error instanceof Error ? error.message : String(error), 'error') | ||
| }).finally(() => { | ||
| target.value = '' | ||
| }) | ||
| } |
There was a problem hiding this comment.
For consistency with the handleImport function in the same file, it's recommended to refactor handleProviderImport to use async/await instead of .then() chains. This will improve code readability and maintainability across the component.
async function handleProviderImport(event: Event) {
const target = event.target as HTMLInputElement
const file = target.files?.[0]
if (!file)
return
try {
const raw = await file.text()
const parsed = JSON.parse(raw) as Record<string, unknown>
importProviderConfigurations(parsed)
setStatus(t('settings.pages.data.status.providers_imported'))
importError.value = ''
}
catch (error) {
console.error(error)
importError.value = t('settings.pages.data.status.providers_import_error')
setStatus(error instanceof Error ? error.message : String(error), 'error')
}
finally {
target.value = ''
}
}
Summary
Adds the ability to export and import provider configurations (API keys, endpoints, settings) from the Data settings page. This addresses Issue #973.
Changes
How it works
Testing
Closes #973