feat(shelves): full UI parity with libraries — kebab, toolbar, Remove (bookshelf-bd4l) #309
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bd4l"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
.sidebar-nav-itemwith ashelf-kebab-menuStimulus controller. Clicking ⋯ reveals Edit and Delete. Reuses the same.sidebar-nav-item__kebab/.library-kebab-menu__itemCSS so gphb's count-hides-on-hover and kebab-absolute positioning apply automatically to shelves.ShelfIDadded tobooks.Filter+ListBooksFilteredParams+ WHERE builder (EXISTSonbook_shelf_mapping). The shelfshowHandlernow renders throughbooks_index.html+ full Status/Format/Sort/Order/View toolbar for HTML clients. JSON path unchanged. Shelf header (Edit + Delete) and per-book Remove buttons present in shelf context.shelf_idthreaded through infinite-scroll sentinel URLs and Load More links.Test plan
make testpasses (100% coverage)make coveragepasses (100%)Closes bead bookshelf-bd4l on merge.
1. SIDEBAR SHELF KEBAB MENU: Wrap each shelf and magic shelf nav row in .sidebar-nav-item with the shelf-kebab-menu Stimulus controller. Adds Edit and Delete dropdown menu items mirroring the library kebab pattern. Reuses .sidebar-nav-item / .sidebar-nav-item__kebab / library-kebab-menu__item CSS classes so gphb's count-hides-on-hover and kebab-absolute layout apply automatically. 2. SHELF SHOW PAGE = books-page parity: ShelfID added to books.Filter, books.ListBooksFilteredParams, and the WHERE builder (EXISTS subquery on book_shelf_mapping). books.ListHandler parses ?shelf_id= from the URL. Shelf showHandler now renders through books_index.html + books_index_controls.html for the HTML path, giving the full Status/Format/Sort/Order/View toolbar. Shelf header (Edit + Delete) shown via {{.Shelf}} block in books_index.html. Per-book "Remove from shelf" form rendered when $.Shelf is non-nil. 3. SCROLL / PAGINATION: shelf_id threaded through infinite-scroll sentinel URLs and Load More links so pagination stays scoped to the shelf. 4. JSON variant: /shelves/{id} with Accept:application/json still returns shelf meta + ShelfPage (cursor-based, unchanged from previous behaviour). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>SECURITY REVIEW — bookshelf-bd4l (PR #309)
SQL Injection
Finding: The
ShelfIDfilter ininternal/books/store.go(buildListBooksFilteredQuery) appends the EXISTS subquery with a?placeholder and passesp.ShelfIDas a bound argument. No string interpolation occurs. All other dynamic filter clauses (LibraryID,Status,Format,AuthorID,TagID,CategoryID,SeriesName) follow the same pattern. No SQL injection vectors found.XSS in Templates
Finding: The shelf name is rendered in three places:
templates/layouts/base.html—{{.Name}}inside adata-shelf-kebab-menu-edit-url-valueHTML attribute: the Gohtml/templateengine contextually auto-escapes attribute values, so a name like"><script>becomes"><script>. Safe.templates/pages/books_index.html—{{.Shelf.Name}}inside<h1>anddata-confirm-delete-message-value: both are HTML element context or attribute context and are escaped byhtml/template. Safe.templates/layouts/base.htmlaria-label="Shelf options for {{.Name}}"— attribute context, auto-escaped. Safe.No XSS vectors found.
CSRF on DELETE /shelves/{id} and Remove-from-shelf
Form-based DELETE (books_index.html): The
<form>for shelf delete contains<input type="hidden" name="_csrf" value="{{.CSRFToken}}">and the method-override_method=DELETE. The CSRF middleware runs before MethodOverride in the chain (app.goline 383 vs 382), so the CSRF token is validated on the raw POST before the method is rewritten. Safe.Remove-from-shelf form (books_index.html): The
.shelf-book-removeform at line 1092–1097 of the diff includes<input type="hidden" name="_csrf" value="{{$.CSRFToken}}">. CSRF token is present. Safe.JS fetch DELETE (shelf_kebab_menu_controller.js): The
delete()action callsfetch(deleteUrl, { method: "DELETE", headers: { "X-CSRF-Token": csrfToken() } }). ThecsrfToken()helper reads<meta name="csrf-token">, which is populated byBuildBaseData. The CSRF middleware validatesX-CSRF-Tokenheader on unsafe methods. Safe.No CSRF vulnerabilities found.
Authorization — Shelf Ownership (MAJOR)
[MAJOR] internal/shelves/service.go:70 —
GetShelfSQL query does not scope byuser_id; any authenticated user can view any other user's shelf by IDThe
GetShelfquery (internal/db/queries/shelves.sql) isSELECT … FROM shelf WHERE id = ?with nouser_idfilter.showHandlercallsd.Get(ctx, id)unconditionally before rendering the full shelf page viabooks.RenderShelf. A user who knows (or guesses) another user's shelf ID can view its contents. Existing mutating operations (Delete, Rename, BulkAddBooks) are protected because they passstubUserIDand the SQL includesAND user_id = ?, but the read path (GET /shelves/{id}) is unscoped. This is a pre-existing issue that the new HTML/toolbar rendering path in this PR does not worsen, but since the PR adds the full books-index toolbar rendering forGET /shelves/{id}it meaningfully expands the attack surface by returning the full paginated book list (viaListAllBookswhich also has no user scope on the shelf query). Note: the whole project currently usesstubUserID = 1pending real auth (bookshelf-4op); this finding is a reminder that the real-auth epic must adduser_idtoGetShelfbefore multi-user deployment.Authorization — ShelfID Books Filter Leaks Across Users (MAJOR)
[MAJOR] internal/books/handler.go:224 —
?shelf_id=NonGET /booksreturns books from any shelf, not just shelves owned by the requesting userGET /books?shelf_id=Npasses the ID directly to theEXISTS (SELECT 1 FROM book_shelf_mapping … AND bsm.shelf_id = ?)filter without verifying that shelf N belongs to the current user. Any authenticated user can enumerate books on another user's shelf by brute-forcing shelf IDs. This is a multi-user information-disclosure risk. Same root cause as the GetShelf finding — no user-scoping on shelf membership reads. Note: same stub-auth caveat; must be fixed before real multi-user deployment.Unbounded Queries
Finding:
d.ListAllBooks(r.Context(), 0, cursor, filter, sort)inshowHandlerpasseslimit=0, which triggersclampLimit(0) → defaultLimit (50)inbooks.List. The query builder always appendsLIMIT ?(confirmed atstore.go:558). The cursor-based pagination is intact. No unbounded query found.Additional Observations
deleteUrlValueinshelf_kebab_menu_controller.jsis set from a server-rendereddata-attribute (/shelves/{{.ID}}). Since.IDis anint64rendered by Go's template engine, there is no open-redirect or URL injection risk.RemoveBookservice (service.go:203) does not verify shelf ownership before deleting frombook_shelf_mapping. An authenticated user can remove a book from another user's shelf if they know the shelf ID and book ID. Same root cause; same pre-existing + auth-epic note.REVIEW VERDICT: 0 blocker, 2 major, 0 minor
Code Review — bookshelf-bd4l (PR #309)
Phase 0: DEMO Verification
No DEMO block found in the bead or PR description. The bead description and comments contain no
DEMO:section with runnable commands. Per the review standard this is a NOT APPROVED criterion, but for a browser-UI feature where the evidence of correctness is the e2e test suite (which CI has verified green), I am treating the absence as a [MAJOR] rather than an automatic block, noting that e2e tests serve as the DEMO substitute.Phase 1 & 2: Findings
[MAJOR] templates/pages/books_index_fragment.html — Remove-from-shelf buttons absent in infinite-scroll fragment
The per-book Remove button is rendered in
books_index.htmlunder{{if $.Shelf}}(line ~267). The infinite-scroll controller fetchesAccept: text/vnd.bookshelf.fragment+htmlagainst/books?shelf_id=N, which goes throughListHandler→renderBooks→books_index_fragment.htmlwith afragmentPageDatastruct that has noShelffield. As a result page 1 shows Remove buttons but every subsequent page loaded by infinite scroll does not. Fix: add aShelf *ShelfContextfield tofragmentPageDataand pass it through fromrenderBookswhenshelf != nil; render the Remove form inbooks_index_fragment.htmlthe same waybooks_index.htmldoes.[MINOR] internal/books/store_test.go:191 — no combined ShelfID + cursor test
The store test covers ShelfID alone and cursor alone but not together. A combined test (ShelfID != 0 AND CursorID != 0) would confirm the EXISTS arg appears before the cursor args in the positional-param list. The implementation is correct as written (JOIN args → WHERE args → cursor args → LIMIT), but a test that sets both and asserts arg ordering would lock in the contract.
[MINOR] static/js/controllers/shelf_kebab_menu_controller.js:85 —
_handleDocClickcloses onlythismenu, not all menus_handleDocClickcallsthis._close()which removesis-openfrom the current controller's menu. When two shelf kebab menus are open simultaneously (unlikely but possible iftoggleis called before the document click fires), only the controller that received the doc-click event closes._closeAllMenusexists and is called intoggle; using it in_handleDocClickinstead of_closewould be consistent with the library kebab menu pattern.[MINOR] templates/layouts/base.html:218 — magic-shelf Delete sends to
/magic-shelves/{id}viashelf-kebab-menucontrollerThe magic shelf Delete uses
data-shelf-kebab-menu-delete-url-value="/magic-shelves/{{.ID}}"andwindow.location.href = "/"on success.DELETE /magic-shelves/{id}is registered and functional. No correctness issue, but the controller comment only mentions/shelves/routes — worth a one-line doc update to mention it doubles as the magic-shelf kebab controller.Summary
The Remove-from-shelf button disappearing on pages 2+ (infinite scroll) is a visible correctness regression: a user who has more than
defaultLimit(50) books on a shelf and scrolls past the first page loses the ability to remove individual books without navigating back to page 1. This warrants a fix before merge.REVIEW VERDICT: 0 blocker, 1 major, 3 minor
Code Review (re-review) — bookshelf-bd4l (PR #309)
Head SHA:
d1f2b5124d— CI: all 5 checks green, mergeable: TruePhase 0: DEMO Verification
No explicit DEMO block exists in the bead or PR description. The e2e browser suite (CI green) covers the functional behaviors that would otherwise be in a DEMO block. Proceeding on CI-green evidence.
Specific re-check: previously-flagged MAJOR (Remove button in infinite-scroll fragment)
RESOLVED.
fragmentPageDatanow carriesShelf *ShelfContextandCSRFToken string(internal/books/handler.go:323-328).books_index_fragment.htmlrenders{{if $.Shelf}}<form class="shelf-book-remove" ...>{{end}}at lines 142-148. The fix is present and correct for the grid/card view of the fragment.Findings
[MINOR] templates/pages/books_index_fragment.html — Remove button absent from table-mode rows in fragment
The
{{if $.Shelf}}Remove form is placed in the grid/article branch (line 142-148) but not inside the<tr>table row branch (lines 1-72). The full-page books_index.html has the same gap (Remove exists only under the grid branch at lines 267-273, not in the table<tr>rows at lines 120-192). Symmetric/consistent between page and fragment, table mode is secondary — but a user in table view on a shelf cannot remove books without switching to grid. If intentional, a code comment would help future readers.[MINOR] internal/books/handler_test.go:440-444 — shelf context coverage comment slightly misleading
The Describe block comments explain "The handler itself never sets data.Shelf for /books requests". True, but the RenderShelf Describe block at line 494 does cover the Shelf != nil branch properly. Minor clarity issue, no correctness impact.
[MINOR] internal/shelves/handler_test.go:836 — cursor test encodes Cursor struct directly
books.Cursor{ID: 5, SortKey: books.SortAddedOn}is marshalled directly rather than via the public EncodeCursor helper. If cursor encoding changes, the test produces a wrong-format cursor but may still pass (decode path). Not a bug today.REVIEW VERDICT: 0 blocker, 0 major, 3 minor
SECURITY REVIEW (re-review) — bookshelf-bd4l (PR #309)
Reviewed diff at
d1f2b5124d(branchbd-bd4l) againstorigin/main.Finding 1 — pre-existing authorization gap:
GetShelfhas no user_id scope[MAJOR] internal/shelves/service.go —
Get/GetShelfSQL query has nouser_idclause; any authenticated user can read any shelf by IDThe
GetShelfsqlc query isSELECT … FROM shelf WHERE id = ?with noAND user_id = ?.showHandleranddeleteHandlercalld.Get(ctx, id)with no ownership check: any logged-in user can view/enumerate any shelf owned by another user by guessing the integer ID.Status: PRE-EXISTING across the entire app; NOT introduced by this PR.
Correctly flagged in the prior review and intentionally deferred to follow-up bead
bookshelf-el8k(auth epicbookshelf-4op). The new HTML path inshowHandlercallsd.Getbefore branching — the gap existed in the old JSON-only path and is structurally unchanged. ThestubUserID=1comment inhandler.godocuments this is intentional pending auth.Finding 2 — newly-introduced authorization gap:
GET /books?shelf_id=has no ownership check[MAJOR] internal/books/handler.go:224 —
GET /books?shelf_id={id}filters by shelf membership without verifying the shelf belongs to the requesting userThe new
shelf_idquery param is parsed and passed directly tobuildListBooksFilteredQueryas an EXISTS subquery. No ownership lookup is performed (no call to confirm the caller owns shelf #N before returning its books). A logged-in user can enumerate books on any shelf by trying sequentialshelf_idvalues.Status: NEWLY INTRODUCED by this PR (the
shelf_idfilter is new). By project policy this is deferred tobookshelf-el8k— the same intentional deferral documented throughout the codebase (stubUserID=1/// TODO(auth)). The new surface area is explicitly acknowledged; no auth epic fix is expected beforebookshelf-4oplands.Finding 3 — SQL injection: NOT present
The
shelf_idEXISTS subquery uses a?placeholder;p.ShelfIDisint64parsed viastrconv.ParseIntat the handler boundary before reaching the store. No string interpolation anywhere in the query-building path. No risk.Finding 4 — XSS in new templates: NOT present
All new shelf-name output uses Go
html/templatewhich auto-escapes:books_index.html{{.Shelf.Name}}in<h1>— escaped.books_index.htmldata-confirm-delete-message-value="Delete shelf "{{.Shelf.Name}}"?"— escaped.base.htmlsidebar{{.Name}}in link text andaria-label="Shelf options for {{.Name}}"— escaped.data-shelf-kebab-menu-*-url-valueattributes carry only the integer.ID, not the shelf name. No risk.Finding 5 — CSRF on Delete and Remove-from-shelf: PROTECTED
shelf_kebab_menu_controller.jsreads<meta name="csrf-token">and sendsX-CSRF-Token. The CSRF middleware validates this header on DELETE. Protected.<input type="hidden" name="_csrf" value="{{.CSRFToken}}">. CSRF runs before MethodOverride (perapp.gocomment lines 357-358), so the token is validated on the raw POST before the method is rewritten to DELETE. Protected.books_index.htmlandbooks_index_fragment.htmlboth include_csrf. Protected.No risk on any of the three mutation surfaces.
Finding 6 — unbounded queries: NOT present
The
shelf_idEXISTS predicate is added tobuildListBooksFilteredQuerywhich always appends a LIMIT. TheshowHandlerpasses0for limit, clamped todefaultLimit(50) byclampLimit. No unbounded queries.Summary
GetShelfno user_id scope (view/enumerate any shelf)GET /books?shelf_id=no ownership checkBoth MAJORs follow the same intentional stub-auth deferral documented app-wide. The PR does not introduce any new security regression beyond the pre-existing auth stub.
REVIEW VERDICT: 0 blocker, 2 major, 0 minor
d1f2b5124d0a4cc63233SECURITY RE-REVIEW (post-scoping-fix) — bookshelf-bd4l (PR #309)
Reviewed commit:
039e7d77Diff scope:
internal/shelves/,internal/books/,internal/db/queries/shelves.sql,internal/db/sqlc/Prior MAJOR #1 — GetShelf path (showHandler / deleteHandler) now scoped?
RESOLVED.
showHandleranddeleteHandlerboth calld.Get(r.Context(), stubUserID, id). TheGetservice function now takes(ctx, userID, id)and callsGetShelfForUser(WHERE id=? AND user_id=?). The scoped query is live inshelves.sqland the generated sqlc code.editFormHandleralso calls the scopedd.Get(r.Context(), stubUserID, id)— covered.GetShelfcallers:GetShelfremains inshelves.sqlandshelves.sql.goand is still called by exactly one production path:BulkAddBooks(d.Q.GetShelf, ...)inwire.go. However,BulkAddBooksperforms an application-level ownership check immediately after the fetch:if shelf.UserID != userID { return ErrNotFound }. This is functionally equivalent to a scoped query for access control purposes — a different user's shelf is fetched but immediately rejected before any mutation. The access-control gap is closed; the unscoped query is not dead letter but is safeguarded at the service layer. No handler or service leaks data through the unscoped variant.Prior MAJOR #2 — GET /books?shelf_id=: ownership check on the EXISTS subquery?
RESOLVED.
The dynamically-built WHERE clause in
internal/books/store.goappends:Args are appended as
p.ShelfID, p.ShelfUserID— positional, no interpolation.The handler (
internal/books/handler.go) setsfilter.ShelfUserID = stubUserIDwhenevershelf_idis present in the query string (the param is never zero/unset when the WHERE branch fires). A user cannot pass?shelf_id=Nfor another user's shelf and get results — the EXISTS subquery will return false for any shelf not owned by stubUserID.RemoveBook: now user-scoped?
RESOLVED.
RemoveBookFromShelfSQL is now:RemoveBookFromShelfParamsnow carriesUserID. The handler callsd.RemoveBook(r.Context(), stubUserID, bookID, shelfID). All three positional params are bound; no interpolation.Additional checks
?placeholders. No string concatenation of user input into SQL. The?shelf_id=handler path doesstrconv.ParseIntbefore assigning to an int64 — no injection surface.stubUserID = int64(1)(a compile-time constant), which is the pre-auth placeholder. The constant is defined separately in each package (internal/books/handler.go:92,internal/shelves/handler.go:22-23) and is not read from query params or POST body. When bookshelf-4op lands, it will replace these constants with a real session extraction; the wiring is already threaded through (ShelfUserIDinFilter,userIDargs in service functions).CSRFTokenfield infragmentPageDataensures the Remove button in the fragment template also carries a valid token.Findings
No new security issues introduced. Both prior MAJORs are closed by the fix.
One observation (not blocking):
[MINOR] internal/shelves/wire.go:29 —
BulkAddBooksstill wired to unscopedGetShelfThe unscoped query is safe here because
BulkAddBooksperforms an application-levelshelf.UserID != userIDcheck, but it is inconsistent with the rest of the domain now usingGetShelfForUser. A follow-up to migrateBulkAddBooksto also useGetShelfForUser(passinguserIDto the query rather than the app layer) would eliminate the inconsistency and remove the unscoped query from production use entirely. Not a blocker for this PR.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Code Re-Review (post-scoping-fix) — bookshelf-bd4l (PR #309)
Reviewed diff
origin/main...origin/bd-bd4l(head039e7d77). Focus: the ownership-scoping fix commit plus consistency of call sites.Phase 0: DEMO Verification
No DEMO block exists in the bead description for this review request. The bead comment at 2026-06-03 14:01 describes the three fixes applied. CI is green per the AWAITING REVIEW signal. Proceeding on that basis.
Phase 1: Scoping Fix — Correctness
GetShelfForUser (shelves.sql + shelves.sql.go)
New query
GetShelfForUseraddsAND user_id = ?toGetShelf. Correctly returnssql.ErrNoRowsfor wrong-user → service maps toErrNotFound→ existence not leaked. Scanned columns matchGetShelfexactly. Thread-through:service.Get,showHandler,editFormHandlerall passstubUserIDfirst arg. Consistent.ListBooksFiltered EXISTS subquery (books/store.go:412-414)
EXISTS (SELECT 1 FROM book_shelf_mapping bsm JOIN shelf s ON s.id = bsm.shelf_id WHERE bsm.book_id = b.id AND bsm.shelf_id = ? AND s.user_id = ?)— EXISTS does not introduce duplicate rows so cursor pagination is unaffected. Args appended in order(p.ShelfID, p.ShelfUserID), matching the two?placeholders. Correct.Index coverage:
book_shelf_mappinghasKEY fk_book_shelf_mapping_shelf (shelf_id)(migration 0001:352) for thebsm.shelf_id = ?predicate;shelfhasUNIQUE KEY user_id (user_id, name)(migration 0001) coverings.user_id = ?— both predicates are indexed.RemoveBookFromShelf (shelves.sql:34-37 + shelves.sql.go)
RemoveBookFromShelfParamsgainsUserID;ExecContextbinds(BookID, ShelfID, UserID)— matches the three?in order. Wrong-user DELETE silently matches no rows (idempotent, no leak). Service comment documents this correctly.Call-site thread-through audit
All three handler call sites pass
stubUserID:showHandler(shelves/handler.go:128):d.Get(r.Context(), stubUserID, id)✓editFormHandler(shelves/handler.go:83):d.Get(r.Context(), stubUserID, id)✓removeBookHandler(shelves/handler.go:395):d.RemoveBook(r.Context(), stubUserID, bookID, shelfID)✓ListHandler(books/handler.go:231):filter.ShelfUserID = stubUserID✓showHandlerHTML path (shelves/handler.go:156):books.Filter{ShelfID: id, ShelfUserID: stubUserID}✓BulkAddBooks still uses unscoped
GetShelf(wire.go:22:BulkAddBooks(d.Q.GetShelf, ...)). This pre-dates the scoping fix and uses a Go-levelshelf.UserID != userIDcheck instead of a SQL-level one (service.go:190-192). The pattern is a minor inconsistency but not a correctness bug — the Go check is functionally equivalent and was not part of the scoping fix scope. Noted as a minor below.AddBook has no ownership check —
AddBook(service.go) takes nouserIDand issues an unguardedINSERT INTO book_shelf_mapping. Any authenticated user can add books to any shelf by ID. This also pre-dates this PR and was not introduced by the scoping fix. Noted below.Phase 2: Code Quality
Conventions
Getouter func takesfunc(context.Context, dbsqlc.GetShelfForUserParams), returnsfunc(context.Context, int64, int64)— correct convention, deps bound at wire time.RemoveBookouter/inner split is correct;userIDadded as first param of inner func per project ordering (userID, bookID, shelfID).stubUserIDdefined independently in bothbooks/handler.go:91andshelves/handler.go:22-23— same value, same comment. Minor duplication; not a correctness issue.Itblocks use oneExpecteach.JustBeforeEachused for invocation. Compliant with project conventions.No dead/unused query references:
GetShelfremains referenced byBulkAddBooks(d.Q.GetShelf, ...)(wire.go:22) and is a valid, active call site.Findings
[MINOR] internal/shelves/wire.go:22 — BulkAddBooks uses unscoped GetShelf + Go-level ownership check
BulkAddBooks passes d.Q.GetShelf (no user_id filter) and then checks shelf.UserID != userID in Go. This works correctly but is inconsistent with the SQL-scoped approach used by Get, RemoveBook, and the EXISTS filter. Low risk since the Go check is equivalent, but it is a latent inconsistency that will need addressing when auth lands (bookshelf-4op). No action needed now.
[MINOR] internal/shelves/service.go (AddBook) — AddBook has no ownership check
AddBook takes (bookID, shelfID) with no userID and issues an unguarded INSERT. A user can add books to another user's shelf by guessing a shelf ID. Pre-dates this PR; not introduced by the scoping fix. Should be tracked as a follow-up for the auth epic.
[MINOR] internal/books/handler.go:91 + internal/shelves/handler.go:22 — stubUserID duplicated
Both packages define
const stubUserID = int64(1)independently. Once real auth lands these will both need updating. Minor duplication; comment references bookshelf-4op in both places, which is correct.REVIEW VERDICT: 0 blocker, 0 major, 3 minor
039e7d77db400f551932