diff --git a/backend/__tests__/utils/safePathUploadedAssets.test.js b/backend/__tests__/utils/safePathUploadedAssets.test.js new file mode 100644 index 00000000..07ae5d6a --- /dev/null +++ b/backend/__tests__/utils/safePathUploadedAssets.test.js @@ -0,0 +1,59 @@ +/** + * Containment for the two admin-writable "delete the old file" paths. + * + * Settings → Branding persists logo_url / favicon_url verbatim and, on + * clear, unlinked `path.join(storage, url)` after a mere prefix check. + * Business profile did the same for logo_path behind a `/pdf-logo-\d+\./` + * marker. Both let an admin delete any file the process can reach. The + * helpers below only ever name a flat leaf inside the fixed directory. + */ +const path = require('path'); +const { uploadedAssetPath, uploadedPdfLogoPath } = require('../../src/utils/safePath'); + +const root = '/srv/picpeak/storage'; + +describe('uploadedAssetPath', () => { + it('resolves a flat leaf inside the named upload directory', () => { + expect(uploadedAssetPath('/uploads/logos/logo-1.png', 'logos', root)) + .toBe(path.join(root, 'uploads', 'logos', 'logo-1.png')); + expect(uploadedAssetPath('/uploads/favicons/fav.ico', 'favicons', root)) + .toBe(path.join(root, 'uploads', 'favicons', 'fav.ico')); + }); + + it.each([ + '/uploads/logos/../../../data/picpeak.db', + '/uploads/logos/..', + '/uploads/logos/', + '/uploads/logos/sub/dir.png', + '/uploads/favicons/x.ico', // wrong kind + 'uploads/logos/logo.png', // not /-rooted + 'https://example.com/uploads/logos/logo.png', + '', + null, + 42, + ])('refuses %p', (value) => { + expect(uploadedAssetPath(value, 'logos', root)).toBeNull(); + }); +}); + +describe('uploadedPdfLogoPath', () => { + it('resolves the file the upload route writes', () => { + expect(uploadedPdfLogoPath('/uploads/logos/pdf-logo-1700000000000.png', root)) + .toBe(path.join(root, 'uploads', 'logos', 'pdf-logo-1700000000000.png')); + expect(uploadedPdfLogoPath('uploads/logos/pdf-logo-1.svg', root)) + .toBe(path.join(root, 'uploads', 'logos', 'pdf-logo-1.svg')); + }); + + it.each([ + 'pdf-logo-1./../../../../etc/target', + '/uploads/logos/pdf-logo-1./../../secret', + '/etc/pdf-logo-1.x', + '/uploads/logos/pdf-logo-1.png/../other', + '/uploads/logos/other-logo.png', + '/uploads/contracts/signed/pdf-logo-1.pdf', + '', + null, + ])('refuses %p', (value) => { + expect(uploadedPdfLogoPath(value, root)).toBeNull(); + }); +}); diff --git a/backend/server.js b/backend/server.js index 84e45283..efeb35f2 100644 --- a/backend/server.js +++ b/backend/server.js @@ -791,10 +791,15 @@ app.get( // whereas Firefox/Chrome do — so a 302 worked everywhere except // Safari. sendFile sets the right content-type from the extension. const rel = String(url).replace(/^\/+/, '').replace(/^uploads\//, ''); + // Containment is the two public asset trees, not the whole uploads/ + // root: that root also holds signed contracts and client transfer + // files, and the favicon URL is an admin-writable setting, so the + // wider check let `/uploads/contracts/signed/` be served here + // unauthenticated with a day of cache. const uploadsRoot = path.resolve(path.join(storagePath, 'uploads')); const resolved = path.resolve(path.join(uploadsRoot, rel)); - // Path containment — never serve outside the uploads dir. - if (resolved.startsWith(uploadsRoot + path.sep) && fs.existsSync(resolved)) { + const servableRoots = ['favicons', 'logos'].map((d) => path.join(uploadsRoot, d) + path.sep); + if (servableRoots.some((root) => resolved.startsWith(root)) && fs.existsSync(resolved)) { // This route streams the file directly, bypassing the secureStatic // middleware — so re-apply its SVG hardening here. An admin-uploaded // SVG favicon could contain