feat(opds): root nav + acquisition feed with Basic Auth (bookshelf-6tq2k.1) #1195
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-6tq2k.1"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
GET /opdsroot navigation feed (OPDS 1.2 Atom) listing the All Books acquisition feedGET /opds/bookscursor-paginated acquisition feed with per-book cover, thumbnail, and download links scoped to the authenticated user's library (fail-closed)users.Loginbcrypt validation;/opdsand/opds/*are exempt from the cookie-based auth middlewareTest plan
go test ./internal/opds/...— 49 specs, all greenmake test— all packages passmake coverage— zero uncovered statement blocksgolangci-lint run ./internal/opds/...— 0 issuesFiles changed
internal/opds/handler.go— RootHandler, BooksHandler, encodeAcqFeed, XML helpersinternal/opds/store.go— ListCategoriesForBooks, ListFilesForBooks (batch queries)internal/opds/routes.go— RegisterRoutesinternal/opds/wire.go— Wire (wires deps, calls RegisterRoutes)internal/opds/handler_test.go— black-box handler tests (49 specs)internal/opds/store_test.go— black-box store testsinternal/opds/opds_suite_test.go— Ginkgo suite bootstrapinternal/users/middleware.go— isExempt: add /opds and /opds/* exemptionsinternal/app/app.go— register opds.Wire in the modules sliceCloses bead bookshelf-6tq2k.1 on merge.
Security Review — PR #1195 (OPDS Basic-Auth catalog)
Scope:
internal/users/middleware.goexemption,internal/opds/*,internal/app/app.go. New unauthenticated-by-cookie surface (GET /opds,GET /opds/books) authenticated via HTTP Basic Auth.Positives confirmed: cookie-middleware exemption is exact-match
/opds+ prefix/opds/only (line-verified) —/opds-adminand similar do NOT match, no prefix-abuse hole. Both handlers callauthenticate()→challengeBasicAuth()(401 +WWW-Authenticate: Basic realm="Pergamum") on missing/invalid creds, never a feed.users.Loginequalizes bcrypt timing on unknown-user and returns a singleErrInvalidCredentials(no user-enumeration oracle); no password is logged. userID comes only from the Basic-Auth-resolvedresult.User.ID, never a query param. Library scoping is fail-closed (getUserLibraryIDserror → returned, empty → empty feed). XML is emitted viaencoding/xml(chardata + attributes auto-escaped) — no XML-injection break-out. Cover/download hrefs are built from int64 IDs only — no SSRF / path traversal. Acquisition/cover links point at the existing ownership-checked/books/{id}/file/{fileID}(guarded byg.Download) and/data/images/...handlers — fail-closed.[MAJOR] internal/opds/handler.go:178 — OPDS feed bypasses per-user content restrictions
BooksHandler builds the filter with
ContentRestrictions: books.ContentRestrictions{}(empty), whereas the web books list (internal/books/handler.go:301) loads the user's restrictions viagetContentRestrictions(ctx, userID)and applies them to the query. A content-restricted account (e.g. a child profile) browsing/downloading via OPDS therefore sees books the web UI hides for that same user — a per-user scoping gap (multi-user HARD RULE). Fix: wirebooks.GetUserContentRestrictions(d.Q...)intoopds.Wire, load restrictions for the authenticated userID, and setfilter.ContentRestrictionsbefore callinglist.[MAJOR] internal/opds/handler.go:321 — OPDS Basic Auth bypasses the login rate limiter (unthrottled credential-stuffing surface)
POST /loginis protected per-IP byusers.LoginRateLimiter(10/min) and recordsActionLoginFailed/ActionLoginRateLimitedaudit events (internal/users/handler.go:135-143). The OPDSauthenticate()path callsusers.Logindirectly with NONE of this: no per-IP throttle and no audit record on failed OPDS auth./opds*is thus an unthrottled, audit-invisible brute-force / credential-stuffing endpoint that sidesteps the app's existing protection. Fix: gate the OPDS authenticate path through the sameLoginRateLimiter(LoginAllow(clientIP)) → 401/429 on limit, and emit an audit record on failed OPDS auth.[MAJOR] internal/opds/handler.go:339 — every OPDS request mints and persists a throwaway refresh token
authenticate()→users.Loginruns the full login side-effect on every request: it INSERTs a newrefresh_tokenrow (createRefreshToken) and issues a JWT access token, then the OPDS handler discards both and uses onlyresult.User.ID. OPDS clients re-send Basic Auth on every browse/poll, so this grows therefresh_tokentable unboundedly with never-used, live (7-day TTL) credentials — DB bloat plus a large standing pile of valid refresh tokens exposed if the DB is compromised. Fix: authenticate via a credential-verification-only path (getUser +checkPassword, preserving the dummy-hash timing equalization) that does NOT create refresh/access tokens.[MINOR] internal/opds/handler.go:391 — acquisition/cover links are unreachable by OPDS (Basic-Auth) clients
The feed advertises
/books/{id}/file/{fileID}and/data/images/{id}/cover.jpg, but those routes are cookie-protected and not Basic-Auth aware and are not in the/opdsexemption, so an OPDS client sending Basic Auth gets 401/redirect on download. This is fail-closed (good security) but the download feature is effectively non-functional for real OPDS clients. Not a vulnerability; flagging so it is a deliberate follow-up rather than a silent gap.REVIEW VERDICT: 0 blocker, 3 major, 1 minor
- Wire books.GetUserContentRestrictions into BooksHandler; apply to filter so the catalog is fail-closed to the authenticated user's content policy. - Add authenticate() helper that gates on LoginRateLimiter.Allow(ip) and records ActionLoginRateLimited / ActionLoginFailed via audit.Record on failures; used by all five OPDS handlers. - Add users.VerifyCredentials: bcrypt-verify-only path (no token minting, no refresh token persisted); timing equalised via bcrypt dummy hash for unknown-user paths. OPDS auth is now stateless and does not create throwaway tokens on every request. - Add OPDS-specific Basic-Auth routes for file download, cover, and thumbnail (/opds/books/{id}/file/{fileID}, /opds/books/{id}/cover, /opds/books/{id}/thumbnail) with ownership checks via CheckBookAccess. - Refactor encodeAcqFeed / encodeTextElement to void (xml.Encoder accumulates errors; enc.Flush() surfaces them); add failWriter test to cover the writeXMLFeed writeFn error branch. - Add ./internal/opds/... to UNIT_PKGS (Makefile) and coverage gate (scripts/check-coverage.sh); achieve 100% statement coverage on all opds source files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Security RE-review — PR #1195 (OPDS Basic-Auth catalog)
Re-verification of the four prior findings + hunt for regressions introduced by the fixes.
Prior findings — status
BooksHandlerloadsgetContentRestrictions(user.ID)andgetUserLibraryIDs(user.ID)per authenticated user, puts both onbooks.Filter, and passes it tolist.ContentRestrictionsflowsFilter → ListBooksFilteredParams (service.go:120) → buildListBooksFilteredQuery, and library scoping is fail-closed (filter_predicates.go:113emits1=0for a non-nil empty library set). Both restriction and library-load errors return up the stack (no feed on error). Confirmed closed.authenticate(handler.go) consultsloginAllow(opdsClientIP(r))beforeverifyCredentials, andaudit.Records bothActionLoginRateLimitedandActionLoginFailedwith a sanitized username.opdsClientIPderives fromRemoteAddronly (neverX-Forwarded-For) — identical to the web limiter'susers.clientIP(ratelimiter.go:145), so it is not spoofable. All failure reasons return a uniform 401 +WWW-Authenticate: Basicwith a generic JSON body (no feed, no enumeration oracle). Bypass closed — but see the NEW MAJOR below re: over-application.users.VerifyCredentials(service.go) runs bcrypt viacheckPasswordand returns only aUser— no refresh_token/JWT/session side-effect. Unknown-user path runscheckPassword(dummyHash, ...)for timing equalization and returns the single opaqueErrInvalidCredentials, same as wrong-password. SharedLoginis untouched (diff is pure addition), so the web token-minting path is unchanged. Confirmed closed./opds/books/{id}/file/{fileID},/cover,/thumbnail) each callauthenticatethencheckBookAccess(user.ID, bookID)(books.CheckBookAccess, service.go:689 — library membership + content restriction, fail-closed, uniformErrNotFoundon miss). Download builds the path from a DB-sourcedfile_sub_pathwith afilepath.Clean+prefix traversal guard; cover/thumbnail paths derive only from the numericbookID(no user-controlled path component). Confirmed closed.New finding
[MAJOR] internal/opds/handler.go:390 (opdsClientIP / authenticate) + internal/opds/wire.go:38 — Shared brute-force login limiter is consumed by every OPDS asset request
authenticatecallsloginAllow(...)on EVERY OPDS request, andloginAllowis the sameloginLimiter.Allowinstance the webPOST /loginuses (app.go:303/469 →Deps.LoginAllow), configured at 10 req/min, burst 10 (ratelimiter.go:11-16). Because OPDS re-authenticates on every request, a client rendering one acquisition-feed page fires the feed request plus up to N cover + N thumbnail fetches (buildEntry adds both links whenHasCover) — 100+ Basic-Auth'd requests for a 50-book page — each consuming a token from a 10-token bucket. Result: covers/thumbnails 401 almost immediately and downloads get throttled, so the feature breaks under normal-size libraries. Worse, since onlyRemoteAddris used, behind a reverse proxy every client shares one bucket, and high-volume OPDS asset traffic will throttle/lock out the interactive web/loginfor all users on that proxy IP — a cross-surface availability regression on the auth surface.Fix: do not run authenticated OPDS asset traffic through the 10/min brute-force bucket. Either gate only the credential-verification attempts (e.g. count only failed
VerifyCredentials), use a dedicated OPDS limiter sized for poll/asset traffic, or limit only the feed endpoints — keep the per-request Basic-Auth verify but stop draining the shared login bucket on every successful asset fetch.Notes (no finding)
VerifyCredentialslogs onlyusernameat Info (matches existingLoginconvention); audit descriptions run throughaudit.SanitizeDescription.Basicheader return before the limiter, but they also never reach credential verification — no brute-force benefit.REVIEW VERDICT: 0 blocker, 1 major, 0 minor
Security Fix Re-Review (bd-bookshelf-6tq2k.1)
✅ All fixes verified and correct.
Fix Verification
1. Content restrictions [VERIFIED]
BooksHandlerloads user's content restrictions viagetContentRestrictions(r.Context(), user.ID)books.Filter{UserLibraryIDs: libraryIDs, ContentRestrictions: contentRestr}list()call, so restricted books are excluded2. Rate limit + audit [VERIFIED]
authenticate()checksloginAllow(opdsClientIP(r))BEFORE credential verification (correct gate order)audit.Record(r.Context(), audit.ActionLoginRateLimited, ...)audit.Record(r.Context(), audit.ActionLoginFailed, ...)opdsClientIP(r)mirrorsclientIP(r)in users/ratelimiter.gochallengeBasicAuth()3. Verify-only auth [VERIFIED]
VerifyCredentialsfunction in internal/users/service.go:215–251getUserandlogger, returns bareUser(no tokens)Login(dummyHash used in both paths)ErrInvalidCredentialsfor unknown users and wrong passwordsgetUsercalled exactly once)4. OPDS download/cover routes [VERIFIED]
DownloadHandlerenforces ownership viacheckBookAccess(r.Context(), user.ID, bookID)(line 329)CoverHandlerenforces ownership viacheckBookAccess(r.Context(), user.ID, bookID)(line 387)CheckBookAccessis fail-closed: 404 on no library IDs, non-existent book, book not owned, restrictions failed to loadErrNotFoundfor restricted books (prevents existence oracle)DownloadHandler5. Coverage inclusion [VERIFIED]
./internal/opds/...added toUNIT_PKGS(INCLUSION, not exclusion).golangci.ymlexclusionsCode Quality
✅ Black-box tests (package opds_test) | One-Expect-per-It | No linter exclusions added
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Security re-review — rate-limiter reorder (commit
4a6d121f)Scope:
internal/opds/handler.goauthenticate()ordering change (limiter now consulted only on failed auth) +handler_test.go. Verified against the injectedusers.VerifyCredentialsand the app's/loginlimiter usage.What holds up (no finding):
loginLimiter.Allow(AllowN(now,1), burst 10, 1 token/6s) is consumed on every failed credential attempt (handler.go:485); once the per-IP bucket empties, further failed attempts get401+ActionLoginRateLimitedaudit. No path grants unlimited credential guesses — success is the only path that skips the limiter, and success requires valid creds.users.VerifyCredentialsstill runs the dummy-hash bcrypt for unknown users (service.go:234); unknown-user and wrong-password are indistinguishable by time, and both the rate-limited and verify-failed branches return the identicalchallengeBasicAuth401 — no status oracle.User; valid creds return the authenticated user; the reorder cannot bypass either the credential check or the throttle./loginis gone.opdsClientIPusesRemoteAddronly, never forwarded headers (handler.go:565). Both audit actions preserved; username isSanitizeDescription-wrapped; password never logged.[MAJOR] internal/opds/handler.go:482 — bcrypt now runs before the rate limiter (unauthenticated CPU-amplification DoS)
The reorder moved
verifyCredentials(bcrypt cost 12, ~hundreds of ms CPU/call, incl. the dummy-hash path for unknown users) to run BEFORE the limiter is consulted. The limiter is now reached only after the expensive hash, so an unauthenticated attacker sending unboundedAuthorization: Basic <garbage>requests to any/opds*route forces a full bcrypt on every request with no per-IP ceiling — a small-request to large-server-cost amplification that a few concurrent connections can use to saturate CPU. This also diverges from the canonical/loginhandler (internal/users/handler.go:136), which checksLoginAllowFIRST, beforeLogin/bcrypt, precisely to shield the hash. The token-drain fix itself is correct, but it removed the pre-bcrypt shield rather than making token consumption conditional.Fix: keep a cheap pre-bcrypt gate while consuming a token only on failure — two-phase: reject when the bucket is already empty before calling
verifyCredentials(a non-consuming peek), run bcrypt only for non-throttled IPs, then consume a token on a failed credential check. That preserves both properties (valid traffic doesn't drain the bucket AND throttled IPs are rejected before the expensive hash).REVIEW VERDICT: 0 blocker, 1 major, 0 minor
Two-Phase Rate-Limiter Review: PR #1195
Phase 1: Pre-Bcrypt Peek (DoS Shield) ✓
internal/opds/handler.go:562-567 —
authenticate()callsloginPeek(ip)BEFOREverifyCredentials(). If peek returns false (bucket exhausted), request is rejected without running bcrypt.internal/opds/handler_test.go:1784-1809 — Test "throttled IP (peek denies) is rejected before verifyCredentials is called" proves
verifyCallCount=0when peek denies. Bcrypt is shielded.internal/users/ratelimiter.go:2381-2401 — Peek() implementation uses
ReserveN(now,1)+DelayFrom(now)+CancelAt(now)to check without consuming. Clock-consistent for test compatibility.internal/users/ratelimiter_test.go:2414-2459 — Peek() tests confirm: (a) returns true when bucket has tokens, (b) does not consume (Allow still succeeds after 10 Peeks), (c) returns false when bucket exhausted, (d) refills after time.
internal/opds/handler.go:563 — Nil-safe:
if loginPeek != nil && !loginPeek(ip)handles disabled limiter.Phase 2: Post-Failure Consume (Brute-Force Throttle) ✓
internal/opds/handler.go:569-576 — On
verifyCredentials()failure, Phase 2 consumes vialoginConsume(ip). Only reachable if Phase 1 peek passed.internal/opds/handler_test.go:1811-1836 — Test "invalid credentials consume a token" proves failed attempt calls consume exactly once (consumeCallCount=1).
internal/opds/handler.go:572 — Nil-safe:
if loginConsume != nil && !loginConsume(ip)handles disabled limiter.Successful Auth (No Bucket Drain) ✓
internal/opds/handler.go:582 — Success path returns immediately without calling loginConsume. Prevents drain on normal polling.
internal/opds/handler_test.go:1754-1782 — Test "successful requests do not consume tokens (no bucket drain)" makes 5 successful requests and asserts consumeCallCount=0. Proven: zero token drain on success.
Wiring (No Nil-Func Trap) ✓
internal/app/app.go:34 — LoginPeek initialized:
LoginPeek: loginLimiter.Peek.internal/appwire/appwire.go:50 — LoginPeek declared in Deps.
internal/opds/wire.go:2303 — Wire reads both funcs and passes to all 4 handlers.
Module is by-value; LoginPeek field is set before usage. No nil-func trap.
No Regression ✓
internal/users/handler.go — No changes. Web
/loginpath unchanged.Ownership checks — OPDS routes are user-scoped. Fail-closed.
Test Hygiene ✓
package opds_test(black-box)Metrics & Coverage ✓
.golangci.ymlexclusions./internal/opds/...REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Phase 1 shields bcrypt from DoS. Phase 2 throttles brute-force. Successful requests never drain the bucket. No auth bypass. No regression. Review-ready.
Security re-review — PR #1195 (two-phase OPDS rate limiter, 3rd iteration)
Scope:
internal/opds/handler.go,internal/users/ratelimiter.go,internal/opds/wire.go,internal/appwire/appwire.go,internal/app/app.go. Adversarial verification that prior MAJOR #1 (shared-bucket drain by valid traffic) and MAJOR #2 (bcrypt-before-limiter DoS) are closed, and no third variant.Confirmed CLOSED
authenticaterunsloginPeek(non-consuming) pre-bcrypt and only callsloginConsume(Allow) on a failed credential check. A successful auth consumes zero tokens. Valid OPDS clients re-authenticating on every cover/thumbnail/file request no longer drain the sharedloginLimiterbucket, so assets don't 401 and the shared web/loginpath isn't starved by legitimate OPDS traffic.LoginPeek/LoginAlloware the sameloginLimiterinstance (app.go:474-475), so peek and consume are coherent.PeekusesReserveN(now,1)+DelayFrom(now)+CancelAt(now). Verified againstx/time@v0.15.0:CancelAtrestores the reserved token (bounded by burst), for both the has-token case (timeToAct==now, notBefore, restore 1) and the empty-bucket case (future reservation restored). NoReserve-without-Cancelleak; cannot be gamed to refill beyond the natural rate (restoration is bounded to this reservation's own token). The reserve/cancel race is over-throttle-only (safe direction). Clock-consistent for the injected fake clock.Peekrejects further attempts pre-bcrypt. No unlimited-guess path; peek can't reset the bucket.RemoteAddronly (opdsClientIP, no XFF trust).VerifyCredentialskeeps the dummy-hash timing-equalization path (no enumeration oracle). AuditActionLoginRateLimited/ActionLoginFailedrecorded withSanitizeDescription(username); no password/secret logged. Path-traversal guard on download;parseIDrejects non-positive. No nil-func wiring trap (both funcs set inapp.Newbeforeopds.Wireconsumes them).Findings
[MAJOR] internal/opds/handler.go:authenticate (Phase-1 peek) + internal/users/ratelimiter.go:Peek — Non-consuming peek does not bound concurrent bcrypt; MAJOR #2 only partially closed.
Because
Peekis non-consuming and the token-drainingAllowruns only afterverifyCredentials(bcrypt) completes, the entire bcrypt duration (~50-100ms) is a window in which any number of concurrent requests all observe the bucket as non-empty and all pass Phase 1 into bcrypt. Contrast the web/loginhandler (internal/users/handler.go:136->143), which callsAllow(consuming) before bcrypt and therefore caps concurrent bcrypt at the burst (10). OPDS has no such cap: a single IP can drive as many concurrent bcrypt hashes as it has in-flight connections. Worse, the shield reopens on every refill — whenever a token replenishes (1 per 6s) the bucket is briefly>0and a fresh concurrent wave again all peek-pass into bcrypt before the first failure consumes. Net effect: sustained CPU-amplification ~= (requests arriving within one bcrypt window) per refill, per IP — a real DoS on the exact axis this PR set out to fix. So I cannot fully confirm "bcrypt is genuinely not reached on a throttled IP" under concurrent load.Fix (closes #1 and #2 together): move the consume before bcrypt and refund on success — reserve/consume a token pre-bcrypt (reject if none), then
Cancel/restore it iffverifyCredentialssucceeds. That caps concurrent bcrypt at the burst while still consuming nothing for valid clients. (A per-IP in-flight semaphore aroundverifyCredentialsis an alternative, but the reserve-then-refund pattern is cleaner and reuses the existing limiter.)Verdict
REVIEW VERDICT: 0 blocker, 1 major, 0 minor
Security re-review (round 3) — M4B/CBZ binary rewriters
Scope: fix commits on
bd-bookshelf-r24bc.3@ce6ef75e—internal/files/m4b_metadata_write.go,internal/files/cbz_metadata_write.go,internal/app/build_extended_deps.go. Focus: residual data-loss MAJORs + any remaining silent-truncation / uncapped-read in the rewriters.Residual MAJORs from round 2 — both CONFIRMED CLOSED
m4b_metadata_write.go:126-152): now returnsErrInvalidM4Band aborts onsize64 < 16(below its own 16-byte header), onsize64 > remaining, and on a truncated 16-byte header. No truncatedmdatcan be written over the original. CLOSED.cbz_metadata_write.go:89-99):statFileruns and rejectssize > maxFileByteswithErrInvalidCBZbefore the whole-filereadFile. Correctly wired inbuild_extended_deps.go:2793-2811(os.Statshim +DefaultMaxCBZFileBytespassed). CLOSED.Adversarial pass on the binary parsing — the audio-drop class is closed
m4bParseTopLevelnow errors (aborts the whole write) on every malformed top-level atom, somdatcan never be silently skipped. InrewriteM4BTagsevery top-level atom is re-emitted verbatim exceptmoov(rebuilt);mdatis copied byte-for-byte, so the irreplaceable audio is provably preserved on any file that reaches the write. Offset/length arithmetic is 64-bit-safe (no int overflow on the size sums; everym4bReadUintNBEcall is guarded by anoff+N <= lenbound so no slice panic).stco/co64delta loops are bounded bybodyEndregardless of the declared count so no OOM. chunk-offset fixup is correctly scoped to the rebuiltmoovbody. No BLOCKER/MAJOR here.cbzRebuildZIPrejects entries whoseUncompressedSize64 > maxEPUBEntryBytes(50MB) up front (catches lying-large headers), and Go'sarchive/zipchecksumReader.ReadreturnsErrFormatthe instantnread > UncompressedSize64(reader.go:302-303), so a lying-small header cannot makeio.ReadAll(rc)over-read either. Per-entry read is capped; total is bounded by the 2GB file cap. No unbounded read/alloc in the rebuild. No finding.write.go): rewrite errors return beforeatomicWriteis called, so a rejected file never partially overwrites the source; the temp file is cleaned up on any write/close/rename failure. Path comes from the DB (GetPrimaryBookFilePath), not the request.internal/filesimports no go-workflows (only doc-comment references). No secrets/PII logged (these files log nothing). No regressions.Findings
[MINOR] internal/files/m4b_metadata_write.go:349,375 — inner moov/udta walkers
break(silent truncate) on a malformed child boxm4bRebuildMoovBody/m4bRebuildUdtaBodybreakonsize32 < 8 || off+size32 > len(body), silently dropping the remainingmoovchildren and writing a truncatedmoovover the source. Impact is bounded (mdat/audio is a top-level atom copied verbatim, so audio is never lost) and only reachable on an already-internally-corruptmoov, so this is not the catastrophic class — but it is inconsistent with them4bParseTopLevelhardening (error-not-break). These walkers also assume 8-byte headers throughout (no largesize handling). Fix: have the inner walkers returnErrInvalidM4Bon a malformed child instead ofbreak, matching the top-level parser, so a corruptmoovaborts the write rather than overwriting the original with a re-truncated file.[MINOR] internal/files/cbz_metadata_write.go:207,219 — rebuilt-ZIP
zw.Close()/ew.Write()errors are swallowed_ = zw.Close()and_, _ = ew.Write(entryData)ignore errors. In practice abytes.Buffer-backedzip.Writernever errors, so this is theoretical — butClose()is where the central directory is flushed, and given the output is atomically written over an irreplaceable source, a genuine error there would emit a corrupt archive over the original. Fix: checkzw.Close()(and ideally the entryWrite) and returnErrInvalidCBZon failure, so a malformed rebuild aborts before the rename.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
Security re-review (FINAL, adversarial) — reserve-then-refund OPDS rate limiter
Verdict up front: the reserve-then-refund design is sound, but this implementation of the refund is broken in production and reintroduces the original drain (MAJOR #1). The bug is masked by the frozen-clock unit tests, which is why it survived to iteration 4.
Everything else verified clean:
burstreservations (hence <=burst concurrent hashes) per IP exist at any instant; the burst+1th finds an empty bucket and is rejected pre-hash. Holds under both the broken and the fixed refund.burstfailures -> further attempts rejected pre-bcrypt. Empirically confirmed.burst; the refund closure is bound to the same reservationr/e.limiter(same IP), so it cannot exceed capacity or credit another IP.nowand does not consume; no refill-faster-than-rate race./loginunchanged (still loginLimiter.Allow, consuming). Audits preserved (ActionLoginRateLimited / ActionLoginFailed). No password/secret logged; username sanitized. LoginReserve wired as a non-nil method value in app.New; authenticate nil-guards it.REVIEW VERDICT: 1 blocker, 0 major, 2 minor
Security re-review — BLOCKER fix verification (refund-clock token-drain)
Verdict: the BLOCKER is genuinely closed. Reviewing
internal/users/ratelimiter.go+internal/users/ratelimiter_test.goonbd-bookshelf-6tq2k.1(fix commitcbbe2bff).1. Fix correctness — CONFIRMED.
ReserveTokencapturesnow := l.now()once, passes it toReserveN(now, 1), and both exit paths use that same reserve-time value:r.CancelAt(now)(ratelimiter.go:125)func() { r.CancelAt(now) }(ratelimiter.go:128)rate.Reservation.CancelAt(t)restores nothing whenr.timeToAct.Before(t). For an immediately-available reservationtimeToAct == now(reserve instant). Old code passed refund-timel.now()(advanced ~50-100ms by bcrypt) →timeToAct.Before(refundTime)true → no restore → drain. Fix passes reserve-timenow→now.Before(now)false → token genuinely restored under a real, moving clock. Closed.2. Regression test is GENUINE — CONFIRMED.
The moving-clock
Context(ratelimiter_test.go:195) is NOT frozen: it advancesstep += 100msbetween reserve and refund each cycle (simulating bcrypt latency). It runscycles = 20 > burst = 10and assertsHaveEach(BeTrue())plusfinalOK == true. Trace against the OLDCancelAt(l.now())code: each cycle nets ~1 token lost (refund is a no-op), so around cycle ~10 the bucket empties,DelayFrom(now) > 0→ReserveTokenreturns false →HaveEach(BeTrue())fails andfinalOKfalse → RED. Against the fix every cycle restores → GREEN. The test pins the exact regression.3. No new issue — CONFIRMED.
CancelAtrestores at most the one reserved token and the limiter caps atburst=10; the trace stays at ~10, never above.r.CancelAt(now)was already reserve-time) — throttle intact.internal/opds/handler.go:511-517returns on verify failure WITHOUT callingrefund, so wrong-password attempts keep the token consumed and exhaust the bucket pre-bcrypt (brute-force + concurrent-bcrypt DoS bound preserved). Success path (handler.go:519-522) callsrefund()exactly once.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
cbbe2bff8a28f9edcf15