feat(api): Enhance API key handling and masking in image generation #7220
+67
−22
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…onfig - Implement logic to preserve existing API key if not explicitly changed. - Add a utility function to mask API keys for security in responses. - Update frontend to reflect changes in API key handling and validation logic. - Ensure API key change status is tracked and passed to backend for configuration updates.
Contributor
Greptile SummaryThis PR enhances API key handling and security for image generation configuration by implementing masking in API responses and preserving existing keys unless explicitly changed. The backend correctly implements key masking and preservation logic, and the service layer properly supports the new Key Changes:
Issues Found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Form as Form Component
participant Service as API Service
participant Handler as API Handler
rect rgb(200, 220, 255)
note over User,Handler: Issue: Masked Key Detection
User->>Form: Open edit form
Form->>Form: Load masked initial value
User->>Form: Submit unchanged
Form->>Form: Compare current with initial
Form->>Form: Values differ (false positive)
Form->>Service: Send test request
Service->>Handler: Validate masked key
Handler->>Service: Validation fails
end
rect rgb(200, 255, 220)
note over User,Handler: Correct: User Changes Key
User->>Form: Edit key field
Form->>Form: Track value change
User->>Form: Submit
Form->>Form: Detect actual modification
Form->>Service: Send test with new key
Service->>Handler: Validate actual key
Handler->>Service: Validation succeeds
Form->>Service: Send update with flag
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Additional Comments (1)
-
web/src/app/admin/configuration/image-generation/forms/ImageGenFormWrapper.tsx, line 213-218 (link)style: When
apiKeyChangedis false in edit mode, a masked API key value may be sent inpayload.apiKey(e.g., "key1****key4"). While the backend correctly ignores this due to theapi_key_changedflag, it's clearer to explicitly sendnullfor the API key when it hasn't changed to make the intent explicit and prevent potential future bugs if this logic is refactored.
5 files reviewed, 2 comments
Comment on lines
185
to
206
| // New credentials mode - check if API key was changed from initial (masked) value | ||
| const initialApiKey = (mergedInitialValues as Record<string, unknown>) | ||
| .api_key as string | undefined; | ||
| const apiKeyChanged = apiKeyValue !== initialApiKey; | ||
|
|
||
| if (!result.ok) { | ||
| setApiStatus("error"); | ||
| setErrorMessage(result.errorMessage || "API key validation failed"); | ||
| setIsSubmitting(false); | ||
| return; | ||
| // Test the API key first (only if changed or creating new config) | ||
| if (apiKeyChanged) { | ||
| const result = await testImageGenerationApiKey(payload.modelName, { | ||
| provider: payload.provider, | ||
| apiKey: payload.apiKey, | ||
| apiBase: payload.apiBase, | ||
| apiVersion: payload.apiVersion, | ||
| deploymentName: payload.deploymentName, | ||
| }); | ||
|
|
||
| if (!result.ok) { | ||
| setApiStatus("error"); | ||
| setErrorMessage(result.errorMessage || "API key validation failed"); | ||
| setIsSubmitting(false); | ||
| return; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
logic: In edit mode with a masked API key (e.g., "key1key4"), comparing the form's current value to initialApiKey will incorrectly detect a change when the user hasn't modified the field. This causes unnecessary API testing of a masked key, which will fail. The logic should explicitly check if the value looks like a masked key (contains "") to properly detect actual user changes vs. the initial masked value.
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/app/admin/configuration/image-generation/forms/ImageGenFormWrapper.tsx
Line: 185:206
Comment:
**logic:** In edit mode with a masked API key (e.g., "key1****key4"), comparing the form's current value to `initialApiKey` will incorrectly detect a change when the user hasn't modified the field. This causes unnecessary API testing of a masked key, which will fail. The logic should explicitly check if the value looks like a masked key (contains "****") to properly detect actual user changes vs. the initial masked value.
How can I resolve this? If you propose a fix, please make it concise.
…e generation config - Update logic to preserve existing API key when not explicitly changed and provided key is masked. - Adjust tests to verify that masked API keys are correctly handled in responses. - Refine frontend validation to check for changes against masked API key values.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/onyx/server/manage/image_generation/api.py">
<violation number="1" location="backend/onyx/server/manage/image_generation/api.py:387">
P1: Logic flaw: If `api_key_changed=False` but the provided key is None or doesn't contain `****`, the existing key won't be preserved. This can cause unexpected 400 errors or accidental key overwrites. Consider also preserving when the key is None:
```python
if not config_update.api_key_changed and (not config_update.api_key or provided_key_is_masked):
Or simply trust the api_key_changed flag as before, and separately validate that masked keys shouldn't be saved.
</details>
<sub>Reply with feedback, questions, or to request a fix. Tag `@cubic-dev-ai` to re-run a review.</sub>
| provided_key_is_masked = ( | ||
| config_update.api_key and "****" in config_update.api_key | ||
| ) | ||
| if not config_update.api_key_changed and provided_key_is_masked: |
Contributor
There was a problem hiding this comment.
P1: Logic flaw: If api_key_changed=False but the provided key is None or doesn't contain ****, the existing key won't be preserved. This can cause unexpected 400 errors or accidental key overwrites. Consider also preserving when the key is None:
if not config_update.api_key_changed and (not config_update.api_key or provided_key_is_masked):Or simply trust the api_key_changed flag as before, and separately validate that masked keys shouldn't be saved.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/onyx/server/manage/image_generation/api.py, line 387:
<comment>Logic flaw: If `api_key_changed=False` but the provided key is None or doesn't contain `****`, the existing key won't be preserved. This can cause unexpected 400 errors or accidental key overwrites. Consider also preserving when the key is None:
```python
if not config_update.api_key_changed and (not config_update.api_key or provided_key_is_masked):
Or simply trust the api_key_changed flag as before, and separately validate that masked keys shouldn't be saved.
✅ Addressed in 1b2bd4a
…config - Update conditions for preserving existing API key to include checks for empty or masked keys. - Ensure consistency in handling API key changes and improve clarity in the code logic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request improves the handling and security of API keys for image generation provider configuration, both in the backend and frontend. The changes ensure that API keys are masked in API responses, preserve existing keys unless explicitly changed. Additionally, the frontend now tracks whether the API key was actually modified, and the backend logic uses this information to determine which key to use.
How Has This Been Tested?
Tested from UI
Additional Options
Summary by cubic
Enhanced API key handling for image generation configs: keys are masked in responses, and existing keys are preserved unless a change is confirmed. This reduces accidental key overwrites and avoids unnecessary validation calls.
New Features
Bug Fixes
Written for commit 1b2bd4a. Summary will update on new commits.