feat: add column-level encryption for database fields#1067
feat: add column-level encryption for database fields#1067Davidson3556 wants to merge 9 commits intoInsForge:mainfrom
Conversation
Implement AES-256-GCM column-level encryption with transparent encrypt-on-write and decrypt-on-read through the PostgREST proxy. - Add encrypted_columns registry table (migration 029) - Add EncryptedColumnService with caching and batch operations - Add encryption admin API routes (list, encrypt-column, key rotation) - Intercept record CRUD to encrypt/decrypt transparently - Extend shared column schema with `encrypted` field - Add Encrypted checkbox to dashboard table form UI - Add startup validation for encryption key configuration - Fix Dockerfile.deno user creation for Alpine base image Closes InsForge#1064
|
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:
WalkthroughAdds column-level encryption: registry table and migrations, key-versioned ciphertext handling, a new EncryptedColumnService (cache, encrypt/decrypt, migrate/rotate), PostgREST proxy integration, admin routes for migration/rotation, table lifecycle hooks, dashboard toggle/validation, and startup seed checks for encryption config. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/src/api/routes/database/records.routes.ts (1)
35-36:⚠️ Potential issue | 🔴 CriticalSplit schema-qualified names before loading encryption metadata.
EncryptedColumnService.getEncryptedColumns()defaults the schema to'public'and queriestable_name = $2. Passingauth.user_providersorpublic.ordersstraight through here misses the registry row, so the proxy silently skips encryption/decryption and can persist plaintext in columns that are supposed to be protected.🛡️ Proposed fix
const forwardToPostgrest = async (req: AuthRequest, res: Response, next: NextFunction) => { const { tableName, path: wildcardPath } = req.params; const path = wildcardPath ? `/${tableName}/${wildcardPath}` : `/${tableName}`; + const [tableSchema, unqualifiedTableName] = tableName.includes('.') + ? tableName.split('.', 2) + : ['public', tableName]; try { @@ - const encryptedColumns = await encryptedColumnService.getEncryptedColumns(tableName); + const encryptedColumns = await encryptedColumnService.getEncryptedColumns( + unqualifiedTableName, + tableSchema + );I'd reuse the same parsed
tableSchema/unqualifiedTableNamefor the other table-specific metadata lookups in this handler as well.Also applies to: 53-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/api/routes/database/records.routes.ts` around lines 35 - 36, The handler currently uses req.params.tableName (which may be schema-qualified like "public.orders") when building path and when calling EncryptedColumnService.getEncryptedColumns(), causing schema mismatch; parse tableName into tableSchema and unqualifiedTableName (reuse the same parsed values used elsewhere in this handler) and pass unqualifiedTableName and tableSchema into EncryptedColumnService.getEncryptedColumns() and the other table-specific metadata lookups (the same places referenced near lines 53-54) instead of the raw tableName so the registry query uses the correct schema and table_name values.packages/shared-schemas/src/database.schema.ts (1)
41-53:⚠️ Potential issue | 🟠 MajorReject unsupported encrypted-column combinations.
This change lets callers submit
encryptedtogether withisPrimaryKey,isUnique,foreignKey, or a DBdefaultValue. With randomized AES-GCM ciphertext and TEXT storage, those combinations cannot behave correctly; they either fail later or create misleading guarantees. Please reject them in the shared schema and mirror that in the form UI.🛡️ Proposed fix
-export const columnSchema = z.object({ - columnName: z - .string() - .min(1, 'Column name cannot be empty') - .max(64, 'Column name must be less than 64 characters'), - type: z.union([columnTypeSchema, z.string()]), - defaultValue: z.string().optional(), - isPrimaryKey: z.boolean().optional(), - isNullable: z.boolean(), - isUnique: z.boolean(), - foreignKey: foreignKeySchema.optional(), - encrypted: z.boolean().optional(), -}); +export const columnSchema = z + .object({ + columnName: z + .string() + .min(1, 'Column name cannot be empty') + .max(64, 'Column name must be less than 64 characters'), + type: z.union([columnTypeSchema, z.string()]), + defaultValue: z.string().optional(), + isPrimaryKey: z.boolean().optional(), + isNullable: z.boolean(), + isUnique: z.boolean(), + foreignKey: foreignKeySchema.optional(), + encrypted: z.boolean().optional(), + }) + .superRefine((column, ctx) => { + if (!column.encrypted) { + return; + } + + if (column.isPrimaryKey || column.isUnique || column.foreignKey || column.defaultValue) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['encrypted'], + message: + 'Encrypted columns cannot be primary keys, unique, foreign keys, or have database defaults.', + }); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-schemas/src/database.schema.ts` around lines 41 - 53, Update the columnSchema to reject unsupported combinations where encrypted is true with isPrimaryKey, isUnique, foreignKey, or a non-empty defaultValue by adding a Zod refinement/superRefine on columnSchema: check when encrypted === true and then error if isPrimaryKey === true, isUnique === true, foreignKey is present, or defaultValue is a non-empty string, producing clear field-specific messages; reference the columnSchema, encrypted, isPrimaryKey, isUnique, foreignKey, and defaultValue symbols so callers and the form UI can mirror these validation rules.backend/src/services/database/database-table.service.ts (1)
234-269:⚠️ Potential issue | 🟠 MajorMake the DDL and registry mutations atomic.
createTable()applies the schema change first and only then callsEncryptedColumnService.registerColumn(), while drop/delete do the inverse withunregister*(). Those service methods use separate pool queries, so a failure there leaves eitherTEXTcolumns with no registry entry or stalesystem.encrypted_columnsrows that break startup validation and rotation. Keep the schema change and registry update on the same client/transaction boundary.As per coding guidelines "Implement proper database transaction handling".
Also applies to: 523-525, 672-674
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/services/database/database-table.service.ts` around lines 234 - 269, createTable() performs DDL and then calls EncryptedColumnService.registerColumn() using separate pool queries, which can leave schema and registry out of sync; wrap the CREATE TABLE, ALTER/CREATE TRIGGER and the registerColumn calls in a single DB transaction on the same client so they are atomic. Change the flow in createTable() to BEGIN on the provided client, run the CREATE TABLE / ALTER TABLE / CREATE TRIGGER statements, call EncryptedColumnService.getInstance().registerColumn(...) using a transactional variant or by passing the same client/connection into registerColumn (and add a matching transactional unregister method used by drops), and COMMIT; on any error ROLLBACK. Apply the same pattern for the inverse unregister logic (drop/delete) and for the other occurrences noted (the other register/unregister call sites).
🧹 Nitpick comments (2)
backend/src/infra/database/migrations/029_create-encrypted-columns-registry.sql (1)
13-17: Remove the redundant lookup index.The unique constraint on Line 13 already creates a btree index on
(table_schema, table_name, column_name), and PostgreSQL can use that left prefix for theWHERE table_schema = $1 AND table_name = $2lookup. Keepingidx_encrypted_columns_tableadds extra write/maintenance cost for every registry update.♻️ Proposed fix
-CREATE INDEX IF NOT EXISTS idx_encrypted_columns_table - ON system.encrypted_columns (table_schema, table_name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/infra/database/migrations/029_create-encrypted-columns-registry.sql` around lines 13 - 17, Remove the redundant index creation for idx_encrypted_columns_table on system.encrypted_columns: the UNIQUE (table_schema, table_name, column_name) constraint already provides a btree index that PostgreSQL can use for lookups by table_schema and table_name, so delete the CREATE INDEX IF NOT EXISTS idx_encrypted_columns_table ... statement to avoid extra write/maintenance cost on updates to the encrypted_columns registry.backend/src/services/database/encrypted-column.service.ts (1)
1-5: Rename this service file to camelCase.
encrypted-column.service.tsdoes not match the repo rule forbackend/src/**/*.service.tsfiles.As per coding guidelines "Name service files in camelCase with .service.ts suffix (e.g., auth.service.ts)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/services/database/encrypted-column.service.ts` around lines 1 - 5, The file name encrypted-column.service.ts should be renamed to camelCase (e.g., encryptedColumn.service.ts) to follow the repo rule; rename the file and update all imports that reference it (search for imports referencing 'encrypted-column.service' and change to 'encryptedColumn.service') and ensure any references in build/config (if any) are updated; use the symbols in the file (DatabaseManager, EncryptionManager, logger, Pool) to locate usages and verify imports compile after the rename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/api/routes/database/encryption.routes.ts`:
- Around line 90-98: After calling
encryptedColumnService.migrateColumnToEncrypted(...) and
DatabaseManager.clearColumnTypeCache(tableName), also send a PostgREST schema
reload notification so PostgREST will pick up the column type change
immediately; add a DB notification like NOTIFY pgrst, 'reload schema' using your
DB client (e.g. DatabaseManager.pool.query or DatabaseManager.query or the
equivalent DB helper) executed right after
DatabaseManager.clearColumnTypeCache(tableName) in the same request flow.
- Around line 53-55: The code throws an AppError using parseResult.error.errors
which is invalid in Zod 4; update the error extraction to use
parseResult.error.issues instead and map each issue's message (e.g.,
parseResult.error.issues.map(i => i.message).join(', ')) before passing to the
AppError constructor so the thrown message contains the Zod validation issue
texts; locate this change in the block that checks if (!parseResult.success) and
references parseResult and AppError.
In `@backend/src/infra/security/encryption.manager.ts`:
- Around line 4-5: The CURRENT_KEY_VERSION constant and decrypt() need to be
changed to support key rotation: add a version→key resolver (e.g., a map or
function like resolveKeyForVersion(version) that reads candidate keys from
env/config) and make decrypt() call getKeyVersion(ciphertext) to parse the "vN:"
prefix and then use resolveKeyForVersion(N) to obtain the correct key; ensure
encrypt() still prefixes ciphertext with the active version (CURRENT_KEY_VERSION
or a dynamic getCurrentKeyVersion()), and update any references in this file
(decrypt, encrypt, getKeyVersion, CURRENT_KEY_VERSION) so decrypt selects the
proper key for the parsed version and fails with a clear error if no key is
available for that version.
- Around line 108-113: isConfigured() currently returns true if JWT_SECRET is
set, which lets column-encryption run on a rotating auth secret; change it to
require a dedicated ENCRYPTION_KEY for stored-field encryption by checking
process.env.ENCRYPTION_KEY exists and meets a minimum length/entropy (e.g., >=
32 chars) instead of falling back to JWT_SECRET, or alternatively leave
isConfigured() as-is for legacy use and add a new helper (e.g.,
isEncryptionKeyConfigured or isColumnEncryptionConfigured) that performs the
ENCRYPTION_KEY presence and min-length check and use that guard for encrypted
columns.
In `@backend/src/services/database/database-table.service.ts`:
- Around line 523-525: In updateTableSchema() the addColumns path must mirror
createTable()’s encryption handling: when iterating new columns (addColumns),
check col.encrypted and if true call
EncryptedColumnService.getInstance().registerColumn(tableName, col) (and update
any returned metadata), set the actual SQL column type to TEXT instead of the
original SQL type, and add the column to the table’s encrypted-columns tracking
so it will use the encrypt/decrypt hooks; also ensure the corresponding
cleanup/unregister logic (EncryptedColumnService.unregisterColumn) remains for
dropped columns. Apply the same change to the other add/modify column logic
noted around lines 577–605 so newly added encrypted columns from the edit flow
are registered and created as TEXT just like createTable().
- Around line 523-525: The encrypted-column registry is not updated when
columns/tables are renamed: locate the two rename code paths that call
EncryptedColumnService.getInstance().unregisterColumn(tableName, col) (and the
similar blocks at the other ranges) and update them to perform the registry
change inside the same DB transaction as the rename — either by calling a new
EncryptedColumnService.renameColumn(oldTable, oldCol, newTable, newCol,
transaction) helper or by invoking unregisterColumn(old...) and
registerColumn(new...) while passing the active transaction (e.g.,
trx/transaction) so the registry update is committed/rolled back together with
the schema rename; ensure you update both rename branches mentioned and use the
same transaction object used for the table/column rename.
- Around line 388-393: The COUNT query interpolates raw table into SQL and
bypasses existing protections; use the service's identifier sanitizers instead:
call validateIdentifier(table) and then use quoteIdentifier(table) (or the
equivalent helper) to produce a safely quoted identifier and plug that into the
client.query call used alongside
EncryptedColumnService.getEncryptedColumns(table) and client.query; update the
code around client.query(`SELECT COUNT(*) as row_count FROM "${table}"`) to
first validate and quote the table identifier via the
validateIdentifier()/quoteIdentifier() helpers before building the SQL string.
In `@backend/src/services/database/encrypted-column.service.ts`:
- Around line 315-343: The loop is reprocessing rows because UPDATE changes
ctid; stop using ctid paging or exclude already-updated rows: replace ctid-based
pagination (variables lastCtid and ctid comparisons) with stable pagination on
the table's primary key (e.g., use the PK column in the SELECT/ORDER BY/WHERE
and pass lastPk), or add an additional WHERE clause to the SELECT that skips
values already using the current encryption version (e.g., WHERE "${columnName}"
IS NOT NULL AND "${columnName}" NOT LIKE '<current-version-prefix>%') before
calling EncryptionManager.decrypt/encryptVersioned so rows already prefixed with
the current key are not selected. Ensure the UPDATE uses the PK in its WHERE
when you switch pagination.
- Around line 176-185: The decrypted value is always being left as a string,
breaking schema types; in the decryption block (around EncryptionManager.decrypt
usage in encrypted-column.service.ts) inspect entry.originalType and convert the
decrypted string back to the original scalar type before assigning to
row[colName]: for 'json'/'jsonb' keep the existing JSON.parse fallback, for
booleans map 'true'/'false' to true/false, for integer types (e.g. 'int',
'bigint', 'integer') use integer parsing with NaN checks, for float/numeric use
parseFloat with NaN handling, for timestamp/date types convert to a Date (or
leave as ISO string only if invalid), and for any unsupported originalType
either throw or log/reject so callers know encryption of that type isn’t
supported; update the assignment to set row[colName] to the correctly typed
value and ensure you reference EncryptionManager, entry.originalType, row and
colName when making the change.
- Around line 211-223: The SQL uses interpolated identifiers (qualifiedTable and
columnName) which can be broken by unvalidated input; update
encrypted-column.service.ts to validate and safely quote table/schema/column
identifiers by calling the identifier-quoting helper on DatabaseTableService
(reuse its sanitize/quoteIdentifier method) before building qualifiedTable and
any `"${columnName}"` usage, and replace the raw interpolations in all migration
SQL blocks (the ALTER/UPDATE statements and the other occurrences noted) to use
the returned safe identifiers; ensure you still use parameterized values for
data portions and apply this change consistently wherever qualifiedTable or
columnName are composed.
In `@deploy/Dockerfile.deno`:
- Line 4: The Dockerfile RUN that currently uses "RUN addgroup -S deno
2>/dev/null; adduser -S deno -G deno 2>/dev/null; true" masks failures; remove
the trailing "; true" and replace with explicit checks that create the
group/user only if they don't exist (e.g., use getent group deno || addgroup -S
deno and getent passwd deno || adduser -S deno -G deno) so failures surface and
the build fails on real provisioning errors; ensure commands still redirect only
expected stderr and exit non‑zero on unexpected failures.
---
Outside diff comments:
In `@backend/src/api/routes/database/records.routes.ts`:
- Around line 35-36: The handler currently uses req.params.tableName (which may
be schema-qualified like "public.orders") when building path and when calling
EncryptedColumnService.getEncryptedColumns(), causing schema mismatch; parse
tableName into tableSchema and unqualifiedTableName (reuse the same parsed
values used elsewhere in this handler) and pass unqualifiedTableName and
tableSchema into EncryptedColumnService.getEncryptedColumns() and the other
table-specific metadata lookups (the same places referenced near lines 53-54)
instead of the raw tableName so the registry query uses the correct schema and
table_name values.
In `@backend/src/services/database/database-table.service.ts`:
- Around line 234-269: createTable() performs DDL and then calls
EncryptedColumnService.registerColumn() using separate pool queries, which can
leave schema and registry out of sync; wrap the CREATE TABLE, ALTER/CREATE
TRIGGER and the registerColumn calls in a single DB transaction on the same
client so they are atomic. Change the flow in createTable() to BEGIN on the
provided client, run the CREATE TABLE / ALTER TABLE / CREATE TRIGGER statements,
call EncryptedColumnService.getInstance().registerColumn(...) using a
transactional variant or by passing the same client/connection into
registerColumn (and add a matching transactional unregister method used by
drops), and COMMIT; on any error ROLLBACK. Apply the same pattern for the
inverse unregister logic (drop/delete) and for the other occurrences noted (the
other register/unregister call sites).
In `@packages/shared-schemas/src/database.schema.ts`:
- Around line 41-53: Update the columnSchema to reject unsupported combinations
where encrypted is true with isPrimaryKey, isUnique, foreignKey, or a non-empty
defaultValue by adding a Zod refinement/superRefine on columnSchema: check when
encrypted === true and then error if isPrimaryKey === true, isUnique === true,
foreignKey is present, or defaultValue is a non-empty string, producing clear
field-specific messages; reference the columnSchema, encrypted, isPrimaryKey,
isUnique, foreignKey, and defaultValue symbols so callers and the form UI can
mirror these validation rules.
---
Nitpick comments:
In
`@backend/src/infra/database/migrations/029_create-encrypted-columns-registry.sql`:
- Around line 13-17: Remove the redundant index creation for
idx_encrypted_columns_table on system.encrypted_columns: the UNIQUE
(table_schema, table_name, column_name) constraint already provides a btree
index that PostgreSQL can use for lookups by table_schema and table_name, so
delete the CREATE INDEX IF NOT EXISTS idx_encrypted_columns_table ... statement
to avoid extra write/maintenance cost on updates to the encrypted_columns
registry.
In `@backend/src/services/database/encrypted-column.service.ts`:
- Around line 1-5: The file name encrypted-column.service.ts should be renamed
to camelCase (e.g., encryptedColumn.service.ts) to follow the repo rule; rename
the file and update all imports that reference it (search for imports
referencing 'encrypted-column.service' and change to 'encryptedColumn.service')
and ensure any references in build/config (if any) are updated; use the symbols
in the file (DatabaseManager, EncryptionManager, logger, Pool) to locate usages
and verify imports compile after the rename.
🪄 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: 40b3c31e-5aa9-4937-ab5e-e581d8d49518
📒 Files selected for processing (12)
backend/src/api/routes/database/encryption.routes.tsbackend/src/api/routes/database/index.routes.tsbackend/src/api/routes/database/records.routes.tsbackend/src/infra/database/migrations/029_create-encrypted-columns-registry.sqlbackend/src/infra/security/encryption.manager.tsbackend/src/services/database/database-table.service.tsbackend/src/services/database/encrypted-column.service.tsbackend/src/utils/seed.tsdeploy/Dockerfile.denopackages/dashboard/src/features/database/components/TableForm.tsxpackages/dashboard/src/features/database/components/TableFormColumn.tsxpackages/shared-schemas/src/database.schema.ts
| /** | ||
| * Check whether ENCRYPTION_KEY (or JWT_SECRET fallback) is configured | ||
| */ | ||
| static isConfigured(): boolean { | ||
| return !!(process.env.ENCRYPTION_KEY || process.env.JWT_SECRET); | ||
| } |
There was a problem hiding this comment.
Don't let JWT_SECRET satisfy the column-encryption startup check.
isConfigured() returns true when only JWT_SECRET is set, so the new fail-fast guard will allow encrypted columns to run on a key that is commonly rotated for auth and already warned about above. For stored field encryption, require a dedicated ENCRYPTION_KEY (ideally with a minimum length/entropy check) instead of treating the JWT fallback as safe.
🛡️ Proposed fix
- static isConfigured(): boolean {
- return !!(process.env.ENCRYPTION_KEY || process.env.JWT_SECRET);
- }
+ static isConfigured(): boolean {
+ return (
+ typeof process.env.ENCRYPTION_KEY === 'string' &&
+ process.env.ENCRYPTION_KEY.trim().length >= 32
+ );
+ }If the JWT fallback must remain for legacy secret storage, I'd split this into a dedicated column-encryption config helper instead of reusing isConfigured().
📝 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.
| /** | |
| * Check whether ENCRYPTION_KEY (or JWT_SECRET fallback) is configured | |
| */ | |
| static isConfigured(): boolean { | |
| return !!(process.env.ENCRYPTION_KEY || process.env.JWT_SECRET); | |
| } | |
| /** | |
| * Check whether ENCRYPTION_KEY (or JWT_SECRET fallback) is configured | |
| */ | |
| static isConfigured(): boolean { | |
| return ( | |
| typeof process.env.ENCRYPTION_KEY === 'string' && | |
| process.env.ENCRYPTION_KEY.trim().length >= 32 | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/infra/security/encryption.manager.ts` around lines 108 - 113,
isConfigured() currently returns true if JWT_SECRET is set, which lets
column-encryption run on a rotating auth secret; change it to require a
dedicated ENCRYPTION_KEY for stored-field encryption by checking
process.env.ENCRYPTION_KEY exists and meets a minimum length/entropy (e.g., >=
32 chars) instead of falling back to JWT_SECRET, or alternatively leave
isConfigured() as-is for legacy use and add a new helper (e.g.,
isEncryptionKeyConfigured or isColumnEncryptionConfigured) that performs the
ENCRYPTION_KEY presence and min-length check and use that guard for encrypted
columns.
There was a problem hiding this comment.
Warning already logged on fallback
There was a problem hiding this comment.
@Davidson3556, acknowledged — the logger.warn in getEncryptionKey() does surface the risk at runtime whenever the fallback path is actually exercised. The remaining gap is that isConfigured() → startup guard path passes silently at boot without ever hitting getEncryptionKey(), so an operator could deploy with only JWT_SECRET, see no startup error, and only encounter the warning on first use. That said, I'll respect the design decision to keep the current behaviour.
ᓚᘏᗢ
✏️ Learnings added
Learnt from: Davidson3556
Repo: InsForge/InsForge PR: 1067
File: backend/src/infra/security/encryption.manager.ts:108-113
Timestamp: 2026-04-05T09:01:07.043Z
Learning: In `backend/src/infra/security/encryption.manager.ts`, `EncryptionManager.isConfigured()` intentionally returns `true` when only `JWT_SECRET` is set (no dedicated `ENCRYPTION_KEY`). The JWT_SECRET fallback is considered acceptable for column-level encryption; the risk is mitigated by a `logger.warn` emitted in `getEncryptionKey()` at first use. Do not flag this as a bug or suggest splitting `isConfigured()` into a dedicated column-encryption check — the maintainer has explicitly declined that change.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Davidson3556
Repo: InsForge/InsForge PR: 1067
File: backend/src/infra/security/encryption.manager.ts:4-5
Timestamp: 2026-04-05T08:59:05.065Z
Learning: In `backend/src/infra/security/encryption.manager.ts`, the `v1:` version prefix in `CURRENT_KEY_VERSION`/`encryptVersioned` is intentionally a forward-compatibility placeholder for v1. Multi-key version→key resolver support for real key rotation is deferred to a future version. Do not flag the absence of a key resolver as a bug in this codebase.
| // Get row count and encrypted columns in parallel | ||
| const encryptedColumnService = EncryptedColumnService.getInstance(); | ||
| const [rowCountResult, encryptedColumns] = await Promise.all([ | ||
| client.query(`SELECT COUNT(*) as row_count FROM "${table}"`), | ||
| encryptedColumnService.getEncryptedColumns(table), | ||
| ]); |
There was a problem hiding this comment.
Don't interpolate table directly into the count query.
This path skips the service's existing validateIdentifier() / quoteIdentifier() protections and builds SQL from raw identifier text. A double quote in table breaks the statement and turns this into an injection surface.
🛡️ Suggested fix
async getTableSchema(table: string): Promise<GetTableSchemaResponse> {
+ validateIdentifier(table, 'table');
const client = await this.getPool().connect();
try {
@@
const [rowCountResult, encryptedColumns] = await Promise.all([
- client.query(`SELECT COUNT(*) as row_count FROM "${table}"`),
+ client.query(`SELECT COUNT(*) as row_count FROM ${this.quoteIdentifier(table)}`),
encryptedColumnService.getEncryptedColumns(table),
]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/services/database/database-table.service.ts` around lines 388 -
393, The COUNT query interpolates raw table into SQL and bypasses existing
protections; use the service's identifier sanitizers instead: call
validateIdentifier(table) and then use quoteIdentifier(table) (or the equivalent
helper) to produce a safely quoted identifier and plug that into the
client.query call used alongside
EncryptedColumnService.getEncryptedColumns(table) and client.query; update the
code around client.query(`SELECT COUNT(*) as row_count FROM "${table}"`) to
first validate and quote the table identifier via the
validateIdentifier()/quoteIdentifier() helpers before building the SQL string.
| # Create non-root user | ||
| RUN addgroup -S deno && adduser -S deno -G deno | ||
| # Create non-root user (skip if already exists in base image) | ||
| RUN addgroup -S deno 2>/dev/null; adduser -S deno -G deno 2>/dev/null; true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Dockerfiles do not mask provisioning failures with unconditional '; true'
fd -i 'Dockerfile*' -t f | xargs -r rg -n --fixed-strings '; true' -C2Repository: InsForge/InsForge
Length of output: 348
Remove unconditional ; true to expose real provisioning failures.
Line 4 uses ; true to force success regardless of whether addgroup or adduser fail. This masks real errors—permission denied, missing binaries, invalid options—that could prevent the user from being created while the build still succeeds. Replace with explicit existence checks:
Proposed fix
-RUN addgroup -S deno 2>/dev/null; adduser -S deno -G deno 2>/dev/null; true
+RUN if ! grep -q '^deno:' /etc/group; then addgroup -S deno; fi \
+ && if ! grep -q '^deno:' /etc/passwd; then adduser -S deno -G deno; fi📝 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.
| RUN addgroup -S deno 2>/dev/null; adduser -S deno -G deno 2>/dev/null; true | |
| RUN if ! grep -q '^deno:' /etc/group; then addgroup -S deno; fi \ | |
| && if ! grep -q '^deno:' /etc/passwd; then adduser -S deno -G deno; fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/Dockerfile.deno` at line 4, The Dockerfile RUN that currently uses
"RUN addgroup -S deno 2>/dev/null; adduser -S deno -G deno 2>/dev/null; true"
masks failures; remove the trailing "; true" and replace with explicit checks
that create the group/user only if they don't exist (e.g., use getent group deno
|| addgroup -S deno and getent passwd deno || adduser -S deno -G deno) so
failures surface and the build fails on real provisioning errors; ensure
commands still redirect only expected stderr and exit non‑zero on unexpected
failures.
- Add PostgREST NOTIFY after encrypt-column migration - Fix SQL injection in COUNT query using quoteIdentifier - Handle encrypted columns in updateTableSchema addColumns path - Update encrypted column registry on column/table rename - Use safe identifier quoting in encrypted-column.service - Skip already-encrypted rows during batch migration (ctid fix) - Fix Dockerfile.deno to surface real provisioning errors - Fix ESLint/Prettier formatting (line endings, ternaries)
There was a problem hiding this comment.
♻️ Duplicate comments (3)
backend/src/api/routes/database/encryption.routes.ts (1)
53-56:⚠️ Potential issue | 🟠 MajorUse
ZodError.issuesinstead of.errors.On Line 55,
parseResult.error.errorsis not the correct property in Zod v4 and can break validation error handling.✅ Suggested fix
- parseResult.error.errors.map((e) => e.message).join(', '), + parseResult.error.issues.map((issue) => issue.message).join(', '),In Zod v4, what is the correct property on ZodError for validation items: `.errors` or `.issues`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/api/routes/database/encryption.routes.ts` around lines 53 - 56, The error handling uses the wrong ZodError property: replace references to parseResult.error.errors with parseResult.error.issues and map over those to build the message; update the throw in the block that creates the AppError (the parseResult handling code) to use parseResult.error.issues.map(e => e.message).join(', ') so validation messages come from Zod v4's issues array.backend/src/services/database/database-table.service.ts (1)
261-269:⚠️ Potential issue | 🔴 CriticalMake schema and encryption-registry writes atomic.
These registry writes happen outside the active schema-operation connection/transaction, so failures can desync DB structure from
system.encrypted_columns(e.g., encryptedTEXTcolumn not registered, or stale registry after rename/drop). That can bypass encrypt/decrypt hooks and break startup key checks. Same pattern also appears at Line 524–Line 527, Line 579–Line 580, Line 617–Line 618, Line 662–Line 664, and Line 698–Line 701.🔧 Directional fix
-const encryptedColumnService = EncryptedColumnService.getInstance(); -await encryptedColumnService.registerColumn(table_name, col.columnName, originalSqlType); +// Pass the same `client` through registry methods, and wrap DDL + registry in BEGIN/COMMIT. +await encryptedColumnService.registerColumn(table_name, col.columnName, originalSqlType, 'public', client);+await client.query('BEGIN'); // DDL mutations... // registry mutations using same client... +await client.query('COMMIT');Based on learnings "Applies to backend/src/**/*.{ts,js} : Implement proper database transaction handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/services/database/database-table.service.ts` around lines 261 - 269, The schema change and registry writes for encrypted columns (code using validatedColumns, COLUMN_TYPES[col.type], table_name and calling EncryptedColumnService.getInstance().registerColumn) must be made atomic: use the same active schema-operation DB connection/transaction used to alter the table and perform registerColumn calls within that transaction (or provide a transactional overload on EncryptedColumnService.registerColumn that accepts the connection/transaction and uses it), and rollback both schema mutation and registry updates on failure; apply the same pattern to the other occurrences of registerColumn/registering logic noted in the comment so schema changes and system.encrypted_columns updates always happen inside the same transaction/connection.backend/src/services/database/encrypted-column.service.ts (1)
183-192:⚠️ Potential issue | 🟠 MajorRestore original scalar types after decryption.
On Line 190–Line 192, non-
json/jsonbdecrypted values are always returned as strings. This breaks transparent behavior for encrypted booleans/numbers/dates and can change API/runtime semantics for existing consumers.💡 Suggested fix
- } else { - row[colName] = decrypted; - } + } else { + const normalizedType = entry.originalType.toLowerCase(); + switch (normalizedType) { + case 'boolean': + case 'bool': + row[colName] = decrypted === 'true' ? true : decrypted === 'false' ? false : decrypted; + break; + case 'smallint': + case 'int': + case 'integer': + case 'bigint': { + const parsed = Number.parseInt(decrypted, 10); + row[colName] = Number.isNaN(parsed) ? decrypted : parsed; + break; + } + case 'real': + case 'double precision': + case 'numeric': + case 'decimal': { + const parsed = Number.parseFloat(decrypted); + row[colName] = Number.isNaN(parsed) ? decrypted : parsed; + break; + } + default: + row[colName] = decrypted; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/services/database/encrypted-column.service.ts` around lines 183 - 192, The decrypted branch currently always assigns strings to row[colName] for non-json types; update the logic in the decryption routine (look for variables entry.originalType, row, colName, decrypted) to coerce restored scalar types: detect boolean types (e.g., originalType includes 'bool') and set row[colName] = decrypted === 'true' (handle case-insensitive), detect integer/number types (e.g., 'int', 'integer', 'numeric', 'float', 'double') and use Number(decrypted) but fall back to the original string if Number is NaN, and detect date/time types (e.g., 'date','timestamp','timestamptz') and convert to a Date object if valid (otherwise leave as string); keep the existing JSON parsing branch for 'json'/'jsonb' untouched and ensure any parse/conversion failures fall back to the raw decrypted string.
🧹 Nitpick comments (1)
backend/src/api/routes/database/encryption.routes.ts (1)
46-52: ValidatetableNamewith schema/identifier checks at the route boundary.The body is validated, but
req.params.tableNameis not. Adding param validation here keeps failures explicit (400) and keeps DB-facing identifiers consistently sanitized.As per coding guidelines "Use Zod schemas for request/response validation in backend" and "Sanitize data before database operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/api/routes/database/encryption.routes.ts` around lines 46 - 52, Validate and sanitize req.params.tableName at the route boundary before using it in DB operations: add a Zod schema (or identifier check) for tableName and run it in the same handler that uses EncryptColumnSchema so invalid params return 400; for example, validate req.params.tableName with a Zod string/regex or an isValidIdentifier helper, reject on failure, and only proceed to use tableName in the encrypt-column logic if it passes validation (reference: EncryptColumnSchema, the route handler async (req: AuthRequest, res: Response, next: NextFunction), and req.params.tableName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/src/api/routes/database/encryption.routes.ts`:
- Around line 53-56: The error handling uses the wrong ZodError property:
replace references to parseResult.error.errors with parseResult.error.issues and
map over those to build the message; update the throw in the block that creates
the AppError (the parseResult handling code) to use
parseResult.error.issues.map(e => e.message).join(', ') so validation messages
come from Zod v4's issues array.
In `@backend/src/services/database/database-table.service.ts`:
- Around line 261-269: The schema change and registry writes for encrypted
columns (code using validatedColumns, COLUMN_TYPES[col.type], table_name and
calling EncryptedColumnService.getInstance().registerColumn) must be made
atomic: use the same active schema-operation DB connection/transaction used to
alter the table and perform registerColumn calls within that transaction (or
provide a transactional overload on EncryptedColumnService.registerColumn that
accepts the connection/transaction and uses it), and rollback both schema
mutation and registry updates on failure; apply the same pattern to the other
occurrences of registerColumn/registering logic noted in the comment so schema
changes and system.encrypted_columns updates always happen inside the same
transaction/connection.
In `@backend/src/services/database/encrypted-column.service.ts`:
- Around line 183-192: The decrypted branch currently always assigns strings to
row[colName] for non-json types; update the logic in the decryption routine
(look for variables entry.originalType, row, colName, decrypted) to coerce
restored scalar types: detect boolean types (e.g., originalType includes 'bool')
and set row[colName] = decrypted === 'true' (handle case-insensitive), detect
integer/number types (e.g., 'int', 'integer', 'numeric', 'float', 'double') and
use Number(decrypted) but fall back to the original string if Number is NaN, and
detect date/time types (e.g., 'date','timestamp','timestamptz') and convert to a
Date object if valid (otherwise leave as string); keep the existing JSON parsing
branch for 'json'/'jsonb' untouched and ensure any parse/conversion failures
fall back to the raw decrypted string.
---
Nitpick comments:
In `@backend/src/api/routes/database/encryption.routes.ts`:
- Around line 46-52: Validate and sanitize req.params.tableName at the route
boundary before using it in DB operations: add a Zod schema (or identifier
check) for tableName and run it in the same handler that uses
EncryptColumnSchema so invalid params return 400; for example, validate
req.params.tableName with a Zod string/regex or an isValidIdentifier helper,
reject on failure, and only proceed to use tableName in the encrypt-column logic
if it passes validation (reference: EncryptColumnSchema, the route handler async
(req: AuthRequest, res: Response, next: NextFunction), and
req.params.tableName).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e3eb5a9-7413-43c4-aabd-35371cf4a826
📒 Files selected for processing (4)
backend/src/api/routes/database/encryption.routes.tsbackend/src/services/database/database-table.service.tsbackend/src/services/database/encrypted-column.service.tsdeploy/Dockerfile.deno
✅ Files skipped from review due to trivial changes (1)
- deploy/Dockerfile.deno
- Use Zod .issues instead of .errors for forward compatibility - Add type coercion in decryptRow for boolean, integer, float types - Add validatedColumnSchema with superRefine rejecting encrypted + isPrimaryKey/isUnique/foreignKey combinations - Remove redundant index on encrypted_columns (UNIQUE constraint suffices)
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)
packages/shared-schemas/src/database.schema.ts (1)
87-87:⚠️ Potential issue | 🟠 Major
tableSchemashould referencevalidatedColumnSchemato enforce encrypted-column constraints.Currently, this line uses
columnSchema, which lacks the encryption-related validation rules. Any consumer oftableSchema(includingcreateTableRequestSchema) won't benefit from the constraints defined invalidatedColumnSchema. Based on learnings, Zod schemas are used for request/response validation in the backend, so this gap could allow invalid column configurations through the API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-schemas/src/database.schema.ts` at line 87, The tableSchema currently uses columnSchema for its columns array which skips encryption-related validation; change the columns definition in tableSchema to use validatedColumnSchema instead of columnSchema so encrypted-column constraints are enforced (ensure validatedColumnSchema is imported/defined in the same module) and verify downstream schemas like createTableRequestSchema that rely on tableSchema now inherit the tighter validation.
🧹 Nitpick comments (1)
backend/src/services/database/encrypted-column.service.ts (1)
25-25: Rename this service file to camelCase.This new path uses kebab-case, but backend service files in this repo are supposed to use camelCase (
encryptedColumn.service.ts). Renaming it now avoids spreading a non-standard import path through the codebase.As per coding guidelines "backend/src/**/*.service.ts: Name service files in camelCase with .service.ts suffix (e.g., auth.service.ts)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/services/database/encrypted-column.service.ts` at line 25, Rename the file to follow the project's camelCase service naming convention and update any imports: change the filename from encrypted-column.service.ts to encryptedColumn.service.ts, and then update all import statements referencing EncryptedColumnService (the exported class name EncryptedColumnService) to the new path so imports resolve correctly; ensure build/tests pass after renaming.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/services/database/encrypted-column.service.ts`:
- Around line 97-101: hasAnyEncryptedColumns currently queries
system.encrypted_columns directly and throws on fresh DBs where that table
(created by migration 029) doesn't exist; change hasAnyEncryptedColumns in
encrypted-column.service.ts so it first checks for the relation and returns
false if absent (either by using to_regclass('system.encrypted_columns') IS NULL
or querying pg/class or information_schema for the table) and only then executes
the SELECT EXISTS query, or alternately wrap the existing query in a try/catch
and return false on a "relation does not exist" error; ensure you reference and
update the getPool().query usage inside the async hasAnyEncryptedColumns()
method.
- Around line 107-149: The registry write helpers registerColumn,
unregisterColumn and unregisterTable run queries directly on the pool which
prevents callers from making the registry update part of their DDL transaction;
change each method signature to accept an optional transaction executor (e.g.,
executor/tx: Queryable | PoolClient) defaulting to this.getPool(), run the
INSERT/DELETE using that executor instead of always this.getPool(), and only
call this.clearCache(tableName, tableSchema) when the default pool was used
(i.e., when no external transaction executor was supplied) so callers can pass
their active transaction and keep the registry write atomic with their DDL.
- Around line 217-218: The migration starts a transaction with
client.query('BEGIN') but when originalType === 'text' there is no
write-blocking lock before the ctid scan and the registry row is only inserted
after the scan, so concurrent inserts/updates can commit plaintext into a column
that will be marked encrypted; fix by either (A) acquiring a table-level
write-blocking lock at the start of the migration (e.g., LOCK TABLE <table> IN
ACCESS EXCLUSIVE MODE or the minimal lock that prevents writes) immediately
after client.query('BEGIN') so the ctid scan and registry insertion are atomic,
or (B) implement a two-phase cutover: first change the write-path so new writes
are encrypted (flip a registry flag or update the application/write function),
wait for that cutover to be active, then run the backfill ctid scan to convert
historical rows; ensure the code paths referencing originalType, the ctid scan
routine, and the registry insertion are updated accordingly so no plaintext
writes can occur between scan start and COMMIT.
- Around line 231-235: The SQL skip predicate only checks for the newer "vN:..."
format and thus re-encrypts rows already stored in the legacy
"iv:authTag:ciphertext" format; update the WHERE clause in the query inside
encrypted-column.service.ts (the client.query that selects ctid and
${safeColumn} from ${qualifiedTable}) to exclude both formats by expanding the
regex to OR-match the legacy IV:authTag:ciphertext pattern using the exact hex
lengths produced by EncryptionManager (use the IV and auth-tag lengths from
EncryptionManager to build the regex) so legacy ciphertext rows are skipped
during backfill.
In `@packages/shared-schemas/src/database.schema.ts`:
- Around line 55-80: The validatedColumnSchema.superRefine rules are never
applied because tableSchema and the addColumns branch of
updateTableSchemaRequestSchema still use columnSchema; update those places to
use the refined schema: in tableSchema replace columns: z.array(columnSchema)
with columns: z.array(validatedColumnSchema), and in
updateTableSchemaRequestSchema (the addColumns path) replace columnSchema.omit({
foreignKey: true }) with validatedColumnSchema.omit({ foreignKey: true }) so the
encryption-related validation runs for all column validations.
---
Outside diff comments:
In `@packages/shared-schemas/src/database.schema.ts`:
- Line 87: The tableSchema currently uses columnSchema for its columns array
which skips encryption-related validation; change the columns definition in
tableSchema to use validatedColumnSchema instead of columnSchema so
encrypted-column constraints are enforced (ensure validatedColumnSchema is
imported/defined in the same module) and verify downstream schemas like
createTableRequestSchema that rely on tableSchema now inherit the tighter
validation.
---
Nitpick comments:
In `@backend/src/services/database/encrypted-column.service.ts`:
- Line 25: Rename the file to follow the project's camelCase service naming
convention and update any imports: change the filename from
encrypted-column.service.ts to encryptedColumn.service.ts, and then update all
import statements referencing EncryptedColumnService (the exported class name
EncryptedColumnService) to the new path so imports resolve correctly; ensure
build/tests pass after renaming.
🪄 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: 4727e518-1ed6-42d0-91cd-f3fe7acbe065
📒 Files selected for processing (4)
backend/src/api/routes/database/encryption.routes.tsbackend/src/infra/database/migrations/029_create-encrypted-columns-registry.sqlbackend/src/services/database/encrypted-column.service.tspackages/shared-schemas/src/database.schema.ts
✅ Files skipped from review due to trivial changes (1)
- backend/src/api/routes/database/encryption.routes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/infra/database/migrations/029_create-encrypted-columns-registry.sql
| /** Validated column schema that rejects invalid encrypted column combinations */ | ||
| export const validatedColumnSchema = columnSchema.superRefine((data, ctx) => { | ||
| if (data.encrypted) { | ||
| if (data.isPrimaryKey) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'Encrypted columns cannot be primary keys', | ||
| path: ['isPrimaryKey'], | ||
| }); | ||
| } | ||
| if (data.isUnique) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'Encrypted columns cannot have a unique constraint', | ||
| path: ['isUnique'], | ||
| }); | ||
| } | ||
| if (data.foreignKey) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'Encrypted columns cannot have foreign key references', | ||
| path: ['foreignKey'], | ||
| }); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find usages of validatedColumnSchema across the codebase
echo "=== Searching for validatedColumnSchema imports and usages ==="
rg -n "validatedColumnSchema" --type ts
echo ""
echo "=== Checking how tableSchema is used in API schemas ==="
rg -n -A5 "tableSchema" packages/shared-schemas/src/database-api.schema.tsRepository: InsForge/InsForge
Length of output: 937
🏁 Script executed:
# Get context around tableSchema definition
sed -n '70,100p' packages/shared-schemas/src/database.schema.tsRepository: InsForge/InsForge
Length of output: 1058
🏁 Script executed:
# Check for other uses of columnSchema in API schema contexts
rg -n "columnSchema" packages/shared-schemas/src/database-api.schema.tsRepository: InsForge/InsForge
Length of output: 107
🏁 Script executed:
# Get context around the updateTableSchemaRequestSchema
sed -n '34,50p' packages/shared-schemas/src/database-api.schema.tsRepository: InsForge/InsForge
Length of output: 499
Apply validatedColumnSchema to all column validation points in API schemas.
The superRefine constraints in validatedColumnSchema are correctly defined but unused. Both tableSchema (line 87) and updateTableSchemaRequestSchema.addColumns (line 37) currently use the unrefined columnSchema, allowing API requests to bypass encryption validation constraints.
Update:
- Line 87: change
columns: z.array(columnSchema)tocolumns: z.array(validatedColumnSchema) - Line 37: change
columnSchema.omit({ foreignKey: true })tovalidatedColumnSchema.omit({ foreignKey: true })
Diff for tableSchema
export const tableSchema = z.object({
tableName: z
.string()
.min(1, 'Table name cannot be empty')
.max(64, 'Table name must be less than 64 characters'),
- columns: z.array(columnSchema).min(1, 'At least one column is required'),
+ columns: z.array(validatedColumnSchema).min(1, 'At least one column is required'),
recordCount: z.number().optional(),
createdAt: z.string().optional(),
updatedAt: z.string().optional(),
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-schemas/src/database.schema.ts` around lines 55 - 80, The
validatedColumnSchema.superRefine rules are never applied because tableSchema
and the addColumns branch of updateTableSchemaRequestSchema still use
columnSchema; update those places to use the refined schema: in tableSchema
replace columns: z.array(columnSchema) with columns:
z.array(validatedColumnSchema), and in updateTableSchemaRequestSchema (the
addColumns path) replace columnSchema.omit({ foreignKey: true }) with
validatedColumnSchema.omit({ foreignKey: true }) so the encryption-related
validation runs for all column validations.
…l encryption (InsForge#1064) - Add superRefine validation to prevent encrypted columns from being primary keys or having unique constraints in addColumns schema - Use validatedColumnSchema in tableSchema for consistent validation - Harden EncryptedColumnService with improved error handling - Add comprehensive unit tests for schema validation and service (encrypt/decrypt, type casting, cache behavior, registry operations)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/tests/unit/encrypted-column-schema.test.ts (1)
169-215: Add a regression test for encrypted-column foreign key addition in the same update request.This block validates
addColumns, but it does not coveraddForeignKeysagainst newly encrypted columns. Please add a case whereaddColumnscreates{ encrypted: true }andaddForeignKeysreferences that same column, expecting parse failure.🧪 Suggested test addition
describe('updateTableSchemaRequestSchema uses validatedColumnSchema for addColumns', () => { @@ test('accepts addColumns with valid encrypted column', () => { const result = updateTableSchemaRequestSchema.safeParse({ addColumns: [ { columnName: 'secret', type: 'string', isNullable: true, isUnique: false, encrypted: true, }, ], }); expect(result.success).toBe(true); }); + + test('rejects addForeignKeys targeting encrypted columns added in same request', () => { + const result = updateTableSchemaRequestSchema.safeParse({ + addColumns: [ + { + columnName: 'secret', + type: 'string', + isNullable: false, + isUnique: false, + encrypted: true, + }, + ], + addForeignKeys: [ + { + columnName: 'secret', + foreignKey: { + referenceTable: 'users', + referenceColumn: 'id', + onDelete: 'CASCADE', + onUpdate: 'CASCADE', + }, + }, + ], + }); + expect(result.success).toBe(false); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/encrypted-column-schema.test.ts` around lines 169 - 215, Add a regression test to updateTableSchemaRequestSchema that ensures addForeignKeys referencing columns added as encrypted in the same request are rejected: create a new test in encrypted-column-schema.test.ts that calls updateTableSchemaRequestSchema.safeParse with addColumns containing a column object with encrypted: true (e.g., columnName: 'secret') and addForeignKeys referencing that same columnName, then assert result.success is false; reference updateTableSchemaRequestSchema, addColumns and addForeignKeys to locate where to add the test.backend/tests/unit/encrypted-column-service.test.ts (1)
52-370: Please add regression coverage for the migration and proxy paths.This suite never exercises
migrateColumnToEncrypted(),reEncryptColumn(), legacy ciphertext handling, or the record-route cutover wheregetEncryptedColumns()drives transparent encrypt/decrypt. Those are the highest-risk flows in this PR, and they still have no tests pinning their behavior.Based on learnings "Applies to tests/**/*.test.{ts,js} : Implement integration tests for API endpoints".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/encrypted-column-service.test.ts` around lines 52 - 370, Tests currently miss regression coverage for migration/proxy flows: add unit/integration tests that exercise migrateColumnToEncrypted and reEncryptColumn, legacy ciphertext handling, and the record-route cutover where getEncryptedColumns drives transparent encrypt/decrypt. Specifically, add cases that (1) mock DB rows and an executor to call migrateColumnToEncrypted and verify it issues the expected INSERT/UPDATE/DELETE queries and clears or preserves cache appropriately; (2) call reEncryptColumn with a tx executor and assert it reads legacy ciphertext, writes new versioned ciphertext, and invokes the expected queries; (3) produce legacy-format ciphertext (use EncryptionManager or a test helper) and assert decryptRow correctly handles legacy values and casts back to original types after getEncryptedColumns triggers the proxy/migration path; and (4) add an integration-style test where getEncryptedColumns returns registry entries, then encryptRow/decryptRow are exercised end-to-end to confirm transparent encrypt/decrypt during the record-route cutover. Use the existing mocks (mockQuery, vi.fn executors) and reference the methods migrateColumnToEncrypted, reEncryptColumn, getEncryptedColumns, encryptRow, and decryptRow to locate code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/services/database/encrypted-column.service.ts`:
- Around line 34-36: The in-memory cache (private cache Map and clearCache())
causes cross-process staleness so other API nodes may keep writing plaintext
after registerColumn() or migrateColumnToEncrypted() commits; change the
invalidation to be global by publishing cache invalidation events (e.g.,
PostgreSQL LISTEN/NOTIFY) or flip the cutover to enable encrypt-on-write before
backfill starts. Concretely: when registerColumn() or migrateColumnToEncrypted()
commits, emit a NOTIFY message (or update a shared DB flag) and modify the
cache-handling code (clearCache() and any place that sets/reads the cache used
by records.routes) to subscribe to that channel and invalidate entries on
notification; alternatively implement the cutover reversal so write-time
encryption is enabled prior to backfill instead of relying on local-only cache
invalidation.
- Around line 72-77: The getEncryptedColumns() function should guard against the
registry table being absent: before running the query in
encrypted-column.service.ts (the call using this.getPool().query(...) that
selects from system.encrypted_columns), check table existence via SELECT
to_regclass('system.encrypted_columns') IS NOT NULL (or call
hasAnyEncryptedColumns() but ensure you do an uncached existence check) and if
the table is missing immediately return an uncached empty Map() instead of
executing the SELECT; keep the rest of getEncryptedColumns() logic unchanged so
callers (e.g., hasAnyEncryptedColumns() and routes) receive a safe empty map on
fresh/pre-migration DBs.
In `@packages/shared-schemas/src/database-api.schema.ts`:
- Around line 37-53: The request schema currently checks per-column rules in
columnSchema.superRefine (preventing encrypted columns from being primary or
unique) but does not prevent addForeignKeys from referencing newly added
encrypted columns; update the overall operation/request schema that composes
addColumns and addForeignKeys to include an object-level superRefine which:
iterates addColumns to collect the names of columns where encrypted === true,
then iterates addForeignKeys and for each foreign key's column name ensures it
is not in that encrypted set, and if it is call ctx.addIssue with a clear custom
message (e.g., "Cannot add foreign key to an encrypted column") and the
appropriate path into addForeignKeys; reference the existing symbols addColumns
and addForeignKeys (and columnSchema) when adding this validation so the check
runs atomically for the combined payload.
---
Nitpick comments:
In `@backend/tests/unit/encrypted-column-schema.test.ts`:
- Around line 169-215: Add a regression test to updateTableSchemaRequestSchema
that ensures addForeignKeys referencing columns added as encrypted in the same
request are rejected: create a new test in encrypted-column-schema.test.ts that
calls updateTableSchemaRequestSchema.safeParse with addColumns containing a
column object with encrypted: true (e.g., columnName: 'secret') and
addForeignKeys referencing that same columnName, then assert result.success is
false; reference updateTableSchemaRequestSchema, addColumns and addForeignKeys
to locate where to add the test.
In `@backend/tests/unit/encrypted-column-service.test.ts`:
- Around line 52-370: Tests currently miss regression coverage for
migration/proxy flows: add unit/integration tests that exercise
migrateColumnToEncrypted and reEncryptColumn, legacy ciphertext handling, and
the record-route cutover where getEncryptedColumns drives transparent
encrypt/decrypt. Specifically, add cases that (1) mock DB rows and an executor
to call migrateColumnToEncrypted and verify it issues the expected
INSERT/UPDATE/DELETE queries and clears or preserves cache appropriately; (2)
call reEncryptColumn with a tx executor and assert it reads legacy ciphertext,
writes new versioned ciphertext, and invokes the expected queries; (3) produce
legacy-format ciphertext (use EncryptionManager or a test helper) and assert
decryptRow correctly handles legacy values and casts back to original types
after getEncryptedColumns triggers the proxy/migration path; and (4) add an
integration-style test where getEncryptedColumns returns registry entries, then
encryptRow/decryptRow are exercised end-to-end to confirm transparent
encrypt/decrypt during the record-route cutover. Use the existing mocks
(mockQuery, vi.fn executors) and reference the methods migrateColumnToEncrypted,
reEncryptColumn, getEncryptedColumns, encryptRow, and decryptRow to locate code
under test.
🪄 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: 530079dc-6a1c-4744-8d35-4a50636e05e5
📒 Files selected for processing (5)
backend/src/services/database/encrypted-column.service.tsbackend/tests/unit/encrypted-column-schema.test.tsbackend/tests/unit/encrypted-column-service.test.tspackages/shared-schemas/src/database-api.schema.tspackages/shared-schemas/src/database.schema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared-schemas/src/database.schema.ts
| // Cache keyed by "schema.table" | ||
| private cache = new Map<string, CacheEntry>(); | ||
| private static readonly CACHE_TTL = 5 * 60 * 1000; // 5 minutes |
There was a problem hiding this comment.
Local-only cache invalidation can leave other API nodes writing plaintext.
If this runs on more than one backend process, clearCache() only updates the process that handled the registry change, while backend/src/api/routes/database/records.routes.ts:49-99 uses this cache to decide whether writes should be encrypted. After migrateColumnToEncrypted() or registerColumn() commits, another instance can keep a stale empty entry for up to 5 minutes and continue inserting plaintext into a column that is now registered as encrypted. Push invalidation through a shared channel (for example LISTEN/NOTIFY) or switch the cutover so encrypt-on-write is enabled before the backfill starts.
As per coding guidelines "Implement proper caching strategies".
Also applies to: 137-139, 158-160, 178-180, 312-312, 396-396, 447-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/services/database/encrypted-column.service.ts` around lines 34 -
36, The in-memory cache (private cache Map and clearCache()) causes
cross-process staleness so other API nodes may keep writing plaintext after
registerColumn() or migrateColumnToEncrypted() commits; change the invalidation
to be global by publishing cache invalidation events (e.g., PostgreSQL
LISTEN/NOTIFY) or flip the cutover to enable encrypt-on-write before backfill
starts. Concretely: when registerColumn() or migrateColumnToEncrypted() commits,
emit a NOTIFY message (or update a shared DB flag) and modify the cache-handling
code (clearCache() and any place that sets/reads the cache used by
records.routes) to subscribe to that channel and invalidate entries on
notification; alternatively implement the cutover reversal so write-time
encryption is enabled prior to backfill instead of relying on local-only cache
invalidation.
There was a problem hiding this comment.
InsForge runs as a single backend process. Multi-node cache invalidation is over-engineering.
There was a problem hiding this comment.
@Davidson3556, understood — thanks for the clarification. If InsForge is deployed as a single backend process, local in-memory cache invalidation via clearCache() is entirely sufficient and there's no cross-process staleness risk. I'll note this for future reviews.
✏️ Learnings added
Learnt from: Davidson3556
Repo: InsForge/InsForge PR: 1067
File: backend/src/services/database/encrypted-column.service.ts:34-36
Timestamp: 2026-04-06T17:49:50.951Z
Learning: InsForge (`InsForge/InsForge`) runs as a single backend process. Multi-node or cross-process cache invalidation (e.g., via PostgreSQL LISTEN/NOTIFY) is not needed for `EncryptedColumnService`'s in-memory cache in `backend/src/services/database/encrypted-column.service.ts`. Do not flag local-only `clearCache()` as a multi-node staleness risk.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Davidson3556
Repo: InsForge/InsForge PR: 1067
File: backend/src/infra/security/encryption.manager.ts:108-113
Timestamp: 2026-04-05T09:01:07.043Z
Learning: In `backend/src/infra/security/encryption.manager.ts`, `EncryptionManager.isConfigured()` intentionally returns `true` when only `JWT_SECRET` is set (no dedicated `ENCRYPTION_KEY`). The JWT_SECRET fallback is considered acceptable for column-level encryption; the risk is mitigated by a `logger.warn` emitted in `getEncryptionKey()` at first use. Do not flag this as a bug or suggest splitting `isConfigured()` into a dedicated column-encryption check — the maintainer has explicitly declined that change.
Learnt from: Davidson3556
Repo: InsForge/InsForge PR: 1067
File: backend/src/infra/security/encryption.manager.ts:4-5
Timestamp: 2026-04-05T08:59:05.065Z
Learning: In `backend/src/infra/security/encryption.manager.ts`, the `v1:` version prefix in `CURRENT_KEY_VERSION`/`encryptVersioned` is intentionally a forward-compatibility placeholder for v1. Multi-key version→key resolver support for real key rotation is deferred to a future version. Do not flag the absence of a key resolver as a bug in this codebase.
Learnt from: CR
Repo: InsForge/InsForge PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T06:28:41.515Z
Learning: Applies to backend/src/**/*.{ts,js} : Implement proper database indexing
…cryption (InsForge#1064) - Add superRefine validation on addColumns to reject encrypted columns as primary keys or with unique constraints - Add cross-field validation to reject foreign keys on encrypted columns - Add to_regclass guard in getEncryptedColumns for fresh-DB safety - Use validatedColumnSchema in tableSchema for consistent validation - Fix test mocks to account for two-query flow (to_regclass + SELECT) - Add comprehensive unit tests for schema validation and service (encrypt/decrypt, type casting, cache behavior, registry operations)
Davidson3556
force-pushed
the
feat/column-level-encryption
branch
from
0a95253 to
d5440cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/src/services/database/encrypted-column.service.ts (1)
195-209: Consider destructuring only the key.The
_entryvariable is unused. You can simplify to iterate only over keys.♻️ Suggested simplification
- for (const [colName, _entry] of encryptedColumns) { + for (const colName of encryptedColumns.keys()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/services/database/encrypted-column.service.ts` around lines 195 - 209, The loop in encryptRow currently destructures an unused _entry from encryptedColumns; change the iteration to only the keys to remove the unused variable — e.g., iterate with for (const colName of encryptedColumns.keys()) (or for (const colName of encryptedColumns) if you prefer keys() omitted) in the encryptRow method so you only reference colName and drop _entry/its underscore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/services/database/encrypted-column.service.ts`:
- Around line 430-438: The bigint/int8 branch currently uses parseInt on
"decrypted", losing precision; change handling so that for 'bigint' and 'int8'
you do NOT parse to Number or BigInt but return the decrypted string to preserve
precision (leave parsing for 'integer'/'int'/'int4'/'smallint'/'int2' as-is).
Locate the switch handling these types (cases
'integer','int','int4','smallint','int2','bigint','int8'), remove parseInt usage
for 'bigint'/'int8' (and any use of BigInt) and return decrypted as string so
JSON.stringify/res.json and socket broadcasts won't fail. Ensure variable names
like "decrypted" and "parsed" are updated accordingly.
In `@backend/tests/unit/encrypted-column-service.test.ts`:
- Line 1: Run the Prettier formatter on the test file to fix formatting errors:
format backend/tests/unit/encrypted-column-service.test.ts (e.g., npx prettier
--write backend/tests/unit/encrypted-column-service.test.ts) so the import line
"import { describe, test, expect, vi, beforeEach } from 'vitest';" and the rest
of the file adhere to the repo's Prettier rules, then re-run the pipeline to
confirm the formatting issues are resolved.
---
Nitpick comments:
In `@backend/src/services/database/encrypted-column.service.ts`:
- Around line 195-209: The loop in encryptRow currently destructures an unused
_entry from encryptedColumns; change the iteration to only the keys to remove
the unused variable — e.g., iterate with for (const colName of
encryptedColumns.keys()) (or for (const colName of encryptedColumns) if you
prefer keys() omitted) in the encryptRow method so you only reference colName
and drop _entry/its underscore.
🪄 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: 1bc5ae27-fdf8-418c-bb12-8906fe419b16
📒 Files selected for processing (4)
backend/src/services/database/encrypted-column.service.tsbackend/tests/unit/encrypted-column-schema.test.tsbackend/tests/unit/encrypted-column-service.test.tspackages/shared-schemas/src/database-api.schema.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/unit/encrypted-column-schema.test.ts
- packages/shared-schemas/src/database-api.schema.ts
Separate bigint/int8 from the integer parsing path in castDecryptedValue to avoid precision loss beyond Number.MAX_SAFE_INTEGER.
|
@tonychang04 @Fermionic-Lyu @jwfing |
encrypted-column.service.ts:938-1003 reEncryptColumn: Comment says "Assumes the key has already been rotated in the environment." But encryption.manager.ts:11-27 caches a single key derived from process.env.ENCRYPTION_KEY. After rotation, decrypt() uses the new key on ciphertext encrypted with the old key. AES-GCM auth tag verification will fail on the very first row. Every "rotated" call returns 0 rows re-encrypted, or worse, throws and rolls back. The keyVersion column in 029_create-encrypted-columns-registry.sql is cosmetic. There is no keyring, no OLD_ENCRYPTION_KEY, no version → key map. The whole rotation feature is non-functional. |
records.routes.ts:50-95 intercepts writes only inside forwardToPostgrest. But the PR doesn't audit all write paths:
|
|
on it |
encrypted-column.service.ts:638 caches the encrypted-columns map for 5 minutes per process. When you call POST /api/database/tables/foo/encrypt-column, only the instance handling the request clears its cache. Other backend replicas still see the column as plaintext for up to 5 minutes. Writes routed to those replicas during that window will write plaintext into a column that's now expected to be encrypted, silently corrupting data. encryption.routes.ts:104 issues NOTIFY pgrst, 'reload schema' for PostgREST but does nothing to broadcast cache invalidation to peer Node instances. I mention it because that we are discussing backend replication feature, maybe in the future we need to handle it. |
Address two critical findings from review: 1. Key rotation was non-functional. EncryptionManager now loads a versioned key map from ENCRYPTION_KEY_V<N> env vars, with ENCRYPTION_KEY_CURRENT_VERSION selecting which version is used for new encryptions. decrypt() parses the version prefix and looks up the matching key, so old ciphertexts remain decryptable as long as their key version stays in the env. Bare ENCRYPTION_KEY is still loaded as v1 for backward compat. 2. Encrypted columns could be bypassed by writing plaintext through raw SQL, RPC, bulk upsert, or import — only the PostgREST proxy path applied encryptRow. Added a CHECK constraint installed at register/migrate time that requires every value match the ciphertext format, blocking plaintext writes from any path at the database layer. Constraint is dropped on unregister. Tests updated to account for the additional ALTER TABLE queries issued during register/unregister.
|
@jwfing kindly review |
|
@Fermionic-Lyu please kindly when you are less busy |
|
In my opinion, column encryption doesn't actually solve the problem of seeing sensitive data in the dashboard. The dashboard fetches via |
Keep the encrypted-column-aware version of the row-count path: run COUNT(*) and encryptedColumnService.getEncryptedColumns() in parallel with Promise.all. Upstream's SQL-injection fix (quoteIdentifier on the table name) is preserved.
The POST /tables/:tableName/encrypt-column handler previously lived in encryptionRouter mounted at /, which meant it squatted on the /tables namespace owned by tablesRouter and only resolved via Express fall-through. Move the handler into tables.routes.ts where it belongs (table-scoped operation) and mount encryptionRouter at /encryption so its remaining routes (GET / list, POST /rotate) are properly scoped. External URLs are unchanged: POST /api/database/tables/:tableName/encrypt-column GET /api/database/encryption POST /api/database/encryption/rotate
|
Thanks @jwfing , fair critique, and worth being explicit about the threat model this feature addresses. Column encryption protects two things:
It does not protect against authorized API readers. The dashboard calls /database/records, which decrypts on the way out, so admins see plaintext. That's by design, column encryption is storage-level confidentiality, not RBAC. If the intent is "admins shouldn't see SSNs in the dashboard," that's field masking and should be a separate feature (opt-in reveal with audit log, etc.), not a side effect of encryption. On the raw-SQL-returns-ciphertext vs /records-returns-plaintext asymmetry: I think that's the correct tradeoff to keep. Two options considered:
I'd rather go with (2) + a note in the encryption docs. Raw SQL is the power-user escape hatch; showing ciphertext is honest about what's actually stored. Happy to revisit if you disagree, but my preference is not to paper over it with fragile auto-decryption. Also pushed the route-nesting fix (encrypt-column now lives in tables.routes.ts, encryptionRouter mounted at /encryption, external URLs unchanged) and resolved the database-table.service.ts merge conflict. |
|
@Davidson3556 Thanks for the clarification. @tonychang04 had relayed the original intent as preventing sensitive data from being visible in the dashboard, so I'd been reviewing this PR under that assumption — that explains the mismatch on my side. |
|
@jwfing Makes sense, I appreciate the context on the original intent, that explains the framing gap. Happy to wait on internal alignment before doing more here. On the key management concerns, you are right and I won't defend them: Env-only storage is a v1 shortcut, not a production answer. KMS-style integration (AWS KMS, GCP KMS, Vault transit) is the right long-term shape ,keys never sit in plaintext env, and rotation becomes a KMS operation rather than an operator-managed env dance. |
2 participants
Summary
v1:iv:authTag:ciphertext)system.encrypted_columnsregistry table to track which columns are encryptedCloses #1064
###video link
https://www.loom.com/share/9121b36fd7e34c6785ed386607debdfa
How did you test this change?
encryptedcheckbox checked on acredentialscolumn via the dashboard UIv1:0bee9654...)TEXTin the database while reporting the original type (json) in the schema APINote
Add column-level encryption for database fields with key rotation support
EncryptedColumnServiceto manage encrypted column metadata, handle transparent encrypt/decrypt on reads and writes via PostgREST, migrate existing plaintext columns to encrypted, and re-encrypt rows for key rotation.EncryptionManageris extended to support multiple versioned keys viaENCRYPTION_KEY_V<N>env vars, with backward-compatible decryption of legacy ciphertexts.GET /api/database/encryption(list encrypted columns) andPOST /api/database/encryption/rotate(trigger re-encryption), both with audit logging.DatabaseTableServicekeeps thesystem.encrypted_columnsregistry in sync across create, rename, drop, and schema update operations; the schema API reports original types and anencryptedflag.TableFormgains an Encrypted checkbox for new columns; shared Zod schemas enforce that encrypted columns cannot be primary keys, unique, or have foreign keys.Macroscope summarized 16ccde5.
Summary by CodeRabbit
New Features
Validation
Tests
Chores