feat(series): series detail page + book-page series nav (bookshelf-xm84.1) #454

Merged

zombor merged 7 commits from bd-bookshelf-xm84.1 into main 2026-06-09 17:17:25 +00:00

Conversation 12 Commits 7 Files changed 22 +3523 -13

zombor commented 2026-06-09 02:48:45 +00:00

Owner

Copy link

Summary

• New GET /series/{seriesName} route: HTML + JSON detail page with two tabs (series-details, book-list); 404 for unknown series
• Book detail page (GET /books/{id}): series header links to /series/{name} (url-path-escaped), "More in Series" tab with cover grid — hidden when book has no series
• BooksInSeries store func ordered by series_number ASC, book_id ASC, LIMIT 200; optional library filter via userLibraryIDs (stub for auth epic 4op.5)
• GetDetail service returns middleware.ErrNotFound when series has no books
• SeriesBooksFromStore adapter in books package keeps cross-domain coupling clean
• urlpath (url.PathEscape) added to template funcMap for path-safe links
• 100% unit coverage on all new code; e2e/api tests for the new route

Test plan

• make test passes
• make coverage gate passes at 100%
• make build compiles clean
• e2e/api tests added: JSON/HTML 200, 404 for missing series, URL-encoded name
• Unit tests: BooksInSeries ordering + library filter, GetDetail not-found + success, DetailHandler HTML/JSON/404/empty-seriesName, series link + More-in-Series panel render, hidden when no series, SeriesBooksFromStore

Closes bead bookshelf-xm84.1 on merge.

## Summary - New `GET /series/{seriesName}` route: HTML + JSON detail page with two tabs (series-details, book-list); 404 for unknown series - Book detail page (`GET /books/{id}`): series header links to `/series/{name}` (url-path-escaped), "More in Series" tab with cover grid — hidden when book has no series - `BooksInSeries` store func ordered by `series_number ASC, book_id ASC`, LIMIT 200; optional library filter via `userLibraryIDs` (stub for auth epic 4op.5) - `GetDetail` service returns `middleware.ErrNotFound` when series has no books - `SeriesBooksFromStore` adapter in books package keeps cross-domain coupling clean - `urlpath` (`url.PathEscape`) added to template funcMap for path-safe links - 100% unit coverage on all new code; e2e/api tests for the new route ## Test plan - [x] `make test` passes - [x] `make coverage` gate passes at 100% - [x] `make build` compiles clean - [x] e2e/api tests added: JSON/HTML 200, 404 for missing series, URL-encoded name - [x] Unit tests: `BooksInSeries` ordering + library filter, `GetDetail` not-found + success, `DetailHandler` HTML/JSON/404/empty-seriesName, series link + More-in-Series panel render, hidden when no series, `SeriesBooksFromStore` Closes bead bookshelf-xm84.1 on merge.

zombor added 1 commit 2026-06-09 02:48:46 +00:00

feat(series): series detail page + book-page series nav (bookshelf-xm84.1) ... 2ff27dcec2

- GET /series/{seriesName}: HTML + JSON detail page with two tabs
  (series-details, book-list); returns 404 for unknown series
- BooksInSeries store func ordered by series_number/book_id, LIMIT 200,
  optional library filter (userLibraryIDs stub for 4op.5)
- GetDetail service: not-found when series has no books
- books_show.html: series header links to /series/{name} (url.PathEscape);
  "More in Series" tab hidden when book has no series, lists other books
  in the same series excluding current
- SeriesBooksFromStore adapter in books package (no cross-domain import)
- urlpath funcMap entry (url.PathEscape) for path-safe template links
- 100% unit coverage on new code; e2e/api tests for the new route

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

zombor commented 2026-06-09 03:12:23 +00:00

Author

Owner

Copy link

Security Review (bot)

PR #454 · bead bookshelf-xm84.1 · branch bd-bookshelf-xm84.1
New GET /series/{seriesName} detail page + More-in-Series panel on the book page.

---

SQL injection (BooksInSeries)

seriesName is bound as a parameterized ? placeholder in booksInSeriesBaseQuery; the optional AND b.library_id IN (...) clause is built with one ? per element from userLibraryIDs (integer IDs, never user-supplied strings). No user input is interpolated into the query string. Clean.

XSS via series name in HTML templates

• books_show.html: {{.Book.Series.Name}} in text context (auto-escaped by html/template). {{.SeriesLink}} used as an href value — Go's html/template applies URL-context sanitization; the value is always /series/<path-escaped-name> (built server-side with url.PathEscape), which the engine treats as a safe relative URL and passes through correctly.
• series_show.html (diff): {{.Detail.Name}} in text context (auto-escaped). {{.Detail.CoverBookID}} is an int64 rendered as an integer literal in a path — no injection possible. data-cover-img-title-value="{{.Title}}" — attribute auto-escaped. {{urlquery .Detail.Name}} for the book-list query param — correctly encoded.
• No raw template.HTML casts anywhere in the diff.

Open redirect / path-escape via SeriesLink

SeriesLink = "/series/" + url.PathEscape(book.Series.Name). The value always starts with the literal /series/ prefix; url.PathEscape encodes any special characters in the name, and the whole thing is a root-relative path — no //host or scheme prefix is possible. No open-redirect risk.

LIMIT bound

BooksInSeries appends LIMIT 200 unconditionally before executing the query. Bounded. No unbounded read.

?tab= parameter validation

validDetailTab (series detail page) and validBookTab (book page, with "series" added) both use closed allowlist maps. Any unrecognised tab value falls back to the default — no user-controlled string reaches the template unvalidated.

Library scoping (deferred)

Both call sites pass nil as userLibraryIDs, which the store func treats as "no library filter." The code is wired to accept and apply real IDs once 4op.5 lands. Per review scope, this is an acknowledged stub acceptable on main — not a finding.

Cover image path

/data/images/{{.ID}}/thumbnail.jpg — .ID is int64, rendered as an integer. No path traversal via a crafted ID.

---

No security findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review (bot) **PR #454 · bead bookshelf-xm84.1 · branch `bd-bookshelf-xm84.1`** New `GET /series/{seriesName}` detail page + More-in-Series panel on the book page. --- ### SQL injection (`BooksInSeries`) `seriesName` is bound as a parameterized `?` placeholder in `booksInSeriesBaseQuery`; the optional `AND b.library_id IN (...)` clause is built with one `?` per element from `userLibraryIDs` (integer IDs, never user-supplied strings). No user input is interpolated into the query string. Clean. ### XSS via series name in HTML templates - `books_show.html`: `{{.Book.Series.Name}}` in text context (auto-escaped by `html/template`). `{{.SeriesLink}}` used as an `href` value — Go's `html/template` applies URL-context sanitization; the value is always `/series/<path-escaped-name>` (built server-side with `url.PathEscape`), which the engine treats as a safe relative URL and passes through correctly. - `series_show.html` (diff): `{{.Detail.Name}}` in text context (auto-escaped). `{{.Detail.CoverBookID}}` is an `int64` rendered as an integer literal in a path — no injection possible. `data-cover-img-title-value="{{.Title}}"` — attribute auto-escaped. `{{urlquery .Detail.Name}}` for the book-list query param — correctly encoded. - No raw `template.HTML` casts anywhere in the diff. ### Open redirect / path-escape via `SeriesLink` `SeriesLink = "/series/" + url.PathEscape(book.Series.Name)`. The value always starts with the literal `/series/` prefix; `url.PathEscape` encodes any special characters in the name, and the whole thing is a root-relative path — no `//host` or scheme prefix is possible. No open-redirect risk. ### LIMIT bound `BooksInSeries` appends `LIMIT 200` unconditionally before executing the query. Bounded. No unbounded read. ### `?tab=` parameter validation `validDetailTab` (series detail page) and `validBookTab` (book page, with `"series"` added) both use closed allowlist maps. Any unrecognised tab value falls back to the default — no user-controlled string reaches the template unvalidated. ### Library scoping (deferred) Both call sites pass `nil` as `userLibraryIDs`, which the store func treats as "no library filter." The code is wired to accept and apply real IDs once 4op.5 lands. Per review scope, this is an acknowledged stub acceptable on main — not a finding. ### Cover image path `/data/images/{{.ID}}/thumbnail.jpg` — `.ID` is `int64`, rendered as an integer. No path traversal via a crafted ID. --- No security findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-09 03:15:08 +00:00

Author

Owner

Copy link

Code Review (bot)

PR #454 · bead bookshelf-xm84.1 · branch bd-bookshelf-xm84.1
Series detail page v1: GET /series/{seriesName}, More-in-Series panel, series header link.

---

Phase 0: DEMO Verification

No explicit DEMO: command block is present in the PR. The bead description required "authenticated screenshot of the series page + the More-in-Series tab" — no screenshot is posted in the PR comments. This is a UI surface that cannot be reproduced as a CLI command; noting for human visual verification. The e2e tests cover HTTP-layer behavior (status codes, JSON shape, 404, URL-encoded names). Proceeding to Phase 1 with this caveat.

---

Phase 1: Spec Compliance

All spec requirements are present:

• GET /series/{seriesName} route registered — internal/series/routes.go:842
• BooksInSeries store query: series_number ASC, book_id ASC, LIMIT 200, uses idx_book_sort_series, optional userLibraryIDs filter — internal/series/store.go:1117
• GetDetail service: ErrNotFound when empty — internal/series/service.go:185
• DetailHandler: HTML + JSON + 404 — internal/series/handler.go:587
• Series header made a link via url.PathEscape — internal/books/handler.go:136
• "More in Series" tab conditional on {{if .Book.Series}} — templates/pages/books_show.html:1386
• Current book excluded from grid — internal/books/handler.go:139
• urlpath (url.PathEscape) added to funcMap — internal/tmpl/renderer.go:1348
• userLibraryIDs stubbed as nil on main (4op.5-ready)
• 100% coverage gate passes per CI

No missing requirements. No extra/unneeded work.

---

Phase 2: Code Quality

[MINOR] internal/books/handler_test.go:3282-3287 — three Expect calls in one It block ("excludes the current book from the series grid"): reads body, checks err, asserts data-id="99" present, and asserts data-id="42" absent. Project convention mandates exactly one Expect per It. Same pattern in "renders a series link in the HTML body" (3269-3272) and "series link href contains..." (3274-3277) — each has two Expect calls. Split each into separate It blocks with a shared JustBeforeEach that captures body, err.

[MINOR] templates/pages/series_show.html:31 — main cover image <img src="/data/images/{{.Detail.CoverBookID}}/cover.jpg"> has no ?v= cache-buster. Every other cover in the codebase (e.g. books_show.html:56, home.html) appends ?v={{.CoverHash}}. The SeriesDetail struct doesn't carry a CoverBookHash but Detail.Books[0].CoverHash is available when len(Detail.Books) > 0. No correctness impact; the image will go stale in browser cache after a cover regen until hard-refresh.

[MINOR] internal/books/handler.go:896 — listSeriesBooks error is silently discarded (scErr is checked and dropped). The listNotes call above it has //nolint:errcheck — graceful degradation; empty list on error documenting the intent. The series-books call is missing this comment, making the intent opaque in future reads.

---

No blockers. No majors. Three minors (test-convention, cache-buster, undocumented degradation) — none block merge.

REVIEW VERDICT: 0 blocker, 0 major, 3 minor

## Code Review (bot) **PR #454 · bead bookshelf-xm84.1 · branch `bd-bookshelf-xm84.1`** Series detail page v1: `GET /series/{seriesName}`, More-in-Series panel, series header link. --- ### Phase 0: DEMO Verification No explicit `DEMO:` command block is present in the PR. The bead description required "authenticated screenshot of the series page + the More-in-Series tab" — no screenshot is posted in the PR comments. This is a UI surface that cannot be reproduced as a CLI command; noting for human visual verification. The e2e tests cover HTTP-layer behavior (status codes, JSON shape, 404, URL-encoded names). Proceeding to Phase 1 with this caveat. --- ### Phase 1: Spec Compliance All spec requirements are present: - `GET /series/{seriesName}` route registered — `internal/series/routes.go:842` - `BooksInSeries` store query: `series_number ASC, book_id ASC, LIMIT 200`, uses `idx_book_sort_series`, optional `userLibraryIDs` filter — `internal/series/store.go:1117` - `GetDetail` service: `ErrNotFound` when empty — `internal/series/service.go:185` - `DetailHandler`: HTML + JSON + 404 — `internal/series/handler.go:587` - Series header made a link via `url.PathEscape` — `internal/books/handler.go:136` - "More in Series" tab conditional on `{{if .Book.Series}}` — `templates/pages/books_show.html:1386` - Current book excluded from grid — `internal/books/handler.go:139` - `urlpath` (`url.PathEscape`) added to funcMap — `internal/tmpl/renderer.go:1348` - `userLibraryIDs` stubbed as `nil` on main (4op.5-ready) - 100% coverage gate passes per CI No missing requirements. No extra/unneeded work. --- ### Phase 2: Code Quality [MINOR] `internal/books/handler_test.go:3282-3287` — three `Expect` calls in one `It` block ("excludes the current book from the series grid"): reads body, checks `err`, asserts `data-id="99"` present, and asserts `data-id="42"` absent. Project convention mandates exactly one `Expect` per `It`. Same pattern in "renders a series link in the HTML body" (3269-3272) and "series link href contains..." (3274-3277) — each has two `Expect` calls. Split each into separate `It` blocks with a shared `JustBeforeEach` that captures `body, err`. [MINOR] `templates/pages/series_show.html:31` — main cover image `<img src="/data/images/{{.Detail.CoverBookID}}/cover.jpg">` has no `?v=` cache-buster. Every other cover in the codebase (e.g. `books_show.html:56`, `home.html`) appends `?v={{.CoverHash}}`. The `SeriesDetail` struct doesn't carry a `CoverBookHash` but `Detail.Books[0].CoverHash` is available when `len(Detail.Books) > 0`. No correctness impact; the image will go stale in browser cache after a cover regen until hard-refresh. [MINOR] `internal/books/handler.go:896` — `listSeriesBooks` error is silently discarded (`scErr` is checked and dropped). The `listNotes` call above it has `//nolint:errcheck — graceful degradation; empty list on error` documenting the intent. The series-books call is missing this comment, making the intent opaque in future reads. --- No blockers. No majors. Three minors (test-convention, cache-buster, undocumented degradation) — none block merge. REVIEW VERDICT: 0 blocker, 0 major, 3 minor

zombor commented 2026-06-09 03:15:15 +00:00

Author

Owner

Copy link

Code Review (bot)

PR #454 · bead bookshelf-xm84.1 · branch bd-bookshelf-xm84.1
Series detail page v1: GET /series/{seriesName}, More-in-Series panel, series header link.

---

Phase 0: DEMO Verification

No explicit DEMO: command block is present in the PR. The bead description required "authenticated screenshot of the series page + the More-in-Series tab" — no screenshot is posted in the PR comments. This is a UI surface that cannot be reproduced as a CLI command; noting for human visual verification. The e2e tests cover HTTP-layer behavior (status codes, JSON shape, 404, URL-encoded names). Proceeding to Phase 1 with this caveat.

---

Phase 1: Spec Compliance

All spec requirements are present:

• GET /series/{seriesName} route registered — internal/series/routes.go:842
• BooksInSeries store query: series_number ASC, book_id ASC, LIMIT 200, uses idx_book_sort_series, optional userLibraryIDs filter — internal/series/store.go:1117
• GetDetail service: ErrNotFound when empty — internal/series/service.go:185
• DetailHandler: HTML + JSON + 404 — internal/series/handler.go:587
• Series header made a link via url.PathEscape — internal/books/handler.go:136
• "More in Series" tab conditional on {{if .Book.Series}} — templates/pages/books_show.html:1386
• Current book excluded from grid — internal/books/handler.go:139
• urlpath (url.PathEscape) added to funcMap — internal/tmpl/renderer.go:1348
• userLibraryIDs stubbed as nil on main (4op.5-ready)
• 100% coverage gate passes per CI

No missing requirements. No extra/unneeded work.

---

Phase 2: Code Quality

[MINOR] internal/books/handler_test.go:3282-3287 — three Expect calls in one It block ("excludes the current book from the series grid"): reads body, checks err, asserts data-id="99" present, and asserts data-id="42" absent. Project convention mandates exactly one Expect per It. Same pattern in "renders a series link in the HTML body" (3269-3272) and "series link href contains..." (3274-3277) — each has two Expect calls. Split each into separate It blocks with a shared JustBeforeEach that captures body, err.

[MINOR] templates/pages/series_show.html:31 — main cover image <img src="/data/images/{{.Detail.CoverBookID}}/cover.jpg"> has no ?v= cache-buster. Every other cover in the codebase (e.g. books_show.html:56, home.html) appends ?v={{.CoverHash}}. The SeriesDetail struct doesn't carry a CoverBookHash but Detail.Books[0].CoverHash is available when len(Detail.Books) > 0. No correctness impact; the image will go stale in browser cache after a cover regen until hard-refresh.

[MINOR] internal/books/handler.go:896 — listSeriesBooks error is silently discarded (scErr is checked and dropped). The listNotes call above it has //nolint:errcheck — graceful degradation; empty list on error documenting the intent. The series-books call is missing this comment, making the intent opaque in future reads.

---

No blockers. No majors. Three minors (test-convention, cache-buster, undocumented degradation) — none block merge.

REVIEW VERDICT: 0 blocker, 0 major, 3 minor

## Code Review (bot) **PR #454 · bead bookshelf-xm84.1 · branch `bd-bookshelf-xm84.1`** Series detail page v1: `GET /series/{seriesName}`, More-in-Series panel, series header link. --- ### Phase 0: DEMO Verification No explicit `DEMO:` command block is present in the PR. The bead description required "authenticated screenshot of the series page + the More-in-Series tab" — no screenshot is posted in the PR comments. This is a UI surface that cannot be reproduced as a CLI command; noting for human visual verification. The e2e tests cover HTTP-layer behavior (status codes, JSON shape, 404, URL-encoded names). Proceeding to Phase 1 with this caveat. --- ### Phase 1: Spec Compliance All spec requirements are present: - `GET /series/{seriesName}` route registered — `internal/series/routes.go:842` - `BooksInSeries` store query: `series_number ASC, book_id ASC, LIMIT 200`, uses `idx_book_sort_series`, optional `userLibraryIDs` filter — `internal/series/store.go:1117` - `GetDetail` service: `ErrNotFound` when empty — `internal/series/service.go:185` - `DetailHandler`: HTML + JSON + 404 — `internal/series/handler.go:587` - Series header made a link via `url.PathEscape` — `internal/books/handler.go:136` - "More in Series" tab conditional on `{{if .Book.Series}}` — `templates/pages/books_show.html:1386` - Current book excluded from grid — `internal/books/handler.go:139` - `urlpath` (`url.PathEscape`) added to funcMap — `internal/tmpl/renderer.go:1348` - `userLibraryIDs` stubbed as `nil` on main (4op.5-ready) - 100% coverage gate passes per CI No missing requirements. No extra/unneeded work. --- ### Phase 2: Code Quality [MINOR] `internal/books/handler_test.go:3282-3287` — three `Expect` calls in one `It` block ("excludes the current book from the series grid"): reads body, checks `err`, asserts `data-id="99"` present, and asserts `data-id="42"` absent. Project convention mandates exactly one `Expect` per `It`. Same pattern in "renders a series link in the HTML body" (3269-3272) and "series link href contains..." (3274-3277) — each has two `Expect` calls. Split each into separate `It` blocks with a shared `JustBeforeEach` that captures `body, err`. [MINOR] `templates/pages/series_show.html:31` — main cover image `<img src="/data/images/{{.Detail.CoverBookID}}/cover.jpg">` has no `?v=` cache-buster. Every other cover in the codebase (e.g. `books_show.html:56`, `home.html`) appends `?v={{.CoverHash}}`. The `SeriesDetail` struct doesn't carry a `CoverBookHash` but `Detail.Books[0].CoverHash` is available when `len(Detail.Books) > 0`. No correctness impact; the image will go stale in browser cache after a cover regen until hard-refresh. [MINOR] `internal/books/handler.go:896` — `listSeriesBooks` error is silently discarded (`scErr` is checked and dropped). The `listNotes` call above it has `//nolint:errcheck — graceful degradation; empty list on error` documenting the intent. The series-books call is missing this comment, making the intent opaque in future reads. --- No blockers. No majors. Three minors (test-convention, cache-buster, undocumented degradation) — none block merge. REVIEW VERDICT: 0 blocker, 0 major, 3 minor

zombor force-pushed bd-bookshelf-xm84.1 from 2ff27dcec2 to b76a1dd899 2026-06-09 12:12:38 +00:00 Compare

zombor added 1 commit 2026-06-09 12:49:48 +00:00

feat(series): Grimmory parity – enriched detail, inline book list, cover fix (bookshelf-xm84.1) ... 63df3f45e7

- Fix blank-render bug: add .series-detail__cover CSS (fixed 260px width,
  aspect-ratio 1/1.5, object-fit cover) + ?v= cache-buster on cover img src
- Series Details tab: cover + full Grimmory metadata panel (title, author link
  to /books?author_id=, genre/tag chips, badge row with year-range/pages/
  formats/language/owned count, avg rating badges, Next Up card, read progress
  bar, Synopsis from book #1)
- Book List tab: inline table rows (series#, thumb, title link, year, pages,
  read-status badge, format badge); remove old "Browse in Book List" link
- Extend BooksInSeries query with 9 new columns (published_year, page_count,
  language, series_total, goodreads_rating, hardcover_rating, description,
  author_id, author_name) via GROUP BY + LEFT JOIN author – no N+1
- New batch store functions: SeriesBookFormats, SeriesTags, SeriesCategories,
  SeriesReadStatus (all single IN(...) queries, no N+1)
- GetDetail now takes userID; read_status scoped to session user (multi-user)
- buildSeriesDetail: pure aggregation fn computing aggregates, avg ratings,
  next-up, read count, synopsis, formats, deduplicated chips
- readPercent helper with denom-zero guard and 100-clamp
- 100% coverage across all new code paths; e2e seedRichSeries + 20 new assertions

zombor added 1 commit 2026-06-09 13:03:56 +00:00

fix(e2e): correct book_file INSERT in seedRichSeries (file_sub_path, dynamic user ID) ... 51271088d5

- Replace nonexistent `file_size`/`current_hash` columns with correct
  `file_sub_path` (NOT NULL) in book_file INSERT
- Replace hardcoded user_id=1 with dynamic lookup of testadmin's id
  so the read-status seed survives auto_increment gaps across test runs

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-06-09 13:15:39 +00:00

Author

Owner

Copy link

Code RE-Review (bot)

PR #454 · bead bookshelf-xm84.1 · branch bd-bookshelf-xm84.1
Full Grimmory-parity rebuild: cover CSS fix, genre/tag chips, badge row, avg-rating badges, per-user Next-Up, read-progress bar, synopsis, books-in-series cover grid, inline Book List tab.

---

Phase 0: DEMO Verification

No explicit DEMO: command block exists in this PR or bead. This is a UI-surface PR with visual output. The implementation is fully covered by e2e tests (e2e/api/series_test.go: GET /series/{seriesName} enriched detail, aggregates, read-status, formats, tags — all verified against a real seeded DB at CI). Proceeding on that basis.

---

Phase 1: Spec Compliance

All items from the re-review prompt are present:

• Cover (fixed-size via .series-detail__cover .cover-image CSS with aspect-ratio: 1/1.5 + parent width: 260px from .book-detail__cover) — static/css/main.css:5201
• Blank-render CSS fix scoped to .series-detail__cover — does not touch .book-detail__cover — static/css/main.css:5196–5216
• Genre/tag chips (union of series books' categories + tags, deduplicated) — internal/series/service.go:buildSeriesDetail
• Badge row (N books, year-range, total pages, formats, language, X/N owned) — templates/pages/series_show.html:65–88
• Avg-rating badges (Goodreads/Hardcover) — templates/pages/series_show.html:97–104
• Per-user Next-Up (first UNREAD by series_number) scoped by session userID — internal/series/service.go:buildSeriesDetail, internal/series/handler.go:DetailHandler
• Read-progress bar — templates/pages/series_show.html:124–135
• Synopsis from book #1 description — internal/series/service.go:buildSeriesDetail (i == 0 branch)
• Books-in-series cover grid — templates/pages/series_show.html:150–175
• Inline Book List tab (rows: # / cover / title / year / pages / status / format / actions) — templates/pages/series_show.html:185–230
• No N+1: formats, tags, categories, read-status each fetched in one IN(...) batch query — internal/series/store.go
• Read-status scoped by session userID (extractUser(r).ID), never from query param — internal/series/handler.go:204
• All per-book follow-ups batched — confirmed
• 100% coverage gate passes (CI green)

---

Phase 2: Code Quality

[MAJOR] internal/series/store.go:258–259 — MIN(a.id) and MIN(a.name) can return data from different author rows

booksInSeriesBaseQuery uses COALESCE(MIN(a.id), 0) AS author_id and COALESCE(MIN(a.name), '') AS author_name as two independent aggregate expressions. MySQL evaluates each independently across all joined author rows. If a book has two authors — id=1 "Zorro" and id=2 "Aaron" — MIN(id)=1 selects Zorro while MIN(name)="Aaron" selects the name of author id=2. The result is AuthorID=1 paired with AuthorName="Aaron", pointing the author link (/books?author_id=1) at the wrong author. The ListSeriesAuthors function in the same file solves this correctly using a ROW_NUMBER() OVER (PARTITION BY ... ORDER BY b.id ASC, m.sort_order ASC, a.id ASC) window, which ensures both id and name come from the same row. The fix is to use the same ROW_NUMBER approach (or a subquery) for per-book author selection in BooksInSeries instead of MIN(a.id) + MIN(a.name).

[MAJOR] templates/pages/series_show.html:85 — "Owned" badge shows ReadCount not BookCount

The badge {{.Detail.ReadCount}}/{{.Detail.SeriesTotal}} Owned shows the number of read books (user read-status = READ) out of the canonical series total, but labels this as "Owned." In Grimmory/Booklore vocabulary, "owned" means books imported into the library (i.e. BookCount — how many books are present in the DB). ReadCount is the read-progress metric, already correctly used by the progress bar immediately below. The result is a badge that says e.g. "1/6 Owned" when a user has read 1 of their 6 imported books, even if they own all 6. Correct render: {{.Detail.BookCount}}/{{.Detail.SeriesTotal}} Owned for the owned badge; the ReadCount remains correct for the progress bar label and fill.

[MINOR] templates/pages/series_show.html:35 — Main series cover has no cache-buster query param

<img src="/data/images/{{.Detail.CoverBookID}}/cover.jpg?v={{.AssetVersion}}"> — the ?v= uses AssetVersion (a static hash of embedded static files), not the book's CoverHash. If a book's cover is regenerated, the browser will serve the old image until cache expires. Detail.Books[0].CoverHash is available and should be used instead. The thumbnail grid covers below correctly use ?v={{.CoverHash}}.

[MINOR] internal/books/handler.go:895–901 — listSeriesBooks error silently dropped without comment

The scErr is captured and checked (if scErr == nil), but when non-nil the function continues silently. The analogous listNotes call above has //nolint:errcheck — graceful degradation; empty list on error. The intent for series books should be documented the same way to make the deliberate degradation explicit.

[MINOR] internal/books/handler_test.go — multiple Expect calls per It in new series-nav tests

"excludes the current book from the series grid" (handler_test.go:3282) makes three Expect calls (body read + present check + absent check). "renders a series link" and "series link href" each make two. Project convention: exactly one Expect per It. Split with shared JustBeforeEach capturing (body []byte, err error).

---

REVIEW VERDICT: 0 blocker, 2 major, 3 minor

## Code RE-Review (bot) **PR #454 · bead bookshelf-xm84.1 · branch `bd-bookshelf-xm84.1`** Full Grimmory-parity rebuild: cover CSS fix, genre/tag chips, badge row, avg-rating badges, per-user Next-Up, read-progress bar, synopsis, books-in-series cover grid, inline Book List tab. --- ### Phase 0: DEMO Verification No explicit `DEMO:` command block exists in this PR or bead. This is a UI-surface PR with visual output. The implementation is fully covered by e2e tests (`e2e/api/series_test.go`: `GET /series/{seriesName}` enriched detail, aggregates, read-status, formats, tags — all verified against a real seeded DB at CI). Proceeding on that basis. --- ### Phase 1: Spec Compliance All items from the re-review prompt are present: - Cover (fixed-size via `.series-detail__cover .cover-image` CSS with `aspect-ratio: 1/1.5` + parent `width: 260px` from `.book-detail__cover`) — `static/css/main.css:5201` - Blank-render CSS fix scoped to `.series-detail__cover` — does not touch `.book-detail__cover` — `static/css/main.css:5196–5216` - Genre/tag chips (union of series books' categories + tags, deduplicated) — `internal/series/service.go:buildSeriesDetail` - Badge row (N books, year-range, total pages, formats, language, X/N owned) — `templates/pages/series_show.html:65–88` - Avg-rating badges (Goodreads/Hardcover) — `templates/pages/series_show.html:97–104` - Per-user Next-Up (first UNREAD by series_number) scoped by session userID — `internal/series/service.go:buildSeriesDetail`, `internal/series/handler.go:DetailHandler` - Read-progress bar — `templates/pages/series_show.html:124–135` - Synopsis from book #1 description — `internal/series/service.go:buildSeriesDetail` (`i == 0` branch) - Books-in-series cover grid — `templates/pages/series_show.html:150–175` - Inline Book List tab (rows: # / cover / title / year / pages / status / format / actions) — `templates/pages/series_show.html:185–230` - No N+1: formats, tags, categories, read-status each fetched in one `IN(...)` batch query — `internal/series/store.go` - Read-status scoped by session userID (`extractUser(r).ID`), never from query param — `internal/series/handler.go:204` - All per-book follow-ups batched — confirmed - 100% coverage gate passes (CI green) --- ### Phase 2: Code Quality **[MAJOR] `internal/series/store.go:258–259` — `MIN(a.id)` and `MIN(a.name)` can return data from different author rows** `booksInSeriesBaseQuery` uses `COALESCE(MIN(a.id), 0) AS author_id` and `COALESCE(MIN(a.name), '') AS author_name` as two independent aggregate expressions. MySQL evaluates each independently across all joined author rows. If a book has two authors — `id=1 "Zorro"` and `id=2 "Aaron"` — `MIN(id)=1` selects Zorro while `MIN(name)="Aaron"` selects the name of author id=2. The result is `AuthorID=1` paired with `AuthorName="Aaron"`, pointing the author link (`/books?author_id=1`) at the wrong author. The `ListSeriesAuthors` function in the same file solves this correctly using a `ROW_NUMBER() OVER (PARTITION BY ... ORDER BY b.id ASC, m.sort_order ASC, a.id ASC)` window, which ensures both id and name come from the same row. The fix is to use the same `ROW_NUMBER` approach (or a subquery) for per-book author selection in `BooksInSeries` instead of `MIN(a.id)` + `MIN(a.name)`. **[MAJOR] `templates/pages/series_show.html:85` — "Owned" badge shows `ReadCount` not `BookCount`** The badge `{{.Detail.ReadCount}}/{{.Detail.SeriesTotal}} Owned` shows the number of **read** books (user read-status = READ) out of the canonical series total, but labels this as "Owned." In Grimmory/Booklore vocabulary, "owned" means books imported into the library (i.e. `BookCount` — how many books are present in the DB). `ReadCount` is the read-progress metric, already correctly used by the progress bar immediately below. The result is a badge that says e.g. "1/6 Owned" when a user has read 1 of their 6 imported books, even if they own all 6. Correct render: `{{.Detail.BookCount}}/{{.Detail.SeriesTotal}} Owned` for the owned badge; the `ReadCount` remains correct for the progress bar label and fill. **[MINOR] `templates/pages/series_show.html:35` — Main series cover has no cache-buster query param** `<img src="/data/images/{{.Detail.CoverBookID}}/cover.jpg?v={{.AssetVersion}}">` — the `?v=` uses `AssetVersion` (a static hash of embedded static files), not the book's `CoverHash`. If a book's cover is regenerated, the browser will serve the old image until cache expires. `Detail.Books[0].CoverHash` is available and should be used instead. The thumbnail grid covers below correctly use `?v={{.CoverHash}}`. **[MINOR] `internal/books/handler.go:895–901` — `listSeriesBooks` error silently dropped without comment** The `scErr` is captured and checked (`if scErr == nil`), but when non-nil the function continues silently. The analogous `listNotes` call above has `//nolint:errcheck — graceful degradation; empty list on error`. The intent for series books should be documented the same way to make the deliberate degradation explicit. **[MINOR] `internal/books/handler_test.go` — multiple `Expect` calls per `It` in new series-nav tests** "excludes the current book from the series grid" (`handler_test.go:3282`) makes three `Expect` calls (body read + present check + absent check). "renders a series link" and "series link href" each make two. Project convention: exactly one `Expect` per `It`. Split with shared `JustBeforeEach` capturing `(body []byte, err error)`. --- REVIEW VERDICT: 0 blocker, 2 major, 3 minor

zombor commented 2026-06-09 13:16:29 +00:00

Author

Owner

Copy link

Security RE-Review (bot)

Security re-review of PR #454 (bead bookshelf-xm84.1) — series page full Grimmory parity.
Diff base: git merge-base origin/main origin/bd-bookshelf-xm84.1.

---

Focus areas checked

1. Per-user read_status/progress scoping (IDOR)

DetailHandler (internal/series/handler.go:201-206) derives userID exclusively from extractUser(r).ID, which reads ClaimsFromContext(r.Context()) set by AuthMiddleware from a JWT. No request body, query param, or path value contributes to the user ID. The seriesReadStatusQuery (internal/series/store.go) uses WHERE user_id = ? AND book_id IN (...) with userID as the first bound parameter — cross-user read data cannot leak. GetDetail skips the status query entirely when userID == 0 (unauthenticated), defaulting all books to UNREAD. This is correct.

2. SQL parameterisation

All five new store functions (BooksInSeries, SeriesBookFormats, SeriesTags, SeriesCategories, SeriesReadStatus) build dynamic SQL via placeholder strings of "?" and pass user-sourced values only through the variadic args ...any slice — never via string interpolation. The seriesName filter is WHERE bm.series_name = ?, the IN (...) clause is constructed from integer IDs only. No injection vector found.

3. XSS via crafted metadata (html/template)

All metadata strings — Detail.Name, Detail.Synopsis, Detail.FirstAuthor, Detail.Tags items, SeriesDetailBook.Title, SeriesDetailBook.ReadStatus, SeriesDetailBook.Format — are rendered via {{.Foo}} inside html/template blocks, which applies context-sensitive auto-escaping. No template.HTML, template.URL, template.JS, or safeHTML-style bypasses appear in the new code. Chip/badge labels from tags and categories are emitted via {{.}} in text context — escaped. Synopsis in <p> — escaped.

4. URL construction and open-redirect risk

SeriesLink is computed server-side as "/series/" + url.PathEscape(book.Series.Name) — a relative path starting with /. No external host can be injected via the series name. html/template applies additional URL-context escaping when rendering {{.SeriesLink}} in an href attribute, so a crafted series name cannot produce a javascript: URL or redirect to a foreign host.

FirstAuthorID is an int64 from the database, rendered as {{.Detail.FirstAuthorID}} in a query string: html/template will HTML-entity-escape numeric values in attribute context (harmless; integers contain no special chars).

5. Inline style injection

style="width:{{printf "%.1f" .ReadPercent}}%" — ReadPercent is a float64 computed by readPercent() from server-side integer counts, capped at 100. No user input reaches this value.

6. CoverHash in URL query params

book_cover_hash is always a 16-character lowercase hex FNV hash (see cover/generate.go:coverHash). Used as ?v=<hash> cache-buster — no special characters possible; no injection surface.

7. Write surface

No new POST/PUT/PATCH/DELETE routes are introduced. The only new HTTP surface is GET /series/{seriesName} (read-only) and the additions to the existing GET /books/{id} response.

8. Library scoping consistency

getDetail passes nil for userLibraryIDs, so BooksInSeries applies no library filter. This is the same behaviour as the existing GET /series list (which also queries across all libraries without a per-user library filter). The library-scoping stub is consistent with the pre-existing posture and is noted as intentional in the PR description. Not a regression introduced by this PR.

---

Findings

None.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security RE-Review (bot) Security re-review of PR #454 (bead bookshelf-xm84.1) — series page full Grimmory parity. Diff base: `git merge-base origin/main origin/bd-bookshelf-xm84.1`. --- ### Focus areas checked **1. Per-user read_status/progress scoping (IDOR)** `DetailHandler` (`internal/series/handler.go:201-206`) derives `userID` exclusively from `extractUser(r).ID`, which reads `ClaimsFromContext(r.Context())` set by `AuthMiddleware` from a JWT. No request body, query param, or path value contributes to the user ID. The `seriesReadStatusQuery` (`internal/series/store.go`) uses `WHERE user_id = ? AND book_id IN (...)` with `userID` as the first bound parameter — cross-user read data cannot leak. `GetDetail` skips the status query entirely when `userID == 0` (unauthenticated), defaulting all books to `UNREAD`. This is correct. **2. SQL parameterisation** All five new store functions (`BooksInSeries`, `SeriesBookFormats`, `SeriesTags`, `SeriesCategories`, `SeriesReadStatus`) build dynamic SQL via placeholder strings of `"?"` and pass user-sourced values only through the variadic `args ...any` slice — never via string interpolation. The `seriesName` filter is `WHERE bm.series_name = ?`, the `IN (...)` clause is constructed from integer IDs only. No injection vector found. **3. XSS via crafted metadata (html/template)** All metadata strings — `Detail.Name`, `Detail.Synopsis`, `Detail.FirstAuthor`, `Detail.Tags` items, `SeriesDetailBook.Title`, `SeriesDetailBook.ReadStatus`, `SeriesDetailBook.Format` — are rendered via `{{.Foo}}` inside `html/template` blocks, which applies context-sensitive auto-escaping. No `template.HTML`, `template.URL`, `template.JS`, or `safeHTML`-style bypasses appear in the new code. Chip/badge labels from tags and categories are emitted via `{{.}}` in text context — escaped. Synopsis in `<p>` — escaped. **4. URL construction and open-redirect risk** `SeriesLink` is computed server-side as `"/series/" + url.PathEscape(book.Series.Name)` — a relative path starting with `/`. No external host can be injected via the series name. `html/template` applies additional URL-context escaping when rendering `{{.SeriesLink}}` in an `href` attribute, so a crafted series name cannot produce a `javascript:` URL or redirect to a foreign host. `FirstAuthorID` is an `int64` from the database, rendered as `{{.Detail.FirstAuthorID}}` in a query string: `html/template` will HTML-entity-escape numeric values in attribute context (harmless; integers contain no special chars). **5. Inline style injection** `style="width:{{printf "%.1f" .ReadPercent}}%"` — `ReadPercent` is a `float64` computed by `readPercent()` from server-side integer counts, capped at 100. No user input reaches this value. **6. CoverHash in URL query params** `book_cover_hash` is always a 16-character lowercase hex FNV hash (see `cover/generate.go:coverHash`). Used as `?v=<hash>` cache-buster — no special characters possible; no injection surface. **7. Write surface** No new `POST`/`PUT`/`PATCH`/`DELETE` routes are introduced. The only new HTTP surface is `GET /series/{seriesName}` (read-only) and the additions to the existing `GET /books/{id}` response. **8. Library scoping consistency** `getDetail` passes `nil` for `userLibraryIDs`, so `BooksInSeries` applies no library filter. This is the same behaviour as the existing `GET /series` list (which also queries across all libraries without a per-user library filter). The library-scoping stub is consistent with the pre-existing posture and is noted as intentional in the PR description. Not a regression introduced by this PR. --- ### Findings None. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-09 13:16:37 +00:00

Author

Owner

Copy link

Security RE-Review (bot)

Security re-review of PR #454 (bead bookshelf-xm84.1) — series page full Grimmory parity.
Diff base: git merge-base origin/main origin/bd-bookshelf-xm84.1.

---

Focus areas checked

1. Per-user read_status/progress scoping (IDOR)

DetailHandler (internal/series/handler.go:201-206) derives userID exclusively from extractUser(r).ID, which reads ClaimsFromContext(r.Context()) set by AuthMiddleware from a JWT. No request body, query param, or path value contributes to the user ID. The seriesReadStatusQuery (internal/series/store.go) uses WHERE user_id = ? AND book_id IN (...) with userID as the first bound parameter — cross-user read data cannot leak. GetDetail skips the status query entirely when userID == 0 (unauthenticated), defaulting all books to UNREAD. This is correct.

2. SQL parameterisation

All five new store functions (BooksInSeries, SeriesBookFormats, SeriesTags, SeriesCategories, SeriesReadStatus) build dynamic SQL via placeholder strings of "?" and pass user-sourced values only through the variadic args ...any slice — never via string interpolation. The seriesName filter is WHERE bm.series_name = ?, the IN (...) clause is constructed from integer IDs only. No injection vector found.

3. XSS via crafted metadata (html/template)

All metadata strings — Detail.Name, Detail.Synopsis, Detail.FirstAuthor, Detail.Tags items, SeriesDetailBook.Title, SeriesDetailBook.ReadStatus, SeriesDetailBook.Format — are rendered via {{.Foo}} inside html/template blocks, which applies context-sensitive auto-escaping. No template.HTML, template.URL, template.JS, or safeHTML-style bypasses appear in the new code. Chip/badge labels from tags and categories are emitted via {{.}} in text context — escaped. Synopsis in <p> — escaped.

4. URL construction and open-redirect risk

SeriesLink is computed server-side as "/series/" + url.PathEscape(book.Series.Name) — a relative path starting with /. No external host can be injected via the series name. html/template applies additional URL-context escaping when rendering {{.SeriesLink}} in an href attribute, so a crafted series name cannot produce a javascript: URL or redirect to a foreign host.

FirstAuthorID is an int64 from the database, rendered as {{.Detail.FirstAuthorID}} in a query string: html/template will HTML-entity-escape numeric values in attribute context (harmless; integers contain no special chars).

5. Inline style injection

style="width:{{printf "%.1f" .ReadPercent}}%" — ReadPercent is a float64 computed by readPercent() from server-side integer counts, capped at 100. No user input reaches this value.

6. CoverHash in URL query params

book_cover_hash is always a 16-character lowercase hex FNV hash (see cover/generate.go:coverHash). Used as ?v=<hash> cache-buster — no special characters possible; no injection surface.

7. Write surface

No new POST/PUT/PATCH/DELETE routes are introduced. The only new HTTP surface is GET /series/{seriesName} (read-only) and the additions to the existing GET /books/{id} response.

8. Library scoping consistency

getDetail passes nil for userLibraryIDs, so BooksInSeries applies no library filter. This is the same behaviour as the existing GET /series list (which also queries across all libraries without a per-user library filter). The library-scoping stub is consistent with the pre-existing posture and is noted as intentional in the PR description. Not a regression introduced by this PR.

---

Findings

None.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security RE-Review (bot) Security re-review of PR #454 (bead bookshelf-xm84.1) — series page full Grimmory parity. Diff base: `git merge-base origin/main origin/bd-bookshelf-xm84.1`. --- ### Focus areas checked **1. Per-user read_status/progress scoping (IDOR)** `DetailHandler` (`internal/series/handler.go:201-206`) derives `userID` exclusively from `extractUser(r).ID`, which reads `ClaimsFromContext(r.Context())` set by `AuthMiddleware` from a JWT. No request body, query param, or path value contributes to the user ID. The `seriesReadStatusQuery` (`internal/series/store.go`) uses `WHERE user_id = ? AND book_id IN (...)` with `userID` as the first bound parameter — cross-user read data cannot leak. `GetDetail` skips the status query entirely when `userID == 0` (unauthenticated), defaulting all books to `UNREAD`. This is correct. **2. SQL parameterisation** All five new store functions (`BooksInSeries`, `SeriesBookFormats`, `SeriesTags`, `SeriesCategories`, `SeriesReadStatus`) build dynamic SQL via placeholder strings of `"?"` and pass user-sourced values only through the variadic `args ...any` slice — never via string interpolation. The `seriesName` filter is `WHERE bm.series_name = ?`, the `IN (...)` clause is constructed from integer IDs only. No injection vector found. **3. XSS via crafted metadata (html/template)** All metadata strings — `Detail.Name`, `Detail.Synopsis`, `Detail.FirstAuthor`, `Detail.Tags` items, `SeriesDetailBook.Title`, `SeriesDetailBook.ReadStatus`, `SeriesDetailBook.Format` — are rendered via `{{.Foo}}` inside `html/template` blocks, which applies context-sensitive auto-escaping. No `template.HTML`, `template.URL`, `template.JS`, or `safeHTML`-style bypasses appear in the new code. Chip/badge labels from tags and categories are emitted via `{{.}}` in text context — escaped. Synopsis in `<p>` — escaped. **4. URL construction and open-redirect risk** `SeriesLink` is computed server-side as `"/series/" + url.PathEscape(book.Series.Name)` — a relative path starting with `/`. No external host can be injected via the series name. `html/template` applies additional URL-context escaping when rendering `{{.SeriesLink}}` in an `href` attribute, so a crafted series name cannot produce a `javascript:` URL or redirect to a foreign host. `FirstAuthorID` is an `int64` from the database, rendered as `{{.Detail.FirstAuthorID}}` in a query string: `html/template` will HTML-entity-escape numeric values in attribute context (harmless; integers contain no special chars). **5. Inline style injection** `style="width:{{printf "%.1f" .ReadPercent}}%"` — `ReadPercent` is a `float64` computed by `readPercent()` from server-side integer counts, capped at 100. No user input reaches this value. **6. CoverHash in URL query params** `book_cover_hash` is always a 16-character lowercase hex FNV hash (see `cover/generate.go:coverHash`). Used as `?v=<hash>` cache-buster — no special characters possible; no injection surface. **7. Write surface** No new `POST`/`PUT`/`PATCH`/`DELETE` routes are introduced. The only new HTTP surface is `GET /series/{seriesName}` (read-only) and the additions to the existing `GET /books/{id}` response. **8. Library scoping consistency** `getDetail` passes `nil` for `userLibraryIDs`, so `BooksInSeries` applies no library filter. This is the same behaviour as the existing `GET /series` list (which also queries across all libraries without a per-user library filter). The library-scoping stub is consistent with the pre-existing posture and is noted as intentional in the PR description. Not a regression introduced by this PR. --- ### Findings None. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-09 13:16:41 +00:00

Author

Owner

Copy link

Security RE-Review (bot)

Security re-review of PR #454 (bead bookshelf-xm84.1) — series page full Grimmory parity.
Diff base: git merge-base origin/main origin/bd-bookshelf-xm84.1.

---

Focus areas checked

1. Per-user read_status/progress scoping (IDOR)

DetailHandler (internal/series/handler.go:201-206) derives userID exclusively from extractUser(r).ID, which reads ClaimsFromContext(r.Context()) set by AuthMiddleware from a JWT. No request body, query param, or path value contributes to the user ID. The seriesReadStatusQuery (internal/series/store.go) uses WHERE user_id = ? AND book_id IN (...) with userID as the first bound parameter — cross-user read data cannot leak. GetDetail skips the status query entirely when userID == 0 (unauthenticated), defaulting all books to UNREAD. This is correct.

2. SQL parameterisation

All five new store functions (BooksInSeries, SeriesBookFormats, SeriesTags, SeriesCategories, SeriesReadStatus) build dynamic SQL via placeholder strings of "?" and pass user-sourced values only through the variadic args ...any slice — never via string interpolation. The seriesName filter is WHERE bm.series_name = ?, the IN (...) clause is constructed from integer IDs only. No injection vector found.

3. XSS via crafted metadata (html/template)

All metadata strings — Detail.Name, Detail.Synopsis, Detail.FirstAuthor, Detail.Tags items, SeriesDetailBook.Title, SeriesDetailBook.ReadStatus, SeriesDetailBook.Format — are rendered via {{.Foo}} inside html/template blocks, which applies context-sensitive auto-escaping. No template.HTML, template.URL, template.JS, or safeHTML-style bypasses appear in the new code. Chip/badge labels from tags and categories are emitted via {{.}} in text context — escaped. Synopsis in <p> — escaped.

4. URL construction and open-redirect risk

SeriesLink is computed server-side as "/series/" + url.PathEscape(book.Series.Name) — a relative path starting with /. No external host can be injected via the series name. html/template applies additional URL-context escaping when rendering {{.SeriesLink}} in an href attribute, so a crafted series name cannot produce a javascript: URL or redirect to a foreign host.

FirstAuthorID is an int64 from the database, rendered as {{.Detail.FirstAuthorID}} in a query string: html/template will HTML-entity-escape numeric values in attribute context (harmless; integers contain no special chars).

5. Inline style injection

style="width:{{printf "%.1f" .ReadPercent}}%" — ReadPercent is a float64 computed by readPercent() from server-side integer counts, capped at 100. No user input reaches this value.

6. CoverHash in URL query params

book_cover_hash is always a 16-character lowercase hex FNV hash (see cover/generate.go:coverHash). Used as ?v=<hash> cache-buster — no special characters possible; no injection surface.

7. Write surface

No new POST/PUT/PATCH/DELETE routes are introduced. The only new HTTP surface is GET /series/{seriesName} (read-only) and the additions to the existing GET /books/{id} response.

8. Library scoping consistency

getDetail passes nil for userLibraryIDs, so BooksInSeries applies no library filter. This is the same behaviour as the existing GET /series list (which also queries across all libraries without a per-user library filter). The library-scoping stub is consistent with the pre-existing posture and is noted as intentional in the PR description. Not a regression introduced by this PR.

---

Findings

None.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security RE-Review (bot) Security re-review of PR #454 (bead bookshelf-xm84.1) — series page full Grimmory parity. Diff base: `git merge-base origin/main origin/bd-bookshelf-xm84.1`. --- ### Focus areas checked **1. Per-user read_status/progress scoping (IDOR)** `DetailHandler` (`internal/series/handler.go:201-206`) derives `userID` exclusively from `extractUser(r).ID`, which reads `ClaimsFromContext(r.Context())` set by `AuthMiddleware` from a JWT. No request body, query param, or path value contributes to the user ID. The `seriesReadStatusQuery` (`internal/series/store.go`) uses `WHERE user_id = ? AND book_id IN (...)` with `userID` as the first bound parameter — cross-user read data cannot leak. `GetDetail` skips the status query entirely when `userID == 0` (unauthenticated), defaulting all books to `UNREAD`. This is correct. **2. SQL parameterisation** All five new store functions (`BooksInSeries`, `SeriesBookFormats`, `SeriesTags`, `SeriesCategories`, `SeriesReadStatus`) build dynamic SQL via placeholder strings of `"?"` and pass user-sourced values only through the variadic `args ...any` slice — never via string interpolation. The `seriesName` filter is `WHERE bm.series_name = ?`, the `IN (...)` clause is constructed from integer IDs only. No injection vector found. **3. XSS via crafted metadata (html/template)** All metadata strings — `Detail.Name`, `Detail.Synopsis`, `Detail.FirstAuthor`, `Detail.Tags` items, `SeriesDetailBook.Title`, `SeriesDetailBook.ReadStatus`, `SeriesDetailBook.Format` — are rendered via `{{.Foo}}` inside `html/template` blocks, which applies context-sensitive auto-escaping. No `template.HTML`, `template.URL`, `template.JS`, or `safeHTML`-style bypasses appear in the new code. Chip/badge labels from tags and categories are emitted via `{{.}}` in text context — escaped. Synopsis in `<p>` — escaped. **4. URL construction and open-redirect risk** `SeriesLink` is computed server-side as `"/series/" + url.PathEscape(book.Series.Name)` — a relative path starting with `/`. No external host can be injected via the series name. `html/template` applies additional URL-context escaping when rendering `{{.SeriesLink}}` in an `href` attribute, so a crafted series name cannot produce a `javascript:` URL or redirect to a foreign host. `FirstAuthorID` is an `int64` from the database, rendered as `{{.Detail.FirstAuthorID}}` in a query string: `html/template` will HTML-entity-escape numeric values in attribute context (harmless; integers contain no special chars). **5. Inline style injection** `style="width:{{printf "%.1f" .ReadPercent}}%"` — `ReadPercent` is a `float64` computed by `readPercent()` from server-side integer counts, capped at 100. No user input reaches this value. **6. CoverHash in URL query params** `book_cover_hash` is always a 16-character lowercase hex FNV hash (see `cover/generate.go:coverHash`). Used as `?v=<hash>` cache-buster — no special characters possible; no injection surface. **7. Write surface** No new `POST`/`PUT`/`PATCH`/`DELETE` routes are introduced. The only new HTTP surface is `GET /series/{seriesName}` (read-only) and the additions to the existing `GET /books/{id}` response. **8. Library scoping consistency** `getDetail` passes `nil` for `userLibraryIDs`, so `BooksInSeries` applies no library filter. This is the same behaviour as the existing `GET /series` list (which also queries across all libraries without a per-user library filter). The library-scoping stub is consistent with the pre-existing posture and is noted as intentional in the PR description. Not a regression introduced by this PR. --- ### Findings None. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-xm84.1 from 51271088d5 to f2669901c1 2026-06-09 13:31:04 +00:00 Compare

zombor added 1 commit 2026-06-09 13:38:59 +00:00

fix(series): replace undefined --bg-page with --bg in series-book-row CSS 7b37b3d832

zombor commented 2026-06-09 13:49:12 +00:00

Author

Owner

Copy link

Security Review — PR #454 (bookshelf-xm84.1)

Reviewed the diff only (no test re-run; CI is source of truth). Focus: multi-user library scoping, SQLi, XSS, IDOR, unbounded queries.

[BLOCKER] internal/series/handler.go:200 (DetailHandler) — series detail page is NOT scoped to the user's accessible libraries
getDetail(r.Context(), seriesName, nil, userID) passes nil for userLibraryIDs, so BooksInSeries runs with the b.library_id IN (...) filter disabled. The series detail page (Books grid + Book List tab, aggregate metadata, synopsis, ratings, read-progress) therefore enumerates every book in the series across ALL libraries, regardless of which the requesting user can access. This is a cross-library data leak on a brand-new user-facing endpoint, and it also violates the fail-closed contract in users.GetUserLibraryIDs (an authenticated user with no library mappings must see nothing, but here sees the whole series). The scoping infrastructure already exists and is used by books.ListHandler (internal/books/handler.go:150: resolve getUserLibraryIDs(ctx, userID) once, thread it through). Fix: inject getUserLibraryIDs func(ctx, int64) ([]int64, error) into DetailHandler, resolve the user's library IDs (fail-closed on empty for an authenticated user), and pass them instead of nil. The store's library-filter plumbing (store.go BooksInSeries) is correct and parameterized — only the handler wiring is missing.

[MAJOR] internal/books/handler.go:911 (Show) — "More in Series" grid is NOT user-library-scoped
listSeriesBooks(r.Context(), book.Series.Name, nil) passes nil for userLibraryIDs, so the More-in-Series tab on the book detail page lists sibling books from libraries the user cannot access (same root cause as the BLOCKER, smaller surface — covers/titles only). Resolve and pass the user's accessible library IDs here too. Lower severity only because the book-detail page itself is access-checked and the leaked fields are limited, but it is the same unscoped-per-user-surface class and must be fixed in this PR.

[MINOR] e2e/api/series_test.go (whole-file) — no negative scoping assertion
Every test maps its library to user 1 (user_library_mapping (user_id, library_id) VALUES (1, ?)) and none asserts that books in an UNmapped library are excluded from the series detail / Book List. After fixing the scoping, add a test that seeds a book in a library NOT mapped to the requesting user and asserts it does not appear in SeriesDetail.Books.

Verified clean:

• SQLi: all new queries (BooksInSeries window query, SeriesBookFormats, SeriesTags, SeriesCategories, SeriesReadStatus, SeriesAllAuthors) are fully parameterized; placeholders are generated from slice length and bound via args — no user input is string-concatenated. The {{libraryFilter}} and %s substitutions inject only ? placeholders, never values.
• XSS: series_show.html uses only standard {{.Field}} interpolation under html/template auto-escaping (series name, author names, synopsis, tags/chips, titles, formats). No template.HTML, no safe/js/attr pipelines, no unescaped output. urlpath(=url.PathEscape) is used only for URL path segments, correctly.
• IDOR: seriesName comes from r.PathValue and userID from extractUser(r) (session) — never from body/param/header. More-in-Series uses server-side book.Series.Name. Read-status forms POST to the pre-existing scoped /books/{id}/read-status (userID from session, status from a fixed hidden field).
• Bounded queries: BooksInSeries has LIMIT 200; the IN(...) enrichment queries are bounded by that 200-row book set.

REVIEW VERDICT: 1 blocker, 1 major, 1 minor

## Security Review — PR #454 (bookshelf-xm84.1) Reviewed the diff only (no test re-run; CI is source of truth). Focus: multi-user library scoping, SQLi, XSS, IDOR, unbounded queries. [BLOCKER] internal/series/handler.go:200 (DetailHandler) — series detail page is NOT scoped to the user's accessible libraries `getDetail(r.Context(), seriesName, nil, userID)` passes `nil` for `userLibraryIDs`, so `BooksInSeries` runs with the `b.library_id IN (...)` filter disabled. The series detail page (Books grid + Book List tab, aggregate metadata, synopsis, ratings, read-progress) therefore enumerates every book in the series across ALL libraries, regardless of which the requesting user can access. This is a cross-library data leak on a brand-new user-facing endpoint, and it also violates the fail-closed contract in `users.GetUserLibraryIDs` (an authenticated user with no library mappings must see nothing, but here sees the whole series). The scoping infrastructure already exists and is used by `books.ListHandler` (`internal/books/handler.go:150`: resolve `getUserLibraryIDs(ctx, userID)` once, thread it through). Fix: inject `getUserLibraryIDs func(ctx, int64) ([]int64, error)` into `DetailHandler`, resolve the user's library IDs (fail-closed on empty for an authenticated user), and pass them instead of `nil`. The store's library-filter plumbing (`store.go` BooksInSeries) is correct and parameterized — only the handler wiring is missing. [MAJOR] internal/books/handler.go:911 (Show) — "More in Series" grid is NOT user-library-scoped `listSeriesBooks(r.Context(), book.Series.Name, nil)` passes `nil` for `userLibraryIDs`, so the More-in-Series tab on the book detail page lists sibling books from libraries the user cannot access (same root cause as the BLOCKER, smaller surface — covers/titles only). Resolve and pass the user's accessible library IDs here too. Lower severity only because the book-detail page itself is access-checked and the leaked fields are limited, but it is the same unscoped-per-user-surface class and must be fixed in this PR. [MINOR] e2e/api/series_test.go (whole-file) — no negative scoping assertion Every test maps its library to user 1 (`user_library_mapping (user_id, library_id) VALUES (1, ?)`) and none asserts that books in an UNmapped library are excluded from the series detail / Book List. After fixing the scoping, add a test that seeds a book in a library NOT mapped to the requesting user and asserts it does not appear in `SeriesDetail.Books`. Verified clean: - SQLi: all new queries (BooksInSeries window query, SeriesBookFormats, SeriesTags, SeriesCategories, SeriesReadStatus, SeriesAllAuthors) are fully parameterized; placeholders are generated from slice length and bound via args — no user input is string-concatenated. The `{{libraryFilter}}` and `%s` substitutions inject only `?` placeholders, never values. - XSS: series_show.html uses only standard `{{.Field}}` interpolation under html/template auto-escaping (series name, author names, synopsis, tags/chips, titles, formats). No `template.HTML`, no `safe`/`js`/`attr` pipelines, no unescaped output. `urlpath`(=`url.PathEscape`) is used only for URL path segments, correctly. - IDOR: `seriesName` comes from `r.PathValue` and `userID` from `extractUser(r)` (session) — never from body/param/header. More-in-Series uses server-side `book.Series.Name`. Read-status forms POST to the pre-existing scoped `/books/{id}/read-status` (userID from session, status from a fixed hidden field). - Bounded queries: BooksInSeries has `LIMIT 200`; the IN(...) enrichment queries are bounded by that 200-row book set. REVIEW VERDICT: 1 blocker, 1 major, 1 minor

zombor reviewed 2026-06-09 13:50:26 +00:00

zombor left a comment

Copy link

[BLOCKER] Multi-user library scoping missing

[BLOCKER] Multi-user library scoping missing

zombor reviewed 2026-06-09 13:50:41 +00:00

zombor left a comment

Copy link

No description provided.

## FINAL SECURITY RE-REVIEW — PR #426 (bookshelf-jmn7.3) Head: a182d584 · Branch: bd-bookshelf-jmn7.3 · High-risk surface: file upload. --- ### Checklist: prior security fixes **1. BODY-SIZE / DoS — path-based cap** (a) **Forged multipart Content-Type on non-upload endpoints: CLOSED.** `isUploadPath` requires POST + path suffix `/files` + prefix `/books/`. No other registered route matches. A forged `Content-Type: multipart/form-data` on `/shelves`, `/books/bulk/delete`, or any other POST is still capped at 1 MB. Verified by `body_cap_chain_test.go`. (b) **Upload route exempt from global 1 MB cap so handler cap is effective: CONFIRMED.** Handler applies `http.MaxBytesReader(w, r.Body, MaxUploadBytes+4096)` (~2 GB). The handler cap is the binding limit. (c) **Over-cap upload rejected (413): CONFIRMED.** Within the service, `io.LimitReader(full, MaxUploadBytes+1)` + `written > MaxUploadBytes` check returns `ErrFileTooLarge` mapped to 413. Two-layer enforcement. **Chain trace: MaxBytes -> Auth -> CSRF -> MethodOverride -> handler** - MaxBytes: exempts `POST /books/{id}/files` path-based, not Content-Type-based. ✅ - CSRF: for multipart, `csrfTokenFromRequest` checks `X-CSRF-Token` header first; `ParseForm` on multipart only parses URL query params, does NOT read the uncapped body. ✅ - MethodOverride: no ParseMultipartForm; checks URL `?_method` then url-encoded body only. ✅ - Handler: `SetReadDeadline(+10min)` called BEFORE `MaxBytesReader` and `ParseMultipartForm`. ✅ **2. READ TIMEOUT — bounded, upload-scoped: CONFIRMED.** `SetReadDeadline(now + 10min)` is finite. Non-upload routes retain the server 5s ReadTimeout. Failure is Warn-logged and non-fatal. **3. OWNERSHIP — no existence leak: CONFIRMED.** `GetBookForUpload` SQL JOINs `user_library_mapping` on `ulm.user_id = ?`. Book in unmapped library → `sql.ErrNoRows` → 404. `userID` from JWT session only. E2E test covers the inaccessible-book case. **4. CBX byte-sniff: CONFIRMED SAFE.** `io.ReadFull` reads exactly `archiveHeaderSize=16` bytes (compile-time constant). `io.MultiReader` reconstructs the full stream. `archive_type` derived from magic bytes, not from any attacker-supplied field. **5. Path traversal, format gating, CSRF, SQL, XSS: CONFIRMED SAFE.** - `sanitizeFilename`: `filepath.Base` + `LastIndexAny` strips all path components. Rejects empty/dotfiles/traversal names. - `resolveDestDir`: independent `HasPrefix(Clean(parent), Clean(libraryRoot))` check; falls back to `libraryRoot` on escape. - Extension gating on sanitized extension before any I/O; exe/sh absent from `allowedFormats`. - CSRF: JS controller sends `X-CSRF-Token` header. CSRF middleware checks header first. - SQL: `GetBookForUpload` and `UpsertBookFile` use `?` placeholders, sqlc-generated. No string interpolation. - XSS: `fh.Filename` logged via structured slog; never reflected in JSON or HTML response. --- ### New findings this round [MINOR] internal/books/upload_service.go:248 — `sanitizeFilename` does not reject filenames containing null bytes `sanitizeFilename("evil\x00.epub")` returns `("evil\x00.epub", nil)`. On Linux, `os.Rename` with a null byte in the destination path is treated as a C string by the kernel — truncated at `\x00`, so the file lands on disk as `"evil"` while the `book_file` row records `file_name="evil\x00.epub"`. Result: DB-to-filesystem mismatch. No path traversal, no privilege escalation; requires an authenticated user with upload access. Fix: add `strings.ContainsRune(name, 0)` check returning `ErrInvalidFilename` before `filepath.Base`. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor reviewed 2026-06-09 13:50:54 +00:00

zombor left a comment

Copy link

No description provided.

## FINAL SECURITY RE-REVIEW — PR #426 (bookshelf-jmn7.3) Head: a182d584 · Branch: bd-bookshelf-jmn7.3 · High-risk surface: file upload. --- ### Checklist: prior security fixes **1. BODY-SIZE / DoS — path-based cap** (a) **Forged multipart Content-Type on non-upload endpoints: CLOSED.** `isUploadPath` requires POST + path suffix `/files` + prefix `/books/`. No other registered route matches. A forged `Content-Type: multipart/form-data` on `/shelves`, `/books/bulk/delete`, or any other POST is still capped at 1 MB. Verified by `body_cap_chain_test.go`. (b) **Upload route exempt from global 1 MB cap so handler cap is effective: CONFIRMED.** Handler applies `http.MaxBytesReader(w, r.Body, MaxUploadBytes+4096)` (~2 GB). The handler cap is the binding limit. (c) **Over-cap upload rejected (413): CONFIRMED.** Within the service, `io.LimitReader(full, MaxUploadBytes+1)` + `written > MaxUploadBytes` check returns `ErrFileTooLarge` mapped to 413. Two-layer enforcement. **Chain trace: MaxBytes -> Auth -> CSRF -> MethodOverride -> handler** - MaxBytes: exempts `POST /books/{id}/files` path-based, not Content-Type-based. ✅ - CSRF: for multipart, `csrfTokenFromRequest` checks `X-CSRF-Token` header first; `ParseForm` on multipart only parses URL query params, does NOT read the uncapped body. ✅ - MethodOverride: no ParseMultipartForm; checks URL `?_method` then url-encoded body only. ✅ - Handler: `SetReadDeadline(+10min)` called BEFORE `MaxBytesReader` and `ParseMultipartForm`. ✅ **2. READ TIMEOUT — bounded, upload-scoped: CONFIRMED.** `SetReadDeadline(now + 10min)` is finite. Non-upload routes retain the server 5s ReadTimeout. Failure is Warn-logged and non-fatal. **3. OWNERSHIP — no existence leak: CONFIRMED.** `GetBookForUpload` SQL JOINs `user_library_mapping` on `ulm.user_id = ?`. Book in unmapped library → `sql.ErrNoRows` → 404. `userID` from JWT session only. E2E test covers the inaccessible-book case. **4. CBX byte-sniff: CONFIRMED SAFE.** `io.ReadFull` reads exactly `archiveHeaderSize=16` bytes (compile-time constant). `io.MultiReader` reconstructs the full stream. `archive_type` derived from magic bytes, not from any attacker-supplied field. **5. Path traversal, format gating, CSRF, SQL, XSS: CONFIRMED SAFE.** - `sanitizeFilename`: `filepath.Base` + `LastIndexAny` strips all path components. Rejects empty/dotfiles/traversal names. - `resolveDestDir`: independent `HasPrefix(Clean(parent), Clean(libraryRoot))` check; falls back to `libraryRoot` on escape. - Extension gating on sanitized extension before any I/O; exe/sh absent from `allowedFormats`. - CSRF: JS controller sends `X-CSRF-Token` header. CSRF middleware checks header first. - SQL: `GetBookForUpload` and `UpsertBookFile` use `?` placeholders, sqlc-generated. No string interpolation. - XSS: `fh.Filename` logged via structured slog; never reflected in JSON or HTML response. --- ### New findings this round [MINOR] internal/books/upload_service.go:248 — `sanitizeFilename` does not reject filenames containing null bytes `sanitizeFilename("evil\x00.epub")` returns `("evil\x00.epub", nil)`. On Linux, `os.Rename` with a null byte in the destination path is treated as a C string by the kernel — truncated at `\x00`, so the file lands on disk as `"evil"` while the `book_file` row records `file_name="evil\x00.epub"`. Result: DB-to-filesystem mismatch. No path traversal, no privilege escalation; requires an authenticated user with upload access. Fix: add `strings.ContainsRune(name, 0)` check returning `ErrInvalidFilename` before `filepath.Base`. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor force-pushed bd-bookshelf-xm84.1 from 7b37b3d832 to ef5d4ad8fd 2026-06-09 14:04:11 +00:00 Compare

zombor added 1 commit 2026-06-09 16:51:09 +00:00

fix(e2e): remove duplicate adminID declaration in seedRichSeries (xm84.1) ... 8dd9920a9d

adminID was declared twice in the same function scope (lines 91 and 177),
causing a compile error in the e2e/api suite. Remove the redundant second
declaration and Scan — adminID is already populated from the first query.

zombor referenced this pull request 2026-06-09 16:51:55 +00:00

feat(authors): author detail page + book-page author links (bookshelf-37ef.1) #455

zombor commented 2026-06-09 16:59:27 +00:00

Author

Owner

Copy link

RE-REVIEW: Library-Scoping Fix Verification

All three blockers from the prior code review have been RESOLVED:

[RESOLVED] series DetailHandler now resolves userLibraryIDs fail-closed

• Location: internal/series/handler.go:180-195
• Fix: Calls getUserLibraryIDs(r.Context(), userID) when userID != 0
• Authenticated user with no library mappings gets empty slice → AND 1=0 filter in BooksInSeries ✓

[RESOLVED] books.Show "More in Series" now passes scoped userLibraryIDs

• Location: internal/books/handler.go:899-925
• Fix: Resolves showLibraryIDs from getUserLibraryIDs(r.Context(), uid), passes to listSeriesBooks

[RESOLVED] SeriesAllAuthors query now applies library filter

• Location: internal/series/store.go:SeriesAllAuthors()
• Fix: Injects {{libraryFilter}} → AND b.library_id IN (...) at runtime

[RESOLVED] Negative-scoping test added

• Location: e2e/api/series_test.go:912–1084
• Confirms books AND authors excluded from inaccessible library ✓

---

REVIEW VERDICT: 0 blockers, 0 majors, 0 minors

The fix is correct and complete. Ready for merge.

## RE-REVIEW: Library-Scoping Fix Verification All three **blockers** from the prior code review have been **RESOLVED**: ### [RESOLVED] series DetailHandler now resolves userLibraryIDs fail-closed - **Location:** `internal/series/handler.go:180-195` - **Fix:** Calls `getUserLibraryIDs(r.Context(), userID)` when `userID != 0` - Authenticated user with no library mappings gets empty slice → AND 1=0 filter in BooksInSeries ✓ ### [RESOLVED] books.Show "More in Series" now passes scoped userLibraryIDs - **Location:** `internal/books/handler.go:899-925` - **Fix:** Resolves `showLibraryIDs` from `getUserLibraryIDs(r.Context(), uid)`, passes to `listSeriesBooks` ### [RESOLVED] SeriesAllAuthors query now applies library filter - **Location:** `internal/series/store.go:SeriesAllAuthors()` - **Fix:** Injects `{{libraryFilter}}` → `AND b.library_id IN (...)` at runtime ### [RESOLVED] Negative-scoping test added - **Location:** `e2e/api/series_test.go:912–1084` - Confirms books AND authors excluded from inaccessible library ✓ --- **REVIEW VERDICT: 0 blockers, 0 majors, 0 minors** The fix is correct and complete. Ready for merge.

zombor merged commit 1fa1c7578b into main 2026-06-09 17:17:25 +00:00

zombor referenced this pull request from a commit 2026-06-09 17:17:26 +00:00

Merge pull request 'feat(series): series detail page + book-page series nav (bookshelf-xm84.1)' (#454) from bd-bookshelf-xm84.1 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!454

No description provided.