fix(books): ErrNoMatch on Fetch Metadata shows calm 'no match' instead of scary 404 (bookshelf-zedtf) #1245
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-zedtf"
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
RefetchMetadataHandlerpreviously mappedmetadata.ErrNoMatch→middleware.ErrNotFound→ HTTP 404{"error":"Not Found"}. The JS controller showed "Error: server returned 404" when providers had no match for a book — a legitimate, non-error outcome presented as a broken endpoint.{"matched":false,"cover_pending":false}(JSON) or redirects to/books/{id}?no_match=1(browser) onErrNoMatch. All other errors still propagate unchanged.refetchMetadataResponsegains aMatched boolfield:trueon successful match,falseon no-match.matched===falseand shows neutral status "No metadata match found for this book", re-enables button, and does NOT navigate. Only non-2xx/network errors show "Error: server returned N".refetchNoMatchResponse+refetchMatchResponsehelpers to keepRefetchMetadataHandlerwithin thefunlengate.FetchCandidatesalready handlesErrNoMatchgracefully (empty candidates list, not 404). No sibling bead needed.Test plan
ErrNoMatch→ 200 +{matched:false}(NOT 404); successful match → 200 +{matched:true}; non-ErrNoMatcherrors still propagate; browser redirect paths both testedfailWritermatched:false→ neutral status + button re-enabled + no navigation;matched:true+cover_pending:true→ navigates with?cover_pending=1;matched:true+cover_pending:false→ navigates with?refreshed=1; non-2xx still shows errormake coveragepasses (100% oninternal/)make lintclean on this worktree's filesCloses bead bookshelf-zedtf on merge.
When all metadata providers return no match for a book, the handler now responds with HTTP 200 {"matched":false,"cover_pending":false} (JSON) or redirects to /books/{id}?no_match=1 (browser) instead of propagating middleware.ErrNotFound which rendered as "Error: server returned 404". The JS Stimulus controller reads matched===false and shows a neutral "No metadata match found for this book" status message, re-enables the button, and does not navigate — distinguishing a legitimate no-match from a real server error. Also adds the Matched bool field to refetchMetadataResponse so JSON clients can distinguish match vs no-match outcomes. On a successful match, matched=true is now explicitly set. Extracts refetchNoMatchResponse and refetchMatchResponse helpers to keep RefetchMetadataHandler within the funlen gate. Closes bead bookshelf-zedtf on merge. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Security review — PR #1239 (
bd-bookshelf-cs2zl.2)New
POST /books/bulk/filter/scan-file(LLM scan-file over all filter-matching / whole-library books) + library kebab trigger.Multi-user scoping — PASS (fail-closed).
bulkScanFileFilterRequestreuses the sharedbulkFilterHandler:userIDis taken from the session (userIDFromRequest(r)), never the body;library_id/shelf_id/statusare re-scoped to that userID (ShelfUserID/StatusUserID);magic_shelf_idis ownership-checked viacheckMagicShelfAccess(404 on miss). Book-ID resolution re-resolvesuserLibraryIDsper ContinueAsNew epoch inbuildListFilteredIDsPageFnand passes them toListFilteredBookIDsPage, whose predicate is fail-closed (internal/books/filter_predicates.go:104→library_id IN (...); non-nil empty →1=0).users.GetUserLibraryIDsnormalizesnil → []int64{}, so a zero-library user takes the1=0branch — a body-suppliedlibrary_idthe user cannot access yields no rows, not a cross-user leak.Auth — PASS. Route is gated
g.BulkScanFile(...)→BookBulkScanFileRequired→users.PermissionRequired(..., PermissionBulkAutoFetchMetadata)(internal/app/app.go:355), same real permission as the sibling by-IDs endpoint. Not "any logged-in user."Resource-exhaustion / cost DoS — PASS. Fan-out is bounded single-digit (
defaultFanOutConcurrency = 4,internal/wfengine/fanout.go:19) and sub-workflows route to the LLM queue (scanFileFanOutOptions) so vision activities respect the GPU/concurrency cap; the kebab entry is gated behind{{if $.LLMVisionAvailable}}and adialog.confirmcount prompt.Injection — PASS. All SQL is sqlc/parameterized;
view_queryvalidated viaParseViewQueryFilterat the boundary; status/format/metadata filters allowlist-validated; audit action is a constant.Workflow versioning — SAFE (no gate needed). The new
case BulkFilterOpScanFileinbulkByFilterApplyOpis selected by the per-instance-immutableinput.Op; in-flight instances carry a differentOpand keep their original command sequence, so replay does not diverge.Findings
[MINOR] templates/layouts/base.html:200 — count-confirmation shows 0 for the largest libraries
data-...-book-count-value="{{if .HasCount}}{{.Count}}{{else}}0{{end}}"falls back to 0 when the count is unavailable (HasCountfalse) — which per the Scale convention is exactly the large/unfiltered libraries where the "you are about to scan N files … uses your LLM budget" confirmation matters most. The most expensive case shows the least alarming number, weakening the secondary cost guard. Authorization/scoping/permission gates still fully protect the operation, so this is UX-quality, not a vulnerability. Suggest an indeterminate message ("all files in this library") whenHasCountis false rather than "0 files".REVIEW VERDICT: 0 blocker, 0 major, 1 minor
[MINOR] internal/books/metadata_store_test.go:709-710 — multiple Expect calls in single It block
This test has two Expect calls:
Expect(handlerErr).To(HaveOccurred())Expect(handlerErr.Error()).To(ContainSubstring("encode"))Project convention (project-conventions.md) requires exactly one Expect per It block. While there is precedent for this pattern in metadata_handler_test.go (ListProvidersHandler encode-error test), the convention should be followed. Fix: Split into two It blocks or combine into a single assertion. Note: This does not impact correctness or functionality, only test style consistency.
REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Security Review — PR #1245 (bd-bookshelf-zedtf)
Scope: metadata-refetch handler now returns HTTP 200
{"matched":false}(was 404) when providers find no match, plus a?no_match=1browser-redirect param. Checked authz preservation, injection/open-redirect, and existence-oracle leak.Findings
No blockers, majors, or minors.
(1) Ownership/authz check preserved — PASS.
RefetchMetadataHandlercallscheckBookAccess(ctx, userIDFromRequest(r), id)and returns its error unchanged beforerefetch(...)runs. Onlymetadata.ErrNoMatchfrom the post-access-checkrefetchcall is converted to 200; every non-ErrNoMatcherror still propagates. A book the user cannot access still fails atcheckBookAccess, which returns wrappedmiddleware.ErrNotFound(uniform 404) for all denial cases — nonexistent, unowned, content-restricted, unauthenticated (userID==0). The 200 no-match path is unreachable for an inaccessible book. (internal/books/metadata_handler.goRefetchMetadataHandler;internal/books/service.go:684CheckBookAccess)(2) No injection / open-redirect — PASS. The
?no_match=1and?cover_pending=1params are static string literals. Redirect targets arefmt.Sprintf("/books/%d...", id)whereidis anint64produced byparseID(strconv.ParseInt, base-10) — non-numeric input is rejected as a validation error before any formatting, so%dcannot inject path/query/CRLF content. The redirect destination is a fixed relative path (no user-controlled URL) — no open-redirect. JSON body is static booleans only; no user-influenced content reflected.(3) No new existence oracle — PASS. "Book exists but no provider match" (200) is only reachable after a successful
checkBookAccess, i.e. only for books the requesting user can already access. "Book not accessible" still returns uniform 404. An attacker cannot use the 200-vs-404 split to distinguish existence of a book they lack access to — the access check gates the 200 path entirely.Tests (Go handler specs + Vitest controller specs) cover both JSON and browser no-match paths and the matched=true path; the
checkBookAccessdenial paths remain covered by existing specs.REVIEW VERDICT: 0 blocker, 0 major, 0 minor