docs(opds): clarify accepted bcrypt-amplification baseline (bookshelf-2dcn3) #1400

Merged

zombor merged 3 commits from bd-bookshelf-2dcn3 into main 2026-08-09 13:51:01 +00:00

Conversation 2 Commits 3 Files changed 1 +10

zombor commented 2026-08-09 01:57:38 +00:00

Owner

Copy link

Follow-up to #1195 (6tq2k.1). Two review MINORs:

1. opdsClientIP duplicating internal/users clientIP — verified already resolved by bookshelf-9snmz.3.2 (commit 21f133f99), which extracted the shared audit.ExtractIPWithPolicy helper now used by both packages. No code change needed here.
2. Valid-cred concurrent-bcrypt amplification — accepted as-is per the bead. Added a short doc comment on authenticate() in internal/opds/handler.go documenting this as a reviewed, accepted baseline (bounded at burst, requires valid auth, matches /login's reserve/refund pattern) so future reviewers don't re-flag it.

Docs: N/A — internal code-comment clarification only, no user-facing behavior change.

Test plan:

• go build ./... and go vet ./internal/opds/... pass.
• go test ./internal/opds/... passes (comment-only change, no behavior/coverage impact).

Closes bead bookshelf-2dcn3 on merge.

Follow-up to #1195 (6tq2k.1). Two review MINORs: 1. `opdsClientIP` duplicating `internal/users` `clientIP` — verified already resolved by bookshelf-9snmz.3.2 (commit 21f133f99), which extracted the shared `audit.ExtractIPWithPolicy` helper now used by both packages. No code change needed here. 2. Valid-cred concurrent-bcrypt amplification — accepted as-is per the bead. Added a short doc comment on `authenticate()` in `internal/opds/handler.go` documenting this as a reviewed, accepted baseline (bounded at burst, requires valid auth, matches /login's reserve/refund pattern) so future reviewers don't re-flag it. Docs: N/A — internal code-comment clarification only, no user-facing behavior change. Test plan: - `go build ./...` and `go vet ./internal/opds/...` pass. - `go test ./internal/opds/...` passes (comment-only change, no behavior/coverage impact). Closes bead bookshelf-2dcn3 on merge.

zombor added 1 commit 2026-08-09 01:57:40 +00:00

docs(opds): clarify accepted valid-cred bcrypt-amplification baseline (bookshelf-2dcn3) ... bca2b24664

Follow-up to #1195 (6tq2k.1). Two review MINORs tracked here:

1. opdsClientIP/clientIP duplication was already resolved by
   bookshelf-9snmz.3.2, which extracted the shared
   audit.ExtractIPWithPolicy helper used by both internal/opds and
   internal/users. No further action needed.

2. Add a short doc comment on authenticate() documenting that the
   valid-cred concurrent-bcrypt amplification (an attacker with ONE
   valid credential can drive up to ~burst concurrent bcrypt calls
   before refund) is an accepted, reviewed baseline — bounded at
   burst, requires valid auth, matches the /login reserve/refund
   pattern — so future reviewers don't re-flag it.

zombor commented 2026-08-09 02:33:12 +00:00

Author

Owner

Copy link

Security review — PR #1400 (bd-bookshelf-2dcn3)

Diff is doc-comment-only (8 lines added to internal/opds/handler.go, no code changes). Verified the claim the new comment makes against the actual wiring, not just trusting the prose.

Verification performed:

• authenticate() (internal/opds/handler.go:489-546): confirmed the reserve-then-refund sequence is real — loginReserve(ip) is called before verifyCredentials (the bcrypt call), refund only fires on success (line 541-543), and on failure the token stays consumed. This matches the comment's description exactly.
• LoginReserve is wired in internal/app/app.go:478 to loginLimiter.ReserveToken, the same *users.LoginRateLimiter instance used by the standard /login path (LoginAllow: loginLimiter.Allow on the adjacent line) — confirms "same baseline as /login" is literally true, not just similar.
• internal/users/ratelimiter.go: burst = 10 (loginRateLimiterBurst), sustained rate 10/min, per-IP rate.Limiter. ReserveToken uses ReserveN + CancelAt, correctly bounding concurrent in-flight reservations to burst size before any refund can occur — so "up to ~burst concurrent bcrypt verifications per IP" is an accurate upper bound, not an understatement.
• opdsClientIP() (handler.go:648-657) mirrors users.clientIP(): uses only r.RemoteAddr, never trusts X-Forwarded-For, so the per-IP bucket keying can't be trivially bypassed by a spoofed header — consistent with the documented baseline being enforced per real IP, not attacker-controlled.
• No secrets, tokens, or PII in the added comment text.

No BLOCKER/MAJOR/MINOR findings — the comment's claims are accurate and the underlying rate-limit behavior is unchanged (doc-only diff).

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security review — PR #1400 (bd-bookshelf-2dcn3) Diff is doc-comment-only (8 lines added to `internal/opds/handler.go`, no code changes). Verified the claim the new comment makes against the actual wiring, not just trusting the prose. **Verification performed:** - `authenticate()` (internal/opds/handler.go:489-546): confirmed the reserve-then-refund sequence is real — `loginReserve(ip)` is called *before* `verifyCredentials` (the bcrypt call), refund only fires on success (line 541-543), and on failure the token stays consumed. This matches the comment's description exactly. - `LoginReserve` is wired in `internal/app/app.go:478` to `loginLimiter.ReserveToken`, the **same** `*users.LoginRateLimiter` instance used by the standard `/login` path (`LoginAllow: loginLimiter.Allow` on the adjacent line) — confirms "same baseline as /login" is literally true, not just similar. - `internal/users/ratelimiter.go`: burst = 10 (`loginRateLimiterBurst`), sustained rate 10/min, per-IP `rate.Limiter`. `ReserveToken` uses `ReserveN` + `CancelAt`, correctly bounding concurrent in-flight reservations to burst size before any refund can occur — so "up to ~burst concurrent bcrypt verifications per IP" is an accurate upper bound, not an understatement. - `opdsClientIP()` (handler.go:648-657) mirrors `users.clientIP()`: uses only `r.RemoteAddr`, never trusts `X-Forwarded-For`, so the per-IP bucket keying can't be trivially bypassed by a spoofed header — consistent with the documented baseline being enforced per real IP, not attacker-controlled. - No secrets, tokens, or PII in the added comment text. No BLOCKER/MAJOR/MINOR findings — the comment's claims are accurate and the underlying rate-limit behavior is unchanged (doc-only diff). REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-09 02:33:12 +00:00

Author

Owner

Copy link

Diff-review of PR #1400 (bd-bookshelf-2dcn3) — comment-only follow-up to bookshelf-6tq2k.1.

Phase 1 — spec compliance

MINOR #2 claim verified: opdsClientIP no longer exists anywhere on origin/main as a function; internal/opds/handler.go:559 already calls the shared audit.ExtractIPWithPolicy(r, false), same helper internal/users/* uses (device_handler.go, handler.go, oidc_handler.go). Only remaining references to the old name are in handler_test.go comments (stale text, not code) — no action needed there, consistent with the bead's claim that bookshelf-9snmz.3.2 already consolidated this.

MINOR #1 (bcrypt-amplification doc comment): added, matches the bead ask — an 8-line addition to the authenticate() doc comment, no behavior change. Confirmed genuinely comment-only via git diff origin/main...origin/bd-bookshelf-2dcn3 (single hunk, all + lines, no code).

Phase 2 — accuracy of the new comment

[MINOR] internal/opds/handler.go:527-533 — "matches the same reserve/refund baseline used by the standard /login rate limiter" is imprecise.
Checked internal/app/app.go:378-389 on main: LoginReserve: loginLimiter.ReserveToken (OPDS, reserve-then-refund) vs LoginAllow: loginLimiter.Allow (web /login, consume-before-bcrypt with no refund — see the adjacent app.go comment: "LoginAllow is the consuming check used by the web /login handler (consume before bcrypt, no refund)"). Both share the same limiter instance and both cap concurrent bcrypt calls at burst size (which is the actual point being defended), but /login does not "refund" — so calling it the "same reserve/refund baseline" slightly mischaracterizes /login's mechanism to a future reader. Suggest: "matches the same pre-bcrypt consume-before-verify baseline used by the standard /login rate limiter (which consumes but does not refund)." Non-blocking — doc-only, doesn't change any assessment of risk.

No other findings. Change is exactly what the bead describes: comment-only, no logic/behavior touched.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

Diff-review of PR #1400 (bd-bookshelf-2dcn3) — comment-only follow-up to bookshelf-6tq2k.1. **Phase 1 — spec compliance** MINOR #2 claim verified: `opdsClientIP` no longer exists anywhere on `origin/main` as a function; `internal/opds/handler.go:559` already calls the shared `audit.ExtractIPWithPolicy(r, false)`, same helper `internal/users/*` uses (`device_handler.go`, `handler.go`, `oidc_handler.go`). Only remaining references to the old name are in `handler_test.go` comments (stale text, not code) — no action needed there, consistent with the bead's claim that bookshelf-9snmz.3.2 already consolidated this. MINOR #1 (bcrypt-amplification doc comment): added, matches the bead ask — an 8-line addition to the `authenticate()` doc comment, no behavior change. Confirmed genuinely comment-only via `git diff origin/main...origin/bd-bookshelf-2dcn3` (single hunk, all `+` lines, no code). **Phase 2 — accuracy of the new comment** [MINOR] internal/opds/handler.go:527-533 — "matches the same reserve/refund baseline used by the standard /login rate limiter" is imprecise. Checked `internal/app/app.go:378-389` on main: `LoginReserve: loginLimiter.ReserveToken` (OPDS, reserve-then-refund) vs `LoginAllow: loginLimiter.Allow` (web `/login`, consume-before-bcrypt with **no refund** — see the adjacent app.go comment: "LoginAllow is the consuming check used by the web /login handler (consume before bcrypt, no refund)"). Both share the same limiter instance and both cap *concurrent* bcrypt calls at burst size (which is the actual point being defended), but /login does not "refund" — so calling it the "same reserve/refund baseline" slightly mischaracterizes /login's mechanism to a future reader. Suggest: "matches the same pre-bcrypt consume-before-verify baseline used by the standard /login rate limiter (which consumes but does not refund)." Non-blocking — doc-only, doesn't change any assessment of risk. No other findings. Change is exactly what the bead describes: comment-only, no logic/behavior touched. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

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

docs(opds): clarify /login limiter is consume-no-refund (review minor, bookshelf-2dcn3) 1741c759be

zombor added 1 commit 2026-08-09 13:42:35 +00:00

Merge branch 'main' into bd-bookshelf-2dcn3 da4ec0258a

zombor merged commit a85bf021db into main 2026-08-09 13:51:01 +00:00

zombor referenced this pull request from a commit 2026-08-09 13:51:09 +00:00

Merge pull request 'docs(opds): clarify accepted bcrypt-amplification baseline (bookshelf-2dcn3)' (#1400) from bd-bookshelf-2dcn3 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!1400

No description provided.