zombor/pergamum
1
0
Fork 0
Code Issues Pull requests 5 Projects Releases Packages 2 Wiki Activity Actions 1

Library + LibraryPath CRUD API + UI (bookshelf-axt.1) #18

Merged
zombor merged 1 commit from bd-bookshelf-axt.1 into main 2026-05-21 01:56:08 +00:00
Conversation 8 Commits 1 Files changed 26 +4203 -135
zombor commented 2026-05-20 21:10:14 +00:00
Owner
Copy link

Summary

  • Implements full Library + LibraryPath CRUD: sqlc queries, functional-args service layer, HTTP handlers with JSON/HTML content negotiation, three HTML templates, two Stimulus controllers
  • 100% test coverage on internal/library/ (unit) and internal/app/ (integration with real MySQL)
  • Routes: GET/POST /libraries, GET/PUT/PATCH/DELETE /libraries/{id}, POST /libraries/{id}/paths, DELETE /libraries/{id}/paths/{pathId}, plus GET /libraries/new and GET /libraries/{id}/edit

Test plan

  • make test passes locally (100% coverage gate)
  • internal/library unit tests: service (validation, mapping, error wrapping), handler (all routes, JSON + HTML, error paths)
  • internal/app integration tests: full CRUD exercising all wiring closures against real MySQL
  • E2E tests in e2e/api/libraries_test.go (e2e build tag)
  • CI green on Forgejo

Closes bead bookshelf-axt.1 on merge.

🤖 Generated with Claude Code

## Summary - Implements full Library + LibraryPath CRUD: sqlc queries, functional-args service layer, HTTP handlers with JSON/HTML content negotiation, three HTML templates, two Stimulus controllers - 100% test coverage on `internal/library/` (unit) and `internal/app/` (integration with real MySQL) - Routes: GET/POST /libraries, GET/PUT/PATCH/DELETE /libraries/{id}, POST /libraries/{id}/paths, DELETE /libraries/{id}/paths/{pathId}, plus GET /libraries/new and GET /libraries/{id}/edit ## Test plan - [x] `make test` passes locally (100% coverage gate) - [x] `internal/library` unit tests: service (validation, mapping, error wrapping), handler (all routes, JSON + HTML, error paths) - [x] `internal/app` integration tests: full CRUD exercising all wiring closures against real MySQL - [x] E2E tests in `e2e/api/libraries_test.go` (e2e build tag) - [ ] CI green on Forgejo Closes bead bookshelf-axt.1 on merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
feat(bookshelf-axt.1): Library + LibraryPath CRUD (API + UI)
All checks were successful
/ Lint (pull_request) Successful in 3m36s
Details
/ E2E (pull_request) Successful in 4m27s
Details
/ Test (pull_request) Successful in 4m38s
Details
/ Coverage (pull_request) Successful in 1m15s
Details
779ee38ea6 
Implements the full library management feature from the Grimmory baseline
schema: sqlc queries, domain service with functional-args style, HTTP
handlers with content negotiation (JSON + HTML), Stimulus controllers for
confirm-delete and form validation hints, three page templates, and
100% coverage across internal/library and internal/app.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
zombor commented 2026-05-20 23:02:50 +00:00
Author
Owner
Copy link

Security Review (automated)

Reviewed commit 779ee38 — Library + LibraryPath CRUD API + UI.

Threat assessment

# Threat Status Notes
1 SQL injection OK All queries go through sqlc with ? placeholders. No hand-written string-interpolated SQL found anywhere in the diff.
2 Path traversal WARN library_path.path is stored verbatim with zero validation — no length cap, no canonicalization, no rejection of .. sequences or absolute paths that escape a configured root. The scanner (AXT-2) hasn't landed yet so there's no exploit path today, but the input gate is open. See finding #1 below.
3 XSS WARN No template.HTML/template.JS raw casts — html/template auto-escaping is intact for all {{.}} expressions. However, data-confirm-message="Delete library {{.Name}}?" puts user-controlled text into an HTML attribute without the " inside the value being escapable by Go's template engine in that context. See finding #2.
4 Authorization NA No auth in MVP by design. No routes assume auth-gating exists. TODO-AUTH callsites are absent — recommend adding a comment at RegisterRoutes so the auth bead knows where to insert middleware.
5 Input validation WARN name has no length cap in Go-side validate(). DB column is varchar(255) so oversized input will produce a DB error, but the error is mapped to a generic 500. allowed_formats correctly rejects whitespace and non-lowercase. organization_mode uses a closed enum map — correct. path (AddPath) has no validation at all. See finding #1.
6 CSRF NA No auth means there are no cross-origin-forgeable credentials today. Gap noted: when auth lands the form submissions (POST /libraries, POST /libraries/{id}/paths, DELETE routes) will need CSRF tokens. The _method override pattern makes this more important — any origin can POST with _method=DELETE. Document for bookshelf-4op.
7 Logging hygiene OK Logs emit only library_id and path_id (integers). No PII, no tokens, no full request bodies.
8 Error message leakage OK writeError in error_mapper.go returns only http.StatusText(status) — no internal paths, DB error text, or stack traces reach the client.
9 Resource exhaustion WARN ListLibraries has LIMIT 200 (hard-coded defaultListLimit). ListLibraryPaths inside Get has no LIMIT — SELECT id, path, library_id FROM library_path WHERE library_id = ? returns all paths for a library unconditionally. In practice libraries have few paths but it violates the project's "no unbounded queries" rule. See finding #3.
10 HTTP body size WARN No http.MaxBytesReader wrapping on any handler. ReadTimeout: 5s on the server provides an indirect cap, but a slow client sending a multi-megabyte JSON body will hold the goroutine for the full 5 seconds before a timeout fires. See finding #4.

Findings

WARN internal/library/service.go:AddPath (line 169) — path is stored with no validation. No length check, no rejection of .., no normalization. When AXT-2 lands and the scanner walks these paths, a crafted value like ../../../../etc or an overly long string could cause filesystem access outside the intended root or a DB truncation error surfaced as an opaque 500.

WARN templates/pages/libraries_index.html:30, templates/pages/libraries_show.html:8,39 — data-confirm-message="Delete library {{.Name}}?" and data-confirm-message="Remove path {{.Path}}?". Go's html/template correctly HTML-escapes " inside attribute values (it becomes &#34;) so attribute injection is blocked. The JS then reads this.element.dataset.confirmMessage which is the decoded value, so the confirm dialog will show the raw library name. This is fine for display. However, if a library name contains a single quote (e.g. O'Reilly) the dialog shows it correctly because window.confirm() takes a string, not HTML. No actual XSS here — downgrading this to a note rather than a blocker.

WARN internal/db/queries/library.sql:25-27 — ListLibraryPaths has no LIMIT. Query: SELECT id, path, library_id FROM library_path WHERE library_id = ?. Violates the project rule "no unbounded queries over user-growable tables."

WARN internal/library/handler.go / internal/app/app.go — No http.MaxBytesReader on request bodies for create/update/addPath handlers. A 5-second ReadTimeout provides a loose safety net but does not bound body size directly.

NOTE internal/library/handler.go:deletePathHandler (lines 288-319) — libraryID is parsed from the URL but the DeletePath service call uses only pathID. There is no check that the path actually belongs to the given library. Today this just means a client can delete any path by its numeric ID regardless of the {id} in the URL. With no auth this is not exploitable beyond a UI inconsistency, but it will matter once auth scopes libraries to users (TODO-AUTH).

NOTE internal/library/service.go:197-214 (validate) — name has no len(name) > 255 guard. DB column is varchar(255). The error path on overlong insert is an unhandled driver error → generic 500 with no user-friendly message.

Verdict

⚠️ Notes (won't block merge) — no blockers found, but three items should become follow-up beads before AXT-2 (scanner) lands:

  1. Path validation at AddPath boundary — at minimum: len(path) > 4096 rejection, block empty string, block paths containing \x00. Full canonicalization can wait until scan bead but the null-byte guard should land with this PR or immediately after.
  2. Name length guard — add len(strings.TrimSpace(name)) > 255 → ErrValidation to produce a 400 instead of a 500.
  3. LIMIT on ListLibraryPaths — add LIMIT 1000 (or a configurable cap) to the SQL query.
  4. TODO-AUTH comment at RegisterRoutes — single line so the auth bead author doesn't miss the injection point.

Items 2–4 are safe to ship as-is. Item 1 (path null-byte) is low risk now but should be a fast follow-up bead before the scanner lands.

## Security Review (automated) Reviewed commit `779ee38` — Library + LibraryPath CRUD API + UI. ### Threat assessment | # | Threat | Status | Notes | |---|--------|--------|-------| | 1 | SQL injection | **OK** | All queries go through sqlc with `?` placeholders. No hand-written string-interpolated SQL found anywhere in the diff. | | 2 | Path traversal | **WARN** | `library_path.path` is stored verbatim with zero validation — no length cap, no canonicalization, no rejection of `..` sequences or absolute paths that escape a configured root. The scanner (AXT-2) hasn't landed yet so there's no exploit path today, but the input gate is open. See finding #1 below. | | 3 | XSS | **WARN** | No `template.HTML`/`template.JS` raw casts — `html/template` auto-escaping is intact for all `{{.}}` expressions. However, `data-confirm-message="Delete library {{.Name}}?"` puts user-controlled text into an HTML attribute without the `"` inside the value being escapable by Go's template engine in that context. See finding #2. | | 4 | Authorization | **NA** | No auth in MVP by design. No routes assume auth-gating exists. `TODO-AUTH` callsites are absent — recommend adding a comment at `RegisterRoutes` so the auth bead knows where to insert middleware. | | 5 | Input validation | **WARN** | `name` has no length cap in Go-side `validate()`. DB column is `varchar(255)` so oversized input will produce a DB error, but the error is mapped to a generic 500. `allowed_formats` correctly rejects whitespace and non-lowercase. `organization_mode` uses a closed enum map — correct. `path` (AddPath) has **no validation at all**. See finding #1. | | 6 | CSRF | **NA** | No auth means there are no cross-origin-forgeable credentials today. Gap noted: when auth lands the form submissions (POST /libraries, POST /libraries/{id}/paths, DELETE routes) will need CSRF tokens. The `_method` override pattern makes this more important — any origin can POST with `_method=DELETE`. Document for bookshelf-4op. | | 7 | Logging hygiene | **OK** | Logs emit only `library_id` and `path_id` (integers). No PII, no tokens, no full request bodies. | | 8 | Error message leakage | **OK** | `writeError` in `error_mapper.go` returns only `http.StatusText(status)` — no internal paths, DB error text, or stack traces reach the client. | | 9 | Resource exhaustion | **WARN** | `ListLibraries` has `LIMIT 200` (hard-coded `defaultListLimit`). `ListLibraryPaths` inside `Get` has **no LIMIT** — `SELECT id, path, library_id FROM library_path WHERE library_id = ?` returns all paths for a library unconditionally. In practice libraries have few paths but it violates the project's "no unbounded queries" rule. See finding #3. | | 10 | HTTP body size | **WARN** | No `http.MaxBytesReader` wrapping on any handler. `ReadTimeout: 5s` on the server provides an indirect cap, but a slow client sending a multi-megabyte JSON body will hold the goroutine for the full 5 seconds before a timeout fires. See finding #4. | --- ### Findings **WARN** `internal/library/service.go:AddPath` (line 169) — `path` is stored with no validation. No length check, no rejection of `..`, no normalization. When AXT-2 lands and the scanner walks these paths, a crafted value like `../../../../etc` or an overly long string could cause filesystem access outside the intended root or a DB truncation error surfaced as an opaque 500. **WARN** `templates/pages/libraries_index.html:30`, `templates/pages/libraries_show.html:8,39` — `data-confirm-message="Delete library {{.Name}}?"` and `data-confirm-message="Remove path {{.Path}}?"`. Go's `html/template` correctly HTML-escapes `"` inside attribute values (it becomes `&#34;`) so attribute injection is blocked. The JS then reads `this.element.dataset.confirmMessage` which is the decoded value, so the confirm dialog will show the raw library name. This is fine for display. However, if a library name contains a single quote (e.g. `O'Reilly`) the dialog shows it correctly because `window.confirm()` takes a string, not HTML. **No actual XSS here** — downgrading this to a note rather than a blocker. **WARN** `internal/db/queries/library.sql:25-27` — `ListLibraryPaths` has no `LIMIT`. Query: `SELECT id, path, library_id FROM library_path WHERE library_id = ?`. Violates the project rule "no unbounded queries over user-growable tables." **WARN** `internal/library/handler.go` / `internal/app/app.go` — No `http.MaxBytesReader` on request bodies for create/update/addPath handlers. A 5-second `ReadTimeout` provides a loose safety net but does not bound body size directly. **NOTE** `internal/library/handler.go:deletePathHandler` (lines 288-319) — `libraryID` is parsed from the URL but the `DeletePath` service call uses only `pathID`. There is no check that the path actually belongs to the given library. Today this just means a client can delete any path by its numeric ID regardless of the `{id}` in the URL. With no auth this is not exploitable beyond a UI inconsistency, but it will matter once auth scopes libraries to users (TODO-AUTH). **NOTE** `internal/library/service.go:197-214` (`validate`) — `name` has no `len(name) > 255` guard. DB column is `varchar(255)`. The error path on overlong insert is an unhandled driver error → generic 500 with no user-friendly message. --- ### Verdict ⚠️ Notes (won't block merge) — no blockers found, but three items should become follow-up beads before AXT-2 (scanner) lands: 1. **Path validation at AddPath boundary** — at minimum: `len(path) > 4096` rejection, block empty string, block paths containing `\x00`. Full canonicalization can wait until scan bead but the null-byte guard should land with this PR or immediately after. 2. **Name length guard** — add `len(strings.TrimSpace(name)) > 255` → ErrValidation to produce a 400 instead of a 500. 3. **LIMIT on ListLibraryPaths** — add `LIMIT 1000` (or a configurable cap) to the SQL query. 4. **TODO-AUTH comment at RegisterRoutes** — single line so the auth bead author doesn't miss the injection point. Items 2–4 are safe to ship as-is. Item 1 (path null-byte) is low risk now but should be a fast follow-up bead before the scanner lands.
zombor commented 2026-05-20 23:02:55 +00:00
Author
Owner
Copy link

Security Review (automated)

Reviewed commit 779ee38 — Library + LibraryPath CRUD API + UI.

Threat assessment

# Threat Status Notes
1 SQL injection OK All queries go through sqlc with ? placeholders. No hand-written string-interpolated SQL found anywhere in the diff.
2 Path traversal WARN library_path.path is stored verbatim with zero validation — no length cap, no canonicalization, no rejection of .. sequences or absolute paths that escape a configured root. The scanner (AXT-2) hasn't landed yet so there's no exploit path today, but the input gate is open. See finding #1 below.
3 XSS WARN No template.HTML/template.JS raw casts — html/template auto-escaping is intact for all {{.}} expressions. However, data-confirm-message="Delete library {{.Name}}?" puts user-controlled text into an HTML attribute without the " inside the value being escapable by Go's template engine in that context. See finding #2.
4 Authorization NA No auth in MVP by design. No routes assume auth-gating exists. TODO-AUTH callsites are absent — recommend adding a comment at RegisterRoutes so the auth bead knows where to insert middleware.
5 Input validation WARN name has no length cap in Go-side validate(). DB column is varchar(255) so oversized input will produce a DB error, but the error is mapped to a generic 500. allowed_formats correctly rejects whitespace and non-lowercase. organization_mode uses a closed enum map — correct. path (AddPath) has no validation at all. See finding #1.
6 CSRF NA No auth means there are no cross-origin-forgeable credentials today. Gap noted: when auth lands the form submissions (POST /libraries, POST /libraries/{id}/paths, DELETE routes) will need CSRF tokens. The _method override pattern makes this more important — any origin can POST with _method=DELETE. Document for bookshelf-4op.
7 Logging hygiene OK Logs emit only library_id and path_id (integers). No PII, no tokens, no full request bodies.
8 Error message leakage OK writeError in error_mapper.go returns only http.StatusText(status) — no internal paths, DB error text, or stack traces reach the client.
9 Resource exhaustion WARN ListLibraries has LIMIT 200 (hard-coded defaultListLimit). ListLibraryPaths inside Get has no LIMIT — SELECT id, path, library_id FROM library_path WHERE library_id = ? returns all paths for a library unconditionally. In practice libraries have few paths but it violates the project's "no unbounded queries" rule. See finding #3.
10 HTTP body size WARN No http.MaxBytesReader wrapping on any handler. ReadTimeout: 5s on the server provides an indirect cap, but a slow client sending a multi-megabyte JSON body will hold the goroutine for the full 5 seconds before a timeout fires. See finding #4.

Findings

WARN internal/library/service.go:AddPath (line 169) — path is stored with no validation. No length check, no rejection of .., no normalization. When AXT-2 lands and the scanner walks these paths, a crafted value like ../../../../etc or an overly long string could cause filesystem access outside the intended root or a DB truncation error surfaced as an opaque 500.

WARN templates/pages/libraries_index.html:30, templates/pages/libraries_show.html:8,39 — data-confirm-message="Delete library {{.Name}}?" and data-confirm-message="Remove path {{.Path}}?". Go's html/template correctly HTML-escapes " inside attribute values (it becomes &#34;) so attribute injection is blocked. The JS then reads this.element.dataset.confirmMessage which is the decoded value, so the confirm dialog will show the raw library name. This is fine for display. However, if a library name contains a single quote (e.g. O'Reilly) the dialog shows it correctly because window.confirm() takes a string, not HTML. No actual XSS here — downgrading this to a note rather than a blocker.

WARN internal/db/queries/library.sql:25-27 — ListLibraryPaths has no LIMIT. Query: SELECT id, path, library_id FROM library_path WHERE library_id = ?. Violates the project rule "no unbounded queries over user-growable tables."

WARN internal/library/handler.go / internal/app/app.go — No http.MaxBytesReader on request bodies for create/update/addPath handlers. A 5-second ReadTimeout provides a loose safety net but does not bound body size directly.

NOTE internal/library/handler.go:deletePathHandler (lines 288-319) — libraryID is parsed from the URL but the DeletePath service call uses only pathID. There is no check that the path actually belongs to the given library. Today this just means a client can delete any path by its numeric ID regardless of the {id} in the URL. With no auth this is not exploitable beyond a UI inconsistency, but it will matter once auth scopes libraries to users (TODO-AUTH).

NOTE internal/library/service.go:197-214 (validate) — name has no len(name) > 255 guard. DB column is varchar(255). The error path on overlong insert is an unhandled driver error → generic 500 with no user-friendly message.

Verdict

⚠️ Notes (won't block merge) — no blockers found, but three items should become follow-up beads before AXT-2 (scanner) lands:

  1. Path validation at AddPath boundary — at minimum: len(path) > 4096 rejection, block empty string, block paths containing \x00. Full canonicalization can wait until scan bead but the null-byte guard should land with this PR or immediately after.
  2. Name length guard — add len(strings.TrimSpace(name)) > 255 → ErrValidation to produce a 400 instead of a 500.
  3. LIMIT on ListLibraryPaths — add LIMIT 1000 (or a configurable cap) to the SQL query.
  4. TODO-AUTH comment at RegisterRoutes — single line so the auth bead author doesn't miss the injection point.

Items 2–4 are safe to ship as-is. Item 1 (path null-byte) is low risk now but should be a fast follow-up bead before the scanner lands.

## Security Review (automated) Reviewed commit `779ee38` — Library + LibraryPath CRUD API + UI. ### Threat assessment | # | Threat | Status | Notes | |---|--------|--------|-------| | 1 | SQL injection | **OK** | All queries go through sqlc with `?` placeholders. No hand-written string-interpolated SQL found anywhere in the diff. | | 2 | Path traversal | **WARN** | `library_path.path` is stored verbatim with zero validation — no length cap, no canonicalization, no rejection of `..` sequences or absolute paths that escape a configured root. The scanner (AXT-2) hasn't landed yet so there's no exploit path today, but the input gate is open. See finding #1 below. | | 3 | XSS | **WARN** | No `template.HTML`/`template.JS` raw casts — `html/template` auto-escaping is intact for all `{{.}}` expressions. However, `data-confirm-message="Delete library {{.Name}}?"` puts user-controlled text into an HTML attribute without the `"` inside the value being escapable by Go's template engine in that context. See finding #2. | | 4 | Authorization | **NA** | No auth in MVP by design. No routes assume auth-gating exists. `TODO-AUTH` callsites are absent — recommend adding a comment at `RegisterRoutes` so the auth bead knows where to insert middleware. | | 5 | Input validation | **WARN** | `name` has no length cap in Go-side `validate()`. DB column is `varchar(255)` so oversized input will produce a DB error, but the error is mapped to a generic 500. `allowed_formats` correctly rejects whitespace and non-lowercase. `organization_mode` uses a closed enum map — correct. `path` (AddPath) has **no validation at all**. See finding #1. | | 6 | CSRF | **NA** | No auth means there are no cross-origin-forgeable credentials today. Gap noted: when auth lands the form submissions (POST /libraries, POST /libraries/{id}/paths, DELETE routes) will need CSRF tokens. The `_method` override pattern makes this more important — any origin can POST with `_method=DELETE`. Document for bookshelf-4op. | | 7 | Logging hygiene | **OK** | Logs emit only `library_id` and `path_id` (integers). No PII, no tokens, no full request bodies. | | 8 | Error message leakage | **OK** | `writeError` in `error_mapper.go` returns only `http.StatusText(status)` — no internal paths, DB error text, or stack traces reach the client. | | 9 | Resource exhaustion | **WARN** | `ListLibraries` has `LIMIT 200` (hard-coded `defaultListLimit`). `ListLibraryPaths` inside `Get` has **no LIMIT** — `SELECT id, path, library_id FROM library_path WHERE library_id = ?` returns all paths for a library unconditionally. In practice libraries have few paths but it violates the project's "no unbounded queries" rule. See finding #3. | | 10 | HTTP body size | **WARN** | No `http.MaxBytesReader` wrapping on any handler. `ReadTimeout: 5s` on the server provides an indirect cap, but a slow client sending a multi-megabyte JSON body will hold the goroutine for the full 5 seconds before a timeout fires. See finding #4. | --- ### Findings **WARN** `internal/library/service.go:AddPath` (line 169) — `path` is stored with no validation. No length check, no rejection of `..`, no normalization. When AXT-2 lands and the scanner walks these paths, a crafted value like `../../../../etc` or an overly long string could cause filesystem access outside the intended root or a DB truncation error surfaced as an opaque 500. **WARN** `templates/pages/libraries_index.html:30`, `templates/pages/libraries_show.html:8,39` — `data-confirm-message="Delete library {{.Name}}?"` and `data-confirm-message="Remove path {{.Path}}?"`. Go's `html/template` correctly HTML-escapes `"` inside attribute values (it becomes `&#34;`) so attribute injection is blocked. The JS then reads `this.element.dataset.confirmMessage` which is the decoded value, so the confirm dialog will show the raw library name. This is fine for display. However, if a library name contains a single quote (e.g. `O'Reilly`) the dialog shows it correctly because `window.confirm()` takes a string, not HTML. **No actual XSS here** — downgrading this to a note rather than a blocker. **WARN** `internal/db/queries/library.sql:25-27` — `ListLibraryPaths` has no `LIMIT`. Query: `SELECT id, path, library_id FROM library_path WHERE library_id = ?`. Violates the project rule "no unbounded queries over user-growable tables." **WARN** `internal/library/handler.go` / `internal/app/app.go` — No `http.MaxBytesReader` on request bodies for create/update/addPath handlers. A 5-second `ReadTimeout` provides a loose safety net but does not bound body size directly. **NOTE** `internal/library/handler.go:deletePathHandler` (lines 288-319) — `libraryID` is parsed from the URL but the `DeletePath` service call uses only `pathID`. There is no check that the path actually belongs to the given library. Today this just means a client can delete any path by its numeric ID regardless of the `{id}` in the URL. With no auth this is not exploitable beyond a UI inconsistency, but it will matter once auth scopes libraries to users (TODO-AUTH). **NOTE** `internal/library/service.go:197-214` (`validate`) — `name` has no `len(name) > 255` guard. DB column is `varchar(255)`. The error path on overlong insert is an unhandled driver error → generic 500 with no user-friendly message. --- ### Verdict ⚠️ Notes (won't block merge) — no blockers found, but three items should become follow-up beads before AXT-2 (scanner) lands: 1. **Path validation at AddPath boundary** — at minimum: `len(path) > 4096` rejection, block empty string, block paths containing `\x00`. Full canonicalization can wait until scan bead but the null-byte guard should land with this PR or immediately after. 2. **Name length guard** — add `len(strings.TrimSpace(name)) > 255` → ErrValidation to produce a 400 instead of a 500. 3. **LIMIT on ListLibraryPaths** — add `LIMIT 1000` (or a configurable cap) to the SQL query. 4. **TODO-AUTH comment at RegisterRoutes** — single line so the auth bead author doesn't miss the injection point. Items 2–4 are safe to ship as-is. Item 1 (path null-byte) is low risk now but should be a fast follow-up bead before the scanner lands.
zombor commented 2026-05-20 23:05:23 +00:00
Author
Owner
Copy link

Code Review (automated)

Demo verification

  • make test — passes (verified via go test -race ./internal/...)
  • make coverage — 100% on internal/library/ confirmed (go test -race -coverprofile=... ./internal/library/... → total: 100.0%)
  • make e2e — not run (Docker required; CI is green per implementer)
  • docker compose up — not run locally

Spec compliance

All deliverables from the bead description are present:

  • internal/db/queries/library.sql — all 8 queries (ListLibraries, GetLibrary, CreateLibrary, UpdateLibrary, DeleteLibrary, ListLibraryPaths, AddLibraryPath, DeleteLibraryPath)
  • internal/db/sqlc/library.sql.go — regenerated
  • internal/library service package — functional-args style, no Queries struct
  • internal/library handler package — all routes registered, content-negotiated, WantsJSON branches
  • templates/pages/libraries_index.html, libraries_show.html, libraries_form.html
  • Stimulus controllers: libraries_form_controller.js, confirm_delete_controller.js
  • main.go / internal/app wired
  • e2e/api/libraries_test.go — all endpoint coverage
  • 100% coverage on internal/library/...

Missing/concern on spec:

  • PathCount in the list view (libraries_index.html) always renders as 0 because ListLibraries does not join paths and rowToLibrary() (used by List()) never sets PathCount. The template at libraries_index.html shows a "Paths" column that will always display 0. Not a blocker for MVP but misleading.

Code quality findings

Critical:

  • templates/pages/libraries_index.html:37, templates/pages/libraries_show.html:6, templates/pages/libraries_form.html:18 — HTML forms use <input type="hidden" name="_method" value="DELETE"> and value="PUT" for browser-initiated delete/update, but there is no method-override middleware in internal/app/app.go or anywhere in internal/middleware/. The Go 1.22 mux pattern DELETE /libraries/{id} will never match a POST with _method=DELETE. Browser delete and edit-save will both silently fail (likely route to wrong handler or 405). The JSON API path is unaffected. This is a bug that makes the HTML UI non-functional.

Important:

  • internal/library/handler.go:108,211,239,275,310 — All Logger.Info calls omit trace_id. The logging standard requires "Always trace_id to correlate requests across services." The middleware.TraceIDFromContext(r.Context()) helper is already used in internal/app/app.go:155 but not in any library handler log line. Example fix: d.Logger.Info("library created", "library_id", id, "trace_id", middleware.TraceIDFromContext(r.Context())).

Minor:

  • internal/library/service.go:83 — PathCount is only set in Get() but not in rowToLibrary(). The list view therefore always shows 0 paths. A quick N+1-free fix would be a separate SELECT library_id, COUNT(*) FROM library_path GROUP BY library_id batched query, but this is an MVP scope call.
  • e2e/api/libraries_test.go:222 — Expect(lib["paths"]).To(BeNil()) — after deleting the only path, the JSON response for Get returns an empty slice [], which marshals to null due to omitempty on a nil slice vs empty slice. Worth verifying the actual behavior is consistent.
  • internal/library/handler.go:838 — Both PUT and PATCH are wired to the same updateHandler but the _method override only sends PUT, so PATCH is dead code unless called via the API directly. Not a bug but YAGNI.

Tests

Depth is good. The service tests exercise all error paths including LastInsertId failures. Handler tests cover both HTML and JSON branches, validation errors, and all sentinel error types. The 848-line handler test covers every registered route including invalid-ID and error-propagation cases. Coverage is genuinely 100%, not just line-coverage theatre.

One gap: no test verifies that PathCount is 0 in the list response (because it's not yet wired). That's acceptable for MVP but worth noting.

Verdict

⚠️ Nits, may merge — except the _method override bug is significant: the HTML UI's delete and edit buttons are broken. If this PR is merged as-is, the browser UI will not work for delete/update actions.

If the HTML UI being broken is acceptable for this MVP bead (JSON API works), it can merge with a follow-up bead to add method-override middleware. Otherwise the fix is one middleware function + one line in app.go.

Must-fix before merge if browser UI is in scope:

  • Add a method-override middleware that reads _method from POST form values and rewrites r.Method before routing (standard Go pattern: ~10 lines). Wire it in internal/app/app.go before the mux.

Should-fix (non-blocking):

  • Add trace_id to all Logger.Info calls in internal/library/handler.go.
## Code Review (automated) ### Demo verification - [x] `make test` — passes (verified via `go test -race ./internal/...`) - [x] `make coverage` — 100% on `internal/library/` confirmed (`go test -race -coverprofile=... ./internal/library/...` → `total: 100.0%`) - [ ] `make e2e` — not run (Docker required; CI is green per implementer) - [ ] `docker compose up` — not run locally ### Spec compliance All deliverables from the bead description are present: - [x] `internal/db/queries/library.sql` — all 8 queries (ListLibraries, GetLibrary, CreateLibrary, UpdateLibrary, DeleteLibrary, ListLibraryPaths, AddLibraryPath, DeleteLibraryPath) - [x] `internal/db/sqlc/library.sql.go` — regenerated - [x] `internal/library` service package — functional-args style, no Queries struct - [x] `internal/library` handler package — all routes registered, content-negotiated, WantsJSON branches - [x] `templates/pages/libraries_index.html`, `libraries_show.html`, `libraries_form.html` - [x] Stimulus controllers: `libraries_form_controller.js`, `confirm_delete_controller.js` - [x] `main.go` / `internal/app` wired - [x] `e2e/api/libraries_test.go` — all endpoint coverage - [x] 100% coverage on `internal/library/...` **Missing/concern on spec:** - `PathCount` in the list view (`libraries_index.html`) always renders as `0` because `ListLibraries` does not join paths and `rowToLibrary()` (used by `List()`) never sets `PathCount`. The template at `libraries_index.html` shows a "Paths" column that will always display 0. Not a blocker for MVP but misleading. ### Code quality findings **Critical:** - `templates/pages/libraries_index.html:37`, `templates/pages/libraries_show.html:6`, `templates/pages/libraries_form.html:18` — HTML forms use `<input type="hidden" name="_method" value="DELETE">` and `value="PUT"` for browser-initiated delete/update, but **there is no method-override middleware** in `internal/app/app.go` or anywhere in `internal/middleware/`. The Go 1.22 mux pattern `DELETE /libraries/{id}` will never match a `POST` with `_method=DELETE`. Browser delete and edit-save will both silently fail (likely route to wrong handler or 405). The JSON API path is unaffected. This is a **bug that makes the HTML UI non-functional**. **Important:** - `internal/library/handler.go:108,211,239,275,310` — All `Logger.Info` calls omit `trace_id`. The logging standard requires "Always trace_id to correlate requests across services." The `middleware.TraceIDFromContext(r.Context())` helper is already used in `internal/app/app.go:155` but not in any library handler log line. Example fix: `d.Logger.Info("library created", "library_id", id, "trace_id", middleware.TraceIDFromContext(r.Context()))`. **Minor:** - `internal/library/service.go:83` — `PathCount` is only set in `Get()` but not in `rowToLibrary()`. The list view therefore always shows 0 paths. A quick N+1-free fix would be a separate `SELECT library_id, COUNT(*) FROM library_path GROUP BY library_id` batched query, but this is an MVP scope call. - `e2e/api/libraries_test.go:222` — `Expect(lib["paths"]).To(BeNil())` — after deleting the only path, the JSON response for `Get` returns an empty slice `[]`, which marshals to `null` due to `omitempty` on a nil slice vs empty slice. Worth verifying the actual behavior is consistent. - `internal/library/handler.go:838` — Both `PUT` and `PATCH` are wired to the same `updateHandler` but the `_method` override only sends `PUT`, so `PATCH` is dead code unless called via the API directly. Not a bug but YAGNI. ### Tests Depth is good. The service tests exercise all error paths including `LastInsertId` failures. Handler tests cover both HTML and JSON branches, validation errors, and all sentinel error types. The 848-line handler test covers every registered route including invalid-ID and error-propagation cases. Coverage is genuinely 100%, not just line-coverage theatre. One gap: no test verifies that `PathCount` is 0 in the list response (because it's not yet wired). That's acceptable for MVP but worth noting. ### Verdict ⚠️ Nits, may merge — **except the `_method` override bug is significant**: the HTML UI's delete and edit buttons are broken. If this PR is merged as-is, the browser UI will not work for delete/update actions. If the HTML UI being broken is acceptable for this MVP bead (JSON API works), it can merge with a follow-up bead to add method-override middleware. Otherwise the fix is one middleware function + one line in `app.go`. **Must-fix before merge if browser UI is in scope:** - Add a method-override middleware that reads `_method` from POST form values and rewrites `r.Method` before routing (standard Go pattern: ~10 lines). Wire it in `internal/app/app.go` before the mux. **Should-fix (non-blocking):** - Add `trace_id` to all `Logger.Info` calls in `internal/library/handler.go`.
fix(bookshelf-axt.1): address code and security review findings from PR #18
All checks were successful
/ Lint (pull_request) Successful in 3m58s
Details
/ Test (pull_request) Successful in 4m9s
Details
/ E2E (pull_request) Successful in 4m52s
Details
/ Coverage (pull_request) Successful in 1m37s
Details
b465add41b
zombor commented 2026-05-20 23:25:25 +00:00
Author
Owner
Copy link

Review-fix commit

Addressed all findings from the code review (comment #1350) and security review (comment #1345):

  • Method-override middleware (internal/middleware/method_override.go) — POST + _method=DELETE|PUT|PATCH now correctly rewrites to the target verb before routing. Browser delete/edit flows are functional. 100% test coverage including multipart/form-data, invalid override values, and non-POST passthrough cases.
  • trace_id on Logger.Info — all 5 log calls in internal/library/handler.go now include trace_id from context per logging-standard.
  • LIMIT on ListLibraryPaths — SQL query capped at 500, satisfies no-unbounded-queries rule.
  • MaxBytes middleware (internal/middleware/max_bytes.go) — 1 MB body cap on all non-GET/HEAD requests; wired in app.go.
  • Name length guard — validate() now returns ErrValidation (400) on len(name) > 255 before hitting the DB.
  • Path input validation — validatePath() in service rejects: empty/whitespace, null bytes, len > 4096, relative paths, .. traversal segments.
  • TODO-AUTH comments — added at deletePathHandler, MethodOverride middleware, and RegisterRoutes.
  • Browser e2e test — e2e/browser/libraries_test.go exercises the full HTML delete flow (navigate → confirm dialog bypassed → delete submitted → row gone). This test would have caught the original method-override bug.

CI: success
Mergeable: True

## Review-fix commit Addressed all findings from the code review (comment #1350) and security review (comment #1345): - **Method-override middleware** (`internal/middleware/method_override.go`) — POST + `_method=DELETE|PUT|PATCH` now correctly rewrites to the target verb before routing. Browser delete/edit flows are functional. 100% test coverage including multipart/form-data, invalid override values, and non-POST passthrough cases. - **trace_id on Logger.Info** — all 5 log calls in `internal/library/handler.go` now include `trace_id` from context per logging-standard. - **LIMIT on ListLibraryPaths** — SQL query capped at 500, satisfies no-unbounded-queries rule. - **MaxBytes middleware** (`internal/middleware/max_bytes.go`) — 1 MB body cap on all non-GET/HEAD requests; wired in `app.go`. - **Name length guard** — `validate()` now returns `ErrValidation` (400) on `len(name) > 255` before hitting the DB. - **Path input validation** — `validatePath()` in service rejects: empty/whitespace, null bytes, len > 4096, relative paths, `..` traversal segments. - **TODO-AUTH comments** — added at `deletePathHandler`, `MethodOverride` middleware, and `RegisterRoutes`. - **Browser e2e test** — `e2e/browser/libraries_test.go` exercises the full HTML delete flow (navigate → confirm dialog bypassed → delete submitted → row gone). This test would have caught the original method-override bug. CI: success Mergeable: True
zombor commented 2026-05-20 23:27:44 +00:00
Author
Owner
Copy link

Security Re-review (round 2)

Fix verification

  • ListLibraryPaths LIMIT capped — internal/db/queries/library.sql line 28: LIMIT ? parameterised and called with maxLibraryPaths = 500 (service.go:48, 73). SQL for ListLibraries also uses LIMIT ?, called with defaultListLimit = 200 (handler.go:17, 57). Both hard-coded at the call site — no user-controlled value reaches the DB. ✅
  • Body size cap (MaxBytesReader) — internal/middleware/max_bytes.go wraps all non-GET/HEAD requests at 1 << 20 (1 MB) via http.MaxBytesReader. Applied globally in app.go at the outer middleware chain level (line 164), so every handler is covered. ✅
  • Name length guard — service.go:221: len(name) > maxNameLen (255) → ErrValidation before any DB call. ✅
  • Path validation (empty / null / length / traversal / absolute) — validatePath() at service.go:242–263 covers all five checks:
    • Empty/whitespace: strings.TrimSpace(path) == "" → ErrValidation ✅
    • Null byte: strings.ContainsRune(path, 0) → ErrValidation ✅
    • Length: len(path) > maxPathLen (4096) → ErrValidation ✅
    • Absolute path required: !filepath.IsAbs(path) → ErrValidation ✅
    • Traversal (.. segments): segments loop rejects any component == ".." → ErrValidation ✅
  • TODO-AUTH comments — present at three locations:
    • handler.go:36: RegisterRoutes — TODO(auth) note about auth middleware insertion
    • handler.go:307: deletePathHandler — TODO(auth) note about ownership verification
    • method_override.go:32: TODO(auth) CSRF ordering note ✅

Fresh-pass findings

internal/middleware/method_override.go

Solid design. The validOverrides closed set correctly whitelists only PUT/PATCH/DELETE, so injection of arbitrary methods (CONNECT, TRACE, etc.) is impossible. The middleware only fires when r.Method == POST, preventing non-POST requests from being silently mutated. JSON bodies are not parsed — only x-www-form-urlencoded and multipart/form-data trigger ParseForm/ParseMultipartForm, so a JSON-encoded _method field in a JSON POST body is harmless (verified by test at method_override_test.go:82–87).

One minor note: the multipart/form-data branch calls r.ParseMultipartForm(maxFormSize) with its own 1 MB cap (1 << 20, defined locally in method_override.go:17). At the outer middleware chain level MaxBytes already wraps the body before MethodOverride runs (app.go:163–164, outer → inner order: MaxBytes wraps, then MethodOverride wraps), so an oversized multipart body is rejected by MaxBytesReader before ParseMultipartForm even attempts to buffer it into memory. The double-cap is belt-and-suspenders and is not a bypass vector.

internal/middleware/max_bytes.go

Clean. The middleware delegating 413 status relies on downstream code reading r.Body and surfacing the error. In practice the echo handler in the test, and the real handlers all read via json.NewDecoder(r.Body).Decode(...) or r.FormValue(...) which go through ParseForm. When MaxBytesReader limit fires, Read returns *http.MaxBytesError; Decode surfaces it as a non-nil error that propagates up through ErrorMapper to a 413 response. No bypass identified.

internal/library/service.go validatePath()

The ..-rejection algorithm splits on filepath.Separator and checks each segment for literal "..". This correctly catches:

  • /../etc/passwd → absolute + has .. segment → rejected ✅
  • /foo/../bar → has .. segment → rejected ✅
  • /foo/..bar/baz → segment is "..bar", not ".." → passes (correct — ..bar is not a traversal component; only .. alone is) ✅
  • foo/bar → not absolute → rejected before traversal check ✅

No symlink resolution is performed, which is consistent with the comment "does NOT stat the filesystem — the scanner (AXT-2) owns that concern." This is the right split of responsibility: the scanner can canonicalise / follow symlinks when it actually touches the filesystem. The current lexical validation at the API boundary is appropriate.

One observation: the order of checks puts the filepath.IsAbs guard before the traversal loop, which means a relative path like foo/../bar fails at the absolute check first — so the traversal check never runs for relative paths. This is fine because relative paths are already rejected, but it means the traversal check is only meaningful for absolute paths starting with /. That is correct behaviour, not a bug.

Verdict

✅ No security concerns — all round-1 items are addressed correctly and the new middleware has no exploitable weaknesses. Ready to merge.

## Security Re-review (round 2) ### Fix verification - [x] **ListLibraryPaths LIMIT capped** — `internal/db/queries/library.sql` line 28: `LIMIT ?` parameterised and called with `maxLibraryPaths = 500` (service.go:48, 73). SQL for `ListLibraries` also uses `LIMIT ?`, called with `defaultListLimit = 200` (handler.go:17, 57). Both hard-coded at the call site — no user-controlled value reaches the DB. ✅ - [x] **Body size cap (MaxBytesReader)** — `internal/middleware/max_bytes.go` wraps all non-GET/HEAD requests at `1 << 20` (1 MB) via `http.MaxBytesReader`. Applied globally in `app.go` at the outer middleware chain level (line 164), so every handler is covered. ✅ - [x] **Name length guard** — `service.go:221`: `len(name) > maxNameLen` (255) → `ErrValidation` before any DB call. ✅ - [x] **Path validation (empty / null / length / traversal / absolute)** — `validatePath()` at service.go:242–263 covers all five checks: - Empty/whitespace: `strings.TrimSpace(path) == ""` → ErrValidation ✅ - Null byte: `strings.ContainsRune(path, 0)` → ErrValidation ✅ - Length: `len(path) > maxPathLen` (4096) → ErrValidation ✅ - Absolute path required: `!filepath.IsAbs(path)` → ErrValidation ✅ - Traversal (`..` segments): segments loop rejects any component `== ".."` → ErrValidation ✅ - [x] **TODO-AUTH comments** — present at three locations: - `handler.go:36`: `RegisterRoutes` — TODO(auth) note about auth middleware insertion - `handler.go:307`: `deletePathHandler` — TODO(auth) note about ownership verification - `method_override.go:32`: TODO(auth) CSRF ordering note ✅ ### Fresh-pass findings **`internal/middleware/method_override.go`** Solid design. The `validOverrides` closed set correctly whitelists only PUT/PATCH/DELETE, so injection of arbitrary methods (CONNECT, TRACE, etc.) is impossible. The middleware only fires when `r.Method == POST`, preventing non-POST requests from being silently mutated. JSON bodies are not parsed — only `x-www-form-urlencoded` and `multipart/form-data` trigger `ParseForm`/`ParseMultipartForm`, so a JSON-encoded `_method` field in a JSON POST body is harmless (verified by test at method_override_test.go:82–87). One minor note: the `multipart/form-data` branch calls `r.ParseMultipartForm(maxFormSize)` with its own 1 MB cap (`1 << 20`, defined locally in method_override.go:17). At the outer middleware chain level `MaxBytes` already wraps the body before `MethodOverride` runs (app.go:163–164, outer → inner order: MaxBytes wraps, then MethodOverride wraps), so an oversized multipart body is rejected by `MaxBytesReader` before `ParseMultipartForm` even attempts to buffer it into memory. The double-cap is belt-and-suspenders and is not a bypass vector. **`internal/middleware/max_bytes.go`** Clean. The middleware delegating 413 status relies on downstream code reading `r.Body` and surfacing the error. In practice the echo handler in the test, and the real handlers all read via `json.NewDecoder(r.Body).Decode(...)` or `r.FormValue(...)` which go through `ParseForm`. When `MaxBytesReader` limit fires, `Read` returns `*http.MaxBytesError`; `Decode` surfaces it as a non-nil error that propagates up through `ErrorMapper` to a 413 response. No bypass identified. **`internal/library/service.go` `validatePath()`** The `..`-rejection algorithm splits on `filepath.Separator` and checks each segment for literal `".."`. This correctly catches: - `/../etc/passwd` → absolute + has `..` segment → rejected ✅ - `/foo/../bar` → has `..` segment → rejected ✅ - `/foo/..bar/baz` → segment is `"..bar"`, not `".."` → **passes** (correct — `..bar` is not a traversal component; only `..` alone is) ✅ - `foo/bar` → not absolute → rejected before traversal check ✅ No symlink resolution is performed, which is consistent with the comment "does NOT stat the filesystem — the scanner (AXT-2) owns that concern." This is the right split of responsibility: the scanner can canonicalise / follow symlinks when it actually touches the filesystem. The current lexical validation at the API boundary is appropriate. One observation: the order of checks puts the `filepath.IsAbs` guard before the traversal loop, which means a relative path like `foo/../bar` fails at the absolute check first — so the traversal check never runs for relative paths. This is fine because relative paths are already rejected, but it means the traversal check is only meaningful for absolute paths starting with `/`. That is correct behaviour, not a bug. ### Verdict ✅ No security concerns — all round-1 items are addressed correctly and the new middleware has no exploitable weaknesses. Ready to merge.
zombor commented 2026-05-21 00:09:31 +00:00
Author
Owner
Copy link

Code Re-review (round 2)

Fix verification

  • Method-override middleware exists + wired + tested

    • internal/middleware/method_override.go — clean whitelist-based implementation (PUT, PATCH, DELETE only), guards on Content-Type (url-encoded + multipart), no-op passthrough for all other methods.
    • Wired in internal/app/app.go:163 as chain = middleware.MethodOverride()(chain), placed before TraceID and the mux — correct order per the comment at line 161.
    • internal/middleware/method_override_test.go — 11 cases: POST→DELETE (url-encoded), POST→PUT, POST→PATCH, POST→DELETE (multipart), GET passthrough, DELETE passthrough, application/json ignored, missing _method field, invalid override value (CONNECT), lowercase override value (delete), empty value, absent Content-Type. All required edge cases are covered.
    • Coverage: MethodOverride hits 100% in the per-package run; the overall middleware package is 99% only because NewContextWithTraceID (a test-injection helper) is not called within the middleware package's own tests — it is exercised from internal/tmpl tests and reaches 100% in the cross-package coverage run.

  • trace_id on all 5 Logger.Info calls

    • handler.go:109 — library created
    • handler.go:212 — library updated
    • handler.go:240 — library deleted
    • handler.go:276 — library path added
    • handler.go:312 — library path deleted
      All five now include "trace_id", middleware.TraceIDFromContext(r.Context()).

  • Browser e2e exercises delete flow

    • e2e/browser/libraries_test.go seeds a library via JSON API, navigates to /libraries, verifies the row is present, stubs window.confirm to auto-accept, clicks form[action="/libraries/{id}"] button[type="submit"], waits for page load, then asserts via Eventually that the row selector returns 0 elements. This directly exercises the _method=DELETE form path and would have caught the original missing-middleware bug.

Test results

All non-DB-dependent packages pass with race detector:

ok  internal/middleware   (MethodOverride 100%, MaxBytes 100%)
ok  internal/library      100% coverage on all handlers and service functions
ok  internal/handler
ok  internal/tmpl
ok  internal/config
ok  internal/httpserver
ok  internal/observability
ok  internal/version
ok  static

internal/app timed out locally due to Docker/MySQL not running — this is the expected behaviour when testcontainers cannot start a container; CI runs against a live MySQL service container. Not a regression.

Fresh-pass nits

  • Minor: internal/middleware/method_override.go:17 — maxFormSize is declared as a const inside the middleware file but is also the effective cap used when MethodOverride parses multipart bodies. internal/middleware/max_bytes.go:8 declares maxRequestBodySize as a separate constant with the same 1 MB value. These two constants are not shared; if someone bumps one they will not bump the other. Harmless now, but worth unifying in a follow-up.
  • Minor: The browser e2e test uses page.MustWaitLoad() after the delete click. rod's MustWaitLoad waits for the load event but not for network quiescence. On a slow CI runner this could be flaky; MustWaitStable() (as used on the initial navigation) would be more robust. Not a blocker.

Verdict

✅ Ready to merge — all three must-fixes from round 1 are addressed, tests pass, coverage is 100% on internal/library and 100% on internal/middleware/method_override.go. The two fresh nits are minor and do not block merge.

## Code Re-review (round 2) ### Fix verification - [x] **Method-override middleware exists + wired + tested** - `internal/middleware/method_override.go` — clean whitelist-based implementation (PUT, PATCH, DELETE only), guards on Content-Type (url-encoded + multipart), no-op passthrough for all other methods. - Wired in `internal/app/app.go:163` as `chain = middleware.MethodOverride()(chain)`, placed before `TraceID` and the mux — correct order per the comment at line 161. - `internal/middleware/method_override_test.go` — 11 cases: POST→DELETE (url-encoded), POST→PUT, POST→PATCH, POST→DELETE (multipart), GET passthrough, DELETE passthrough, application/json ignored, missing `_method` field, invalid override value (`CONNECT`), lowercase override value (`delete`), empty value, absent Content-Type. All required edge cases are covered. - Coverage: `MethodOverride` hits 100% in the per-package run; the overall middleware package is 99% only because `NewContextWithTraceID` (a test-injection helper) is not called within the middleware package's own tests — it is exercised from `internal/tmpl` tests and reaches 100% in the cross-package coverage run. - [x] **trace_id on all 5 Logger.Info calls** - `handler.go:109` — library created - `handler.go:212` — library updated - `handler.go:240` — library deleted - `handler.go:276` — library path added - `handler.go:312` — library path deleted All five now include `"trace_id", middleware.TraceIDFromContext(r.Context())`. - [x] **Browser e2e exercises delete flow** - `e2e/browser/libraries_test.go` seeds a library via JSON API, navigates to `/libraries`, verifies the row is present, stubs `window.confirm` to auto-accept, clicks `form[action="/libraries/{id}"] button[type="submit"]`, waits for page load, then asserts via `Eventually` that the row selector returns 0 elements. This directly exercises the `_method=DELETE` form path and would have caught the original missing-middleware bug. ### Test results All non-DB-dependent packages pass with race detector: ``` ok internal/middleware (MethodOverride 100%, MaxBytes 100%) ok internal/library 100% coverage on all handlers and service functions ok internal/handler ok internal/tmpl ok internal/config ok internal/httpserver ok internal/observability ok internal/version ok static ``` `internal/app` timed out locally due to Docker/MySQL not running — this is the expected behaviour when testcontainers cannot start a container; CI runs against a live MySQL service container. Not a regression. ### Fresh-pass nits - **Minor:** `internal/middleware/method_override.go:17` — `maxFormSize` is declared as a `const` inside the middleware file but is also the effective cap used when `MethodOverride` parses multipart bodies. `internal/middleware/max_bytes.go:8` declares `maxRequestBodySize` as a separate constant with the same 1 MB value. These two constants are not shared; if someone bumps one they will not bump the other. Harmless now, but worth unifying in a follow-up. - **Minor:** The browser e2e test uses `page.MustWaitLoad()` after the delete click. `rod`'s `MustWaitLoad` waits for the `load` event but not for network quiescence. On a slow CI runner this could be flaky; `MustWaitStable()` (as used on the initial navigation) would be more robust. Not a blocker. ### Verdict ✅ Ready to merge — all three must-fixes from round 1 are addressed, tests pass, coverage is 100% on `internal/library` and 100% on `internal/middleware/method_override.go`. The two fresh nits are minor and do not block merge.
zombor reviewed 2026-05-21 00:17:17 +00:00
internal/app/app.go Outdated
@ -95,0 +99,4 @@
Logger: logger,
Renderer: renderer,
AssetVersion: assetVersion,
ListFn: func(ctx context.Context, limit int32) ([]library.Library, error) {
zombor commented 2026-05-21 00:12:34 +00:00
Author
Owner
Copy link

Can't all of these be something like:

ListFn: library.List,
GetFn, library.Get,

etc?

Can't all of these be something like: ``` ListFn: library.List, GetFn, library.Get, ``` etc?
zombor marked this conversation as resolved
internal/app/app_test.go Outdated
@ -234,1 +236,4 @@
})
Describe("GET /libraries (JSON)", func() {
It("returns 200 with an empty JSON array", func() {
zombor commented 2026-05-21 00:13:28 +00:00
Author
Owner
Copy link

It blocks should have a single assertion in it. The test setup should be in BeforeEach and invocation of the thing under test should happen in JustBeforeEach. Make sure this happens for all tests in this PR.

`It` blocks should have a single assertion in it. The test setup should be in `BeforeEach` and invocation of the thing under test should happen in `JustBeforeEach`. Make sure this happens for all tests in this PR.
zombor marked this conversation as resolved
internal/library/handler.go Outdated
@ -0,0 +22,4 @@
Renderer *tmpl.Renderer
AssetVersion string
ListFn func(context.Context, int32) ([]Library, error)
zombor commented 2026-05-21 00:14:21 +00:00
Author
Owner
Copy link

Let's omit the Fn suffix on these as a project convention.

Let's omit the `Fn` suffix on these as a project convention.
zombor marked this conversation as resolved
internal/library/service.go Outdated
@ -0,0 +19,4 @@
}
// List returns up to limit libraries ordered by sort/name.
func List(
zombor commented 2026-05-21 00:16:41 +00:00
Author
Owner
Copy link

As a project convention, our functions should have dependencies in the top level function and it should return another function with the actual parameters, like:

func List(
  list func(context.Context, int32) ([]sqlc.ListLibrariesRow, error),
) func(context.Context, int) ([]Library, error) {
  return func(ctx context.Context, limit int) ([]Library, error) {
    ...
  }
}
As a project convention, our functions should have dependencies in the top level function and it should return another function with the actual parameters, like: ``` func List( list func(context.Context, int32) ([]sqlc.ListLibrariesRow, error), ) func(context.Context, int) ([]Library, error) { return func(ctx context.Context, limit int) ([]Library, error) { ... } } ```
zombor marked this conversation as resolved
Apply review comments: curried service funcs, no Fn suffix, single-assertion tests
Some checks failed
/ Lint (pull_request) Failing after 1m7s
Details
/ Test (pull_request) Has been cancelled
Details
/ E2E (pull_request) Has been cancelled
Details
/ Coverage (pull_request) Has been cancelled
Details
e8d1594365
fix: add missing library_extra.go (CountPathsForLibraries) to git tracking
Some checks failed
/ Test (pull_request) Failing after 5m11s
Details
/ Coverage (pull_request) Has been skipped
Details
/ E2E (pull_request) Failing after 6m10s
Details
/ Lint (pull_request) Successful in 7m4s
Details
ff4337689a
fix: merge double-It into single-It to prevent double POST in CI
Some checks failed
/ E2E (pull_request) Failing after 2m54s
Details
/ Lint (pull_request) Successful in 3m54s
Details
/ Test (pull_request) Successful in 4m12s
Details
/ Coverage (pull_request) Successful in 1m15s
Details
1c61627eb1
fix: truncate tables between e2e TestEnv instances when using shared DSN
All checks were successful
/ Lint (pull_request) Successful in 3m45s
Details
/ E2E (pull_request) Successful in 5m13s
Details
/ Test (pull_request) Successful in 6m36s
Details
/ Coverage (pull_request) Successful in 1m12s
Details
2834182b41
zombor commented 2026-05-21 01:17:37 +00:00
Author
Owner
Copy link

Security Review (round 3)

Refactor security check

  • New path_count SQL is parameterized + bounded
  • Curried closures don't leak per-request state
  • E2E table truncation doesn't mask security state

Round-1 fix regression check

  • ListLibraryPaths LIMIT intact
  • MaxBytes intact
  • Name length guard intact
  • validatePath intact
  • TODO-AUTH comments intact

Fresh findings

path_count SQL (internal/db/sqlc/library_extra.go): The CountPathsForLibraries function builds an IN (?, ?, ...) clause by constructing one "?" placeholder per ID in a loop, then passes args []any to QueryContext. All values are int64 — no user-supplied strings, no string interpolation of user data. The query has no LIMIT but this is appropriate: it is a SELECT library_id, COUNT(*) … GROUP BY library_id aggregation over a bounded set of IDs from the prior ListLibraries query (itself LIMIT ?). At most defaultListLimit (200) IDs are passed in; the result set has at most one row per library. No injection risk; no unbounded scan.

Curried service functions (internal/library/service.go): Each outer function (e.g. List, Get, Create, AddPath, …) captures only its func arguments — the sqlc method references (q.ListLibraries, q.CountPathsForLibraries, etc.) which are bound once at app startup via app.go:102–109. The returned inner function receives ctx context.Context and request-scoped scalar params as regular arguments. No per-request state (auth tokens, request headers, trace IDs, body content) is ever captured in the outer closure; all of that flows through ctx and explicit parameters on the inner call. No leakage vector.

E2E table truncation (e2e/testutil/server.go): Truncation only runs when BOOKSHELF_DSN is set (shared CI MySQL service container). In that mode, each NewTestEnv() call truncates all user-data tables except schema_migrations before starting the test. This improves isolation — each test starts from a blank slate. The truncation cannot mask security bugs because auth is not yet implemented (all routes are unauthenticated); once auth lands, the truncation will correctly wipe any lingering session/user state between tests. The SET FOREIGN_KEY_CHECKS=0/1 bracket around truncation is correct practice and does not create a window for data leakage between test cases. The only minor note: table names come from information_schema.tables and are inserted into TRUNCATE TABLE \%s`with backtick quoting — the schema isDATABASE()` scoped, so no user input reaches the table-name string. No injection risk.

TODO-AUTH comments: Two are present and unchanged — handler.go:37 (route registration comment) and handler.go:307 (deletePathHandler body, noting path-ownership check deferred to auth landing). Both reference bead bookshelf-4op. The deletePathHandler still accepts any valid pathId without verifying it belongs to the given libraryId. This is the known, tracked, intentional gap — not a regression.

Verdict

✅ Clean — all round-1 fixes survive the refactor, the new SQL and curried-closure pattern introduce no new vulnerabilities, and E2E isolation is correct. The only open item remains the TODO-AUTH path-ownership check, already tracked in bead bookshelf-4op.

## Security Review (round 3) ### Refactor security check - [x] New path_count SQL is parameterized + bounded - [x] Curried closures don't leak per-request state - [x] E2E table truncation doesn't mask security state ### Round-1 fix regression check - [x] ListLibraryPaths LIMIT intact - [x] MaxBytes intact - [x] Name length guard intact - [x] validatePath intact - [x] TODO-AUTH comments intact ### Fresh findings **path_count SQL (`internal/db/sqlc/library_extra.go`):** The `CountPathsForLibraries` function builds an `IN (?, ?, ...)` clause by constructing one `"?"` placeholder per ID in a loop, then passes `args []any` to `QueryContext`. All values are `int64` — no user-supplied strings, no string interpolation of user data. The query has no `LIMIT` but this is appropriate: it is a `SELECT library_id, COUNT(*) … GROUP BY library_id` aggregation over a bounded set of IDs from the prior `ListLibraries` query (itself `LIMIT ?`). At most `defaultListLimit` (200) IDs are passed in; the result set has at most one row per library. No injection risk; no unbounded scan. **Curried service functions (`internal/library/service.go`):** Each outer function (e.g. `List`, `Get`, `Create`, `AddPath`, …) captures only its `func` arguments — the sqlc method references (`q.ListLibraries`, `q.CountPathsForLibraries`, etc.) which are bound once at app startup via `app.go:102–109`. The returned inner function receives `ctx context.Context` and request-scoped scalar params as regular arguments. No per-request state (auth tokens, request headers, trace IDs, body content) is ever captured in the outer closure; all of that flows through `ctx` and explicit parameters on the inner call. No leakage vector. **E2E table truncation (`e2e/testutil/server.go`):** Truncation only runs when `BOOKSHELF_DSN` is set (shared CI MySQL service container). In that mode, each `NewTestEnv()` call truncates all user-data tables except `schema_migrations` before starting the test. This _improves_ isolation — each test starts from a blank slate. The truncation cannot mask security bugs because auth is not yet implemented (all routes are unauthenticated); once auth lands, the truncation will correctly wipe any lingering session/user state between tests. The `SET FOREIGN_KEY_CHECKS=0/1` bracket around truncation is correct practice and does not create a window for data leakage between test cases. The only minor note: table names come from `information_schema.tables` and are inserted into `TRUNCATE TABLE \`%s\`` with backtick quoting — the schema is `DATABASE()` scoped, so no user input reaches the table-name string. No injection risk. **TODO-AUTH comments:** Two are present and unchanged — `handler.go:37` (route registration comment) and `handler.go:307` (deletePathHandler body, noting path-ownership check deferred to auth landing). Both reference bead `bookshelf-4op`. The `deletePathHandler` still accepts any valid `pathId` without verifying it belongs to the given `libraryId`. This is the known, tracked, intentional gap — not a regression. ### Verdict ✅ Clean — all round-1 fixes survive the refactor, the new SQL and curried-closure pattern introduce no new vulnerabilities, and E2E isolation is correct. The only open item remains the TODO-AUTH path-ownership check, already tracked in bead bookshelf-4op.
docs(conventions): curried function pattern + ginkgo test structure
All checks were successful
/ Lint (pull_request) Successful in 3m47s
Details
/ Test (pull_request) Successful in 4m8s
Details
/ Coverage (pull_request) Successful in 1m48s
Details
/ E2E (pull_request) Successful in 6m26s
Details
d5bfdc2bfd
 
Update Code Style section to match the pattern adopted in PR #18:

- Replace the inline-deps "functional arguments" example with the curried
  form (outer func takes deps, returns request-handling func). Deps bound
  once at wiring, returned func has clean signature.
- Add "no Fn suffix" naming rule for function-valued variables/fields.
- New "Test structure (ginkgo)" subsection: BeforeEach for setup,
  JustBeforeEach for invoking the system under test, one Expect per It.

Future bead agents pick this up automatically.
zombor force-pushed bd-bookshelf-axt.1 from d5bfdc2bfd
All checks were successful
/ Lint (pull_request) Successful in 3m47s
Details
/ Test (pull_request) Successful in 4m8s
Details
/ Coverage (pull_request) Successful in 1m48s
Details
/ E2E (pull_request) Successful in 6m26s
Details
to c928e3c8c4
All checks were successful
/ Lint (pull_request) Successful in 4m1s
Details
/ Test (pull_request) Successful in 4m12s
Details
/ E2E (pull_request) Successful in 5m51s
Details
/ Coverage (pull_request) Successful in 1m37s
Details
2026-05-21 01:49:24 +00:00 Compare
zombor merged commit 07e9aa8860 into main 2026-05-21 01:56:08 +00:00
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
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!18
No description provided.