feat(dedup): move files to target directory after merge (bookshelf-c15a.3) #606

Merged

zombor merged 3 commits from bd-bookshelf-c15a.3 into main 2026-06-18 13:28:08 +00:00

Conversation 3 Commits 3 Files changed 15 +996 -22

zombor commented 2026-06-18 02:45:05 +00:00

Owner

Copy link

Summary

• Adds a Move files to target directory checkbox to each duplicate group in the Find Duplicates modal
• When checked + Merge clicked, physically relocates source book files to the target book's library directory via os.Rename after the DB merge
• DISK_TYPE=NETWORK guard: pre-checked before DB merge returns HTTP 409 Conflict; checkbox disabled in UI on NETWORK disk
• Path containment guard: null bytes and source-outside-library-root paths are skipped and Warn-logged
• Best-effort: rename failure is Warn-logged, never blocks the merge (DB already committed)
• CSP-compliant: uses CSS classes, no inline style= attributes
• 100% coverage on internal/dedup
• go-rod e2e browser test: checkbox renders unchecked by default, toggle verified + screenshots

Test plan

• make test passes
• make coverage - zero uncovered statement blocks
• golangci-lint run ./internal/dedup/... - 0 issues
• Browser e2e: move-files checkbox renders + toggle works

Closes bead bookshelf-c15a.3 on merge.

## Summary - Adds a **Move files to target directory** checkbox to each duplicate group in the Find Duplicates modal - When checked + Merge clicked, physically relocates source book files to the target book's library directory via os.Rename after the DB merge - DISK_TYPE=NETWORK guard: pre-checked before DB merge returns HTTP 409 Conflict; checkbox disabled in UI on NETWORK disk - Path containment guard: null bytes and source-outside-library-root paths are skipped and Warn-logged - Best-effort: rename failure is Warn-logged, never blocks the merge (DB already committed) - CSP-compliant: uses CSS classes, no inline style= attributes - 100% coverage on internal/dedup - go-rod e2e browser test: checkbox renders unchecked by default, toggle verified + screenshots ## Test plan - [x] make test passes - [x] make coverage - zero uncovered statement blocks - [x] golangci-lint run ./internal/dedup/... - 0 issues - [x] Browser e2e: move-files checkbox renders + toggle works Closes bead bookshelf-c15a.3 on merge.

zombor added 1 commit 2026-06-18 02:45:06 +00:00

feat(dedup): move files to target directory after merge (bookshelf-c15a.3) ... a86e70662b

Adds a 'Move files to target directory' checkbox to each duplicate group
in the Find Duplicates modal. When checked, physically relocates source
book files to the target book's library directory via os.Rename after the
DB merge completes.

- DISK_TYPE=NETWORK guard: pre-checked before DB merge → HTTP 409
- Path containment guards: null bytes, source-outside-library-root
- Best-effort: rename failure is Warn-logged, never blocks the merge
- CSP-compliant: no inline style= attributes (CSS classes only)
- 100% coverage on internal/dedup
- Browser e2e test: checkbox renders + toggle verified

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

zombor added 1 commit 2026-06-18 02:59:50 +00:00

feat(dedup): move-files-to-target-dir after merge (bookshelf-c15a.3) ... 365d7bec31

- Add MoveFilesAfterMerge service: renames each source book file into
  the target book's library directory; best-effort (rename failure is
  logged at Warn, never returned as an error to the caller)
- DISK_TYPE=NETWORK guard returns ErrNetworkDisk → HTTP 409 before the
  DB merge runs, and after if somehow triggered post-merge
- Null byte + path traversal guards; dst-already-exists guard via os.Stat
- Add ListSourceBookFiles / GetTargetBookLibraryRoot store functions
- Wire moveFiles into MergeHandler and ScanHandler (NetworkDisk flag)
- Restore full multi-criteria scan (external_id, same_directory, filename)
  + KeepStrategy from main; re-sync with PR #600 e2e pyramid refactor
- Add move_files checkbox to dedup_results.html template; update
  find_duplicates_controller.js to include move_files in merge POST body
- Add journey browser e2e step: checkbox unchecked by default, toggles
- 100% internal/ coverage maintained

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

zombor force-pushed bd-bookshelf-c15a.3 from 365d7bec31 to cfcc1957b5 2026-06-18 03:14:51 +00:00 Compare

zombor reviewed 2026-06-18 04:02:06 +00:00

zombor left a comment

Copy link

No description provided.

## CODE REVIEW: APPROVED Reviewed diff for `bd-bookshelf-c15a.3` (Find Duplicates v3: Move files / Merge All / Dismiss / Delete). --- ### Phase 0: DEMO Verification No standalone DEMO block — the changes are UI/browser-tested via the e2e Ordered journey. The move-files checkbox e2e test (`e2e/browser/journey_duplicates_test.go:70`) is present and covers the interactive toggle. Accepted as the verification mechanism per the review prompt. --- ### Phase 1: Spec Compliance All four v3 features are present: - **Merge All (N)** — `mergeAll` in `find_duplicates_controller.js:443` iterates all `.dup-group` elements sequentially via `_mergeGroup`, shows a confirm dialog, and posts results in a summary toast. - **Per-group Dismiss** — `dismiss` method removes the group element from the DOM; button wired in `dedup_results.html`. - **Per-book checkbox + Delete** — `onCheckboxChange` / `deleteChecked` present (pre-existing). - **Move files to target directory** — `MoveFilesAfterMerge` service, guarded by DISK_TYPE=NETWORK (ErrNetworkDisk → 409), disabled checkbox in template when `NetworkDisk=true`. Rebase coherence: `internal/dedup/dedup.go` has both main's `ScanCriteria`/`PresetCriteria` and branch's `ErrNetworkDisk`/`MoveFiles`. `handler.go` has both main's `KeepStrategy` flow and branch's `diskType`/`NetworkDisk`/`moveFiles`. No dropped fields detected. Multi-user: scan is scoped to `userLibraryIDs`; merge calls `checkOwnership` for all book IDs before `moveFiles` is invoked; `moveFiles` SQL queries operate only on IDs already verified owned. CSP: no `style=`, `on*=`, or inline `<script>` in `dedup_results.html` diff. --- ### Phase 2: Code Quality **Path-traversal analysis (adversarial):** `moveOneFile` in `internal/dedup/move_files_service.go:491` guards source with `HasPrefix(srcAbs+sep, srcRoot+sep)`. The null-byte check fires before `filepath.Clean`. A traversal attempt via `FileSubPath="../../../etc/cron.d/evil"` produces `srcAbs="/etc/cron.d/evil"`, which fails the `HasPrefix` check and is logged+skipped before `renameFile` is ever called. The destination path uses the same `FileSubPath`, so destination traversal is implicitly blocked by the same source guard. The destination-exists guard (`os.Stat`) prevents overwrites. The NETWORK guard fires before any FS mutation. **Double-call design (handler.go:308–332):** The pre-check call `moveFiles(ctx, MergeRequest{MoveFiles:true})` passes empty `SourceBookIDs`, which short-circuits inside `MoveFilesAfterMerge` after the NETWORK check. On LOCAL disk this call is a cheap no-op (returns nil immediately). The post-merge call at line 326 is unconditional but `MoveFilesAfterMerge` short-circuits on `req.MoveFiles=false`. No wasted DB queries when the feature is off. **No findings requiring blocking:** --- [MINOR] `e2e/browser/journey_duplicates_test.go:70` — four `Expect` calls in one `It` block (checked-before, checked-after, plus two error checks). Project convention is one `Expect` per `It`. A toggle test is a reasonable exception for an Ordered browser journey where splitting into two `It` blocks would require duplicating the click interaction, but it's a convention deviation. [MINOR] `internal/dedup/move_files_service_test.go:231-234` — the "happy path" test asserts `renamedSrc == renamedDst == "/library/books/dune.epub"` (src == dst). The stub `renameFile` is called because `/library/books/dune.epub` doesn't exist on the test host (so `os.Stat` returns error → dst-exists guard skips). This is not a bug but the test is really exercising the "file not on disk" path, not a genuine cross-directory move. The intent is clear but a comment or a `GinkgoT().TempDir()`-backed test with distinct src/dst dirs would be more honest. (Pre-existing pattern acceptable for a unit stub test.) --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented 2026-06-18 04:02:42 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Reviewed diff for bd-bookshelf-c15a.3 (Find Duplicates v3: Move files / Merge All / Dismiss / Delete).

---

Phase 0: DEMO Verification

No standalone DEMO block — changes are verified via the e2e Ordered journey. The move-files checkbox e2e test (e2e/browser/journey_duplicates_test.go:70) covers the interactive toggle. Accepted as the verification mechanism per review scope.

---

Phase 1: Spec Compliance

All four v3 features present:

• Merge All (N) — mergeAll in find_duplicates_controller.js:443 iterates all .dup-group elements sequentially via _mergeGroup, shows a confirm dialog, posts a summary toast.
• Per-group Dismiss — dismiss method removes the group element; button wired in dedup_results.html.
• Per-book checkbox + Delete — onCheckboxChange/deleteChecked present (pre-existing).
• Move files to target directory — MoveFilesAfterMerge service, guarded by DISK_TYPE=NETWORK (ErrNetworkDisk → 409), checkbox disabled in template when NetworkDisk=true.

Rebase coherence: dedup.go has both main's ScanCriteria/PresetCriteria and branch's ErrNetworkDisk/MoveFiles. handler.go has both main's KeepStrategy flow and branch's diskType/NetworkDisk/moveFiles. No dropped fields.

Multi-user: scan scoped to userLibraryIDs; merge calls checkOwnership for all book IDs before moveFiles fires; move SQL queries operate only on pre-verified IDs.

CSP: no style=, on*=, or inline <script> in template diff.

---

Phase 2: Code Quality

Path-traversal (adversarial):

moveOneFile (move_files_service.go:491) guards source with HasPrefix(srcAbs+sep, srcRoot+sep). Null-byte check fires before filepath.Clean. A traversal via FileSubPath="../../../etc/cron.d/evil" produces srcAbs="/etc/cron.d/evil", failing the HasPrefix check — logged and skipped before renameFile is called. Destination uses the same FileSubPath, so destination traversal is implicitly blocked by the source guard. Destination-exists guard (os.Stat) prevents overwrites. NETWORK guard fires before any FS mutation. Path-traversal protection is sound.

Double-call design (handler.go:308–332): Pre-check call moveFiles(ctx, MergeRequest{MoveFiles:true}) passes empty SourceBookIDs, short-circuiting after the NETWORK check — cheap no-op on LOCAL. Post-merge call at line 326 is unconditional but MoveFilesAfterMerge short-circuits on req.MoveFiles=false. No wasted DB queries when feature is off.

---

[MINOR] e2e/browser/journey_duplicates_test.go:70 — four Expect calls in one It block (pre-click check, post-click check, plus two err checks). Project convention is one Expect per It. Reasonable exception for an Ordered browser toggle test where splitting would duplicate the click interaction, but is a documented deviation.

[MINOR] internal/dedup/move_files_service_test.go:231-234 — "happy path" test asserts renamedSrc == renamedDst == "/library/books/dune.epub" (src equals dst). The stub renameFile is called because /library doesn't exist on the test host (os.Stat returns error → dst-exists guard doesn't fire). This exercises the "file absent from disk" path rather than a genuine cross-directory move. Not a bug — the service logic is covered — but a comment clarifying the test intent would reduce confusion for future readers.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## CODE REVIEW: APPROVED Reviewed diff for `bd-bookshelf-c15a.3` (Find Duplicates v3: Move files / Merge All / Dismiss / Delete). --- ### Phase 0: DEMO Verification No standalone DEMO block — changes are verified via the e2e Ordered journey. The move-files checkbox e2e test (`e2e/browser/journey_duplicates_test.go:70`) covers the interactive toggle. Accepted as the verification mechanism per review scope. --- ### Phase 1: Spec Compliance All four v3 features present: - **Merge All (N)** — `mergeAll` in `find_duplicates_controller.js:443` iterates all `.dup-group` elements sequentially via `_mergeGroup`, shows a confirm dialog, posts a summary toast. - **Per-group Dismiss** — `dismiss` method removes the group element; button wired in `dedup_results.html`. - **Per-book checkbox + Delete** — `onCheckboxChange`/`deleteChecked` present (pre-existing). - **Move files to target directory** — `MoveFilesAfterMerge` service, guarded by DISK_TYPE=NETWORK (ErrNetworkDisk → 409), checkbox disabled in template when `NetworkDisk=true`. Rebase coherence: `dedup.go` has both main's `ScanCriteria`/`PresetCriteria` and branch's `ErrNetworkDisk`/`MoveFiles`. `handler.go` has both main's `KeepStrategy` flow and branch's `diskType`/`NetworkDisk`/`moveFiles`. No dropped fields. Multi-user: scan scoped to `userLibraryIDs`; merge calls `checkOwnership` for all book IDs before `moveFiles` fires; move SQL queries operate only on pre-verified IDs. CSP: no `style=`, `on*=`, or inline `<script>` in template diff. --- ### Phase 2: Code Quality **Path-traversal (adversarial):** `moveOneFile` (`move_files_service.go:491`) guards source with `HasPrefix(srcAbs+sep, srcRoot+sep)`. Null-byte check fires before `filepath.Clean`. A traversal via `FileSubPath="../../../etc/cron.d/evil"` produces `srcAbs="/etc/cron.d/evil"`, failing the `HasPrefix` check — logged and skipped before `renameFile` is called. Destination uses the same `FileSubPath`, so destination traversal is implicitly blocked by the source guard. Destination-exists guard (`os.Stat`) prevents overwrites. NETWORK guard fires before any FS mutation. **Path-traversal protection is sound.** **Double-call design (handler.go:308–332):** Pre-check call `moveFiles(ctx, MergeRequest{MoveFiles:true})` passes empty `SourceBookIDs`, short-circuiting after the NETWORK check — cheap no-op on LOCAL. Post-merge call at line 326 is unconditional but `MoveFilesAfterMerge` short-circuits on `req.MoveFiles=false`. No wasted DB queries when feature is off. --- [MINOR] `e2e/browser/journey_duplicates_test.go:70` — four `Expect` calls in one `It` block (pre-click check, post-click check, plus two `err` checks). Project convention is one `Expect` per `It`. Reasonable exception for an Ordered browser toggle test where splitting would duplicate the click interaction, but is a documented deviation. [MINOR] `internal/dedup/move_files_service_test.go:231-234` — "happy path" test asserts `renamedSrc == renamedDst == "/library/books/dune.epub"` (src equals dst). The stub `renameFile` is called because `/library` doesn't exist on the test host (`os.Stat` returns error → dst-exists guard doesn't fire). This exercises the "file absent from disk" path rather than a genuine cross-directory move. Not a bug — the service logic is covered — but a comment clarifying the test intent would reduce confusion for future readers. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented 2026-06-18 04:05:33 +00:00

Author

Owner

Copy link

Security Review — bookshelf-c15a.3 (Find Duplicates v3: move files after merge)

Scope reviewed

Path traversal / arbitrary file write, DISK_TYPE=NETWORK guard, ownership scoping, partial-data-loss consistency, SQL injection, secrets/PII in logs.

---

Findings

[MINOR] internal/dedup/move_files_service.go:118 — destination path lacks explicit containment guard
The source path is guarded: srcAbs is checked to start with srcRoot before the move proceeds. The destination dstAbs = filepath.Clean(filepath.Join(targetRoot, f.FileSubPath)) is not checked to start with targetRoot. This is currently not exploitable — the source containment check is sufficient: any FileSubPath that does not escape srcRoot also cannot escape any targetRoot. But the asymmetry is a missing defence-in-depth layer. A future refactor that relaxes the source check could silently open a traversal on the destination side. Adding strings.HasPrefix(dstAbs+string(filepath.Separator), filepath.Clean(targetRoot)+string(filepath.Separator)) after line 118 is cheap and makes the invariant explicit.

[MINOR] internal/dedup/move_files_service_test.go:231 — happy-path test uses identical src and dst
The default BeforeEach sets LibraryRoot = "/library" and getTargetRoot returns "/library", so renamedSrc == renamedDst. This does not exercise the cross-directory move (src library != target library). A variant with different roots would catch path-construction bugs the current test misses.

---

Items cleared (no finding)

• Path traversal (source): srcAbs guarded by strings.HasPrefix(srcAbs+sep, srcRoot+sep). Absolute FileSubPath values cause filepath.Join to discard the root; guard fires. Null-byte check present.
• Path traversal (destination): Not exploitable. If FileSubPath keeps srcAbs within srcRoot, the same relative components cannot escape any targetRoot. Absolute paths caught by source guard first.
• DISK_TYPE=NETWORK guard: Fires at top of closure before any FS or DB operation. Handler pre-flight (empty SourceBookIDs) triggers it before merge() touches the DB. Correct ordering confirmed.
• Ownership / cross-user access: merge() checks ownership of all book IDs against session-derived userLibraryIDs. moveFiles called post-merge with same in-process req — no ID substitution possible. GetTargetBookLibraryRoot queries only an already-owned book.
• SQL injection: ListSourceBookFiles builds IN (...) with ? placeholders and typed int64 args. No user-controlled string interpolation.
• Partial-data-loss consistency: DB merge in one atomic transaction; file moves are best-effort post-commit, logged at Warn. Documented design decision, not a security issue.
• PII / secrets in logs: Only filesystem paths logged; no tokens, passwords, or user data.
• No DELETE FS operations: Despite review brief mentioning per-book DELETE, this PR implements MOVE only (os.Rename). No os.Remove/os.RemoveAll in diff.
• Config validation: DiskType validated at startup to be LOCAL or NETWORK; service strings.ToUpper is redundant but harmless.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Security Review — bookshelf-c15a.3 (Find Duplicates v3: move files after merge) ### Scope reviewed Path traversal / arbitrary file write, DISK_TYPE=NETWORK guard, ownership scoping, partial-data-loss consistency, SQL injection, secrets/PII in logs. --- ### Findings **[MINOR] internal/dedup/move_files_service.go:118 — destination path lacks explicit containment guard** The source path is guarded: `srcAbs` is checked to start with `srcRoot` before the move proceeds. The destination `dstAbs = filepath.Clean(filepath.Join(targetRoot, f.FileSubPath))` is not checked to start with `targetRoot`. This is currently **not exploitable** — the source containment check is sufficient: any `FileSubPath` that does not escape `srcRoot` also cannot escape any `targetRoot`. But the asymmetry is a missing defence-in-depth layer. A future refactor that relaxes the source check could silently open a traversal on the destination side. Adding `strings.HasPrefix(dstAbs+string(filepath.Separator), filepath.Clean(targetRoot)+string(filepath.Separator))` after line 118 is cheap and makes the invariant explicit. **[MINOR] internal/dedup/move_files_service_test.go:231 — happy-path test uses identical src and dst** The default `BeforeEach` sets `LibraryRoot = "/library"` and `getTargetRoot` returns `"/library"`, so `renamedSrc == renamedDst`. This does not exercise the cross-directory move (src library != target library). A variant with different roots would catch path-construction bugs the current test misses. --- ### Items cleared (no finding) - **Path traversal (source):** `srcAbs` guarded by `strings.HasPrefix(srcAbs+sep, srcRoot+sep)`. Absolute `FileSubPath` values cause `filepath.Join` to discard the root; guard fires. Null-byte check present. - **Path traversal (destination):** Not exploitable. If `FileSubPath` keeps `srcAbs` within `srcRoot`, the same relative components cannot escape any `targetRoot`. Absolute paths caught by source guard first. - **DISK_TYPE=NETWORK guard:** Fires at top of closure before any FS or DB operation. Handler pre-flight (empty `SourceBookIDs`) triggers it before `merge()` touches the DB. Correct ordering confirmed. - **Ownership / cross-user access:** `merge()` checks ownership of all book IDs against session-derived `userLibraryIDs`. `moveFiles` called post-merge with same in-process `req` — no ID substitution possible. `GetTargetBookLibraryRoot` queries only an already-owned book. - **SQL injection:** `ListSourceBookFiles` builds `IN (...)` with `?` placeholders and typed `int64` args. No user-controlled string interpolation. - **Partial-data-loss consistency:** DB merge in one atomic transaction; file moves are best-effort post-commit, logged at Warn. Documented design decision, not a security issue. - **PII / secrets in logs:** Only filesystem paths logged; no tokens, passwords, or user data. - **No DELETE FS operations:** Despite review brief mentioning per-book DELETE, this PR implements MOVE only (`os.Rename`). No `os.Remove`/`os.RemoveAll` in diff. - **Config validation:** `DiskType` validated at startup to be `LOCAL` or `NETWORK`; service `strings.ToUpper` is redundant but harmless. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor referenced this pull request from a commit 2026-06-18 11:01:16 +00:00

test(dedup): genuine cross-directory move test for moveOneFile (bookshelf-c15a.3)

zombor added 1 commit 2026-06-18 11:01:16 +00:00

test(dedup): genuine cross-directory move test for moveOneFile (bookshelf-c15a.3) ... 53cfe0435a

Add two new test cases for moveOneFile that exercise genuine cross-directory
moves using real temp directories and the actual os.Rename function. The
existing "happy path" test was exercising only the "destination stat fails"
branch (both src and dst were in the same /library root, which doesn't exist
on test host), so it wasn't testing a real cross-directory move.

New tests:
- Assert the file genuinely moves from source to target directory
- Assert renamedSrc and renamedDst point to different library roots
- Use os.Stat to verify destination exists after the move
- Verify source file is deleted from source directory

Fixes the code-review minor from PR #606.

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

zombor referenced this pull request from a commit 2026-06-18 13:24:54 +00:00

test(dedup): genuine cross-directory move test for moveOneFile (bookshelf-c15a.3)

zombor force-pushed bd-bookshelf-c15a.3 from 53cfe0435a to 9d8dc1a085 2026-06-18 13:24:54 +00:00 Compare

zombor merged commit 6a8e3a30f1 into main 2026-06-18 13:28:08 +00:00

zombor referenced this pull request from a commit 2026-06-18 13:28:09 +00:00

Merge pull request 'feat(dedup): move files to target directory after merge (bookshelf-c15a.3)' (#606) from bd-bookshelf-c15a.3 into main

zombor referenced this pull request from a commit 2026-06-18 14:10:49 +00:00

test(dedup): genuine cross-directory move test for moveOneFile (bookshelf-c15a.3)

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

No description provided.