feat(bookdrop): ComicInfo.xml exact provider-ID extraction (bookshelf-qgd90.5) #1240

Merged

zombor merged 3 commits from bd-bookshelf-qgd90.5 into main 2026-07-26 03:10:45 +00:00

Conversation 2 Commits 3 Files changed 12 +1024 -21

zombor commented 2026-07-25 00:31:21 +00:00

Owner

Copy link

Summary

• Extends metadata.MatchQuery with ExternalID + ExternalProviderID so providers can skip fuzzy search and fetch a precise issue directly by ID
• New internal/bookdrop/comicinfo_provider_id.go parses ComicVine/Metron issue IDs from <Web> URLs and <Notes> annotations in ComicInfo.xml sidecars
• ComicVine provider: exact-by-ID early exit in New via new fetchExactByComicVineID helper (maps ErrIssueNotFound → ErrNoMatch for graceful fallback)
• Metron provider: exact-by-ID early exit in doSearch before the fuzzy resolveSearchFields path
• ImportMetadataForProposals gains a readComicArchive dep (nil = disabled, graceful fallback) and threads the extracted IDs into the per-proposal query
• Wire sites (wire.go, build_extended_deps.go) pass ReadComicArchiveFromPath
• 100% coverage maintained; all parse errors silently ignored (fallback to fuzzy search)

Test plan

• make test passes
• make coverage passes (100% on all three domains: bookdrop, comicvine, metron)
• make lint no new issues from our worktree
• ExtractProviderIDFromComicInfo: WebLink ComicVine, WebLink Metron, Notes annotation, empty/unrecognised, zero-ID guard all tested
• ReadComicArchiveFromPath: CBZ with ComicInfo.xml, CBR delegation, unsupported extension all tested
• ImportMetadataForProposals with stub readComicArchive returning ComicVine/Metron IDs — provider receives ExternalID > 0; error and nil cases fall back to fuzzy search
• ComicVine exact-by-ID: happy path, 404 → ErrNoMatch, 503 → error propagated, volume-fetch failure is non-fatal
• Metron exact-by-ID: happy path, 404 → ErrNoMatch, 503 → error propagated

Closes bead bookshelf-qgd90.5 on merge.

## Summary - Extends `metadata.MatchQuery` with `ExternalID` + `ExternalProviderID` so providers can skip fuzzy search and fetch a precise issue directly by ID - New `internal/bookdrop/comicinfo_provider_id.go` parses ComicVine/Metron issue IDs from `<Web>` URLs and `<Notes>` annotations in `ComicInfo.xml` sidecars - ComicVine provider: exact-by-ID early exit in `New` via new `fetchExactByComicVineID` helper (maps `ErrIssueNotFound` → `ErrNoMatch` for graceful fallback) - Metron provider: exact-by-ID early exit in `doSearch` before the fuzzy `resolveSearchFields` path - `ImportMetadataForProposals` gains a `readComicArchive` dep (nil = disabled, graceful fallback) and threads the extracted IDs into the per-proposal query - Wire sites (`wire.go`, `build_extended_deps.go`) pass `ReadComicArchiveFromPath` - 100% coverage maintained; all parse errors silently ignored (fallback to fuzzy search) ## Test plan - [x] `make test` passes - [x] `make coverage` passes (100% on all three domains: bookdrop, comicvine, metron) - [x] `make lint` no new issues from our worktree - [x] `ExtractProviderIDFromComicInfo`: WebLink ComicVine, WebLink Metron, Notes annotation, empty/unrecognised, zero-ID guard all tested - [x] `ReadComicArchiveFromPath`: CBZ with ComicInfo.xml, CBR delegation, unsupported extension all tested - [x] `ImportMetadataForProposals` with stub `readComicArchive` returning ComicVine/Metron IDs — provider receives `ExternalID > 0`; error and nil cases fall back to fuzzy search - [x] ComicVine exact-by-ID: happy path, 404 → ErrNoMatch, 503 → error propagated, volume-fetch failure is non-fatal - [x] Metron exact-by-ID: happy path, 404 → ErrNoMatch, 503 → error propagated Closes bead bookshelf-qgd90.5 on merge.

zombor added 1 commit 2026-07-25 00:31:22 +00:00

feat(bookdrop): ComicInfo.xml exact provider-ID extraction (bookshelf-qgd90.5) ... ab32f97700

When a comic archive's ComicInfo.xml contains a ComicVine or Metron issue
URL in <Web>, or a [ComicVine:4000-{id}] annotation in <Notes>, the bookdrop
metadata fetch now looks up the EXACT issue by ID via the provider's detail
endpoint instead of doing a fuzzy Series+Issue+Year search.

- Add ExternalID/ExternalProviderID to metadata.MatchQuery
- New comicinfo_provider_id.go: extractProviderIDFromComicInfo + ReadComicArchiveFromPath
- ComicVine: exact-by-ID early exit in New via fetchExactByComicVineID helper
- Metron: exact-by-ID early exit in doSearch via fetchDetail
- readComicArchive dep threaded into ImportMetadataForProposals (nil = fallback)
- Wire sites (wire.go, build_extended_deps.go) updated
- 100% coverage maintained across all three domains

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

zombor commented 2026-07-25 00:49:34 +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 commented 2026-07-25 00:50:26 +00:00

Author

Owner

Copy link

Code Review: AttachFiles Data-Loss Fix (bd-bookshelf-du8hq)

1. Retain-vs-Delete Correctness ✓

The logic correctly implements selective retention:

• bookFileTotalCount counts all source-book files from the DB query
• bookFileReassignCount counts ONLY files that pass all guards (null byte, path confinement, destination existence, move success)
• fullyReassignedBookIDs includes only books where total == reassigned (all files passed)
• Deletion is called ONLY on fullyReassignedBookIDs, preserving books with any skipped file

All five guard paths (null byte, source-confinement, destination-confinement, destination-exists, move-failure) correctly skip the counter increment with continue. A book with even one skipped file will have total > reassigned, preventing its deletion.

Atomic transaction: Both reassign and delete execute in a single runInTx block. If delete fails, reassign rolls back. ✓

2. Over-Deletion Regression Prevention ✓

Books with all files passing guards WILL satisfy total == reassigned, so they ARE still deleted. The happy path is preserved. ✓

3. Coverage Exclusion: Valid Repath, Not New Exclusion ✓

• Pre-fix: books/attach_service.go:137.28,143.14
• Post-fix: books/attach_service.go:146.28,152.14
• Reason: Repath of existing defense-in-depth destination-containment guard (different line numbers due to prepended code)
• New code coverage: bookFileTotalCount loop, bookFileReassignCount increments, and retention logic all have real test coverage—no exclusions hiding them ✓

4. Test Structure & Conventions ✓

• Black-box: package books_test ✓
• One-Expect-per-It: All new It blocks follow this pattern ✓
• Logging: Warn-level with structured key-value pairs (source_book_id, total_files, reassigned_files, trace_id); fires only when total > reassigned ✓
• Test cases added:
  • Null-byte context: 2 books (1 good, 1 with null byte) → only book 1 deleted ✓
  • Rename-fails context: single book with failed move → retained, counts=0 ✓
  • Mixed-batch context: one fully-reassigned, one partial → correct selective deletion ✓

5. Result Struct Assignment ✓

FilesReassigned and SourceBooksRemoved counts reflect actual guarded/deleted behavior. Tests validate both counts. ✓

6. Edge Cases ✓

• Zero-file source book: same pre-fix behavior (not a regression)
• All files skipped: no deletes, counts=0, tested ✓
• Empty request: handled correctly ✓
• reassignCount ≤ totalCount: guaranteed by guard structure ✓

---

REVIEW VERDICT: 0 blockers, 0 majors, 0 minors

This is a correct, focused fix with rigorous test coverage and no regressions. Ready for merge.

## Code Review: AttachFiles Data-Loss Fix (bd-bookshelf-du8hq) ### 1. Retain-vs-Delete Correctness ✓ The logic correctly implements selective retention: - **bookFileTotalCount** counts all source-book files from the DB query - **bookFileReassignCount** counts ONLY files that pass all guards (null byte, path confinement, destination existence, move success) - **fullyReassignedBookIDs** includes only books where `total == reassigned` (all files passed) - **Deletion** is called ONLY on fullyReassignedBookIDs, preserving books with any skipped file All five guard paths (null byte, source-confinement, destination-confinement, destination-exists, move-failure) correctly skip the counter increment with `continue`. A book with even one skipped file will have `total > reassigned`, preventing its deletion. **Atomic transaction:** Both reassign and delete execute in a single `runInTx` block. If delete fails, reassign rolls back. ✓ ### 2. Over-Deletion Regression Prevention ✓ Books with all files passing guards WILL satisfy `total == reassigned`, so they ARE still deleted. The happy path is preserved. ✓ ### 3. Coverage Exclusion: Valid Repath, Not New Exclusion ✓ - **Pre-fix:** `books/attach_service.go:137.28,143.14` - **Post-fix:** `books/attach_service.go:146.28,152.14` - **Reason:** Repath of existing defense-in-depth destination-containment guard (different line numbers due to prepended code) - **New code coverage:** bookFileTotalCount loop, bookFileReassignCount increments, and retention logic all have real test coverage—no exclusions hiding them ✓ ### 4. Test Structure & Conventions ✓ - **Black-box:** `package books_test` ✓ - **One-Expect-per-It:** All new `It` blocks follow this pattern ✓ - **Logging:** Warn-level with structured key-value pairs (source_book_id, total_files, reassigned_files, trace_id); fires only when `total > reassigned` ✓ - **Test cases added:** - Null-byte context: 2 books (1 good, 1 with null byte) → only book 1 deleted ✓ - Rename-fails context: single book with failed move → retained, counts=0 ✓ - Mixed-batch context: one fully-reassigned, one partial → correct selective deletion ✓ ### 5. Result Struct Assignment ✓ FilesReassigned and SourceBooksRemoved counts reflect actual guarded/deleted behavior. Tests validate both counts. ✓ ### 6. Edge Cases ✓ - Zero-file source book: same pre-fix behavior (not a regression) - All files skipped: no deletes, counts=0, tested ✓ - Empty request: handled correctly ✓ - reassignCount ≤ totalCount: guaranteed by guard structure ✓ --- **REVIEW VERDICT: 0 blockers, 0 majors, 0 minors** This is a correct, focused fix with rigorous test coverage and no regressions. Ready for merge.

zombor added 1 commit 2026-07-25 01:00:46 +00:00

fix(bookdrop): remove ExtractProviderIDFromComicInfo export; cover parser via public ImportMetadataForProposals ... 6655f8a889

- Delete `var ExtractProviderIDFromComicInfo = extractProviderIDFromComicInfo` from
  export_test.go — the only white-box export added by this branch, flagged by review.
- Rewrite comicinfo_provider_id_test.go: all extractProviderIDFromComicInfo and
  extractIDFromPattern edge-case branches (ComicVine WebLink, different slug, Metron
  WebLink, Notes annotation, WebLink-over-Notes priority, zero-ID guard, unrecognised URL,
  ErrNoComicInfo fallback, nil readComicArchive) are now covered through the public
  ImportMetadataForProposals function. One Expect per It throughout.
- Fix goimports ordering in import_metadata_service.go: internal/comic now precedes
  internal/db/sqlc (alphabetical within import group).
- Coverage: 100% on internal/ maintained. test-policy-check: clean.

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

zombor referenced this pull request 2026-07-25 11:35:29 +00:00

fix(e2e-browser): BROWSER_PROCS 8→4 + Chrome memory flags to eliminate proc-crash flake (bookshelf-lrlp6) #1242

zombor force-pushed bd-bookshelf-qgd90.5 from 6655f8a889 to 7119c94d37 2026-07-26 03:03:49 +00:00 Compare

zombor merged commit a807e5b9e9 into main 2026-07-26 03:10:45 +00:00

zombor referenced this pull request from a commit 2026-07-26 03:10:48 +00:00

Merge pull request 'feat(bookdrop): ComicInfo.xml exact provider-ID extraction (bookshelf-qgd90.5)' (#1240) from bd-bookshelf-qgd90.5 into main

zombor referenced this pull request 2026-07-26 19:34:47 +00:00

fix(files): cap CBR/CBT/CB7 decompressed-bytes per-load + byte-budgeted page cache (bookshelf-t582g.10) #1250

zombor referenced this pull request 2026-07-26 19:35:13 +00:00

fix(files): cap CBR/CBT/CB7 decompressed-bytes per-load + byte-budgeted page cache (bookshelf-t582g.10) #1250

zombor referenced this pull request 2026-07-26 19:35:19 +00:00

fix(files): cap CBR/CBT/CB7 decompressed-bytes per-load + byte-budgeted page cache (bookshelf-t582g.10) #1250

zombor referenced this pull request 2026-07-26 19:35:51 +00:00

fix(files): cap CBR/CBT/CB7 decompressed-bytes per-load + byte-budgeted page cache (bookshelf-t582g.10) #1250

zombor referenced this pull request 2026-07-26 19:45:20 +00:00

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

zombor referenced this pull request 2026-07-26 21:58:20 +00:00

fix(ratelimit): key rate limiters on real client IP behind trusted proxy (bookshelf-t582g.9) #1247

zombor referenced this pull request 2026-07-26 21:58:42 +00:00

fix(ratelimit): key rate limiters on real client IP behind trusted proxy (bookshelf-t582g.9) #1247

zombor referenced this pull request 2026-07-26 21:58:47 +00:00

fix(bookdrop): prevent source data-loss on staging failure + dest-collision overwrite (bookshelf-t582g.7 + t582g.8) #1251

zombor referenced this pull request 2026-07-26 21:59:16 +00:00

fix(bookdrop): prevent source data-loss on staging failure + dest-collision overwrite (bookshelf-t582g.7 + t582g.8) #1251

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!1240

No description provided.