fix(nav): per-user library-scope Authors/AllBooks/ByLibrary nav counts (bookshelf-ebbx) #1090

Merged

zombor merged 2 commits from bd-bookshelf-ebbx into main 2026-07-10 23:55:56 +00:00

Conversation 2 Commits 2 Files changed 3 +294 -23

zombor commented 2026-07-10 23:00:13 +00:00

Owner

Copy link

What was leaking

Two nav badge counts used a global WrapInt64 cache shared across all users, so every user saw the same system-wide total regardless of which libraries they have access to:

• CountAllBooks — SELECT COUNT(*) FROM book WHERE deleted = 0: global count of every book; a user scoped to 2 libraries out of 10 would see the full 10-library total.
• CountAuthors — SELECT COUNT(*) FROM author: counts every author record in the system, not just authors with books in the user's libraries.

CountBooksByLibrary (global map of library_id→count) was audited and found safe to cache globally: applyNavCounts only looks up entries for libraries already in the user's sidebar, which are themselves user-scoped, so no cross-user count is ever displayed. A justification comment was added to the struct field.

Fix

• Changed NavCountDeps.CountAllBooks signature from func(context.Context) (int64, error) to func(context.Context, []int64) (int64, error) (library-scoped).
• Changed NavCountDeps.CountAuthors signature from func(context.Context) (int64, error) to func(context.Context, []int64) (int64, error) (library-scoped).
• launchNavBookCounts: CountAllBooks goroutine now calls resolveNavLibraryIDs before querying — the same fail-closed pattern used by CountDistinctSeries/CountCategories.
• launchNavMetaCounts: CountAuthors goroutine likewise uses resolveNavLibraryIDs.
• App wiring changed to use librarystats.MakeCountBooks / librarystats.MakeCountAuthors wrapped with WrapInt64WithLibraries (per-library-set cache entry, preventing cross-user leakage).

Test plan

• make test — all packages green including internal/middleware suite with 8 new specs proving:
  • CountAllBooks receives the user's resolved library IDs
  • CountAllBooks fail-closed when user has no library access (returns 0)
  • GetUserLibraryIDs error suppresses AllBooks badge (non-fatal)
  • CountAuthors receives the user's resolved library IDs
  • CountAuthors fail-closed when user has no library access (returns 0)
  • GetUserLibraryIDs error suppresses Authors badge (non-fatal)
• make coverage — 100% coverage maintained
• golangci-lint run ./internal/middleware/... ./internal/app/... ./internal/librarystats/... — 0 issues

Closes bead bookshelf-ebbx on merge.

## What was leaking Two nav badge counts used a **global** `WrapInt64` cache shared across all users, so every user saw the same system-wide total regardless of which libraries they have access to: - **CountAllBooks** — `SELECT COUNT(*) FROM book WHERE deleted = 0`: global count of every book; a user scoped to 2 libraries out of 10 would see the full 10-library total. - **CountAuthors** — `SELECT COUNT(*) FROM author`: counts every author record in the system, not just authors with books in the user's libraries. **CountBooksByLibrary** (global map of library_id→count) was audited and found **safe** to cache globally: `applyNavCounts` only looks up entries for libraries already in the user's sidebar, which are themselves user-scoped, so no cross-user count is ever displayed. A justification comment was added to the struct field. ## Fix - Changed `NavCountDeps.CountAllBooks` signature from `func(context.Context) (int64, error)` to `func(context.Context, []int64) (int64, error)` (library-scoped). - Changed `NavCountDeps.CountAuthors` signature from `func(context.Context) (int64, error)` to `func(context.Context, []int64) (int64, error)` (library-scoped). - `launchNavBookCounts`: CountAllBooks goroutine now calls `resolveNavLibraryIDs` before querying — the same fail-closed pattern used by CountDistinctSeries/CountCategories. - `launchNavMetaCounts`: CountAuthors goroutine likewise uses `resolveNavLibraryIDs`. - App wiring changed to use `librarystats.MakeCountBooks` / `librarystats.MakeCountAuthors` wrapped with `WrapInt64WithLibraries` (per-library-set cache entry, preventing cross-user leakage). ## Test plan - `make test` — all packages green including `internal/middleware` suite with 8 new specs proving: - CountAllBooks receives the user's resolved library IDs - CountAllBooks fail-closed when user has no library access (returns 0) - GetUserLibraryIDs error suppresses AllBooks badge (non-fatal) - CountAuthors receives the user's resolved library IDs - CountAuthors fail-closed when user has no library access (returns 0) - GetUserLibraryIDs error suppresses Authors badge (non-fatal) - `make coverage` — 100% coverage maintained - `golangci-lint run ./internal/middleware/... ./internal/app/... ./internal/librarystats/...` — 0 issues Closes bead bookshelf-ebbx on merge.

zombor added 1 commit 2026-07-10 23:00:13 +00:00

fix(nav): library-scope Authors/AllBooks nav counts; document ByLibrary global cache safety ... 83f2ddb1a6

CountAllBooks and CountAuthors were wired with a global WrapInt64 cache
(shared across all users), leaking cross-user nav badge counts when users
have access to different library subsets.

Investigation:
- CountAllBooks: `SELECT COUNT(*) FROM book WHERE deleted = 0` — no library
  filter; all users saw the same total regardless of which libraries they
  can access.
- CountAuthors: `SELECT COUNT(*) FROM author` — counts ALL authors in the
  system, not just those with books in the requesting user's libraries.
- CountBooksByLibrary: returns a global map of all library_id→count, but
  applyNavCounts only keys into it for libraries already in the user's
  sidebar (which are user-scoped), so no cross-user count is ever displayed.
  SAFE to cache globally — added justification comment.

Fix:
- Change NavCountDeps.CountAllBooks signature from func(ctx) to
  func(ctx, []int64) (library-scoped), wired via
  librarystats.MakeCountBooks + WrapInt64WithLibraries.
- Change NavCountDeps.CountAuthors signature from func(ctx) to
  func(ctx, []int64), wired via librarystats.MakeCountAuthors +
  WrapInt64WithLibraries.
- launchNavBookCounts: CountAllBooks goroutine now calls
  resolveNavLibraryIDs before querying, using the same fail-closed
  pattern as CountDistinctSeries/CountCategories.
- launchNavMetaCounts: CountAuthors goroutine likewise uses
  resolveNavLibraryIDs before querying.
- New tests prove each count receives the user's resolved library IDs,
  that an empty library set returns 0 (fail-closed), and that a
  GetUserLibraryIDs error suppresses the badge (non-fatal).

Closes bead bookshelf-ebbx on merge.

zombor commented 2026-07-10 23:11:38 +00:00

Author

Owner

Copy link

Security Review — bookshelf-ebbx (PR #1090)

Focus: multi-user scoping correctness for CountAllBooks and CountAuthors nav badges.

---

Multi-user scoping

CountAllBooks scoping — CORRECT. launchNavBookCounts now calls resolveNavLibraryIDs before invoking CountAllBooks. resolveNavLibraryIDs extracts the userID from the session (never from the request body/params), calls GetUserLibraryIDs(ctx, userID), and returns the result. MakeCountBooks then issues SELECT COUNT(*) FROM book WHERE deleted = 0 AND library_id IN (?, ...) with those IDs as bound parameters. No global count path remains.

CountAuthors scoping — CORRECT. Same pattern via launchNavMetaCounts. MakeCountAuthors issues SELECT COUNT(DISTINCT bma.author_id) … WHERE b.library_id IN (?, ...) AND b.deleted = 0 with the user's library IDs bound as parameters.

Cache key includes library scope — CORRECT. WrapInt64WithLibraries uses librarySetKey(ids) which maps nil → "nil", empty slice → "", and any non-empty slice → sorted-joined string (e.g. "1,10,2" for {2,1,10}). Each distinct library set gets its own independent NavCache entry. User A's library set {1,2} is keyed separately from user B's {3,4}. Cross-user cache contamination is impossible.

Fail-closed for empty library access — CORRECT. MakeCountBooks and MakeCountAuthors both guard with if len(ids) == 0 { return 0, nil }. An authenticated user with no accessible libraries gets count 0, never the global count.

Error path suppresses badge — CORRECT. resolveNavLibraryIDs returns (nil, false) on error; the caller checks !ok and returns without setting HasAllBooks/HasAuthors, so the badge is suppressed rather than showing a stale or global value.

userID source — CORRECT. resolveNavLibraryIDs takes userID int64 from the middleware's authenticated session, never from request body or query params.

CountBooksByLibrary global-map safety — CORRECT. The map is populated with all library_id → count pairs, but applyNavCounts only indexes into it using library IDs that are already in libs []tmpl.SidebarLibrary — a user-scoped slice. A user cannot observe a count for a library outside their sidebar. The justification in the comment is accurate.

---

[MINOR] internal/middleware/nav.go:26-28 — Doc comment "nil userLibraryIDs = unscoped" is inaccurate for the new CountAllBooks/CountAuthors functions
The comment inherited from the CountDistinctSeries pattern says "nil = unscoped (all libraries)", but MakeCountBooks and MakeCountAuthors treat nil identically to an empty slice — len(nil) == 0 is true in Go, so both return 0 immediately. The actual behavior is more restrictive than the comment implies (unauthenticated users see 0, not the global count). This is secure, but the comment could mislead a future maintainer adding a new count function under this interface who follows the doc and tries to implement a nil-means-unscoped path. Fix: update the doc comment to "nil userLibraryIDs = no libraries accessible, returns 0" for these two fields, or remove the "unscoped" language and standardize on "non-nil empty = zero (fail-closed)".

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security Review — bookshelf-ebbx (PR #1090) **Focus: multi-user scoping correctness for CountAllBooks and CountAuthors nav badges.** --- ### Multi-user scoping **CountAllBooks scoping — CORRECT.** `launchNavBookCounts` now calls `resolveNavLibraryIDs` before invoking `CountAllBooks`. `resolveNavLibraryIDs` extracts the userID from the session (never from the request body/params), calls `GetUserLibraryIDs(ctx, userID)`, and returns the result. `MakeCountBooks` then issues `SELECT COUNT(*) FROM book WHERE deleted = 0 AND library_id IN (?, ...)` with those IDs as bound parameters. No global count path remains. **CountAuthors scoping — CORRECT.** Same pattern via `launchNavMetaCounts`. `MakeCountAuthors` issues `SELECT COUNT(DISTINCT bma.author_id) … WHERE b.library_id IN (?, ...) AND b.deleted = 0` with the user's library IDs bound as parameters. **Cache key includes library scope — CORRECT.** `WrapInt64WithLibraries` uses `librarySetKey(ids)` which maps `nil` → `"nil"`, empty slice → `""`, and any non-empty slice → sorted-joined string (e.g. `"1,10,2"` for `{2,1,10}`). Each distinct library set gets its own independent `NavCache` entry. User A's library set `{1,2}` is keyed separately from user B's `{3,4}`. Cross-user cache contamination is impossible. **Fail-closed for empty library access — CORRECT.** `MakeCountBooks` and `MakeCountAuthors` both guard with `if len(ids) == 0 { return 0, nil }`. An authenticated user with no accessible libraries gets count `0`, never the global count. **Error path suppresses badge — CORRECT.** `resolveNavLibraryIDs` returns `(nil, false)` on error; the caller checks `!ok` and `return`s without setting `HasAllBooks`/`HasAuthors`, so the badge is suppressed rather than showing a stale or global value. **userID source — CORRECT.** `resolveNavLibraryIDs` takes `userID int64` from the middleware's authenticated session, never from request body or query params. **CountBooksByLibrary global-map safety — CORRECT.** The map is populated with all library_id → count pairs, but `applyNavCounts` only indexes into it using library IDs that are already in `libs []tmpl.SidebarLibrary` — a user-scoped slice. A user cannot observe a count for a library outside their sidebar. The justification in the comment is accurate. --- [MINOR] internal/middleware/nav.go:26-28 — Doc comment "nil userLibraryIDs = unscoped" is inaccurate for the new CountAllBooks/CountAuthors functions The comment inherited from the CountDistinctSeries pattern says "nil = unscoped (all libraries)", but `MakeCountBooks` and `MakeCountAuthors` treat `nil` identically to an empty slice — `len(nil) == 0` is true in Go, so both return `0` immediately. The actual behavior is more restrictive than the comment implies (unauthenticated users see 0, not the global count). This is secure, but the comment could mislead a future maintainer adding a new count function under this interface who follows the doc and tries to implement a nil-means-unscoped path. Fix: update the doc comment to "nil userLibraryIDs = no libraries accessible, returns 0" for these two fields, or remove the "unscoped" language and standardize on "non-nil empty = zero (fail-closed)". --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-07-10 23:12:20 +00:00

Author

Owner

Copy link

CODE REVIEW — bookshelf-ebbx (PR #1090)

Phase 0: DEMO Verification

No runnable DEMO block exists in the bead (bug-fix/audit task). Proceeding on diff review.

Phase 1: Spec Compliance

• CountAllBooks scoped via WrapInt64WithLibraries(librarystats.MakeCountBooks(...), ...) — matched ✓
• CountAuthors scoped via WrapInt64WithLibraries(librarystats.MakeCountAuthors(...), ...) — matched ✓
• CountBooksByLibrary audited and documented as safe to cache globally — matched ✓
• PR #932 pattern (WrapInt64WithLibraries + resolveNavLibraryIDs) followed exactly — matched ✓

Phase 2: Code Quality

Correctness — SQL queries are genuinely scoped

The critical concern (scoped cache over an unscoped query) does not apply here. Both MakeCountBooks and MakeCountAuthors in internal/librarystats/store.go carry genuine WHERE ... library_id IN (...) filters and fail-closed on empty input. The nav badge fix is end-to-end, not just a cache-key fix.

Nil vs empty distinction — documentation inconsistency with CountDistinctSeries

CountDistinctSeries (established in PR #932) correctly distinguishes nil from empty:

// nil = unscoped (global count for unauthenticated callers)
if userLibraryIDs != nil && len(userLibraryIDs) == 0 {
    return 0, nil  // fail-closed only for authenticated user with no access
}

MakeCountBooks and MakeCountAuthors both use:

if len(ids) == 0 {
    return 0, nil  // fires for BOTH nil and empty
}

The doc comment on CountAllBooks and CountAuthors in NavCountDeps (nav.go) says nil userLibraryIDs = unscoped; non-nil empty = fail-closed — but the implementation returns 0 for nil too, contradicting the stated contract. resolveNavLibraryIDs returns (nil, true) for unauthenticated users (userID==0), so those users see 0 books/authors while seeing the global series count (CountDistinctSeries handles nil correctly). The inconsistency doesn't create a security regression (0 is more restrictive than global), but the misleading doc comment is a latent trap for the next developer.

[MINOR] internal/librarystats/store.go MakeCountBooks line 9, MakeCountAuthors line 48 — nil treated as fail-closed, contradicting nil = unscoped doc comment and diverging from the CountDistinctSeries precedent in internal/series/store.go. Fix: change if len(ids) == 0 to if ids != nil && len(ids) == 0 (matching CountDistinctSeries) and add the nil = global-count unscoped path, OR correct the NavCountDeps doc comment to say nil OR empty = fail-closed.

CountBooksByLibrary global justification

Accurate. applyNavCounts keys into the returned map only for libraries present in the user's sidebar (populated via listLibraries with userID scoping), so no cross-user count is exposed. Comment added in both nav.go and app.go confirms this.

app.go wiring

Clean. WrapInt64WithLibraries returns nil when fn is nil, but librarystats.MakeCountBooks / MakeCountAuthors always return non-nil closures, so no nil func-field trap applies here.

Tests

• package middleware_test — black-box ✓
• Curried-DI capture pattern (capturedLibIDs) correctly validates IDs are forwarded ✓
• 8 new tests: ID forwarding, fail-closed, error suppression — for both AllBooks and Authors ✓
• One Expect per It throughout ✓
• JustBeforeEach / BeforeEach separation maintained ✓

wg count unchanged

wg.Add(9) unchanged. New resolveNavLibraryIDs calls are inside existing goroutines (not new ones). defer wg.Done() fires correctly even when resolveNavLibraryIDs returns false.

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## CODE REVIEW — bookshelf-ebbx (PR #1090) ### Phase 0: DEMO Verification No runnable DEMO block exists in the bead (bug-fix/audit task). Proceeding on diff review. ### Phase 1: Spec Compliance - CountAllBooks scoped via `WrapInt64WithLibraries(librarystats.MakeCountBooks(...), ...)` — matched ✓ - CountAuthors scoped via `WrapInt64WithLibraries(librarystats.MakeCountAuthors(...), ...)` — matched ✓ - CountBooksByLibrary audited and documented as safe to cache globally — matched ✓ - PR #932 pattern (WrapInt64WithLibraries + resolveNavLibraryIDs) followed exactly — matched ✓ ### Phase 2: Code Quality **Correctness — SQL queries are genuinely scoped** The critical concern (scoped cache over an unscoped query) does not apply here. Both `MakeCountBooks` and `MakeCountAuthors` in `internal/librarystats/store.go` carry genuine `WHERE ... library_id IN (...)` filters and fail-closed on empty input. The nav badge fix is end-to-end, not just a cache-key fix. **Nil vs empty distinction — documentation inconsistency with CountDistinctSeries** `CountDistinctSeries` (established in PR #932) correctly distinguishes nil from empty: ```go // nil = unscoped (global count for unauthenticated callers) if userLibraryIDs != nil && len(userLibraryIDs) == 0 { return 0, nil // fail-closed only for authenticated user with no access } ``` `MakeCountBooks` and `MakeCountAuthors` both use: ```go if len(ids) == 0 { return 0, nil // fires for BOTH nil and empty } ``` The doc comment on `CountAllBooks` and `CountAuthors` in `NavCountDeps` (nav.go) says `nil userLibraryIDs = unscoped; non-nil empty = fail-closed` — but the implementation returns 0 for nil too, contradicting the stated contract. `resolveNavLibraryIDs` returns `(nil, true)` for unauthenticated users (userID==0), so those users see 0 books/authors while seeing the global series count (CountDistinctSeries handles nil correctly). The inconsistency doesn't create a security regression (0 is more restrictive than global), but the misleading doc comment is a latent trap for the next developer. [MINOR] `internal/librarystats/store.go` `MakeCountBooks` line 9, `MakeCountAuthors` line 48 — nil treated as fail-closed, contradicting `nil = unscoped` doc comment and diverging from the CountDistinctSeries precedent in `internal/series/store.go`. Fix: change `if len(ids) == 0` to `if ids != nil && len(ids) == 0` (matching CountDistinctSeries) and add the nil = global-count unscoped path, OR correct the NavCountDeps doc comment to say `nil OR empty = fail-closed`. **CountBooksByLibrary global justification** Accurate. `applyNavCounts` keys into the returned map only for libraries present in the user's sidebar (populated via `listLibraries` with userID scoping), so no cross-user count is exposed. Comment added in both nav.go and app.go confirms this. **app.go wiring** Clean. `WrapInt64WithLibraries` returns nil when fn is nil, but `librarystats.MakeCountBooks` / `MakeCountAuthors` always return non-nil closures, so no nil func-field trap applies here. **Tests** - `package middleware_test` — black-box ✓ - Curried-DI capture pattern (capturedLibIDs) correctly validates IDs are forwarded ✓ - 8 new tests: ID forwarding, fail-closed, error suppression — for both AllBooks and Authors ✓ - One `Expect` per `It` throughout ✓ - `JustBeforeEach` / `BeforeEach` separation maintained ✓ **wg count unchanged** `wg.Add(9)` unchanged. New `resolveNavLibraryIDs` calls are inside existing goroutines (not new ones). `defer wg.Done()` fires correctly even when `resolveNavLibraryIDs` returns false. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor added 1 commit 2026-07-10 23:16:08 +00:00

docs: correct nil-library-IDs fail-closed comment on nav counts 84e780c8ef

zombor merged commit 4444f0ee18 into main 2026-07-10 23:55:56 +00:00

zombor referenced this pull request from a commit 2026-07-10 23:56:01 +00:00

Merge pull request 'fix(nav): per-user library-scope Authors/AllBooks/ByLibrary nav counts (bookshelf-ebbx)' (#1090) from bd-bookshelf-ebbx 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!1090

No description provided.