fix(reader): CBX-aware reader + persistComic + hasReadableComic (bookshelf-b3s1) #285

Merged

zombor merged 3 commits from bd-bookshelf-b3s1 into main 2026-06-02 15:57:25 +00:00

Conversation 3 Commits 3 Files changed 23 +1160 -138

zombor commented 2026-06-02 15:23:51 +00:00

Owner

Copy link

Summary

Three layered bugs fixed — all caused by checking file extension instead of the production schema fields (book_type='CBX', archive_type='ZIP'|'RAR').

Bug #1 — hasReadableComic (internal/books/service.go):

• Renamed hasReadableCBZ → hasReadableComic; now checks book_type='CBX' (case-insensitive)
• Production rows always have book_type='CBX' per Grimmory V91 migration
• The Comic Info section on book detail pages now renders for ALL ingested comics

Bug #2 — Reader handler (page count + serving):

• Added CBR page count + page serving via rardecode/v2 (already a dep from dct5)
• Pages extracted once on first access, cached in process-level sync.Map keyed by archive path
• ServeComicPageHandler dispatches on archive_type: 'RAR' → rardecode, otherwise → ZIP
• ComicPageCounter likewise dispatches on archive_type
• GetPrimaryCBZFileRow and GetBookFileRow now include archive_type

Bug #3 — persistComic (internal/library/scan/persist.go):

• Trigger changed from file extension (.cbz) to book_type='CBX'
• Signature extended with archiveType parameter
• library/wire.go dispatches comic.ParseCBR vs comic.ParseCBZ based on archive_type
• comic.ParseCBR added: streams RAR archive without buffering full file into RAM

Architecture choice (caching strategy)

For RAR page serving, went with option (c): in-memory cache. On first page request for a given archive path, all image pages are extracted and cached in a process-level sync.Map. Subsequent requests serve from cache without reopening the archive. This is correct for the sequential-only rardecode/v2 API and avoids O(n²) behaviour for late-page reads.

Test plan

• internal/files/cbr_pages_test.go — CBRPageCount and ServeCBRPage: 2-page happy path, no-images, corrupt, corrupt-header (Next error), directory entry skipped, oversized entry (ErrCoverTooLarge), truncated body, cache behavior, open error, page out of range
• internal/books/has_readable_comic_test.go — table test: true for CBX/ZIP, true for CBX/RAR, true for lowercase cbx, false for cbz, false for epub, false for empty
• internal/comic/comicinfo_cbr_test.go — ParseCBR: happy path (parses series/issue/writer/characters), no ComicInfo.xml → ErrNoComicInfo, open failure, corrupt RAR, corrupt header (Next error), directory entry skipped, size cap via internal test
• internal/comic/comicinfo_internal_test.go — TestParseCBR_OversizedComicInfo covers the size-cap branch
• internal/library/scan/persist_test.go — updated CBX trigger test + captures archiveType passed to callback
• internal/books/reader_handler_test.go — ServeComicPageHandler: ZIP happy path, RAR dispatch, RAR out-of-range, RAR generic error, path traversal, all ID validation paths
• internal/books/reader_service_test.go — ComicPageCounter: ZIP dispatch, RAR dispatch, library path error, zip open error, count error, traversal guard
• 100% coverage on internal/
• make test green
• make lint clean

Closes bead bookshelf-b3s1 on merge.

## Summary Three layered bugs fixed — all caused by checking file extension instead of the production schema fields (`book_type='CBX'`, `archive_type='ZIP'|'RAR'`). **Bug #1 — hasReadableComic** (`internal/books/service.go`): - Renamed `hasReadableCBZ` → `hasReadableComic`; now checks `book_type='CBX'` (case-insensitive) - Production rows always have `book_type='CBX'` per Grimmory V91 migration - The Comic Info section on book detail pages now renders for ALL ingested comics **Bug #2 — Reader handler** (page count + serving): - Added CBR page count + page serving via `rardecode/v2` (already a dep from dct5) - Pages extracted once on first access, cached in process-level `sync.Map` keyed by archive path - `ServeComicPageHandler` dispatches on `archive_type`: `'RAR'` → rardecode, otherwise → ZIP - `ComicPageCounter` likewise dispatches on `archive_type` - `GetPrimaryCBZFileRow` and `GetBookFileRow` now include `archive_type` **Bug #3 — persistComic** (`internal/library/scan/persist.go`): - Trigger changed from file extension (`.cbz`) to `book_type='CBX'` - Signature extended with `archiveType` parameter - `library/wire.go` dispatches `comic.ParseCBR` vs `comic.ParseCBZ` based on `archive_type` - `comic.ParseCBR` added: streams RAR archive without buffering full file into RAM ## Architecture choice (caching strategy) For RAR page serving, went with **option (c): in-memory cache**. On first page request for a given archive path, all image pages are extracted and cached in a process-level `sync.Map`. Subsequent requests serve from cache without reopening the archive. This is correct for the sequential-only rardecode/v2 API and avoids O(n²) behaviour for late-page reads. ## Test plan - [x] `internal/files/cbr_pages_test.go` — CBRPageCount and ServeCBRPage: 2-page happy path, no-images, corrupt, corrupt-header (Next error), directory entry skipped, oversized entry (ErrCoverTooLarge), truncated body, cache behavior, open error, page out of range - [x] `internal/books/has_readable_comic_test.go` — table test: true for CBX/ZIP, true for CBX/RAR, true for lowercase cbx, false for cbz, false for epub, false for empty - [x] `internal/comic/comicinfo_cbr_test.go` — ParseCBR: happy path (parses series/issue/writer/characters), no ComicInfo.xml → ErrNoComicInfo, open failure, corrupt RAR, corrupt header (Next error), directory entry skipped, size cap via internal test - [x] `internal/comic/comicinfo_internal_test.go` — TestParseCBR_OversizedComicInfo covers the size-cap branch - [x] `internal/library/scan/persist_test.go` — updated CBX trigger test + captures archiveType passed to callback - [x] `internal/books/reader_handler_test.go` — ServeComicPageHandler: ZIP happy path, RAR dispatch, RAR out-of-range, RAR generic error, path traversal, all ID validation paths - [x] `internal/books/reader_service_test.go` — ComicPageCounter: ZIP dispatch, RAR dispatch, library path error, zip open error, count error, traversal guard - [x] 100% coverage on `internal/` - [x] `make test` green - [x] `make lint` clean Closes bead bookshelf-b3s1 on merge.

zombor added 1 commit 2026-06-02 15:23:51 +00:00

fix(reader): CBX-aware reader + persistComic + hasReadableComic (bookshelf-b3s1) ... c16b44c8b5

Three layered bugs fixed — all caused by checking file extension instead of
the production schema fields (book_type='CBX', archive_type='ZIP'|'RAR').

Bug #1 — hasReadableComic (internal/books/service.go):
Renamed hasReadableCBZ → hasReadableComic; now checks book_type='CBX'
(case-insensitive). Production rows always have book_type='CBX' per the
Grimmory V91 migration. The Comic Info section on the book detail page
will now render for all ingested comics.

Bug #2 — Reader handler (internal/books/reader_handler.go):
- Added CBR page count + page serving via rardecode/v2 (already a dep).
- Pages are extracted once on first access and cached in a process-level
  sync.Map keyed by archive path (option c: in-memory cache).
- ServeComicPageHandler dispatches on archive_type: 'RAR' → rardecode,
  otherwise → zip.OpenReader (ZIP / legacy rows).
- ComicPageCounter likewise dispatches on archive_type.
- GetPrimaryCBZFileRow and GetBookFileRow now include archive_type.

Bug #3 — persistComic (internal/library/scan/persist.go):
- Trigger changed from file extension (.cbz) to book_type='CBX'.
- Signature extended with archiveType parameter.
- library/wire.go comicPersistCallback dispatches comic.ParseCBR vs
  comic.ParseCBZ based on archive_type.
- comic.ParseCBR added to internal/comic/comicinfo.go: streams the RAR
  archive sequentially without buffering the full file into RAM.

Tests: 100% coverage on internal/. New test files:
- internal/files/cbr_pages_test.go
- internal/books/has_readable_comic_test.go
- internal/comic/comicinfo_cbr_test.go + testdata/comic-with-info.cbr

zombor added 1 commit 2026-06-02 15:32:50 +00:00

test(b3s1): replace internal-test additions with export_test.go bridge ... 52390462e6

Move TestParseCBR_OversizedComicInfo out of the package-internal test file
and into the external comic_test package via a new export_test.go bridge
(MaxComicInfoBytes = &maxComicInfoBytes). Rewrite as a Ginkgo spec in
comicinfo_cbr_test.go. Remove the os/fstest imports that were only needed
by the moved test.

zombor commented 2026-06-02 15:43:21 +00:00

Author

Owner

Copy link

Security Review — PR #285 (bd-bookshelf-b3s1, HEAD 52390462)

Reviewed: internal/files/cbr_pages.go, internal/comic/comicinfo.go, internal/books/reader_handler.go, internal/books/reader_service.go, internal/db/sqlc/books_extra.go, internal/library/scan/persist.go

---

Findings

[MAJOR] internal/files/cbr_pages.go:60 — No entry-count cap when iterating RAR archive for images

loadCBRPages iterates every entry in the archive looking for image files. A crafted RAR with millions of tiny non-image entries (e.g. text files, each well below maxCBRPageBytes) will loop indefinitely, holding an open file descriptor and consuming CPU per-entry CRC validation for the entire duration of the request. There is no bail-out after N entries. The analogous concern was flagged in the dct5 review for ParseCBR (comicinfo scanning). Both paths need the same fix: a maxEntriesToScan constant (e.g. const maxCBREntriesToScan = 5000) and a counter that returns an error or ErrNoCBRImages when exceeded.

Suggested fix:

const maxCBREntriesToScan = 5000
var scanned int
for {
    scanned++
    if scanned > maxCBREntriesToScan {
        return nil, fmt.Errorf("cbr pages: too many entries in archive")
    }
    h, nextErr := r.Next()
    ...
}

The same fix should be applied to ParseCBR in internal/comic/comicinfo.go:150.

---

[MAJOR] internal/files/cbr_pages.go:82 — RAR entry filename leaked via http.ServeContent name parameter

ServeCBRPage calls http.ServeContent(w, req, p.name, ...) where p.name is the raw RAR entry filename (e.g. ../../../../etc/passwd or a crafted string). While http.ServeContent does not set Content-Disposition directly, in Go ≥ 1.20 it uses name to set a Content-Disposition: inline; filename=... header when the MIME type could be a download or when a redirect is triggered. More concretely: serveContent calls setLastModified and may call http.Redirect with the name embedded in the URL path component — if the client sends a request for ? the redirect can include the raw name. This is a latent XSS/header-injection vector if the name contains special characters.

The CBZ path (internal/files/cbz.go:161) has the same pattern.

Suggested fix: pass a synthetic name (e.g. fmt.Sprintf("page%04d%s", n, ext)) derived from the index and extension, not the raw entry name.

---

[MINOR] internal/comic/comicinfo.go:170 — ParseCBR: no entry-count cap for ComicInfo.xml scanning

ParseCBR iterates all RAR entries looking for ComicInfo.xml. A crafted archive with 100,000 non-matching entries (all below maxComicInfoBytes) causes O(N) CPU work during library scan for every CBR file. Low severity because this runs once per book at scan time (not per HTTP request), but the same maxEntriesToScan constant from the MAJOR above should be applied here for consistency and defence in depth.

---

Confirmed Clean

• Decompression bomb (page images): h.UnPackedSize > maxCBRPageBytes guard at line 75 + io.LimitReader defence-in-depth at line 78. Matches the dct5 cover cap. ✓
• ParseCBR streaming: rardecode.NewReader(f) receives the fs.File directly — no io.ReadAll of the archive body. ✓
• maxComicInfoBytes cap: applied before decodeComicInfoReader at line 172, also enforced via LimitReader inside decodeComicInfoReader. ✓
• Cap test fires: comicinfo_cbr_test.go:167 lowers cap to 1 byte via MaxComicInfoBytes bridge and asserts error. ✓
• Path traversal (HTTP): Both ServeComicPageHandler and ComicPageCounter perform the filepath.Clean + HasPrefix guard before constructing the full path. ✓
• Page number is integer: URL path value is parsed by parseID (integer parse), not passed as a filename to the RAR reader. ✓
• SQL injection: GetBookFile and GetPrimaryCBZFile use ? parameterized queries. ✓
• rardecode CRC: rardecode.NewReader uses default options; CRC validation is enabled by default in rardecode v2. ✓
• DoS via sequential RAR re-reads: cbrPageCache (sync.Map) caches all pages on first load; subsequent page requests for the same archive are served from memory with no re-open. ✓

---

REVIEW VERDICT: 0 blocker, 2 major, 1 minor

## Security Review — PR #285 (bd-bookshelf-b3s1, HEAD 52390462) Reviewed: `internal/files/cbr_pages.go`, `internal/comic/comicinfo.go`, `internal/books/reader_handler.go`, `internal/books/reader_service.go`, `internal/db/sqlc/books_extra.go`, `internal/library/scan/persist.go` --- ### Findings **[MAJOR] internal/files/cbr_pages.go:60 — No entry-count cap when iterating RAR archive for images** `loadCBRPages` iterates every entry in the archive looking for image files. A crafted RAR with millions of tiny non-image entries (e.g. text files, each well below `maxCBRPageBytes`) will loop indefinitely, holding an open file descriptor and consuming CPU per-entry CRC validation for the entire duration of the request. There is no bail-out after N entries. The analogous concern was flagged in the dct5 review for `ParseCBR` (comicinfo scanning). Both paths need the same fix: a `maxEntriesToScan` constant (e.g. `const maxCBREntriesToScan = 5000`) and a counter that returns an error or `ErrNoCBRImages` when exceeded. Suggested fix: ```go const maxCBREntriesToScan = 5000 var scanned int for { scanned++ if scanned > maxCBREntriesToScan { return nil, fmt.Errorf("cbr pages: too many entries in archive") } h, nextErr := r.Next() ... } ``` The same fix should be applied to `ParseCBR` in `internal/comic/comicinfo.go:150`. --- **[MAJOR] internal/files/cbr_pages.go:82 — RAR entry filename leaked via `http.ServeContent` name parameter** `ServeCBRPage` calls `http.ServeContent(w, req, p.name, ...)` where `p.name` is the raw RAR entry filename (e.g. `../../../../etc/passwd` or a crafted string). While `http.ServeContent` does not set `Content-Disposition` directly, in Go ≥ 1.20 it uses `name` to set a `Content-Disposition: inline; filename=...` header when the MIME type could be a download or when a redirect is triggered. More concretely: `serveContent` calls `setLastModified` and may call `http.Redirect` with the name embedded in the URL path component — if the client sends a request for `?` the redirect can include the raw `name`. This is a latent XSS/header-injection vector if the name contains special characters. The CBZ path (`internal/files/cbz.go:161`) has the same pattern. Suggested fix: pass a synthetic name (e.g. `fmt.Sprintf("page%04d%s", n, ext)`) derived from the index and extension, not the raw entry name. --- **[MINOR] internal/comic/comicinfo.go:170 — ParseCBR: no entry-count cap for ComicInfo.xml scanning** `ParseCBR` iterates all RAR entries looking for `ComicInfo.xml`. A crafted archive with 100,000 non-matching entries (all below `maxComicInfoBytes`) causes O(N) CPU work during library scan for every CBR file. Low severity because this runs once per book at scan time (not per HTTP request), but the same `maxEntriesToScan` constant from the MAJOR above should be applied here for consistency and defence in depth. --- ### Confirmed Clean - **Decompression bomb (page images):** `h.UnPackedSize > maxCBRPageBytes` guard at line 75 + `io.LimitReader` defence-in-depth at line 78. Matches the dct5 cover cap. ✓ - **ParseCBR streaming:** `rardecode.NewReader(f)` receives the `fs.File` directly — no `io.ReadAll` of the archive body. ✓ - **maxComicInfoBytes cap:** applied before `decodeComicInfoReader` at line 172, also enforced via `LimitReader` inside `decodeComicInfoReader`. ✓ - **Cap test fires:** `comicinfo_cbr_test.go:167` lowers cap to 1 byte via `MaxComicInfoBytes` bridge and asserts error. ✓ - **Path traversal (HTTP):** Both `ServeComicPageHandler` and `ComicPageCounter` perform the `filepath.Clean` + `HasPrefix` guard before constructing the full path. ✓ - **Page number is integer:** URL path value is parsed by `parseID` (integer parse), not passed as a filename to the RAR reader. ✓ - **SQL injection:** `GetBookFile` and `GetPrimaryCBZFile` use `?` parameterized queries. ✓ - **rardecode CRC:** `rardecode.NewReader` uses default options; CRC validation is enabled by default in rardecode v2. ✓ - **DoS via sequential RAR re-reads:** `cbrPageCache` (`sync.Map`) caches all pages on first load; subsequent page requests for the same archive are served from memory with no re-open. ✓ --- REVIEW VERDICT: 0 blocker, 2 major, 1 minor

zombor commented 2026-06-02 15:44:35 +00:00

Author

Owner

Copy link

CODE REVIEW: NOT APPROVED

Phase 0: DEMO Verification

No DEMO block found in the bead description or PR body. Per review standard: NOT APPROVED — "No DEMO block provided". This is a bug-fix PR so a DEMO showing the fixed behavior is mandatory (e.g. a curl against a CBR book's reader endpoint, or the make test output showing the new CBX-aware test cases passing). Without it, there is no verification the fixes actually apply to the production scenario.

---

Phase 1: Spec Compliance — PASSES (all three bugs addressed)

• Bug #1 (hasReadableComic): strings.EqualFold(f.BookType, "cbx") correct. Handler call site (handler.go:560) updated. hasReadable is also updated to check CBX. Export bridge in export_test.go is re-export only.
• Bug #2 (reader dispatch): ComicPageCounter and ServeComicPageHandler both dispatch on archive_type; GetPrimaryCBZFileRow and GetBookFileRow now include archive_type; wiring in books/wire.go is complete.
• Bug #3 (persistComic): trigger changed to strings.EqualFold(nb.sr.BookType, "CBX"); archiveType parameter threaded through; library/wire.go dispatches ParseCBR vs ParseCBZ.

RAR page-serving strategy (full extraction to process-level sync.Map cache) is documented. ParseCBR passes f (an io.Reader) directly to rardecode.NewReader — no full-archive buffer. maxComicInfoBytes cap is present. export_test.go bridges contain only var/const re-exports, no test functions.

---

Phase 2: Code Quality

[BLOCKER] internal/comic/comicinfo.go:171 — uint64 cast of int64 UnPackedSize silently rejects valid ComicInfo.xml in unknown-size RAR entries

rardecode/v2's FileHeader.UnPackedSize is int64. Per the library source (archive15.go:283–285), when UnKnownSize == true (solid archives, streaming RAR4), UnPackedSize is set to -1. The code does:

unpackedSize := uint64(h.UnPackedSize) //nolint:gosec
if unpackedSize > maxComicInfoBytes {
    return Metadata{}, fmt.Errorf("comic parse cbr: ComicInfo.xml too large (%d bytes)", unpackedSize)
}

uint64(-1) is 18446744073709551615 — far above the 1 MB cap — so any RAR archive whose ComicInfo.xml entry has UnKnownSize=true returns an error even when the file is tiny and legitimate. In contrast cbr_pages.go:75 correctly compares h.UnPackedSize > maxCBRPageBytes using int64 arithmetic (where -1 is less than 50 MB and falls through safely). The fix: check h.UnKnownSize first and skip the size pre-check when the size is unknown; rely solely on decodeComicInfoReader's LimitReader cap, which already handles size=0 correctly by defaulting to maxComicInfoBytes.

[MINOR] internal/books/service.go:862 — stale strings.EqualFold(f.ArchiveType, "cbz") branch in hasReadable (pre-existing, not introduced by this PR)

archive_type in the database is 'ZIP' or 'RAR' (confirmed in internal/library/scan/booktype.go); 'cbz' can never match. This branch is dead and misleading. It was not introduced by this PR (present in main) so does not block merge, but should be filed as a follow-up.

---

REVIEW VERDICT: 1 blocker, 0 major, 1 minor

The blocker (uint64(-1) cast in ParseCBR) will cause comic.ParseCBR to silently reject valid ComicInfo.xml entries in solid/streaming RAR4 archives. Fix: guard with if !h.UnKnownSize before the size pre-check, or compare as int64 (matching cbr_pages.go's pattern).

## CODE REVIEW: NOT APPROVED ### Phase 0: DEMO Verification No DEMO block found in the bead description or PR body. Per review standard: **NOT APPROVED — "No DEMO block provided"**. This is a bug-fix PR so a DEMO showing the fixed behavior is mandatory (e.g. a curl against a CBR book's reader endpoint, or the `make test` output showing the new CBX-aware test cases passing). Without it, there is no verification the fixes actually apply to the production scenario. --- ### Phase 1: Spec Compliance — PASSES (all three bugs addressed) - Bug #1 (`hasReadableComic`): `strings.EqualFold(f.BookType, "cbx")` correct. Handler call site (`handler.go:560`) updated. `hasReadable` is also updated to check `CBX`. Export bridge in `export_test.go` is re-export only. - Bug #2 (reader dispatch): `ComicPageCounter` and `ServeComicPageHandler` both dispatch on `archive_type`; `GetPrimaryCBZFileRow` and `GetBookFileRow` now include `archive_type`; wiring in `books/wire.go` is complete. - Bug #3 (`persistComic`): trigger changed to `strings.EqualFold(nb.sr.BookType, "CBX")`; `archiveType` parameter threaded through; `library/wire.go` dispatches `ParseCBR` vs `ParseCBZ`. RAR page-serving strategy (full extraction to process-level `sync.Map` cache) is documented. `ParseCBR` passes `f` (an `io.Reader`) directly to `rardecode.NewReader` — no full-archive buffer. `maxComicInfoBytes` cap is present. `export_test.go` bridges contain only `var`/`const` re-exports, no test functions. --- ### Phase 2: Code Quality **[BLOCKER] internal/comic/comicinfo.go:171 — uint64 cast of int64 UnPackedSize silently rejects valid ComicInfo.xml in unknown-size RAR entries** `rardecode/v2`'s `FileHeader.UnPackedSize` is `int64`. Per the library source (`archive15.go:283–285`), when `UnKnownSize == true` (solid archives, streaming RAR4), `UnPackedSize` is set to `-1`. The code does: ```go unpackedSize := uint64(h.UnPackedSize) //nolint:gosec if unpackedSize > maxComicInfoBytes { return Metadata{}, fmt.Errorf("comic parse cbr: ComicInfo.xml too large (%d bytes)", unpackedSize) } ``` `uint64(-1)` is `18446744073709551615` — far above the 1 MB cap — so any RAR archive whose ComicInfo.xml entry has `UnKnownSize=true` returns an error even when the file is tiny and legitimate. In contrast `cbr_pages.go:75` correctly compares `h.UnPackedSize > maxCBRPageBytes` using `int64` arithmetic (where -1 is less than 50 MB and falls through safely). The fix: check `h.UnKnownSize` first and skip the size pre-check when the size is unknown; rely solely on `decodeComicInfoReader`'s `LimitReader` cap, which already handles `size=0` correctly by defaulting to `maxComicInfoBytes`. **[MINOR] internal/books/service.go:862 — stale `strings.EqualFold(f.ArchiveType, "cbz")` branch in `hasReadable` (pre-existing, not introduced by this PR)** `archive_type` in the database is `'ZIP'` or `'RAR'` (confirmed in `internal/library/scan/booktype.go`); `'cbz'` can never match. This branch is dead and misleading. It was not introduced by this PR (present in `main`) so does not block merge, but should be filed as a follow-up. --- **REVIEW VERDICT: 1 blocker, 0 major, 1 minor** The blocker (`uint64(-1)` cast in `ParseCBR`) will cause `comic.ParseCBR` to silently reject valid `ComicInfo.xml` entries in solid/streaming RAR4 archives. Fix: guard with `if !h.UnKnownSize` before the size pre-check, or compare as `int64` (matching `cbr_pages.go`'s pattern).

zombor commented 2026-06-02 15:44:50 +00:00

Author

Owner

Copy link

CODE REVIEW: NOT APPROVED

Phase 0: DEMO Verification

No DEMO block found in the bead description or PR body. Per review standard: NOT APPROVED — "No DEMO block provided". This is a bug-fix PR so a DEMO showing the fixed behavior is mandatory (e.g. a curl against a CBR book's reader endpoint, or the make test output showing the new CBX-aware test cases passing). Without it there is no verification the fixes actually apply to the production scenario.

---

Phase 1: Spec Compliance — PASSES (all three bugs addressed)

• Bug #1 (hasReadableComic): strings.EqualFold(f.BookType, "cbx") correct. Handler call site (handler.go:560) updated. hasReadable also updated to check CBX. Export bridge in export_test.go is re-export only.
• Bug #2 (reader dispatch): ComicPageCounter and ServeComicPageHandler both dispatch on archive_type; GetPrimaryCBZFileRow and GetBookFileRow now include archive_type; wiring in books/wire.go is complete.
• Bug #3 (persistComic): trigger changed to strings.EqualFold(nb.sr.BookType, "CBX"); archiveType parameter threaded through; library/wire.go dispatches ParseCBR vs ParseCBZ.

RAR page-serving strategy (full extraction to process-level sync.Map cache) is documented. ParseCBR passes f (an io.Reader) directly to rardecode.NewReader — no full-archive buffer. maxComicInfoBytes cap is present. export_test.go bridges contain only var/const re-exports, no test functions.

---

Phase 2: Code Quality

[BLOCKER] internal/comic/comicinfo.go:171 — uint64 cast of int64 UnPackedSize silently rejects valid ComicInfo.xml in unknown-size RAR entries

rardecode/v2 FileHeader.UnPackedSize is int64. Per the library source (archive15.go:283-285), when UnKnownSize == true (solid archives, streaming RAR4), UnPackedSize is set to -1. The code does:

unpackedSize := uint64(h.UnPackedSize) //nolint:gosec
if unpackedSize > maxComicInfoBytes { ... }

uint64(-1) is 18446744073709551615 — far above the 1 MB cap — so any RAR archive whose ComicInfo.xml entry has UnKnownSize=true returns an error even when the file is tiny and legitimate. In contrast cbr_pages.go:75 correctly compares h.UnPackedSize > maxCBRPageBytes using int64 arithmetic (where -1 is less than 50 MB and falls through safely). Fix: check h.UnKnownSize first and skip the size pre-check when the size is unknown; rely solely on decodeComicInfoReader's LimitReader cap, which already handles size=0 correctly by defaulting to maxComicInfoBytes.

[MINOR] internal/books/service.go:862 — stale strings.EqualFold(f.ArchiveType, "cbz") branch in hasReadable (pre-existing, not introduced by this PR)

archive_type in the database is 'ZIP' or 'RAR' (confirmed in internal/library/scan/booktype.go); 'cbz' can never match. This branch is dead and misleading. It was not introduced by this PR (present in main) so does not block merge, but should be filed as a follow-up.

---

REVIEW VERDICT: 1 blocker, 0 major, 1 minor

The blocker (uint64(-1) cast in ParseCBR) will cause comic.ParseCBR to silently reject valid ComicInfo.xml entries in solid/streaming RAR4 archives. Fix: guard with if !h.UnKnownSize before the size pre-check, or compare as int64 (matching cbr_pages.go's pattern). Also: DEMO block is required per review standard.

## CODE REVIEW: NOT APPROVED ### Phase 0: DEMO Verification No DEMO block found in the bead description or PR body. Per review standard: **NOT APPROVED — "No DEMO block provided"**. This is a bug-fix PR so a DEMO showing the fixed behavior is mandatory (e.g. a curl against a CBR book's reader endpoint, or the make test output showing the new CBX-aware test cases passing). Without it there is no verification the fixes actually apply to the production scenario. --- ### Phase 1: Spec Compliance — PASSES (all three bugs addressed) - Bug #1 (hasReadableComic): strings.EqualFold(f.BookType, "cbx") correct. Handler call site (handler.go:560) updated. hasReadable also updated to check CBX. Export bridge in export_test.go is re-export only. - Bug #2 (reader dispatch): ComicPageCounter and ServeComicPageHandler both dispatch on archive_type; GetPrimaryCBZFileRow and GetBookFileRow now include archive_type; wiring in books/wire.go is complete. - Bug #3 (persistComic): trigger changed to strings.EqualFold(nb.sr.BookType, "CBX"); archiveType parameter threaded through; library/wire.go dispatches ParseCBR vs ParseCBZ. RAR page-serving strategy (full extraction to process-level sync.Map cache) is documented. ParseCBR passes f (an io.Reader) directly to rardecode.NewReader — no full-archive buffer. maxComicInfoBytes cap is present. export_test.go bridges contain only var/const re-exports, no test functions. --- ### Phase 2: Code Quality [BLOCKER] internal/comic/comicinfo.go:171 — uint64 cast of int64 UnPackedSize silently rejects valid ComicInfo.xml in unknown-size RAR entries rardecode/v2 FileHeader.UnPackedSize is int64. Per the library source (archive15.go:283-285), when UnKnownSize == true (solid archives, streaming RAR4), UnPackedSize is set to -1. The code does: unpackedSize := uint64(h.UnPackedSize) //nolint:gosec if unpackedSize > maxComicInfoBytes { ... } uint64(-1) is 18446744073709551615 — far above the 1 MB cap — so any RAR archive whose ComicInfo.xml entry has UnKnownSize=true returns an error even when the file is tiny and legitimate. In contrast cbr_pages.go:75 correctly compares h.UnPackedSize > maxCBRPageBytes using int64 arithmetic (where -1 is less than 50 MB and falls through safely). Fix: check h.UnKnownSize first and skip the size pre-check when the size is unknown; rely solely on decodeComicInfoReader's LimitReader cap, which already handles size=0 correctly by defaulting to maxComicInfoBytes. [MINOR] internal/books/service.go:862 — stale strings.EqualFold(f.ArchiveType, "cbz") branch in hasReadable (pre-existing, not introduced by this PR) archive_type in the database is 'ZIP' or 'RAR' (confirmed in internal/library/scan/booktype.go); 'cbz' can never match. This branch is dead and misleading. It was not introduced by this PR (present in main) so does not block merge, but should be filed as a follow-up. --- REVIEW VERDICT: 1 blocker, 0 major, 1 minor The blocker (uint64(-1) cast in ParseCBR) will cause comic.ParseCBR to silently reject valid ComicInfo.xml entries in solid/streaming RAR4 archives. Fix: guard with if !h.UnKnownSize before the size pre-check, or compare as int64 (matching cbr_pages.go's pattern). Also: DEMO block is required per review standard.

zombor referenced this pull request from a commit 2026-06-02 15:51:42 +00:00

fix(security): address review findings from PR #285

zombor added 1 commit 2026-06-02 15:51:42 +00:00

fix(security): address review findings from PR #285 ... 1f77e53bf5

- BLOCKER: fix uint64(-1) overflow for UnKnownSize entries in ParseCBR;
  skip pre-check when h.UnKnownSize is true and fall through to
  decodeComicInfoReader's LimitReader cap instead
- MAJOR: add maxCBREntriesToScan cap (5000) to loadCBRPages to prevent
  unbounded CPU/I/O from archives with millions of non-image entries
- MAJOR: pass synthetic page%04d<ext> name (not raw entry filename) to
  http.ServeContent in ServeCBRPage to prevent Content-Disposition
  header injection
- MINOR: add maxCBRComicInfoEntries cap (5000) to ParseCBR's iteration loop
- MINOR: remove dead strings.EqualFold(f.ArchiveType, "cbz") branch from
  hasReadable; archive_type is always ZIP/RAR, never cbz; CBX book_type
  already covers comic archives; remove corresponding dead tests

Tests: 100% coverage maintained; new It blocks for entry-count caps and
synthetic-name guard; UnKnownSize gap documented in test comment.

zombor merged commit 9b37870fad into main 2026-06-02 15:57:25 +00:00

zombor referenced this pull request from a commit 2026-06-02 15:57:27 +00:00

Merge pull request 'fix(reader): CBX-aware reader + persistComic + hasReadableComic (bookshelf-b3s1)' (#285) from bd-bookshelf-b3s1 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!285

No description provided.