fix(books): NULL-aware cursor pagination for nullable sort columns (bookshelf-t582g.14) #1252

Merged

zombor merged 1 commit from bd-bookshelf-t582g.14 into main 2026-07-27 00:31:12 +00:00

Conversation 2 Commits 1 Files changed 3 +720 -51

zombor commented 2026-07-26 19:20:48 +00:00

Owner

Copy link

Summary

• Single-field path (rating, page_count, published_date, series): adds (col IS NULL) ASC ISNULL trick to ORDER BY so NULLs sort last in both ASC and DESC, forming a stable total order. Adds a NULL cursor predicate (col IS NULL AND id dir ?) — the missing predicate caused an infinite-scroll loop when paging into the NULL tail.
• Multi-field path (buildMultiSortOrderBy): prepends (col IS NULL) ASC for each nullable sort term; fixes buildMultiCursorPredicate so that for non-NULL cursor values, NULLs are included in the "after" set in both ASC and DESC (they now sort last in both directions).
• Dead code removed: series NULL encodes as "" not nil, so the else if p.CursorID != 0 branch for series was unreachable; deleted.
• Index note: existing (col, book_id) indexes cannot eliminate filesort for the ISNULL-prefixed ORDER BY; a future follow-up covering functional index per nullable column is noted in a comment near nullableCol.

Test plan

• New internal/books/nullable_cursor_test.go covering:
  • ORDER BY includes (col IS NULL) ASC for every nullable column in both single-field and multi-field paths
  • NULL cursor produces an IS NULL predicate (no restart from page 1)
  • Non-NULL cursor includes NULLs in the "after" set for both ASC and DESC
  • published_date ASC + NULL cursor branch (was previously uncovered)
  • DescribeTable pagination simulation for multiple nullable column + direction combinations
• Updated store_test.go assertions that checked the old ORDER BY col dir format now check for the new format with ISNULL trick
• All existing tests pass; 100% coverage gate clean

Closes bead bookshelf-t582g.14 on merge.

## Summary - **Single-field path** (rating, page_count, published_date, series): adds `(col IS NULL) ASC` ISNULL trick to `ORDER BY` so NULLs sort last in both ASC and DESC, forming a stable total order. Adds a `NULL cursor` predicate `(col IS NULL AND id dir ?)` — the missing predicate caused an infinite-scroll loop when paging into the NULL tail. - **Multi-field path** (`buildMultiSortOrderBy`): prepends `(col IS NULL) ASC` for each nullable sort term; fixes `buildMultiCursorPredicate` so that for non-NULL cursor values, NULLs are included in the "after" set in both ASC and DESC (they now sort last in both directions). - **Dead code removed**: series NULL encodes as `""` not nil, so the `else if p.CursorID != 0` branch for series was unreachable; deleted. - **Index note**: existing `(col, book_id)` indexes cannot eliminate filesort for the ISNULL-prefixed ORDER BY; a future follow-up covering functional index per nullable column is noted in a comment near `nullableCol`. ## Test plan - [x] New `internal/books/nullable_cursor_test.go` covering: - ORDER BY includes `(col IS NULL) ASC` for every nullable column in both single-field and multi-field paths - NULL cursor produces an IS NULL predicate (no restart from page 1) - Non-NULL cursor includes NULLs in the "after" set for both ASC and DESC - `published_date` ASC + NULL cursor branch (was previously uncovered) - `DescribeTable` pagination simulation for multiple nullable column + direction combinations - [x] Updated `store_test.go` assertions that checked the old `ORDER BY col dir` format now check for the new format with ISNULL trick - [x] All existing tests pass; 100% coverage gate clean Closes bead bookshelf-t582g.14 on merge.

zombor added 1 commit 2026-07-26 19:20:48 +00:00

fix(books): NULL-aware cursor pagination for nullable sort columns ... 658e494b1c

Single-field path: add `(col IS NULL) ASC` ISNULL trick to ORDER BY for
rating/page_count/published_date/series so NULLs sort last in both ASC
and DESC, forming a stable total order.  Fix the NULL cursor (CursorSortVal
nil) by emitting `(col IS NULL AND id dir ?)` instead of dropping the
predicate — the missing predicate caused an infinite-scroll loop when
paging through the NULL tail.

Multi-field path: extend buildMultiSortOrderBy to prepend `(col IS NULL) ASC`
for each nullable sort term, consistent with the single-field path.  Fix
buildMultiCursorPredicate: now that the ISNULL trick IS emitted in ORDER BY,
a non-NULL cursor value must include `(col IS NULL OR col op ?)` in BOTH ASC
and DESC (NULLs sort last, so they appear after any non-NULL cursor in both
directions).

Add nullable_cursor_test.go — a DescribeTable + per-case Contexts that cover:
• ORDER BY includes ISNULL trick for every nullable column in both paths
• NULL cursor produces an IS NULL predicate (not an empty predicate)
• Non-NULL cursor includes NULLs in the "after" set (they sort last)
• published_date ASC + NULL cursor branch (previously uncovered)

Remove dead code: series NULL is encoded as "" (empty string), not nil, so
the `else if p.CursorID != 0` branch for series was unreachable; deleted it.

Index note: the existing sort indexes (col, book_id) cannot eliminate filesort
for `ORDER BY (col IS NULL) ASC, col dir, id dir`.  A covering functional index
per nullable column would be needed for full ORDER BY elimination; noted in a
comment near nullableCol for a future follow-up.

Closes bead bookshelf-t582g.14 on merge.

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

zombor commented 2026-07-26 19:44:34 +00:00

Author

Owner

Copy link

Security Review — PR #1252 (NULL-aware cursor pagination)

Scope: SQL injection surface of the new nullable-column ORDER BY + cursor predicates in internal/books/store.go; multi-user library scoping / content-restriction integrity; crafted-cursor bypass / unbounded-scan risk.

Findings

No blocker, major, or minor security findings.

Verified — SQL injection (identifiers from allowlist, values parameterized):

• Sort column identifiers come only from sortKeyColumn / nullableCol maps keyed by the SortKey type (store.go:1996, :2016); seriesSortExpr/titleSortExpr are package constants. No user string is ever concatenated into a column position.
• Direction is derived solely from t.Order/p.SortOrder compared against the literal set {asc,desc} — never interpolated raw.
• The boundary (parseSort/parseMultiSort, handler.go:610/:642) rejects any ?sort=/?order= value not in the closed validSortKeys map or {asc,desc} with ErrValidation (400). The store never sees an unlisted key.
• Every cursor value (cv, CursorID) is bound with ?; the new NULL-tail branches (bm.col IS NULL AND bm.book_id op ?) and the (col IS NULL OR col op ?) change in buildMultiCursorPredicate add only parameterized placeholders. CursorSortVal is type-asserted (.(time.Time)/.(int64)/…) with ErrValidation on mismatch.

Verified — multi-user scoping / content restrictions preserved (AND, not replace):

• The final WHERE is strings.Join(wheres, "\n AND ") in both buildListBooksFilteredQuery (store.go:1915) and buildMultiFieldListQuery (:1543). The cursor predicate is appended as one more slice element and thus ANDs with every scoping clause.
• Library scoping + deleted (sharedWheres from buildBookFilterPredicates), the per-user LEFT JOIN user_book_progress … AND ubp.user_id = ?, MagicWhere, and ContentRestrictionPredicates are all appended to wheres before the sort/cursor switch and are untouched by this diff.
• The multi-cursor OR-branch predicate is wrapped in outer parens ("(" + join(branches) + ")"), so its internal ORs cannot disjoin with scoping clauses.

Verified — no crafted-cursor bypass / unbounded scan:

• A crafted CursorID/CursorSortVal only shifts the keyset window; it is ANDed under all scoping predicates, so it cannot reach cross-user or restricted rows.
• LIMIT ? is unconditionally appended on every path (store.go:1918, :1546), so no cursor value produces an unbounded result set.
• The buildListBooksByIDsQuery path and all scoping predicates are unmodified by this diff (all 6 hunks are confined to sort/cursor ORDER BY logic).

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1252 (NULL-aware cursor pagination) Scope: SQL injection surface of the new nullable-column ORDER BY + cursor predicates in `internal/books/store.go`; multi-user library scoping / content-restriction integrity; crafted-cursor bypass / unbounded-scan risk. ### Findings No blocker, major, or minor security findings. **Verified — SQL injection (identifiers from allowlist, values parameterized):** - Sort column identifiers come only from `sortKeyColumn` / `nullableCol` maps keyed by the `SortKey` type (`store.go:1996`, `:2016`); `seriesSortExpr`/`titleSortExpr` are package constants. No user string is ever concatenated into a column position. - Direction is derived solely from `t.Order`/`p.SortOrder` compared against the literal set `{asc,desc}` — never interpolated raw. - The boundary (`parseSort`/`parseMultiSort`, `handler.go:610`/`:642`) rejects any `?sort=`/`?order=` value not in the closed `validSortKeys` map or `{asc,desc}` with `ErrValidation` (400). The store never sees an unlisted key. - Every cursor value (`cv`, `CursorID`) is bound with `?`; the new NULL-tail branches `(bm.col IS NULL AND bm.book_id op ?)` and the `(col IS NULL OR col op ?)` change in `buildMultiCursorPredicate` add only parameterized placeholders. `CursorSortVal` is type-asserted (`.(time.Time)`/`.(int64)`/…) with `ErrValidation` on mismatch. **Verified — multi-user scoping / content restrictions preserved (AND, not replace):** - The final WHERE is `strings.Join(wheres, "\n AND ")` in both `buildListBooksFilteredQuery` (`store.go:1915`) and `buildMultiFieldListQuery` (`:1543`). The cursor predicate is appended as one more slice element and thus ANDs with every scoping clause. - Library scoping + `deleted` (`sharedWheres` from `buildBookFilterPredicates`), the per-user `LEFT JOIN user_book_progress … AND ubp.user_id = ?`, `MagicWhere`, and `ContentRestrictionPredicates` are all appended to `wheres` before the sort/cursor switch and are untouched by this diff. - The multi-cursor OR-branch predicate is wrapped in outer parens (`"(" + join(branches) + ")"`), so its internal ORs cannot disjoin with scoping clauses. **Verified — no crafted-cursor bypass / unbounded scan:** - A crafted `CursorID`/`CursorSortVal` only shifts the keyset window; it is ANDed under all scoping predicates, so it cannot reach cross-user or restricted rows. - `LIMIT ?` is unconditionally appended on every path (`store.go:1918`, `:1546`), so no cursor value produces an unbounded result set. - The `buildListBooksByIDsQuery` path and all scoping predicates are unmodified by this diff (all 6 hunks are confined to sort/cursor ORDER BY logic). ### REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-07-26 19:45:20 +00:00

Author

Owner

Copy link

Security Review — PR #1240 (ComicInfo.xml → exact-by-ID provider fetch)

Adversarial focus: ComicInfo.xml is attacker-controlled (inside an untrusted .cbz/.cbr).

SSRF (untrusted <Web>/<Notes> steering outbound host) — CLEAN. The raw
URL is never used as a request target. extractProviderIDFromComicInfo
(internal/bookdrop/comicinfo_provider_id.go) only regex-captures a numeric issue
ID (4000-(\d+), metron\.cloud/issue/(\d+), \[ComicVine:4000-(\d+)\]). Both
providers build the request from the FIXED provider base URL:

• comicvine: fmt.Sprintf("%s/issue/4000-%d/?%s", baseURL, issueID, ...) (internal/metadata/comicvine/search.go:778), baseURL = defaultBaseURL/test server.
• metron: fmt.Sprintf("%s/issue/%d/", d.baseURL, issueID) (internal/metadata/metron/provider.go:425).
  The persisted WebLink also passes through urlutil.SafeURL (internal/comic/persist.go:163).

Injection / request-splitting — CLEAN. The ID is parsed with
strconv.ParseInt(match[1], 10, 64) and rejected unless > 0; it is emitted as
%d. No non-numeric content can reach the path/query. Provider tag is a fixed
literal ("comicvine"/"metron"), never concatenated into a URL.

XXE / XML bomb — CLEAN. Parsing uses Go encoding/xml (decodeComicInfoReader,
internal/comic/comicinfo.go:258) with no CharsetReader and no custom Entity
map — external/DTD entities are not resolved by default. Decompressed size is
double-bounded: the zip/rar header uncompressed-size guard rejects > maxComicInfoBytes (1 MB) before open, and an io.LimitReader(rc, cap) caps the
decode. Archive read is io.LimitReader(f, 512MB). Zip entry is read, not
extracted to disk (no zip-slip).

API keys — CLEAN. New log lines emit only issue_id/volume_id/err
(internal/metadata/comicvine/provider.go); no api_key in the new code.

Permanent vs transient — CORRECT. A malformed/missing/unsupported archive →
readComicArchive error/ErrNoComicInfo is swallowed and falls back to fuzzy
search (fetchMergedCandidateForProposal). A provider 404 for an
attacker-forged ID maps to metadata.ErrNoMatch (fall through to other
providers), not a hard error. Exact-by-ID fetch still runs under the existing
rate limiter and only for already-enabled/active providers.

[MINOR] internal/bookdrop/import_metadata_service.go:15 — import out of goimports order
internal/comic is placed after internal/db/sqlc (comic < db/sqlc). If
golangci-lint/gci is not flagging it, harmless, but reorder for tidiness. No
security impact.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security Review — PR #1240 (ComicInfo.xml → exact-by-ID provider fetch) Adversarial focus: ComicInfo.xml is attacker-controlled (inside an untrusted .cbz/.cbr). **SSRF (untrusted `<Web>`/`<Notes>` steering outbound host) — CLEAN.** The raw URL is never used as a request target. `extractProviderIDFromComicInfo` (`internal/bookdrop/comicinfo_provider_id.go`) only regex-captures a numeric issue ID (`4000-(\d+)`, `metron\.cloud/issue/(\d+)`, `\[ComicVine:4000-(\d+)\]`). Both providers build the request from the FIXED provider base URL: - comicvine: `fmt.Sprintf("%s/issue/4000-%d/?%s", baseURL, issueID, ...)` (`internal/metadata/comicvine/search.go:778`), `baseURL` = `defaultBaseURL`/test server. - metron: `fmt.Sprintf("%s/issue/%d/", d.baseURL, issueID)` (`internal/metadata/metron/provider.go:425`). The persisted `WebLink` also passes through `urlutil.SafeURL` (`internal/comic/persist.go:163`). **Injection / request-splitting — CLEAN.** The ID is parsed with `strconv.ParseInt(match[1], 10, 64)` and rejected unless `> 0`; it is emitted as `%d`. No non-numeric content can reach the path/query. Provider tag is a fixed literal (`"comicvine"`/`"metron"`), never concatenated into a URL. **XXE / XML bomb — CLEAN.** Parsing uses Go `encoding/xml` (`decodeComicInfoReader`, `internal/comic/comicinfo.go:258`) with no `CharsetReader` and no custom `Entity` map — external/DTD entities are not resolved by default. Decompressed size is double-bounded: the zip/rar header uncompressed-size guard rejects `> maxComicInfoBytes` (1 MB) before open, and an `io.LimitReader(rc, cap)` caps the decode. Archive read is `io.LimitReader(f, 512MB)`. Zip entry is read, not extracted to disk (no zip-slip). **API keys — CLEAN.** New log lines emit only `issue_id`/`volume_id`/`err` (`internal/metadata/comicvine/provider.go`); no `api_key` in the new code. **Permanent vs transient — CORRECT.** A malformed/missing/unsupported archive → `readComicArchive` error/`ErrNoComicInfo` is swallowed and falls back to fuzzy search (`fetchMergedCandidateForProposal`). A provider 404 for an attacker-forged ID maps to `metadata.ErrNoMatch` (fall through to other providers), not a hard error. Exact-by-ID fetch still runs under the existing rate limiter and only for already-enabled/active providers. [MINOR] internal/bookdrop/import_metadata_service.go:15 — import out of goimports order `internal/comic` is placed after `internal/db/sqlc` (comic < db/sqlc). If golangci-lint/gci is not flagging it, harmless, but reorder for tidiness. No security impact. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor force-pushed bd-bookshelf-t582g.14 from 658e494b1c to b6c551bfba 2026-07-27 00:21:34 +00:00 Compare

zombor merged commit c14db82629 into main 2026-07-27 00:31:12 +00:00

zombor referenced this pull request from a commit 2026-07-27 00:31:15 +00:00

Merge pull request 'fix(books): NULL-aware cursor pagination for nullable sort columns (bookshelf-t582g.14)' (#1252) from bd-bookshelf-t582g.14 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!1252

No description provided.