From 801ffbfb7b4686f6750fa8bbeb4b23528f4aff4c Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 15 Apr 2026 05:16:47 +0200 Subject: [PATCH] fix(kml-import): address PR #488 review issues - Strip BOM (U+FEFF) from 14 translation files injected by editor - Guard KMZ unpack against zip-bomb: check entry.uncompressedSize against 50 MB cap (KMZ_DECOMPRESSED_SIZE_LIMIT) before calling .buffer(); limit is an exported constant so tests can override it - Fix non-BMP HTML entity decoding: replace String.fromCharCode with String.fromCodePoint + 0x10FFFF bounds check so emoji like 😀 round-trip correctly - Switch KML namespace stripping from regex to fast-xml-parser's removeNSPrefix option; XMLValidator accepts namespaced XML natively, making the pre-strip step unnecessary - Remove dead skippedCount overwrite after transaction; per-loop increment already tracks it alongside per-item error messages - Type multer req.file as Express.Multer.File on both /import/gpx and /import/map routes instead of (req as any).file - Add unit tests: emoji entity decoding (decimal + hex), KMZ zip-bomb rejection, KMZ-with-no-KML rejection --- client/src/i18n/translations/ar.ts | 2 +- client/src/i18n/translations/br.ts | 2 +- client/src/i18n/translations/cs.ts | 2 +- client/src/i18n/translations/de.ts | 2 +- client/src/i18n/translations/en.ts | 2 +- client/src/i18n/translations/es.ts | 2 +- client/src/i18n/translations/fr.ts | 2 +- client/src/i18n/translations/hu.ts | 2 +- client/src/i18n/translations/it.ts | 2 +- client/src/i18n/translations/nl.ts | 2 +- client/src/i18n/translations/pl.ts | 2 +- client/src/i18n/translations/ru.ts | 2 +- client/src/i18n/translations/zh.ts | 2 +- client/src/i18n/translations/zhTw.ts | 2 +- server/src/routes/places.ts | 4 +- server/src/services/kmlImport.ts | 12 +--- server/src/services/placeService.ts | 20 ++++-- .../unit/services/kmlImportUtils.test.ts | 19 +++--- server/tests/unit/services/kmzUnpack.test.ts | 61 +++++++++++++++++++ 19 files changed, 103 insertions(+), 41 deletions(-) create mode 100644 server/tests/unit/services/kmzUnpack.test.ts diff --git a/client/src/i18n/translations/ar.ts b/client/src/i18n/translations/ar.ts index 42dfb7dd..b766e2b5 100644 --- a/client/src/i18n/translations/ar.ts +++ b/client/src/i18n/translations/ar.ts @@ -1,4 +1,4 @@ -import en from './en' +import en from './en' const ar: Record = { ...en, diff --git a/client/src/i18n/translations/br.ts b/client/src/i18n/translations/br.ts index b9eae0ce..0dbb37e9 100644 --- a/client/src/i18n/translations/br.ts +++ b/client/src/i18n/translations/br.ts @@ -1,4 +1,4 @@ -const br: Record = { +const br: Record = { // Common 'common.save': 'Salvar', 'common.showMore': 'Mostrar mais', diff --git a/client/src/i18n/translations/cs.ts b/client/src/i18n/translations/cs.ts index bafaaa54..e6caf7bd 100644 --- a/client/src/i18n/translations/cs.ts +++ b/client/src/i18n/translations/cs.ts @@ -1,4 +1,4 @@ -const cs: Record = { +const cs: Record = { // Společné (Common) 'common.save': 'Uložit', 'common.showMore': 'Zobrazit více', diff --git a/client/src/i18n/translations/de.ts b/client/src/i18n/translations/de.ts index 40a36bc3..aada158a 100644 --- a/client/src/i18n/translations/de.ts +++ b/client/src/i18n/translations/de.ts @@ -1,4 +1,4 @@ -const de: Record = { +const de: Record = { // Allgemein 'common.save': 'Speichern', 'common.showMore': 'Mehr anzeigen', diff --git a/client/src/i18n/translations/en.ts b/client/src/i18n/translations/en.ts index 2d2ddbef..d9f3ce5a 100644 --- a/client/src/i18n/translations/en.ts +++ b/client/src/i18n/translations/en.ts @@ -1,4 +1,4 @@ -const en: Record = { +const en: Record = { // Common 'common.save': 'Save', 'common.showMore': 'Show more', diff --git a/client/src/i18n/translations/es.ts b/client/src/i18n/translations/es.ts index da9d07a3..971093fd 100644 --- a/client/src/i18n/translations/es.ts +++ b/client/src/i18n/translations/es.ts @@ -1,4 +1,4 @@ -const es: Record = { +const es: Record = { // Common 'common.save': 'Guardar', 'common.showMore': 'Ver más', diff --git a/client/src/i18n/translations/fr.ts b/client/src/i18n/translations/fr.ts index d6f89ea9..5e5d2aa6 100644 --- a/client/src/i18n/translations/fr.ts +++ b/client/src/i18n/translations/fr.ts @@ -1,4 +1,4 @@ -const fr: Record = { +const fr: Record = { // Common 'common.save': 'Enregistrer', 'common.showMore': 'Voir plus', diff --git a/client/src/i18n/translations/hu.ts b/client/src/i18n/translations/hu.ts index 91391f26..539509a7 100644 --- a/client/src/i18n/translations/hu.ts +++ b/client/src/i18n/translations/hu.ts @@ -1,4 +1,4 @@ -const hu: Record = { +const hu: Record = { // Általános 'common.save': 'Mentés', 'common.showMore': 'Továbbiak', diff --git a/client/src/i18n/translations/it.ts b/client/src/i18n/translations/it.ts index 5ad06408..88b5576c 100644 --- a/client/src/i18n/translations/it.ts +++ b/client/src/i18n/translations/it.ts @@ -1,4 +1,4 @@ -const it: Record = { +const it: Record = { // Common 'common.save': 'Salva', 'common.showMore': 'Mostra di più', diff --git a/client/src/i18n/translations/nl.ts b/client/src/i18n/translations/nl.ts index 6fb8b852..1e372ab1 100644 --- a/client/src/i18n/translations/nl.ts +++ b/client/src/i18n/translations/nl.ts @@ -1,4 +1,4 @@ -const nl: Record = { +const nl: Record = { // Common 'common.save': 'Opslaan', 'common.showMore': 'Meer tonen', diff --git a/client/src/i18n/translations/pl.ts b/client/src/i18n/translations/pl.ts index d1e6bc56..4b81e680 100644 --- a/client/src/i18n/translations/pl.ts +++ b/client/src/i18n/translations/pl.ts @@ -1,4 +1,4 @@ -const pl: Record = { +const pl: Record = { // Common 'common.save': 'Zapisz', 'common.showMore': 'Pokaż więcej', diff --git a/client/src/i18n/translations/ru.ts b/client/src/i18n/translations/ru.ts index 9659d2f6..2db4b31e 100644 --- a/client/src/i18n/translations/ru.ts +++ b/client/src/i18n/translations/ru.ts @@ -1,4 +1,4 @@ -const ru: Record = { +const ru: Record = { // Common 'common.save': 'Сохранить', 'common.showMore': 'Показать больше', diff --git a/client/src/i18n/translations/zh.ts b/client/src/i18n/translations/zh.ts index e514daa3..2b6d782f 100644 --- a/client/src/i18n/translations/zh.ts +++ b/client/src/i18n/translations/zh.ts @@ -1,4 +1,4 @@ -const zh: Record = { +const zh: Record = { // Common 'common.save': '保存', 'common.showMore': '显示更多', diff --git a/client/src/i18n/translations/zhTw.ts b/client/src/i18n/translations/zhTw.ts index 27fff651..e6b28a85 100644 --- a/client/src/i18n/translations/zhTw.ts +++ b/client/src/i18n/translations/zhTw.ts @@ -1,4 +1,4 @@ -const zhTw: Record = { +const zhTw: Record = { // Common 'common.save': '儲存', 'common.showMore': '顯示更多', diff --git a/server/src/routes/places.ts b/server/src/routes/places.ts index 4eb726aa..48d9f4e4 100644 --- a/server/src/routes/places.ts +++ b/server/src/routes/places.ts @@ -63,7 +63,7 @@ router.post('/import/gpx', authenticate, requireTripAccess, uploadMulter.single( return res.status(403).json({ error: 'No permission' }); const { tripId } = req.params; - const file = (req as any).file; + const file = req.file as Express.Multer.File | undefined; if (!file) return res.status(400).json({ error: 'No file uploaded' }); const created = importGpx(tripId, file.buffer); @@ -84,7 +84,7 @@ router.post('/import/map', authenticate, requireTripAccess, uploadMulter.single( } const { tripId } = req.params; - const file = (req as any).file; + const file = req.file as Express.Multer.File | undefined; if (!file) return res.status(400).json({ error: 'No file uploaded' }); try { diff --git a/server/src/services/kmlImport.ts b/server/src/services/kmlImport.ts index 4489d54b..9c7f2790 100644 --- a/server/src/services/kmlImport.ts +++ b/server/src/services/kmlImport.ts @@ -50,22 +50,14 @@ function decodeHtmlEntities(value: string): string { return withNamedEntities .replace(/&#(\d+);/g, (_, dec) => { const code = Number(dec); - return Number.isFinite(code) ? String.fromCharCode(code) : _; + return Number.isFinite(code) && code >= 0 && code <= 0x10ffff ? String.fromCodePoint(code) : _; }) .replace(/&#x([0-9a-fA-F]+);/g, (_, hex) => { const code = Number.parseInt(hex, 16); - return Number.isFinite(code) ? String.fromCharCode(code) : _; + return Number.isFinite(code) && code >= 0 && code <= 0x10ffff ? String.fromCodePoint(code) : _; }); } -export function stripXmlNamespaces(xml: string): string { - // KML exports vary heavily; stripping namespace declarations/prefixes makes parsing resilient. - return xml - .replace(/\sxmlns(:\w+)?="[^"]*"/g, '') - .replace(/\sxmlns(:\w+)?='[^']*'/g, '') - .replace(/<(\/?)\w+:/g, '<$1'); -} - export function decodeUtf8WithWarning(fileBuffer: Buffer): { text: string; warning: string | null } { try { return { text: UTF8_DECODER_FATAL.decode(fileBuffer), warning: null }; diff --git a/server/src/services/placeService.ts b/server/src/services/placeService.ts index 434232a1..35e7e84c 100644 --- a/server/src/services/placeService.ts +++ b/server/src/services/placeService.ts @@ -11,7 +11,6 @@ import { extractKmlPlacemarkNodes, parsePlacemarkNode, resolveCategoryIdForFolder, - stripXmlNamespaces, type KmlImportSummary, } from './kmlImport'; @@ -254,9 +253,12 @@ const gpxParser = new XMLParser({ const kmlParser = new XMLParser({ ignoreAttributes: false, attributeNamePrefix: '@_', + removeNSPrefix: true, isArray: (name) => ['Placemark', 'Folder', 'Document'].includes(name), }); +export const KMZ_DECOMPRESSED_SIZE_LIMIT = 50 * 1024 * 1024; // 50 MB + export function importGpx(tripId: string, fileBuffer: Buffer) { const parsed = gpxParser.parse(fileBuffer.toString('utf-8')); const gpx = parsed?.gpx; @@ -327,14 +329,13 @@ export function importGpx(tripId: string, fileBuffer: Buffer) { export function importKmlPlaces(tripId: string, fileBuffer: Buffer): PlaceImportResult { const decoded = decodeUtf8WithWarning(fileBuffer); - const xmlWithoutNamespaces = stripXmlNamespaces(decoded.text); - const validationResult = XMLValidator.validate(xmlWithoutNamespaces); + const validationResult = XMLValidator.validate(decoded.text); if (validationResult !== true) { throw new Error('Malformed KML: invalid XML structure'); } - const parsed = kmlParser.parse(xmlWithoutNamespaces); + const parsed = kmlParser.parse(decoded.text); const kmlRoot = parsed?.kml ?? parsed; if (!kmlRoot || typeof kmlRoot !== 'object') { @@ -391,7 +392,6 @@ export function importKmlPlaces(tripId: string, fileBuffer: Buffer): PlaceImport }); insertAll(); - summary.skippedCount = summary.totalPlacemarks - summary.createdCount; if (summary.totalPlacemarks === 0) { summary.errors.push('No Placemarks found in KML file.'); @@ -400,7 +400,10 @@ export function importKmlPlaces(tripId: string, fileBuffer: Buffer): PlaceImport return { places: created, count: created.length, summary }; } -export async function unpackKmzToKml(kmzBuffer: Buffer): Promise { +export async function unpackKmzToKml( + kmzBuffer: Buffer, + decompressedSizeLimit = KMZ_DECOMPRESSED_SIZE_LIMIT, +): Promise { let zip; try { zip = await unzipper.Open.buffer(kmzBuffer); @@ -414,6 +417,11 @@ export async function unpackKmzToKml(kmzBuffer: Buffer): Promise { } const preferredEntry = kmlEntries.find((entry) => entry.path.toLowerCase().endsWith('doc.kml')) || kmlEntries[0]; + + if (preferredEntry.uncompressedSize > decompressedSizeLimit) { + throw new Error('KMZ archive exceeds the maximum allowed decompressed size.'); + } + return preferredEntry.buffer(); } diff --git a/server/tests/unit/services/kmlImportUtils.test.ts b/server/tests/unit/services/kmlImportUtils.test.ts index 7ac8dd7d..330543d3 100644 --- a/server/tests/unit/services/kmlImportUtils.test.ts +++ b/server/tests/unit/services/kmlImportUtils.test.ts @@ -7,18 +7,9 @@ import { parsePlacemarkNode, resolveCategoryIdForFolder, sanitizeKmlDescription, - stripXmlNamespaces, } from '../../../src/services/kmlImport'; describe('kmlImportUtils', () => { - it('strips KML namespaces and prefixes', () => { - const xml = ''; - const stripped = stripXmlNamespaces(xml); - expect(stripped).not.toContain('xmlns'); - expect(stripped).toContain(''); - expect(stripped).toContain(' { const input = 'Line 1
Line 2 & more'; const output = sanitizeKmlDescription(input); @@ -64,6 +55,16 @@ describe('kmlImportUtils', () => { expect(resolveCategoryIdForFolder('parks', lookup)).toBe(4); }); + it('decodes non-BMP decimal HTML entities (emoji)', () => { + // 😀 = U+1F600 = 😀 — requires String.fromCodePoint, not fromCharCode + expect(sanitizeKmlDescription('😀')).toBe('😀'); + }); + + it('decodes non-BMP hex HTML entities (emoji)', () => { + // 😀 = U+1F600 = 😀 + expect(sanitizeKmlDescription('😀')).toBe('😀'); + }); + it('returns warning for non-UTF8 payload', () => { const buffer = Buffer.concat([ Buffer.from('Caf'), diff --git a/server/tests/unit/services/kmzUnpack.test.ts b/server/tests/unit/services/kmzUnpack.test.ts new file mode 100644 index 00000000..30e9fdd3 --- /dev/null +++ b/server/tests/unit/services/kmzUnpack.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect, vi } from 'vitest'; +import path from 'path'; +import fs from 'fs'; + +vi.mock('../../../src/db/database', () => ({ + db: { prepare: vi.fn() }, + getPlaceWithTags: vi.fn(), +})); +vi.mock('../../../src/config', () => ({ + JWT_SECRET: 'test', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, +})); + +import { unpackKmzToKml, KMZ_DECOMPRESSED_SIZE_LIMIT } from '../../../src/services/placeService'; + +const KMZ_FIXTURE = path.join(__dirname, '../../fixtures/test.kmz'); + +describe('unpackKmzToKml', () => { + it('extracts the KML entry from a valid KMZ', async () => { + const kmzBuffer = fs.readFileSync(KMZ_FIXTURE); + const kmlBuffer = await unpackKmzToKml(kmzBuffer); + expect(kmlBuffer.length).toBeGreaterThan(0); + expect(kmlBuffer.toString('utf-8')).toContain(' { + const kmzBuffer = fs.readFileSync(KMZ_FIXTURE); + // test.kmz contains a KML with uncompressedSize 634 — set limit to 1 byte + await expect(unpackKmzToKml(kmzBuffer, 1)).rejects.toThrow('exceeds the maximum allowed decompressed size'); + }); + + it('rejects a KMZ that contains no KML file', async () => { + // Craft a minimal ZIP containing only a non-KML entry using raw ZIP bytes + // We use the test GPX fixture (a real file) re-zipped via Node's zlib/archiver + // Simplest: a KMZ whose only file has a .txt extension + const Archiver = await import('archiver'); + const archiver = Archiver.default; + const { PassThrough } = await import('stream'); + + const chunks: Buffer[] = []; + const output = new PassThrough(); + output.on('data', (chunk) => chunks.push(chunk)); + + const archive = archiver('zip', { zlib: { level: 1 } }); + archive.pipe(output); + archive.append(Buffer.from('not a kml'), { name: 'data.txt' }); + await archive.finalize(); + + const zipBuffer = Buffer.concat(chunks); + await expect(unpackKmzToKml(zipBuffer)).rejects.toThrow('does not contain a KML file'); + }); + + it('rejects a buffer that is not a valid ZIP archive', async () => { + await expect(unpackKmzToKml(Buffer.from('this is not a zip'))).rejects.toThrow('Invalid KMZ archive'); + }); + + it('exports KMZ_DECOMPRESSED_SIZE_LIMIT as 50 MB', () => { + expect(KMZ_DECOMPRESSED_SIZE_LIMIT).toBe(50 * 1024 * 1024); + }); +});