diff --git a/server/src/nest/files/files.constants.ts b/server/src/nest/files/files.constants.ts index e3144ce1b..ea336244b 100644 --- a/server/src/nest/files/files.constants.ts +++ b/server/src/nest/files/files.constants.ts @@ -31,8 +31,10 @@ export function isVideoExtension(ext: string): boolean { // `allowed_file_types` is `*`, this list is still enforced so the // wildcard doesn't silently admit executables/scripts. export const BLOCKED_EXTENSIONS = [ - // Server-rendered / scripted content that could XSS a viewer - '.svg', '.html', '.htm', '.xml', '.xhtml', + // Server-rendered / scripted content that could XSS a viewer. Downloads are + // served inline with an extension-derived Content-Type, so every spelling a + // browser renders as a document has to be listed, not just the common ones. + '.svg', '.svgz', '.html', '.htm', '.shtml', '.shtm', '.xml', '.xhtml', '.xht', // Scripts '.js', '.jsx', '.ts', '.tsx', '.mjs', '.cjs', '.php', '.py', '.rb', '.pl', // Executables diff --git a/server/src/nest/files/files.controller.ts b/server/src/nest/files/files.controller.ts index 34f2c0767..246c4b953 100644 --- a/server/src/nest/files/files.controller.ts +++ b/server/src/nest/files/files.controller.ts @@ -279,6 +279,13 @@ export class FilesController { if (!this.files.can('file_edit', trip, user)) { throw new HttpException({ error: 'No permission' }, 403); } + // deleteFileLink scopes by (linkId, fileId) only, so the file itself has to + // be resolved against :tripId first. Otherwise a member of any trip could + // drop a link row belonging to a foreign trip's file. + const file = this.files.getFileById(id, tripId); + if (!file) { + throw new HttpException({ error: 'File not found' }, 404); + } this.files.deleteFileLink(linkId, id); return { success: true }; } @@ -286,6 +293,10 @@ export class FilesController { @UseGuards(TripAccessGuard) @Get(':id/links') links(@CurrentUser() user: User, @Trip() trip: TripAccess, @Param('tripId') tripId: string, @Param('id') id: string) { + const file = this.files.getFileById(id, tripId); + if (!file) { + throw new HttpException({ error: 'File not found' }, 404); + } return { links: this.files.getFileLinks(id) }; } } diff --git a/server/src/nest/journey/journey-book.service.ts b/server/src/nest/journey/journey-book.service.ts index 57c2b5da6..9df274c64 100644 --- a/server/src/nest/journey/journey-book.service.ts +++ b/server/src/nest/journey/journey-book.service.ts @@ -9,10 +9,11 @@ import { JourneyDomainService } from './journey-domain.service'; * * ── Access ─────────────────────────────────────────────────────────────── * - * A book belongs to its journey, so it inherits the journey's access exactly — - * every contributor can open and edit it. There is no separate book permission - * and there should not be: a second access model over the same object is how - * two rules end up disagreeing about who may do what. + * A book belongs to its journey, so it inherits the journey's access exactly: + * every contributor can open it, and the ones who may edit the journey may edit + * it. There is no separate book permission and there should not be: a second + * access model over the same object is how two rules end up disagreeing about + * who may do what. * * ── Concurrency ────────────────────────────────────────────────────────── * @@ -37,6 +38,15 @@ export class JourneyBookService { return !!this.journey.canAccessJourney(journeyId, userId); } + /** + * Writing the book is an edit like any other in this domain, so it takes the + * same owner-or-editor check the entry and photo writes take. canAccess also + * covers role 'viewer', who may read the book but must not overwrite it. + */ + private canWrite(journeyId: number, userId: number): boolean { + return this.journey.canEdit(journeyId, userId); + } + /** * Whether this user may open the journey's book at all. * @@ -115,7 +125,7 @@ export class JourneyBookService { userId: number, input: { title: string; document: unknown; baseVersion?: number }, ): { record: BookRecord } | { conflict: BookRecord } | null { - if (!this.canAccess(journeyId, userId)) return null; + if (!this.canWrite(journeyId, userId)) return null; const document = JSON.stringify(normalizeBookDocument(input.document)); const existing = this.db @@ -160,7 +170,7 @@ export class JourneyBookService { } deleteBook(journeyId: number, userId: number): boolean | null { - if (!this.canAccess(journeyId, userId)) return null; + if (!this.canWrite(journeyId, userId)) return null; const result = this.db .prepare('DELETE FROM journey_books WHERE journey_id = ?') .run(journeyId); diff --git a/server/src/nest/journey/journey-public.controller.ts b/server/src/nest/journey/journey-public.controller.ts index fff8c467f..a52fe23af 100644 --- a/server/src/nest/journey/journey-public.controller.ts +++ b/server/src/nest/journey/journey-public.controller.ts @@ -1,8 +1,11 @@ -import { Controller, Get, HttpException, Param, Res } from '@nestjs/common'; +import { Controller, Get, HttpException, Param, Res, UseGuards } from '@nestjs/common'; import type { Response } from 'express'; import path from 'node:path'; import { JourneyService } from './journey.service'; import { StorageService } from '../storage/storage.service'; +import { AddonGuard } from '../addons/addon.guard'; +import { RequireAddon } from '../addons/require-addon.decorator'; +import { ADDON_IDS } from '../../addons'; import { Public } from '../auth/public.decorator'; /** @@ -10,11 +13,18 @@ import { Public } from '../auth/public.decorator'; * proxy for publicly shared journeys. * * Byte-identical to the legacy Express route (server/src/routes/journeyPublic.ts): - * NOT behind any guard, every route validates the share token (404 on failure), + * no authentication, every route validates the share token (404 on failure), * the unified proxy streams by trek_photo_id and the legacy proxy serves local * files (with the uploads-dir traversal guard) or proxies immich/synology. + * + * The addon gate is the one guard here, and it has to be: turning the Journey + * addon off is meant to take the feature away, and without it already-published + * journeys stayed publicly readable while the authenticated surface went dark. + * AddonGuard only reads addon state, so it is safe in front of a @Public route. */ @Public('share-token validated: the whole point is a link that works without an account') +@UseGuards(AddonGuard) +@RequireAddon(ADDON_IDS.JOURNEY, 'Journey') @Controller('api/public/journey') export class JourneyPublicController { constructor( @@ -72,7 +82,9 @@ export class JourneyPublicController { return; } - const effectiveOwnerId = valid.ownerId || Number(ownerId); + // :ownerId stays in the path so old share links keep routing, but it is not + // read any more: the owner comes from the row the token resolved to. + const effectiveOwnerId = valid.ownerId; const streaming = this.journey.streamProviderAsset( res, provider, diff --git a/server/src/nest/journey/journey-share.service.ts b/server/src/nest/journey/journey-share.service.ts index 56a7ec1c9..53ad8ccf0 100644 --- a/server/src/nest/journey/journey-share.service.ts +++ b/server/src/nest/journey/journey-share.service.ts @@ -44,6 +44,22 @@ export class JourneyShareService { // able to publish the journey or change which screens are shared. if (!this.journey.isOwner(journeyId, createdBy)) return null; + const existing = this.db.prepare('SELECT token, share_timeline, share_gallery, share_map, newest_first FROM journey_share_tokens WHERE journey_id = ?') + .get(journeyId) as { token: string; share_timeline: number; share_gallery: number; share_map: number; newest_first: number } | undefined; + + if (existing) { + // An update only changes the flags it was actually given. Falling back to + // the create-time defaults here would silently re-publish a gallery or map + // the owner had switched off, at the unchanged token. + const share_timeline = permissions.share_timeline ?? !!existing.share_timeline; + const share_gallery = permissions.share_gallery ?? !!existing.share_gallery; + const share_map = permissions.share_map ?? !!existing.share_map; + const newest_first = permissions.newest_first ?? !!existing.newest_first; + this.db.prepare('UPDATE journey_share_tokens SET share_timeline = ?, share_gallery = ?, share_map = ?, newest_first = ? WHERE journey_id = ?') + .run(share_timeline ? 1 : 0, share_gallery ? 1 : 0, share_map ? 1 : 0, newest_first ? 1 : 0, journeyId); + return { token: existing.token, created: false }; + } + const { share_timeline = true, share_gallery = true, @@ -51,13 +67,6 @@ export class JourneyShareService { newest_first = false, } = permissions; - const existing = this.db.prepare('SELECT token FROM journey_share_tokens WHERE journey_id = ?').get(journeyId) as { token: string } | undefined; - if (existing) { - this.db.prepare('UPDATE journey_share_tokens SET share_timeline = ?, share_gallery = ?, share_map = ?, newest_first = ? WHERE journey_id = ?') - .run(share_timeline ? 1 : 0, share_gallery ? 1 : 0, share_map ? 1 : 0, newest_first ? 1 : 0, journeyId); - return { token: existing.token, created: false }; - } - const token = crypto.randomBytes(24).toString('base64url'); this.db.prepare('INSERT INTO journey_share_tokens (journey_id, token, created_by, share_timeline, share_gallery, share_map, newest_first) VALUES (?, ?, ?, ?, ?, ?, ?)') .run(journeyId, token, createdBy, share_timeline ? 1 : 0, share_gallery ? 1 : 0, share_map ? 1 : 0, newest_first ? 1 : 0); @@ -109,13 +118,18 @@ export class JourneyShareService { // owner shared the gallery. if (!row.share_gallery) return null; const photo = this.db.prepare(` - SELECT tkp.owner_id FROM journey_photos gp + SELECT tkp.owner_id, j.user_id AS journey_owner_id + FROM journey_photos gp JOIN trek_photos tkp ON tkp.id = gp.photo_id + JOIN journeys j ON j.id = gp.journey_id WHERE tkp.asset_id = ? AND gp.journey_id = ? `).get(assetId, row.journey_id) as any; // Only resolve assets that actually belong to this shared journey. if (!photo) return null; - return { ownerId: photo.owner_id }; + // trek_photos.owner_id can be NULL. The journey's owner is the fallback, the + // same one the photo proxy uses. Whose provider credentials get tried must + // never come from a number an anonymous caller put in the URL. + return { ownerId: photo.owner_id || photo.journey_owner_id }; } getPublicJourney(token: string) { diff --git a/server/src/nest/journey/journey.controller.ts b/server/src/nest/journey/journey.controller.ts index 2f00865ca..a1a9f7c54 100644 --- a/server/src/nest/journey/journey.controller.ts +++ b/server/src/nest/journey/journey.controller.ts @@ -152,6 +152,20 @@ export class JourneyController { } } + /** + * Drop uploads that were committed before the authorization result was known. + * + * The commit has to happen first (the Immich mirror reads the final path), so + * every refusal after it has to take the bytes back out. Nothing sweeps + * orphans, and a loop of rejected POSTs would otherwise just fill the disk. + */ + private async discardJourneyUploads(files: Express.Multer.File[]): Promise { + for (const f of files) { + if (f.path) { try { fs.unlinkSync(f.path); } catch { /* best-effort */ } } + await this.storage.delete('journey', f.filename).catch(() => {}); + } + } + // The add call carries only an asset id, so when and where the picture was taken // are fetched from the provider afterwards rather than trusted from the client // (#1614). Detached: a slow or unreachable provider must not hold up the add. @@ -220,10 +234,15 @@ export class JourneyController { // reads each file at its final uploads/journey path. await this.commitJourneyUploads(files); const results: unknown[] = []; + const refused: Express.Multer.File[] = []; for (const file of files) { const relativePath = `journey/${file.filename}`; const photo = this.journey.addPhoto(Number(entryId), user.id, relativePath, undefined, body?.caption as string | undefined); - if (!photo) continue; + if (!photo) { + // No row points at this file, so nothing would ever clean it up. + refused.push(file); + continue; + } // Mirror to Immich only when the user explicitly opted in (#730). if (this.journey.immichAutoUploadEnabled(user.id)) { try { @@ -238,6 +257,9 @@ export class JourneyController { } results.push(photo); } + if (refused.length) { + await this.discardJourneyUploads(refused); + } if (!results.length) { throw new HttpException({ error: 'Not allowed' }, 403); } @@ -348,6 +370,7 @@ export class JourneyController { const filePaths = files.map((f) => ({ path: `journey/${f.filename}` })); const photos = this.journey.uploadGalleryPhotos(Number(id), user.id, filePaths); if (!photos.length) { + await this.discardJourneyUploads(files); throw new HttpException({ error: 'Not allowed' }, 403); } // An uploaded file carries its own EXIF; reading it is what puts the photo on @@ -465,6 +488,7 @@ export class JourneyController { await this.commitJourneyUploads([file]); const result = this.journey.updateJourney(Number(id), user.id, { cover_image: `journey/${file.filename}` }); if (!result) { + await this.discardJourneyUploads([file]); throw new HttpException({ error: 'Journey not found' }, 404); } return result; @@ -679,6 +703,7 @@ export class JourneyController { share_timeline: body.share_timeline as boolean | undefined, share_gallery: body.share_gallery as boolean | undefined, share_map: body.share_map as boolean | undefined, + newest_first: body.newest_first as boolean | undefined, }); if (!result) { throw new HttpException({ error: 'Not allowed' }, 403); diff --git a/server/src/nest/journey/journey.mcp.ts b/server/src/nest/journey/journey.mcp.ts index 69e69eeec..fcb7d3470 100644 --- a/server/src/nest/journey/journey.mcp.ts +++ b/server/src/nest/journey/journey.mcp.ts @@ -408,28 +408,36 @@ export class JourneyMcp { @Tool({ name: 'get_journey_share_link', - description: 'Get the current public share link for a journey. Returns null if none exists.', + description: 'Get the current public share link for a journey. Owner only. Returns null if none exists.', inputSchema: { journeyId: z.number().int().positive() }, annotations: TOOL_ANNOTATIONS_READONLY, when: journeyAddonOn, access: { group: 'journey', mode: 'share' }, }) getJourneyShareLink({ journeyId }: { journeyId: number }, ctx: McpContext) { - if (!this.journey.canAccessJourney(journeyId, ctx.userId)) return notFound('Journey not found or access denied.'); + // Owner only, same as the REST route: handing out the token is handing out + // the journey. + if (!this.journey.isOwner(journeyId, ctx.userId)) return notFound('Journey not found or access denied.'); return ok({ shareLink: this.share.getJourneyShareLink(journeyId) }); } @Tool({ name: 'create_journey_share_link', - description: 'Create or update the public share link for a journey. Owner only.', - inputSchema: { journeyId: z.number().int().positive() }, + description: 'Create or update the public share link for a journey. Owner only. Flags left out keep their current value on an existing link; a new link defaults to timeline/gallery/map on.', + inputSchema: { + journeyId: z.number().int().positive(), + share_timeline: z.boolean().optional(), + share_gallery: z.boolean().optional(), + share_map: z.boolean().optional(), + newest_first: z.boolean().optional(), + }, annotations: TOOL_ANNOTATIONS_WRITE, when: journeyAddonOn, access: { group: 'journey', mode: 'share' }, }) - createJourneyShareLink({ journeyId }: { journeyId: number }, ctx: McpContext) { + createJourneyShareLink({ journeyId, ...permissions }: { journeyId: number; share_timeline?: boolean; share_gallery?: boolean; share_map?: boolean; newest_first?: boolean }, ctx: McpContext) { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); - const shareLink = this.share.createOrUpdateJourneyShareLink(journeyId, ctx.userId, {}); + const shareLink = this.share.createOrUpdateJourneyShareLink(journeyId, ctx.userId, permissions); if (!shareLink) return notFound('Journey not found or access denied.'); return ok({ shareLink }); } diff --git a/server/src/nest/journey/journey.service.ts b/server/src/nest/journey/journey.service.ts index bd21a24f7..ff13a8a8a 100644 --- a/server/src/nest/journey/journey.service.ts +++ b/server/src/nest/journey/journey.service.ts @@ -78,11 +78,11 @@ export class JourneyService { removeContributor(id: number, userId: number, targetUserId: number) { return this.journey.removeContributor(id, userId, targetUserId); } // Share links - // Authorization: only someone with access to the journey may read its public - // share token — same access model as create/delete here and the - // get_journey_share_link MCP tool. + // Authorization: the token is the whole credential, so reading it needs the + // same owner check create/delete use. A viewer who reads it keeps a working + // anonymous link after being removed as a contributor. getJourneyShareLink(id: number, userId: number) { - if (!this.journey.canAccessJourney(id, userId)) return null; + if (!this.journey.isOwner(id, userId)) return null; return this.share.getJourneyShareLink(id); } createOrUpdateJourneyShareLink(id: number, userId: number, data: Parameters[2]) { return this.share.createOrUpdateJourneyShareLink(id, userId, data); } diff --git a/server/src/nest/trips/trips.controller.ts b/server/src/nest/trips/trips.controller.ts index 36b6bd7c4..43197a89f 100644 --- a/server/src/nest/trips/trips.controller.ts +++ b/server/src/nest/trips/trips.controller.ts @@ -239,6 +239,13 @@ export class TripsController { if (!owner) { throw new HttpException({ error: 'Trip not found' }, 404); } + // Someone with no access at all gets the same 404 as a trip that does not + // exist, otherwise the 403 below turns sequential ids into an existence + // oracle. Admins are exempt: they may delete trips they are not a member of, + // which is what the isAdminDelete branch further down relies on. + if (user.role !== 'admin' && !this.trips.canAccessTrip(id, user.id)) { + throw new HttpException({ error: 'Trip not found' }, 404); + } if (!this.trips.can('trip_delete', user.role, owner.user_id, user.id, owner.user_id !== user.id)) { throw new HttpException({ error: 'No permission to delete this trip' }, 403); } diff --git a/server/tests/e2e/journey.e2e.test.ts b/server/tests/e2e/journey.e2e.test.ts index dacb7a6f0..a77f52927 100644 --- a/server/tests/e2e/journey.e2e.test.ts +++ b/server/tests/e2e/journey.e2e.test.ts @@ -317,6 +317,15 @@ describe('Journey e2e (real auth guard + temp SQLite)', () => { expect(res.body).toEqual({ id: 9 }); }); + it('public journey read still answers the addon gate when Journey is off', async () => { + // The authenticated surface goes dark with the addon; a published journey + // has to go with it, or turning the switch off does not take the feature away. + isAddonEnabled.mockReturnValue(false); + const res = await request(server).get('/api/public/journey/tok'); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Journey addon is not enabled' }); + }); + it('the image fileFilter is wired: a disallowed extension is rejected, not stored', async () => { // The multer options are built by MulterModule.registerAsync now, and Nest // injects MULTER_MODULE_OPTIONS with @Optional(): a token that fails to diff --git a/server/tests/integration/journey.test.ts b/server/tests/integration/journey.test.ts index 421b07751..0f0f1f7f1 100644 --- a/server/tests/integration/journey.test.ts +++ b/server/tests/integration/journey.test.ts @@ -417,6 +417,48 @@ describe('Journey share link', () => { expect(get.body.link.share_map).toBe(false); }); + it('JOURNEY-INT-044 — a contributor cannot read the token, only the owner can', async () => { + const { user: owner } = createUser(testDb); + const { user: helper } = createUser(testDb); + const journey = createJourney(testDb, owner.id); + addJourneyContributor(testDb, journey.id, helper.id, 'editor'); + await request(app) + .post(`/api/journeys/${journey.id}/share-link`) + .set('Cookie', authCookie(owner.id)) + .send({ share_timeline: true, share_gallery: true, share_map: true }); + + // The token is the whole credential and it outlives the contributor's + // access, so reading it takes the same owner check create and delete take. + const res = await request(app) + .get(`/api/journeys/${journey.id}/share-link`) + .set('Cookie', authCookie(helper.id)); + + expect(res.status).toBe(200); + expect(res.body.link).toBeNull(); + }); + + it('JOURNEY-INT-045 — a flag-only update leaves the other flags alone', async () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + await request(app) + .post(`/api/journeys/${journey.id}/share-link`) + .set('Cookie', authCookie(user.id)) + .send({ share_timeline: true, share_gallery: false, share_map: false, newest_first: true }); + + await request(app) + .post(`/api/journeys/${journey.id}/share-link`) + .set('Cookie', authCookie(user.id)) + .send({ share_timeline: false }); + + const get = await request(app) + .get(`/api/journeys/${journey.id}/share-link`) + .set('Cookie', authCookie(user.id)); + expect(get.body.link.share_timeline).toBe(false); + expect(get.body.link.share_gallery).toBe(false); + expect(get.body.link.share_map).toBe(false); + expect(get.body.link.newest_first).toBe(true); + }); + it('JOURNEY-INT-017 — DELETE /api/journeys/:id/share-link deletes the share link', async () => { const { user } = createUser(testDb); const journey = createJourney(testDb, user.id); @@ -1082,7 +1124,7 @@ describe('Journey upload parity', () => { expect(res.body.photos.length).toBeGreaterThan(0); }); - it('JOURNEY-P05 — non-contributor gallery upload is 403 and files remain on disk (no cleanup today)', async () => { + it('JOURNEY-P05 — non-contributor gallery upload is 403 and the bytes are taken back off disk', async () => { const { user: owner } = createUser(testDb); const { user: other } = createUser(testDb); const journey = createJourney(testDb, owner.id); @@ -1094,8 +1136,10 @@ describe('Journey upload parity', () => { .attach('photos', FIXTURE_IMG); expect(res.status).toBe(403); expect(res.body.error).toBe('Not allowed'); + // The commit runs before the permission answer is known, so the refusal has + // to delete the object again. Nothing sweeps orphans. const after = fsMod.existsSync(journeyDir) ? fsMod.readdirSync(journeyDir).length : 0; - expect(after).toBe(before + 1); + expect(after).toBe(before); }); it('JOURNEY-P06 — video+poster upload: poster is ALWAYS stored as .jpg (stored-XSS pin)', async () => { diff --git a/server/tests/integration/trips.test.ts b/server/tests/integration/trips.test.ts index b878a1eab..8a78b573c 100644 --- a/server/tests/integration/trips.test.ts +++ b/server/tests/integration/trips.test.ts @@ -527,7 +527,7 @@ describe('Delete trip', () => { expect(getRes.status).toBe(404); }); - it('TRIP-019 — Regular user cannot delete another users trip → 403', async () => { + it('TRIP-019 — Regular user cannot delete another users trip → 404', async () => { const { user: owner } = createUser(testDb); const { user: other } = createUser(testDb); const trip = createTrip(testDb, owner.id, { title: "Owner's Trip" }); @@ -536,8 +536,10 @@ describe('Delete trip', () => { .delete(`/api/trips/${trip.id}`) .set('Cookie', authCookie(other.id)); - // getTripOwner finds the trip (it exists); checkPermission fails for non-members → 403 - expect(res.status).toBe(403); + // 404, not 403: someone with no access at all must not be able to tell an + // existing trip from a missing one by walking sequential ids. A member who + // simply lacks trip_delete still gets 403 — see the case below. + expect(res.status).toBe(404); // Trip still exists const tripInDb = testDb.prepare('SELECT id FROM trips WHERE id = ?').get(trip.id); diff --git a/server/tests/unit/db/trip-access.test.ts b/server/tests/unit/db/trip-access.test.ts index a24cfc2e0..6a4567e90 100644 --- a/server/tests/unit/db/trip-access.test.ts +++ b/server/tests/unit/db/trip-access.test.ts @@ -7,6 +7,7 @@ */ import { describe, it, expect, beforeEach } from 'vitest'; import { db, canAccessTrip } from '../../../src/db/database'; +import { CAN_ACCESS_TRIP_SQL, buildDbMock, createTestDb } from '../../helpers/test-db'; function seedUser(username: string): number { return Number( @@ -52,3 +53,34 @@ describe('canAccessTrip', () => { expect(canAccessTrip(tripId, stranger)).toBeUndefined(); }); }); + +/** + * The mock every domain test runs against had the pre-#1543 SELECT, so the very + * regression the block above pins was invisible to the suites that mock the db. + */ +describe('the buildDbMock stand-in for canAccessTrip', () => { + it('hands back the trip currency, like the real one', () => { + const testDb = createTestDb(); + try { + const owner = Number( + testDb.prepare("INSERT INTO users (username, email, password_hash, role) VALUES ('m', 'm@example.test', 'x', 'user')") + .run().lastInsertRowid, + ); + const tripId = Number( + testDb.prepare("INSERT INTO trips (user_id, title, currency) VALUES (?, 'Trip', 'ISK')") + .run(owner).lastInsertRowid, + ); + + expect(buildDbMock(testDb).canAccessTrip(tripId, owner)).toMatchObject({ id: tripId, currency: 'ISK' }); + } finally { + testDb.close(); + } + }); + + it('selects the same columns the production statement does', () => { + const columns = (sql: string) => sql.replace(/\s+/g, ' ').trim(); + expect(columns(CAN_ACCESS_TRIP_SQL)).toBe( + 'SELECT t.id, t.user_id, t.currency FROM trips t LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? WHERE t.id = ? AND (t.user_id = ? OR m.user_id IS NOT NULL)', + ); + }); +}); diff --git a/server/tests/unit/mcp/tools-journey.test.ts b/server/tests/unit/mcp/tools-journey.test.ts index 06327ffff..eb913c2f7 100644 --- a/server/tests/unit/mcp/tools-journey.test.ts +++ b/server/tests/unit/mcp/tools-journey.test.ts @@ -441,6 +441,46 @@ describe('journey share-link tools', () => { }); }); + it('passes the share flags through, and an omitted flag keeps its value', async () => { + const { user } = createUser(testDb); + await withHarness(user.id, async (h) => { + const journey = await seedJourney(h); + await h.client.callTool({ + name: 'create_journey_share_link', + arguments: { journeyId: journey.id, share_gallery: false, newest_first: true }, + }); + // Calling it again without the flags must not re-publish the gallery. + await h.client.callTool({ name: 'create_journey_share_link', arguments: { journeyId: journey.id } }); + const read = parseToolResult(await h.client.callTool({ + name: 'get_journey_share_link', arguments: { journeyId: journey.id }, + })) as any; + expect(read.shareLink.share_gallery).toBe(false); + expect(read.shareLink.newest_first).toBe(true); + expect(read.shareLink.share_timeline).toBe(true); + }); + }); + + it('get_journey_share_link is owner-only, like create and delete', async () => { + const { user: owner } = createUser(testDb); + const { user: guest } = createUser(testDb); + let journeyId = 0; + await withHarness(owner.id, async (h) => { + const journey = await seedJourney(h); + journeyId = journey.id; + await h.client.callTool({ name: 'create_journey_share_link', arguments: { journeyId } }); + }); + testDb.prepare( + 'INSERT INTO journey_contributors (journey_id, user_id, role, added_at) VALUES (?, ?, ?, ?)', + ).run(journeyId, guest.id, 'viewer', Date.now()); + await withHarness(guest.id, async (h) => { + // The token is the whole credential: a contributor must not be able to + // read out a link that keeps working after they are removed. + expect((await h.client.callTool({ + name: 'get_journey_share_link', arguments: { journeyId }, + })).isError).toBe(true); + }); + }); + it('every share tool refuses a foreign journey', async () => { const { user } = createUser(testDb); const { journeyId } = foreignJourney(); diff --git a/server/tests/unit/nest/files.controller.test.ts b/server/tests/unit/nest/files.controller.test.ts index 426d8418c..2a3006180 100644 --- a/server/tests/unit/nest/files.controller.test.ts +++ b/server/tests/unit/nest/files.controller.test.ts @@ -171,12 +171,22 @@ describe('FilesController (parity with the legacy /api/trips/:tripId/files route it('DELETE /:id/link/:linkId removes the link; GET /:id/links lists', () => { const deleteFileLink = vi.fn(); - expect(fc(fsvc({ deleteFileLink } as Partial)).unlink(user, trip, '5', '9', '3')).toEqual({ success: true }); + expect(fc(fsvc({ getFileById: vi.fn().mockReturnValue({ id: 9 }), deleteFileLink } as Partial)).unlink(user, trip, '5', '9', '3')).toEqual({ success: true }); expect(deleteFileLink).toHaveBeenCalledWith('3', '9'); - const s = fsvc({ getFileLinks: vi.fn().mockReturnValue([{ id: 1 }]) } as Partial); + const s = fsvc({ getFileById: vi.fn().mockReturnValue({ id: 9 }), getFileLinks: vi.fn().mockReturnValue([{ id: 1 }]) } as Partial); expect(fc(s).links(user, trip, '5', '9')).toEqual({ links: [{ id: 1 }] }); }); + it('the link routes resolve the file against :tripId, so a foreign file is 404', () => { + const foreign = () => fsvc({ getFileById: vi.fn().mockReturnValue(undefined), deleteFileLink: vi.fn(), getFileLinks: vi.fn() } as Partial); + const unlinkSvc = foreign(); + expect(thrown(() => fc(unlinkSvc).unlink(user, trip, '5', '9', '3'))).toEqual({ status: 404, body: { error: 'File not found' } }); + expect(unlinkSvc.deleteFileLink).not.toHaveBeenCalled(); + const listSvc = foreign(); + expect(thrown(() => fc(listSvc).links(user, trip, '5', '9'))).toEqual({ status: 404, body: { error: 'File not found' } }); + expect(listSvc.getFileLinks).not.toHaveBeenCalled(); + }); + it('the trash + link routes all reject without file_delete / file_edit', async () => { const denied = () => fsvc({ can: vi.fn().mockReturnValue(false) }); await expect(fc(denied()).permanent(user, trip, '5', '9')).rejects.toMatchObject({ status: 403 }); diff --git a/server/tests/unit/nest/files.service.test.ts b/server/tests/unit/nest/files.service.test.ts index 69fdeb9b1..6fbcc027d 100644 --- a/server/tests/unit/nest/files.service.test.ts +++ b/server/tests/unit/nest/files.service.test.ts @@ -141,10 +141,14 @@ describe('files.constants', () => { }); it('FILE-SVC-003: blocklist, size caps and the default allowlist are pinned', () => { - expect(BLOCKED_EXTENSIONS).toHaveLength(25); + expect(BLOCKED_EXTENSIONS).toHaveLength(29); expect(BLOCKED_EXTENSIONS).toContain('.svg'); expect(BLOCKED_EXTENSIONS).toContain('.exe'); expect(BLOCKED_EXTENSIONS).toContain('.ps1'); + // The alternative spellings a browser still renders as a document. + for (const ext of ['.svgz', '.shtml', '.shtm', '.xht']) { + expect(BLOCKED_EXTENSIONS).toContain(ext); + } expect(MAX_FILE_SIZE).toBe(50 * 1024 * 1024); expect(MAX_VIDEO_SIZE).toBe(500 * 1024 * 1024); expect(DEFAULT_ALLOWED_EXTENSIONS).toBe('jpg,jpeg,png,gif,webp,heic,pdf,doc,docx,xls,xlsx,txt,csv,pkpass,pkpasses,md,markdown'); diff --git a/server/tests/unit/nest/journey-book.service.test.ts b/server/tests/unit/nest/journey-book.service.test.ts index 6c34dbce8..9fe725dec 100644 --- a/server/tests/unit/nest/journey-book.service.test.ts +++ b/server/tests/unit/nest/journey-book.service.test.ts @@ -1,7 +1,7 @@ /** * Storing TREK Studio books (#1973). * - * The half worth testing hard is concurrency. Everyone on a journey can edit + * The half worth testing hard is concurrency. Every editor on a journey edits * the same book, so "two saves arrive together" is the ordinary case rather * than an edge one, and the failure mode of getting it wrong is somebody's * afternoon disappearing with no error anywhere. @@ -103,6 +103,20 @@ describe('access', () => { expect(books.getBook(journey.id, helper.id)).not.toBeNull(); }); + it('lets a viewer read the book but not overwrite or delete it', () => { + const { user: owner } = createUser(testDb); + const { user: guest } = createUser(testDb); + const journey = createJourney(testDb, owner.id); + addJourneyContributor(testDb, journey.id, guest.id, 'viewer'); + books.saveBook(journey.id, owner.id, { title: 'Iceland', document: doc() }); + + expect(books.canOpen(journey.id, guest.id)).toBe(true); + expect(books.getBook(journey.id, guest.id)).not.toBeNull(); + expect(books.saveBook(journey.id, guest.id, { title: 'mine now', document: doc('two') })).toBeNull(); + expect(books.deleteBook(journey.id, guest.id)).toBeNull(); + expect(books.getBook(journey.id, owner.id)!.title).toBe('Iceland'); + }); + it('says nothing about a journey that does not exist', () => { const { user } = createUser(testDb); expect(books.canOpen(999_999, user.id)).toBe(false); diff --git a/server/tests/unit/nest/journey-share.service.test.ts b/server/tests/unit/nest/journey-share.service.test.ts index 005f946ef..6b5fee09e 100644 --- a/server/tests/unit/nest/journey-share.service.test.ts +++ b/server/tests/unit/nest/journey-share.service.test.ts @@ -148,6 +148,41 @@ describe('createOrUpdateJourneyShareLink', () => { expect(link!.share_map).toBe(false); }); + it('JOURNEY-SHARE-027: an update leaves out flags it was not given', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + + svc.createOrUpdateJourneyShareLink(journey.id, user.id, { + share_timeline: true, + share_gallery: false, + share_map: false, + newest_first: true, + }); + // A caller that only flips the timeline must not silently re-publish the + // gallery and map at the unchanged token. + svc.createOrUpdateJourneyShareLink(journey.id, user.id, { share_timeline: false }); + + const link = svc.getJourneyShareLink(journey.id); + expect(link!.share_timeline).toBe(false); + expect(link!.share_gallery).toBe(false); + expect(link!.share_map).toBe(false); + expect(link!.newest_first).toBe(true); + }); + + it('JOURNEY-SHARE-028: newest_first survives a flag-only update and is settable', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + + svc.createOrUpdateJourneyShareLink(journey.id, user.id, {}); + expect(svc.getJourneyShareLink(journey.id)!.newest_first).toBe(false); + + svc.createOrUpdateJourneyShareLink(journey.id, user.id, { newest_first: true }); + expect(svc.getJourneyShareLink(journey.id)!.newest_first).toBe(true); + + svc.createOrUpdateJourneyShareLink(journey.id, user.id, { share_gallery: false }); + expect(svc.getJourneyShareLink(journey.id)!.newest_first).toBe(true); + }); + it('JOURNEY-SHARE-004: different journeys get different tokens', () => { const { user } = createUser(testDb); const j1 = createJourney(testDb, user.id); @@ -323,6 +358,21 @@ describe('validateShareTokenForAsset', () => { expect(svc.validateShareTokenForAsset(token, 'immich-asset-999')).toBeNull(); }); + it('JOURNEY-SHARE-029: falls back to the journey owner when the photo has no owner_id', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const entry = createJourneyEntry(testDb, journey.id, user.id); + insertJourneyPhoto(entry.id, { assetId: 'immich-asset-orphan' }); + const { token } = svc.createOrUpdateJourneyShareLink(journey.id, user.id, {}); + + // Without the fallback the controller filled the gap from the :ownerId path + // segment, i.e. an anonymous caller picked whose provider credentials to try. + const result = svc.validateShareTokenForAsset(token, 'immich-asset-orphan'); + + expect(result).not.toBeNull(); + expect(result!.ownerId).toBe(user.id); + }); + 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); diff --git a/server/tests/unit/nest/journey.controller.test.ts b/server/tests/unit/nest/journey.controller.test.ts index 47e120170..3e8022711 100644 --- a/server/tests/unit/nest/journey.controller.test.ts +++ b/server/tests/unit/nest/journey.controller.test.ts @@ -6,6 +6,9 @@ import { JourneyController } from '../../../src/nest/journey/journey.controller' import { journeyThumbName } from '../../../src/nest/memories/thumbnail.service'; import type { PhotoCaptureBackfillService } from '../../../src/nest/memories/photo-capture-backfill.service'; import { JourneyPublicController } from '../../../src/nest/journey/journey-public.controller'; +import { AddonGuard } from '../../../src/nest/addons/addon.guard'; +import { REQUIRE_ADDON } from '../../../src/nest/addons/require-addon.decorator'; +import { ADDON_IDS } from '../../../src/addons'; import type { JourneyService } from '../../../src/nest/journey/journey.service'; import type { JourneyBookService } from '../../../src/nest/journey/journey-book.service'; import type { StorageService } from '../../../src/nest/storage/storage.service'; @@ -186,10 +189,48 @@ describe('JourneyController', () => { expect(thrown(() => ctl(svc({ updateJourneyPreferences: vi.fn().mockReturnValue(null) } as Partial)).preferences(user, '9', {}))).toEqual({ status: 403, body: { error: 'Not allowed' } }); expect(ctl(svc({ getJourneyShareLink: vi.fn().mockReturnValue({ token: 'abc' }) } as Partial)).getShareLink(user, '9')).toEqual({ link: { token: 'abc' } }); expect(thrown(() => ctl(svc({ createOrUpdateJourneyShareLink: vi.fn().mockReturnValue(null) } as Partial)).setShareLink(user, '9', {}))).toEqual({ status: 403, body: { error: 'Not allowed' } }); - expect(ctl(svc({ createOrUpdateJourneyShareLink: vi.fn().mockReturnValue({ token: 'abc' }) } as Partial)).setShareLink(user, '9', { share_timeline: true })).toEqual({ token: 'abc' }); + const setLink = vi.fn().mockReturnValue({ token: 'abc' }); + expect(ctl(svc({ createOrUpdateJourneyShareLink: setLink } as Partial)).setShareLink(user, '9', { share_timeline: true })).toEqual({ token: 'abc' }); + // newest_first is part of the shared contract and the public viewer reads + // it, so it has to reach the service instead of being dropped here (#1614). + expect(setLink).toHaveBeenCalledWith(9, 1, { share_timeline: true, share_gallery: undefined, share_map: undefined, newest_first: undefined }); + const withOrder = vi.fn().mockReturnValue({ token: 'abc' }); + ctl(svc({ createOrUpdateJourneyShareLink: withOrder } as Partial)).setShareLink(user, '9', { newest_first: true }); + expect(withOrder).toHaveBeenCalledWith(9, 1, { share_timeline: undefined, share_gallery: undefined, share_map: undefined, newest_first: true }); expect(thrown(() => ctl(svc({ deleteJourneyShareLink: vi.fn().mockReturnValue(false) } as Partial)).deleteShareLink(user, '9'))).toEqual({ status: 403, body: { error: 'Not allowed' } }); }); + it('takes the committed bytes back out when the upload is refused', async () => { + // The commit happens before the authorization result is known (the Immich + // mirror reads the final path), so every refusal after it has to delete the + // object again. Nothing sweeps orphans. + const file = { filename: 'a.jpg', originalname: 'a.jpg' } as Express.Multer.File; + await thrownAsync(() => ctl(svc({ addPhoto: vi.fn().mockReturnValue(null) } as Partial)).uploadEntryPhotos(user, '3', [file], {})); + expect(storageStub.delete).toHaveBeenCalledWith('journey', 'a.jpg'); + + vi.clearAllMocks(); + await thrownAsync(() => ctl(svc({ uploadGalleryPhotos: vi.fn().mockReturnValue([]) } as Partial)).uploadGalleryPhotos(user, '3', [{ filename: 'g.jpg' } as Express.Multer.File])); + expect(storageStub.delete).toHaveBeenCalledWith('journey', 'g.jpg'); + + vi.clearAllMocks(); + await thrownAsync(() => ctl(svc({ updateJourney: vi.fn().mockReturnValue(null) } as Partial)).cover(user, '9', { filename: 'c.jpg' } as Express.Multer.File)); + expect(storageStub.delete).toHaveBeenCalledWith('journey', 'c.jpg'); + }); + + it('drops the files of a partly refused entry batch, keeping the accepted ones', async () => { + // A 200 with fewer photos than files still leaves unreferenced objects behind. + const addPhoto = vi.fn().mockReturnValueOnce({ id: 5 }).mockReturnValueOnce(null); + const immichAutoUploadEnabled = vi.fn().mockReturnValue(false); + const files = [ + { filename: 'ok.jpg', originalname: 'ok.jpg' } as Express.Multer.File, + { filename: 'no.jpg', originalname: 'no.jpg' } as Express.Multer.File, + ]; + const res = await ctl(svc({ addPhoto, immichAutoUploadEnabled } as Partial)).uploadEntryPhotos(user, '3', files, {}); + expect(res).toEqual({ photos: [{ id: 5 }] }); + expect(storageStub.delete).toHaveBeenCalledWith('journey', 'no.jpg'); + expect(storageStub.delete).not.toHaveBeenCalledWith('journey', 'ok.jpg'); + }); + it('entry photo upload mirrors to Immich only when opted in', async () => { const addPhoto = vi.fn().mockReturnValue({ id: 5 }); const uploadToImmich = vi.fn().mockResolvedValue('immich-1'); @@ -376,6 +417,13 @@ describe('JourneyController', () => { }); describe('JourneyPublicController', () => { + it('is addon-gated like the authenticated surface', () => { + // Turning the Journey addon off is meant to take the feature away; without + // this, already-published journeys stayed publicly readable. + expect(Reflect.getMetadata(REQUIRE_ADDON, JourneyPublicController)).toEqual({ addonId: ADDON_IDS.JOURNEY, label: 'Journey' }); + expect((Reflect.getMetadata('__guards__', JourneyPublicController) ?? [])[0]).toBe(AddonGuard); + }); + it('GET /:token 404 / json', () => { expect(thrown(() => new JourneyPublicController(svc({ getPublicJourney: vi.fn().mockReturnValue(null) } as Partial), storageStub).get('tok'))).toEqual({ status: 404, body: { error: 'Not found' } }); expect(new JourneyPublicController(svc({ getPublicJourney: vi.fn().mockReturnValue({ id: 1 }) } as Partial), storageStub).get('tok')).toEqual({ id: 1 }); @@ -435,11 +483,14 @@ describe('JourneyPublicController', () => { expect(json).toHaveBeenCalledWith({ error: 'Provider not supported' }); }); - it('legacy photo proxy: falls back to the path ownerId when the token has none', async () => { + it('legacy photo proxy: ignores the path ownerId, taking the owner off the token', async () => { + // Whose provider credentials get tried is not something an anonymous caller + // may choose from the URL. The share service resolves it, falling back to + // the journey owner when the photo row carries no owner_id. const streamProviderAsset = vi.fn().mockResolvedValue(undefined); - const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 0 }), streamProviderAsset } as Partial); + const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 3 }), streamProviderAsset } as Partial); await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'immich', 'a1', '8', 'original', {} as Response); - expect(streamProviderAsset).toHaveBeenCalledWith({}, 'immich', { userId: 8, ownerId: 8, assetId: 'a1' }, 'original'); + expect(streamProviderAsset).toHaveBeenCalledWith({}, 'immich', { userId: 3, ownerId: 3, assetId: 'a1' }, 'original'); }); it('legacy photo proxy: local provider 404s when the object does not exist', async () => { diff --git a/server/tests/unit/nest/trips.controller.test.ts b/server/tests/unit/nest/trips.controller.test.ts index a26a8c935..1d2acfa98 100644 --- a/server/tests/unit/nest/trips.controller.test.ts +++ b/server/tests/unit/nest/trips.controller.test.ts @@ -280,6 +280,20 @@ describe('TripsController (parity with the legacy /api/trips route)', () => { expect(thrown(() => tc(s).remove(user, '9', req))).toEqual({ status: 403, body: { error: 'No permission to delete this trip' } }); }); + it('404s for someone with no access, so the 403 is not an existence oracle', () => { + const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 2 }), canAccessTrip: vi.fn().mockReturnValue(null), remove: vi.fn() } as Partial); + expect(thrown(() => tc(s).remove(user, '9', req))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(s.remove).not.toHaveBeenCalled(); + }); + + it('still lets an admin delete a trip they are not a member of', () => { + const admin = { id: 1, role: 'admin', email: 'a@example.test' } as User; + const remove = vi.fn().mockReturnValue({ tripId: 9, title: 'T', isAdminDelete: true, ownerEmail: 'owner@x.y' }); + const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 2 }), canAccessTrip: vi.fn().mockReturnValue(null), remove, broadcast: vi.fn() } as Partial); + expect(tc(s).remove(admin, '9', req)).toEqual({ success: true }); + expect(remove).toHaveBeenCalledWith('9', 1, 'admin'); + }); + it('deletes, audits and broadcasts', () => { const remove = vi.fn().mockReturnValue({ tripId: 9, title: 'T', isAdminDelete: false }); const broadcast = vi.fn(); const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 1 }), remove, broadcast } as Partial);