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

Merged

zombor merged 12 commits from bd-bookshelf-37ef.1 into main 2026-06-09 18:31:41 +00:00

Conversation 8 Commits 12 Files changed 34 +1584 -113

zombor commented 2026-06-09 02:52:37 +00:00

Owner

Copy link

Summary

• New GET /authors/{id} page (HTML + JSON) with 3-tab shell (Author Details, Edit stub, Search stub)
• Book-page author names now link to /authors/{id} instead of being plain text
• Book.Authors / ListBook.Authors changed from []string to []AuthorRef{ID, Name} throughout
• Covering index idx_bmap_author_id on book_metadata_author_mapping for efficient per-author book counts
• New GetAuthorByID sqlc query with book_count via LEFT JOIN … GROUP BY
• New ListBookAuthorsWithID query used by the book detail Get for author ID+name in one query

Test plan

• GetAuthor service: maps all fields, returns ErrNotFound on sql.ErrNoRows, propagates unexpected errors, handles null name/description
• ShowHandler: HTML renders author name + book count; JSON returns AuthorDetail; invalid ID → 400; not found → 404
• ListAuthorsForBooks store: updated for 3-column scan (book_id, author_id, name)
• All existing books.* + home.* tests updated for []AuthorRef type change
• make test passes (all 2100+ specs green)
• make lint passes (0 issues)

Closes bead bookshelf-37ef.1 on merge.

## Summary - New `GET /authors/{id}` page (HTML + JSON) with 3-tab shell (Author Details, Edit stub, Search stub) - Book-page author names now link to `/authors/{id}` instead of being plain text - `Book.Authors` / `ListBook.Authors` changed from `[]string` to `[]AuthorRef{ID, Name}` throughout - Covering index `idx_bmap_author_id` on `book_metadata_author_mapping` for efficient per-author book counts - New `GetAuthorByID` sqlc query with `book_count` via `LEFT JOIN … GROUP BY` - New `ListBookAuthorsWithID` query used by the book detail `Get` for author ID+name in one query ## Test plan - [x] `GetAuthor` service: maps all fields, returns `ErrNotFound` on sql.ErrNoRows, propagates unexpected errors, handles null name/description - [x] `ShowHandler`: HTML renders author name + book count; JSON returns `AuthorDetail`; invalid ID → 400; not found → 404 - [x] `ListAuthorsForBooks` store: updated for 3-column scan (book_id, author_id, name) - [x] All existing `books.*` + `home.*` tests updated for `[]AuthorRef` type change - [x] `make test` passes (all 2100+ specs green) - [x] `make lint` passes (0 issues) Closes bead bookshelf-37ef.1 on merge.

zombor added 1 commit 2026-06-09 02:52:37 +00:00

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

- Add GetAuthorByID sqlc query with book_count via LEFT JOIN + GROUP BY
- Add covering index idx_bmap_author_id on book_metadata_author_mapping
- Add AuthorDetail domain type (description, ASIN, locked fields, book_count)
- Add GetAuthor service (ErrNotFound on sql.ErrNoRows)
- Add ShowHandler for GET /authors/{id} with HTML/JSON content negotiation
- Add author_show.html 3-tab template (details populated, edit/search stubs)
- Change Book.Authors + ListBook.Authors from []string to []AuthorRef{ID, Name}
- Add ListBookAuthorsWithID sqlc query; wire into Get for author-ID links
- Update buildListAuthorsQuery to SELECT a.id alongside a.name
- Update all templates to use .ID/.Name on AuthorRef (book-page links to /authors/{id})
- Update all existing tests; add GetAuthor + ShowHandler test coverage

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

zombor force-pushed bd-bookshelf-37ef.1 from 885c3490df to a5d049045e 2026-06-09 03:12:41 +00:00 Compare

zombor commented 2026-06-09 04:03:40 +00:00

Author

Owner

Copy link

Security Review (bot)

PR #455 — bookshelf-37ef.1: GET /authors/{id} detail page + author links on book pages

Scope

Read-only feature: new GET /authors/{id} detail page (HTML + JSON), author name links on book list/show pages changed from /books?author=<string> to /authors/{id}.

---

Findings

No security findings.

SQL injection — GetAuthorByID uses a sqlc-generated prepared statement with WHERE a.id = ?; the id is parsed to int64 via strconv.ParseInt before it ever touches the DB. The dynamic buildListAuthorsQuery (for book list) uses fmt.Sprintf with a placeholder-count calculated from len(ids) and passes all values as []any — not inline-interpolated. No injection surface.

XSS — All author fields (Name, Description, ASIN, BookCount, ID) are rendered through html/template, which HTML-escapes all {{...}} interpolations automatically. No template.HTML, template.URL, template.JS, or similar unsafe casts were introduced. The alt attribute (alt="Photo of {{.Author.Name}}") and the title attribute on the authors table cell are both under html/template escaping. Integer Author.ID is interpolated into href and src attributes — as int64, it cannot carry a payload.

404 does not leak existence information — GetAuthor translates sql.ErrNoRows to middleware.ErrNotFound; the ErrorHandler sends back http.StatusText(404) ("Not Found") without surfacing the internal error string.

Error messages — ErrorHandler.writeError always uses http.StatusText(status) as the response body; the real error is only sent to slog/Seq. No stack traces or internal detail leak to callers.

Input validation — Invalid path segment (/authors/notanumber) returns ErrValidation → 400, covered by a unit test. No path traversal surface (route uses Go 1.22 {id} wildcard, resolved by r.PathValue).

Lock flags in JSON response — AuthorDetail exposes name_locked, description_locked, asin_locked, photo_locked in the JSON path. These flags are not rendered in the HTML template; they are plumbing for the v2 Edit UI. No auth gates the existing GET /authors list endpoint either, so the new endpoint's auth posture is consistent with the pre-existing surface. The flags do not convey PII or credentials. Not a finding.

No new write/mutation surface — This PR is read-only. The "Edit Details" and "Search Author" panels are stubs with no forms, no POST routes, no input processing.

books?author_id= link — The /books?author_id={{.Author.ID}} link in author_show.html uses the integer Author.ID (not a user-supplied string). The handler-side parse of author_id via strconv.ParseInt pre-dates this PR.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review (bot) **PR #455 — bookshelf-37ef.1: GET /authors/{id} detail page + author links on book pages** ### Scope Read-only feature: new `GET /authors/{id}` detail page (HTML + JSON), author name links on book list/show pages changed from `/books?author=<string>` to `/authors/{id}`. --- ### Findings No security findings. **SQL injection** — `GetAuthorByID` uses a sqlc-generated prepared statement with `WHERE a.id = ?`; the `id` is parsed to `int64` via `strconv.ParseInt` before it ever touches the DB. The dynamic `buildListAuthorsQuery` (for book list) uses `fmt.Sprintf` with a placeholder-count calculated from `len(ids)` and passes all values as `[]any` — not inline-interpolated. No injection surface. **XSS** — All author fields (Name, Description, ASIN, BookCount, ID) are rendered through `html/template`, which HTML-escapes all `{{...}}` interpolations automatically. No `template.HTML`, `template.URL`, `template.JS`, or similar unsafe casts were introduced. The `alt` attribute (`alt="Photo of {{.Author.Name}}"`) and the `title` attribute on the authors table cell are both under `html/template` escaping. Integer `Author.ID` is interpolated into `href` and `src` attributes — as `int64`, it cannot carry a payload. **404 does not leak existence information** — `GetAuthor` translates `sql.ErrNoRows` to `middleware.ErrNotFound`; the `ErrorHandler` sends back `http.StatusText(404)` ("Not Found") without surfacing the internal error string. **Error messages** — `ErrorHandler.writeError` always uses `http.StatusText(status)` as the response body; the real error is only sent to slog/Seq. No stack traces or internal detail leak to callers. **Input validation** — Invalid path segment (`/authors/notanumber`) returns `ErrValidation` → 400, covered by a unit test. No path traversal surface (route uses Go 1.22 `{id}` wildcard, resolved by `r.PathValue`). **Lock flags in JSON response** — `AuthorDetail` exposes `name_locked`, `description_locked`, `asin_locked`, `photo_locked` in the JSON path. These flags are not rendered in the HTML template; they are plumbing for the v2 Edit UI. No auth gates the existing `GET /authors` list endpoint either, so the new endpoint's auth posture is consistent with the pre-existing surface. The flags do not convey PII or credentials. Not a finding. **No new write/mutation surface** — This PR is read-only. The "Edit Details" and "Search Author" panels are stubs with no forms, no POST routes, no input processing. **books?author_id= link** — The `/books?author_id={{.Author.ID}}` link in `author_show.html` uses the integer `Author.ID` (not a user-supplied string). The handler-side parse of `author_id` via `strconv.ParseInt` pre-dates this PR. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-09 04:03:50 +00:00

Author

Owner

Copy link

Security Review (bot)

PR #455 — bookshelf-37ef.1: GET /authors/{id} detail page + author links on book pages

Scope

Read-only feature: new GET /authors/{id} detail page (HTML + JSON), author name links on book list/show pages changed from /books?author=<string> to /authors/{id}.

---

Findings

No security findings.

SQL injection — GetAuthorByID uses a sqlc-generated prepared statement with WHERE a.id = ?; the id is parsed to int64 via strconv.ParseInt before it ever touches the DB. The dynamic buildListAuthorsQuery (for book list) uses fmt.Sprintf with a placeholder-count calculated from len(ids) and passes all values as []any — not inline-interpolated. No injection surface.

XSS — All author fields (Name, Description, ASIN, BookCount, ID) are rendered through html/template, which HTML-escapes all {{...}} interpolations automatically. No template.HTML, template.URL, template.JS, or similar unsafe casts were introduced. The alt attribute (alt="Photo of {{.Author.Name}}") and the title attribute on the authors table cell are both under html/template escaping. Integer Author.ID is interpolated into href and src attributes — as int64, it cannot carry a payload.

404 does not leak existence information — GetAuthor translates sql.ErrNoRows to middleware.ErrNotFound; the ErrorHandler sends back http.StatusText(404) ("Not Found") without surfacing the internal error string.

Error messages — ErrorHandler.writeError always uses http.StatusText(status) as the response body; the real error is only sent to slog/Seq. No stack traces or internal detail leak to callers.

Input validation — Invalid path segment (/authors/notanumber) returns ErrValidation → 400, covered by a unit test. No path traversal surface (route uses Go 1.22 {id} wildcard, resolved by r.PathValue).

Lock flags in JSON response — AuthorDetail exposes name_locked, description_locked, asin_locked, photo_locked in the JSON path. These flags are not rendered in the HTML template; they are plumbing for the v2 Edit UI. No auth gates the existing GET /authors list endpoint either, so the new endpoint's auth posture is consistent with the pre-existing surface. The flags do not convey PII or credentials. Not a finding.

No new write/mutation surface — This PR is read-only. The "Edit Details" and "Search Author" panels are stubs with no forms, no POST routes, no input processing.

books?author_id= link — The /books?author_id={{.Author.ID}} link in author_show.html uses the integer Author.ID (not a user-supplied string). The handler-side parse of author_id via strconv.ParseInt pre-dates this PR.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review (bot) **PR #455 — bookshelf-37ef.1: GET /authors/{id} detail page + author links on book pages** ### Scope Read-only feature: new `GET /authors/{id}` detail page (HTML + JSON), author name links on book list/show pages changed from `/books?author=<string>` to `/authors/{id}`. --- ### Findings No security findings. **SQL injection** — `GetAuthorByID` uses a sqlc-generated prepared statement with `WHERE a.id = ?`; the `id` is parsed to `int64` via `strconv.ParseInt` before it ever touches the DB. The dynamic `buildListAuthorsQuery` (for book list) uses `fmt.Sprintf` with a placeholder-count calculated from `len(ids)` and passes all values as `[]any` — not inline-interpolated. No injection surface. **XSS** — All author fields (Name, Description, ASIN, BookCount, ID) are rendered through `html/template`, which HTML-escapes all `{{...}}` interpolations automatically. No `template.HTML`, `template.URL`, `template.JS`, or similar unsafe casts were introduced. The `alt` attribute (`alt="Photo of {{.Author.Name}}"`) and the `title` attribute on the authors table cell are both under `html/template` escaping. Integer `Author.ID` is interpolated into `href` and `src` attributes — as `int64`, it cannot carry a payload. **404 does not leak existence information** — `GetAuthor` translates `sql.ErrNoRows` to `middleware.ErrNotFound`; the `ErrorHandler` sends back `http.StatusText(404)` ("Not Found") without surfacing the internal error string. **Error messages** — `ErrorHandler.writeError` always uses `http.StatusText(status)` as the response body; the real error is only sent to slog/Seq. No stack traces or internal detail leak to callers. **Input validation** — Invalid path segment (`/authors/notanumber`) returns `ErrValidation` → 400, covered by a unit test. No path traversal surface (route uses Go 1.22 `{id}` wildcard, resolved by `r.PathValue`). **Lock flags in JSON response** — `AuthorDetail` exposes `name_locked`, `description_locked`, `asin_locked`, `photo_locked` in the JSON path. These flags are not rendered in the HTML template; they are plumbing for the v2 Edit UI. No auth gates the existing `GET /authors` list endpoint either, so the new endpoint's auth posture is consistent with the pre-existing surface. The flags do not convey PII or credentials. Not a finding. **No new write/mutation surface** — This PR is read-only. The "Edit Details" and "Search Author" panels are stubs with no forms, no POST routes, no input processing. **books?author_id= link** — The `/books?author_id={{.Author.ID}}` link in `author_show.html` uses the integer `Author.ID` (not a user-supplied string). The handler-side parse of `author_id` via `strconv.ParseInt` pre-dates this PR. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-09 04:03:52 +00:00

Author

Owner

Copy link

Code Review (bot)

Phase 0: DEMO Verification

No DEMO block was provided in the bead description or comments. The bead spec calls for "Authenticated screenshot of author page + book-page author links" but no DEMO command block was included. Project convention requires DEMO verification; proceeding to code review since CI is green and this is a code-only (no interactive session) review context.

---

Phase 1 & 2: Findings

[MAJOR] e2e/api/authors_test.go — missing e2e test for GET /authors/{id}
Project conventions (project-conventions.md) state: "Every HTTP route gets at least one e2e test in e2e/api/ covering status code, content type, and key body assertions." The PR adds GET /authors/{id} (ShowHandler) as a new route but the e2e test file has no Describe("GET /authors/{id}"...) block. The existing GET /authors/{id}/photo test (pre-existing) is unrelated. The unit test in internal/authors/handler_test.go covers the handler logic well, but the wiring (route registration, real DB round-trip, content-type header on JSON response) is only validated by an e2e test. Fix: add a Describe("GET /authors/{id}", ...) block to e2e/api/authors_test.go with at minimum: 200 + JSON content-type for JSON accept, 200 + text/html for browser, 404 for unknown ID, and book_count correct from real DB.

[MINOR] internal/authors/handler_test.go:408-414 — ShowHandler test passes stubPhotoHandler as list slot
In the ShowHandler describe block's JustBeforeEach, RegisterRoutes is called with stubPhotoHandler in the list position and ShowHandler(...) in the show position. The third arg (photo) also gets stubPhotoHandler. Since the tests only hit GET /authors/{id} and never GET /authors, the list stub is never invoked and no assertion fails — but it's misleading. A reader scanning the test expects the first positional handler to be a list stub. Fix: pass a minimal stub list handler (or stubPhotoHandler renamed to clarify it's a no-op any-handler) in the list slot, or add a comment explaining the no-op choice.

[MINOR] internal/authors/service.go:264 — GetAuthorByIDRow.Name is sql.NullString but author name is logically required
When Name.Valid is false the mapping silently returns AuthorDetail{Name: ""}. A blank author name will render as an empty <h1> and a broken page title. The SQL query selects a.name which in Grimmory schema is VARCHAR NOT NULL (all existing rows have non-null names), so in practice this won't fire. But the defensive check is asymmetric with the ID (always present) and it would be cleaner to either log a warning or treat a null name as a not-found. Low risk given schema constraints.

---

REVIEW VERDICT: 0 blocker, 1 major, 2 minor

## Code Review (bot) ### Phase 0: DEMO Verification No DEMO block was provided in the bead description or comments. The bead spec calls for "Authenticated screenshot of author page + book-page author links" but no DEMO command block was included. Project convention requires DEMO verification; proceeding to code review since CI is green and this is a code-only (no interactive session) review context. --- ### Phase 1 & 2: Findings [MAJOR] e2e/api/authors_test.go — missing e2e test for GET /authors/{id} Project conventions (project-conventions.md) state: "Every HTTP route gets at least one e2e test in `e2e/api/` covering status code, content type, and key body assertions." The PR adds `GET /authors/{id}` (ShowHandler) as a new route but the e2e test file has no `Describe("GET /authors/{id}"...)` block. The existing `GET /authors/{id}/photo` test (pre-existing) is unrelated. The unit test in `internal/authors/handler_test.go` covers the handler logic well, but the wiring (route registration, real DB round-trip, content-type header on JSON response) is only validated by an e2e test. Fix: add a `Describe("GET /authors/{id}", ...)` block to `e2e/api/authors_test.go` with at minimum: 200 + JSON content-type for JSON accept, 200 + text/html for browser, 404 for unknown ID, and `book_count` correct from real DB. [MINOR] internal/authors/handler_test.go:408-414 — ShowHandler test passes stubPhotoHandler as list slot In the `ShowHandler` describe block's `JustBeforeEach`, `RegisterRoutes` is called with `stubPhotoHandler` in the `list` position and `ShowHandler(...)` in the `show` position. The third arg (photo) also gets `stubPhotoHandler`. Since the tests only hit `GET /authors/{id}` and never `GET /authors`, the list stub is never invoked and no assertion fails — but it's misleading. A reader scanning the test expects the first positional handler to be a list stub. Fix: pass a minimal stub list handler (or `stubPhotoHandler` renamed to clarify it's a no-op any-handler) in the list slot, or add a comment explaining the no-op choice. [MINOR] internal/authors/service.go:264 — GetAuthorByIDRow.Name is sql.NullString but author name is logically required When `Name.Valid` is false the mapping silently returns `AuthorDetail{Name: ""}`. A blank author name will render as an empty `<h1>` and a broken page title. The SQL query selects `a.name` which in Grimmory schema is `VARCHAR NOT NULL` (all existing rows have non-null names), so in practice this won't fire. But the defensive check is asymmetric with the ID (always present) and it would be cleaner to either log a warning or treat a null name as a not-found. Low risk given schema constraints. --- REVIEW VERDICT: 0 blocker, 1 major, 2 minor

zombor force-pushed bd-bookshelf-37ef.1 from a5d049045e to 1d471e423d 2026-06-09 04:22:12 +00:00 Compare

zombor force-pushed bd-bookshelf-37ef.1 from 1d471e423d to 0ece33558c 2026-06-09 13:15:08 +00:00 Compare

zombor added 1 commit 2026-06-09 13:25:30 +00:00

fix(e2e): use LastInsertId for book seed in authors HTML test ... f5c858634e

Replace seedBooksForList (timestamp-order SELECT, racy in parallel suite)
with direct INSERT + LastInsertId for the authors HTML response test, fixing
the Duplicate entry '1' PRIMARY key error under --procs=8.

zombor force-pushed bd-bookshelf-37ef.1 from f5c858634e to b133826250 2026-06-09 14:10:40 +00:00 Compare

zombor commented 2026-06-09 16:51:55 +00:00

Author

Owner

Copy link

Security Review — PR #455 (bookshelf-37ef.1) author pages

Reviewed the diff only (no test re-run), focused on multi-user scoping, SQLi, IDOR, path traversal, and XSS.

[BLOCKER] internal/authors/service.go:271 — author→books enumeration is NOT scoped to the requesting user's accessible libraries (cross-library data leak)
ListAuthorBooks runs:

SELECT b.id, b.book_cover_hash, bm.title, bm.series_number
FROM book b JOIN book_metadata_author_mapping m ON m.book_id = b.id
LEFT JOIN book_metadata bm ON bm.book_id = b.id
WHERE m.author_id = ? AND b.deleted = 0
ORDER BY bm.series_number ASC, b.id ASC LIMIT ?

There is no b.library_id IN (...) predicate and ShowHandler (internal/authors/show_handler.go:54 → listBooks(r.Context(), id)) passes no userLibraryIDs. Any authenticated user visiting /authors/{id} sees titles + covers + book IDs (links to /books/{id}) for books in libraries they have NO access to. This is the identical defect to sibling #454's nil-userLibraryIDs series leak. The whole books surface fail-closes on this (internal/books/service.go:286, handler.go:150, store.go:340-348): resolve userID from session, call users.GetUserLibraryIDs, thread the result (non-nil empty slice ⇒ deny all) into the query — AND b.library_id IN (?) when non-empty, return [] when empty-but-non-nil, skip only when nil (system/no-auth).
Fix: add a userLibraryIDs []int64 parameter to ListAuthorBooks, build the b.library_id IN (...) clause with the fail-closed semantics already used in books/store.go, and in ShowHandler resolve userID-from-session → getUserLibraryIDs → pass it in (mirror books/handler.go:148-155). Add an e2e/service negative case: a book mapped to a library the user is NOT in must be absent from both the Books grid and ideally the count. (e2e/api/authors_test.go currently only seeds an accessible library for user 1, so the leak is untested.)

[MAJOR] internal/db/queries/authors.sql:16 — book_count on the detail page is a global, cross-library count
GetAuthorByID's COUNT(m.book_id) counts every mapped book regardless of library access, and the template renders it as "Books by X (N)" (templates/pages/author_show.html:58). With the BLOCKER fixed, a user could see a heading count larger than the books actually listed, leaking the existence/volume of books in libraries they cannot access. Scope the count to the user's libraries (join book b/b.library_id IN (...) in the count) so the displayed total matches the accessible set. (The pre-existing /authors LIST shares this aggregate-count behavior; that's prior art, but the new detail page makes the discrepancy user-visible against the now-scoped grid — fix here.)

Clean (verified, no finding):

• SQLi: ListAuthorBooks, GetAuthorByID, ListBookAuthorsWithID are all fully parameterized (? placeholders / sqlc); author id is strconv.ParseInt'd (show_handler.go:43), no string-concat of id/name.
• IDOR: author id comes from r.PathValue("id") only; identity is never read from body/query/header.
• Path traversal (/authors/{id}/photo): PhotoHandler is unchanged by this PR and safe — the filesystem path is built from the parsed int64 via files.AuthorImagePath, never raw input; placeholder SVG served on miss with a safe content type.
• XSS (author_show.html): all dynamic values (.Author.Name, .Description, .ASIN, book .Title) go through html/template auto-escaping; no template.HTML/safeHTML bypass; slice .Title 0 1 is guarded by {{if .Title}}.
• LIMIT: books-by-author is bounded by authorBooksLimit = 50 — no unbounded query.

REVIEW VERDICT: 1 blocker, 1 major, 0 minor

## Security Review — PR #455 (bookshelf-37ef.1) author pages Reviewed the diff only (no test re-run), focused on multi-user scoping, SQLi, IDOR, path traversal, and XSS. [BLOCKER] internal/authors/service.go:271 — author→books enumeration is NOT scoped to the requesting user's accessible libraries (cross-library data leak) `ListAuthorBooks` runs: ```sql SELECT b.id, b.book_cover_hash, bm.title, bm.series_number FROM book b JOIN book_metadata_author_mapping m ON m.book_id = b.id LEFT JOIN book_metadata bm ON bm.book_id = b.id WHERE m.author_id = ? AND b.deleted = 0 ORDER BY bm.series_number ASC, b.id ASC LIMIT ? ``` There is no `b.library_id IN (...)` predicate and `ShowHandler` (internal/authors/show_handler.go:54 → `listBooks(r.Context(), id)`) passes no `userLibraryIDs`. Any authenticated user visiting `/authors/{id}` sees titles + covers + book IDs (links to `/books/{id}`) for books in libraries they have NO access to. This is the identical defect to sibling #454's nil-userLibraryIDs series leak. The whole books surface fail-closes on this (internal/books/service.go:286, handler.go:150, store.go:340-348): resolve `userID` from session, call `users.GetUserLibraryIDs`, thread the result (non-nil empty slice ⇒ deny all) into the query — `AND b.library_id IN (?)` when non-empty, return `[]` when empty-but-non-nil, skip only when nil (system/no-auth). Fix: add a `userLibraryIDs []int64` parameter to `ListAuthorBooks`, build the `b.library_id IN (...)` clause with the fail-closed semantics already used in books/store.go, and in `ShowHandler` resolve userID-from-session → `getUserLibraryIDs` → pass it in (mirror books/handler.go:148-155). Add an e2e/service negative case: a book mapped to a library the user is NOT in must be absent from both the `Books` grid and ideally the count. (e2e/api/authors_test.go currently only seeds an accessible library for user 1, so the leak is untested.) [MAJOR] internal/db/queries/authors.sql:16 — `book_count` on the detail page is a global, cross-library count `GetAuthorByID`'s `COUNT(m.book_id)` counts every mapped book regardless of library access, and the template renders it as "Books by X (N)" (templates/pages/author_show.html:58). With the BLOCKER fixed, a user could see a heading count larger than the books actually listed, leaking the existence/volume of books in libraries they cannot access. Scope the count to the user's libraries (join `book b`/`b.library_id IN (...)` in the count) so the displayed total matches the accessible set. (The pre-existing `/authors` LIST shares this aggregate-count behavior; that's prior art, but the new detail page makes the discrepancy user-visible against the now-scoped grid — fix here.) Clean (verified, no finding): - SQLi: `ListAuthorBooks`, `GetAuthorByID`, `ListBookAuthorsWithID` are all fully parameterized (`?` placeholders / sqlc); author id is `strconv.ParseInt`'d (show_handler.go:43), no string-concat of id/name. - IDOR: author id comes from `r.PathValue("id")` only; identity is never read from body/query/header. - Path traversal (`/authors/{id}/photo`): PhotoHandler is unchanged by this PR and safe — the filesystem path is built from the parsed int64 via `files.AuthorImagePath`, never raw input; placeholder SVG served on miss with a safe content type. - XSS (author_show.html): all dynamic values (`.Author.Name`, `.Description`, `.ASIN`, book `.Title`) go through html/template auto-escaping; no `template.HTML`/`safeHTML` bypass; `slice .Title 0 1` is guarded by `{{if .Title}}`. - LIMIT: books-by-author is bounded by `authorBooksLimit = 50` — no unbounded query. REVIEW VERDICT: 1 blocker, 1 major, 0 minor

zombor reviewed 2026-06-09 16:52:12 +00:00

zombor left a comment

Copy link

No description provided.

[BLOCKER] Code review: Author books query lacks user library scoping, allowing access to inaccessible libraries. See full review in thread.

zombor reviewed 2026-06-09 16:52:24 +00:00

zombor left a comment

Copy link

No description provided.

[BLOCKER] internal/authors/service.go:145 — Author books query lacks user library scoping The `ListAuthorBooks` function queries books by author ID without filtering by the requesting user's accessible libraries: ```sql WHERE m.author_id = ? AND b.deleted = 0 ``` This allows any authenticated user to view books from all libraries on an author detail page, violating the multi-user access control requirement (epic bookshelf-4op.5). An author page showing books from inaccessible libraries is a data-access vulnerability. Fix: Add a `JOIN user_library_mapping ulm` and `AND b.library_id = ulm.library_id AND ulm.user_id = ?` to the query. Pass the requesting user's ID through the handler and service signature. --- [BLOCKER] internal/authors/show_handler.go:ShowHandler — User ID not extracted or passed to ListAuthorBooks The handler does not extract the user ID from the request context. The curried `listBooks` function signature accepts only `authorID`: ```go listBooks func(context.Context, int64) ([]AuthorBook, error) ``` The handler must extract the user ID via the middleware pattern and thread it through the service signature. Follow the pattern from books/handler.go:ListHandler. Fix: Add user ID extraction and pass it to the service; revise service signature to accept userID or getUserLibraryIDs callback. --- [MAJOR] e2e/api/authors_test.go — No multi-user library-scoping test The test suite does not verify that books from inaccessible libraries are not displayed. All tests use a single authenticated user. Multi-user scenarios are never exercised. Fix: Add an e2e test that seeds two libraries with books, assigns the requesting user to only one, maps an author to books in both, and asserts only accessible-library books are shown. --- [MINOR] internal/authors/service.go:buildListAuthorsQuery — Query library-scoping assumption not documented The comment for `ListAuthors` does not clarify whether author listing is global or scoped by library. Fix: Add clarifying comments. If ListAuthors is intentionally global, document it. If it should also be scoped, apply the same library-filtering fix. --- [MINOR] AuthorRef ripple change is consistent across templates The []string → []AuthorRef change in books.go ripples correctly through templates. Template escaping is proper. Consistency verified across books_show.html, books_index.html, books_index_fragment.html. No action needed. --- REVIEW VERDICT: [B] 2 blockers, [M] 1 major, [m] 2 minors

zombor added 1 commit 2026-06-09 17:02:19 +00:00

fix(authors): scope author-page book enumeration + count to user libraries (37ef.1) ... 030a100037

- ListAuthorBooks: add userLibraryIDs []int64 param; apply AND b.library_id IN (...)
  fail-closed: nil=no predicate, non-nil-empty=AND 1=0 (no access), non-empty=scoped
- CountAuthorBooks: new function returning scoped book count for the heading
- ShowHandler: inject getUserLibraryIDs + userIDFromRequest deps, resolve IDs
  per request, pass into listBooks + countBooks; override AuthorDetail.BookCount
  with scoped count so HTML heading matches visible grid
- wire.go: wire new deps (users.GetUserLibraryIDs + userIDFromRequest lambda)
- buildListAuthorsQuery: add comment documenting the known authors-index existence
  leak (scoping deferred to follow-up bead — LEFT JOIN semantics are non-trivial)
- Tests: update ListAuthorBooks tests for new signature; add CountAuthorBooks tests
  including fail-closed path; add ShowHandler error-path tests for new deps
- e2e: add negative multi-user scoping test — book in inaccessible library is
  excluded from the HTML grid AND the "Books by X (N)" count heading

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

zombor commented 2026-06-09 17:14:11 +00:00

Author

Owner

Copy link

Security re-review (focused) — PR #455 / bookshelf-37ef.1

Re-verified the prior library-scoping findings against origin/main...origin/bd-bookshelf-37ef.1. Most are resolved; one regression-of-scope remains on the JSON surface.

Prior [BLOCKER] — ListAuthorBooks unscoped book grid: RESOLVED.
ListAuthorBooks now takes userLibraryIDs []int64 with correct fail-closed semantics (service.go): nil → no predicate (system path), non-nil-empty → returns []AuthorBook{} (a user with zero library mappings sees nothing), non-nil → AND b.library_id IN (?, ...). users.GetUserLibraryIDs normalises a no-mapping user to a non-nil empty slice, so authenticated-but-unmapped users correctly fail closed. ShowHandler resolves userID := userIDFromRequest(r) and, when != 0, calls getUserLibraryIDs and threads the result into both listBooks and countBooks — identical to the established books/handler.go ListHandler pattern.

Negative-scoping e2e test: GENUINE.
seedBookInInaccessibleLibrary inserts a library with NO user_library_mapping row for user 1, plus a book + metadata + author mapping. seedBooksForList maps its own library to user 1, so the accessible book is real. The test asserts the HTML grid excludes "Inaccessible Book" and the heading shows Books by Kafka, Franz (1) (not 2). Real positive-and-negative coverage on the HTML path.

---

[MAJOR] internal/authors/show_handler.go:58 — JSON GET /authors/{id} returns the UNSCOPED global book_count
The HTML path overrides author.BookCount with the scoped countBooks(...), but the JSON branch (if handler.WantsJSON(r) { ... return json.NewEncoder(w).Encode(author) }) returns author before any library scoping runs — so author.BookCount is still the global count from GetAuthor (the unscoped GetAuthorByID GROUP BY across all libraries). A JSON API client thus learns the total volume of an author's books including those in libraries it cannot access. This is the same class as the prior "book_count global" MAJOR — the fix wired the scoped count into the HTML path only. The unit test at handler_test.go:508 ("returns the book_count in JSON") actually pins the leak: it asserts the JSON book_count equals the raw GetAuthor value (12), not the scoped count.
Note this is a count-volume disclosure only — the JSON path does NOT return the book list, so no inaccessible-book identities leak (that was the BLOCKER and is fixed). Fix: move the getUserLibraryIDs resolution above the WantsJSON branch and apply countBooks to author.BookCount before encoding JSON (so both surfaces use the scoped count).

[MINOR] internal/authors/service.go:~78 (buildListAuthorsQuery doc) — authors INDEX existence leak is a conscious deferral
GET /authors still lists ALL authors globally, including authors whose only books are in inaccessible libraries (author-existence + global per-author count leak). This is documented in the buildListAuthorsQuery comment as a tracked follow-up, with a sound rationale (LEFT-JOIN semantics change). Acceptable as a deferred follow-up; calling it out so the follow-up bead is not lost. Severity is existence/aggregate-count disclosure, not content disclosure.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Security re-review (focused) — PR #455 / bookshelf-37ef.1 Re-verified the prior library-scoping findings against `origin/main...origin/bd-bookshelf-37ef.1`. Most are resolved; one regression-of-scope remains on the JSON surface. **Prior [BLOCKER] — ListAuthorBooks unscoped book grid: RESOLVED.** `ListAuthorBooks` now takes `userLibraryIDs []int64` with correct fail-closed semantics (`service.go`): `nil` → no predicate (system path), non-nil-empty → returns `[]AuthorBook{}` (a user with zero library mappings sees nothing), non-nil → `AND b.library_id IN (?, ...)`. `users.GetUserLibraryIDs` normalises a no-mapping user to a non-nil empty slice, so authenticated-but-unmapped users correctly fail closed. `ShowHandler` resolves `userID := userIDFromRequest(r)` and, when `!= 0`, calls `getUserLibraryIDs` and threads the result into both `listBooks` and `countBooks` — identical to the established `books/handler.go ListHandler` pattern. **Negative-scoping e2e test: GENUINE.** `seedBookInInaccessibleLibrary` inserts a library with NO `user_library_mapping` row for user 1, plus a book + metadata + author mapping. `seedBooksForList` maps its own library to user 1, so the accessible book is real. The test asserts the HTML grid excludes "Inaccessible Book" and the heading shows `Books by Kafka, Franz (1)` (not 2). Real positive-and-negative coverage on the HTML path. --- [MAJOR] internal/authors/show_handler.go:58 — JSON `GET /authors/{id}` returns the UNSCOPED global `book_count` The HTML path overrides `author.BookCount` with the scoped `countBooks(...)`, but the JSON branch (`if handler.WantsJSON(r) { ... return json.NewEncoder(w).Encode(author) }`) returns `author` *before* any library scoping runs — so `author.BookCount` is still the global count from `GetAuthor` (the unscoped `GetAuthorByID` GROUP BY across all libraries). A JSON API client thus learns the total volume of an author's books including those in libraries it cannot access. This is the same class as the prior "book_count global" MAJOR — the fix wired the scoped count into the HTML path only. The unit test at `handler_test.go:508` ("returns the book_count in JSON") actually pins the leak: it asserts the JSON `book_count` equals the raw `GetAuthor` value (12), not the scoped count. Note this is a count-volume disclosure only — the JSON path does NOT return the book list, so no inaccessible-book *identities* leak (that was the BLOCKER and is fixed). Fix: move the `getUserLibraryIDs` resolution above the `WantsJSON` branch and apply `countBooks` to `author.BookCount` before encoding JSON (so both surfaces use the scoped count). [MINOR] internal/authors/service.go:~78 (buildListAuthorsQuery doc) — authors INDEX existence leak is a conscious deferral `GET /authors` still lists ALL authors globally, including authors whose only books are in inaccessible libraries (author-existence + global per-author count leak). This is documented in the `buildListAuthorsQuery` comment as a tracked follow-up, with a sound rationale (LEFT-JOIN semantics change). Acceptable as a deferred follow-up; calling it out so the follow-up bead is not lost. Severity is existence/aggregate-count disclosure, not content disclosure. REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor force-pushed bd-bookshelf-37ef.1 from 030a100037 to 28a5f76788 2026-06-09 17:18:52 +00:00 Compare

zombor force-pushed bd-bookshelf-37ef.1 from 28a5f76788 to 52c5bd261d 2026-06-09 17:29:50 +00:00 Compare

zombor added 1 commit 2026-06-09 17:32:59 +00:00

fix(books): correct []string{} → []AuthorRef{} in reader handler test ... 1a32a805cb

reader_handler_test.go:2640 was missed when Book.Authors changed from
[]string to []AuthorRef in e6abc2b4 — caused a test build failure in CI.

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

zombor commented 2026-06-09 17:42:42 +00:00

Author

Owner

Copy link

Security re-review — JSON-count MAJOR resolved

Focused re-review of the prior remaining [MAJOR] (JSON branch of GET /authors/{id} returned the UNSCOPED global book_count). Verified against git diff origin/main...origin/bd-bookshelf-37ef.1 (CI green @ 1a32a805).

1. Scoped count now runs before content negotiation — RESOLVED.
In internal/authors/show_handler.go, ShowHandler resolves userLibraryIDs via getUserLibraryIDs (fail-closed: userID==0 leaves userLibraryIDs nil only for the system path; an authenticated user with no mappings yields []int64{} → CountAuthorBooks returns 0) and then unconditionally overrides:

author.BookCount, err = countBooks(r.Context(), id, userLibraryIDs)
...
if handler.WantsJSON(r) {
    return json.NewEncoder(w).Encode(author)   // scoped count
}

Both the JSON branch and the HTML heading now emit the SCOPED count; the unscoped book_count from GetAuthorByID is discarded. CountAuthorBooks builds its library_id IN (...) clause with ? placeholders only — no injection.

2. Unit test updated — RESOLVED.
handler_test.go ShowHandler specs set the get stub BookCount:12 (unscoped) but countBooks stub 7 (scoped). Tests assert:

• JSON: decoded.BookCount == 7 AND explicitly NotTo(Equal(12)).
• HTML: body ContainSubstring("7 books").
  This proves JSON and HTML agree on the scoped value and the unscoped value is never surfaced.

3. No regression on the book-list BLOCKER / negative-scoping e2e.
ListAuthorBooks still applies the same library_id IN (...) scoping (fail-closed empty → []AuthorBook{}). The e2e book in inaccessible library is excluded from grid and count test seeds a book in an unmapped library and asserts the HTML grid excludes it AND the heading reads Books by Kafka, Franz (1). Intact.

4. Authors-index global-listing MINOR — still a documented deferral.
The scoping note in buildListAuthorsQuery (service.go) remains, describing the existence leak as a tracked follow-up. Not regressed into a blocker.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security re-review — JSON-count MAJOR resolved Focused re-review of the prior remaining `[MAJOR]` (JSON branch of `GET /authors/{id}` returned the UNSCOPED global `book_count`). Verified against `git diff origin/main...origin/bd-bookshelf-37ef.1` (CI green @ 1a32a805). **1. Scoped count now runs before content negotiation — RESOLVED.** In `internal/authors/show_handler.go`, `ShowHandler` resolves `userLibraryIDs` via `getUserLibraryIDs` (fail-closed: `userID==0` leaves `userLibraryIDs` nil only for the system path; an authenticated user with no mappings yields `[]int64{}` → `CountAuthorBooks` returns 0) and then unconditionally overrides: ```go author.BookCount, err = countBooks(r.Context(), id, userLibraryIDs) ... if handler.WantsJSON(r) { return json.NewEncoder(w).Encode(author) // scoped count } ``` Both the JSON branch and the HTML heading now emit the SCOPED count; the unscoped `book_count` from `GetAuthorByID` is discarded. `CountAuthorBooks` builds its `library_id IN (...)` clause with `?` placeholders only — no injection. **2. Unit test updated — RESOLVED.** `handler_test.go` `ShowHandler` specs set the `get` stub `BookCount:12` (unscoped) but `countBooks` stub `7` (scoped). Tests assert: - JSON: `decoded.BookCount == 7` AND explicitly `NotTo(Equal(12))`. - HTML: body `ContainSubstring("7 books")`. This proves JSON and HTML agree on the scoped value and the unscoped value is never surfaced. **3. No regression on the book-list BLOCKER / negative-scoping e2e.** `ListAuthorBooks` still applies the same `library_id IN (...)` scoping (fail-closed empty → `[]AuthorBook{}`). The e2e `book in inaccessible library is excluded from grid and count` test seeds a book in an unmapped library and asserts the HTML grid excludes it AND the heading reads `Books by Kafka, Franz (1)`. Intact. **4. Authors-index global-listing MINOR — still a documented deferral.** The scoping note in `buildListAuthorsQuery` (service.go) remains, describing the existence leak as a tracked follow-up. Not regressed into a blocker. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

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

Merge branch 'main' into bd-bookshelf-37ef.1 e836199817

zombor added 1 commit 2026-06-09 18:26:43 +00:00

fix(lint): use books.AuthorRef{} in external test package (37ef.1) ... 799de4d4d3

routes_test.go is package books_test (external), so AuthorRef needs the
books. qualifier. The merge of main (RBAC gates in #460) added a new
getFn stub using []string{} for Authors which conflicted with this
branch's []AuthorRef change — update to books.AuthorRef{} to match.

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

zombor merged commit 08f0604fb6 into main 2026-06-09 18:31:41 +00:00

zombor referenced this pull request from a commit 2026-06-09 18:31:43 +00:00

Merge pull request 'feat(authors): author detail page + book-page author links (bookshelf-37ef.1)' (#455) from bd-bookshelf-37ef.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!455

No description provided.