feat: Api Keys database encryption.#33
Conversation
|
@denisgmarques is attempting to deploy a commit to the Thales Laray Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis pull request implements end-to-end encryption for AI provider API keys stored in the database. A new encryption module using AES-256-CBC is introduced alongside environment configuration. API routes are updated to decrypt keys when retrieving from storage and encrypt keys when persisting. Database migrations document encrypted column storage. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/settings/ai/route.ts (1)
81-91:⚠️ Potential issue | 🟠 MajorAdd error handling for decryption failures in GET.
The
decrypt()calls on lines 85-87 are not wrapped in try/catch, unlike the pattern inapp/api/ai/chat/route.tsandapp/api/ai/actions/route.ts. If a stored key is corrupted, uses an old format, or was never encrypted (pre-migration data), this will throw an unhandled exception and return a 500 without a meaningful error message.Suggested fix
+ let aiGoogleKey = ''; + let aiOpenaiKey = ''; + let aiAnthropicKey = ''; + try { + aiGoogleKey = orgSettings?.ai_google_key ? decrypt(orgSettings.ai_google_key) : ''; + aiOpenaiKey = orgSettings?.ai_openai_key ? decrypt(orgSettings.ai_openai_key) : ''; + aiAnthropicKey = orgSettings?.ai_anthropic_key ? decrypt(orgSettings.ai_anthropic_key) : ''; + } catch (e) { + console.error('[api/settings/ai] Failed to decrypt API keys:', e); + return json({ error: 'Failed to decrypt stored API keys' }, 500); + } + return json({ aiEnabled, aiProvider: (orgSettings?.ai_provider || 'google') as Provider, aiModel: orgSettings?.ai_model || AI_DEFAULT_MODELS.google, - aiGoogleKey: orgSettings?.ai_google_key ? decrypt(orgSettings.ai_google_key) : '', - aiOpenaiKey: orgSettings?.ai_openai_key ? decrypt(orgSettings.ai_openai_key) : '', - aiAnthropicKey: orgSettings?.ai_anthropic_key ? decrypt(orgSettings.ai_anthropic_key) : '', + aiGoogleKey, + aiOpenaiKey, + aiAnthropicKey, aiHasGoogleKey: Boolean(orgSettings?.ai_google_key), aiHasOpenaiKey: Boolean(orgSettings?.ai_openai_key), aiHasAnthropicKey: Boolean(orgSettings?.ai_anthropic_key), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/settings/ai/route.ts` around lines 81 - 91, The decrypt(...) calls for aiGoogleKey, aiOpenaiKey, and aiAnthropicKey can throw and need try/catch: wrap each decrypt(orgSettings.ai_<provider>_key) used to build aiGoogleKey/aiOpenaiKey/aiAnthropicKey in a try/catch, on error log the exception (include context like org id or orgSettings) and set the returned key to '' and its corresponding aiHas... flag to false; ensure the GET handler in app/api/settings/ai/route.ts returns a safe value instead of letting decrypt throw (follow the same pattern used in app/api/ai/chat/route.ts and app/api/ai/actions/route.ts).
🧹 Nitpick comments (1)
lib/security/encryption.ts (1)
1-30: Consider using authenticated encryption (AES-GCM) instead of CBC.AES-CBC without message authentication is vulnerable to padding oracle attacks. An attacker who can observe decryption errors may be able to decrypt ciphertext without knowing the key.
For encrypting sensitive data like API keys, authenticated encryption modes like AES-256-GCM are recommended as they provide both confidentiality and integrity in a single operation.
Example using AES-256-GCM
import crypto from 'crypto'; const ALGORITHM = 'aes-256-gcm'; const IV_LENGTH = 12; // GCM recommended IV length const AUTH_TAG_LENGTH = 16; export function encrypt(text: string): string { const ENCRYPTION_SECRET = process.env.ENCRYPTION_SECRET; if (!ENCRYPTION_SECRET) { throw new Error('ENCRYPTION_SECRET environment variable is not set.'); } const salt = crypto.randomBytes(16); const key = crypto.scryptSync(ENCRYPTION_SECRET, salt, 32); const iv = crypto.randomBytes(IV_LENGTH); const cipher = crypto.createCipheriv(ALGORITHM, key, iv, { authTagLength: AUTH_TAG_LENGTH }); let encrypted = cipher.update(text, 'utf8', 'hex'); encrypted += cipher.final('hex'); const authTag = cipher.getAuthTag(); // Format: ciphertext:iv:salt:authTag (all hex) return `${encrypted}:${iv.toString('hex')}:${salt.toString('hex')}:${authTag.toString('hex')}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/security/encryption.ts` around lines 1 - 30, The encrypt implementation uses AES-256-CBC without authentication; change it to AES-256-GCM to provide authenticated encryption: update ALGORITHM to 'aes-256-gcm' and IV_LENGTH to 12, add an AUTH_TAG_LENGTH constant, generate a random salt (e.g., crypto.randomBytes(16)) and derive the key with crypto.scryptSync(ENCRYPTION_SECRET, salt, 32), create the cipher with crypto.createCipheriv(ALGORITHM, key, iv, { authTagLength: AUTH_TAG_LENGTH }), call cipher.getAuthTag() after final(), and return a formatted string that includes ciphertext, iv, salt, and authTag (e.g., hex-separated) so the corresponding decrypt function can re-derive the key and verify the tag; update the encrypt function, constants ALGORITHM/IV_LENGTH/AUTH_TAG_LENGTH, and ensure any decrypt logic is updated to parse the new format and verify the auth tag using cipher.getAuthTag()/setAuthTag().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 35-37: Update the .env.example by removing the surrounding double
quotes from the ENCRYPTION_SECRET placeholder so users don't accidentally
include literal quote characters, and add a short requirement note for
ENCRYPTION_SECRET specifying expected entropy (e.g., minimum length and
randomness), e.g., mention ENCRYPTION_SECRET should be a long random string
(recommend at least 32-64 characters or 256 bits of entropy) and that it must be
kept secret; reference the ENCRYPTION_SECRET key in your note so it's easy to
locate.
In `@lib/security/encryption.ts`:
- Around line 21-23: Replace the hardcoded salt passed to crypto.scryptSync with
a securely generated and stored salt: either read a per-installation salt from
an environment variable like ENCRYPTION_SALT (use crypto.randomBytes(16) at
setup time and persist it) or, for stronger security, generate a random salt per
encryption and include it with the output (e.g., store/send ciphertext, iv and
salt together) and use that salt in the scryptSync call; update the scryptSync
usage (the call where key is derived from ENCRYPTION_SECRET) and the
corresponding decrypt path to read the same salt (reference the scryptSync call
that creates const key, ENCRYPTION_SECRET, and IV_LENGTH) so key derivation is
deterministic for decryption.
- Around line 6-13: The JSDoc for the encrypt function is incorrect: the
function returns hex-encoded values not base64. Update the comment for the
function (in lib/security/encryption.ts) to state that the output is hex-encoded
and clarify the format is "ciphertext:iv" where both parts are hex; reference
the encrypt function and the return description so it matches the actual
implementation that uses hex encoding for ciphertext and iv.
- Around line 32-39: The JSDoc for the decrypt function incorrectly states the
input is base64-encoded; update the comment in lib/security/encryption.ts for
the decrypt function to state the ciphertext and IV are hex-encoded and that the
expected input format is "ciphertext:iv" with both parts hex-encoded, and keep
the notes about deriving the AES-256-CBC key from ENCRYPTION_SECRET and throwing
on missing secret or invalid format; reference the decrypt function name when
making this documentation change.
In `@supabase/migrations/20260402224235_encrypt_ai_api_keys.sql`:
- Around line 1-8: The migration only adds comments and omits converting
existing plaintext API keys, so add a data migration that locates rows in
public.organization_settings where ai_google_key, ai_openai_key, or
ai_anthropic_key are stored as plaintext, encrypts each plaintext value using
the same application encryption routine (the function used by your app's
encrypt/decrypt pipeline), and updates those columns with the new ciphertext:iv
format; implement this as a separate SQL/JS migration run during deployment (or
include a reversible migration) that reads each column, skips already-encrypted
values, calls the app's encrypt(...) for ai_google_key, ai_openai_key,
ai_anthropic_key, and writes back the encrypted value, or alternatively document
this required manual migration step in the deployment notes.
---
Outside diff comments:
In `@app/api/settings/ai/route.ts`:
- Around line 81-91: The decrypt(...) calls for aiGoogleKey, aiOpenaiKey, and
aiAnthropicKey can throw and need try/catch: wrap each
decrypt(orgSettings.ai_<provider>_key) used to build
aiGoogleKey/aiOpenaiKey/aiAnthropicKey in a try/catch, on error log the
exception (include context like org id or orgSettings) and set the returned key
to '' and its corresponding aiHas... flag to false; ensure the GET handler in
app/api/settings/ai/route.ts returns a safe value instead of letting decrypt
throw (follow the same pattern used in app/api/ai/chat/route.ts and
app/api/ai/actions/route.ts).
---
Nitpick comments:
In `@lib/security/encryption.ts`:
- Around line 1-30: The encrypt implementation uses AES-256-CBC without
authentication; change it to AES-256-GCM to provide authenticated encryption:
update ALGORITHM to 'aes-256-gcm' and IV_LENGTH to 12, add an AUTH_TAG_LENGTH
constant, generate a random salt (e.g., crypto.randomBytes(16)) and derive the
key with crypto.scryptSync(ENCRYPTION_SECRET, salt, 32), create the cipher with
crypto.createCipheriv(ALGORITHM, key, iv, { authTagLength: AUTH_TAG_LENGTH }),
call cipher.getAuthTag() after final(), and return a formatted string that
includes ciphertext, iv, salt, and authTag (e.g., hex-separated) so the
corresponding decrypt function can re-derive the key and verify the tag; update
the encrypt function, constants ALGORITHM/IV_LENGTH/AUTH_TAG_LENGTH, and ensure
any decrypt logic is updated to parse the new format and verify the auth tag
using cipher.getAuthTag()/setAuthTag().
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 875a60f5-a749-4341-9385-cb983e72b09b
📒 Files selected for processing (7)
.env.exampleapp/api/ai/actions/route.tsapp/api/ai/chat/route.tsapp/api/settings/ai/route.tslib/security/encryption.tssupabase/migrations/20251201000000_schema_init.sqlsupabase/migrations/20260402224235_encrypt_ai_api_keys.sql
|
|
||
| # Chave para encriptar as chaves de api no banco de dados evitando deixá-las expostas | ||
| ENCRYPTION_SECRET="sua_chave_secreta_longa_e_aleatoria_aqui" |
There was a problem hiding this comment.
Remove quotes from the example value and add key requirements.
The double quotes around the placeholder value will be interpreted literally in .env files, causing the actual secret to include the quote characters. Also, consider documenting the recommended key entropy.
Suggested fix
# Chave para encriptar as chaves de api no banco de dados evitando deixá-las expostas
-ENCRYPTION_SECRET="sua_chave_secreta_longa_e_aleatoria_aqui"
+# Use a strong random string (minimum 32 characters recommended). Generate with: openssl rand -base64 32
+ENCRYPTION_SECRET=📝 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.
| # Chave para encriptar as chaves de api no banco de dados evitando deixá-las expostas | |
| ENCRYPTION_SECRET="sua_chave_secreta_longa_e_aleatoria_aqui" | |
| # Chave para encriptar as chaves de api no banco de dados evitando deixá-las expostas | |
| # Use a strong random string (minimum 32 characters recommended). Generate with: openssl rand -base64 32 | |
| ENCRYPTION_SECRET= |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 37-37: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 35 - 37, Update the .env.example by removing the
surrounding double quotes from the ENCRYPTION_SECRET placeholder so users don't
accidentally include literal quote characters, and add a short requirement note
for ENCRYPTION_SECRET specifying expected entropy (e.g., minimum length and
randomness), e.g., mention ENCRYPTION_SECRET should be a long random string
(recommend at least 32-64 characters or 256 bits of entropy) and that it must be
kept secret; reference the ENCRYPTION_SECRET key in your note so it's easy to
locate.
| /** | ||
| * Encrypts a plaintext string using AES-256-CBC. | ||
| * The encryption key is derived from the ENCRYPTION_SECRET environment variable. | ||
| * The output is a base64-encoded string in the format "ciphertext:iv". | ||
| * | ||
| * @param text The plaintext string to encrypt. | ||
| * @returns The encrypted string in "ciphertext:iv" format (base64-encoded). | ||
| * @throws Error if ENCRYPTION_SECRET is not set. |
There was a problem hiding this comment.
Fix documentation: output is hex-encoded, not base64.
The JSDoc comment states the output is "base64-encoded" but the implementation uses hex encoding (lines 26-29). Update the documentation to match the actual behavior.
Suggested fix
/**
* Encrypts a plaintext string using AES-256-CBC.
* The encryption key is derived from the ENCRYPTION_SECRET environment variable.
- * The output is a base64-encoded string in the format "ciphertext:iv".
+ * The output is a hex-encoded string in the format "ciphertext:iv".
*
* `@param` text The plaintext string to encrypt.
- * `@returns` The encrypted string in "ciphertext:iv" format (base64-encoded).
+ * `@returns` The encrypted string in "ciphertext:iv" format (hex-encoded).
* `@throws` Error if ENCRYPTION_SECRET is not set.
*/📝 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.
| /** | |
| * Encrypts a plaintext string using AES-256-CBC. | |
| * The encryption key is derived from the ENCRYPTION_SECRET environment variable. | |
| * The output is a base64-encoded string in the format "ciphertext:iv". | |
| * | |
| * @param text The plaintext string to encrypt. | |
| * @returns The encrypted string in "ciphertext:iv" format (base64-encoded). | |
| * @throws Error if ENCRYPTION_SECRET is not set. | |
| /** | |
| * Encrypts a plaintext string using AES-256-CBC. | |
| * The encryption key is derived from the ENCRYPTION_SECRET environment variable. | |
| * The output is a hex-encoded string in the format "ciphertext:iv". | |
| * | |
| * `@param` text The plaintext string to encrypt. | |
| * `@returns` The encrypted string in "ciphertext:iv" format (hex-encoded). | |
| * `@throws` Error if ENCRYPTION_SECRET is not set. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/security/encryption.ts` around lines 6 - 13, The JSDoc for the encrypt
function is incorrect: the function returns hex-encoded values not base64.
Update the comment for the function (in lib/security/encryption.ts) to state
that the output is hex-encoded and clarify the format is "ciphertext:iv" where
both parts are hex; reference the encrypt function and the return description so
it matches the actual implementation that uses hex encoding for ciphertext and
iv.
| // Use a consistent key length for AES-256 (32 bytes) | ||
| const key = crypto.scryptSync(ENCRYPTION_SECRET, 'salt', 32); | ||
| const iv = crypto.randomBytes(IV_LENGTH); |
There was a problem hiding this comment.
Use a unique salt instead of a fixed literal.
The hardcoded 'salt' value significantly weakens the scrypt key derivation. scrypt's salt is designed to be unique per key derivation to prevent rainbow table attacks and ensure that identical secrets produce different keys across installations.
Options to improve:
- Per-installation salt: Store a random salt in an environment variable (e.g.,
ENCRYPTION_SALT) generated at setup time. - Per-ciphertext salt: Generate a random salt for each encryption and store it alongside the ciphertext (e.g.,
ciphertext:iv:salt).
Option 2 is more secure but requires a format change. Option 1 is simpler and still provides meaningful protection.
Is it safe to use a fixed salt with scrypt for encryption key derivation?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/security/encryption.ts` around lines 21 - 23, Replace the hardcoded salt
passed to crypto.scryptSync with a securely generated and stored salt: either
read a per-installation salt from an environment variable like ENCRYPTION_SALT
(use crypto.randomBytes(16) at setup time and persist it) or, for stronger
security, generate a random salt per encryption and include it with the output
(e.g., store/send ciphertext, iv and salt together) and use that salt in the
scryptSync call; update the scryptSync usage (the call where key is derived from
ENCRYPTION_SECRET) and the corresponding decrypt path to read the same salt
(reference the scryptSync call that creates const key, ENCRYPTION_SECRET, and
IV_LENGTH) so key derivation is deterministic for decryption.
| /** | ||
| * Decrypts an encrypted string (in "ciphertext:iv" format) using AES-256-CBC. | ||
| * The encryption key is derived from the ENCRYPTION_SECRET environment variable. | ||
| * | ||
| * @param encryptedText The encrypted string in "ciphertext:iv" format (base64-encoded). | ||
| * @returns The decrypted plaintext string. | ||
| * @throws Error if ENCRYPTION_SECRET is not set or if the format is invalid. | ||
| */ |
There was a problem hiding this comment.
Fix documentation for decrypt function as well.
Same issue: the JSDoc says "base64-encoded" but the function expects hex-encoded input.
Suggested fix
/**
* Decrypts an encrypted string (in "ciphertext:iv" format) using AES-256-CBC.
* The encryption key is derived from the ENCRYPTION_SECRET environment variable.
*
- * `@param` encryptedText The encrypted string in "ciphertext:iv" format (base64-encoded).
+ * `@param` encryptedText The encrypted string in "ciphertext:iv" format (hex-encoded).
* `@returns` The decrypted plaintext string.
* `@throws` Error if ENCRYPTION_SECRET is not set or if the format is invalid.
*/📝 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.
| /** | |
| * Decrypts an encrypted string (in "ciphertext:iv" format) using AES-256-CBC. | |
| * The encryption key is derived from the ENCRYPTION_SECRET environment variable. | |
| * | |
| * @param encryptedText The encrypted string in "ciphertext:iv" format (base64-encoded). | |
| * @returns The decrypted plaintext string. | |
| * @throws Error if ENCRYPTION_SECRET is not set or if the format is invalid. | |
| */ | |
| /** | |
| * Decrypts an encrypted string (in "ciphertext:iv" format) using AES-256-CBC. | |
| * The encryption key is derived from the ENCRYPTION_SECRET environment variable. | |
| * | |
| * `@param` encryptedText The encrypted string in "ciphertext:iv" format (hex-encoded). | |
| * `@returns` The decrypted plaintext string. | |
| * `@throws` Error if ENCRYPTION_SECRET is not set or if the format is invalid. | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/security/encryption.ts` around lines 32 - 39, The JSDoc for the decrypt
function incorrectly states the input is base64-encoded; update the comment in
lib/security/encryption.ts for the decrypt function to state the ciphertext and
IV are hex-encoded and that the expected input format is "ciphertext:iv" with
both parts hex-encoded, and keep the notes about deriving the AES-256-CBC key
from ENCRYPTION_SECRET and throwing on missing secret or invalid format;
reference the decrypt function name when making this documentation change.
| -- This migration marks the AI API key columns in organization_settings | ||
| -- as intended for encrypted storage. The application layer will handle | ||
| -- encryption and decryption. | ||
|
|
||
| -- Add comments to the columns for clarity | ||
| COMMENT ON COLUMN public.organization_settings.ai_google_key IS 'Encrypted Google/Gemini API key (ciphertext:iv)'; | ||
| COMMENT ON COLUMN public.organization_settings.ai_openai_key IS 'Encrypted OpenAI API key (ciphertext:iv)'; | ||
| COMMENT ON COLUMN public.organization_settings.ai_anthropic_key IS 'Encrypted Anthropic/Claude API key (ciphertext:iv)'; |
There was a problem hiding this comment.
Missing data migration for existing plaintext keys.
This migration only adds documentation comments but does not encrypt any existing plaintext API keys in the database. If there are existing rows in organization_settings with plaintext keys, they will cause decryption failures after deploying this PR since the new code paths call decrypt() on all stored values.
Consider adding a data migration script (run separately or as part of deployment) to:
- Read existing plaintext keys
- Encrypt them using the new format
- Update the rows with encrypted values
Alternatively, document that this is a breaking change requiring fresh data or a manual migration step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260402224235_encrypt_ai_api_keys.sql` around lines 1 -
8, The migration only adds comments and omits converting existing plaintext API
keys, so add a data migration that locates rows in public.organization_settings
where ai_google_key, ai_openai_key, or ai_anthropic_key are stored as plaintext,
encrypts each plaintext value using the same application encryption routine (the
function used by your app's encrypt/decrypt pipeline), and updates those columns
with the new ciphertext:iv format; implement this as a separate SQL/JS migration
run during deployment (or include a reversible migration) that reads each
column, skips already-encrypted values, calls the app's encrypt(...) for
ai_google_key, ai_openai_key, ai_anthropic_key, and writes back the encrypted
value, or alternatively document this required manual migration step in the
deployment notes.
Code reviewFound 3 issues:
The migration
The GET handler calls nossocrm/app/api/settings/ai/route.ts Lines 82 to 88 in 5bb0f27
nossocrm/supabase/migrations/20251201000000_schema_init.sql Lines 1760 to 1765 in 5bb0f27 🤖 Generated with Claude Code If this code review was useful, please react with 👍. Otherwise, react with 👎. |
2 participants
Motivation
Improving security with LLM API Keys avoing database plain text exposition.
Summary by CodeRabbit
Security Enhancements
Error Handling