diff --git a/backend/controllers/backoffice/backoffice-controller.ts b/backend/controllers/backoffice/backoffice-controller.ts index 6f4e128e..6a34e67d 100644 --- a/backend/controllers/backoffice/backoffice-controller.ts +++ b/backend/controllers/backoffice/backoffice-controller.ts @@ -1097,6 +1097,7 @@ export class BackofficeController { const previousPublicId = type === 'original' ? pdfInfo?.original_pdf_public_id : pdfInfo?.validated_pdf_public_id + // Type, bytes and size were checked by singleInvoicePdf (backofficeUpload.ts) if (!req.file) { res.status(400).json({ success: false, @@ -1106,27 +1107,6 @@ export class BackofficeController { return } - // Validar tipo de archivo - if (req.file.mimetype !== 'application/pdf') { - res.status(400).json({ - success: false, - error: ERROR_CODES.BACKOFFICE_PDF_ONLY, - code: ERROR_CODES.BACKOFFICE_PDF_ONLY, - }) - return - } - - // Validar tamaño (máx 10MB) - const maxSize = 10 * 1024 * 1024 - if (req.file.size > maxSize) { - res.status(400).json({ - success: false, - error: ERROR_CODES.BACKOFFICE_FILE_TOO_LARGE, - code: ERROR_CODES.BACKOFFICE_FILE_TOO_LARGE, - }) - return - } - logger.debug('[BackofficeController.uploadInvoicePdf] Uploading to Cloudinary...') // Subir a Cloudinary (usar uploadPdf para archivos PDF) @@ -1282,6 +1262,7 @@ export class BackofficeController { return } + // Type, bytes and size were checked by singleAssetImage (backofficeUpload.ts) if (!req.file) { res.status(400).json({ success: false, @@ -1291,17 +1272,6 @@ export class BackofficeController { return } - // Validar tipo de archivo - const allowedTypes = ['image/png', 'image/webp'] - if (!allowedTypes.includes(req.file.mimetype)) { - res.status(400).json({ - success: false, - error: ERROR_CODES.BACKOFFICE_INVALID_IMAGE_TYPE, - code: ERROR_CODES.BACKOFFICE_INVALID_IMAGE_TYPE, - }) - return - } - // Subir a Cloudinary const cloudinaryResult = await CloudinaryService.uploadImage( req.file.buffer, diff --git a/backend/middlewares/backofficeUpload.ts b/backend/middlewares/backofficeUpload.ts new file mode 100644 index 00000000..2e381f6f --- /dev/null +++ b/backend/middlewares/backofficeUpload.ts @@ -0,0 +1,69 @@ +// middlewares/backofficeUpload.ts + +import type { Request, Response, NextFunction, RequestHandler } from 'express' +import multer from 'multer' +import { ERROR_CODES } from '../config/error-codes.js' +import { isPdfBuffer } from '../services/uploads/pdf-signature.js' +import { detectImageType } from '../services/uploads/image-signature.js' + +export const MAX_BACKOFFICE_FILE_BYTES = 10 * 1024 * 1024 + +// Stamps and signatures are laid over the invoice PDF: only formats with +// transparency +const ASSET_IMAGE_TYPES = ['image/png', 'image/webp'] + +/** + * One file in `field`, at most 10 MB: an oversized file answers 413 and one + * whose declared type or bytes `accepts` refuses answers 400 with `wrongType`. + * A request without a file goes on to the controller, which answers it. + */ +function singleFile( + field: string, + accepts: (file: Express.Multer.File) => boolean, + wrongType: string +): RequestHandler { + // Multer stops reading at the limit and rejects a file whose size reaches + // fileSize, hence the +1: exactly 10 MB stays valid + const handler = multer({ + storage: multer.memoryStorage(), + limits: { fileSize: MAX_BACKOFFICE_FILE_BYTES + 1, files: 1 }, + }).single(field) + + return (req: Request, res: Response, next: NextFunction) => { + handler(req, res, (err?: unknown) => { + const fail = (status: number, code: string) => { + res.status(status).json({ success: false, error: code, code }) + } + if (err instanceof multer.MulterError) { + if (err.code === 'LIMIT_FILE_SIZE') fail(413, ERROR_CODES.BACKOFFICE_FILE_TOO_LARGE) + else fail(400, wrongType) + return + } + if (!err && req.file && !accepts(req.file)) { + fail(400, wrongType) + return + } + next(err) + }) + } +} + +/** The invoice PDF: declared as PDF and starting like one. */ +export function singleInvoicePdf(field: string): RequestHandler { + return singleFile( + field, + (file) => file.mimetype === 'application/pdf' && isPdfBuffer(file.buffer), + ERROR_CODES.BACKOFFICE_PDF_ONLY + ) +} + +/** A stamp or signature: a PNG or WebP by its declared type and its bytes. */ +export function singleAssetImage(field: string): RequestHandler { + return singleFile( + field, + (file) => + ASSET_IMAGE_TYPES.includes(file.mimetype) && + ASSET_IMAGE_TYPES.includes(detectImageType(file.buffer) ?? ''), + ERROR_CODES.BACKOFFICE_INVALID_IMAGE_TYPE + ) +} diff --git a/backend/routes/backoffice/backoffice-routes.ts b/backend/routes/backoffice/backoffice-routes.ts index 1f466ce1..e7570278 100644 --- a/backend/routes/backoffice/backoffice-routes.ts +++ b/backend/routes/backoffice/backoffice-routes.ts @@ -9,23 +9,15 @@ */ import { Router } from 'express' -import multer from 'multer' import { BackofficeController } from '../../controllers/backoffice/backoffice-controller.js' import { authenticateToken } from '../../middlewares/authenticateToken.js' import { canAccessBackoffice, isAdmin } from '../../middlewares/roleCheck.js' import { denyDemo } from '../../middlewares/demoRestriction.js' import { requireStorage } from '../../middlewares/requireStorage.js' +import { singleAssetImage, singleInvoicePdf } from '../../middlewares/backofficeUpload.js' const router = Router() -// Configurar multer para uploads en memoria -const upload = multer({ - storage: multer.memoryStorage(), - limits: { - fileSize: 10 * 1024 * 1024, // 10MB para PDFs - }, -}) - // Aplicar autenticación y verificación de acceso a backoffice router.use(authenticateToken) router.use(canAccessBackoffice) @@ -267,7 +259,7 @@ router.post( '/invoices/:id/pdf', isAdmin, requireStorage, - upload.single('pdf'), + singleInvoicePdf('pdf'), BackofficeController.uploadInvoicePdf ) @@ -310,7 +302,7 @@ router.post( '/assets', isAdmin, requireStorage, - upload.single('image'), + singleAssetImage('image'), BackofficeController.createAsset ) diff --git a/backend/services/fnb/pdf-parser.service.ts b/backend/services/fnb/pdf-parser.service.ts index b97044ec..b673abff 100644 --- a/backend/services/fnb/pdf-parser.service.ts +++ b/backend/services/fnb/pdf-parser.service.ts @@ -1,5 +1,6 @@ import pdfParse from 'pdf-parse' import { trackedCodesSet } from './fnb-categories.cache.js' +import { isPdfBuffer } from '../uploads/pdf-signature.js' export interface ParsedPdfData { date: string | null // YYYY-MM-DD @@ -13,16 +14,10 @@ export class FnbPdfError extends Error { status = 422 } -const PDF_MAGIC = Buffer.from('%PDF-') // Opera's F&B report is 3 pages; anything far longer is not that report const MAX_PAGES = 20 export const PARSE_TIMEOUT_MS = 15_000 -// PDF readers accept the header anywhere in the first 1024 bytes -export function isPdfBuffer(buffer: Buffer): boolean { - return buffer.subarray(0, 1024).includes(PDF_MAGIC) -} - // Stops waiting for the parser; pdf.js cannot be aborted, so the page limit is // what bounds the work left running in the background export function withTimeout(promise: Promise, ms: number): Promise { diff --git a/backend/services/uploads/pdf-signature.ts b/backend/services/uploads/pdf-signature.ts new file mode 100644 index 00000000..dccc9273 --- /dev/null +++ b/backend/services/uploads/pdf-signature.ts @@ -0,0 +1,11 @@ +// services/uploads/pdf-signature.ts + +const PDF_MAGIC = Buffer.from('%PDF-') + +/** + * True when the bytes are a PDF. PDF readers accept the header anywhere in the + * first 1024 bytes; the declared mimetype is whatever the client says. + */ +export function isPdfBuffer(buffer: Buffer): boolean { + return buffer.subarray(0, 1024).includes(PDF_MAGIC) +} diff --git a/backend/services/uploads/private-files.ts b/backend/services/uploads/private-files.ts index 32e94b6f..08b21837 100644 --- a/backend/services/uploads/private-files.ts +++ b/backend/services/uploads/private-files.ts @@ -14,9 +14,14 @@ export interface FetchedFile { contentType: string } +// No upload is over 10 MB (backoffice PDFs; images are 5 MB): the margin covers +// the stored format, and anything bigger is not one of our files +export const MAX_STORED_FILE_BYTES = 20 * 1024 * 1024 + /** * Downloads a stored Cloudinary URL. Throws if the URL is not in our cloud - * (SSRF) or Cloudinary does not answer 2xx. No redirects are followed. + * (SSRF), Cloudinary does not answer 2xx or the file is over + * MAX_STORED_FILE_BYTES. No redirects are followed. */ export async function fetchStoredFile(storedUrl: string): Promise { if (!isOwnCloudinaryUrl(storedUrl)) { @@ -26,6 +31,7 @@ export async function fetchStoredFile(storedUrl: string): Promise { responseType: 'arraybuffer', timeout: 30000, maxRedirects: 0, + maxContentLength: MAX_STORED_FILE_BYTES, }) const contentType = String(response.headers['content-type'] || 'application/octet-stream') return { buffer: Buffer.from(response.data), contentType } diff --git a/backend/tests/backoffice/upload-guard.test.ts b/backend/tests/backoffice/upload-guard.test.ts new file mode 100644 index 00000000..9a78a1ce --- /dev/null +++ b/backend/tests/backoffice/upload-guard.test.ts @@ -0,0 +1,151 @@ +// tests/backoffice/upload-guard.test.ts +// Regression tests for the backoffice uploads: the invoice PDF and the stamp or +// signature image were accepted by their declared type alone (any file sent +// as application/pdf went to Cloudinary), and a PDF over 10 MB answered 500 +// because the multer error reached the global handler. +// No database: real multipart requests to a real Express app with multer. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import express from 'express' +import { readFileSync } from 'node:fs' +import { join, dirname } from 'node:path' +import { fileURLToPath } from 'node:url' +import type { Server } from 'node:http' +import type { AddressInfo } from 'node:net' +import { + MAX_BACKOFFICE_FILE_BYTES, + singleAssetImage, + singleInvoicePdf, +} from '../../middlewares/backofficeUpload.js' + +let server: Server +let base: string + +beforeAll(async () => { + const app = express() + // Same fields as routes/backoffice/backoffice-routes.ts + app.post('/pdf', singleInvoicePdf('pdf'), (req, res) => { + res.status(201).json({ size: req.file?.size }) + }) + app.post('/assets', singleAssetImage('image'), (req, res) => { + res.status(201).json({ size: req.file?.size }) + }) + server = app.listen(0) + await new Promise((resolve) => server.once('listening', resolve)) + base = `http://127.0.0.1:${(server.address() as AddressInfo).port}` +}) + +afterAll(() => { + server.close() +}) + +const PDF_START = Buffer.from('%PDF-1.7\n') +const PNG_START = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]) +const WEBP_START = Buffer.from('RIFF\0\0\0\0WEBP') +const JPEG_START = Buffer.from([0xff, 0xd8, 0xff, 0xe0]) + +// A file of `bytes` bytes that starts with `start` +function file(bytes: number, start: Buffer = Buffer.alloc(0)): Uint8Array { + const data = new Uint8Array(bytes) + data.set(start) + return data +} + +async function upload(path: string, field: string, data: Uint8Array, type: string) { + const form = new FormData() + form.append(field, new Blob([data], { type }), 'fichero') + const res = await fetch(`${base}${path}`, { method: 'POST', body: form }) + return { status: res.status, body: await res.json() } +} + +describe('singleInvoicePdf', () => { + it('accepts a PDF up to 10 MB', async () => { + const res = await upload( + '/pdf', + 'pdf', + file(MAX_BACKOFFICE_FILE_BYTES, PDF_START), + 'application/pdf' + ) + expect(res).toEqual({ status: 201, body: { size: MAX_BACKOFFICE_FILE_BYTES } }) + }) + + it('refuses a file declared as PDF whose bytes are not a PDF', async () => { + const res = await upload('/pdf', 'pdf', file(1024, Buffer.from('MZ')), 'application/pdf') + expect(res.status).toBe(400) + expect(res.body.code).toBe('BACKOFFICE_PDF_ONLY') + }) + + it('refuses a real PDF declared with another type', async () => { + const res = await upload('/pdf', 'pdf', file(1024, PDF_START), 'text/html') + expect(res.status).toBe(400) + expect(res.body.code).toBe('BACKOFFICE_PDF_ONLY') + }) + + it('answers 413, not 500, over 10 MB', async () => { + const res = await upload( + '/pdf', + 'pdf', + file(MAX_BACKOFFICE_FILE_BYTES + 1, PDF_START), + 'application/pdf' + ) + expect(res.status).toBe(413) + expect(res.body.code).toBe('BACKOFFICE_FILE_TOO_LARGE') + }) + + it('leaves a request without a file to the controller', async () => { + const res = await fetch(`${base}/pdf`, { method: 'POST', body: new FormData() }) + expect(res.status).toBe(201) + }) +}) + +describe('singleAssetImage', () => { + it.each([ + ['PNG', PNG_START, 'image/png'], + ['WebP', WEBP_START, 'image/webp'], + ])('accepts a %s', async (_name, start, type) => { + expect((await upload('/assets', 'image', file(2048, start), type)).status).toBe(201) + }) + + it('refuses a JPEG renamed as PNG: the stamp needs transparency', async () => { + const res = await upload('/assets', 'image', file(2048, JPEG_START), 'image/png') + expect(res.status).toBe(400) + expect(res.body.code).toBe('BACKOFFICE_INVALID_IMAGE_TYPE') + }) + + it('refuses anything else declared as PNG', async () => { + const res = await upload('/assets', 'image', file(2048, PDF_START), 'image/png') + expect(res.status).toBe(400) + expect(res.body.code).toBe('BACKOFFICE_INVALID_IMAGE_TYPE') + }) + + it('answers 413 over 10 MB', async () => { + const res = await upload( + '/assets', + 'image', + file(MAX_BACKOFFICE_FILE_BYTES + 1, PNG_START), + 'image/png' + ) + expect(res.status).toBe(413) + expect(res.body.code).toBe('BACKOFFICE_FILE_TOO_LARGE') + }) +}) + +describe('routes', () => { + const routes = readFileSync( + join( + dirname(fileURLToPath(import.meta.url)), + '..', + '..', + 'routes', + 'backoffice', + 'backoffice-routes.ts' + ), + 'utf8' + ) + + it('use the guarded uploads, not a bare multer', () => { + expect(routes).toContain("singleInvoicePdf('pdf')") + expect(routes).toContain("singleAssetImage('image')") + expect(routes).not.toContain('multer') + }) +}) diff --git a/backend/tests/fnb/pdf-upload-guard.test.ts b/backend/tests/fnb/pdf-upload-guard.test.ts index 9adeb59f..42c72cf2 100644 --- a/backend/tests/fnb/pdf-upload-guard.test.ts +++ b/backend/tests/fnb/pdf-upload-guard.test.ts @@ -6,12 +6,8 @@ import { describe, it, expect, vi } from 'vitest' import { readFileSync } from 'fs' import { join, dirname } from 'path' import { fileURLToPath } from 'url' -import { - FnbPdfError, - isPdfBuffer, - parseOperaPdf, - withTimeout, -} from '../../services/fnb/pdf-parser.service.js' +import { FnbPdfError, parseOperaPdf, withTimeout } from '../../services/fnb/pdf-parser.service.js' +import { isPdfBuffer } from '../../services/uploads/pdf-signature.js' // The categories cache imports the DB pool; none of these cases reach it vi.mock('../../services/fnb/fnb-categories.cache.js', () => ({ diff --git a/backend/tests/uploads/private-files.test.ts b/backend/tests/uploads/private-files.test.ts index 13e85dfe..7fca2e9a 100644 --- a/backend/tests/uploads/private-files.test.ts +++ b/backend/tests/uploads/private-files.test.ts @@ -3,12 +3,17 @@ // secure_url is signed and works for anyone forever, so no API response may // carry it: responses carry the API path that serves the file. -import { describe, it, expect } from 'vitest' +import { describe, it, expect, vi, afterEach } from 'vitest' +import axios from 'axios' import { readFileSync } from 'node:fs' import { join, dirname } from 'node:path' import { fileURLToPath } from 'node:url' import type { Response } from 'express' -import { fetchStoredFile, sendPrivateFile } from '../../services/uploads/private-files.js' +import { + fetchStoredFile, + MAX_STORED_FILE_BYTES, + sendPrivateFile, +} from '../../services/uploads/private-files.js' import { presentAsset, presentInvoice, @@ -80,9 +85,28 @@ describe('sendPrivateFile', () => { }) describe('fetchStoredFile', () => { + afterEach(() => { + vi.restoreAllMocks() + vi.unstubAllEnvs() + }) + it('refuses a URL outside our Cloudinary cloud before any request', async () => { await expect(fetchStoredFile('https://evil.example/x.pdf')).rejects.toThrow( 'outside our Cloudinary cloud' ) }) + + // The ZIP of validated invoices downloads up to 100 of them into memory + it('follows no redirect and stops at 20 MB', async () => { + vi.stubEnv('CLOUDINARY_CLOUD_NAME', 'fourpoints') + const get = vi + .spyOn(axios, 'get') + .mockResolvedValue({ data: new ArrayBuffer(1), headers: {} }) + await fetchStoredFile(SIGNED) + expect(get).toHaveBeenCalledWith( + SIGNED, + expect.objectContaining({ maxRedirects: 0, maxContentLength: MAX_STORED_FILE_BYTES }) + ) + expect(MAX_STORED_FILE_BYTES).toBe(20 * 1024 * 1024) + }) }) diff --git a/docs/backoffice/README.md b/docs/backoffice/README.md index 5deba492..df486e1b 100644 --- a/docs/backoffice/README.md +++ b/docs/backoffice/README.md @@ -66,6 +66,9 @@ PDFs and images are stored in **Cloudinary**; the database keeps their URLs. - **Deleting an invoice is permanent** and also deletes its history. - **The automatic payment on the 10th does not work today**: it records the change as a system user that does not exist, and the database rejects it. +- **Uploads are checked by their bytes**, not only the type the browser + declares: a PDF for invoices, a PNG or WebP for stamps and signatures, up to + 10 MB (413 above it). - **PDFs are downloaded through the backend**, which fetches them from Cloudinary. The files themselves are uploaded with public delivery, so their Cloudinary URLs also work without logging in.