From 864aa41527272e8a3e71e2de5fad2040acd54730 Mon Sep 17 00:00:00 2001 From: Rowee13 Date: Tue, 18 Aug 2026 17:58:53 +0800 Subject: [PATCH 1/2] feat(api): add email retention sweep and reduce demo cleanup frequency Captured emails were kept forever, so the database grew without bound. Add a daily retention job that deletes emails older than EMAIL_RETENTION_DAYS (default 7). Attachment rows cascade with the email but their files on the mounted volume do not, so the job unlinks those first; a missing file is logged and skipped rather than aborting the sweep. Also drop the demo cleanup cron from every 5 minutes to every 30. The demo TTL is 60 minutes, so 30 is still timely, and the old interval kept a connection busy around the clock for a table that is almost always empty. Move ScheduleModule.forRoot() from DemoModule to AppModule. Every cron in the app depended on it being registered there, so disabling the demo module would have silently stopped all scheduled jobs. Co-Authored-By: Claude Opus 5 (1M context) --- apps/api/.env.example | 3 + apps/api/src/app.module.ts | 2 + apps/api/src/demo/demo-cleanup.service.ts | 2 +- apps/api/src/demo/demo.module.ts | 3 +- apps/api/src/emails/email-retention.config.ts | 12 ++ .../emails/email-retention.service.spec.ts | 109 ++++++++++++++++++ .../api/src/emails/email-retention.service.ts | 53 +++++++++ apps/api/src/emails/emails.module.ts | 4 +- 8 files changed, 184 insertions(+), 4 deletions(-) create mode 100644 apps/api/src/emails/email-retention.config.ts create mode 100644 apps/api/src/emails/email-retention.service.spec.ts create mode 100644 apps/api/src/emails/email-retention.service.ts diff --git a/apps/api/.env.example b/apps/api/.env.example index 60d171a..b12300b 100644 --- a/apps/api/.env.example +++ b/apps/api/.env.example @@ -36,3 +36,6 @@ DEMO_MODE_ENABLED=false DEMO_TTL_MINUTES=60 # Max demo account creations per IP per hour DEMO_RATE_LIMIT_PER_HOUR=2 + +# Days to keep captured emails (0 disables the daily retention sweep) +EMAIL_RETENTION_DAYS=7 diff --git a/apps/api/src/app.module.ts b/apps/api/src/app.module.ts index 430a677..9e6416a 100644 --- a/apps/api/src/app.module.ts +++ b/apps/api/src/app.module.ts @@ -1,6 +1,7 @@ import { Module } from '@nestjs/common'; import { ConfigModule } from '@nestjs/config'; import { APP_GUARD } from '@nestjs/core'; +import { ScheduleModule } from '@nestjs/schedule'; import { AppController } from './app.controller'; import { AppService } from './app.service'; import { AuthModule } from './auth/auth.module'; @@ -17,6 +18,7 @@ import { SmtpModule } from './smtp/smtp.module'; isGlobal: true, envFilePath: '.env', }), + ScheduleModule.forRoot(), PrismaModule, AuthModule, ProjectsModule, diff --git a/apps/api/src/demo/demo-cleanup.service.ts b/apps/api/src/demo/demo-cleanup.service.ts index 219c8a8..197b66f 100644 --- a/apps/api/src/demo/demo-cleanup.service.ts +++ b/apps/api/src/demo/demo-cleanup.service.ts @@ -12,7 +12,7 @@ export class DemoCleanupService { private config: DemoConfig, ) {} - @Cron(CronExpression.EVERY_5_MINUTES) + @Cron(CronExpression.EVERY_30_MINUTES) async handleCleanup() { if (!this.config.enabled) return; const cutoff = new Date(Date.now() - this.config.ttlMinutes * 60_000); diff --git a/apps/api/src/demo/demo.module.ts b/apps/api/src/demo/demo.module.ts index 37ab5a6..c8fe79a 100644 --- a/apps/api/src/demo/demo.module.ts +++ b/apps/api/src/demo/demo.module.ts @@ -1,6 +1,5 @@ import { Module } from '@nestjs/common'; import { APP_GUARD } from '@nestjs/core'; -import { ScheduleModule } from '@nestjs/schedule'; import { PrismaModule } from '../prisma/prisma.module'; import { AuthModule } from '../auth/auth.module'; import { DemoCleanupService } from './demo-cleanup.service'; @@ -14,7 +13,7 @@ import { BlockDemoGuard } from './guards/block-demo.guard'; import { DevInboxDemoSeeder } from './seeders/devinbox-demo.seeder'; @Module({ - imports: [ScheduleModule.forRoot(), PrismaModule, AuthModule], + imports: [PrismaModule, AuthModule], providers: [ DemoConfig, DemoService, diff --git a/apps/api/src/emails/email-retention.config.ts b/apps/api/src/emails/email-retention.config.ts new file mode 100644 index 0000000..037eac7 --- /dev/null +++ b/apps/api/src/emails/email-retention.config.ts @@ -0,0 +1,12 @@ +import { Injectable } from '@nestjs/common'; +import { ConfigService } from '@nestjs/config'; + +@Injectable() +export class EmailRetentionConfig { + constructor(private config: ConfigService) {} + + // Days to keep captured emails. 0 (or less) disables retention sweeps. + get retentionDays(): number { + return parseInt(this.config.get('EMAIL_RETENTION_DAYS') || '7', 10); + } +} diff --git a/apps/api/src/emails/email-retention.service.spec.ts b/apps/api/src/emails/email-retention.service.spec.ts new file mode 100644 index 0000000..10463d9 --- /dev/null +++ b/apps/api/src/emails/email-retention.service.spec.ts @@ -0,0 +1,109 @@ +import { Logger } from '@nestjs/common'; +import * as fs from 'fs'; +import { PrismaService } from '../prisma/prisma.service'; +import { EmailRetentionConfig } from './email-retention.config'; +import { EmailRetentionService } from './email-retention.service'; + +describe('EmailRetentionService', () => { + let service: EmailRetentionService; + let prisma: { + email: { findMany: jest.Mock; deleteMany: jest.Mock }; + }; + let config: { retentionDays: number }; + let logSpy: jest.SpyInstance; + let warnSpy: jest.SpyInstance; + let unlinkSpy: jest.SpyInstance; + + beforeEach(() => { + prisma = { email: { findMany: jest.fn(), deleteMany: jest.fn() } }; + config = { retentionDays: 7 }; + service = new EmailRetentionService( + prisma as unknown as PrismaService, + config as unknown as EmailRetentionConfig, + ); + logSpy = jest + .spyOn(Logger.prototype, 'log') + .mockImplementation(() => undefined); + warnSpy = jest + .spyOn(Logger.prototype, 'warn') + .mockImplementation(() => undefined); + unlinkSpy = jest + .spyOn(fs.promises, 'unlink') + .mockResolvedValue(undefined as never); + }); + + afterEach(() => { + jest.useRealTimers(); + jest.restoreAllMocks(); + }); + + it('is a no-op when retention is disabled', async () => { + config.retentionDays = 0; + + await service.handleRetention(); + + expect(prisma.email.findMany).not.toHaveBeenCalled(); + expect(prisma.email.deleteMany).not.toHaveBeenCalled(); + }); + + it('deletes emails older than the retention cutoff', async () => { + const fixedNow = new Date('2026-08-18T12:00:00.000Z'); + jest.useFakeTimers().setSystemTime(fixedNow); + prisma.email.findMany.mockResolvedValue([ + { id: 'e1', attachments: [] }, + { id: 'e2', attachments: [] }, + ]); + prisma.email.deleteMany.mockResolvedValue({ count: 2 }); + + await service.handleRetention(); + + const expectedCutoff = new Date(fixedNow.getTime() - 7 * 86_400_000); + expect(prisma.email.findMany).toHaveBeenCalledWith({ + where: { receivedAt: { lt: expectedCutoff } }, + select: { id: true, attachments: { select: { storagePath: true } } }, + }); + expect(prisma.email.deleteMany).toHaveBeenCalledWith({ + where: { id: { in: ['e1', 'e2'] } }, + }); + expect(logSpy).toHaveBeenCalledWith( + 'Deleted 2 email(s) older than 7 day(s)', + ); + }); + + it('unlinks attachment files before deleting the rows', async () => { + prisma.email.findMany.mockResolvedValue([ + { id: 'e1', attachments: [{ storagePath: '/data/a.pdf' }] }, + ]); + prisma.email.deleteMany.mockResolvedValue({ count: 1 }); + + await service.handleRetention(); + + expect(unlinkSpy).toHaveBeenCalledWith('/data/a.pdf'); + }); + + it('still deletes rows when an attachment file is already gone', async () => { + unlinkSpy.mockRejectedValue(new Error('ENOENT')); + prisma.email.findMany.mockResolvedValue([ + { id: 'e1', attachments: [{ storagePath: '/data/missing.pdf' }] }, + ]); + prisma.email.deleteMany.mockResolvedValue({ count: 1 }); + + await service.handleRetention(); + + expect(warnSpy).toHaveBeenCalledWith( + 'Failed to delete attachment file: /data/missing.pdf', + ); + expect(prisma.email.deleteMany).toHaveBeenCalledWith({ + where: { id: { in: ['e1'] } }, + }); + }); + + it('does nothing further when no emails have expired', async () => { + prisma.email.findMany.mockResolvedValue([]); + + await service.handleRetention(); + + expect(prisma.email.deleteMany).not.toHaveBeenCalled(); + expect(logSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/emails/email-retention.service.ts b/apps/api/src/emails/email-retention.service.ts new file mode 100644 index 0000000..53d0242 --- /dev/null +++ b/apps/api/src/emails/email-retention.service.ts @@ -0,0 +1,53 @@ +import { Injectable, Logger } from '@nestjs/common'; +import { Cron, CronExpression } from '@nestjs/schedule'; +import * as fs from 'fs'; +import { PrismaService } from '../prisma/prisma.service'; +import { EmailRetentionConfig } from './email-retention.config'; + +const DAY_MS = 86_400_000; + +@Injectable() +export class EmailRetentionService { + private readonly logger = new Logger(EmailRetentionService.name); + + constructor( + private prisma: PrismaService, + private config: EmailRetentionConfig, + ) {} + + @Cron(CronExpression.EVERY_DAY_AT_3AM) + async handleRetention() { + const days = this.config.retentionDays; + if (!days || days <= 0) return; + + const cutoff = new Date(Date.now() - days * DAY_MS); + const expired = await this.prisma.email.findMany({ + where: { receivedAt: { lt: cutoff } }, + select: { id: true, attachments: { select: { storagePath: true } } }, + }); + + if (expired.length === 0) return; + + // Attachment rows cascade with the email, but their files on the mounted + // volume do not - remove them first so the volume does not leak. + for (const email of expired) { + for (const attachment of email.attachments) { + try { + await fs.promises.unlink(attachment.storagePath); + } catch { + this.logger.warn( + `Failed to delete attachment file: ${attachment.storagePath}`, + ); + } + } + } + + const result = await this.prisma.email.deleteMany({ + where: { id: { in: expired.map((email) => email.id) } }, + }); + + this.logger.log( + `Deleted ${result.count} email(s) older than ${days} day(s)`, + ); + } +} diff --git a/apps/api/src/emails/emails.module.ts b/apps/api/src/emails/emails.module.ts index 0e00894..c98d5e1 100644 --- a/apps/api/src/emails/emails.module.ts +++ b/apps/api/src/emails/emails.module.ts @@ -1,10 +1,12 @@ import { Module } from '@nestjs/common'; +import { EmailRetentionConfig } from './email-retention.config'; +import { EmailRetentionService } from './email-retention.service'; import { EmailsController } from './emails.controller'; import { EmailsService } from './emails.service'; @Module({ controllers: [EmailsController], - providers: [EmailsService], + providers: [EmailsService, EmailRetentionConfig, EmailRetentionService], exports: [EmailsService], }) export class EmailsModule {} From 3f5942f8f0909de1e7f084e1985cec557eea5320 Mon Sep 17 00:00:00 2001 From: Rowee13 Date: Tue, 18 Aug 2026 17:59:07 +0800 Subject: [PATCH 2/2] fix(api): harden handling of untrusted attachment metadata Attachment filenames and content types come from the MIME headers of inbound mail, which anyone can send to the public SMTP listener, so they must be treated as untrusted input. Filenames were interpolated straight into the storage path. The timestamp prefix only absorbed the first path segment, so a crafted name could still resolve outside the uploads directory. safeAttachmentPath() now keeps only the basename, rejects null bytes, and verifies the result stays inside uploadsDir before anything is written. The download endpoint reflected the same values into response headers. The content type is now constrained to a well-formed, non-renderable mime type (falling back to application/octet-stream) and the filename is stripped of quotes, backslashes and control characters so it cannot escape the Content-Disposition parameter. Added X-Content-Type-Options: nosniff. Co-Authored-By: Claude Opus 5 (1M context) --- .../api/src/emails/attachment-headers.spec.ts | 56 +++++++++++++++++ apps/api/src/emails/attachment-headers.ts | 44 +++++++++++++ apps/api/src/emails/attachment-path.spec.ts | 63 +++++++++++++++++++ apps/api/src/emails/attachment-path.ts | 32 ++++++++++ apps/api/src/emails/emails.controller.ts | 9 ++- apps/api/src/emails/emails.service.ts | 14 ++++- 6 files changed, 214 insertions(+), 4 deletions(-) create mode 100644 apps/api/src/emails/attachment-headers.spec.ts create mode 100644 apps/api/src/emails/attachment-headers.ts create mode 100644 apps/api/src/emails/attachment-path.spec.ts create mode 100644 apps/api/src/emails/attachment-path.ts diff --git a/apps/api/src/emails/attachment-headers.spec.ts b/apps/api/src/emails/attachment-headers.spec.ts new file mode 100644 index 0000000..57a5ed2 --- /dev/null +++ b/apps/api/src/emails/attachment-headers.spec.ts @@ -0,0 +1,56 @@ +import { + contentDispositionFilename, + safeContentType, +} from './attachment-headers'; + +describe('safeContentType', () => { + it('passes through a well-formed mime type', () => { + expect(safeContentType('image/png')).toBe('image/png'); + expect(safeContentType('application/vnd.ms-excel')).toBe( + 'application/vnd.ms-excel', + ); + }); + + it('falls back to octet-stream for renderable or malformed types', () => { + expect(safeContentType('text/html')).toBe('application/octet-stream'); + expect(safeContentType('image/svg+xml')).toBe('application/octet-stream'); + expect(safeContentType('application/xhtml+xml')).toBe( + 'application/octet-stream', + ); + expect(safeContentType(undefined)).toBe('application/octet-stream'); + expect(safeContentType('not a mime type')).toBe('application/octet-stream'); + expect(safeContentType('text/html\r\nX-Injected: 1')).toBe( + 'application/octet-stream', + ); + }); +}); + +describe('contentDispositionFilename', () => { + it('quotes an ordinary filename', () => { + expect(contentDispositionFilename('report.pdf')).toBe( + 'attachment; filename="report.pdf"', + ); + }); + + it('does not let a quote escape the filename parameter', () => { + const header = contentDispositionFilename('a".txt'); + + expect(header).toBe('attachment; filename="a.txt"'); + expect(header.match(/"/g)).toHaveLength(2); + }); + + it('strips CR/LF so headers cannot be split', () => { + const header = contentDispositionFilename('a\r\nX-Injected: 1'); + + expect(header).not.toContain('\r'); + expect(header).not.toContain('\n'); + }); + + it('always emits the attachment disposition', () => { + for (const name of ['inline; x', '', undefined, '../../etc/passwd']) { + expect(contentDispositionFilename(name).startsWith('attachment;')).toBe( + true, + ); + } + }); +}); diff --git a/apps/api/src/emails/attachment-headers.ts b/apps/api/src/emails/attachment-headers.ts new file mode 100644 index 0000000..2da1e1f --- /dev/null +++ b/apps/api/src/emails/attachment-headers.ts @@ -0,0 +1,44 @@ +// Attachment metadata comes from inbound mail and is fully attacker +// controlled, so it is sanitised before being reflected in response headers. + +const MIME_PATTERN = + /^[A-Za-z0-9][A-Za-z0-9!#$&^_.+-]{0,126}\/[A-Za-z0-9][A-Za-z0-9!#$&^_.+-]{0,126}$/; + +// Types a browser may render in-origin rather than treat as an opaque download. +const RENDERABLE = new Set([ + 'text/html', + 'application/xhtml+xml', + 'image/svg+xml', + 'application/xml', + 'text/xml', +]); + +export function safeContentType(contentType: string | undefined): string { + const fallback = 'application/octet-stream'; + if (!contentType) return fallback; + + const value = contentType.trim().toLowerCase(); + if (!MIME_PATTERN.test(value)) return fallback; + if (RENDERABLE.has(value)) return fallback; + + return value; +} + +export function contentDispositionFilename( + filename: string | undefined, +): string { + // Drop quotes, backslashes and control characters: the first two would end + // the quoted-string early, the last could split the header. + const QUOTE = 0x22; + const BACKSLASH = 0x5c; + const cleaned = Array.from(filename ?? '') + .filter((ch) => { + const code = ch.codePointAt(0) ?? 0; + if (code < 0x20 || code === 0x7f) return false; + return code !== QUOTE && code !== BACKSLASH; + }) + .join('') + .trim(); + + return `attachment; filename="${cleaned || 'unnamed'}"`; +} diff --git a/apps/api/src/emails/attachment-path.spec.ts b/apps/api/src/emails/attachment-path.spec.ts new file mode 100644 index 0000000..2956d5b --- /dev/null +++ b/apps/api/src/emails/attachment-path.spec.ts @@ -0,0 +1,63 @@ +import * as path from 'path'; +import { safeAttachmentPath } from './attachment-path'; + +describe('safeAttachmentPath', () => { + const uploadsDir = path.join('/app', 'uploads', 'attachments'); + const now = 1787046668457; + + it('keeps an ordinary filename inside the uploads directory', () => { + const result = safeAttachmentPath(uploadsDir, 'report.pdf', now); + + expect(result).toBe(path.join(uploadsDir, `${now}-report.pdf`)); + }); + + it.each([ + ['../../../../etc/cron.d/pwn', 'pwn'], + ['../../../../../../root/.ssh/authorized_keys', 'authorized_keys'], + ['../escape.txt', 'escape.txt'], + ['/etc/passwd', 'passwd'], + ['nested/dir/file.txt', 'file.txt'], + ])('strips traversal from %j', (input, expected) => { + const result = safeAttachmentPath(uploadsDir, input, now); + + expect(result).toBe(path.join(uploadsDir, `${now}-${expected}`)); + expect(path.dirname(result as string)).toBe(uploadsDir); + }); + + it('falls back to a placeholder when there is no usable filename', () => { + expect(safeAttachmentPath(uploadsDir, undefined, now)).toBe( + path.join(uploadsDir, `${now}-unnamed`), + ); + expect(safeAttachmentPath(uploadsDir, '', now)).toBe( + path.join(uploadsDir, `${now}-unnamed`), + ); + expect(safeAttachmentPath(uploadsDir, '..', now)).toBe( + path.join(uploadsDir, `${now}-unnamed`), + ); + expect(safeAttachmentPath(uploadsDir, '.', now)).toBe( + path.join(uploadsDir, `${now}-unnamed`), + ); + }); + + it('rejects filenames containing a null byte', () => { + expect(safeAttachmentPath(uploadsDir, 'evil\u0000.png', now)).toBeNull(); + }); + + it('always resolves within the uploads directory', () => { + const hostile = [ + '../../../../etc/passwd', + '....//....//etc/passwd', + '..', + '../', + 'a/../../../b', + ]; + + for (const name of hostile) { + const result = safeAttachmentPath(uploadsDir, name, now); + if (result === null) continue; + expect( + path.resolve(result).startsWith(path.resolve(uploadsDir) + path.sep), + ).toBe(true); + } + }); +}); diff --git a/apps/api/src/emails/attachment-path.ts b/apps/api/src/emails/attachment-path.ts new file mode 100644 index 0000000..78a017a --- /dev/null +++ b/apps/api/src/emails/attachment-path.ts @@ -0,0 +1,32 @@ +import * as path from 'path'; + +/** + * Build the on-disk path for an inbound attachment. + * + * Attachment filenames come from the MIME headers of mail that anyone can send + * to the public SMTP listener, so they are untrusted. Only the basename is + * kept, and the result is checked to be inside `uploadsDir` before it is used. + * + * Returns null when the name cannot be made safe, in which case the attachment + * should be skipped rather than written. + */ +export function safeAttachmentPath( + uploadsDir: string, + filename: string | undefined, + now: number = Date.now(), +): string | null { + // A null byte can truncate the path inside libc, so refuse it outright. + if (filename?.includes('\u0000')) return null; + + const base = path.basename(filename ?? ''); + const safeName = + base === '' || base === '.' || base === '..' ? 'unnamed' : base; + + const filepath = path.join(uploadsDir, `${now}-${safeName}`); + + // Defence in depth: the basename above should make this unreachable. + const root = path.resolve(uploadsDir) + path.sep; + if (!path.resolve(filepath).startsWith(root)) return null; + + return filepath; +} diff --git a/apps/api/src/emails/emails.controller.ts b/apps/api/src/emails/emails.controller.ts index 31a2b3b..d526473 100644 --- a/apps/api/src/emails/emails.controller.ts +++ b/apps/api/src/emails/emails.controller.ts @@ -19,6 +19,10 @@ import { ApiBearerAuth, } from '@nestjs/swagger'; import { EmailsService } from './emails.service'; +import { + contentDispositionFilename, + safeContentType, +} from './attachment-headers'; import { Request, Response } from 'express'; import * as fs from 'fs'; @@ -155,8 +159,9 @@ export class EmailsController { const file = fs.createReadStream(attachment.storagePath); res.set({ - 'Content-Type': attachment.contentType, - 'Content-Disposition': `attachment; filename="${attachment.filename}"`, + 'Content-Type': safeContentType(attachment.contentType), + 'Content-Disposition': contentDispositionFilename(attachment.filename), + 'X-Content-Type-Options': 'nosniff', }); return new StreamableFile(file); diff --git a/apps/api/src/emails/emails.service.ts b/apps/api/src/emails/emails.service.ts index b7fd076..6d44a84 100644 --- a/apps/api/src/emails/emails.service.ts +++ b/apps/api/src/emails/emails.service.ts @@ -5,6 +5,7 @@ import { ConfigService } from '@nestjs/config'; import * as fs from 'fs'; import * as path from 'path'; import { promisify } from 'util'; +import { safeAttachmentPath } from './attachment-path'; const writeFile = promisify(fs.writeFile); const mkdir = promisify(fs.mkdir); @@ -100,8 +101,17 @@ export class EmailsService { for (const attachment of parsed.attachments) { try { - const filename = `${Date.now()}-${attachment.filename}`; - const filepath = path.join(this.uploadsDir, filename); + const filepath = safeAttachmentPath( + this.uploadsDir, + attachment.filename, + ); + + if (!filepath) { + this.logger.warn( + `Skipped attachment with unsafe filename: ${attachment.filename}`, + ); + continue; + } await writeFile(filepath, attachment.content);