diff --git a/server/src/modules/book/book.controller.test.ts b/server/src/modules/book/book.controller.test.ts index 0512c6c41..678b214e8 100644 --- a/server/src/modules/book/book.controller.test.ts +++ b/server/src/modules/book/book.controller.test.ts @@ -992,6 +992,11 @@ describe('BookController', () => { } }); + it('requires mutation permissions for per-file rename and delete', () => { + expect(Reflect.getMetadata(PERMISSION_KEY, BookController.prototype.renameFile)).toBe(Permission.LibraryEditMetadata); + expect(Reflect.getMetadata(PERMISSION_KEY, BookController.prototype.deleteFile)).toBe(Permission.LibraryDeleteBooks); + }); + it('preserves valid surrogate pairs while stripping lone surrogates in download filenames', async () => { const { controller, bookService } = makeController(); const { reply, headers } = makeReply(); diff --git a/server/src/modules/book/book.controller.ts b/server/src/modules/book/book.controller.ts index 599358bba..74595df5e 100644 --- a/server/src/modules/book/book.controller.ts +++ b/server/src/modules/book/book.controller.ts @@ -484,12 +484,14 @@ export class BookController { @Patch('files/:fileId') @HttpCode(HttpStatus.NO_CONTENT) + @RequirePermission(Permission.LibraryEditMetadata) async renameFile(@Param('fileId', ParseIntPipe) fileId: number, @Body() dto: UpdateBookFileDto, @CurrentUser() user: RequestUser) { await this.bookService.renameFile(fileId, dto, user); } @Delete('files/:fileId') @HttpCode(HttpStatus.NO_CONTENT) + @RequirePermission(Permission.LibraryDeleteBooks) async deleteFile(@Param('fileId', ParseIntPipe) fileId: number, @CurrentUser() user: RequestUser) { await this.bookService.deleteFile(fileId, user); } diff --git a/server/src/modules/book/book.service.test.ts b/server/src/modules/book/book.service.test.ts index 3241d0126..38a6730b6 100644 --- a/server/src/modules/book/book.service.test.ts +++ b/server/src/modules/book/book.service.test.ts @@ -1,4 +1,4 @@ -import { BadRequestException, ConflictException, ForbiddenException, Logger, NotFoundException } from '@nestjs/common'; +import { BadRequestException, ConflictException, ForbiddenException, InternalServerErrorException, Logger, NotFoundException } from '@nestjs/common'; import type { MockedFunction } from 'vitest'; import { access, readdir, rm, stat, rename } from 'fs/promises'; @@ -5256,5 +5256,26 @@ describe('BookService', () => { expect(bookRepo.deleteBookFile).toHaveBeenCalledWith(fileId); expect(bookRepo.updateBookPrimaryFile).toHaveBeenCalledWith(10, 101); }); + + it('preserves the database row and primary file when disk deletion fails', async () => { + const { service, bookRepo } = makeService(); + const fileId = 100; + const file = { absolutePath: '/path/to/old.epub', bookId: 10, libraryId: 1 }; + + bookRepo.findFileById = vi.fn().mockResolvedValue(file); + bookRepo.deleteBookFile = vi.fn(); + bookRepo.findFilesForBook = vi.fn(); + bookRepo.findBookBase = vi.fn(); + bookRepo.updateBookPrimaryFile = vi.fn(); + vi.mocked(rm).mockRejectedValue(Object.assign(new Error('permission denied'), { code: 'EACCES' })); + + await expect(service.deleteFile(fileId, makeUser())).rejects.toBeInstanceOf(InternalServerErrorException); + + expect(rm).toHaveBeenCalledWith(file.absolutePath, { force: true }); + expect(bookRepo.findBookBase).not.toHaveBeenCalled(); + expect(bookRepo.deleteBookFile).not.toHaveBeenCalled(); + expect(bookRepo.findFilesForBook).not.toHaveBeenCalled(); + expect(bookRepo.updateBookPrimaryFile).not.toHaveBeenCalled(); + }); }); }); diff --git a/server/src/modules/book/book.service.ts b/server/src/modules/book/book.service.ts index 3772e77da..54d3efd4b 100644 --- a/server/src/modules/book/book.service.ts +++ b/server/src/modules/book/book.service.ts @@ -1487,13 +1487,9 @@ export class BookService { try { await rm(file.absolutePath, { force: true }); } catch { - this.logger.warn(`Failed to physically delete file at ${file.absolutePath}`); + throw new InternalServerErrorException('Failed to delete file from disk'); } - // the file watcher will eventually catch the unlink and clean up the database. - // however, to be responsive, we can manually clean up the database here too, - // but if the file is the last file, the scanner logic is better suited to mark the book missing. - // So we leave the DB cleanup to the file watcher, which is more robust. const book = await this.bookRepo.findBookBase(file.bookId); const wasPrimary = book?.primaryFileId === fileId; diff --git a/server/test/authorization-matrix.e2e-spec.ts b/server/test/authorization-matrix.e2e-spec.ts index b1db27aa2..603ec2ad7 100644 --- a/server/test/authorization-matrix.e2e-spec.ts +++ b/server/test/authorization-matrix.e2e-spec.ts @@ -1,5 +1,6 @@ import { randomUUID } from 'crypto'; import { readFileSync } from 'fs'; +import { access, chmod } from 'fs/promises'; import { dirname, join } from 'path'; import { fileURLToPath } from 'url'; @@ -91,6 +92,7 @@ interface Personas { koboRevoked: TestUserSession; bookDockUser: TestUserSession; uploadUser: TestUserSession; + downloadOnlyUser: TestUserSession; ownerUser: TestUserSession; otherUser: TestUserSession; targetSuperuser: TestUserSession; @@ -224,6 +226,7 @@ describe('Authorization matrix (e2e)', () => { koboRevoked: await createUserAndLogin(ctx, { permissions: [Permission.KoboSync] }), bookDockUser: await createUserAndLogin(ctx, { permissions: [Permission.BookDockAccess, Permission.LibraryUpload] }), uploadUser: await createUserAndLogin(ctx, { permissions: [Permission.LibraryUpload] }), + downloadOnlyUser: await createUserAndLogin(ctx, { permissions: [Permission.LibraryDownload] }), ownerUser: await createUserAndLogin(ctx), otherUser: await createUserAndLogin(ctx), targetSuperuser: await createUserAndLogin(ctx, { isSuperuser: true }), @@ -234,6 +237,7 @@ describe('Authorization matrix (e2e)', () => { grantLibraryAccess(ctx, personas.allPermsUser.userId, libraryB.libraryId, 'owner'), grantLibraryAccess(ctx, personas.metadataEditor.userId, libraryA.libraryId, 'viewer'), grantLibraryAccess(ctx, personas.bookDockUser.userId, libraryA.libraryId, 'viewer'), + grantLibraryAccess(ctx, personas.downloadOnlyUser.userId, libraryA.libraryId, 'viewer'), grantLibraryAccess(ctx, personas.ownerUser.userId, libraryA.libraryId, 'viewer'), grantLibraryAccess(ctx, personas.opdsOwner.userId, libraryA.libraryId, 'viewer'), grantLibraryAccess(ctx, personas.opdsIntruder.userId, libraryA.libraryId, 'viewer'), @@ -489,6 +493,56 @@ describe('Authorization matrix (e2e)', () => { }); describe('guard matrix - jwt/permission/library/default-password', () => { + it('blocks per-file mutations for a download-only viewer without changing the file record', async () => { + const renameResponse = await ctx.app.inject({ + method: 'PATCH', + url: `/api/v1/books/files/${bookA.bookFileId}`, + headers: authHeader(personas.downloadOnlyUser.accessToken), + payload: { filename: 'unauthorized-rename.epub' }, + }); + expectError(renameResponse, 403, 'Missing permission: library_edit_metadata'); + + const deleteResponse = await ctx.app.inject({ + method: 'DELETE', + url: `/api/v1/books/files/${bookA.bookFileId}`, + headers: authHeader(personas.downloadOnlyUser.accessToken), + }); + expectError(deleteResponse, 403, 'Missing permission: library_delete_books'); + + await expect(access(bookA.absolutePath)).resolves.toBeUndefined(); + const [fileRow] = await ctx.db + .select({ id: schema.bookFiles.id, absolutePath: schema.bookFiles.absolutePath }) + .from(schema.bookFiles) + .where(eq(schema.bookFiles.id, bookA.bookFileId)); + expect(fileRow).toEqual({ id: bookA.bookFileId, absolutePath: bookA.absolutePath }); + }); + + it('preserves the file record when physical deletion fails', async () => { + const failurePath = await createEpubFixture(libraryA.folderPath, `locked-${randomUUID()}/delete-failure.epub`); + await triggerAndWaitForLibraryScan(ctx, libraryA.libraryId); + const failureTarget = await locateBookByAbsolutePath(ctx, failurePath); + const lockedDirectory = dirname(failurePath); + + await chmod(lockedDirectory, 0o555); + try { + const response = await ctx.app.inject({ + method: 'DELETE', + url: `/api/v1/books/files/${failureTarget.bookFileId}`, + headers: authHeader(personas.allPermsUser.accessToken), + }); + expectError(response, 500, 'Failed to delete file from disk'); + + await expect(access(failurePath)).resolves.toBeUndefined(); + const [fileRow] = await ctx.db + .select({ id: schema.bookFiles.id, absolutePath: schema.bookFiles.absolutePath }) + .from(schema.bookFiles) + .where(eq(schema.bookFiles.id, failureTarget.bookFileId)); + expect(fileRow).toEqual({ id: failureTarget.bookFileId, absolutePath: failurePath }); + } finally { + await chmod(lockedDirectory, 0o755); + } + }); + it('keeps the dashboard scroller inventory in parity with the live application', () => { expect(routeInventory.routes).toHaveLength(routeInventory.totalRoutes); const inventoryLabels = routeInventory.routes diff --git a/server/test/e2e/authorization-matrix/route-inventory/manifest.json b/server/test/e2e/authorization-matrix/route-inventory/manifest.json index 4c7cc5e02..a1be280b4 100644 --- a/server/test/e2e/authorization-matrix/route-inventory/manifest.json +++ b/server/test/e2e/authorization-matrix/route-inventory/manifest.json @@ -1,10 +1,10 @@ { - "totalRoutes": 453, + "totalRoutes": 455, "byPermission": { "(none)": 168, "Permission.ManageMetadataConfig": 34, "Permission.EmailSend": 30, - "Permission.LibraryEditMetadata": 34, + "Permission.LibraryEditMetadata": 35, "Permission.BookDockAccess": 17, "Permission.ManageAppSettings": 39, "Permission.ManageLibraries": 17, @@ -17,7 +17,7 @@ "Permission.OpdsAccess": 4, "Permission.ViewAuditLog": 1, "Permission.ViewUserActivity": 6, - "Permission.LibraryDeleteBooks": 10, + "Permission.LibraryDeleteBooks": 11, "Permission.LibraryDownload": 1, "Permission.LibraryUpload": 2, "Permission.ManageBookDock": 3, diff --git a/server/test/e2e/authorization-matrix/route-inventory/routes-02.json b/server/test/e2e/authorization-matrix/route-inventory/routes-02.json index 049e9997d..fe15f2575 100644 --- a/server/test/e2e/authorization-matrix/route-inventory/routes-02.json +++ b/server/test/e2e/authorization-matrix/route-inventory/routes-02.json @@ -239,6 +239,30 @@ "allowDefault": false, "useGuards": [] }, + { + "file": "server/src/modules/book/book.controller.ts", + "className": "BookController", + "methodName": "renameFile", + "httpMethod": "PATCH", + "path": "/books/files/:fileId", + "permissions": ["Permission.LibraryEditMetadata"], + "libraryAccess": [], + "isPublic": false, + "allowDefault": false, + "useGuards": [] + }, + { + "file": "server/src/modules/book/book.controller.ts", + "className": "BookController", + "methodName": "deleteFile", + "httpMethod": "DELETE", + "path": "/books/files/:fileId", + "permissions": ["Permission.LibraryDeleteBooks"], + "libraryAccess": [], + "isPublic": false, + "allowDefault": false, + "useGuards": [] + }, { "file": "server/src/modules/book/book.controller.ts", "className": "BookController", diff --git a/server/test/e2e/authorization-matrix/route-inventory/uncovered-routes.json b/server/test/e2e/authorization-matrix/route-inventory/uncovered-routes.json index d5074dc57..fa60e920b 100644 --- a/server/test/e2e/authorization-matrix/route-inventory/uncovered-routes.json +++ b/server/test/e2e/authorization-matrix/route-inventory/uncovered-routes.json @@ -9,7 +9,6 @@ "DELETE /books/:id|:bookId/cover", "DELETE /books/:id|:bookId/reading-attempts/:attemptId", "DELETE /books/:id|:bookId/sessions/:sessionId", - "DELETE /books/files/:fileId", "DELETE /books/files/:fileId/progress", "DELETE /custom-icons/:slug", "DELETE /custom-metadata/fields/:id", @@ -160,7 +159,6 @@ "PATCH /books/:id|:bookId/personal-note", "PATCH /books/:id|:bookId/reading-attempts/:attemptId", "PATCH /books/:id|:bookId/status", - "PATCH /books/files/:fileId", "PATCH /custom-icons/:slug/svg", "PATCH /custom-metadata/fields/:id", "PATCH /custom-metadata/fields/reorder",