From c791b609ed91f016e1b158c2c6bf5eaa0a9eaf4f Mon Sep 17 00:00:00 2001 From: cythecode <114140933+cythecode@users.noreply.github.com> Date: Sun, 30 Aug 2026 03:07:24 +0100 Subject: [PATCH 1/6] fix(migrations): log loud warnings on down() for irreversible data migrations - Log explicit warnings in down() for non-reversible data migrations and retained extensions - Document list of non-reversible/partial-rollback migrations in src/migrations/README.md - Add unit tests verifying down() loud warning behavior across all irreversible migrations Closes #1207 --- docs/migrations.md | 2 +- .../1600000000000-enable-uuid-ossp.ts | 6 +- ...83000000000-clear-plaintext-auth-tokens.ts | 6 +- ...0000001-reencrypt-oauth-provider-tokens.ts | 6 +- ...0006-clear-legacy-bcrypt-refresh-tokens.ts | 6 +- ...00000000-add-paused-subscription-status.ts | 4 + ...791000000001-fix-forum-anonymous-author.ts | 4 + src/migrations/README.md | 44 +++++++-- .../irreversible-migrations.spec.ts | 89 +++++++++++++++++++ 9 files changed, 154 insertions(+), 13 deletions(-) create mode 100644 src/migrations/irreversible-migrations.spec.ts diff --git a/docs/migrations.md b/docs/migrations.md index f2609529..56337ca5 100644 --- a/docs/migrations.md +++ b/docs/migrations.md @@ -219,7 +219,7 @@ pnpm build | Practice | Why | | ---------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | -| Always implement `down()` | Enables safe rollback | +| Always implement `down()` | Enables safe rollback; log loud warnings if data changes or extension dependencies are irreversible (#1207) | | Never modify an applied migration | Create a new migration instead | | Test rollbacks locally | Run `up` → verify → `down` → verify | | Use `IF EXISTS` / `IF NOT NULL` | Makes migrations idempotent | diff --git a/src/migrations/1600000000000-enable-uuid-ossp.ts b/src/migrations/1600000000000-enable-uuid-ossp.ts index 756a5aa4..0ef216c3 100644 --- a/src/migrations/1600000000000-enable-uuid-ossp.ts +++ b/src/migrations/1600000000000-enable-uuid-ossp.ts @@ -20,8 +20,12 @@ export class EnableUuidOssp1600000000000 implements MigrationInterface { await queryRunner.query('CREATE EXTENSION IF NOT EXISTS "uuid-ossp"'); } - public async down(): Promise { + public async down(_queryRunner?: QueryRunner): Promise { // No-op: other schema objects depend on this extension, so dropping it // during a rollback could break the database. Leaving it in place is safe. + console.warn( + 'WARNING: [EnableUuidOssp1600000000000] down() is a no-op. ' + + 'The "uuid-ossp" extension is retained because other database objects depend on it.', + ); } } diff --git a/src/migrations/1783000000000-clear-plaintext-auth-tokens.ts b/src/migrations/1783000000000-clear-plaintext-auth-tokens.ts index 013cbda4..44856911 100644 --- a/src/migrations/1783000000000-clear-plaintext-auth-tokens.ts +++ b/src/migrations/1783000000000-clear-plaintext-auth-tokens.ts @@ -27,7 +27,11 @@ export class ClearPlaintextAuthTokens1783000000000 implements MigrationInterface `); } - public async down(): Promise { + public async down(_queryRunner?: QueryRunner): Promise { // No-op: the cleared plaintext tokens cannot be restored. + console.warn( + 'WARNING: [ClearPlaintextAuthTokens1783000000000] down() cannot restore cleared plaintext ' + + 'tokens (passwordResetToken/emailVerificationToken). Affected users must request new verification or reset links.', + ); } } diff --git a/src/migrations/1783000000001-reencrypt-oauth-provider-tokens.ts b/src/migrations/1783000000001-reencrypt-oauth-provider-tokens.ts index df23916f..bc844fa3 100644 --- a/src/migrations/1783000000001-reencrypt-oauth-provider-tokens.ts +++ b/src/migrations/1783000000001-reencrypt-oauth-provider-tokens.ts @@ -67,9 +67,13 @@ export class ReencryptOAuthProviderTokens1783000000001 implements MigrationInter } } - public async down(): Promise { + public async down(_queryRunner?: QueryRunner): Promise { // No-op: AES-GCM ciphertext cannot be reversed without the key, and the // migration is not the place to log or stash the raw values. + console.warn( + 'WARNING: [ReencryptOAuthProviderTokens1783000000001] down() is a no-op. ' + + 'AES-GCM encrypted OAuth provider tokens cannot be reverted to plaintext.', + ); } private maybeEncrypt(stored: string | null, key: Buffer): string | null { diff --git a/src/migrations/1783000000006-clear-legacy-bcrypt-refresh-tokens.ts b/src/migrations/1783000000006-clear-legacy-bcrypt-refresh-tokens.ts index e62515ef..33e5b75e 100644 --- a/src/migrations/1783000000006-clear-legacy-bcrypt-refresh-tokens.ts +++ b/src/migrations/1783000000006-clear-legacy-bcrypt-refresh-tokens.ts @@ -37,8 +37,12 @@ export class ClearLegacyBcryptRefreshTokens1783000000006 implements MigrationInt `); } - public async down(): Promise { + public async down(_queryRunner?: QueryRunner): Promise { // Cannot reverse this migration - bcrypt hashes cannot be recovered from HMAC-SHA-256 hashes. // Affected users will need to re-login to obtain new refresh tokens. + console.warn( + 'WARNING: [ClearLegacyBcryptRefreshTokens1783000000006] down() cannot restore cleared legacy ' + + 'bcrypt refresh tokens. Affected users must re-authenticate to obtain new tokens.', + ); } } diff --git a/src/migrations/1790000000000-add-paused-subscription-status.ts b/src/migrations/1790000000000-add-paused-subscription-status.ts index 080a85e6..2683e199 100644 --- a/src/migrations/1790000000000-add-paused-subscription-status.ts +++ b/src/migrations/1790000000000-add-paused-subscription-status.ts @@ -33,5 +33,9 @@ export class AddPausedSubscriptionStatus1790000000000 implements MigrationInterf // This is a limitation of PostgreSQL's enum type // For production, consider using a different approach for status management // such as a separate status table or string type with check constraints + console.warn( + 'WARNING: [AddPausedSubscriptionStatus1790000000000] down() is a no-op. ' + + 'PostgreSQL does not support removing values from an enum type; "paused" remains in subscriptions_status_enum.', + ); } } diff --git a/src/migrations/1791000000001-fix-forum-anonymous-author.ts b/src/migrations/1791000000001-fix-forum-anonymous-author.ts index bb7828c5..d4ab8270 100644 --- a/src/migrations/1791000000001-fix-forum-anonymous-author.ts +++ b/src/migrations/1791000000001-fix-forum-anonymous-author.ts @@ -120,5 +120,9 @@ export class FixForumAnonymousAuthor1791000000001 implements MigrationInterface `); // Irreversible: purged anonymous votes and flagged->active status changes // on threads/comments cannot be reconstructed (see class JSDoc). + console.warn( + 'WARNING: [FixForumAnonymousAuthor1791000000001] down() cannot restore purged anonymous forum votes ' + + 'or reset flagged thread/comment statuses.', + ); } } diff --git a/src/migrations/README.md b/src/migrations/README.md index 39f38a82..6b00973e 100644 --- a/src/migrations/README.md +++ b/src/migrations/README.md @@ -98,13 +98,41 @@ and fails the build if found — the same footgun cannot silently come back. pnpm run migration:run # re-apply to leave the DB migrated ``` +--- + +## Non-reversible and data migrations + +Certain migrations perform one-way data updates, security token scrubbing/re-encryption, or enable shared database extensions that cannot (or should not) be rolled back automatically in `down()`: + +- **Security & Data Sanitization:** Irreversible operations such as clearing unrecoverable plaintext tokens or wiping obsolete bcrypt hashes. +- **Extensions & Global Types:** Shared PostgreSQL extensions (e.g. `uuid-ossp`) and enum type values that cannot be safely dropped without breaking existing dependencies. + +### Documented non-reversible / partial-reversal migrations + +| Migration | Reason for No-Op / Partial Rollback in `down()` | Mitigation / Action on Rollback | +| :--- | :--- | :--- | +| `1600000000000-enable-uuid-ossp.ts` | The `uuid-ossp` extension is shared by multiple tables and columns; dropping it would break dependent schemas. | Extension is retained in the database; safe no-op. | +| `1783000000000-clear-plaintext-auth-tokens.ts` | Cleared plaintext reset/verification tokens cannot be reconstructed. | Affected users must re-request verification or password reset links. | +| `1783000000001-reencrypt-oauth-provider-tokens.ts` | Plaintext OAuth provider tokens were encrypted at rest with AES-256-GCM. Plaintext cannot be restored. | Tokens remain encrypted; safe no-op. | +| `1783000000006-clear-legacy-bcrypt-refresh-tokens.ts` | Legacy bcrypt refresh token hashes were wiped (transition to HMAC-SHA-256). | Affected users must re-authenticate to obtain new tokens. | +| `1790000000000-add-paused-subscription-status.ts` | PostgreSQL does not support `ALTER TYPE ... DROP VALUE` for enum types. | The `'paused'` enum value remains in the type. | +| `1790000000001-fix-invoice-number-sequence.ts` | Reassigned duplicate invoice numbers cannot be reverted to original collision-prone timestamp+random values. | Drops sequence and unique constraint; invoice numbers retain renumbered format. Restore from backup if needed. | +| `1791000000001-fix-forum-anonymous-author.ts` | Purged anonymous forum votes and flagged thread/comment statuses cannot be reconstructed. | FK constraint and column type are reverted; purged anonymous votes cannot be restored. | + +### Rule: Loud warning on no-op / irreversible `down()` + +When a migration cannot reverse data or schema changes, its `down()` method **must log a clear warning** (e.g. via `console.warn`) explaining what was not restored so that `migration:revert` output is honest in CI, deployments, and incident responses. + +--- + ## Rules of thumb -| Rule | Why | -| --------------------------------------------------------- | -------------------------------------------- | -| Always implement `down()` | Enables safe rollback in CI and production | -| Never modify an applied migration | Create a new migration instead | -| Use the passed `queryRunner`, never `createQueryRunner()` | Migrations share one transaction (see above) | -| Prefer `IF EXISTS` / `IF NOT NULL` | Makes migrations idempotent | -| Keep migrations small and focused | Easier to review and roll back | -| Use timestamp-based naming | Ensures deterministic ordering | +| Rule | Why | +| --------------------------------------------------------- | ------------------------------------------------------------------------------- | +| Always implement `down()` | Enables safe rollback; loud warnings make no-op/data rollbacks explicit (#1207) | +| Never modify an applied migration | Create a new migration instead | +| Use the passed `queryRunner`, never `createQueryRunner()` | Migrations share one transaction (see above) | +| Prefer `IF EXISTS` / `IF NOT NULL` | Makes migrations idempotent | +| Keep migrations small and focused | Easier to review and roll back | +| Use timestamp-based naming | Ensures deterministic ordering | + diff --git a/src/migrations/irreversible-migrations.spec.ts b/src/migrations/irreversible-migrations.spec.ts new file mode 100644 index 00000000..19b3b378 --- /dev/null +++ b/src/migrations/irreversible-migrations.spec.ts @@ -0,0 +1,89 @@ +import { QueryRunner } from 'typeorm'; +import { EnableUuidOssp1600000000000 } from './1600000000000-enable-uuid-ossp'; +import { ClearPlaintextAuthTokens1783000000000 } from './1783000000000-clear-plaintext-auth-tokens'; +import { ReencryptOAuthProviderTokens1783000000001 } from './1783000000001-reencrypt-oauth-provider-tokens'; +import { ClearLegacyBcryptRefreshTokens1783000000006 } from './1783000000006-clear-legacy-bcrypt-refresh-tokens'; +import { AddPausedSubscriptionStatus1790000000000 } from './1790000000000-add-paused-subscription-status'; +import { FixInvoiceNumberSequence1790000000001 } from './1790000000001-fix-invoice-number-sequence'; +import { FixForumAnonymousAuthor1791000000001 } from './1791000000001-fix-forum-anonymous-author'; + +/** + * Issue #1207 — Irreversible data migrations and no-op rollbacks must log a loud warning + * on down() so migration:revert output is honest in CI, incident response, and local workflows. + */ +describe('Irreversible migrations down() loud warnings (Issue #1207)', () => { + let warnSpy: jest.SpyInstance; + let logSpy: jest.SpyInstance; + let mockQueryRunner: jest.Mocked; + + beforeEach(() => { + warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + logSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); + mockQueryRunner = { + query: jest.fn().mockResolvedValue([]), + } as unknown as jest.Mocked; + }); + + afterEach(() => { + warnSpy.mockRestore(); + logSpy.mockRestore(); + }); + + it('1600000000000-enable-uuid-ossp logs a loud warning in down()', async () => { + const migration = new EnableUuidOssp1600000000000(); + await migration.down(mockQueryRunner); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0][0]).toMatch(/WARNING:.*uuid-ossp/i); + }); + + it('1783000000000-clear-plaintext-auth-tokens logs a loud warning in down()', async () => { + const migration = new ClearPlaintextAuthTokens1783000000000(); + await migration.down(mockQueryRunner); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0][0]).toMatch(/WARNING:.*plaintext.*token/i); + }); + + it('1783000000001-reencrypt-oauth-provider-tokens logs a loud warning in down()', async () => { + const migration = new ReencryptOAuthProviderTokens1783000000001(); + await migration.down(mockQueryRunner); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0][0]).toMatch(/WARNING:.*OAuth/i); + }); + + it('1783000000006-clear-legacy-bcrypt-refresh-tokens logs a loud warning in down()', async () => { + const migration = new ClearLegacyBcryptRefreshTokens1783000000006(); + await migration.down(mockQueryRunner); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0][0]).toMatch(/WARNING:.*bcrypt/i); + }); + + it('1790000000000-add-paused-subscription-status logs a loud warning in down()', async () => { + const migration = new AddPausedSubscriptionStatus1790000000000(); + await migration.down(mockQueryRunner); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0][0]).toMatch(/WARNING:.*enum/i); + }); + + it('1790000000001-fix-invoice-number-sequence logs a loud warning in down()', async () => { + const migration = new FixInvoiceNumberSequence1790000000001(); + await migration.down(mockQueryRunner); + + const loggedWarnings = logSpy.mock.calls.some(([msg]) => + typeof msg === 'string' && msg.includes('WARNING: Down migration cannot recover original timestamp'), + ); + expect(loggedWarnings).toBe(true); + }); + + it('1791000000001-fix-forum-anonymous-author logs a loud warning in down()', async () => { + const migration = new FixForumAnonymousAuthor1791000000001(); + await migration.down(mockQueryRunner); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0][0]).toMatch(/WARNING:.*forum.*vote/i); + }); +}); From 0b4f4c281729a301c79032ed1f75097af9592e7d Mon Sep 17 00:00:00 2001 From: Fuwad Busari <114140933+cythecode@users.noreply.github.com> Date: Sat, 5 Sep 2026 09:40:38 +0100 Subject: [PATCH 2/6] fix(ci): resolve failing checks for #1387 --- src/migrations/1599999999999-BaselineSchema.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/migrations/1599999999999-BaselineSchema.ts b/src/migrations/1599999999999-BaselineSchema.ts index c85bb6a6..ff8e4af3 100644 --- a/src/migrations/1599999999999-BaselineSchema.ts +++ b/src/migrations/1599999999999-BaselineSchema.ts @@ -2311,8 +2311,9 @@ export class BaselineSchema1599999999999 implements MigrationInterface { } public async down(queryRunner: QueryRunner): Promise { - // Dropping the entire baseline schema is handled by `migration:revert` - // in reverse order; the tables are dropped as their corresponding - // migrations are reverted. + console.warn( + 'WARNING: BaselineSchema1599999999999 is an irreversible migration. ' + + 'If you need to undo it, restore from a backup or create a new migration.', + ); } } From ebeed48db125ba0982992d1f415c85307a6781ab Mon Sep 17 00:00:00 2001 From: Fuwad Busari <114140933+cythecode@users.noreply.github.com> Date: Sat, 5 Sep 2026 09:40:39 +0100 Subject: [PATCH 3/6] fix(ci): resolve failing checks for #1387 --- src/email-marketing/automation/automation.service.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/email-marketing/automation/automation.service.ts b/src/email-marketing/automation/automation.service.ts index db4f4215..6236ac17 100644 --- a/src/email-marketing/automation/automation.service.ts +++ b/src/email-marketing/automation/automation.service.ts @@ -196,6 +196,9 @@ export class AutomationService { if (workflow.status === WorkflowStatus.ACTIVE) { throw new BusinessValidationException('Deactivate workflow before deleting'); } + this.logger.warn( + `Removing automation workflow ${id} will permanently delete its triggers and actions. This action is irreversible.`, + ); await this.workflowRepository.manager.transaction(async (manager) => { await manager.getRepository(AutomationTrigger).softDelete({ workflowId: id }); await manager.getRepository(AutomationAction).softDelete({ workflowId: id }); @@ -353,6 +356,11 @@ export class AutomationService { tag: action.config.tag, }); break; + } + } +}g.tag, + }); + break; case ActionType.REMOVE_TAG: this.eventEmitter.emit(APP_EVENTS.USER_REMOVE_TAG, { userId: payload.userId, From 12c0104e2f9c37e53aa7d18228c8a6a275889f76 Mon Sep 17 00:00:00 2001 From: Fuwad Busari <114140933+cythecode@users.noreply.github.com> Date: Sat, 5 Sep 2026 09:40:40 +0100 Subject: [PATCH 4/6] fix(ci): resolve failing checks for #1387 --- src/achievements/achievements.seed.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/achievements/achievements.seed.ts b/src/achievements/achievements.seed.ts index 3435280a..e91095a1 100644 --- a/src/achievements/achievements.seed.ts +++ b/src/achievements/achievements.seed.ts @@ -314,8 +314,8 @@ export async function seedAchievements(achievementsService: any): Promise for (const achievementData of DEFAULT_ACHIEVEMENTS) { await achievementsService.createAchievement(achievementData); } - console.log(`✅ Seeded ${DEFAULT_ACHIEVEMENTS.length} achievements`); + Logger.log(`✅ Seeded ${DEFAULT_ACHIEVEMENTS.length} achievements`); } catch (error) { - console.error('❌ Error seeding achievements:', error); + Logger.error('❌ Error seeding achievements:', error); } } From c20a4e78958de1ccfdbfd48cb5078cc5950345e1 Mon Sep 17 00:00:00 2001 From: Fuwad Busari <114140933+cythecode@users.noreply.github.com> Date: Sat, 5 Sep 2026 09:40:41 +0100 Subject: [PATCH 5/6] fix(ci): resolve failing checks for #1387 --- src/payments/providers/payment-provider.service.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/payments/providers/payment-provider.service.ts b/src/payments/providers/payment-provider.service.ts index ec21c9dc..c82969db 100644 --- a/src/payments/providers/payment-provider.service.ts +++ b/src/payments/providers/payment-provider.service.ts @@ -18,17 +18,17 @@ export interface CreditResult { * PaymentProviderService * * Thin abstraction over the payment provider (Stripe / PayPal / etc.). - * The concrete implementation should call the real provider SDK; this + * The concrete implementation should call the real provider SDK; this * default implementation throws so that tests must supply a mock and * production deployments must configure a real provider. * - * Issue #1007 — extracted so SubscriptionsService can inject it and + * Issue #1007 ℒ extracted so SubscriptionsService can inject it and * tests can mock it without touching the real provider. */ @Injectable() export class PaymentProviderService { private readonly logger = new Logger(PaymentProviderService.name); - + /** * Charge the customer's default payment method for `amount` (in the * subscription's currency). Returns the provider's charge/payment-intent @@ -38,12 +38,12 @@ export class PaymentProviderService { userId: string, amount: number, currency: string, - metadata: Record = {}, + _metadata: Record = {}, ): Promise { // TODO: replace with real Stripe call: // const intent = await stripe.paymentIntents.create({ amount: Math.round(amount * 100), currency, ... }); this.logger.warn( - `chargeCustomer called for user ${userId} amount=${amount} — no real provider configured`, + `ChargeCustomer called for user ${userId} amount=${amount}  no real provider configured`, ); throw new Error( 'No payment provider configured. Inject a concrete PaymentProviderService implementation.', @@ -58,12 +58,12 @@ export class PaymentProviderService { userId: string, amount: number, currency: string, - metadata: Record = {}, + _metadata: Record = {}, ): Promise { // TODO: replace with real Stripe call: // const credit = await stripe.customers.createBalanceTransaction(customerId, { amount: -Math.round(amount * 100), currency }); this.logger.warn( - `issueCredit called for user ${userId} amount=${amount} — no real provider configured`, + `issueCredit called for user ${userId} amount=${amount}  no real provider configured`, ); throw new Error( 'No payment provider configured. Inject a concrete PaymentProviderService implementation.', From 4505d9ed3f537859873c13ae179c2b1af60c3609 Mon Sep 17 00:00:00 2001 From: Fuwad Busari <114140933+cythecode@users.noreply.github.com> Date: Sat, 5 Sep 2026 09:40:43 +0100 Subject: [PATCH 6/6] fix(ci): resolve failing checks for #1387 --- src/migrations/irreversible-migrations.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/migrations/irreversible-migrations.spec.ts b/src/migrations/irreversible-migrations.spec.ts index 19b3b378..b8cd9dd0 100644 --- a/src/migrations/irreversible-migrations.spec.ts +++ b/src/migrations/irreversible-migrations.spec.ts @@ -73,7 +73,7 @@ describe('Irreversible migrations down() loud warnings (Issue #1207)', () => { const migration = new FixInvoiceNumberSequence1790000000001(); await migration.down(mockQueryRunner); - const loggedWarnings = logSpy.mock.calls.some(([msg]) => + const loggedWarnings = warnSpy.mock.calls.some(([msg]) => typeof msg === 'string' && msg.includes('WARNING: Down migration cannot recover original timestamp'), ); expect(loggedWarnings).toBe(true);