Backend/frontend hardening & consistency cleanups (#1113)

* refactor(auth): session token validation and password-change consistency

* refactor(journey): entry field allow-list and public share-link consistency

* refactor(mcp): align tool authorization with the REST permission checks

* chore: input validation and sanitisation touch-ups (uploads, pdf, maps, backup, csp)

server/tests/unit/services/authServiceDb.test.ts

@@ -36,6 +36,7 @@ vi.mock('../../../src/db/database', () => dbMock);
 vi.mock('../../../src/config', () => ({
   JWT_SECRET: 'test-secret',
   ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2',
+  SESSION_DURATION_SECONDS: 86400,
   updateJwtSecret: () => {},
 }));
 vi.mock('../../../src/services/mfaCrypto', () => ({
@@ -88,7 +89,9 @@ import {
   verifyMfaLogin,
   createMcpToken,
   deleteMcpToken,
+  generateToken,
 } from '../../../src/services/authService';
+import { verifyJwtAndLoadUser } from '../../../src/middleware/auth';
 
 // ---------------------------------------------------------------------------
 // Lifecycle
@@ -573,6 +576,39 @@ describe('changePassword — OIDC-only mode', () => {
   });
 });
 
+describe('changePassword — session invalidation', () => {
+  const pvOf = (id: number) =>
+    (testDb.prepare('SELECT password_version FROM users WHERE id = ?').get(id) as { password_version: number }).password_version;
+  const mcpCount = (id: number) =>
+    (testDb.prepare('SELECT COUNT(*) c FROM mcp_tokens WHERE user_id = ?').get(id) as { c: number }).c;
+
+  it('AUTH-DB-036b: bumps password_version, prunes MCP tokens, and re-issues a session', () => {
+    const { user, password } = createUser(testDb);
+    createMcpToken(user.id, 'cli');
+
+    expect(pvOf(user.id)).toBe(0);
+    expect(mcpCount(user.id)).toBe(1);
+
+    const result = changePassword(user.id, user.email, { current_password: password, new_password: 'New1234!' });
+
+    expect(result.success).toBe(true);
+    expect(typeof result.token).toBe('string'); // fresh session for the current device
+    expect(pvOf(user.id)).toBe(1); // old JWT/cookie sessions now rejected by the pv gate
+    expect(mcpCount(user.id)).toBe(0); // static MCP tokens revoked
+  });
+
+  it('AUTH-DB-036c: a token minted before the change no longer validates afterwards', () => {
+    const { user, password } = createUser(testDb);
+    const stolen = generateToken({ id: user.id }); // pv=0 at mint time
+
+    expect(verifyJwtAndLoadUser(stolen)).not.toBeNull();
+
+    changePassword(user.id, user.email, { current_password: password, new_password: 'New1234!' });
+
+    expect(verifyJwtAndLoadUser(stolen)).toBeNull(); // invalidated by the pv bump
+  });
+});
+
 // ---------------------------------------------------------------------------
 // disableMfa — require_mfa policy
 // ---------------------------------------------------------------------------

server/tests/unit/services/backupService.test.ts

@@ -33,6 +33,9 @@ const archiverMock = vi.hoisted(() => vi.fn());
 
 const unzipperMock = vi.hoisted(() => ({
   Extract: vi.fn(),
+  // Central-directory reader used for the pre-extract zip-bomb size check.
+  // Default to an empty archive so existing restore tests proceed to Extract.
+  Open: { file: vi.fn().mockResolvedValue({ files: [] }) },
 }));
 
 const dbMock = vi.hoisted(() => ({
@@ -532,6 +535,19 @@ describe('BACKUP-038 restoreFromZip', () => {
     expect(result.error).toMatch(/travel\.db not found/i);
     expect(result.status).toBe(400);
   });
+
+  it('BACKUP-038b — rejects a zip bomb whose declared decompressed size exceeds the cap', async () => {
+    unzipperMock.Open.file.mockResolvedValueOnce({
+      files: [{ uncompressedSize: 6 * 1024 * 1024 * 1024 }], // 6 GB > 5 GB cap
+    });
+
+    const result = await restoreFromZip('/data/tmp/bomb.zip');
+
+    expect(result.success).toBe(false);
+    expect(result.status).toBe(400);
+    expect(result.error).toMatch(/decompressed size/i);
+    expect(unzipperMock.Extract).not.toHaveBeenCalled(); // bailed before extracting
+  });
 });
 
 // ---------------------------------------------------------------------------

server/tests/unit/services/budgetServiceDb.test.ts

@@ -0,0 +1,83 @@
+/**
+ * DB-backed unit tests for budgetService trip-scoping (BUDGET-SVC-DB-001+).
+ * Uses a real in-memory SQLite DB so the SQL WHERE clauses are exercised.
+ */
+import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest';
+
+const { testDb, dbMock } = vi.hoisted(() => {
+  const Database = require('better-sqlite3');
+  const db = new Database(':memory:');
+  db.exec('PRAGMA journal_mode = WAL');
+  db.exec('PRAGMA foreign_keys = ON');
+  db.exec('PRAGMA busy_timeout = 5000');
+  const mock = {
+    db,
+    closeDb: () => {},
+    reinitialize: () => {},
+    getPlaceWithTags: () => null,
+    canAccessTrip: () => null,
+    isOwner: () => false,
+  };
+  return { testDb: db, dbMock: mock };
+});
+
+vi.mock('../../../src/db/database', () => dbMock);
+vi.mock('../../../src/config', () => ({
+  JWT_SECRET: 'test-secret',
+  ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2',
+  updateJwtSecret: () => {},
+}));
+
+import { createTables } from '../../../src/db/schema';
+import { runMigrations } from '../../../src/db/migrations';
+import { resetTestDb } from '../../helpers/test-db';
+import { createUser, createTrip } from '../../helpers/factories';
+import { createBudgetItem, updateMembers, toggleMemberPaid } from '../../../src/services/budgetService';
+
+beforeAll(() => {
+  createTables(testDb);
+  runMigrations(testDb);
+});
+
+beforeEach(() => {
+  resetTestDb(testDb);
+});
+
+afterAll(() => {
+  testDb.close();
+});
+
+function paidFlag(itemId: number, memberId: number): number | undefined {
+  const row = testDb
+    .prepare('SELECT paid FROM budget_item_members WHERE budget_item_id = ? AND user_id = ?')
+    .get(itemId, memberId) as { paid: number } | undefined;
+  return row?.paid;
+}
+
+describe('toggleMemberPaid trip-scoping', () => {
+  it('BUDGET-SVC-DB-001: toggles paid for an item that belongs to the given trip', () => {
+    const { user } = createUser(testDb);
+    const trip = createTrip(testDb, user.id, { title: 'Trip A' });
+    const item = createBudgetItem(trip.id, { name: 'Hotel', total_price: 100 });
+    updateMembers(item.id, trip.id, [user.id]);
+
+    const member = toggleMemberPaid(item.id, trip.id, user.id, true);
+
+    expect(member).not.toBeNull();
+    expect(paidFlag(item.id, user.id)).toBe(1);
+  });
+
+  it('BUDGET-SVC-DB-002: refuses to toggle an item from a different trip (cross-trip IDOR)', () => {
+    const { user } = createUser(testDb);
+    const tripA = createTrip(testDb, user.id, { title: 'Trip A' });
+    const tripB = createTrip(testDb, user.id, { title: 'Trip B' });
+    const itemB = createBudgetItem(tripB.id, { name: 'Foreign expense', total_price: 50 });
+    updateMembers(itemB.id, tripB.id, [user.id]);
+
+    // Caller passes a trip they can access (A) but the item lives in trip B.
+    const member = toggleMemberPaid(itemB.id, tripA.id, user.id, true);
+
+    expect(member).toBeNull();
+    expect(paidFlag(itemB.id, user.id)).toBe(0); // unchanged
+  });
+});

server/tests/unit/services/journeyService.test.ts

@@ -596,6 +596,32 @@ describe('updateEntry', () => {
 
     expect(result).toBeNull();
   });
+
+  it('JOURNEY-SVC-034b: ignores injection column keys and mass-assignment attempts', () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+    const entry = createJourneyEntry(testDb, journey.id, user.id, {
+      title: 'Safe',
+      story: 'original',
+      entry_date: '2026-03-01',
+    });
+
+    // The keys come straight from the request body. A crafted key was previously
+    // interpolated as a raw SQL column name (`${key} = ?`), enabling subquery
+    // injection (full DB read) and mass-assignment of protected columns.
+    const malicious: Record<string, unknown> = {
+      title: 'Updated',
+      [`story = (SELECT password_hash FROM users WHERE id = ${user.id}), updated_at`]: 'x',
+      author_id: 999999,
+    };
+
+    const updated = updateEntry(entry.id, user.id, malicious as Parameters<typeof updateEntry>[2]);
+
+    expect(updated).not.toBeNull();
+    expect(updated!.title).toBe('Updated'); // legit field still applied
+    expect(updated!.story).toBe('original'); // injection key dropped — no hash leaked into story
+    expect(updated!.author_id).toBe(user.id); // mass-assignment blocked
+  });
 });
 
 describe('deleteEntry', () => {

server/tests/unit/services/journeyShareService.test.ts

@@ -300,15 +300,17 @@ describe('validateShareTokenForAsset', () => {
     expect(result).toBeNull();
   });
 
-  it('JOURNEY-SHARE-015: falls back to journey owner when asset not found in photos', () => {
+  it('JOURNEY-SHARE-015: denies (returns null) when the asset is not part of the shared journey', () => {
     const { user } = createUser(testDb);
     const journey = createJourney(testDb, user.id);
     const { token } = createOrUpdateJourneyShareLink(journey.id, user.id, {});
 
+    // A valid share token must NOT resolve arbitrary asset IDs to the owner —
+    // otherwise it could proxy any asset out of the owner's Immich/Synology
+    // library (IDOR). Only assets actually in the journey may resolve.
     const result = validateShareTokenForAsset(token, 'nonexistent-asset');
 
-    expect(result).not.toBeNull();
-    expect(result!.ownerId).toBe(user.id);
+    expect(result).toBeNull();
   });
 });
 
@@ -414,4 +416,76 @@ describe('getPublicJourney', () => {
     expect(result!.stats.photos).toBe(0);
     expect(result!.stats.places).toBe(0);
   });
+
+  it('JOURNEY-SHARE-021: withholds timeline, gallery and GPS when all flags are off', () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id, { title: 'Secret' });
+    const entry = createJourneyEntry(testDb, journey.id, user.id, {
+      type: 'entry', title: 'Day 1', story: 'private notes', entry_date: '2026-05-01', location_name: 'Paris',
+    });
+    testDb.prepare('UPDATE journey_entries SET location_lat = ?, location_lng = ? WHERE id = ?').run(48.8566, 2.3522, entry.id);
+    insertJourneyPhoto(entry.id);
+    const { token } = createOrUpdateJourneyShareLink(journey.id, user.id, {
+      share_timeline: false, share_gallery: false, share_map: false,
+    });
+
+    const result = getPublicJourney(token)!;
+    expect(result.entries).toEqual([]); // no timeline / story / GPS leaked
+    expect(result.gallery).toEqual([]); // no gallery leaked
+    expect(result.stats.entries).toBe(1); // counts stay accurate
+  });
+
+  it('JOURNEY-SHARE-022: shares the timeline but strips GPS when the map flag is off', () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+    const entry = createJourneyEntry(testDb, journey.id, user.id, {
+      type: 'entry', title: 'Day 1', story: 'notes', entry_date: '2026-05-01', location_name: 'Paris',
+    });
+    testDb.prepare('UPDATE journey_entries SET location_lat = ?, location_lng = ? WHERE id = ?').run(48.8566, 2.3522, entry.id);
+    const { token } = createOrUpdateJourneyShareLink(journey.id, user.id, {
+      share_timeline: true, share_gallery: true, share_map: false,
+    });
+
+    const result = getPublicJourney(token)!;
+    expect(result.entries).toHaveLength(1);
+    const e = result.entries[0] as Record<string, unknown>;
+    expect(e.story).toBe('notes'); // narrative present
+    expect(e.location_lat).toBeNull(); // GPS withheld
+    expect(e.location_lng).toBeNull();
+  });
+
+  it('JOURNEY-SHARE-023: map-only share exposes coordinates but not the story', () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+    const entry = createJourneyEntry(testDb, journey.id, user.id, {
+      type: 'entry', title: 'Day 1', story: 'private notes', entry_date: '2026-05-01', location_name: 'Paris',
+    });
+    testDb.prepare('UPDATE journey_entries SET location_lat = ?, location_lng = ? WHERE id = ?').run(48.8566, 2.3522, entry.id);
+    const { token } = createOrUpdateJourneyShareLink(journey.id, user.id, {
+      share_timeline: false, share_gallery: false, share_map: true,
+    });
+
+    const result = getPublicJourney(token)!;
+    expect(result.entries).toHaveLength(1);
+    const e = result.entries[0] as Record<string, unknown>;
+    expect(e.location_lat).toBe(48.8566); // coords for the map
+    expect(e.story).toBeUndefined(); // narrative withheld
+  });
+
+  it('JOURNEY-SHARE-024: strips inline entry photos (and their asset metadata) when the gallery is off', () => {
+    const { user } = createUser(testDb);
+    const journey = createJourney(testDb, user.id);
+    const entry = createJourneyEntry(testDb, journey.id, user.id, {
+      type: 'entry', title: 'Day 1', story: 'notes', entry_date: '2026-05-01',
+    });
+    insertJourneyPhoto(entry.id, { ownerId: user.id });
+    const { token } = createOrUpdateJourneyShareLink(journey.id, user.id, {
+      share_timeline: true, share_gallery: false, share_map: true,
+    });
+
+    const result = getPublicJourney(token)!;
+    expect(result.gallery).toEqual([]); // gallery array withheld
+    expect(result.entries).toHaveLength(1);
+    expect((result.entries[0] as Record<string, unknown>).photos).toEqual([]); // inline photos withheld too
+  });
 });

server/tests/unit/services/oidcService.test.ts

@@ -313,7 +313,7 @@ describe('findOrCreateUser', () => {
     const { user } = createUser(testDb, { email: 'bob@example.com' });
 
     const result = findOrCreateUser(
-      { sub: 'sub-bob-new', email: 'bob@example.com', name: 'Bob' },
+      { sub: 'sub-bob-new', email: 'bob@example.com', name: 'Bob', email_verified: true },
       MOCK_CONFIG
     );
     expect('user' in result).toBe(true);
@@ -352,13 +352,13 @@ describe('findOrCreateUser', () => {
     expect((result as { error: string }).error).toBe('registration_disabled');
   });
 
-  it('OIDC-SVC-025: links oidc_sub when existing user has none', () => {
+  it('OIDC-SVC-025: links oidc_sub when existing user has none (verified email)', () => {
     const { user } = createUser(testDb, { email: 'charlie@example.com' });
     // Ensure no oidc_sub set
     testDb.prepare('UPDATE users SET oidc_sub = NULL, oidc_issuer = NULL WHERE id = ?').run(user.id);
 
     findOrCreateUser(
-      { sub: 'sub-charlie-linked', email: 'charlie@example.com', name: 'Charlie' },
+      { sub: 'sub-charlie-linked', email: 'charlie@example.com', name: 'Charlie', email_verified: true },
       MOCK_CONFIG
     );
 
@@ -366,6 +366,23 @@ describe('findOrCreateUser', () => {
     expect(updated.oidc_sub).toBe('sub-charlie-linked');
   });
 
+  it('OIDC-SVC-025b: refuses to link an unverified email to an existing local account', () => {
+    const { user } = createUser(testDb, { email: 'dora@example.com' });
+    testDb.prepare('UPDATE users SET oidc_sub = NULL, oidc_issuer = NULL WHERE id = ?').run(user.id);
+
+    // No email_verified claim — an IdP that lets users set arbitrary emails must
+    // not be able to take over a pre-existing password account.
+    const result = findOrCreateUser(
+      { sub: 'sub-dora-attacker', email: 'dora@example.com', name: 'Dora' },
+      MOCK_CONFIG
+    );
+
+    expect('error' in result).toBe(true);
+    expect((result as { error: string }).error).toBe('email_not_verified');
+    const updated = testDb.prepare('SELECT oidc_sub FROM users WHERE id = ?').get(user.id) as any;
+    expect(updated.oidc_sub).toBeNull(); // account not linked / not hijacked
+  });
+
   it('OIDC-SVC-026: existing user role is updated when OIDC claim mapping changes it', () => {
     const { user } = createUser(testDb, { email: 'diana@example.com', role: 'user' });
     // Link oidc_sub manually so the user is found by sub lookup

server/tests/unit/services/tripService.test.ts

@@ -34,7 +34,8 @@ import { createTables } from '../../../src/db/schema';
 import { runMigrations } from '../../../src/db/migrations';
 import { resetTestDb } from '../../helpers/test-db';
 import { createUser, createTrip, createReservation, createPlace, createDay, createDayAssignment, createDayNote } from '../../helpers/factories';
-import { exportICS, generateDays } from '../../../src/services/tripService';
+import { exportICS, generateDays, deleteOldCover } from '../../../src/services/tripService';
+import fs from 'fs';
 
 beforeAll(() => {
   createTables(testDb);
@@ -397,3 +398,41 @@ describe('exportICS', () => {
     expect(ics).toContain('DTEND:20250602T160000');
   });
 });
+
+// ── deleteOldCover — path containment ──────────────────────────────────────────
+
+describe('deleteOldCover', () => {
+  it('TRIP-SVC-COVER-001: never unlinks outside uploads/covers for a crafted cover_image', () => {
+    const existsSpy = vi.spyOn(fs, 'existsSync').mockReturnValue(true);
+    const unlinkSpy = vi.spyOn(fs, 'unlinkSync').mockImplementation(() => {});
+    try {
+      // Attacker-controlled values aimed at auth-gated sibling upload dirs.
+      deleteOldCover('/uploads/files/victim.pdf');
+      deleteOldCover('/uploads/covers/../files/secret.pdf');
+      deleteOldCover('/uploads/avatars/someone.png');
+
+      for (const call of unlinkSpy.mock.calls) {
+        const target = String(call[0]);
+        expect(target).toMatch(/[\\/]uploads[\\/]covers[\\/]/); // stays in covers
+        expect(target).not.toMatch(/[\\/]files[\\/]/);
+        expect(target).not.toMatch(/[\\/]avatars[\\/]/);
+      }
+    } finally {
+      existsSpy.mockRestore();
+      unlinkSpy.mockRestore();
+    }
+  });
+
+  it('TRIP-SVC-COVER-002: deletes a legitimate cover file', () => {
+    const existsSpy = vi.spyOn(fs, 'existsSync').mockReturnValue(true);
+    const unlinkSpy = vi.spyOn(fs, 'unlinkSync').mockImplementation(() => {});
+    try {
+      deleteOldCover('/uploads/covers/abc123.jpg');
+      expect(unlinkSpy).toHaveBeenCalledTimes(1);
+      expect(String(unlinkSpy.mock.calls[0][0])).toMatch(/[\\/]covers[\\/]abc123\.jpg$/);
+    } finally {
+      existsSpy.mockRestore();
+      unlinkSpy.mockRestore();
+    }
+  });
+});