From 6b6b662e79563c686cf838f374f509936e2e9231 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Thu, 22 Oct 2026 15:31:35 +0000 Subject: [PATCH] fix(books): deliver plain EPUBs that fail text-only inspection A text-only download of an EPUB with no narration returned 400 when the strict inspection tripped on a structural fault such as duplicate manifest ids, a missing resource or a chapter over 16 MiB. Serve such books as stored, and log why inspection failed or why a text-only delivery was refused. Closes #1888 --- .../book/delivery-artifact.service.test.ts | 38 ++++++++++++++++++- .../modules/book/delivery-artifact.service.ts | 33 +++++++++++----- .../book/epub-delivery-validation.test.ts | 15 +++++++- .../modules/book/epub-delivery-validation.ts | 17 ++++++++- 4 files changed, 88 insertions(+), 15 deletions(-) diff --git a/server/src/modules/book/delivery-artifact.service.test.ts b/server/src/modules/book/delivery-artifact.service.test.ts index 6b19c433e..d4f9d0182 100644 --- a/server/src/modules/book/delivery-artifact.service.test.ts +++ b/server/src/modules/book/delivery-artifact.service.test.ts @@ -7,6 +7,7 @@ import { mkdtemp, readFile, rm, writeFile, symlink, mkdir, stat, open } from 'fs import { tmpdir } from 'os'; import { join } from 'path'; import { createHash } from 'crypto'; +import { Logger } from '@nestjs/common'; import { Test } from '@nestjs/testing'; import { ZipArchive } from 'archiver'; import { createWriteStream } from 'fs'; @@ -16,12 +17,12 @@ import { FileLockService } from '../file-write/file-lock.service'; import { DeliveryArtifactRepository } from './delivery-artifact.repository'; import { DeliveryArtifactService } from './delivery-artifact.service'; -async function fixture(path: string, overlay: boolean, unsafe = false): Promise { +async function fixture(path: string, overlay: boolean, unsafe = false, extraManifest = ''): Promise { const archive = new ZipArchive(); archive.append('application/epub+zip', { name: 'mimetype', store: true }); archive.append('', { name: 'META-INF/container.xml' }); archive.append( - `${overlay ? '' : ''}`, + `${overlay ? '' : ''}${extraManifest}`, { name: 'OPS/book.opf' }, ); archive.append('Hello', { name: 'OPS/chapter.xhtml' }); @@ -119,6 +120,39 @@ describe('DeliveryArtifactService', () => { await artifact.release(); }); + it('delivers a plain EPUB that fails inspection as the original and logs why', async () => { + const path = join(directory, 'plain.epub'); + await fixture(path, false, false, ''); + const warn = vi.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); + const spy = vi.spyOn(stripper, 'writeArchive'); + const artifact = await service.prepare({ id: 12, absolutePath: path, format: 'epub', mediaOverlayAvailable: false }, 'audioless_epub'); + expect(artifact.path).toBe(path); + expect(artifact.outcome).toBe('verified_plain'); + expect(artifact.actualVariant).toBe('original'); + expect(spy).not.toHaveBeenCalled(); + expect(repository.saveInspection).toHaveBeenCalledWith(expect.objectContaining({ bookFileId: 12, state: 'verified_absent' })); + expect(warn).toHaveBeenCalledWith(expect.stringMatching(/^\[delivery\.inspect\] \[fail\] fileId=12 .*error="Ambiguous manifest IDs"/)); + await artifact.release(); + }); + + it('still refuses text-only for a narrated EPUB that fails inspection and logs why', async () => { + const path = join(directory, 'narrated.epub'); + await fixture(path, true, false, ''); + const warn = vi.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); + await expect(service.prepare({ id: 13, absolutePath: path, format: 'epub' }, 'audioless_epub')).rejects.toThrow('Ambiguous manifest IDs'); + expect(repository.saveInspection).toHaveBeenCalledWith(expect.objectContaining({ bookFileId: 13, state: 'failed' })); + expect(warn).toHaveBeenCalledWith(expect.stringMatching(/^\[delivery\.prepare\] \[fail\] fileId=13 .* - text-only delivery refused$/)); + }); + + it('trusts a recorded read-along over an archive without narration-named entries', async () => { + const path = join(directory, 'flagged.epub'); + await fixture(path, false, false, ''); + vi.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); + await expect(service.prepare({ id: 14, absolutePath: path, format: 'epub', mediaOverlayAvailable: true }, 'audioless_epub')).rejects.toThrow( + 'Ambiguous manifest IDs', + ); + }); + it('explicit text-only fails while inherited preparation preserves its original fallback', async () => { const path = join(directory, 'bad.epub'); await writeFile(path, 'broken'); diff --git a/server/src/modules/book/delivery-artifact.service.ts b/server/src/modules/book/delivery-artifact.service.ts index 316e2545c..82c2f70fa 100644 --- a/server/src/modules/book/delivery-artifact.service.ts +++ b/server/src/modules/book/delivery-artifact.service.ts @@ -12,7 +12,7 @@ import { sanitizeLogValue } from '../../common/utils/log-sanitize.utils'; import { AudiolessEpubService } from './audioless-epub.service'; import { FileLockService } from '../file-write/file-lock.service'; import { DeliveryArtifactRepository } from './delivery-artifact.repository'; -import { inspectDeliveryEpub } from './epub-delivery-validation'; +import { hasMultimediaEntries, inspectDeliveryEpub } from './epub-delivery-validation'; const DELIVERY_DETECTOR_VERSION = 'overlay-inspection-v3'; export const DELIVERY_TRANSFORM_VERSION = 'text-only-v3'; @@ -262,10 +262,13 @@ export class DeliveryArtifactService { await publish; return { ...metadata, path: join(directory, 'book.epub') }; } catch (error) { - if (requestedVariant !== null || error instanceof ConflictException) throw error; - this.logger.warn( - `[delivery.prepare] [fail] fileId=${source.id} durationMs=${Date.now() - startedAt} errorClass=${error instanceof Error ? error.name : 'Error'} error="${sanitizeLogValue(error instanceof Error ? error.message : String(error))}" - inherited delivery uses original fallback`, - ); + if (error instanceof ConflictException) throw error; + const failure = `fileId=${source.id} durationMs=${Date.now() - startedAt} errorClass=${error instanceof Error ? error.name : 'Error'} error="${sanitizeLogValue(error instanceof Error ? error.message : String(error))}"`; + if (requestedVariant !== null) { + this.logger.warn(`[delivery.prepare] [fail] ${failure} - text-only delivery refused`); + throw error; + } + this.logger.warn(`[delivery.prepare] [fail] ${failure} - inherited delivery uses original fallback`); return this.prepareOriginal(source, sourceRevision, requestedVariant, 'legacy_fallback'); } finally { await rm(temporary, { recursive: true, force: true }); @@ -279,12 +282,22 @@ export class DeliveryArtifactService { if (cached.state === 'verified_present') return true; if (cached.retryAt && cached.retryAt > new Date()) throw new BadRequestException({ code: 'DELIVERY_INSPECTION_FAILED' }); } + const startedAt = Date.now(); try { - const present = await inspectDeliveryEpub( - await unzipper.Open.file(source.absolutePath), - this.limits.maxSourceEntries, - this.limits.maxMetadataBytes, - ); + const zip = await unzipper.Open.file(source.absolutePath); + let present: boolean; + try { + present = await inspectDeliveryEpub(zip, this.limits.maxSourceEntries, this.limits.maxMetadataBytes); + } catch (error) { + // The strict checks guard the narration rewrite. A book with no narration is never rewritten, + // so a fault they trip on (duplicate ids, a missing resource, an oversized chapter) must not + // make it undeliverable: it goes out as stored, exactly as an original request would. + if (!(error instanceof BadRequestException) || source.mediaOverlayAvailable === true || hasMultimediaEntries(zip.files)) throw error; + this.logger.warn( + `[delivery.inspect] [fail] fileId=${source.id} durationMs=${Date.now() - startedAt} errorClass=${error.name} error="${sanitizeLogValue(error.message)}" - no narration present; delivering the original`, + ); + present = false; + } await this.assertSourceRevision(source, sourceRevision); await this.repository.saveInspection({ bookFileId: source.id, diff --git a/server/src/modules/book/epub-delivery-validation.test.ts b/server/src/modules/book/epub-delivery-validation.test.ts index 750d6ff3d..ae382094a 100644 --- a/server/src/modules/book/epub-delivery-validation.test.ts +++ b/server/src/modules/book/epub-delivery-validation.test.ts @@ -1,6 +1,6 @@ import { Readable } from 'stream'; import type * as unzipper from 'unzipper'; -import { inspectDeliveryEpub } from './epub-delivery-validation'; +import { hasMultimediaEntries, inspectDeliveryEpub } from './epub-delivery-validation'; function publication(opfAudioHref: string, smilAudioHref: string, prolog = '', textHref = 'chapter.xhtml#sentence'): unzipper.CentralDirectory { const content = new Map([ @@ -47,3 +47,16 @@ describe('delivery EPUB XML resource references', () => { await expect(inspectDeliveryEpub(zip, 100, 4096)).rejects.toThrow('Invalid overlay fragment'); }); }); + +describe('hasMultimediaEntries', () => { + const files = (...paths: string[]) => paths.map((path) => ({ path })) as unknown as unzipper.File[]; + + it('finds narration by entry name, including a percent-encoded one', () => { + expect(hasMultimediaEntries(files('OPS/chapter.xhtml', 'OPS/Audio/track.M4A'))).toBe(true); + expect(hasMultimediaEntries(files('OPS/chapter.xhtml', 'OPS/overlay%2Esmil'))).toBe(true); + }); + + it('reports a text-only archive, even with a malformed name, as plain', () => { + expect(hasMultimediaEntries(files('OPS/chapter.xhtml', 'OPS/cover.jpg', 'OPS/100%.xhtml'))).toBe(false); + }); +}); diff --git a/server/src/modules/book/epub-delivery-validation.ts b/server/src/modules/book/epub-delivery-validation.ts index 86e9380df..d5c6127f2 100644 --- a/server/src/modules/book/epub-delivery-validation.ts +++ b/server/src/modules/book/epub-delivery-validation.ts @@ -4,6 +4,7 @@ import { XMLParser, XMLValidator } from 'fast-xml-parser'; import type * as unzipper from 'unzipper'; const parser = new XMLParser({ ignoreAttributes: false, removeNSPrefix: true, htmlEntities: true }); +const MULTIMEDIA_ENTRY = /\.(?:smil|mp3|m4a|m4b|aac|ogg|opus|wav|flac|mp4|webm)$/i; const asArray = (value: T | T[] | undefined): T[] => (value === undefined ? [] : Array.isArray(value) ? value : [value]); const invalid = (reason: string): never => { throw new BadRequestException({ code: 'UNSUPPORTED_TEXT_ONLY_EPUB', message: reason }); @@ -63,6 +64,19 @@ export async function readEpubEntry(entry: unzipper.File, maxBytes: number): Pro } } +// Judged by entry names alone, so it still answers for a package too malformed to inspect. +export function hasMultimediaEntries(files: unzipper.File[]): boolean { + return files.some((file) => { + let decoded = file.path; + try { + decoded = decodeURIComponent(file.path); + } catch { + /* the raw name is still tested */ + } + return MULTIMEDIA_ENTRY.test(file.path) || MULTIMEDIA_ENTRY.test(decoded); + }); +} + export function validateEpubEntries(files: unzipper.File[], maxEntries: number): Map { if (files.length > maxEntries) return invalid('Too many EPUB entries'); const entries = new Map(); @@ -124,8 +138,7 @@ export async function inspectDeliveryEpub(zip: unzipper.CentralDirectory, maxEnt const audioPaths = new Set(items.filter((item) => /^audio\//i.test(item['@_media-type'] ?? '')).map((item) => resolve(item['@_href'], opfKey))); const declared = new Set(items.map((item) => resolve(item['@_href'], opfKey))); for (const key of entries.keys()) { - if (/\.(?:smil|mp3|m4a|m4b|aac|ogg|opus|wav|flac|mp4|webm)$/i.test(key) && !declared.has(key)) - return invalid('Undeclared multimedia is unsupported'); + if (MULTIMEDIA_ENTRY.test(key) && !declared.has(key)) return invalid('Undeclared multimedia is unsupported'); } const overlayAudio = new Set(); for (const item of overlayItems) {