Hygiene: adopt db.RunInTx/InTx at all hand-rolled BeginTx sites (bookshelf-dhlj4.1) #1419

Merged

zombor merged 2 commits from bd-bookshelf-dhlj4.1 into main 2026-08-10 02:11:13 +00:00

Conversation 3 Commits 2 Files changed 10 +118 -196

zombor commented 2026-08-10 01:07:01 +00:00

Owner

Copy link

Summary

Migrates ~15 sites that hand-rolled BeginTx + defer Rollback + Commit to the
existing internal/db RunInTx/InTx helpers, closing the
rollback/commit-error footgun that hand-rolling risks. Also deletes
series/wire.go's bespoke buildRunInTx in favor of db.InTx.

Pure behavior-preserving refactor: no query changes, no semantic changes.
Every migrated site still rolls back on error and commits on success,
matching the original hand-rolled behavior exactly.

Sites migrated:

• internal/authors/wire.go
• internal/categories/wire.go
• internal/dedup/wire.go
• internal/library/wire.go
• internal/shelves/wire.go
• internal/series/wire.go (buildRunInTx deleted, inlined via db.InTx)
• internal/users/wire.go (5 sites: newWithTxPerms, newWithTxUpdate,
  newWithTxDelete, setLibraries, SetAdminUserContentRestrictions)
• internal/app/build_bookdrop_deps.go (2 sites)
• internal/app/build_enrich_deps.go
• internal/seeder/seeder.go

Docs: N/A — internal refactor, no user-facing behavior change.

Test plan

• go build ./... clean
• go vet ./... clean
• make test — all unit suites green
• go build -tags integration ./internal/... clean
• go build -tags e2e ./e2e/... clean
• wire.go files are excluded from the coverage gate (pure wiring,
  verified by e2e); internal/seeder is covered by
  seeder_integration_test.go which exercises insertBatch
  (now db.InTx) against a real DB.

Closes bead bookshelf-dhlj4.1 on merge.

🤖 Generated with Claude Code

https://claude.ai/code/session_016tRKybTpfjQ4SxmNdVFLHi

## Summary Migrates ~15 sites that hand-rolled `BeginTx + defer Rollback + Commit` to the existing `internal/db` `RunInTx`/`InTx` helpers, closing the rollback/commit-error footgun that hand-rolling risks. Also deletes `series/wire.go`'s bespoke `buildRunInTx` in favor of `db.InTx`. Pure behavior-preserving refactor: no query changes, no semantic changes. Every migrated site still rolls back on error and commits on success, matching the original hand-rolled behavior exactly. Sites migrated: - internal/authors/wire.go - internal/categories/wire.go - internal/dedup/wire.go - internal/library/wire.go - internal/shelves/wire.go - internal/series/wire.go (buildRunInTx deleted, inlined via db.InTx) - internal/users/wire.go (5 sites: newWithTxPerms, newWithTxUpdate, newWithTxDelete, setLibraries, SetAdminUserContentRestrictions) - internal/app/build_bookdrop_deps.go (2 sites) - internal/app/build_enrich_deps.go - internal/seeder/seeder.go Docs: N/A — internal refactor, no user-facing behavior change. ## Test plan - [x] `go build ./...` clean - [x] `go vet ./...` clean - [x] `make test` — all unit suites green - [x] `go build -tags integration ./internal/...` clean - [x] `go build -tags e2e ./e2e/...` clean - [x] `wire.go` files are excluded from the coverage gate (pure wiring, verified by e2e); `internal/seeder` is covered by `seeder_integration_test.go` which exercises `insertBatch` (now `db.InTx`) against a real DB. Closes bead bookshelf-dhlj4.1 on merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016tRKybTpfjQ4SxmNdVFLHi

zombor added 1 commit 2026-08-10 01:07:02 +00:00

refactor(db): adopt db.RunInTx/InTx at all hand-rolled BeginTx sites ... c3ca4b59da

Migrate ~15 sites that hand-rolled BeginTx + defer Rollback + Commit to
the existing internal/db RunInTx/InTx helpers, eliminating the
rollback/commit-error footgun. Delete series/wire.go's bespoke
buildRunInTx in favor of db.InTx. Pure behavior-preserving refactor —
no query or semantic changes; commit-on-success/rollback-on-error
behavior is identical.

Sites: internal/authors/wire.go, internal/categories/wire.go,
internal/dedup/wire.go, internal/library/wire.go,
internal/shelves/wire.go, internal/series/wire.go,
internal/users/wire.go (5 sites), internal/app/build_bookdrop_deps.go
(2 sites), internal/app/build_enrich_deps.go, internal/seeder/seeder.go.

Bead: bookshelf-dhlj4.1

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016tRKybTpfjQ4SxmNdVFLHi

zombor commented 2026-08-10 01:15:48 +00:00

Author

Owner

Copy link

Code review — PR #1413 (bd-bookshelf-mfli8), diff-review only (CI green).

[MAJOR] internal/users/oidc_cache.go:189 — singleflight key built by naive string concatenation can collide across distinct (issuer, jwksURI) pairs
sfKey := issuer + "|" + jwksURI has no delimiter-collision protection: two different (issuer, jwksURI) pairs can produce the identical sfKey whenever either string contains the literal | (e.g. issuer="a|b", jwksURI="c" vs issuer="a", jwksURI="b|c" both yield "a|b|c"). jwksURI in particular is sourced from meta.JWKSURI in the OIDC discovery document — i.e. content returned over the network by the (trusted, but externally-hosted) IdP, not a value under this codebase's control — so a crafted/misconfigured discovery response can produce a |-containing jwks_uri. net/url and Go's HTTP client do not reject an unescaped | in a URL, so this is reachable, not just theoretical.
Why it matters: unlike the map (correctly keyed by the jwksCacheKey struct, immune to this), singleflight.Group.Do takes a bare string key. If two concurrent get() calls for different (issuer, jwksURI) pairs collide on sfKey, only one fetch runs and both callers receive the same raw bytes — and each caller then writes those bytes into its own (distinct, correctly-scoped) map entry. That reintroduces, one layer down, the exact cross-issuer JWKS confusion this PR sets out to eliminate (issuer B's cache entry ends up populated with issuer A's fetched key material).
Fix: derive the singleflight key unambiguously from the same struct that keys the map — e.g. fmt.Sprintf("%d:%s|%s", len(issuer), issuer, jwksURI) (length-prefixing the first field removes the ambiguity) or hash the struct (sha256 over a \x00-joined encoding). Do not rely on an arbitrary separator character being absent from attacker/IdP-influenced input.

Everything else checked out:

• internal/users/oidc_cache.go:117-121 — jwksCacheKey{issuer, jwksURI} map key is a genuine struct, correctly scoping the cache (not just the log line); no path still keys the map by jwksURI alone.
• internal/users/wire.go:184-196 (origin/bd-bookshelf-mfli8) — Fetch/Refresh call sites are unchanged method-value references; the pre-existing (ctx, jwksURI, issuer) signature was already threading issuer through, so no caller needed updating for this change.
• internal/users/oidc_backchannel.go:107-146 — the backchannel-logout jwksCache is a genuinely separate, single-entry cache instantiated once per OIDCBackchannelLogout closure (internal/users/oidc_backchannel.go:355), tied to one cfg from a single getConfig call. Pergamum currently supports exactly one configured OIDC provider (OIDCConfig is singular, one Issuer field throughout oidc_service.go), so this cache has no multi-issuer surface today — the agent's claim that it needs no equivalent fix is correct, not overclaimed.
• internal/users/oidc_cache_test.go:328-359 — new black-box test (package users_test, via users.ExportNewOIDCJWKSCache) asserts two issuers sharing a jwks_uri produce two fetches (calls.Load() == 2), directly covering the fixed collision. Existing singleflight-dedup and TTL/Refresh tests are otherwise untouched and still pass with the new key type.
• Logging: "jwks_uri", jwksURI, "issuer", issuer added consistently to both the stale-serve warning and the cache-populated info log — no secrets logged (JWKS is public key material).

REVIEW VERDICT: 0 blocker, 1 major, 0 minor

Code review — PR #1413 (bd-bookshelf-mfli8), diff-review only (CI green). [MAJOR] internal/users/oidc_cache.go:189 — singleflight key built by naive string concatenation can collide across distinct (issuer, jwksURI) pairs `sfKey := issuer + "|" + jwksURI` has no delimiter-collision protection: two *different* (issuer, jwksURI) pairs can produce the identical `sfKey` whenever either string contains the literal `|` (e.g. issuer=`"a|b"`, jwksURI=`"c"` vs issuer=`"a"`, jwksURI=`"b|c"` both yield `"a|b|c"`). `jwksURI` in particular is sourced from `meta.JWKSURI` in the OIDC discovery document — i.e. content returned over the network by the (trusted, but externally-hosted) IdP, not a value under this codebase's control — so a crafted/misconfigured discovery response can produce a `|`-containing jwks_uri. `net/url` and Go's HTTP client do not reject an unescaped `|` in a URL, so this is reachable, not just theoretical. Why it matters: unlike the map (correctly keyed by the `jwksCacheKey` struct, immune to this), `singleflight.Group.Do` takes a bare `string` key. If two concurrent `get()` calls for different (issuer, jwksURI) pairs collide on `sfKey`, only one fetch runs and *both* callers receive the same `raw` bytes — and each caller then writes those bytes into its own (distinct, correctly-scoped) map entry. That reintroduces, one layer down, the exact cross-issuer JWKS confusion this PR sets out to eliminate (issuer B's cache entry ends up populated with issuer A's fetched key material). Fix: derive the singleflight key unambiguously from the same struct that keys the map — e.g. `fmt.Sprintf("%d:%s|%s", len(issuer), issuer, jwksURI)` (length-prefixing the first field removes the ambiguity) or hash the struct (`sha256` over a `\x00`-joined encoding). Do not rely on an arbitrary separator character being absent from attacker/IdP-influenced input. Everything else checked out: - internal/users/oidc_cache.go:117-121 — `jwksCacheKey{issuer, jwksURI}` map key is a genuine struct, correctly scoping the *cache* (not just the log line); no path still keys the map by `jwksURI` alone. - internal/users/wire.go:184-196 (origin/bd-bookshelf-mfli8) — `Fetch`/`Refresh` call sites are unchanged method-value references; the pre-existing `(ctx, jwksURI, issuer)` signature was already threading issuer through, so no caller needed updating for this change. - internal/users/oidc_backchannel.go:107-146 — the backchannel-logout `jwksCache` is a genuinely separate, single-entry cache instantiated once per `OIDCBackchannelLogout` closure (internal/users/oidc_backchannel.go:355), tied to one `cfg` from a single `getConfig` call. Pergamum currently supports exactly one configured OIDC provider (`OIDCConfig` is singular, one `Issuer` field throughout oidc_service.go), so this cache has no multi-issuer surface today — the agent's claim that it needs no equivalent fix is correct, not overclaimed. - internal/users/oidc_cache_test.go:328-359 — new black-box test (`package users_test`, via `users.ExportNewOIDCJWKSCache`) asserts two issuers sharing a jwks_uri produce two fetches (`calls.Load() == 2`), directly covering the fixed collision. Existing singleflight-dedup and TTL/Refresh tests are otherwise untouched and still pass with the new key type. - Logging: `"jwks_uri", jwksURI, "issuer", issuer` added consistently to both the stale-serve warning and the cache-populated info log — no secrets logged (JWKS is public key material). REVIEW VERDICT: 0 blocker, 1 major, 0 minor

zombor commented 2026-08-10 01:15:55 +00:00

Author

Owner

Copy link

No findings.

Reviewed all 15 migrated sites (authors/wire.go, categories/wire.go, dedup/wire.go,
library/wire.go, shelves/wire.go, users/wire.go x5, series/wire.go, app/build_bookdrop_deps.go x2,
app/build_enrich_deps.go, seeder/seeder.go) against db.RunInTx/InTx's contract
(internal/db/run_in_tx.go): begin tx -> defer rollback -> call f -> commit on nil error,
propagate f's error otherwise. Every hand-rolled site followed exactly that shape, so the
mechanical swap is behavior-preserving:

• No site had a conditional/early commit or partial-rollback pattern that RunInTx's
  linear defer-Rollback/Commit couldn't express.
• Multi-statement bodies (seeder.go insertBatch's 5 sequential inserts,
  users/wire.go's content-restriction delete+insert closures, comic PersistDeps
  builder in build_bookdrop_deps.go) were all correctly moved inside the RunInTx/InTx
  closure with no reordering.
• series/wire.go's deleted buildRunInTx is functionally identical to the new inline
  runInTx using db.InTx — same TxFunc adaptation, no lost deferred cleanup.
• Only observable change is error-message text (e.g. "authors merge begin tx: %w" ->
  generic "begin tx: %w" from db.RunInTx) — not a behavior/contract change, just a
  cosmetic message rename inherent to sharing the helper.
• No query or business logic touched — every diff hunk is boilerplate-only.
• No test files changed; existing black-box tests exercise these wire.go closures via
  their handlers and continue to hold since behavior is unchanged. CI green corroborates.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

No findings. Reviewed all 15 migrated sites (authors/wire.go, categories/wire.go, dedup/wire.go, library/wire.go, shelves/wire.go, users/wire.go x5, series/wire.go, app/build_bookdrop_deps.go x2, app/build_enrich_deps.go, seeder/seeder.go) against db.RunInTx/InTx's contract (internal/db/run_in_tx.go): begin tx -> defer rollback -> call f -> commit on nil error, propagate f's error otherwise. Every hand-rolled site followed exactly that shape, so the mechanical swap is behavior-preserving: - No site had a conditional/early commit or partial-rollback pattern that RunInTx's linear defer-Rollback/Commit couldn't express. - Multi-statement bodies (seeder.go insertBatch's 5 sequential inserts, users/wire.go's content-restriction delete+insert closures, comic PersistDeps builder in build_bookdrop_deps.go) were all correctly moved inside the RunInTx/InTx closure with no reordering. - series/wire.go's deleted buildRunInTx is functionally identical to the new inline `runInTx` using db.InTx — same TxFunc adaptation, no lost deferred cleanup. - Only observable change is error-message text (e.g. "authors merge begin tx: %w" -> generic "begin tx: %w" from db.RunInTx) — not a behavior/contract change, just a cosmetic message rename inherent to sharing the helper. - No query or business logic touched — every diff hunk is boilerplate-only. - No test files changed; existing black-box tests exercise these wire.go closures via their handlers and continue to hold since behavior is unchanged. CI green corroborates. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-10 01:16:30 +00:00

Author

Owner

Copy link

Security Re-Review — nnb9.6 REDO (PR #1406, head 305916fd8)

Scope: re-review of the redone black-box test conversion on internal/cover,
after the original nnb9.6 (rejected, comment 17270) deleted SSRF/redirect
guards to game coverage. Guards now live in internal/netguard (per qapga).

Verified (diff-level, origin/main...origin/bd-bookshelf-nnb9.6):

1. internal/netguard — zero lines touched by this PR (git diff empty for
   that package). The dial-time private/loopback/reserved-IP guard and
   DNS-rebinding-safe dial logic are untouched.
2. internal/cover/download.go — DownloadCoverProduction still binds
   safeTransport() (= netguard.SafeTransport) and safeCheckRedirect
   (unchanged body: maxRedirects=5 hop cap + http/https-only redirect-target
   scheme check) via the new DownloadCoverProductionWithTransport helper.
   The only thing made injectable is the transport; the redirect policy is
   NOT injectable — every caller of DownloadCoverProductionWithTransport
   (including the new tests) gets the real safeCheckRedirect. Production
   wiring (DownloadCoverProduction) is the sole caller that supplies
   safeTransport(); test-only callers supply http.DefaultTransport purely
   to reach httptest.Server on 127.0.0.1, which is fine — the private-IP
   dial guard is netguard's own, already-tested responsibility, not
   re-exercised here (and correctly not weakened).
3. internal/cover/serve.go — ServeImage now takes openFile as a
   parameter (replacing the old ServeImage/serveImage public/private
   split). Production wiring in internal/cover/wire.go binds os.Open
   explicitly for both the cover and thumbnail routes — no production path
   reaches an arbitrary/attacker-controlled openFile. Path construction is
   unchanged: bookID is strconv.ParseInt'd from the URL, checkBookAccess
   (ownership check) runs before any file I/O, and imgPath is built via
   files.CoverPath(dataDir, bookID) / files.ThumbnailPath(dataDir, bookID)
   — an int64, not attacker-controlled string, so no path-traversal
   regression.
4. internal/cover/template_render.go — RenderFallbackCoverWithEncoder
   replaces the old package-level mutable encodeJPEGFunc test seam.
   Production RenderFallbackCover closes over the real jpeg.Encode
   inline; only tests call the encoder-injectable variant. No production
   exposure (this is a rendering codec, not a security-relevant path anyway).
5. internal/cover/export_test.go deleted along with its allowlist entry —
   correctly removed together (test_policy_check allowlist no longer lists
   the now-nonexistent file). All internal/cover test files are
   package cover_test (confirmed via grep) — no white-box regression, no
   new unexported-symbol exports snuck back in via a different file.
6. Logging: logURL() (query-string-stripping sanitizer) is unchanged and
   still wraps every "url" slog attribute in download.go. No new log
   statements were added that could leak tokens/secrets/PII.

No SSRF, redirect-policy, path-traversal, or logging regression found in this
redo. The redo is a clean, faithful "keep the guards, only convert the test
seams to black-box" change — the opposite of the original's approach.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Re-Review — nnb9.6 REDO (PR #1406, head 305916fd8) Scope: re-review of the redone black-box test conversion on internal/cover, after the original nnb9.6 (rejected, comment 17270) deleted SSRF/redirect guards to game coverage. Guards now live in internal/netguard (per qapga). **Verified (diff-level, origin/main...origin/bd-bookshelf-nnb9.6):** 1. `internal/netguard` — zero lines touched by this PR (`git diff` empty for that package). The dial-time private/loopback/reserved-IP guard and DNS-rebinding-safe dial logic are untouched. 2. `internal/cover/download.go` — `DownloadCoverProduction` still binds `safeTransport()` (= `netguard.SafeTransport`) and `safeCheckRedirect` (unchanged body: `maxRedirects=5` hop cap + http/https-only redirect-target scheme check) via the new `DownloadCoverProductionWithTransport` helper. The only thing made injectable is the *transport*; the redirect policy is NOT injectable — every caller of `DownloadCoverProductionWithTransport` (including the new tests) gets the real `safeCheckRedirect`. Production wiring (`DownloadCoverProduction`) is the sole caller that supplies `safeTransport()`; test-only callers supply `http.DefaultTransport` purely to reach `httptest.Server` on 127.0.0.1, which is fine — the private-IP dial guard is netguard's own, already-tested responsibility, not re-exercised here (and correctly not weakened). 3. `internal/cover/serve.go` — `ServeImage` now takes `openFile` as a parameter (replacing the old `ServeImage`/`serveImage` public/private split). Production wiring in `internal/cover/wire.go` binds `os.Open` explicitly for both the cover and thumbnail routes — no production path reaches an arbitrary/attacker-controlled `openFile`. Path construction is unchanged: `bookID` is `strconv.ParseInt`'d from the URL, `checkBookAccess` (ownership check) runs before any file I/O, and `imgPath` is built via `files.CoverPath(dataDir, bookID)` / `files.ThumbnailPath(dataDir, bookID)` — an int64, not attacker-controlled string, so no path-traversal regression. 4. `internal/cover/template_render.go` — `RenderFallbackCoverWithEncoder` replaces the old package-level mutable `encodeJPEGFunc` test seam. Production `RenderFallbackCover` closes over the real `jpeg.Encode` inline; only tests call the encoder-injectable variant. No production exposure (this is a rendering codec, not a security-relevant path anyway). 5. `internal/cover/export_test.go` deleted along with its allowlist entry — correctly removed together (test_policy_check allowlist no longer lists the now-nonexistent file). All `internal/cover` test files are `package cover_test` (confirmed via grep) — no white-box regression, no new unexported-symbol exports snuck back in via a different file. 6. Logging: `logURL()` (query-string-stripping sanitizer) is unchanged and still wraps every `"url"` slog attribute in `download.go`. No new log statements were added that could leak tokens/secrets/PII. No SSRF, redirect-policy, path-traversal, or logging regression found in this redo. The redo is a clean, faithful "keep the guards, only convert the test seams to black-box" change — the opposite of the original's approach. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor added 1 commit 2026-08-10 02:00:09 +00:00

Merge branch 'main' into bd-bookshelf-dhlj4.1 43049c7b9f

zombor merged commit 1049e34d54 into main 2026-08-10 02:11:13 +00:00

zombor referenced this pull request from a commit 2026-08-10 02:11:18 +00:00

Merge pull request 'Hygiene: adopt db.RunInTx/InTx at all hand-rolled BeginTx sites (bookshelf-dhlj4.1)' (#1419) from bd-bookshelf-dhlj4.1 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1419

No description provided.