From ac1b2cedb44dc4db1a3bc7477eb8a648db234ae1 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Fri, 24 Apr 2026 08:25:38 -0600 Subject: [PATCH] fix: harden uploads and CBR archive handling Handle existing upload folder paths without creating duplicate books, tolerate partial RAR 1.5 archive reads, and clean up related production defaults and dashboard type safety. --- .env.example | 4 +- README.md | 4 +- client/src/components/AppSidebar.vue | 2 +- .../components/widgets/ReadingGoalWidget.vue | 16 +++-- scripts/fixtures/seed-dashboard-widgets.sh | 13 +++- .../src/modules/metadata/lib/cbz-metadata.ts | 12 +++- server/src/modules/metadata/lib/cover-cbr.ts | 23 +++++-- server/src/modules/reader/cbz/cbz.service.ts | 42 +++++++++---- .../upload/upload-processor.service.test.ts | 60 ++++++++++++++++++- .../upload/upload-processor.service.ts | 33 +++++++++- 10 files changed, 175 insertions(+), 34 deletions(-) diff --git a/.env.example b/.env.example index b1f6a988b..3c24d7495 100644 --- a/.env.example +++ b/.env.example @@ -1,5 +1,5 @@ -# Immutable image reference built by CI (use a sha-* tag or @sha256 digest) -APP_IMAGE=ghcr.io/your-org/bookorbit:sha- +# Docker image to run. Pin a sha-* tag or @sha256 digest for stricter deploys. +APP_IMAGE=ghcr.io/neonsolstice/bookorbit:latest # App publish port on host APP_PORT=3000 diff --git a/README.md b/README.md index 46b2806d7..82b7d9fba 100644 --- a/README.md +++ b/README.md @@ -228,13 +228,13 @@ Production uses the root compose file and env file: - Compose: `docker-compose.yml` - Env file: `.env` (from `.env.example`) -- Image source: immutable images built in CI (`.github/workflows/container-image.yml`) +- Image source: CI-published images (`latest`, sha tags, or digests) Quick start: ```bash cp .env.example .env -# edit .env and set APP_IMAGE, secrets, APP_URL, and BOOKS_HOST_PATH +# edit .env and set secrets, APP_URL, BOOKS_HOST_PATH, and optionally APP_IMAGE pnpm prod:up ``` diff --git a/client/src/components/AppSidebar.vue b/client/src/components/AppSidebar.vue index 7980934a1..b669b981c 100644 --- a/client/src/components/AppSidebar.vue +++ b/client/src/components/AppSidebar.vue @@ -187,7 +187,7 @@ onUnmounted(() => stopLibraryUploadListener()) - + data.value?.goalBooks != null && data.value.goalBooks > 0) +const goalBooks = computed(() => data.value?.goalBooks ?? null) +const hasGoal = computed(() => goalBooks.value != null && goalBooks.value > 0) const percentage = computed(() => { - if (!data.value || !hasGoal.value) return 0 - return Math.min(100, Math.round((data.value.completedBooks / data.value.goalBooks!) * 100)) + const goal = goalBooks.value + if (!data.value || goal == null || goal <= 0) return 0 + return Math.min(100, Math.round((data.value.completedBooks / goal) * 100)) }) const option = shallowRef({}) watchEffect(() => { - if (!data.value || !hasGoal.value) { + if (!data.value || !hasGoal.value || goalBooks.value == null) { option.value = {} return } + const goal = goalBooks.value + option.value = { tooltip: { show: false }, series: [ @@ -43,9 +47,9 @@ watchEffect(() => { label: { show: false }, labelLine: { show: false }, data: [ - { value: Math.min(data.value.completedBooks, data.value.goalBooks), name: 'Completed' }, + { value: Math.min(data.value.completedBooks, goal), name: 'Completed' }, { - value: Math.max(data.value.goalBooks - data.value.completedBooks, 0), + value: Math.max(goal - data.value.completedBooks, 0), name: 'Remaining', itemStyle: { color: readCssColor('--muted') }, }, diff --git a/scripts/fixtures/seed-dashboard-widgets.sh b/scripts/fixtures/seed-dashboard-widgets.sh index 0263f00f9..564b2347f 100755 --- a/scripts/fixtures/seed-dashboard-widgets.sh +++ b/scripts/fixtures/seed-dashboard-widgets.sh @@ -106,8 +106,17 @@ BEGIN ) ON CONFLICT (user_id, book_id) DO UPDATE SET status = 'read', - finished_at = COALESCE(user_book_status.finished_at, - date_trunc('year', current_date) + (i * 14 * interval '1 day')), + started_at = LEAST( + COALESCE(user_book_status.started_at, EXCLUDED.started_at), + COALESCE(user_book_status.finished_at, EXCLUDED.finished_at, EXCLUDED.started_at) + ), + finished_at = GREATEST( + COALESCE(user_book_status.finished_at, EXCLUDED.finished_at), + LEAST( + COALESCE(user_book_status.started_at, EXCLUDED.started_at), + COALESCE(user_book_status.finished_at, EXCLUDED.finished_at, EXCLUDED.started_at) + ) + ), updated_at = EXCLUDED.updated_at; END LOOP; RAISE NOTICE 'Year Projection: marked % books as read this year', i; diff --git a/server/src/modules/metadata/lib/cbz-metadata.ts b/server/src/modules/metadata/lib/cbz-metadata.ts index b0e6cc3d8..eac97dd72 100644 --- a/server/src/modules/metadata/lib/cbz-metadata.ts +++ b/server/src/modules/metadata/lib/cbz-metadata.ts @@ -1,7 +1,7 @@ import { readFile } from 'fs/promises'; import { XMLParser } from 'fast-xml-parser'; import { inflateRawSync } from 'zlib'; -import { createExtractorFromData } from 'node-unrar-js'; +import { createExtractorFromData, UnrarError } from 'node-unrar-js'; import { getSevenZip } from '../../../common/sevenzip'; import { cleanupSevenZipArtifacts, createSevenZipTempId, type SevenZipInstance } from './sevenzip-vfs'; @@ -365,8 +365,14 @@ export async function extractCbrMetadata(absolutePath: string): Promise h.name.toLowerCase() === 'comicinfo.xml' }); let xmlBuf: Buffer | undefined; - for (const file of files) { - if (!file.fileHeader.flags.directory && file.extraction) xmlBuf = Buffer.from(file.extraction); + try { + for (const file of files) { + if (!file.fileHeader.flags.directory && file.extraction) xmlBuf = Buffer.from(file.extraction); + } + } catch (err) { + // Some RAR 1.5 archives throw ERAR_BAD_DATA at the end-of-archive marker. + // If we already extracted the file we needed, use it. + if (!(err instanceof UnrarError)) throw err; } return xmlBuf ? parseComicInfoXml(xmlBuf) : null; diff --git a/server/src/modules/metadata/lib/cover-cbr.ts b/server/src/modules/metadata/lib/cover-cbr.ts index 7ea4afb1d..3ac3fd04f 100644 --- a/server/src/modules/metadata/lib/cover-cbr.ts +++ b/server/src/modules/metadata/lib/cover-cbr.ts @@ -1,5 +1,5 @@ import { readFile } from 'fs/promises'; -import { createExtractorFromData } from 'node-unrar-js'; +import { createExtractorFromData, UnrarError } from 'node-unrar-js'; import { isArchiveImageFile, isHiddenArchivePath } from './archive-image-utils'; function toArrayBuffer(buf: Buffer): ArrayBuffer { @@ -14,16 +14,27 @@ export async function extractCbrCover(absolutePath: string): Promise !h.flags.directory && isArchiveImageFile(h.name) && !isHiddenArchivePath(h.name)) - .sort((a, b) => a.name.localeCompare(b.name, undefined, { numeric: true, sensitivity: 'base' })); + const images: { name: string }[] = []; + try { + for (const h of fileHeaders) { + if (!h.flags.directory && isArchiveImageFile(h.name) && !isHiddenArchivePath(h.name)) { + images.push(h); + } + } + } catch (err) { + // Some RAR 1.5 archives throw ERAR_BAD_DATA at the end-of-archive marker + // instead of ERAR_END_ARCHIVE. Accept partial results if we have images. + if (!(err instanceof UnrarError) || images.length === 0) throw err; + } + images.sort((a, b) => a.name.localeCompare(b.name, undefined, { numeric: true, sensitivity: 'base' })); if (images.length === 0) return null; const firstName = images[0].name; - // Second pass: extract only that file. Must drain the generator fully to avoid WASM leak. + // Second pass: pass an array so the extractor stops after the first match + // and never hits the malformed end-of-archive marker in some RAR 1.5 files. const extractExtractor = await createExtractorFromData({ data: ab }); - const { files } = extractExtractor.extract({ files: (h) => h.name === firstName }); + const { files } = extractExtractor.extract({ files: [firstName] }); let result: Uint8Array | undefined; for (const file of files) { if (!file.fileHeader.flags.directory) result = file.extraction; diff --git a/server/src/modules/reader/cbz/cbz.service.ts b/server/src/modules/reader/cbz/cbz.service.ts index a2f5dc580..cb9980553 100644 --- a/server/src/modules/reader/cbz/cbz.service.ts +++ b/server/src/modules/reader/cbz/cbz.service.ts @@ -1,9 +1,9 @@ -import { Injectable, NotFoundException } from '@nestjs/common'; +import { Injectable, NotFoundException, UnprocessableEntityException } from '@nestjs/common'; import { Readable } from 'stream'; import { createReadStream } from 'fs'; import { readFile } from 'fs/promises'; import { createInflateRaw } from 'zlib'; -import { createExtractorFromData } from 'node-unrar-js'; +import { createExtractorFromData, UnrarError } from 'node-unrar-js'; import { getSevenZip } from '../../../common/sevenzip'; import { imageContentTypeFromPath } from '../../../common/image-content-type'; @@ -147,14 +147,27 @@ export class CbzService { if (!this.rarCache.has(fileId)) { const buf = await readFile(filePath); const ab = toArrayBuffer(buf); + const extractor = await createExtractorFromData({ data: ab }); - const { fileHeaders } = extractor.getFileList(); - const pages = [...fileHeaders] - .filter((h) => !h.flags.directory && isImage(h.name) && !isHidden(h.name)) - .map((h) => h.name) - .sort(naturalSort); + const pages: string[] = []; + try { + for (const h of fileHeaders) { + if (!h.flags.directory && isImage(h.name) && !isHidden(h.name)) { + pages.push(h.name); + } + } + } catch (err) { + // Some RAR 1.5 archives throw ERAR_BAD_DATA at the end-of-archive + // marker instead of returning ERAR_END_ARCHIVE. If we already have + // pages, the archive is readable — accept the partial list. + if (!(err instanceof UnrarError) || pages.length === 0) { + throw err instanceof UnrarError ? new UnprocessableEntityException(`CBR archive is unreadable: ${err.message}`) : err; + } + } + + pages.sort(naturalSort); this.rarCache.set(fileId, { buffer: ab, pages }); } return this.rarCache.get(fileId)!; @@ -162,12 +175,21 @@ export class CbzService { private async extractRarPage(buffer: ArrayBuffer, pageName: string): Promise { const extractor = await createExtractorFromData({ data: buffer }); - const { files } = extractor.extract({ files: (h) => h.name === pageName }); + // Pass an array so the extractor stops after the first match and never + // hits the malformed end-of-archive marker present in some RAR 1.5 files. + const { files } = extractor.extract({ files: [pageName] }); // Generators MUST be fully drained to avoid WASM memory leak let result: Uint8Array | undefined; - for (const file of files) { - if (!file.fileHeader.flags.directory) result = file.extraction; + try { + for (const file of files) { + if (!file.fileHeader.flags.directory) result = file.extraction; + } + } catch (err) { + if (err instanceof UnrarError) { + throw new UnprocessableEntityException(`CBR page is unreadable: ${err.message}`); + } + throw err; } if (result === undefined) throw new NotFoundException(`Page not found in RAR archive`); return result; diff --git a/server/src/modules/upload/upload-processor.service.test.ts b/server/src/modules/upload/upload-processor.service.test.ts index dd6af503c..cabd88c32 100644 --- a/server/src/modules/upload/upload-processor.service.test.ts +++ b/server/src/modules/upload/upload-processor.service.test.ts @@ -23,10 +23,16 @@ describe('UploadProcessorService', () => { const insertBookMetadataValues = vi.fn(); const insertBookFilesReturning = vi.fn(); const insertBookFilesValues = vi.fn(); + const insertBookFilesOnConflict = vi.fn(); const updateBooksSet = vi.fn(); const updateBooksWhere = vi.fn(); + const selectFrom = vi.fn(); + const selectWhere = vi.fn(); + const selectLimit = vi.fn(); + const tx = { + select: vi.fn(() => ({ from: selectFrom })), insert: vi.fn((table: unknown) => { if (table === books) { return { values: insertBooksValues }; @@ -54,10 +60,15 @@ describe('UploadProcessorService', () => { beforeEach(() => { vi.resetAllMocks(); + selectFrom.mockReturnValue({ where: selectWhere }); + selectWhere.mockReturnValue({ limit: selectLimit }); + selectLimit.mockResolvedValue([]); // no existing book by default + insertBooksValues.mockReturnValue({ returning: insertBooksReturning }); insertBooksReturning.mockResolvedValue([{ id: 42 }]); insertBookMetadataValues.mockResolvedValue(undefined); - insertBookFilesValues.mockReturnValue({ returning: insertBookFilesReturning }); + insertBookFilesOnConflict.mockReturnValue({ returning: insertBookFilesReturning }); + insertBookFilesValues.mockReturnValue({ returning: insertBookFilesReturning, onConflictDoUpdate: insertBookFilesOnConflict }); insertBookFilesReturning.mockResolvedValue([{ id: 420 }]); updateBooksSet.mockReturnValue({ where: updateBooksWhere }); updateBooksWhere.mockResolvedValue(undefined); @@ -94,6 +105,44 @@ describe('UploadProcessorService', () => { expect(orchestrator.scheduleIfEligible).toHaveBeenCalledWith(42, 1, 'event_import'); }); + it('adds a file to an existing book when the folder path already exists in the library', async () => { + selectLimit.mockResolvedValueOnce([{ id: 99 }]); + + const result = await service.createBookRecord(1, 2, '/folder', '/folder/book2.epub', 'book/book2.epub', 'epub', 5000); + + expect(result).toEqual({ bookId: 99 }); + expect(insertBooksValues).not.toHaveBeenCalled(); + expect(insertBookMetadataValues).not.toHaveBeenCalled(); + expect(updateBooksSet).not.toHaveBeenCalled(); + expect(insertBookFilesValues).toHaveBeenCalledWith( + expect.objectContaining({ + bookId: 99, + libraryFolderId: 2, + absolutePath: '/folder/book2.epub', + relPath: 'book/book2.epub', + format: 'epub', + role: 'content', + }), + ); + expect(insertBookFilesOnConflict).toHaveBeenCalledWith( + expect.objectContaining({ + target: bookFiles.absolutePath, + set: expect.objectContaining({ bookId: 99, libraryFolderId: 2, format: 'epub' }), + }), + ); + expect(orchestrator.scheduleIfEligible).toHaveBeenCalledWith(99, 1, 'event_import'); + }); + + it('refreshes a stale book_files record when the absolute path already exists in the db', async () => { + selectLimit.mockResolvedValueOnce([{ id: 99 }]); + insertBookFilesOnConflict.mockReturnValueOnce({ returning: vi.fn().mockResolvedValue([{ id: 777 }]) }); + + const result = await service.createBookRecord(1, 2, '/folder', '/folder/stale.epub', 'book/stale.epub', 'epub', 1000); + + expect(result).toEqual({ bookId: 99 }); + expect(insertBookFilesOnConflict).toHaveBeenCalled(); + }); + it('throws InternalServerErrorException when creating the book row fails', async () => { insertBooksReturning.mockResolvedValueOnce([]); @@ -110,6 +159,15 @@ describe('UploadProcessorService', () => { ); }); + it('throws InternalServerErrorException when adding a file to an existing book fails', async () => { + selectLimit.mockResolvedValueOnce([{ id: 99 }]); + insertBookFilesReturning.mockResolvedValueOnce([]); + + await expect(service.createBookRecord(1, 2, '/folder', '/folder/book2.epub', 'book/book2.epub', 'epub', 5000)).rejects.toBeInstanceOf( + InternalServerErrorException, + ); + }); + it('logs but suppresses scheduling failures', async () => { const warn = vi.spyOn((service as unknown as { logger: { warn: (m: string) => void } }).logger, 'warn').mockImplementation(); orchestrator.scheduleIfEligible.mockRejectedValue(new Error('queue offline')); diff --git a/server/src/modules/upload/upload-processor.service.ts b/server/src/modules/upload/upload-processor.service.ts index 741dc5fcb..24b3c4f3a 100644 --- a/server/src/modules/upload/upload-processor.service.ts +++ b/server/src/modules/upload/upload-processor.service.ts @@ -1,6 +1,6 @@ import { Inject, Injectable, InternalServerErrorException, Logger, Optional } from '@nestjs/common'; import { stat } from 'fs/promises'; -import { eq } from 'drizzle-orm'; +import { and, eq } from 'drizzle-orm'; import { NodePgDatabase } from 'drizzle-orm/node-postgres'; import { DB } from '../../db'; @@ -53,6 +53,37 @@ export class UploadProcessorService { const [fileStat, hash] = await Promise.all([stat(absolutePath), fingerprintFile(absolutePath)]); const { bookId } = await this.db.transaction(async (tx) => { + const [existingBook] = await tx + .select({ id: books.id }) + .from(books) + .where(and(eq(books.libraryId, libraryId), eq(books.folderPath, folderPath))) + .limit(1); + + if (existingBook) { + const fileValues = { + bookId: existingBook.id, + libraryFolderId, + absolutePath, + relPath, + ino: fileStat.ino, + sizeBytes, + mtime: fileStat.mtime, + hash, + format, + role: 'content' as const, + }; + const [file] = await tx + .insert(bookFiles) + .values(fileValues) + .onConflictDoUpdate({ + target: bookFiles.absolutePath, + set: { bookId: existingBook.id, libraryFolderId, relPath, ino: fileStat.ino, sizeBytes, mtime: fileStat.mtime, hash, format }, + }) + .returning({ id: bookFiles.id }); + if (!file) throw new InternalServerErrorException('Failed to create book file'); + return { bookId: existingBook.id }; + } + const [book] = await tx.insert(books).values({ libraryId, libraryFolderId, folderPath, status: 'present' }).returning({ id: books.id }); if (!book) throw new InternalServerErrorException('Failed to create book record');