-
Notifications
You must be signed in to change notification settings - Fork 3.3k
feat: add Volcengine integration with settings and API client #11482
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.
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
base: main
Are you sure you want to change the base?
Conversation
- Implement Volcengine configuration in multiple languages (el-gr, es-es, fr-fr, ja-jp, pt-pt, ru-ru). - Add Volcengine settings component to manage access key ID, secret access key, and region. - Create Volcengine service for API interactions, including credential management and model listing. - Extend OpenAI API client to support Volcengine's signed API for model retrieval. - Update Redux store to handle Volcengine settings and credentials. - Implement migration for Volcengine settings in the store. - Add hooks for accessing and managing Volcengine settings in the application.
- Fix region field being ignored: pass user-configured region to listFoundationModels and listEndpoints - Add user notification before silent fallback when API fails - Throw error on credential corruption instead of returning null - Remove redundant credentials (accessKeyId, secretAccessKey) from Redux store (they're securely stored via safeStorage) - Add warnings field to ListModelsResult for partial API failures - Fix Redux/IPC order: save to secure storage first, then update Redux on success - Update related tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
0xfullex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review: Critical Security Issues
Thank you for this comprehensive Volcengine integration! The implementation follows existing patterns well and the HMAC-SHA256 signing looks correct. However, I found a few security issues that should be addressed before merge:
🔴 Critical Issue 1: Missing safeStorage Availability Check
File: src/main/services/VolcengineService.ts - saveCredentials method
On some platforms (especially Linux without a keychain), safeStorage.isEncryptionAvailable() returns false and encryption will fail. The code should check this before attempting encryption.
Suggested fix:
public saveCredentials = async (
_: Electron.IpcMainInvokeEvent,
accessKeyId: string,
secretAccessKey: string
): Promise<void> => {
try {
if (!accessKeyId || !secretAccessKey) {
throw new VolcengineServiceError('Access Key ID and Secret Access Key are required')
}
if (!safeStorage.isEncryptionAvailable()) {
throw new VolcengineServiceError('Secure storage is not available on this platform')
}
// ... rest of the code🔴 Critical Issue 2: Missing File Permission Restriction
File: src/main/services/VolcengineService.ts - saveCredentials method
The credential file should be restricted to owner-only access (mode 0o600) to prevent other users on the system from reading it. This is done in AnthropicService but missing here.
Suggested fix:
await fs.promises.writeFile(this.credentialsFilePath, encryptedData)
await fs.promises.chmod(this.credentialsFilePath, 0o600) // Read/write for owner only
logger.info('Volcengine credentials saved successfully')🔴 Critical Issue 3: Silent Fallback Masks Authentication Errors
File: src/renderer/src/aiCore/legacy/clients/volcengine/VolcengineAPIClient.ts - listModels method
The current catch block falls back to super.listModels() for ALL errors, including credential corruption, authentication failures (401/403), and rate limiting. This hides real problems from users who will see "wrong" models without knowing their Volcengine integration is broken.
Suggested fix: Differentiate between expected fallback conditions and errors that should be surfaced:
} catch (error) {
logger.error('Failed to list Volcengine models:', error as Error)
const errorMessage = error instanceof Error ? error.message : String(error)
// Credential errors should not fall back - user must fix
if (errorMessage.includes('could not be loaded') || errorMessage.includes('credentials')) {
window.toast?.error('Volcengine credentials error. Please re-enter your credentials in settings.')
throw error
}
// Auth errors should not fall back
if (errorMessage.includes('401') || errorMessage.includes('403')) {
window.toast?.error('Volcengine authentication failed. Please verify your Access Key.')
throw error
}
// Only fall back for transient network errors
window.toast?.warning('Temporarily unable to fetch Volcengine models. Using fallback.')
return super.listModels()
}Other Recommendations (Non-blocking)
- Add tests for HMAC-SHA256 signing - The cryptographic signing implementation is critical and should have tests with known test vectors
- Handle useEffect errors in VolcengineSettings.tsx - Line 26-28, the
.then()chain should have a.catch()handler - Consider adding
logger.error()in UI catch blocks - VolcengineSettings.tsx error handlers don't log full error context
Overall the implementation quality is good and follows project patterns. Just needs these security fixes before merge. 👍
What this PR does
support volcengine list mdoel
Fixes #11142