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

Merged

zombor merged 3 commits from bd-bookshelf-t582g.7 into main 2026-07-27 02:46:21 +00:00

Conversation 7 Commits 3 Files changed 6 +877 -161

zombor commented 2026-07-26 19:13:18 +00:00

Owner

Copy link

Summary

• BUG 1 (t582g.7): stageFile used os.Rename to move the source into the staging temp on same-device. Any subsequent failure (TX error, finalize error) cleaned up the temp file — destroying the user's only copy while the proposal stayed PENDING_REVIEW. Fix: remove the rename fast-path entirely; always copy+fsync so the source is always intact for retry.
• BUG 2 (t582g.8): AcceptProposal computed destPath = filepath.Join(destDir, safeFileName) with no existence check. Two proposals with the same base filename into the same library path silently overwrote the first accepted file. Fix: uniquifyDestPath stat-probes before staging and appends (N) before the extension on collision (up to N=999). StatFile is injectable for tests, defaults to os.Stat in production.

Test plan

• StageFile source-intact guarantee — asserts source survives a staged sync-fault injection (would have caught BUG 1)
• AcceptProposal — source file not removed on staging or TX failure — asserts RemoveFile is NOT called with the source path on failure (service-level BUG 1 coverage)
• AcceptProposal — dest-filename collision uniquification — asserts second accept lands at novel (2).epub, not novel.epub (BUG 2)
• UniquifyDestPath unit tests — no collision, first collision, no extension, all-slots-exhausted error
• AcceptProposal — uniquify error propagation — 999-slot exhaustion returns an error
• make coverage passes (100% gate — zero uncovered statement blocks)
• make lint passes
• All existing tests unaffected

Closes bead bookshelf-t582g.7 and bookshelf-t582g.8 on merge.

## Summary - **BUG 1 (t582g.7):** `stageFile` used `os.Rename` to move the source into the staging temp on same-device. Any subsequent failure (TX error, finalize error) cleaned up the temp file — destroying the user's only copy while the proposal stayed `PENDING_REVIEW`. Fix: remove the rename fast-path entirely; always copy+fsync so the source is always intact for retry. - **BUG 2 (t582g.8):** `AcceptProposal` computed `destPath = filepath.Join(destDir, safeFileName)` with no existence check. Two proposals with the same base filename into the same library path silently overwrote the first accepted file. Fix: `uniquifyDestPath` stat-probes before staging and appends ` (N)` before the extension on collision (up to N=999). `StatFile` is injectable for tests, defaults to `os.Stat` in production. ## Test plan - [x] `StageFile source-intact guarantee` — asserts source survives a staged sync-fault injection (would have caught BUG 1) - [x] `AcceptProposal — source file not removed on staging or TX failure` — asserts `RemoveFile` is NOT called with the source path on failure (service-level BUG 1 coverage) - [x] `AcceptProposal — dest-filename collision uniquification` — asserts second accept lands at `novel (2).epub`, not `novel.epub` (BUG 2) - [x] `UniquifyDestPath` unit tests — no collision, first collision, no extension, all-slots-exhausted error - [x] `AcceptProposal — uniquify error propagation` — 999-slot exhaustion returns an error - [x] `make coverage` passes (100% gate — zero uncovered statement blocks) - [x] `make lint` passes - [x] All existing tests unaffected Closes bead bookshelf-t582g.7 and bookshelf-t582g.8 on merge.

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

fix(bookdrop): prevent source-file data loss and dest-collision overwrite (t582g.7 + t582g.8) ... af02c436e0

BUG 1 (t582g.7): stageFile used os.Rename to move the source into the
staging temp on same-device. Any failure after staging (TX error, finalize
error) cleaned up the temp — destroying the user's only copy while the
proposal stayed PENDING_REVIEW with no retry possible.

Fix: remove the rename fast-path entirely. stageFile now always
copy+fsyncs the source, leaving it intact for retry on any failure.
The source is only removed from the bookdrop dir after a successful
commit AND finalize.

BUG 2 (t582g.8): AcceptProposal built the dest path as
filepath.Join(destDir, safeFileName) with no existence check. Two
proposals with the same base filename into the same library path would
silently overwrite the first accepted file.

Fix: add uniquifyDestPath (stat-probe before staging) that appends
" (N)" before the extension when a collision is detected, up to N=999.
StatFile is injectable for tests; defaults to os.Stat in production.

Tests added:
- StageFile source-intact guarantee (asserts source survives sync-fault)
- AcceptProposal source-not-removed on TX/finalize failure
- AcceptProposal dest-collision uniquification (novel.epub → novel (2).epub)
- UniquifyDestPath unit tests (no collision, first collision, no extension, exhaustion)
- AcceptProposal uniquify error propagation (all 999 slots taken)

Closes bead bookshelf-t582g.7 and bookshelf-t582g.8 on merge.

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

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

Author

Owner

Copy link

Security review — PR #1251 (bd-bookshelf-t582g.7)

Scope: bookdrop accept staging now always copies (no source-move) + uniquifies dest filenames. Adversarial focus on path-traversal, symlink/TOCTOU, data-loss windows, and temp-file perms/cleanup.

[MAJOR] internal/bookdrop/review_service.go:786 — Uniquify→finalize is a check-then-rename TOCTOU that can still overwrite under concurrent accepts
uniquifyDestPath picks a free name by statFile (default os.Stat), but the actual placement is a much-later FinalizeFile(tempPath, destPath) = os.Rename (build_extended_deps.go:1806), which on POSIX silently overwrites an existing destination. Two accepts of distinct proposals sharing the same base filename (duplicate uploads, or same-named files in different bookdrop subfolders) can both stat novel (2).epub as free, then both rename — the second clobbers the first file's bytes while both DB rows point at the same path. This is exactly the overwrite the PR sets out to prevent, defeated under concurrency (bookdrop accept runs both from the HTTP handler and the wfengine PENDING_ACCEPT worker path, so simultaneity is reachable). Fix: make finalize refuse to clobber — link/rename with an existence guard (e.g. os.Link+os.Remove(temp), or open dest O_CREATE|O_EXCL and copy, or renameatx/RENAME_NOREPLACE where available) and, on EEXIST, re-run uniquify and retry. A pure stat-then-rename cannot be made race-free.

[MINOR] internal/bookdrop/stage.go:80 — Staged file (and thus final library file) is fixed at 0600, losing the source file's mode
The removed rename fast-path moved the source inode, preserving its permissions; the always-copy path uses os.CreateTemp (0600) and os.Rename preserves that mode into the library. Library files that previously landed group/other-readable now land owner-only. Not a security hole (tighter, not looser), but a behavior regression for multi-process/host setups reading the library. Consider chmod-ing the temp to the source's mode (or a configured library file mode) before finalize.

[MINOR] internal/bookdrop/review_service.go:660 — uniquify treats ALL stat errors as "no collision", masking a permission/IO error as a free slot
if _, statErr := statFile(candidate); statErr != nil { return candidate ... } returns the path free on any non-nil error, not just os.ErrNotExist. An EACCES/EIO on the dest dir is read as "safe to write", deferring the real error to the rename. Comment acknowledges this, and impact is low (rename surfaces the error), but distinguishing errors.Is(statErr, fs.ErrNotExist) from other errors would fail fast and avoid a misleading "collision-free" decision. (Note: the filepath.Base(f.FileName) == ".." / "" traversal edge is NOT reachable here — FileName is filepath.Base of a real on-disk scanned file (service.go:143), which can never be ../.; no finding.)

Positive confirmations:

• No path traversal via uniquify: name is filepath.Base(f.FileName) (no separators) and the (N) suffix inserts no separators, so every candidate stays inside destDir.
• No write-through-symlink at finalize: os.Rename replaces a dangling/real symlink at destPath rather than following it out of the tree; destDir itself is symlink-confined via confinedDestDir.
• Source is genuinely never moved (copy+fsync only) — the data-loss window from the old rename-fast-path is closed; failure paths remove only the temp, never f.FilePath (verified by the new source-intact tests).
• Temp created via os.CreateTemp (0600, O_EXCL) and cleaned up on every staging error branch.

REVIEW VERDICT: 0 blocker, 1 major, 2 minor

## Security review — PR #1251 (bd-bookshelf-t582g.7) Scope: bookdrop accept staging now always copies (no source-move) + uniquifies dest filenames. Adversarial focus on path-traversal, symlink/TOCTOU, data-loss windows, and temp-file perms/cleanup. [MAJOR] internal/bookdrop/review_service.go:786 — Uniquify→finalize is a check-then-rename TOCTOU that can still overwrite under concurrent accepts `uniquifyDestPath` picks a free name by `statFile` (default `os.Stat`), but the actual placement is a much-later `FinalizeFile(tempPath, destPath)` = `os.Rename` (build_extended_deps.go:1806), which on POSIX **silently overwrites** an existing destination. Two accepts of distinct proposals sharing the same base filename (duplicate uploads, or same-named files in different bookdrop subfolders) can both stat `novel (2).epub` as free, then both rename — the second clobbers the first file's bytes while both DB rows point at the same path. This is exactly the overwrite the PR sets out to prevent, defeated under concurrency (bookdrop accept runs both from the HTTP handler and the wfengine PENDING_ACCEPT worker path, so simultaneity is reachable). Fix: make finalize refuse to clobber — link/rename with an existence guard (e.g. `os.Link`+`os.Remove(temp)`, or open dest `O_CREATE|O_EXCL` and copy, or `renameatx`/`RENAME_NOREPLACE` where available) and, on `EEXIST`, re-run uniquify and retry. A pure stat-then-rename cannot be made race-free. [MINOR] internal/bookdrop/stage.go:80 — Staged file (and thus final library file) is fixed at 0600, losing the source file's mode The removed rename fast-path moved the source inode, preserving its permissions; the always-copy path uses `os.CreateTemp` (0600) and `os.Rename` preserves that mode into the library. Library files that previously landed group/other-readable now land owner-only. Not a security hole (tighter, not looser), but a behavior regression for multi-process/host setups reading the library. Consider chmod-ing the temp to the source's mode (or a configured library file mode) before finalize. [MINOR] internal/bookdrop/review_service.go:660 — uniquify treats ALL stat errors as "no collision", masking a permission/IO error as a free slot `if _, statErr := statFile(candidate); statErr != nil { return candidate ... }` returns the path free on any non-nil error, not just `os.ErrNotExist`. An `EACCES`/`EIO` on the dest dir is read as "safe to write", deferring the real error to the rename. Comment acknowledges this, and impact is low (rename surfaces the error), but distinguishing `errors.Is(statErr, fs.ErrNotExist)` from other errors would fail fast and avoid a misleading "collision-free" decision. (Note: the `filepath.Base(f.FileName) == ".."` / "" traversal edge is NOT reachable here — `FileName` is `filepath.Base` of a real on-disk scanned file (service.go:143), which can never be `..`/`.`; no finding.) Positive confirmations: - No path traversal via uniquify: `name` is `filepath.Base(f.FileName)` (no separators) and the ` (N)` suffix inserts no separators, so every candidate stays inside `destDir`. - No write-through-symlink at finalize: `os.Rename` replaces a dangling/real symlink at `destPath` rather than following it out of the tree; `destDir` itself is symlink-confined via `confinedDestDir`. - Source is genuinely never moved (copy+fsync only) — the data-loss window from the old rename-fast-path is closed; failure paths remove only the temp, never `f.FilePath` (verified by the new source-intact tests). - Temp created via `os.CreateTemp` (0600, O_EXCL) and cleaned up on every staging error branch. REVIEW VERDICT: 0 blocker, 1 major, 2 minor

zombor force-pushed bd-bookshelf-t582g.7 from af02c436e0 to 89bc69a78e 2026-07-26 19:51:59 +00:00 Compare

zombor commented 2026-07-26 21:58:35 +00:00

Author

Owner

Copy link

[MAJOR] internal/bookdrop/review_service.go:997 — LinkFile nil-default not enforced
The AcceptProposalParams.LinkFile documentation states "Defaults to os.Link when nil"
but line 997 directly assigns linkFn := p.LinkFile without checking for nil. If a
caller creates AcceptProposalParams with LinkFile=nil (violating the code's contract
but matching the documented fallback), finalizeWithRetry will attempt to call a nil
function at line 728, causing a panic. The current wiring in build_extended_deps.go
provides os.Link explicitly, so this doesn't manifest in production, but the code
violates its own documented contract. Fix: add nil-check before finalizeWithRetry
(pattern: if linkFn == nil { linkFn = os.Link }, matching statFn at line 882 and
mkdirAll at line 890).

[MINOR] internal/bookdrop/stage.go:108-110 — chmod timing window
The temp file is created with mode 0o600 (secure), then copied+fsynced, then chmod'd
to 0o644. Between fsync (line 97-101) and chmod (line 111), the file is world-
readable but not yet hard-linked to its final location. Since the temp is in destDir
(a trusted library path), this is acceptable, but it's a subtle security assumption.
Not a correctness bug, just worth documenting. No fix needed, but a clarifying
comment in stage.go (line 108) explaining "os.CreateTemp creates 0o600 for security;
we chmod to 0o644 here so the linked-into-place file is readable by library processes"
would help future reviewers.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

[MAJOR] internal/bookdrop/review_service.go:997 — LinkFile nil-default not enforced The AcceptProposalParams.LinkFile documentation states "Defaults to os.Link when nil" but line 997 directly assigns linkFn := p.LinkFile without checking for nil. If a caller creates AcceptProposalParams with LinkFile=nil (violating the code's contract but matching the documented fallback), finalizeWithRetry will attempt to call a nil function at line 728, causing a panic. The current wiring in build_extended_deps.go provides os.Link explicitly, so this doesn't manifest in production, but the code violates its own documented contract. Fix: add nil-check before finalizeWithRetry (pattern: if linkFn == nil { linkFn = os.Link }, matching statFn at line 882 and mkdirAll at line 890). [MINOR] internal/bookdrop/stage.go:108-110 — chmod timing window The temp file is created with mode 0o600 (secure), then copied+fsynced, then chmod'd to 0o644. Between fsync (line 97-101) and chmod (line 111), the file is world- readable but not yet hard-linked to its final location. Since the temp is in destDir (a trusted library path), this is acceptable, but it's a subtle security assumption. Not a correctness bug, just worth documenting. No fix needed, but a clarifying comment in stage.go (line 108) explaining "os.CreateTemp creates 0o600 for security; we chmod to 0o644 here so the linked-into-place file is readable by library processes" would help future reviewers. REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor commented 2026-07-26 21:58:47 +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-26 21:59:15 +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-26 21:59:23 +00:00

Author

Owner

Copy link

Security re-review — PR #1251 (concurrent-accept TOCTOU MAJOR)

Verified git diff origin/main...origin/bd-bookshelf-t582g.7.

The concurrent-accept clobbering MAJOR is CLOSED. Finalize now uses a non-clobbering os.Link (fails EEXIST when the dest name already exists — os.Link does not follow a symlink at the target and never write-through-clobbers) with a bounded re-uniquify retry (finalizeWithRetry, review_service.go:695-740). Two concurrent accepts of same-base-named proposals can no longer both land on the same path: the link is the atomic exclusive claim; the race loser gets EEXIST and advances to the next (N) suffix. Confirmed via internal/app/build_extended_deps.go:1806 wiring LinkFile: os.Link and the EEXIST-retry test (review_service_test.go:2051-2110).

Checklist against the four points:

1. Atomic/exclusive placement — YES. os.Link+EEXIST is the mutual-exclusion primitive; the winner's file is never overwritten.
2. No new symlink-follow / traversal — CLEAN. Temp is os.CreateTemp(destDir,...) inside the confined destDir; every link candidate is filepath.Join(destDir, base+"(N)"+ext) off a filepath.Base'd name with a numeric suffix. No separators, no .., no follow-through.
3. Temp perms — ACCEPTABLE. 0644 is applied only after content is copied+fsync+closed (stage.go:107-111); during the write window the file is 0600 in the library dir (not a world-writable tmp). 0644 read-only is the intended final library-file mode.
4. Residual TOCTOU — see MAJOR below (correctness, not a security escalation).

[MAJOR] internal/bookdrop/review_service.go:998 — DB records the pre-finalize filename; EEXIST-retry path leaves a dangling book_file reference
The TX records FileName: safeFileName / FileSubPath: fileSubPath (review_service.go:955-957), computed by uniquifyDestPath BEFORE the TX (line 876). finalizeWithRetry runs AFTER the commit (line 998) and, on EEXIST, advances the on-disk path to a different suffix (e.g. DB says novel.epub but the file links to novel (2).epub — exactly the path the EEXIST-retry test asserts, review_service_test.go:2106-2107). Result in the concurrent-race path: the committed book_file row points at a path that does not exist, and the actual file is orphaned on disk (a rescan may re-import it as a duplicate). This is the security fix trading a clobber for a dangling reference — the winner's data is safe (so no security escalation), but it is a real data-integrity regression on the very race this PR fixes. Fix: capture the final linked path/name returned from finalizeWithRetry and reconcile the book_file row (update file_name/file_sub_path) after finalize, or move the link before the TX and record the actually-claimed name. The existing test masks this by stubbing upsertBookFile as a no-op — add an assertion that the stored FileName equals the finally-linked name in the EEXIST-retry case.

[MINOR] internal/bookdrop/review_service.go:997 — LinkFile has no nil-default despite the doc claiming one
The doc comment (review_service.go:272) says "Defaults to os.Link when nil," but linkFn := p.LinkFile is passed straight into finalizeWithRetry with no if linkFn == nil { linkFn = os.Link } guard (contrast StatFile/MkdirAll, which are defaulted). Production wiring passes os.Link so this is not exploitable, but a nil would panic and the code/doc disagree. Add the nil-default to match the contract.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Security re-review — PR #1251 (concurrent-accept TOCTOU MAJOR) Verified `git diff origin/main...origin/bd-bookshelf-t582g.7`. **The concurrent-accept clobbering MAJOR is CLOSED.** Finalize now uses a non-clobbering `os.Link` (fails `EEXIST` when the dest name already exists — `os.Link` does not follow a symlink at the target and never write-through-clobbers) with a bounded re-uniquify retry (`finalizeWithRetry`, review_service.go:695-740). Two concurrent accepts of same-base-named proposals can no longer both land on the same path: the link is the atomic exclusive claim; the race loser gets `EEXIST` and advances to the next `(N)` suffix. Confirmed via `internal/app/build_extended_deps.go:1806` wiring `LinkFile: os.Link` and the EEXIST-retry test (review_service_test.go:2051-2110). Checklist against the four points: 1. Atomic/exclusive placement — YES. `os.Link`+EEXIST is the mutual-exclusion primitive; the winner's file is never overwritten. 2. No new symlink-follow / traversal — CLEAN. Temp is `os.CreateTemp(destDir,...)` inside the confined `destDir`; every link candidate is `filepath.Join(destDir, base+"(N)"+ext)` off a `filepath.Base`'d name with a numeric suffix. No separators, no `..`, no follow-through. 3. Temp perms — ACCEPTABLE. 0644 is applied only *after* content is copied+fsync+closed (stage.go:107-111); during the write window the file is 0600 in the library dir (not a world-writable tmp). 0644 read-only is the intended final library-file mode. 4. Residual TOCTOU — see MAJOR below (correctness, not a security escalation). [MAJOR] internal/bookdrop/review_service.go:998 — DB records the pre-finalize filename; EEXIST-retry path leaves a dangling book_file reference The TX records `FileName: safeFileName` / `FileSubPath: fileSubPath` (review_service.go:955-957), computed by `uniquifyDestPath` BEFORE the TX (line 876). `finalizeWithRetry` runs AFTER the commit (line 998) and, on `EEXIST`, advances the on-disk path to a different suffix (e.g. DB says `novel.epub` but the file links to `novel (2).epub` — exactly the path the EEXIST-retry test asserts, review_service_test.go:2106-2107). Result in the concurrent-race path: the committed `book_file` row points at a path that does not exist, and the actual file is orphaned on disk (a rescan may re-import it as a duplicate). This is the security fix trading a clobber for a dangling reference — the winner's data is safe (so no security escalation), but it is a real data-integrity regression on the very race this PR fixes. Fix: capture the final linked path/name returned from `finalizeWithRetry` and reconcile the `book_file` row (update `file_name`/`file_sub_path`) after finalize, or move the link before the TX and record the actually-claimed name. The existing test masks this by stubbing `upsertBookFile` as a no-op — add an assertion that the stored `FileName` equals the finally-linked name in the EEXIST-retry case. [MINOR] internal/bookdrop/review_service.go:997 — `LinkFile` has no nil-default despite the doc claiming one The doc comment (review_service.go:272) says "Defaults to os.Link when nil," but `linkFn := p.LinkFile` is passed straight into `finalizeWithRetry` with no `if linkFn == nil { linkFn = os.Link }` guard (contrast `StatFile`/`MkdirAll`, which are defaulted). Production wiring passes `os.Link` so this is not exploitable, but a nil would panic and the code/doc disagree. Add the nil-default to match the contract. REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor commented 2026-07-26 21:59:29 +00:00

Author

Owner

Copy link

Security re-review — PR #1251 (concurrent-accept TOCTOU MAJOR)

Verified git diff origin/main...origin/bd-bookshelf-t582g.7.

The concurrent-accept clobbering MAJOR is CLOSED. Finalize now uses a non-clobbering os.Link (fails EEXIST when the dest name already exists — os.Link does not follow a symlink at the target and never write-through-clobbers) with a bounded re-uniquify retry (finalizeWithRetry, review_service.go:695-740). Two concurrent accepts of same-base-named proposals can no longer both land on the same path: the link is the atomic exclusive claim; the race loser gets EEXIST and advances to the next (N) suffix. Confirmed via internal/app/build_extended_deps.go:1806 wiring LinkFile: os.Link and the EEXIST-retry test (review_service_test.go:2051-2110).

Checklist against the four points:

1. Atomic/exclusive placement — YES. os.Link+EEXIST is the mutual-exclusion primitive; the winner's file is never overwritten.
2. No new symlink-follow / traversal — CLEAN. Temp is os.CreateTemp(destDir,...) inside the confined destDir; every link candidate is filepath.Join(destDir, base+"(N)"+ext) off a filepath.Base'd name with a numeric suffix. No separators, no .., no follow-through.
3. Temp perms — ACCEPTABLE. 0644 is applied only after content is copied+fsync+closed (stage.go:107-111); during the write window the file is 0600 in the library dir (not a world-writable tmp). 0644 read-only is the intended final library-file mode.
4. Residual TOCTOU — see MAJOR below (correctness, not a security escalation).

[MAJOR] internal/bookdrop/review_service.go:998 — DB records the pre-finalize filename; EEXIST-retry path leaves a dangling book_file reference
The TX records FileName: safeFileName / FileSubPath: fileSubPath (review_service.go:955-957), computed by uniquifyDestPath BEFORE the TX (line 876). finalizeWithRetry runs AFTER the commit (line 998) and, on EEXIST, advances the on-disk path to a different suffix (e.g. DB says novel.epub but the file links to novel (2).epub — exactly the path the EEXIST-retry test asserts, review_service_test.go:2106-2107). Result in the concurrent-race path: the committed book_file row points at a path that does not exist, and the actual file is orphaned on disk (a rescan may re-import it as a duplicate). This is the security fix trading a clobber for a dangling reference — the winner's data is safe (so no security escalation), but it is a real data-integrity regression on the very race this PR fixes. Fix: capture the final linked path/name returned from finalizeWithRetry and reconcile the book_file row (update file_name/file_sub_path) after finalize, or move the link before the TX and record the actually-claimed name. The existing test masks this by stubbing upsertBookFile as a no-op — add an assertion that the stored FileName equals the finally-linked name in the EEXIST-retry case.

[MINOR] internal/bookdrop/review_service.go:997 — LinkFile has no nil-default despite the doc claiming one
The doc comment (review_service.go:272) says "Defaults to os.Link when nil," but linkFn := p.LinkFile is passed straight into finalizeWithRetry with no if linkFn == nil { linkFn = os.Link } guard (contrast StatFile/MkdirAll, which are defaulted). Production wiring passes os.Link so this is not exploitable, but a nil would panic and the code/doc disagree. Add the nil-default to match the contract.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Security re-review — PR #1251 (concurrent-accept TOCTOU MAJOR) Verified `git diff origin/main...origin/bd-bookshelf-t582g.7`. **The concurrent-accept clobbering MAJOR is CLOSED.** Finalize now uses a non-clobbering `os.Link` (fails `EEXIST` when the dest name already exists — `os.Link` does not follow a symlink at the target and never write-through-clobbers) with a bounded re-uniquify retry (`finalizeWithRetry`, review_service.go:695-740). Two concurrent accepts of same-base-named proposals can no longer both land on the same path: the link is the atomic exclusive claim; the race loser gets `EEXIST` and advances to the next `(N)` suffix. Confirmed via `internal/app/build_extended_deps.go:1806` wiring `LinkFile: os.Link` and the EEXIST-retry test (review_service_test.go:2051-2110). Checklist against the four points: 1. Atomic/exclusive placement — YES. `os.Link`+EEXIST is the mutual-exclusion primitive; the winner's file is never overwritten. 2. No new symlink-follow / traversal — CLEAN. Temp is `os.CreateTemp(destDir,...)` inside the confined `destDir`; every link candidate is `filepath.Join(destDir, base+"(N)"+ext)` off a `filepath.Base`'d name with a numeric suffix. No separators, no `..`, no follow-through. 3. Temp perms — ACCEPTABLE. 0644 is applied only *after* content is copied+fsync+closed (stage.go:107-111); during the write window the file is 0600 in the library dir (not a world-writable tmp). 0644 read-only is the intended final library-file mode. 4. Residual TOCTOU — see MAJOR below (correctness, not a security escalation). [MAJOR] internal/bookdrop/review_service.go:998 — DB records the pre-finalize filename; EEXIST-retry path leaves a dangling book_file reference The TX records `FileName: safeFileName` / `FileSubPath: fileSubPath` (review_service.go:955-957), computed by `uniquifyDestPath` BEFORE the TX (line 876). `finalizeWithRetry` runs AFTER the commit (line 998) and, on `EEXIST`, advances the on-disk path to a different suffix (e.g. DB says `novel.epub` but the file links to `novel (2).epub` — exactly the path the EEXIST-retry test asserts, review_service_test.go:2106-2107). Result in the concurrent-race path: the committed `book_file` row points at a path that does not exist, and the actual file is orphaned on disk (a rescan may re-import it as a duplicate). This is the security fix trading a clobber for a dangling reference — the winner's data is safe (so no security escalation), but it is a real data-integrity regression on the very race this PR fixes. Fix: capture the final linked path/name returned from `finalizeWithRetry` and reconcile the `book_file` row (update `file_name`/`file_sub_path`) after finalize, or move the link before the TX and record the actually-claimed name. The existing test masks this by stubbing `upsertBookFile` as a no-op — add an assertion that the stored `FileName` equals the finally-linked name in the EEXIST-retry case. [MINOR] internal/bookdrop/review_service.go:997 — `LinkFile` has no nil-default despite the doc claiming one The doc comment (review_service.go:272) says "Defaults to os.Link when nil," but `linkFn := p.LinkFile` is passed straight into `finalizeWithRetry` with no `if linkFn == nil { linkFn = os.Link }` guard (contrast `StatFile`/`MkdirAll`, which are defaulted). Production wiring passes `os.Link` so this is not exploitable, but a nil would panic and the code/doc disagree. Add the nil-default to match the contract. REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor referenced this pull request 2026-07-26 21:59:50 +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 22:00:12 +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 from a commit 2026-07-26 22:12:17 +00:00

fix(bookdrop): claim-first ordering eliminates DB/disk name mismatch on EEXIST race

zombor added 1 commit 2026-07-26 22:12:17 +00:00

fix(bookdrop): claim-first ordering eliminates DB/disk name mismatch on EEXIST race ... a2cbb88cba

Round 2 review fixes (re-review of #1251):

1. [MAJOR] DB/disk consistency: rename finalizeWithRetry to claimDestPath with
   new signature returning (claimedPath, claimedName). The link now happens
   BEFORE the DB transaction so the TX records the name that actually exists
   on disk — not a pre-computed stat-probe hint that a concurrent accept may
   have claimed. On TX failure, claimedPath is unlinked (compensation) so no
   orphan remains. Tests assert upsertedBookFile.FileName equals the linked name.

2. [MAJOR] Nil guard: add if linkFn == nil { linkFn = os.Link } mirroring the
   statFn/mkdirAll pattern. A new real-FS test leaves LinkFile nil and verifies
   os.Link default is used, covering the guard branch.

3. [MINOR] stage.go chmod comment: explain 0600→0644 timing rationale so it
   is clear why CreateTemp starts private and why 0644 is needed before
   hard-linking into the shared library location.

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

zombor commented 2026-07-26 22:23:24 +00:00

Author

Owner

Copy link

[MINOR] internal/bookdrop/review_service.go:721 — claimDestPath loop bound off by 1
The loop for n := 1; n <= 1001; n++ allows 1001 total suffixes (hint + (2) through (1001)), but the comment on line 717 says "(2) … "(1000)"" which suggests only 999 additional attempts after the hint. The code actually allows one extra attempt (to 1001 instead of 1000). This is a nit: the practical collision limit is more than sufficient, and no correctness issue arises from the higher bound. Suggested fix: either change the loop to n <= 1000 to match the comment, or update the comment to say "(2) … (1001)" to match the code. This keeps the explicit bound and documentation in sync.

All critical safety properties verified:
✓ DB==disk invariant: the committed book_file row records the ACTUALLY-LINKED name (claimedName) from claimDestPath, not the pre-computed stat-probe hint. Test "non-clobbering finalize on TOCTOU race" confirms the DB row matches the file that was actually hard-linked, even when another accept raced and stole the hint path.
✓ Compensation completeness: on claim failure, temp is removed and source left intact; on TX failure AFTER claim succeeds, BOTH the claimed destination AND the temp are unlinked so no orphan file remains. Tests verify both paths.
✓ Source-intact guarantee: StageFile now copy+fsync only (never renames), so the source file survives any failure before commit. The original data-loss bug (rename fast-path destroying the source on temp cleanup) cannot reoccur.
✓ TOCTOU prevention: os.Link is atomic with EEXIST on collision; two concurrent accepts cannot both record different names for the same file.
✓ Status transitions: proposal stays PENDING_REVIEW on any failure (staging, claim, TX), making retries safe and idempotent.
✓ Nil-default guards: both LinkFile (line 899-901) and StatFile (line 871-873) default to os.Link and os.Stat respectively when nil.
✓ chmod applied: temp file chmod to 0o644 before hard-linking, error path tested.
✓ Coverage: 98.9% (uncovered 1.1% is wire.go, standard wiring exclusion).

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

[MINOR] internal/bookdrop/review_service.go:721 — claimDestPath loop bound off by 1 The loop `for n := 1; n <= 1001; n++` allows 1001 total suffixes (hint + (2) through (1001)), but the comment on line 717 says "(2) … \"(1000)\"" which suggests only 999 additional attempts after the hint. The code actually allows one extra attempt (to 1001 instead of 1000). This is a nit: the practical collision limit is more than sufficient, and no correctness issue arises from the higher bound. Suggested fix: either change the loop to `n <= 1000` to match the comment, or update the comment to say "(2) … (1001)" to match the code. This keeps the explicit bound and documentation in sync. All critical safety properties verified: ✓ DB==disk invariant: the committed book_file row records the ACTUALLY-LINKED name (claimedName) from claimDestPath, not the pre-computed stat-probe hint. Test "non-clobbering finalize on TOCTOU race" confirms the DB row matches the file that was actually hard-linked, even when another accept raced and stole the hint path. ✓ Compensation completeness: on claim failure, temp is removed and source left intact; on TX failure AFTER claim succeeds, BOTH the claimed destination AND the temp are unlinked so no orphan file remains. Tests verify both paths. ✓ Source-intact guarantee: StageFile now copy+fsync only (never renames), so the source file survives any failure before commit. The original data-loss bug (rename fast-path destroying the source on temp cleanup) cannot reoccur. ✓ TOCTOU prevention: os.Link is atomic with EEXIST on collision; two concurrent accepts cannot both record different names for the same file. ✓ Status transitions: proposal stays PENDING_REVIEW on any failure (staging, claim, TX), making retries safe and idempotent. ✓ Nil-default guards: both LinkFile (line 899-901) and StatFile (line 871-873) default to os.Link and os.Stat respectively when nil. ✓ chmod applied: temp file chmod to 0o644 before hard-linking, error path tested. ✓ Coverage: 98.9% (uncovered 1.1% is wire.go, standard wiring exclusion). REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor referenced this pull request from a commit 2026-07-27 02:28:34 +00:00

fix(bookdrop): claim-first ordering eliminates DB/disk name mismatch on EEXIST race

zombor force-pushed bd-bookshelf-t582g.7 from a2cbb88cba to 4ae9243bac 2026-07-27 02:28:34 +00:00 Compare

zombor referenced this pull request 2026-07-27 02:31:00 +00:00

refactor(consolidation): delete dead packages/handlers + unscoped magic footguns (bookshelf-9snmz.1.3) #1255

zombor referenced this pull request from a commit 2026-07-27 02:37:10 +00:00

fix(bookdrop): claim-first ordering eliminates DB/disk name mismatch on EEXIST race

zombor force-pushed bd-bookshelf-t582g.7 from 4ae9243bac to 5c7d2ea3cd 2026-07-27 02:37:10 +00:00 Compare

zombor merged commit e3fa9128b8 into main 2026-07-27 02:46:21 +00:00

zombor referenced this pull request from a commit 2026-07-27 02:46:26 +00:00

Merge pull request 'fix(bookdrop): prevent source data-loss on staging failure + dest-collision overwrite (bookshelf-t582g.7 + t582g.8)' (#1251) from bd-bookshelf-t582g.7 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!1251

No description provided.