fix: cover llm config setup for all robot types#1128
Open
RohitR311 wants to merge 2 commits into
Open
Conversation
RohitR311
added
Type: Bug
WalkthroughThe PR adds prompt LLM fields to robot create and edit flows, encrypts stored API keys, removes them from recording responses, decrypts them for LLM execution, and updates Ollama host access defaults. ChangesPrompt LLM configuration and key handling
Sequence Diagram(s)Prompt LLM key storage sequenceDiagram
participant RobotCreate as src/components/robot/pages/RobotCreate.tsx
participant StorageAPI as src/api/storage.ts
participant RecordingRoutes as server/src/routes/storage.ts
participant AuthUtils as server/src/utils/auth
RobotCreate->>StorageAPI: create robot with promptLlmProvider/model/apiKey/baseUrl
StorageAPI->>RecordingRoutes: POST or PUT prompt LLM fields
RecordingRoutes->>AuthUtils: encrypt(promptLlmApiKey)
AuthUtils-->>RecordingRoutes: encrypted apiKey
RecordingRoutes-->>StorageAPI: sanitized robot metadata
Prompt LLM key use at runtime sequenceDiagram
participant RecordAPI as server/src/api/record.ts
participant TaskRunner as server/src/task-runner.ts
participant Scheduler as server/src/workflow-management/scheduler/index.ts
participant AuthUtils as server/src/utils/auth
RecordAPI->>AuthUtils: safeDecrypt(promptLlmApiKey)
TaskRunner->>AuthUtils: safeDecrypt(promptLlmApiKey)
Scheduler->>AuthUtils: safeDecrypt(promptLlmApiKey)
AuthUtils-->>RecordAPI: decrypted apiKey
AuthUtils-->>TaskRunner: decrypted apiKey
AuthUtils-->>Scheduler: decrypted apiKey
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@server/src/api/record.ts`:
- Line 902: The `promptLlmApiKey` handling in `record.ts`, `scheduler/index.ts`,
and `task-runner.ts` assumes every stored value is already in `iv:ciphertext`
form, so plaintext legacy values will make `decrypt` throw. Add a shared
safe-decrypt helper (or equivalent validation) that checks the format before
calling `decrypt`, and use it at the affected call sites like the `apiKey`
mapping in `record.ts`; also add a migration/backfill to re-encrypt existing
plaintext `promptLlmApiKey` records so the new helper only serves as
compatibility fallback.
In `@server/src/api/sdk.ts`:
- Line 290: The promptLlmApiKey persistence path in sdk.ts currently calls
encrypt() even when encryption is not properly configured, which can create
unrecoverable ciphertext due to the helper’s fallback key behavior. Update the
workflow save flow around the promptLlmApiKey handling to fail fast before
writing, either by making encrypt() throw on missing/invalid ENCRYPTION_KEY or
by checking configuration in the save path and rejecting the request. Ensure the
logic that builds the persisted meta object only stores encrypted data when a
real, stable encryption key is available.
In `@server/src/routes/storage.ts`:
- Around line 542-549: The summary/non-Ollama API-key check in the storage route
is only applied for crawl and search robots, so scrape edits can still save an
invalid summary configuration. Update the validation in the storage route’s
edit/update flow to include robotType === 'scrape' alongside the existing
crawl/search check, and make sure the same promptLlmApiKey / recording_meta
fallback logic is used where the scrape robot is edited. Keep the error response
consistent with the existing summary-output validation.
- Around line 558-559: The create/update storage paths are persisting
promptLlmApiKey into recording_meta and then returning or capturing the robot
metadata without stripping it. Update the relevant create/update handlers around
capture() and the response payload builders to run the metadata through the
existing sanitizer before it leaves the server boundary, ensuring
promptLlmApiKey is removed even when encrypted. Use the same sanitizer
consistently in the affected storage route flows so analytics and API responses
never include this field.
- Around line 391-392: The update validation and persistence logic in storage.ts
is using truthy checks, so explicit empty-string clears for LLM settings are
treated as no-op and the old encrypted value remains. Update the request
presence checks in the update handler to use field-presence detection (for
example via hasOwnProperty or !== undefined) around the existing validation and
the LLM assignment path, and in the persistence logic for
promptLlmApiKey/promptLlmBaseUrl/promptLlmProvider/promptLlmModel ensure
explicit clears remove the stored value instead of skipping it.
In `@server/src/task-runner.ts`:
- Line 293: Legacy stored API keys can be raw values, so direct decrypt() calls
in task-runner and the matching record/scheduler call sites will throw on
non-encrypted keys. Add a shared helper like getPromptLlmApiKey(meta) with an
encrypted-format check (e.g. ENCRYPTED_PROMPT_KEY_RE) that decrypts only
IV:ciphertext values and otherwise returns the stored raw key. Then replace the
direct decrypt(...) usages in taskRunner-related code paths with this helper so
legacy robot configurations remain backward compatible.
In `@server/src/workflow-management/scheduler/index.ts`:
- Line 380: The scheduler in index.ts is calling decrypt() directly on
promptLlmApiKey in multiple places, which breaks legacy configs stored as
plaintext. Update the apiKey handling in the scheduler flow (the recording/robot
config lookups around the affected promptLlmApiKey assignments) to first detect
whether the value is in encrypted iv:encrypted form and only then call
decrypt(), otherwise use the raw string. Prefer a shared safeDecrypt helper or
equivalent logic so the same backward-compatible fallback is applied
consistently at each promptLlmApiKey access site.
In `@src/components/robot/pages/RobotCreate.tsx`:
- Around line 453-455: The summary API-key validation in RobotCreate is blocking
empty Anthropic/OpenAI-compatible keys, but the field copy still implies the key
is optional, so update the helper/label text for the summary provider inputs to
match the validation. Review the summary-related form sections in RobotCreate
and align the copy for the crawlSummaryLlmProvider-dependent API key fields so
they clearly indicate a key is required unless the provider is ollama, keeping
the wording consistent wherever those summary inputs are rendered.
In `@src/components/robot/pages/RobotEditPage.tsx`:
- Around line 1476-1482: The current LLM key validation in RobotEditPage allows
reusing a previously stored encrypted key when the provider changes or the
OpenAI-compatible base URL changes. Update the validation around
shouldShowLlmConfig(), llmProvider, and llmApiKey so a saved key is only
accepted when both the provider and base URL match the existing
robot.recording_meta.promptLlmProvider and stored base URL state; otherwise
require the user to enter a new key and block save. Apply the same check in both
affected validation paths, including the one near the submit/save logic
referenced by the duplicate section.
- Around line 1317-1324: The shouldShowLlmConfig helper in RobotEditPage.tsx is
too restrictive for scrape robots, since it only shows the LLM section when
recording_meta.promptInstructions exists. Update that scrape branch so
summary-only scrape robots created by RobotCreate also return true when their
output format indicates summary, using the existing robot?.recording_meta and
output-format checks already used for crawl/search. Keep the change localized to
shouldShowLlmConfig so edit/persist flows can surface the summary LLM config for
scrape robots without requiring Smart Query promptInstructions.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 198a1baf-fecf-43a6-930c-3ff8c7f9fcd5
📒 Files selected for processing (9)
docker-compose.ymlserver/src/api/record.tsserver/src/api/sdk.tsserver/src/routes/storage.tsserver/src/task-runner.tsserver/src/workflow-management/scheduler/index.tssrc/api/storage.tssrc/components/robot/pages/RobotCreate.tsxsrc/components/robot/pages/RobotEditPage.tsx
| ...((workflowFile.meta as any).promptLlmProvider ? { promptLlmProvider: (workflowFile.meta as any).promptLlmProvider } : {}), | ||
| ...((workflowFile.meta as any).promptLlmModel ? { promptLlmModel: (workflowFile.meta as any).promptLlmModel } : {}), | ||
| ...((workflowFile.meta as any).promptLlmApiKey ? { promptLlmApiKey: (workflowFile.meta as any).promptLlmApiKey } : {}), | ||
| ...((workflowFile.meta as any).promptLlmApiKey ? { promptLlmApiKey: encrypt((workflowFile.meta as any).promptLlmApiKey) } : {}), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail fast when encryption is not configured.
Line 290 persists a key encrypted by encrypt(), but that helper silently generates a random fallback key when ENCRYPTION_KEY is missing/invalid. That makes the stored prompt key unrecoverable on later decrypts or after restart; reject the request or make the helper throw before persisting ciphertext.
🤖 Prompt for 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.
In `@server/src/api/sdk.ts` at line 290, The promptLlmApiKey persistence path in
sdk.ts currently calls encrypt() even when encryption is not properly
configured, which can create unrecoverable ciphertext due to the helper’s
fallback key behavior. Update the workflow save flow around the promptLlmApiKey
handling to fail fast before writing, either by making encrypt() throw on
missing/invalid ENCRYPTION_KEY or by checking configuration in the save path and
rejecting the request. Ensure the logic that builds the persisted meta object
only stores encrypted data when a real, stable encryption key is available.
Comment on lines
+1476
to
+1482
| if (shouldShowLlmConfig() && llmProvider !== 'ollama' && !llmApiKey.trim()) { | ||
| const previouslyHadNonOllamaKey = robot.recording_meta.promptLlmProvider && robot.recording_meta.promptLlmProvider !== 'ollama'; | ||
| if (!previouslyHadNonOllamaKey) { | ||
| notify("error", `An API key is required when using ${llmProvider === 'anthropic' ? 'Anthropic' : 'an OpenAI-compatible'} provider`); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require a new key when the provider or base URL changes.
Because stored keys are not re-entered in the form, this validation can reuse an existing encrypted key after switching Anthropic/OpenAI-compatible providers or changing the OpenAI-compatible base URL. That saves the new provider with the old provider’s credential.
Proposed fix
- if (shouldShowLlmConfig() && llmProvider !== 'ollama' && !llmApiKey.trim()) {
- const previouslyHadNonOllamaKey = robot.recording_meta.promptLlmProvider && robot.recording_meta.promptLlmProvider !== 'ollama';
- if (!previouslyHadNonOllamaKey) {
+ if (shouldShowLlmConfig() && llmProvider !== 'ollama' && !llmApiKey.trim()) {
+ const previousBaseUrl = robot.recording_meta.promptLlmBaseUrl || '';
+ const canReuseStoredKey =
+ robot.recording_meta.promptLlmProvider === llmProvider &&
+ previousBaseUrl === llmBaseUrl.trim();
+
+ if (!canReuseStoredKey) {
notify("error", `An API key is required when using ${llmProvider === 'anthropic' ? 'Anthropic' : 'an OpenAI-compatible'} provider`);
return;
}
}Also applies to: 1569-1573
🤖 Prompt for 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.
In `@src/components/robot/pages/RobotEditPage.tsx` around lines 1476 - 1482, The
current LLM key validation in RobotEditPage allows reusing a previously stored
encrypted key when the provider changes or the OpenAI-compatible base URL
changes. Update the validation around shouldShowLlmConfig(), llmProvider, and
llmApiKey so a saved key is only accepted when both the provider and base URL
match the existing robot.recording_meta.promptLlmProvider and stored base URL
state; otherwise require the user to enter a new key and block save. Apply the
same check in both affected validation paths, including the one near the
submit/save logic referenced by the duplicate section.
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 `@server/src/utils/auth.ts`:
- Around line 59-68: safeDecrypt currently swallows decrypt failures and returns
the original encrypted blob, which can leak iv:ciphertext downstream as if it
were a valid API key. Update safeDecrypt to fail fast when decrypt(value) throws
in the decrypt branch, and make callers handle that error instead of falling
back to the ciphertext; use safeDecrypt and decrypt as the key symbols to locate
the change.
- Around line 31-35: `encrypt` and `decrypt` in `server/src/utils/auth.ts`
currently validate `ENCRYPTION_KEY` by length only, which lets non-hex strings
through. Replace the `key.length !== 64` check in both functions with strict hex
validation that requires exactly 64 hexadecimal characters before calling
`Buffer.from(key, 'hex')`. Keep the existing error path and message, but make
sure invalid characters are rejected early using the `getEnvVariable`,
`encrypt`, and `decrypt` flow.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb53f2fb-c5f5-425a-ab3d-b0616f631cee
📒 Files selected for processing (7)
server/src/api/record.tsserver/src/routes/storage.tsserver/src/task-runner.tsserver/src/utils/auth.tsserver/src/workflow-management/scheduler/index.tssrc/components/robot/pages/RobotCreate.tsxsrc/components/robot/pages/RobotEditPage.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- server/src/api/record.ts
- server/src/task-runner.ts
- server/src/routes/storage.ts
- src/components/robot/pages/RobotEditPage.tsx
- server/src/workflow-management/scheduler/index.ts
- src/components/robot/pages/RobotCreate.tsx
Comment on lines
+31
to
35
| const key = getEnvVariable('ENCRYPTION_KEY'); | ||
| if (!key || key.length !== 64) { | ||
| throw new Error('ENCRYPTION_KEY is missing or invalid. Set a 64-character hex string in your .env file.'); | ||
| } | ||
| const keyBuffer = Buffer.from(key, 'hex'); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only check: demonstrates why length-only validation is insufficient.
node - <<'NODE'
for (const candidate of ['g'.repeat(64), 'a'.repeat(63) + 'z']) {
console.log({
inputLength: candidate.length,
decodedByteLength: Buffer.from(candidate, 'hex').length,
});
}
NODERepository: getmaxun/maxun
Length of output: 237
Enforce strict hex validation for ENCRYPTION_KEY
The current length-only check (key.length !== 64) is insufficient. A 64-character string containing invalid hex characters (e.g., 'g') passes the check, but Buffer.from(key, 'hex') converts it to a buffer of incorrect length (e.g., 0 or 31 bytes instead of 32). This causes downstream crypto operations to fail with non-actionable errors.
Replace the length check with a strict regex validation in both encrypt and decrypt functions to ensure the key contains exactly 64 hexadecimal characters:
Suggested fix
- if (!key || key.length !== 64) {
+ if (!/^[0-9a-fA-F]{64}$/.test(key)) {
throw new Error('ENCRYPTION_KEY is missing or invalid. Set a 64-character hex string in your .env file.');
}Apply this change to lines 33 and 49 in server/src/utils/auth.ts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const key = getEnvVariable('ENCRYPTION_KEY'); | |
| if (!key || key.length !== 64) { | |
| throw new Error('ENCRYPTION_KEY is missing or invalid. Set a 64-character hex string in your .env file.'); | |
| } | |
| const keyBuffer = Buffer.from(key, 'hex'); | |
| const key = getEnvVariable('ENCRYPTION_KEY'); | |
| if (!/^[0-9a-fA-F]{64}$/.test(key)) { | |
| throw new Error('ENCRYPTION_KEY is missing or invalid. Set a 64-character hex string in your .env file.'); | |
| } | |
| const keyBuffer = Buffer.from(key, 'hex'); |
🤖 Prompt for 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.
In `@server/src/utils/auth.ts` around lines 31 - 35, `encrypt` and `decrypt` in
`server/src/utils/auth.ts` currently validate `ENCRYPTION_KEY` by length only,
which lets non-hex strings through. Replace the `key.length !== 64` check in
both functions with strict hex validation that requires exactly 64 hexadecimal
characters before calling `Buffer.from(key, 'hex')`. Keep the existing error
path and message, but make sure invalid characters are rejected early using the
`getEnvVariable`, `encrypt`, and `decrypt` flow.
Comment on lines
+59
to
+68
| export const safeDecrypt = (value: string): string => { | ||
| if (value.includes(':')) { | ||
| try { | ||
| return decrypt(value); | ||
| } catch { | ||
| return value; | ||
| } | ||
| } | ||
| return value; | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don’t fall back to ciphertext after a decrypt failure.
Line 64 returns the encrypted blob as the API key when the key is wrong or the value is corrupt; downstream LLM execution then receives iv:ciphertext instead of failing fast.
Suggested fix
export const safeDecrypt = (value: string): string => {
- if (value.includes(':')) {
- try {
- return decrypt(value);
- } catch {
- return value;
- }
+ const encryptedValuePattern = /^[0-9a-fA-F]{32}:[0-9a-fA-F]{32}(?:[0-9a-fA-F]{32})*$/;
+ if (!encryptedValuePattern.test(value)) {
+ return value;
}
- return value;
+ return decrypt(value);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const safeDecrypt = (value: string): string => { | |
| if (value.includes(':')) { | |
| try { | |
| return decrypt(value); | |
| } catch { | |
| return value; | |
| } | |
| } | |
| return value; | |
| }; | |
| export const safeDecrypt = (value: string): string => { | |
| const encryptedValuePattern = /^[0-9a-fA-F]{32}:[0-9a-fA-F]{32}(?:[0-9a-fA-F]{32})*$/; | |
| if (!encryptedValuePattern.test(value)) { | |
| return value; | |
| } | |
| return decrypt(value); | |
| }; |
🤖 Prompt for 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.
In `@server/src/utils/auth.ts` around lines 59 - 68, safeDecrypt currently
swallows decrypt failures and returns the original encrypted blob, which can
leak iv:ciphertext downstream as if it were a valid API key. Update safeDecrypt
to fail fast when decrypt(value) throws in the decrypt branch, and make callers
handle that error instead of falling back to the ciphertext; use safeDecrypt and
decrypt as the key symbols to locate the change.
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
1 participant
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.
What this PR does?
Summary by CodeRabbit