Making API keys completely write only for the frontend#608
Conversation
WalkthroughEncrypted credentials are no longer exposed to the UI or clients. Backend and UI now return masked values "[ENCRYPTED]" for encrypted items, remove decrypt flows, and add UI handling to treat server-provided credentials as immutable until edited. A new isFromBackend flag distinguishes backend-provided vs. user-created credentials. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as APIKeysSection (UI)
participant FE as credentialsService (FE)
participant API as settings_api.get_credential / lists
participant SVC as credential_service
User->>UI: Open API Keys
UI->>FE: fetch credentials by category
FE->>API: GET credentials (list)
API->>SVC: list_all_credentials / get_credentials_by_category
SVC-->>API: Items (encrypted -> value="[ENCRYPTED]", is_encrypted=true)
API-->>FE: Masked credentials
FE-->>UI: Normalized items (value="[ENCRYPTED]")
UI-->>User: Render masked rows (actions disabled)
note over UI,User: Editing masked value
User->>UI: Click value field on masked row
UI->>UI: Clear value, hide value, set isFromBackend=false
UI-->>User: Row becomes editable like user-owned
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
archon-ui-main/src/services/credentialsService.ts (1)
80-86: Add retry with exponential backoff for backend calls.Per guidelines, wrap fetches with retries and clear errors. Apply here and across this service.
Example utility:
async function fetchWithRetry(input: RequestInfo, init?: RequestInit, attempts = 3, baseDelayMs = 300) { let lastErr: unknown; for (let i = 0; i < attempts; i++) { try { const res = await fetch(input, init); if (!res.ok) throw new Error(`HTTP ${res.status}: ${await res.text()}`); return res; } catch (e) { lastErr = e; if (i < attempts - 1) await new Promise(r => setTimeout(r, baseDelayMs * 2 ** i)); } } throw lastErr instanceof Error ? lastErr : new Error(String(lastErr)); }Usage example here:
- const response = await fetch(`${this.baseUrl}/api/credentials`); + const response = await fetchWithRetry(`${this.baseUrl}/api/credentials`);python/src/server/services/credential_service.py (1)
448-457: Use uppercase key for LLM_PROVIDER in set_active_provider
Change the literal in set_active_provider from"llm_provider"to"LLM_PROVIDER"so it matches the getters in lines 395 and 418, as well as theprovider_credentialslisting.
Location: python/src/server/services/credential_service.py:453
🧹 Nitpick comments (7)
archon-ui-main/src/services/credentialsService.ts (1)
103-110: Centralize the “[ENCRYPTED]” sentinel to avoid drift.Hardcoding the sentinel increases the risk of FE/BE mismatch. Use a shared constant and reuse it here.
Add a shared constant (e.g., archon-ui-main/src/constants/security.ts):
export const ENCRYPTED_PLACEHOLDER = "[ENCRYPTED]";Then here:
- value: "[ENCRYPTED]", + value: ENCRYPTED_PLACEHOLDER,python/src/server/services/credential_service.py (1)
306-310: LGTM: encrypted values are masked.This prevents plaintext exposure. Consider a module-level constant to keep FE/BE in sync.
Example:
ENCRYPTED_PLACEHOLDER = "[ENCRYPTED]" # ... credentials[key] = { "value": ENCRYPTED_PLACEHOLDER, "is_encrypted": True, "description": item["description"], }python/src/server/api_routes/settings_api.py (1)
166-177: Consider including metadata for non-encrypted values as well (consistency).Return category/description for both cases to keep the shape uniform. Optional, but helps clients.
Example:
-# For non-encrypted credentials, return the actual value -return {"key": key, "value": value, "is_encrypted": False} +# For non-encrypted credentials, include metadata if available +return { + "key": key, + "value": value, + "is_encrypted": False, + "category": value.get("category") if isinstance(value, dict) else None, + "description": value.get("description") if isinstance(value, dict) else None, +}archon-ui-main/src/components/settings/APIKeysSection.tsx (4)
55-71: Remove the unused isEncryptedFromBackend local.Dead code trips linters and confuses readers.
- const uiCredentials = apiKeys.map(cred => { - const isEncryptedFromBackend = cred.is_encrypted && cred.value === '[ENCRYPTED]'; - - return { + const uiCredentials = apiKeys.map(cred => { return {
315-319: Clarify tooltip wording to reflect “on save” semantics (not live decrypt).Avoid implying decryption will reveal the value.
- : cred.is_encrypted ? 'Encrypted - click to decrypt' : 'Not encrypted - click to encrypt' + : cred.is_encrypted + ? 'Stored encrypted — click to store as plaintext on save' + : 'Stored plaintext — click to store encrypted on save'
268-276: Consider a shared ENCRYPTED_PLACEHOLDER constant.You compare against '[ENCRYPTED]' in multiple places; use a single constant to avoid drift and ease future changes.
Add near top:
const ENCRYPTED_PLACEHOLDER = '[ENCRYPTED]';Then replace string literals in comparisons and placeholders.
Also applies to: 289-293, 305-319
23-26: Align state names with guidelines (optional).Rename to isLoading/isSaving for consistency with “is[Action]ing”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
archon-ui-main/src/components/settings/APIKeysSection.tsx(7 hunks)archon-ui-main/src/services/credentialsService.ts(1 hunks)python/src/server/api_routes/settings_api.py(2 hunks)python/src/server/services/credential_service.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
python/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/src/**/*.py: Fail fast on service startup failures, missing configuration, database connection failures, authentication/authorization failures, critical dependencies unavailable, and invalid/corrupting data
Never accept or store corrupted data; on operation failure skip the item entirely rather than writing placeholders (e.g., zero embeddings)
For batch/background operations, continue processing but log each failure with details; track both successes and failures
Use specific exception types (avoid bare Exception), include context/IDs/URLs in messages, preserve full stack traces with logging (exc_info=True), and never return None/null to indicate failure—raise with details
Use database task status values directly: todo, doing, review, done
Target Python 3.12 style with 120-character line length; use Ruff for linting and Mypy for type checking
python/src/**/*.py: Target Python 3.12 and keep lines within 120 characters
Use Ruff for linting (errors, warnings, unused imports, code style) and keep code Ruff-clean
Use Mypy for type checking; maintain type-safe code across backend
Files:
python/src/server/api_routes/settings_api.pypython/src/server/services/credential_service.py
python/src/server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI exception handlers to return rich error responses with appropriate HTTP status codes and typed error payloads
Files:
python/src/server/api_routes/settings_api.pypython/src/server/services/credential_service.py
python/src/{server,agents,mcp}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
python/src/{server,agents,mcp}/**/*.py: Fail fast on service startup failures (credentials, DB, or service init) with clear errors
Treat missing configuration (env vars/invalid settings) as fatal and stop execution
Do not suppress database connection failures; bubble up immediately
Authentication/authorization failures must halt the operation and be clearly surfaced
Data corruption or validation errors should raise (use Pydantic to enforce), never silently accept
If a critical dependency is unavailable, fail immediately
Never persist invalid data that would corrupt state (e.g., zero embeddings, null FKs, malformed JSON)
Batch processing should continue, logging detailed errors per item; always return both successes and failures
Background tasks (e.g., embedding generation) should finish queues and log failures without crashing the whole process
Include context and IDs/URLs in error messages; preserve full stack traces with logging (exc_info=True)
Use specific exception types; avoid catching bare Exception
Never return None to signal failure; raise exceptions with details
For crawling/document processing: continue batches, skip failed items entirely, and log detailed errors; never store placeholders (e.g., zeroed embeddings)
Files:
python/src/server/api_routes/settings_api.pypython/src/server/services/credential_service.py
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use state naming conventions: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
archon-ui-main/src/**/*.{ts,tsx}: WebSocket event failures should be logged and not crash serving other clients
External API calls should retry with exponential backoff and ultimately fail with a clear, specific message
Include actionable context in frontend error logs/messages (what was attempted, relevant IDs/URLs)
Never return null to signal failure in async/data flows; throw errors with details
Use polling (HTTP) with provided hooks (usePolling, useDatabaseMutation, useProjectMutation); Socket.IO is removed
State naming: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
Persist theme choice in localStorage and respect Tailwind dark mode classes across components
Files:
archon-ui-main/src/services/credentialsService.tsarchon-ui-main/src/components/settings/APIKeysSection.tsx
archon-ui-main/src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
archon-ui-main/src/services/**/*.ts: Frontend service method naming: get[Resource]sByProject(projectId), getResource, create/update/delete[Resource]
Use GET /api/projects/{id}/tasks (not getTasks) for project tasks
Files:
archon-ui-main/src/services/credentialsService.ts
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid comment keywords like LEGACY, CHANGED, REMOVED; write comments that document current functionality only
Files:
archon-ui-main/src/services/credentialsService.tsarchon-ui-main/src/components/settings/APIKeysSection.tsx
archon-ui-main/src/components/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint (standard React rules) for legacy UI code in /src/components
Files:
archon-ui-main/src/components/settings/APIKeysSection.tsx
archon-ui-main/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Legacy components live under src/components and may be used by pages; add related tests under archon-ui-main/test
Files:
archon-ui-main/src/components/settings/APIKeysSection.tsx
python/src/server/services/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Backend service method naming mirrors CRUD patterns: get/create/update/delete with clear resource scoping
Files:
python/src/server/services/credential_service.py
🧬 Code graph analysis (2)
python/src/server/api_routes/settings_api.py (1)
python/src/server/services/credential_service.py (2)
get_credential(158-175)get_credential(467-469)
archon-ui-main/src/components/settings/APIKeysSection.tsx (1)
archon-ui-main/src/services/credentialsService.ts (1)
updateCredential(222-247)
🔇 Additional comments (7)
python/src/server/services/credential_service.py (1)
334-341: LGTM: list output masks and omits encrypted_value.No plaintext nor ciphertext is leaked in list views. Good alignment with PR goal.
python/src/server/api_routes/settings_api.py (1)
142-149: Never decrypt over HTTP — good change.Forcing decrypt=False ensures the single-credential endpoint stays write-only for secrets.
archon-ui-main/src/components/settings/APIKeysSection.tsx (5)
19-20: LGTM: track backend-origin for write-only semantics.isFromBackend cleanly distinguishes masked, backend-provisioned credentials.
105-111: LGTM: editing a masked value clears placeholder and flips ownership.This avoids sending the sentinel back to the server and matches write-only behavior.
118-124: LGTM: prevent viewing masked backend credentials.Good UX guard with a clear warning.
127-133: LGTM: disable encryption toggle for masked credentials.Prevents misleading “decrypt” actions on write-only data.
396-397: LGTM: security notice matches new write-only flow.Clear guidance for users about masked/edited behavior.
| const uiCredentials = apiKeys.map(cred => { | ||
| const isEncryptedFromBackend = cred.is_encrypted && cred.value === '[ENCRYPTED]'; | ||
|
|
||
| return { | ||
| key: cred.key, | ||
| value: cred.value || '', | ||
| description: cred.description || '', | ||
| originalValue: cred.value || '', | ||
| originalKey: cred.key, // Track original key for updates | ||
| hasChanges: false, | ||
| is_encrypted: cred.is_encrypted || false, | ||
| showValue: false, | ||
| isNew: false, | ||
| isFromBackend: !cred.isNew, // Mark as from backend unless it's a new credential | ||
| }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
Block saves/renames while value is masked to prevent persisting “[ENCRYPTED]”.
If a user renames or saves without editing, you’ll POST/PUT the literal placeholder, corrupting the real secret. Guard early.
for (const cred of customCredentials) {
if (cred.hasChanges || cred.isNew) {
if (!cred.key) {
showToast('Key name cannot be empty', 'error');
hasErrors = true;
continue;
}
+ // Prevent persisting the placeholder for backend-encrypted items
+ if (cred.isFromBackend && cred.is_encrypted && cred.value === '[ENCRYPTED]') {
+ showToast(`Edit ${cred.key} to provide a new value before saving or renaming.`, 'warning');
+ hasErrors = true;
+ continue;
+ }Optionally, also disable key renames for masked items (prevents confusion):
<input
type="text"
value={cred.key}
onChange={(e) => updateCredential(index, 'key', e.target.value)}
disabled={cred.isFromBackend && cred.is_encrypted && cred.value === '[ENCRYPTED]'}
className="w-full px-3 py-2 rounded-md bg-white dark:bg-gray-900 border border-gray-300 dark:border-gray-700 text-sm font-mono"
/>Also applies to: 153-210
There was a problem hiding this comment.
No way to save something without editing it first
There was a problem hiding this comment.
@coleam00 You're right that the hasChanges flag only gets set when specific fields are modified. However, there's still a potential edge case I was concerned about:
If a user renames the key of a masked credential (changes cred.key), that sets hasChanges = true, but the value remains [ENCRYPTED]. Then when they save, the system would attempt to persist that placeholder string as the actual credential value.
Looking at the code:
if (field === 'key' || field === 'value' || field === 'is_encrypted') {
updated.hasChanges = true;
}So while you can't save without some edit, you could potentially save after just renaming the key, which would be problematic.
That said, if this scenario seems unlikely in practice or if you prefer to handle it differently (like showing a more targeted error when it happens), I understand. The core security goal of not exposing decrypted values is already achieved.
There was a problem hiding this comment.
@coleam00 You're absolutely right! Looking at the code flow more carefully:
- Masked credentials start with
hasChanges: false - The save logic only processes items where
hasChanges || isNewis true - To get
hasChanges: true, the credential must be edited viaupdateCredential() - When editing a masked credential's value, the code automatically clears the
"[ENCRYPTED]"placeholder and setsisFromBackend: false
So there's indeed no pathway to save the placeholder - the existing logic already prevents this issue. Thanks for the clarification!
🐰✨ happy rabbit ears perk up with understanding
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
Looks good! |
Fix: WorkflowExecution stale state on runId change and back button escape (#580)
…e-580-v2 Fix: WorkflowExecution stale state on runId change and back button escape (coleam00#580)
…e-580-v2 Fix: WorkflowExecution stale state on runId change and back button escape (coleam00#580)
3 participants
Pull Request
Summary
Making API keys completely write only for the frontend. Before this PR, there was a GET request to the credentials/ endpoint that was returning API keys in plain text over HTTP.
Changes Made
Type of Change
Affected Services
Testing
Test Evidence
Tested by resetting my DB, setting the key for OpenAI and making sure crawls still work. Did the same thing on an existing Archon setup as well to make sure this doesn't affect anyone with Archon already up and running.
Checklist
Summary by CodeRabbit