feat(bookdrop): Bulk Edit metadata across selected proposals (bookshelf-d1x3.2) #603

Merged

zombor merged 3 commits from bd-bookshelf-d1x3.2 into main 2026-06-18 13:05:12 +00:00

Conversation 3 Commits 3 Files changed 14 +1006 -51

zombor commented 2026-06-18 02:39:26 +00:00

Owner

Copy link

Summary

• Add a Bulk Edit toolbar button to the BookDrop review page, enabled only when ≥1 proposal is selected
• Clicking it opens a modal with all FileMetadataJSON fields (title, subtitle, authors, series, publisher, published date, language, ISBN-13/10, description)
• On Apply to Selected, the same patch is written to original_metadata for every selected proposal; the page reloads with a bulk_edit_applied flash
• All writes go to bookdrop_file.original_metadata — no new Grimmory columns
• CSP-safe: zero inline style= attributes; all layout via bbe-* CSS classes

Test plan

• Unit: BulkEditMetadata service — happy path, empty list, partial failure (11 new specs, 551 total)
• Unit: BulkEditMetadataHandler — JSON decoding, missing IDs → 400, partial failure counts
• Unit: validBulkEditAppliedFlash regex — valid format, XSS rejection, empty rejection
• Browser e2e: seeds 2 proposals, select-all, opens modal, fills publisher, applies, verifies DB + flash + posts screenshots to PR
• 100% coverage maintained

Closes bead bookshelf-d1x3.2 on merge.

## Summary - Add a **Bulk Edit** toolbar button to the BookDrop review page, enabled only when ≥1 proposal is selected - Clicking it opens a modal with all FileMetadataJSON fields (title, subtitle, authors, series, publisher, published date, language, ISBN-13/10, description) - On Apply to Selected, the same patch is written to original_metadata for every selected proposal; the page reloads with a bulk_edit_applied flash - All writes go to bookdrop_file.original_metadata — no new Grimmory columns - CSP-safe: zero inline style= attributes; all layout via bbe-* CSS classes ## Test plan - [x] Unit: BulkEditMetadata service — happy path, empty list, partial failure (11 new specs, 551 total) - [x] Unit: BulkEditMetadataHandler — JSON decoding, missing IDs → 400, partial failure counts - [x] Unit: validBulkEditAppliedFlash regex — valid format, XSS rejection, empty rejection - [x] Browser e2e: seeds 2 proposals, select-all, opens modal, fills publisher, applies, verifies DB + flash + posts screenshots to PR - [x] 100% coverage maintained Closes bead bookshelf-d1x3.2 on merge.

bulk_edit_modal_after.png

91 KiB

extract_pattern_modal_after.png

113 KiB

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

feat(bookdrop): Bulk Edit metadata across selected proposals (bookshelf-d1x3.2) ... 28624fc27b

Add a Bulk Edit mode to the BookDrop review page that lets users edit one or
more metadata fields (title, subtitle, author, series, publisher, etc.) across
all selected proposals at once before finalizing import.

- POST /bookdrop/bulk-edit: BulkEditMetadataHandler applies a fileMetadataJSON
  patch to every selected proposal's original_metadata serially, returning
  applied/failed counts
- BulkEditMetadata service function: curried composition over UpdateProposalMetadata
- bookdrop-bulk-edit Stimulus controller: CSP-safe modal (no inline style=),
  toolbar button enabled only when ≥1 row selected, author chip management,
  page reloads with ?bulk_edit_applied= flash on success
- Flash validation regex prevents XSS via URL param (mirrors extract-pattern)
- bbe-* CSS classes for the 2-column grid modal
- go-rod Ordered browser e2e: seeds 2 proposals, selects all, opens modal,
  fills publisher, applies, verifies DB + flash + uploads screenshots to PR
- 100% coverage maintained (+11 specs, 551 total)

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

zombor commented 2026-06-18 02:53:49 +00:00

Author

Owner

Copy link

Security Review — PR #603 (bookshelf-d1x3.2, BookDrop Bulk Edit)

Scope reviewed

• POST /bookdrop/bulk-edit ownership / user scoping
• Input validation and injection surface
• Stimulus controller XSS / eval / innerHTML usage
• CSRF handling
• Logging for PII/secrets

---

Finding 1 — Ownership / user-scoping

The new BulkEditMetadataHandler calls bulkEdit(r.Context(), ids, patch) where ids come from the request body and UpdateProposalMetadata performs only GetBookdropFile(ctx, id) — a bare WHERE id = ? with no user_id filter. The bookdrop_file table has no user_id column (it is a shared, server-global ingest queue in the Grimmory schema). This means any user with permission_access_bookdrop can overwrite the original_metadata of any bookdrop proposal regardless of which user created or owns it.

Assessment — MINOR context: The bookdrop_file table intentionally has no user_id column (Grimmory-compatible schema). The existing UpdateMetadataHandler (single-file) and BulkRejectHandler carry the same absence of per-user scoping — this is the pre-existing design, not a regression introduced by this PR. The BookdropRequired middleware correctly gates the entire route group to authenticated users with the bookdrop permission, so unauthenticated access is blocked. The risk is one privileged user clobbering another privileged user's proposal metadata, which is a low-severity multi-tenancy gap that pre-dates this PR and is consistent across all bookdrop mutation endpoints.

Conclusion: no new vulnerability introduced by this PR. Pre-existing parity with UpdateMetadataHandler and BulkRejectHandler.

---

Finding 2 — Input validation

• parseIDs validates comma-separated IDs to int64, enforces a 1000-ID cap, and rejects empty input. ✓
• http.MaxBytesReader(w, r.Body, 1<<16) (64 KB) caps the JSON body. ✓
• All metadata fields are typed primitives (string, *float64); they are json.Marshal-ed into original_metadata as data, not executed or interpreted. ✓
• No SQL injection surface — sqlc generates parameterised queries; fileMetadataJSON is marshalled to a JSON string and stored as a blob. ✓

---

Finding 3 — XSS / template injection

• BulkEditAppliedFlash is populated from ?bulk_edit_applied=<msg> query param via validBulkEditAppliedFlash() which enforces ^Bulk edit: \d+ updated, \d+ failed\.$ before accepting the value. ✓
• The template renders {{.BulkEditAppliedFlash}} (not {{.BulkEditAppliedFlash | noescape}}), so html/template auto-escaping applies. ✓
• The flash value is constructed client-side from data.applied and data.failed (integers from the JSON response) — no uncontrolled user string is ever in the URL. ✓

---

Finding 4 — Stimulus controller (XSS / CSP / eval)

• No innerHTML, eval, document.write, or insertAdjacentHTML with untrusted data. ✓
• Author chips are built with document.createElement + .textContent assignment — properly escaped. ✓
• No inline style= attributes on dynamic elements (CSP-safe). ✓
• No inline <script> blocks added. Controller loaded as <script defer src="...">. ✓
• CSRF token is read from the bookshelf_csrf cookie and sent in X-CSRF-Token header. ✓

---

Finding 5 — Logging

• Logger records ids_count, applied, failed — no field values, no PII, no proposal content. ✓
• res.Err.Error() is surfaced in the JSON response; these errors are internal (e.g. "update proposal metadata 42: not found") — no secrets or user data. ✓

---

Summary

No new security vulnerabilities introduced by this PR. All identified characteristics (lack of user_id scoping on bookdrop_file) are pre-existing, intentional Grimmory-schema limitations shared by every bookdrop mutation handler.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #603 (bookshelf-d1x3.2, BookDrop Bulk Edit) ### Scope reviewed - `POST /bookdrop/bulk-edit` ownership / user scoping - Input validation and injection surface - Stimulus controller XSS / eval / innerHTML usage - CSRF handling - Logging for PII/secrets --- ### Finding 1 — Ownership / user-scoping The new `BulkEditMetadataHandler` calls `bulkEdit(r.Context(), ids, patch)` where `ids` come from the request body and `UpdateProposalMetadata` performs only `GetBookdropFile(ctx, id)` — a bare `WHERE id = ?` with no `user_id` filter. The `bookdrop_file` table has no `user_id` column (it is a shared, server-global ingest queue in the Grimmory schema). This means any user with `permission_access_bookdrop` can overwrite the `original_metadata` of any bookdrop proposal regardless of which user created or owns it. **Assessment — MINOR context:** The `bookdrop_file` table intentionally has no `user_id` column (Grimmory-compatible schema). The existing `UpdateMetadataHandler` (single-file) and `BulkRejectHandler` carry the same absence of per-user scoping — this is the pre-existing design, not a regression introduced by this PR. The `BookdropRequired` middleware correctly gates the entire route group to authenticated users with the bookdrop permission, so unauthenticated access is blocked. The risk is one privileged user clobbering another privileged user's proposal metadata, which is a low-severity multi-tenancy gap that pre-dates this PR and is consistent across all bookdrop mutation endpoints. **Conclusion:** no new vulnerability introduced by this PR. Pre-existing parity with `UpdateMetadataHandler` and `BulkRejectHandler`. --- ### Finding 2 — Input validation - `parseIDs` validates comma-separated IDs to `int64`, enforces a 1000-ID cap, and rejects empty input. ✓ - `http.MaxBytesReader(w, r.Body, 1<<16)` (64 KB) caps the JSON body. ✓ - All metadata fields are typed primitives (`string`, `*float64`); they are `json.Marshal`-ed into `original_metadata` as data, not executed or interpreted. ✓ - No SQL injection surface — sqlc generates parameterised queries; `fileMetadataJSON` is marshalled to a JSON string and stored as a blob. ✓ --- ### Finding 3 — XSS / template injection - `BulkEditAppliedFlash` is populated from `?bulk_edit_applied=<msg>` query param via `validBulkEditAppliedFlash()` which enforces `^Bulk edit: \d+ updated, \d+ failed\.$` before accepting the value. ✓ - The template renders `{{.BulkEditAppliedFlash}}` (not `{{.BulkEditAppliedFlash | noescape}}`), so `html/template` auto-escaping applies. ✓ - The flash value is constructed client-side from `data.applied` and `data.failed` (integers from the JSON response) — no uncontrolled user string is ever in the URL. ✓ --- ### Finding 4 — Stimulus controller (XSS / CSP / eval) - No `innerHTML`, `eval`, `document.write`, or `insertAdjacentHTML` with untrusted data. ✓ - Author chips are built with `document.createElement` + `.textContent` assignment — properly escaped. ✓ - No inline `style=` attributes on dynamic elements (CSP-safe). ✓ - No inline `<script>` blocks added. Controller loaded as `<script defer src="...">`. ✓ - CSRF token is read from the `bookshelf_csrf` cookie and sent in `X-CSRF-Token` header. ✓ --- ### Finding 5 — Logging - Logger records `ids_count`, `applied`, `failed` — no field values, no PII, no proposal content. ✓ - `res.Err.Error()` is surfaced in the JSON response; these errors are internal (e.g. `"update proposal metadata 42: not found"`) — no secrets or user data. ✓ --- ### Summary No new security vulnerabilities introduced by this PR. All identified characteristics (lack of `user_id` scoping on `bookdrop_file`) are pre-existing, intentional Grimmory-schema limitations shared by every bookdrop mutation handler. ``` REVIEW VERDICT: 0 blocker, 0 major, 0 minor ```

zombor commented 2026-06-18 02:54:31 +00:00

Author

Owner

Copy link

CODE REVIEW — bookshelf-d1x3.2 (PR #603)

VERDICT: 0 blockers, 0 majors, 3 minors

---

Phase 0: DEMO Verification

CI note accepted: 5/6 jobs green; the failing Test job is the confirmed pre-existing BulkLLMSweepWorkflow/lt2p flake — this PR touches no wfengine code. No DEMO block required under the pre-existing-flake carve-out.

---

Phase 1: Spec Compliance — PASS

All five review items verified:

1. Metadata patch: BulkEditMetadata in review_service.go calls UpdateProposalMetadata (which overwrites bookdrop_file.original_metadata with the canonical fileMetadataJSON shape) for each ID. Fields in the patch overwrite; no new Grimmory columns added. Unselected files are untouched.
2. CSP: Controller loaded via base.html <script defer> tag (self-registers on window.BookdropBulkEditController, registered in app.js). No inline <script>, no onclick/on*= handlers, no style= attributes in the template. The fetch call sends X-CSRF-Token from the cookie. Clean.
3. Multi-user: bookdrop_file has no user_id column — it is not per-user scoped at the schema level, which is the existing precedent (same as UpdateMetadataHandler, BulkRejectHandler). The route is gated behind BookdropRequired which enforces ClaimsFromContext and permission check. This is consistent with the existing pattern, not a regression introduced by this PR.
4. Browser e2e: Ordered journey; drives real browser interaction (select-all → enable button → open modal → fill publisher → apply → wait for redirect + flash → DB assertion). Assertions are real.
5. Input validation, curried-func, ginkgo: parseIDs validates the id list; http.MaxBytesReader(w, r.Body, 1<<16) caps the body. Curried-function pattern followed throughout. Ginkgo structure is correct.

---

Phase 2: Code Quality

[MINOR] internal/bookdrop/review_handler_test.go:1319 — Two Expect calls in one It block ("passes the title to the bulk-edit func"): Expect(captured).To(HaveLen(3)) then Expect(captured[0].Title).To(Equal("Bulk Title")). Per project convention (one assertion per It), split into two It blocks.

[MINOR] internal/bookdrop/review_service_test.go:3503 — It("records the correct ids in results") contains three Expect calls (results[0].ID, [1].ID, [2].ID). Split into three It blocks.

[MINOR] internal/bookdrop/review_service_test.go:3524 — It("continues updating the remaining ids") contains two Expect calls (results[0].Err, results[2].Err). Split into two It blocks.

[MINOR] internal/bookdrop/review_handler.go:556 — logger.Info in BulkEditMetadataHandler omits trace_id. All other handlers in this file include "trace_id", middleware.TraceIDFromContext(r.Context()) in their log call (see line 467). Suggestion: add it for consistency with the logging standard.

---

REVIEW VERDICT: 0 blocker, 0 major, 4 minor

## CODE REVIEW — bookshelf-d1x3.2 (PR #603) **VERDICT: 0 blockers, 0 majors, 3 minors** --- ### Phase 0: DEMO Verification CI note accepted: 5/6 jobs green; the failing Test job is the confirmed pre-existing BulkLLMSweepWorkflow/lt2p flake — this PR touches no wfengine code. No DEMO block required under the pre-existing-flake carve-out. --- ### Phase 1: Spec Compliance — PASS All five review items verified: 1. **Metadata patch**: `BulkEditMetadata` in `review_service.go` calls `UpdateProposalMetadata` (which overwrites `bookdrop_file.original_metadata` with the canonical `fileMetadataJSON` shape) for each ID. Fields in the patch overwrite; no new Grimmory columns added. Unselected files are untouched. 2. **CSP**: Controller loaded via `base.html` `<script defer>` tag (self-registers on `window.BookdropBulkEditController`, registered in `app.js`). No inline `<script>`, no `onclick`/`on*=` handlers, no `style=` attributes in the template. The fetch call sends `X-CSRF-Token` from the cookie. Clean. 3. **Multi-user**: `bookdrop_file` has no `user_id` column — it is not per-user scoped at the schema level, which is the existing precedent (same as `UpdateMetadataHandler`, `BulkRejectHandler`). The route is gated behind `BookdropRequired` which enforces `ClaimsFromContext` and permission check. This is consistent with the existing pattern, not a regression introduced by this PR. 4. **Browser e2e**: `Ordered` journey; drives real browser interaction (select-all → enable button → open modal → fill publisher → apply → wait for redirect + flash → DB assertion). Assertions are real. 5. **Input validation, curried-func, ginkgo**: `parseIDs` validates the id list; `http.MaxBytesReader(w, r.Body, 1<<16)` caps the body. Curried-function pattern followed throughout. Ginkgo structure is correct. --- ### Phase 2: Code Quality [MINOR] `internal/bookdrop/review_handler_test.go:1319` — Two `Expect` calls in one `It` block ("passes the title to the bulk-edit func"): `Expect(captured).To(HaveLen(3))` then `Expect(captured[0].Title).To(Equal("Bulk Title"))`. Per project convention (one assertion per `It`), split into two `It` blocks. [MINOR] `internal/bookdrop/review_service_test.go:3503` — `It("records the correct ids in results")` contains three `Expect` calls (results[0].ID, [1].ID, [2].ID). Split into three `It` blocks. [MINOR] `internal/bookdrop/review_service_test.go:3524` — `It("continues updating the remaining ids")` contains two `Expect` calls (results[0].Err, results[2].Err). Split into two `It` blocks. [MINOR] `internal/bookdrop/review_handler.go:556` — `logger.Info` in `BulkEditMetadataHandler` omits `trace_id`. All other handlers in this file include `"trace_id", middleware.TraceIDFromContext(r.Context())` in their log call (see line 467). Suggestion: add it for consistency with the logging standard. --- REVIEW VERDICT: 0 blocker, 0 major, 4 minor

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

fix(bookdrop): address review minors — one-Expect-per-It splits + trace_id in BulkEdit log ... 8924fc1a8e

- Split 2-Expect It in review_handler_test (HaveLen + title) into two Its
- Split 3-Expect It in review_service_test (ids[0/1/2]) into three Its
- Split 2-Expect It in review_service_test (remaining errors) into two Its
- Add "trace_id" attr to BulkEditMetadataHandler logger.Info (matches other handlers)

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

zombor added 1 commit 2026-06-18 12:30:58 +00:00

fix(bookdrop): restyle bulk-edit + extract-pattern modals to canonical form/modal components (bookshelf-d1x3.2) ... 7dc84d8c67

Replace bespoke .bbe-* and .bep-* chrome (header/footer/close btn/labels/inputs)
with canonical .sa-modal-header/.sa-modal-title/.sa-modal-close-btn/.sa-modal-footer
and .metadata-field/.metadata-field-label/.metadata-field-input. Delete the 130
dead CSS lines. Keep .bbe-form grid for two-column layout, .bep-* for the
dynamically-generated preview area HTML (controller JS emits those classes), and
bep-subhead/section/chips for extract-pattern content structure.

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

zombor commented 2026-06-18 12:31:31 +00:00

Author

Owner

Copy link

Visual self-check: AFTER this fix

Bulk Edit Metadata modal (canonical .sa-modal-header/.sa-modal-footer + .metadata-field/.metadata-field-label/.metadata-field-input + .bbe-form 2-col grid):

Extract Metadata from Filenames modal (same canonical chrome; .bep-* kept only for the dynamically-generated preview HTML the controller emits):

Both modals now have:

• ✓ Comfortable header padding (no cramped title)
• ✓ Canonical muted labels (metadata-field-label, color: var(--fg-muted))
• ✓ Canonical input styling (metadata-field-input)
• ✓ Canonical header/close/footer (.sa-modal-header/.sa-modal-close-btn/.sa-modal-footer)
• ✓ No dead gap above footer
• ✓ 130 bespoke CSS lines deleted

## Visual self-check: AFTER this fix **Bulk Edit Metadata modal** (canonical `.sa-modal-header`/`.sa-modal-footer` + `.metadata-field`/`.metadata-field-label`/`.metadata-field-input` + `.bbe-form` 2-col grid):  **Extract Metadata from Filenames modal** (same canonical chrome; `.bep-*` kept only for the dynamically-generated preview HTML the controller emits):  Both modals now have: - ✓ Comfortable header padding (no cramped title) - ✓ Canonical muted labels (`metadata-field-label`, `color: var(--fg-muted)`) - ✓ Canonical input styling (`metadata-field-input`) - ✓ Canonical header/close/footer (`.sa-modal-header`/`.sa-modal-close-btn`/`.sa-modal-footer`) - ✓ No dead gap above footer - ✓ 130 bespoke CSS lines deleted

zombor force-pushed bd-bookshelf-d1x3.2 from 7dc84d8c67 to 39d16af804 2026-06-18 13:01:44 +00:00 Compare

zombor merged commit 7618750a46 into main 2026-06-18 13:05:12 +00:00

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

Merge pull request 'feat(bookdrop): Bulk Edit metadata across selected proposals (bookshelf-d1x3.2)' (#603) from bd-bookshelf-d1x3.2 into main

zombor referenced this pull request from a commit 2026-06-19 00:22:35 +00:00

feat(reviews): ui-reviewer agent + visual gate for template/CSS PRs (bookshelf-g3y8)

zombor referenced this pull request 2026-06-19 00:22:45 +00:00

feat(reviews): ui-reviewer agent + visual gate for UI PRs (bookshelf-g3y8) #624

zombor referenced this pull request from a commit 2026-06-19 00:32:57 +00:00

feat(reviews): ui-reviewer agent + visual gate for template/CSS PRs (bookshelf-g3y8)

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

No description provided.