From 9789c51d4f3c6dd6f3a57cda3b87c8d2b430ed96 Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 15 Apr 2026 04:48:39 +0200 Subject: [PATCH] fix(naver-import): address PR #495 review issues - SSRF: validate user-supplied URLs with checkSsrf() before fetch in both importNaverList and importGoogleList; upgrade naver.me substring check to exact hostname comparison to prevent bypass - i18n: add missing places.importNaverList key to de.ts and es.ts - migration: switch Naver addon seed to INSERT OR IGNORE to preserve admin customizations on re-runs; restore budget_category_order CREATE TABLE to its original formatting - route: remove redundant cast after type-narrowing guard in naver-list handler - component: hoist provider ternary above try/catch in handleListImport - tests: add four new Naver import cases (502, empty list, no-coords, canonical URL skipping redirect fetch) --- .../src/components/Planner/PlacesSidebar.tsx | 3 +- client/src/i18n/translations/de.ts | 1 + client/src/i18n/translations/es.ts | 1 + server/src/db/migrations.ts | 49 ++------- server/src/routes/places.ts | 6 +- server/src/services/placeService.ts | 13 ++- server/tests/integration/places.test.ts | 100 ++++++++++++++++++ 7 files changed, 125 insertions(+), 48 deletions(-) diff --git a/client/src/components/Planner/PlacesSidebar.tsx b/client/src/components/Planner/PlacesSidebar.tsx index a027e500..49f1de50 100644 --- a/client/src/components/Planner/PlacesSidebar.tsx +++ b/client/src/components/Planner/PlacesSidebar.tsx @@ -86,8 +86,8 @@ const PlacesSidebar = React.memo(function PlacesSidebar({ const handleListImport = async () => { if (!listImportUrl.trim()) return setListImportLoading(true) + const provider = listImportProvider === 'naver' && isNaverListImportEnabled ? 'naver' : 'google' try { - const provider = listImportProvider === 'naver' && isNaverListImportEnabled ? 'naver' : 'google' const result = provider === 'google' ? await placesApi.importGoogleList(tripId, listImportUrl.trim()) : await placesApi.importNaverList(tripId, listImportUrl.trim()) @@ -105,7 +105,6 @@ const PlacesSidebar = React.memo(function PlacesSidebar({ }) } } catch (err: any) { - const provider = listImportProvider === 'naver' && isNaverListImportEnabled ? 'naver' : 'google' toast.error(err?.response?.data?.error || t(provider === 'google' ? 'places.googleListError' : 'places.naverListError')) } finally { setListImportLoading(false) diff --git a/client/src/i18n/translations/de.ts b/client/src/i18n/translations/de.ts index baa0ebcf..fe771a55 100644 --- a/client/src/i18n/translations/de.ts +++ b/client/src/i18n/translations/de.ts @@ -899,6 +899,7 @@ const de: Record = { 'places.gpxError': 'GPX-Import fehlgeschlagen', 'places.importList': 'Listenimport', 'places.importGoogleList': 'Google Liste', + 'places.importNaverList': 'Naver Liste', 'places.googleListHint': 'Geteilten Google Maps Listen-Link einfügen, um alle Orte zu importieren.', 'places.googleListImported': '{count} Orte aus "{list}" importiert', 'places.googleListError': 'Google Maps Liste konnte nicht importiert werden', diff --git a/client/src/i18n/translations/es.ts b/client/src/i18n/translations/es.ts index 3ac9313e..c6aab3f5 100644 --- a/client/src/i18n/translations/es.ts +++ b/client/src/i18n/translations/es.ts @@ -870,6 +870,7 @@ const es: Record = { 'places.gpxError': 'Error al importar GPX', 'places.importList': 'Importar lista', 'places.importGoogleList': 'Lista Google', + 'places.importNaverList': 'Lista Naver', 'places.googleListHint': 'Pega un enlace compartido de una lista de Google Maps para importar todos los lugares.', 'places.googleListImported': '{count} lugares importados de "{list}"', 'places.googleListError': 'Error al importar la lista de Google Maps', diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index f56aed10..d7202469 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -868,39 +868,20 @@ function runMigrations(db: Database.Database): void { // Migration: Budget category ordering () => { db.exec(` - CREATE TABLE IF NOT EXISTS budget_category_order - ( - trip_id - INTEGER - NOT - NULL - REFERENCES - trips - ( - id - ) ON DELETE CASCADE, + CREATE TABLE IF NOT EXISTS budget_category_order ( + trip_id INTEGER NOT NULL REFERENCES trips(id) ON DELETE CASCADE, category TEXT NOT NULL, sort_order INTEGER NOT NULL DEFAULT 0, - PRIMARY KEY - ( - trip_id, - category - ) - ); + PRIMARY KEY (trip_id, category) + ); `); // Seed existing categories with alphabetical order - const rows = db.prepare('SELECT DISTINCT trip_id, category FROM budget_items ORDER BY trip_id, category').all() as { - trip_id: number; - category: string - }[]; + const rows = db.prepare('SELECT DISTINCT trip_id, category FROM budget_items ORDER BY trip_id, category').all() as { trip_id: number; category: string }[]; const ins = db.prepare('INSERT OR IGNORE INTO budget_category_order (trip_id, category, sort_order) VALUES (?, ?, ?)'); let lastTripId = -1; let idx = 0; for (const r of rows) { - if (r.trip_id !== lastTripId) { - lastTripId = r.trip_id; - idx = 0; - } + if (r.trip_id !== lastTripId) { lastTripId = r.trip_id; idx = 0; } ins.run(r.trip_id, r.category, idx++); } }, @@ -908,23 +889,9 @@ function runMigrations(db: Database.Database): void { () => { try { db.prepare(` - INSERT INTO addons (id, name, description, type, icon, enabled, sort_order) + INSERT OR IGNORE INTO addons (id, name, description, type, icon, enabled, sort_order) VALUES (?, ?, ?, ?, ?, ?, ?) - ON CONFLICT(id) DO UPDATE SET - name = excluded.name, - description = excluded.description, - type = excluded.type, - icon = excluded.icon, - sort_order = excluded.sort_order - `).run( - 'naver_list_import', - 'Naver List Import', - 'Import places from shared Naver Maps lists', - 'trip', - 'Link2', - 0, - 13, - ); + `).run('naver_list_import', 'Naver List Import', 'Import places from shared Naver Maps lists', 'trip', 'Link2', 0, 13); } catch (err: any) { console.warn('[migrations] Non-fatal migration step failed:', err); } diff --git a/server/src/routes/places.ts b/server/src/routes/places.ts index 86878449..4e1b1208 100644 --- a/server/src/routes/places.ts +++ b/server/src/routes/places.ts @@ -123,10 +123,8 @@ router.post('/import/naver-list', authenticate, requireTripAccess, async (req: R return res.status(result.status).json({ error: result.error }); } - const successResult = result as { places: any[]; listName: string }; - - res.status(201).json({ places: successResult.places, count: successResult.places.length, listName: successResult.listName }); - for (const place of successResult.places) { + res.status(201).json({ places: result.places, count: result.places.length, listName: result.listName }); + for (const place of result.places) { broadcast(tripId, 'place:created', { place }, req.headers['x-socket-id'] as string); } } catch (err: unknown) { diff --git a/server/src/services/placeService.ts b/server/src/services/placeService.ts index 38c6ed7d..63822f5f 100644 --- a/server/src/services/placeService.ts +++ b/server/src/services/placeService.ts @@ -1,6 +1,7 @@ import { XMLParser } from 'fast-xml-parser'; import { db, getPlaceWithTags } from '../db/database'; import { loadTagsByPlaceIds } from './queryHelpers'; +import { checkSsrf } from '../utils/ssrfGuard'; import { Place } from '../types'; interface PlaceWithCategory extends Place { @@ -309,6 +310,10 @@ export async function importGoogleList(tripId: string, url: string) { let listId: string | null = null; let resolvedUrl = url; + // SSRF guard: validate user-supplied URL before fetching + const ssrf = await checkSsrf(url); + if (!ssrf.allowed) return { error: 'URL is not allowed', status: 400 }; + // Follow redirects for short URLs (maps.app.goo.gl, goo.gl) if (url.includes('goo.gl') || url.includes('maps.app')) { const redirectRes = await fetch(url, { redirect: 'follow', signal: AbortSignal.timeout(10000) }); @@ -416,8 +421,14 @@ export async function importNaverList( let resolvedUrl = url; const limit = 20; + // SSRF guard: validate user-supplied URL before fetching + const ssrf = await checkSsrf(url); + if (!ssrf.allowed) return { error: 'URL is not allowed', status: 400 }; + // Resolve naver.me short links to the canonical map.naver.com folder URL. - if (url.includes('naver.me')) { + let parsedUrl: URL; + try { parsedUrl = new URL(url); } catch { return { error: 'Invalid URL', status: 400 }; } + if (parsedUrl.hostname === 'naver.me') { const redirectRes = await fetch(url, { redirect: 'follow', signal: AbortSignal.timeout(10000) }); resolvedUrl = redirectRes.url; } diff --git a/server/tests/integration/places.test.ts b/server/tests/integration/places.test.ts index 6bf3ad2f..5b58033d 100644 --- a/server/tests/integration/places.test.ts +++ b/server/tests/integration/places.test.ts @@ -603,6 +603,106 @@ describe('Naver list import', () => { expect(res.status).toBe(400); expect(res.body.error).toContain('Could not extract folder ID'); }); + + it('POST /import/naver-list returns 502 when Naver API is unavailable', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const folderId = 'abc123'; + + testDb.prepare("UPDATE addons SET enabled = 1 WHERE id = 'naver_list_import'").run(); + + const fetchMock = vi.fn() + .mockResolvedValueOnce({ ok: false }); + + vi.stubGlobal('fetch', fetchMock); + + const res = await request(app) + .post(`/api/trips/${trip.id}/places/import/naver-list`) + .set('Cookie', authCookie(user.id)) + .send({ url: `https://map.naver.com/v5/favorite/myPlace/folder/${folderId}` }); + + expect(res.status).toBe(502); + expect(res.body.error).toContain('Failed to fetch list from Naver Maps'); + }); + + it('POST /import/naver-list returns 400 when list is empty', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const folderId = 'abc123'; + + testDb.prepare("UPDATE addons SET enabled = 1 WHERE id = 'naver_list_import'").run(); + + const fetchMock = vi.fn().mockResolvedValueOnce({ + ok: true, + json: async () => ({ folder: { name: 'Empty List', bookmarkCount: 0 }, bookmarkList: [] }), + }); + + vi.stubGlobal('fetch', fetchMock); + + const res = await request(app) + .post(`/api/trips/${trip.id}/places/import/naver-list`) + .set('Cookie', authCookie(user.id)) + .send({ url: `https://map.naver.com/v5/favorite/myPlace/folder/${folderId}` }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain('List is empty or could not be read'); + }); + + it('POST /import/naver-list returns 400 when all items lack valid coordinates', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const folderId = 'abc123'; + + testDb.prepare("UPDATE addons SET enabled = 1 WHERE id = 'naver_list_import'").run(); + + const fetchMock = vi.fn().mockResolvedValueOnce({ + ok: true, + json: async () => ({ + folder: { name: 'No Coords', bookmarkCount: 2 }, + bookmarkList: [ + { name: 'Place A', px: undefined, py: undefined }, + { name: 'Place B', px: 'not-a-number', py: 'not-a-number' }, + ], + }), + }); + + vi.stubGlobal('fetch', fetchMock); + + const res = await request(app) + .post(`/api/trips/${trip.id}/places/import/naver-list`) + .set('Cookie', authCookie(user.id)) + .send({ url: `https://map.naver.com/v5/favorite/myPlace/folder/${folderId}` }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain('No places with coordinates found in list'); + }); + + it('POST /import/naver-list accepts canonical map.naver.com URL without redirect fetch', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const folderId = 'abc123'; + + testDb.prepare("UPDATE addons SET enabled = 1 WHERE id = 'naver_list_import'").run(); + + const fetchMock = vi.fn().mockResolvedValueOnce({ + ok: true, + json: async () => ({ + folder: { name: 'Seoul', bookmarkCount: 1 }, + bookmarkList: [{ name: 'Gyeongbokgung', px: 126.9770, py: 37.5796, memo: null, address: 'Sejongno Seoul' }], + }), + }); + + vi.stubGlobal('fetch', fetchMock); + + const res = await request(app) + .post(`/api/trips/${trip.id}/places/import/naver-list`) + .set('Cookie', authCookie(user.id)) + .send({ url: `https://map.naver.com/v5/favorite/myPlace/folder/${folderId}` }); + + expect(res.status).toBe(201); + expect(res.body.count).toBe(1); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); }); // ─────────────────────────────────────────────────────────────────────────────