fix: harden the Ask AI CE port after CodeRabbit review - #42092
Merged
Conversation
When the API key test request omitted a key, the server decrypted the saved credential and then sent it to the baseUrl or endpoint supplied in that same request. The UI masks these keys, so anyone who could reach the endpoint could read one back by pointing the test at a collector they control. The saved configuration now decides the destination whenever the key comes from storage. Only fields that cannot change the host — deployment name, API version, model — stay overridable, so an administrator can still try a different deployment against their own resource. A stored credential is also no longer sent anywhere that would put it on the wire in cleartext. Loopback stays exempt so a proxy on the Appsmith host keeps working, and typing a key into the form still tests any endpoint explicitly.
Ciphertext and cleartext are both just strings, and "does it decrypt?" answered the wrong question in both directions. Once the instance encryption password changed, real ciphertext stopped decrypting: it was reported as cleartext, sent to the provider as the API key, and re-encrypted by the next write — after which the original credential was unrecoverable. The administrator saw only "invalid API key". In the other direction, cleartext that happened to decrypt without throwing was treated as already encrypted and stayed in cleartext. Encrypted values now carry an explicit marker. A marked value that will not decrypt raises instead of yielding its own ciphertext, so a changed encryption password surfaces as itself. Migration076 normalises everything written before the marker existed: cleartext is encrypted, and ciphertext from a build that encrypted without marking keeps its bytes and simply gains the marker, so it is never encrypted a second time.
Every AI failure was rewritten to INVALID_PARAMETER, so a provider outage, an invalid API key and a throttled caller all came back as 400. Provider outages stayed out of server-error metrics — the very problem the surrounding comment set out to fix — and a client could not tell "back off and retry" from "you sent something invalid". onErrorMap also dropped the original throwable, losing the stack trace for anything the call sites had not already logged. Failures now keep their own error class, and unexpected ones are chained as the cause rather than discarded. Throttling — both our own limiter and a provider 429 — reports 429 instead of masquerading as a bad request or a server fault. The same treatment applies to updateAIConfig, where a database, serialization or encryption failure had been reported to the caller as invalid input. Authorization denial there was also only half-handled: the permission-aware lookup ends in NO_RESOURCE_FOUND, and only ACL_NO_RESOURCE_FOUND was matched, so a caller without MANAGE_ORGANIZATION got a 404 rather than the intended refusal. Both codes are now recognised. Also drops a duplicate log of the Azure error body. It was already logged one line above, capped at 1000 characters; the second copy was uncapped, so a large provider error body landed in the log in full.
The editor panel let a second request start while the first was still running: the send button and every quick-action chip dispatched independently, and the chips stayed active during a request. Two clicks queued two prompts against a single loading flag — duplicate chat entries and a second billable provider call. All submissions now funnel through one guarded helper, and the controls are disabled while a request is in flight. The quick-action path also loses a 100ms setTimeout it never needed. Alongside that: - Both panels attached mousemove and mouseup to the document when a resize began and only detached them on mouseup. A panel that unmounted mid-drag — which the route-change effect causes directly — left both listeners attached for the rest of the session, calling setPanelWidth on a dead component. - The global panel advertises "Close (Esc)" but only listened on the prompt textarea, so the shortcut did nothing once focus moved to a chip or the response area. - Its Clear Chat chip was the one control not disabled during a request, so a conversation could be cleared while a response was in flight and the saga would append that response to an emptied list. - The editor panel's context label recomputed only when the editor or mode changed, so it kept showing the line the panel opened on while submission sent the real caret position. It now follows the caret. - GraphQL editors run in mode "graphql-js", which isAISupportedMode did not recognise, so Ask AI was disabled there even though getAIContext supported it. Both now share one predicate. - The AI settings test buttons required a newly typed key, leaving an administrator unable to test the key already stored.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates AI editor panels, provider key testing, AI secret normalization, stored-key destination validation, credential binding, and server-side AI error mapping. It adds tests for secret handling and stored-destination API-key checks. ChangesAI assistant updates
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant AdminSettingsAI
participant AIConfigServiceCEImpl
participant AIConfigSecretsCE
participant ProviderEndpoint
AdminSettingsAI->>AIConfigServiceCEImpl: test stored API key
AIConfigServiceCEImpl->>AIConfigSecretsCE: validate saved destination
AIConfigSecretsCE-->>AIConfigServiceCEImpl: allow or reject endpoint
AIConfigServiceCEImpl->>ProviderEndpoint: send request to saved endpoint
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
A marked value that will not decrypt means the instance encryption password changed. That is a server-side condition, so INVALID_PARAMETER was the wrong class for it — the caller got a non-retryable 4xx and the failure stayed out of server-error metrics, which is the same objection this branch already raised against the AIConfigControllerCE mapping. No message is attached, for two reasons. INTERNAL_SERVER_ERROR has no placeholder, so one would be silently dropped. And "re-enter the API key" would point at a fix that resolves the symptom for one credential while leaving every other secret encrypted with the previous password unreadable. The log line carries the real cause instead. Reported by CodeRabbit on appsmith-ee#9417.
subrata71
marked this pull request as ready for review
August 5, 2026 16:11
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/client/src/ce/components/editorComponents/GPT/AISidePanel.tsx`:
- Line 415: Apply the loading guard to every Clear Chat entry point by updating
the AISidePanel and GlobalAISidePanel clear-chat flows: disable the header
ClearChatButton/ClearButton when isLoading is true, and add an early return in
each handleClearChat handler so clear actions are rejected while loading. In
app/client/src/ce/components/editorComponents/GPT/AISidePanel.tsx#L415-L415 and
app/client/src/ce/components/editorComponents/GlobalAISidePanel/index.tsx#L426-L426,
keep the existing quick-action chip behavior consistent with the header
controls.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/AIConfigSecretsCE.java`:
- Around line 90-96: Update the ciphertext migration logic around
EncryptionHelper.decrypt and encrypt so a decryption failure preserves the
original stored value instead of re-encrypting it. Keep the successful decrypt
path adding ENCRYPTED_PREFIX, and ensure Migration076EncryptAIAssistantApiKeys
receives unreadable values unchanged for explicit recovery.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.java`:
- Around line 850-887: Enforce the stored-credential HTTPS policy across the
request paths in AIConfigServiceCEImpl, including callClaudeAPI, callOpenAIAPI,
and callAzureOpenAIAPI, before any stored key is attached. Update
allowsStoredCredential to reject all HTTP destinations, including loopback
addresses, while preserving local HTTP testing only when the key was explicitly
entered in the request rather than loaded from saved configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8079a39-f332-4960-9948-b7d3b6ae7c0c
📒 Files selected for processing (12)
app/client/src/ce/components/editorComponents/GPT/AISidePanel.tsxapp/client/src/ce/components/editorComponents/GPT/trigger.tsxapp/client/src/ce/components/editorComponents/GlobalAISidePanel/index.tsxapp/client/src/pages/AdminSettings/AI/index.tsxapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/AIConfigControllerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/AIConfigSecretsCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration076EncryptAIAssistantApiKeys.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/AIConfigSecretsCETest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/AIConfigServiceCEImplTest.java
…-key request Three review findings, one theme: a stored provider credential must not be destroyed, and must not leave the instance in cleartext. normalizeForStorage encrypted anything that failed to decrypt, treating it as cleartext. If the instance encryption password had changed before the migration ran, valid pre-marker ciphertext failed that decrypt and was encrypted a second time — after which neither the old password nor the new one recovers the key. An unreadable value that still has the shape of our ciphertext (hex-encoded AES-GCM, so 64 hex characters at minimum) is now left untouched and logged for an operator. A cleartext key can only be misread as ciphertext if it is both long enough and hex enough, and the consequence there is that it stays unencrypted rather than being destroyed — the direction this has to fail in. encryptCredentialsInPlace routes through the same decision, so the settings write path cannot nest ciphertext either. The HTTPS destination check only guarded the settings "test key" path, while the Ask AI request path sent stored keys to callClaudeAPI, callOpenAIAPI and callAzureOpenAIAPI with no check at all — a configured http:// provider URL put the credential on the wire in cleartext on every request. The policy now lives in AIConfigSecretsCE.allowsStoredCredential as the single definition, and both paths call it. Loopback stays permitted, since traffic that never reaches a network cannot expose the key, and without it a local LLM proxy is unusable. Clear Chat was disabled as a quick-action chip but still live as a header button in both panels, and neither handler checked. Clearing mid-request empties the list the saga is about to append the in-flight response to, leaving an answer with no question above it. Both controls are disabled and both handlers guarded. Reported by CodeRabbit and Hacktron on appsmith-ee#9417 and #42092.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/AIConfigSecretsCE.java`:
- Around line 95-107: Update allowsStoredCredential and the Azure, Claude, and
OpenAI WebClientUtils call paths so the permitted loopback exception is
reachable only through a client path that validates the hostname before
connection and the resolved remote IP after connection; otherwise remove the
loopback allowance from allowsStoredCredential and update its documentation and
tests accordingly.
- Around line 129-138: Update normalizeForStorage and encryptIfNeeded so a valid
64-character hexadecimal credential is treated as cleartext and encrypted,
rather than classified as unreadable ciphertext and retained unchanged. Preserve
the explicit recovery state for genuinely ambiguous legacy ciphertext. Add
coverage verifying that a 64-character hexadecimal cleartext key is encrypted
and persists correctly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29b64d02-bae5-42c7-8c17-fa159ad1f520
📒 Files selected for processing (6)
app/client/src/ce/components/editorComponents/GPT/AISidePanel.tsxapp/client/src/ce/components/editorComponents/GlobalAISidePanel/index.tsxapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/AIConfigSecretsCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/AIConfigSecretsCETest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/ce/components/editorComponents/GlobalAISidePanel/index.tsx
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.java
- app/client/src/ce/components/editorComponents/GPT/AISidePanel.tsx
Routing encryptIfNeeded through normalizeForStorage borrowed the migration's caution about unreadable ciphertext into a path that must not have it. The shape check cannot tell a 64-character hexadecimal API key from ciphertext, so such a key was left alone and persisted to Mongo in cleartext. The two paths answer different questions. encryptCredentialsInPlace is reached only from PUT /organizations, where the value came from the request body and an unmarked value is therefore always a key the caller just supplied — it is encrypted unconditionally. normalizeForStorage reads values already in storage, where cleartext and pre-marker ciphertext are genuinely indistinguishable once decryption fails, and that is the only place the shape check belongs. Reported by CodeRabbit on appsmith-ee#9417.
Failed server tests
|
Pinning the test request to the saved destination closed only half of this. The same key could still be redirected in two steps: save a new base URL with the key field left blank — blank means "unchanged", so the key survives — and the stored credential now points at a host of the caller's choosing. Testing the key, or simply using the assistant, then sends it there. The settings page masks these keys precisely so an administrator cannot read them back. Changing a provider's base URL or endpoint without supplying a replacement now clears that provider's stored key, so there is nothing left to redirect. The form warns before saving, since the key disappearing silently would be worse than the inconvenience of re-entering it. Also drops the loopback exception from allowsStoredCredential. It was unreachable: these requests all go out through WebClientUtils, whose RestrictedHostFilter denies loopback as a literal host and again after DNS resolution. Permitting http://localhost only described a capability the HTTP layer refuses, so the policy is now HTTPS or nothing. Reported by Hacktron and CodeRabbit on #42092 and appsmith-ee#9417.
| <ButtonRow style={{ marginTop: "8px" }}> | ||
| <Button | ||
| isDisabled={!claudeApiKey} | ||
| isDisabled={!claudeApiKey && !hasStoredClaudeApiKey} |
There was a problem hiding this comment.
The pull request modifies the isDisabled property of the "Test Key" buttons for AI providers (Claude, OpenAI, Azure OpenAI) to allow testing the connection when a stored key exists but no new key has been typed.
However, because the user can modify the "Base URL" or "Endpoint" field in the UI without saving, they can change the destination URL to an attacker-controlled server and click "Test Key". When clicked, the application triggers handleTestApiKey, which sends the modified URL and an undefined API key to the backend. The backend then decrypts the stored API key and sends it in an outbound request to the newly specified arbitrary URL, allowing a malicious or compromised organization administrator to exfiltrate highly sensitive third-party API keys.
While the PR introduces a warning message on save (DestinationChangeWarning), this warning is not displayed or enforced when clicking "Test Key" on unsaved changes, leaving the stored credential vulnerable to silent exfiltration.
Steps to Reproduce
- Navigate to the Admin Settings > AI page as an administrator.
- Select an AI Provider (e.g., Claude) that already has a stored API key.
- Modify the 'Base URL' or 'Endpoint' field to point to an external listener or destination under your control.
- Click the 'Test Key' button.
- Verify if the backend initiates an outbound request to the modified URL containing the decrypted, stored API key.
Fix with AI
A security vulnerability was found by Hacktron.
File: app/client/src/pages/AdminSettings/AI/index.tsx
Lines: 1023
Severity: high
Vulnerability: Stored AI Provider API Key Exfiltration via Test Connection to Arbitrary Base URL
Description:
The pull request modifies the `isDisabled` property of the "Test Key" buttons for AI providers (Claude, OpenAI, Azure OpenAI) to allow testing the connection when a stored key exists but no new key has been typed.
However, because the user can modify the "Base URL" or "Endpoint" field in the UI without saving, they can change the destination URL to an attacker-controlled server and click "Test Key". When clicked, the application triggers `handleTestApiKey`, which sends the modified URL and an `undefined` API key to the backend. The backend then decrypts the stored API key and sends it in an outbound request to the newly specified arbitrary URL, allowing a malicious or compromised organization administrator to exfiltrate highly sensitive third-party API keys.
While the PR introduces a warning message on save (`DestinationChangeWarning`), this warning is not displayed or enforced when clicking "Test Key" on unsaved changes, leaving the stored credential vulnerable to silent exfiltration.
Proof of Concept:
**Steps to Reproduce**
1. Navigate to the Admin Settings > AI page as an administrator.
2. Select an AI Provider (e.g., Claude) that already has a stored API key.
3. Modify the 'Base URL' or 'Endpoint' field to point to an external listener or destination under your control.
4. Click the 'Test Key' button.
5. Verify if the backend initiates an outbound request to the modified URL containing the decrypted, stored API key.
Affected Code:
isDisabled={!claudeApiKey && !hasStoredClaudeApiKey}
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), !accepted_risk <reason>, or !fixed (resolved). Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
Comment on lines
+143
to
+162
| clearKeyIfDestinationChanged( | ||
| previousClaudeBaseUrl, | ||
| assistantConfig.getClaudeBaseUrl(), | ||
| aiConfig.getClaudeApiKey(), | ||
| assistantConfig::setClaudeApiKey); | ||
| clearKeyIfDestinationChanged( | ||
| previousOpenaiBaseUrl, | ||
| assistantConfig.getOpenaiBaseUrl(), | ||
| aiConfig.getOpenaiApiKey(), | ||
| assistantConfig::setOpenaiApiKey); | ||
| clearKeyIfDestinationChanged( | ||
| previousAzureOpenaiEndpoint, | ||
| assistantConfig.getAzureOpenaiEndpoint(), | ||
| aiConfig.getAzureOpenaiApiKey(), | ||
| assistantConfig::setAzureOpenaiApiKey); | ||
| clearKeyIfDestinationChanged( | ||
| previousCopilotEndpoint, | ||
| assistantConfig.getCopilotEndpoint(), | ||
| aiConfig.getCopilotApiKey(), | ||
| assistantConfig::setCopilotApiKey); |
There was a problem hiding this comment.
A logical bypass exists in the AI credential destination binding mechanism (clearKeyIfDestinationChanged) due to unhandled cross-field fallback relationships between Azure OpenAI and Copilot configurations. Specifically, azureOpenaiEndpoint falls back to copilotEndpoint if empty, and azureOpenaiApiKey falls back to copilotApiKey if empty. An attacker with organization management permissions can exploit this fallback logic to change one of the endpoints to an external destination without triggering the credential-clearing logic for the fallback key. When the API key is subsequently tested or used, the decrypted stored credential is sent to the attacker-controlled destination, leading to credential exfiltration.
Fix with AI
A security vulnerability was found by Hacktron.
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.java
Lines: 143-162
Severity: high
Vulnerability: Logical Bypass in AI Credential Destination Binding Allows Leakage of Stored API Keys
Description:
A logical bypass exists in the AI credential destination binding mechanism (clearKeyIfDestinationChanged) due to unhandled cross-field fallback relationships between Azure OpenAI and Copilot configurations. Specifically, azureOpenaiEndpoint falls back to copilotEndpoint if empty, and azureOpenaiApiKey falls back to copilotApiKey if empty. An attacker with organization management permissions can exploit this fallback logic to change one of the endpoints to an external destination without triggering the credential-clearing logic for the fallback key. When the API key is subsequently tested or used, the decrypted stored credential is sent to the attacker-controlled destination, leading to credential exfiltration.
Affected Code:
clearKeyIfDestinationChanged(
previousClaudeBaseUrl,
assistantConfig.getClaudeBaseUrl(),
aiConfig.getClaudeApiKey(),
assistantConfig::setClaudeApiKey);
clearKeyIfDestinationChanged(
previousOpenaiBaseUrl,
assistantConfig.getOpenaiBaseUrl(),
aiConfig.getOpenaiApiKey(),
assistantConfig::setOpenaiApiKey);
clearKeyIfDestinationChanged(
previousAzureOpenaiEndpoint,
assistantConfig.getAzureOpenaiEndpoint(),
aiConfig.getAzureOpenaiApiKey(),
assistantConfig::setAzureOpenaiApiKey);
clearKeyIfDestinationChanged(
previousCopilotEndpoint,
assistantConfig.getCopilotEndpoint(),
aiConfig.getCopilotApiKey(),
assistantConfig::setCopilotApiKey);
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), !accepted_risk <reason>, or !fixed (resolved). Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.java`:
- Around line 83-89: Update AIConfigServiceCEImpl’s credential-destination
handling around the previous endpoint captures and clearKeyIfDestinationChanged:
resolve effective endpoint/key-source pairs before and after the update,
including Azure/Copilot fallback behavior and provider defaults for null
Claude/OpenAI destinations. Clear every stored key whose effective destination
changed unless the request supplies its replacement, covering both Azure/Copilot
fallback directions. Add regression tests for those cases and null-to-default
Claude/OpenAI saves.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f790f7fb-cf85-46c2-bc00-d138cba09e8e
📒 Files selected for processing (5)
app/client/src/pages/AdminSettings/AI/index.tsxapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/AIConfigSecretsCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/AIConfigSecretsCETest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/AIConfigServiceCEImplTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/AIConfigSecretsCETest.java
The previous attempt compared four independent raw field pairs, which got the rule wrong in both directions. It under-enforced. Azure resolves its key as azureOpenaiApiKey then copilotApiKey, and its destination as azureOpenaiEndpoint then copilotEndpoint, and the COPILOT provider reaches the same code. Treating those four fields as two unrelated pairs meant a new Azure endpoint left a stored Copilot key in place, and the read paths then put the two together. It also over-enforced. Stored claudeBaseUrl may be null while the client always submits the provider default, so a save that changed only a model name looked like a destination change and deleted a working key, with no warning shown. Both now resolve the effective destination — the fallback chain the read paths walk, falling back to the provider default — and compare that. When it moves, every key in the group is invalidated unless the same write supplied a replacement for it. The rule also runs on the generic PUT /organizations path. That endpoint merges sparse request fields into the saved document, so a provider URL arriving there would move underneath a key that stays put — the same redirection /ai-config already refuses. Taking whole before/after configurations rather than request fields lets one definition cover both writers. Two smaller defects alongside: validateApiKey now ignores the masked placeholder, which the settings page can echo back and which would otherwise overwrite a working key with bullets; and allowsStoredCredential now requires a host, so "https:///path" no longer passes as a destination. Reviewed against the CWE Top 25 with a second model. Deferred and tracked: persisting each key's bound destination so it survives a change to the default URL constants, and optimistic concurrency for two admins saving at once. Reported by Hacktron and CodeRabbit on #42092 and appsmith-ee#9417.
sondermanish
approved these changes
Aug 6, 2026
2 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
CodeRabbit reviewed appsmith-ee#9417 roughly 30 seconds after its dependency merged, so it saw the stale 39-file diff that still contained the whole Ask AI CE sync from #42065. All 15 findings landed on CE-owned client and server code that is already on
release— none touch the EE shim cleanup that PR actually makes. They are fixed here, in the repo that owns the code.appsmith-ee#9417is unaffected and stays a 10-file EE-only cleanup.The EE repo's own pre-commit hook (
check-ee-only-files.sh) independently confirms the split: it refuses any client change outsidesrc/ee/.Critical — a saved credential could be sent to a caller-chosen host
AIConfigServiceCEImpl.testApiKeyInternaldecrypted the stored provider key when the request omitted one, then sent it to thebaseUrl/endpointsupplied in that same request. Since the UI masks these keys as••••••••, an organization admin could read one back by pointing the test at a collector they control. No transport requirement existed either, so the key could leave over plain HTTP.The endpoint is gated on
MANAGE_ORGANIZATION, so this is admin-only rather than unauthenticated — but it still recovers a secret the product deliberately hides.The saved configuration now decides the destination whenever the key comes from storage. Only fields that cannot change the host — deployment name, API version, model — remain overridable, so an administrator can still test a different deployment against their own resource. A stored credential is no longer sent anywhere that would put it on the wire in cleartext; loopback stays exempt, and typing a key into the form still tests any endpoint explicitly.
Major
AIConfigSecretsCEinferred "is this encrypted?" by attempting a decrypt, which answers the wrong question in both directions. After the instance encryption password changes, real ciphertext stops decrypting — it was reported as cleartext, handed to the provider as the API key, and re-encrypted by the next write, at which point the original was unrecoverable. Conversely, cleartext that happened to decrypt was treated as encrypted and never protected. A marked value that will not decrypt now raises instead of yielding its own ciphertext.Migration076normalises pre-marker values. Cleartext is encrypted; ciphertext written by a build that encrypted without marking keeps its bytes and simply gains the marker, so it is never encrypted twice. The change unit is edited in place rather than stacked because it is in no release tag and has therefore never run.INVALID_PARAMETER, so a provider outage, an invalid key and a throttled caller all returned 400 — outages stayed out of server-error metrics, and clients could not tell "back off" from "bad request". Errors keep their own class now, unexpected ones are chained as the cause, and throttling (ours and the provider's) returns 429.Minor
Resize listeners no longer survive an unmount mid-drag in either panel; Escape closes the global panel wherever focus sits, matching its own "Close (Esc)" tooltip; Clear Chat is disabled during a request; the context label follows the caret instead of showing the line the panel opened on;
graphql-jsis recognised byisAISupportedMode, so Ask AI works in GraphQL editors; the config update path mapsNO_RESOURCE_FOUNDto an authorization refusal rather than a 404; and the Azure error body is no longer logged a second time uncapped.Verification
AIConfigSecretsCETest(9) pins the marker, the round trip, migration normalisation, and the loud failure on a changed encryption password;AIConfigServiceCEImplTest(3 new) pins destination selection by giving the request an HTTPS host and the saved config an HTTP one, so the refusal proves the saved value won.tsc --noEmitreports no errors in any changed file (only the pre-existingpackages/astandpackages/design-systemfailures that are onreleasetoo).Linear: https://linear.app/appsmith/issue/APP-15756
Slack thread: https://theappsmith.slack.com/archives/C09NG5BJ18S/p1785930741904329
Automation
/ok-to-test tags="@tag.All"
Communication
Should the DevRel and Marketing teams inform users about this change?
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/31047449928
Commit: 2adc822
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 05 Aug 2026 22:13:45 UTC
Summary by CodeRabbit
New Features
Bug Fixes