Skip to content

Conversation

@DeJeune
Copy link
Collaborator

@DeJeune DeJeune commented Nov 26, 2025

  • 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.

What this PR does

support volcengine list mdoel

Fixes #11142

- 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.
@DeJeune DeJeune requested a review from 0xfullex as a code owner November 26, 2025 17:16
0xfullex

This comment was marked as resolved.

- 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]>
@DeJeune DeJeune requested review from 0xfullex and EurFelux November 29, 2025 12:48
@DeJeune DeJeune modified the milestones: v1.7.1, v1.7.2 Nov 30, 2025
Copy link
Collaborator

@0xfullex 0xfullex left a 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)

  1. Add tests for HMAC-SHA256 signing - The cryptographic signing implementation is critical and should have tests with known test vectors
  2. Handle useEffect errors in VolcengineSettings.tsx - Line 26-28, the .then() chain should have a .catch() handler
  3. 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. 👍

@kangfenmao kangfenmao modified the milestones: v1.7.2, v1.7.3 Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Volcano Engine cannot fetch model list

4 participants