fix(server): treat plaintext-under-isSecret rows as plaintext in app variable encryption migration (#20590)

## 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
This commit is contained in:
Charles Bochet
2026-05-14 18:40:41 +02:00
committed by GitHub
parent a5880bd8d0
commit ca1571676c
2 changed files with 70 additions and 12 deletions

View File

@@ -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<void> {
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;

View File

@@ -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, {