fix(authors): author photo URL fetch logs correct author_id (bookshelf-0u50.2) #1386

Merged

zombor merged 2 commits from bd-bookshelf-0u50.2 into main 2026-08-07 14:53:32 +00:00

Conversation 4 Commits 2 Files changed 10 +85 -56

zombor commented 2026-08-07 13:30:38 +00:00

Owner

Copy link

Fixes a minor observability bug from PR #1350 round-2 review: wirePhotoUpload's
fetchURL closure hardcoded 0 as the ID argument to the shared
cover.DownloadCoverProduction downloader, so structured logs for author photo
URL fetches (e.g. POST /authors/{id}/photo with a {"url":...} body) showed
book_id=0 instead of the author being updated.

fetchURL's signature is now func(context.Context, int64, string) ([]byte, error)
and the author ID is threaded through PhotoUploadHandler -> readPhotoFromURL
-> the download closure, so the shared downloader's book_id log key now carries
the correct author ID for this call path.

Test plan

• New unit test PhotoUploadHandler > URL variant > calls fetchURL with the author ID (not 0) for log attribution in internal/authors/manage_handler_test.go.
• go test ./internal/authors/... — pass
• make coverage — 100% gate holds
• golangci-lint run ./internal/authors/... — 0 issues

Docs: N/A because internal observability fix, no user-facing surface.

Closes bead bookshelf-0u50.2 on merge.

Fixes a minor observability bug from PR #1350 round-2 review: `wirePhotoUpload`'s `fetchURL` closure hardcoded `0` as the ID argument to the shared `cover.DownloadCoverProduction` downloader, so structured logs for author photo URL fetches (e.g. `POST /authors/{id}/photo` with a `{"url":...}` body) showed `book_id=0` instead of the author being updated. `fetchURL`'s signature is now `func(context.Context, int64, string) ([]byte, error)` and the author ID is threaded through `PhotoUploadHandler` -> `readPhotoFromURL` -> the download closure, so the shared downloader's `book_id` log key now carries the correct author ID for this call path. ## Test plan - New unit test `PhotoUploadHandler > URL variant > calls fetchURL with the author ID (not 0) for log attribution` in `internal/authors/manage_handler_test.go`. - `go test ./internal/authors/...` — pass - `make coverage` — 100% gate holds - `golangci-lint run ./internal/authors/...` — 0 issues Docs: N/A because internal observability fix, no user-facing surface. Closes bead bookshelf-0u50.2 on merge.

zombor added 1 commit 2026-08-07 13:30:39 +00:00

fix(authors): thread author ID through photo URL fetch for correct log attribution ... 52609be33c

Previously wirePhotoUpload's fetchURL closure hardcoded 0 as the ID arg
to the shared cover.DownloadCoverProduction downloader, so structured
logs for author photo URL fetches showed book_id=0 instead of the
author being updated. fetchURL now accepts the author ID and threads
it through from PhotoUploadHandler -> readPhotoFromURL -> the download
closure, so log lines attribute the fetch to the correct author.

Bead: bookshelf-0u50.2

zombor commented 2026-08-07 13:37:06 +00:00

Author

Owner

Copy link

[BLOCKER] none

[MAJOR] none

[MINOR] none

Reviewed the diff only (internal/authors/wire.go, manage_handler.go, manage_handler_test.go). This threads the already-validated authorID (parsed via strconv.ParseInt(r.PathValue("id")) and used earlier in the same handler for the ownership/lock check) through the fetchURL closure so it reaches cover.DownloadCoverProduction as the log-attribution ID instead of a hardcoded 0.

Checked:

• SSRF guard: cover.DownloadCoverProduction (internal/cover/download.go) — safeTransport/safeCheckRedirect, scheme allowlist, size cap, content-type + magic-byte validation — is untouched by this diff. Only the ID argument value changes; the guard chain is identical.
• authorID provenance: comes from the URL path param, already parsed/validated and used for the getAuthor/PhotoLocked authorization check before reaching readPhotoFromURL. No new authz implication — it is a log field only, per the code comment.
• Log injection: authorID is int64, not an attacker-controlled string — no injection surface.
• No new logging of secrets/tokens/PII; the URL itself is not passed through this change (fetchURL still receives req.URL unchanged), and no new log statement was added in the diff.

Proportionate, low-risk change. No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

[BLOCKER] none [MAJOR] none [MINOR] none Reviewed the diff only (internal/authors/wire.go, manage_handler.go, manage_handler_test.go). This threads the already-validated `authorID` (parsed via `strconv.ParseInt(r.PathValue("id"))` and used earlier in the same handler for the ownership/lock check) through the `fetchURL` closure so it reaches `cover.DownloadCoverProduction` as the log-attribution ID instead of a hardcoded `0`. Checked: - SSRF guard: `cover.DownloadCoverProduction` (internal/cover/download.go) — safeTransport/safeCheckRedirect, scheme allowlist, size cap, content-type + magic-byte validation — is untouched by this diff. Only the ID argument value changes; the guard chain is identical. - authorID provenance: comes from the URL path param, already parsed/validated and used for the `getAuthor`/`PhotoLocked` authorization check before reaching `readPhotoFromURL`. No new authz implication — it is a log field only, per the code comment. - Log injection: authorID is `int64`, not an attacker-controlled string — no injection surface. - No new logging of secrets/tokens/PII; the URL itself is not passed through this change (fetchURL still receives `req.URL` unchanged), and no new log statement was added in the diff. Proportionate, low-risk change. No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-07 13:38:52 +00:00

Author

Owner

Copy link

Code Review: bookshelf-0u50.2 (PR #1386)

[MAJOR] internal/authors/wire.go:107-111 — log field is still literally book_id for an author-photo fetch
cover.DownloadCoverProduction's downloader (internal/cover/download.go) hardcodes the slog key "book_id" (and the wrapped error text book_id=%d) for its second int64 parameter, regardless of caller. This PR threads authorID into that param instead of a hardcoded 0, which is a real improvement (no more book_id=0 for every author photo fetch), but the resulting log/error output will now read e.g. book_id=7 when author 7's photo fetch fails — still a mislabeled field, just non-zero. Anyone triaging logs/dashboards by book_id will now see spurious-looking book IDs that are actually author IDs, risking false correlation with real book-download failures. The PR's own doc comment at wire.go:105-108 acknowledges this ("dl's second arg is logged under the key book_id... but here we thread the authorID through") — so it's a known, not hidden, tradeoff, but it only half-closes the observability bug the bead describes (book_id=0 for every fetch → fixed to book_id=, still the wrong key). Suggested fix (small, in-scope): bind an author-context logger before constructing dl, e.g. dl := cover.DownloadCoverProduction(30*time.Second, d.Logger.With("entity", "author")), so log lines carry both the (still misleadingly-named) book_id value and an entity=author disambiguator — or file a fast follow-up bead to make the shared downloader take a generic entityKind/entityID pair instead of a hardcoded book_id name. Note the same shared downloader is also reused unmodified a few lines below in wireAudnexus's downloadImage for Audnexus author-photo fetches (pre-existing, not introduced by this diff, but the identical defect — worth folding into the same follow-up).

[MINOR] internal/authors/manage_handler_test.go:426,447-450,551 — fetchURLCalled is a misleading name for an int64
The variable stores the authorID argument the fake fetchURL closure was invoked with, not a boolean "was it called" flag; the ...Called suffix reads as a call-count/bool convention. Suggest fetchURLAuthorID (or capturedAuthorID) to match what it actually holds. Pure readability, no behavioral impact.

Everything else checks out: authorID is correctly threaded from readPhotoFromURL all the way through fetchURL / wirePhotoUpload / cover.DownloadCoverProduction's existing bookID parameter, with no behavior change to the actual download (same timeout, same SSRF guard, same transport); the curried-functional-argument convention is preserved (no interfaces introduced); the test file stays black-box (package authors_test); the new It block asserts exactly one thing (fetchURLCalled == 1) per the one-Expect-per-It rule; and no coverage exclusion was added.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Code Review: bookshelf-0u50.2 (PR #1386) [MAJOR] internal/authors/wire.go:107-111 — log field is still literally `book_id` for an author-photo fetch `cover.DownloadCoverProduction`'s downloader (internal/cover/download.go) hardcodes the slog key `"book_id"` (and the wrapped error text `book_id=%d`) for its second int64 parameter, regardless of caller. This PR threads `authorID` into that param instead of a hardcoded `0`, which is a real improvement (no more `book_id=0` for every author photo fetch), but the resulting log/error output will now read e.g. `book_id=7` when author 7's photo fetch fails — still a mislabeled field, just non-zero. Anyone triaging logs/dashboards by `book_id` will now see spurious-looking book IDs that are actually author IDs, risking false correlation with real book-download failures. The PR's own doc comment at wire.go:105-108 acknowledges this ("dl's second arg is logged under the key `book_id`... but here we thread the authorID through") — so it's a known, not hidden, tradeoff, but it only half-closes the observability bug the bead describes (book_id=0 for every fetch → fixed to book_id=<real-author-id>, still the wrong key). Suggested fix (small, in-scope): bind an author-context logger before constructing `dl`, e.g. `dl := cover.DownloadCoverProduction(30*time.Second, d.Logger.With("entity", "author"))`, so log lines carry both the (still misleadingly-named) `book_id` value and an `entity=author` disambiguator — or file a fast follow-up bead to make the shared downloader take a generic `entityKind`/`entityID` pair instead of a hardcoded `book_id` name. Note the same shared downloader is also reused unmodified a few lines below in `wireAudnexus`'s `downloadImage` for Audnexus author-photo fetches (pre-existing, not introduced by this diff, but the identical defect — worth folding into the same follow-up). [MINOR] internal/authors/manage_handler_test.go:426,447-450,551 — `fetchURLCalled` is a misleading name for an `int64` The variable stores the `authorID` argument the fake `fetchURL` closure was invoked with, not a boolean "was it called" flag; the `...Called` suffix reads as a call-count/bool convention. Suggest `fetchURLAuthorID` (or `capturedAuthorID`) to match what it actually holds. Pure readability, no behavioral impact. Everything else checks out: `authorID` is correctly threaded from `readPhotoFromURL` all the way through `fetchURL` / `wirePhotoUpload` / `cover.DownloadCoverProduction`'s existing `bookID` parameter, with no behavior change to the actual download (same timeout, same SSRF guard, same transport); the curried-functional-argument convention is preserved (no interfaces introduced); the test file stays black-box (`package authors_test`); the new `It` block asserts exactly one thing (`fetchURLCalled == 1`) per the one-Expect-per-It rule; and no coverage exclusion was added. REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor referenced this pull request from a commit 2026-08-07 14:10:36 +00:00

fix(review): parameterize cover downloader entity label + rename test var

zombor force-pushed bd-bookshelf-0u50.2 from 52609be33c to 3338c7f902 2026-08-07 14:10:37 +00:00 Compare

zombor commented 2026-08-07 14:18:16 +00:00

Author

Owner

Copy link

Security re-review of PR #1386 (bookshelf-0u50.2) — fix that parameterized the SSRF-guarded cover downloader's log label (entityKey/entityID).

Verified via git diff origin/main...origin/bd-bookshelf-0u50.2:

• The SSRF guard chain in internal/cover/download.go (safeTransport/safeDialContext private-IP block, safeCheckRedirect bounded redirects, scheme allowlist, LimitReader body-size cap, Content-Type allowlist, magic-byte validation) is byte-for-byte unchanged in control flow. Every diff hunk inside the returned closure only swaps the hardcoded "book_id" slog key/error label for the entityKey/entityID params — no conditional, ordering, or validation logic changed.
• entityKey is always a hardcoded string literal at every call site ("book_id" in app.go, build_extended_deps.go, build_llm_deps.go, bookdrop/wire.go, cover/wire.go, cover/download_test.go; "author_id" in authors/wire.go) — never derived from request/user input. No log-injection or format-string risk.
• entityID (formerly bookID) is int64 everywhere. internal/authors/manage_handler.go's readPhotoFromURL now threads the real authorID into fetchURL instead of a hardcoded 0 — improves log attribution only, does not touch the URL being fetched or any validation step.
• logURL(rawURL) (query/fragment-stripped) is still called at every log site — not dropped by the refactor.
• No new secrets/PII logged; only numeric IDs and a static string label are added to log/error output.

No blockers, majors, or minors found — this is a pure logging-label parameterization; the SSRF guard chain is intact.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Security re-review of PR #1386 (bookshelf-0u50.2) — fix that parameterized the SSRF-guarded cover downloader's log label (entityKey/entityID). Verified via `git diff origin/main...origin/bd-bookshelf-0u50.2`: - The SSRF guard chain in `internal/cover/download.go` (safeTransport/safeDialContext private-IP block, safeCheckRedirect bounded redirects, scheme allowlist, LimitReader body-size cap, Content-Type allowlist, magic-byte validation) is byte-for-byte unchanged in control flow. Every diff hunk inside the returned closure only swaps the hardcoded `"book_id"` slog key/error label for the `entityKey`/`entityID` params — no conditional, ordering, or validation logic changed. - `entityKey` is always a hardcoded string literal at every call site (`"book_id"` in app.go, build_extended_deps.go, build_llm_deps.go, bookdrop/wire.go, cover/wire.go, cover/download_test.go; `"author_id"` in authors/wire.go) — never derived from request/user input. No log-injection or format-string risk. - `entityID` (formerly `bookID`) is `int64` everywhere. `internal/authors/manage_handler.go`'s `readPhotoFromURL` now threads the real `authorID` into `fetchURL` instead of a hardcoded `0` — improves log attribution only, does not touch the URL being fetched or any validation step. - `logURL(rawURL)` (query/fragment-stripped) is still called at every log site — not dropped by the refactor. - No new secrets/PII logged; only numeric IDs and a static string label are added to log/error output. No blockers, majors, or minors found — this is a pure logging-label parameterization; the SSRF guard chain is intact. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-07 14:19:23 +00:00

Author

Owner

Copy link

Re-review of the in-PR fix for the earlier code-review MAJOR (hardcoded "book_id" log key in the shared cover downloader), PR #1386 (bookshelf-0u50.2).

Verified against the current diff (origin/main...origin/bd-bookshelf-0u50.2):

• internal/cover/download.go: DownloadCoverProduction/DownloadCover/DownloadCoverWithOptions now take a curried entityKey string; the returned closure's second param is entityID. Every logger.Info/logger.Error call and every fmt.Errorf label inside the downloader uses entityKey/entityID - no site still hardcodes "book_id".
• All book-cover callers (internal/app/app.go:440, internal/app/build_extended_deps.go:458, internal/app/build_llm_deps.go:69, internal/bookdrop/wire.go:99, internal/cover/wire.go:200) explicitly pass "book_id" - book-path log/error output is byte-identical to before.
• Both author callers (internal/authors/wire.go:108 and :139) pass "author_id" - author-photo fetches now correctly attribute logs to the author instead of book_id=.
• grep across the branch confirms no missed call site; the 3-arg signature change compiles everywhere.
• Curried-functional-arg convention preserved: entityKey bound once at wiring, request param entityID. Test var rename fetchURLCalled -> fetchURLAuthorID is accurate and the new assertion in internal/authors/manage_handler_test.go follows the project's one-Expect-per-It pattern.
• No scope creep - SSRF guard (safeTransport/safeCheckRedirect/knownImageMagic/content-type allowlist/size cap) is untouched; diff is exclusively the entityKey/entityID rename.

No blockers/majors/minors found. The original MAJOR is fully resolved.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Re-review of the in-PR fix for the earlier code-review MAJOR (hardcoded "book_id" log key in the shared cover downloader), PR #1386 (bookshelf-0u50.2). Verified against the current diff (origin/main...origin/bd-bookshelf-0u50.2): - internal/cover/download.go: DownloadCoverProduction/DownloadCover/DownloadCoverWithOptions now take a curried entityKey string; the returned closure's second param is entityID. Every logger.Info/logger.Error call and every fmt.Errorf label inside the downloader uses entityKey/entityID - no site still hardcodes "book_id". - All book-cover callers (internal/app/app.go:440, internal/app/build_extended_deps.go:458, internal/app/build_llm_deps.go:69, internal/bookdrop/wire.go:99, internal/cover/wire.go:200) explicitly pass "book_id" - book-path log/error output is byte-identical to before. - Both author callers (internal/authors/wire.go:108 and :139) pass "author_id" - author-photo fetches now correctly attribute logs to the author instead of book_id=<authorID>. - grep across the branch confirms no missed call site; the 3-arg signature change compiles everywhere. - Curried-functional-arg convention preserved: entityKey bound once at wiring, request param entityID. Test var rename fetchURLCalled -> fetchURLAuthorID is accurate and the new assertion in internal/authors/manage_handler_test.go follows the project's one-Expect-per-It pattern. - No scope creep - SSRF guard (safeTransport/safeCheckRedirect/knownImageMagic/content-type allowlist/size cap) is untouched; diff is exclusively the entityKey/entityID rename. No blockers/majors/minors found. The original MAJOR is fully resolved. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor referenced this pull request from a commit 2026-08-07 14:46:43 +00:00

fix(review): parameterize cover downloader entity label + rename test var

zombor force-pushed bd-bookshelf-0u50.2 from 3338c7f902 to b9175b7fbd 2026-08-07 14:46:43 +00:00 Compare

zombor merged commit a6141fce44 into main 2026-08-07 14:53:32 +00:00

zombor referenced this pull request from a commit 2026-08-07 14:53:36 +00:00

Merge pull request 'fix(authors): author photo URL fetch logs correct author_id (bookshelf-0u50.2)' (#1386) from bd-bookshelf-0u50.2 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!1386

No description provided.