From ca1571676c0c67f026cd541fecbdc3dc09cc1c59 Mon Sep 17 00:00:00 2001 From: Charles Bochet Date: Thu, 14 May 2026 18:40:41 +0200 Subject: [PATCH] fix(server): treat plaintext-under-isSecret rows as plaintext in app variable encryption migration (#20590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Prod 2.5 upgrade failed on the slow instance command `EncryptApplicationVariableSlowInstanceCommand`: ``` [Nest] LOG [InstanceCommandRunnerService] 2.5.0_EncryptApplicationVariableSlowInstanceCommand_1798000005000 starting data migration... [Nest] WARN [SecretEncryptionService] Decrypted a legacy unprefixed AES-CTR ciphertext... [Nest] ERROR [InstanceCommandRunnerService] data migration failed TypeError: Invalid initialization vector ``` ### Root cause The migration assumes every row matching `isSecret = true AND value <> '' AND value NOT LIKE 'enc:v2:%'` is legacy AES-CTR ciphertext. In prod we found multiple `isSecret = true` rows whose `value` is plaintext (e.g. `SLACK_HOOK_URL = 'https://hooks.slack.com/services/...'`) — most likely the result of `isSecret` being flipped to true on a row that already held a plaintext value, or a write path that bypassed `ApplicationVariableEntityService.update`. Those values can't decode into the 16-byte IV that AES-CTR needs, so `Buffer.from(value, 'base64')` truncates at the first non-base64 char (`:`), the buffer is < 16 bytes, and `createDecipheriv` throws. ### Fix Follow the same policy as `EncryptConnectedAccountTokensSlowInstanceCommand`: anything that isn't already in the `enc:v2:` envelope is plaintext. Concretely: 1. Try `decryptVersioned` — legacy CTR rows decrypt fine. 2. If it throws (mis-classified plaintext), log a warning naming the row id and fall back to treating `row.value` as plaintext. 3. Encrypt the resulting plaintext into the `enc:v2:` envelope and update the row. In-loop `isSecret` guard is kept (alongside the SQL filter) so non-secret rows are never touched even if the SQL filter is ever loosened. ### Integration test coverage Added one new case alongside the existing ones in `…encrypt-application-variable.integration-spec.ts`: - `treats plaintext-under-isSecret=true as plaintext and re-encrypts as v2` — seeds a row with `isSecret = true` and a URL value (`:` and `/` are not base64, so this is the exact failure shape from prod), runs the migration, and asserts the value is now `enc:v2:...` and decrypts back to the original URL. Existing cases unchanged: legacy CTR happy path, non-secret rows untouched, idempotent across re-runs, `up()` adds the CHECK constraint, `down()` removes it. ### Why this is a 2-5 edit `TWENTY_CURRENT_VERSION` is now 2.6.0, so editing a 2-5 file trips the `server-previous-version-upgrade-mutation-guard` — `ci:allow-previous-version-upgrade-mutation` label is on the PR. `up()` and `down()` are unchanged; only `runDataMigration` is modified. ## Test plan - [ ] Re-deploy 2.5 to prod and confirm `EncryptApplicationVariableSlowInstanceCommand` completes - [ ] Inspect warning log to count rows that went through the plaintext fallback - [ ] Verify resulting secret rows all satisfy `value = '' OR value LIKE 'enc:v2:%'` and the CHECK constraint is in place --- ...8000005000-encrypt-application-variable.ts | 62 +++++++++++++++---- ...t-application-variable.integration-spec.ts | 20 ++++++ 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/packages/twenty-server/src/database/commands/upgrade-version-command/2-5/2-5-instance-command-slow-1798000005000-encrypt-application-variable.ts b/packages/twenty-server/src/database/commands/upgrade-version-command/2-5/2-5-instance-command-slow-1798000005000-encrypt-application-variable.ts index babaf6545fd..23f70f04f54 100644 --- a/packages/twenty-server/src/database/commands/upgrade-version-command/2-5/2-5-instance-command-slow-1798000005000-encrypt-application-variable.ts +++ b/packages/twenty-server/src/database/commands/upgrade-version-command/2-5/2-5-instance-command-slow-1798000005000-encrypt-application-variable.ts @@ -1,3 +1,5 @@ +import { Logger } from '@nestjs/common'; + import { isDefined } from 'twenty-shared/utils'; import { DataSource, QueryRunner } from 'typeorm'; @@ -12,30 +14,47 @@ const VALUE_CHECK_CONSTRAINT_NAME = 'CHK_applicationVariable_value_encrypted'; const V2_ENCRYPTED_LIKE_PATTERN = `${SECRET_ENCRYPTION_ENVELOPE_V2_PREFIX}%`; +// Legacy CTR ciphertext is base64-encoded and at least 16 bytes (one IV +// block) — i.e. ≥ 22 base64 chars. Anything outside that shape is plaintext. +// Node's `Buffer.from(value, 'base64')` silently skips invalid chars, so a +// URL like `https://hooks.slack.com/...` would otherwise decode into enough +// bytes to "decrypt" to garbage without throwing. +const LEGACY_CTR_LOOKS_LIKE_BASE64_RE = /^[A-Za-z0-9+/]+={0,2}$/; +const LEGACY_CTR_MIN_LENGTH = 22; + type ApplicationVariableRow = { id: string; workspaceId: string; value: string; + isSecret: boolean; }; +const looksLikeLegacyCtrCiphertext = (value: string): boolean => + value.length >= LEGACY_CTR_MIN_LENGTH && + LEGACY_CTR_LOOKS_LIKE_BASE64_RE.test(value); + @RegisteredInstanceCommand('2.5.0', 1798000005000, { type: 'slow' }) export class EncryptApplicationVariableSlowInstanceCommand implements SlowInstanceCommand { + private readonly logger = new Logger( + EncryptApplicationVariableSlowInstanceCommand.name, + ); + constructor( private readonly secretEncryptionService: SecretEncryptionService, ) {} - // Re-encrypts every secret application variable into the versioned envelope - // bound to its row's workspaceId. Non-secret rows are left untouched — - // their `value` is plaintext by design. Idempotent: the SELECT filter - // skips rows already in v2 form. + // Re-encrypts secret application variables into the v2 envelope. Rows + // marked isSecret=true with a plaintext value (instead of legacy CTR + // ciphertext) are treated as plaintext and encrypted, mirroring + // EncryptConnectedAccountTokensSlowInstanceCommand. async runDataMigration(dataSource: DataSource): Promise { let cursor = '00000000-0000-0000-0000-000000000000'; while (true) { const rows: ApplicationVariableRow[] = await dataSource.query( - `SELECT id, "workspaceId", "value" + `SELECT id, "workspaceId", "value", "isSecret" FROM "core"."applicationVariable" WHERE id > $1 AND "isSecret" = true @@ -51,13 +70,32 @@ export class EncryptApplicationVariableSlowInstanceCommand } for (const row of rows) { - // decryptVersioned handles legacy unprefixed CTR ciphertext by - // falling through to the raw-key decrypt path — exactly what we - // need to read the pre-migration rows. - const plaintext = this.secretEncryptionService.decryptVersioned( - row.value, - { workspaceId: row.workspaceId }, - ); + if (!row.isSecret) { + continue; + } + + let plaintext: string; + + if (looksLikeLegacyCtrCiphertext(row.value)) { + try { + plaintext = this.secretEncryptionService.decryptVersioned( + row.value, + { workspaceId: row.workspaceId }, + ); + } catch (error) { + this.logger.warn( + `applicationVariable row ${row.id} value not valid ciphertext; treating as plaintext. ${ + error instanceof Error ? error.message : String(error) + }`, + ); + plaintext = row.value; + } + } else { + this.logger.warn( + `applicationVariable row ${row.id} value is not base64; treating as plaintext.`, + ); + plaintext = row.value; + } if (!isDefined(plaintext)) { continue; diff --git a/packages/twenty-server/test/integration/upgrade/suites/2-5-instance-command-slow-1798000005000-encrypt-application-variable.integration-spec.ts b/packages/twenty-server/test/integration/upgrade/suites/2-5-instance-command-slow-1798000005000-encrypt-application-variable.integration-spec.ts index 7bdacc3fd18..2d630ecb18c 100644 --- a/packages/twenty-server/test/integration/upgrade/suites/2-5-instance-command-slow-1798000005000-encrypt-application-variable.integration-spec.ts +++ b/packages/twenty-server/test/integration/upgrade/suites/2-5-instance-command-slow-1798000005000-encrypt-application-variable.integration-spec.ts @@ -164,6 +164,26 @@ describe('2-5 slow instance command 1798000005000 - EncryptApplicationVariableSl expect(row.value).toBe(plaintext); }); + it('treats plaintext-under-isSecret=true as plaintext and re-encrypts as v2', async () => { + const plaintext = + 'https://hooks.slack.com/services/T09QGPB2ZP1/B09QUQ5LY2Z/abc'; + const id = await seedRow({ isSecret: true, value: plaintext }); + + await command.runDataMigration(dataSource); + + const [row] = await dataSource.query( + `SELECT "value" FROM "core"."applicationVariable" WHERE id = $1`, + [id], + ); + + expect(row.value.startsWith(SECRET_ENCRYPTION_ENVELOPE_V2_PREFIX)).toBe( + true, + ); + expect( + secretEncryptionService.decryptVersioned(row.value, { workspaceId }), + ).toBe(plaintext); + }); + it('leaves enc:v2 rows untouched and is idempotent across re-runs', async () => { const plaintext = 'already-v2-secret'; const preexistingV2 = secretEncryptionService.encryptVersioned(plaintext, {