fix(providers): cap Retry-After in googlebooks/hardcover (bookshelf-0sha) #569

Merged

zombor merged 2 commits from bd-bookshelf-0sha into main 2026-06-15 17:43:07 +00:00

Conversation 3 Commits 2 Files changed 7 +330 -70

zombor commented 2026-06-15 15:42:21 +00:00

Owner

Copy link

Summary

• Add maxRetryAfterWait = 60 * time.Second const to both googlebooks and hardcover metadata providers (matching the existing audnexus cap from #530/bookshelf-3g0l)
• Cap the parsed Retry-After duration at that limit in both the delay-seconds and HTTP-date forms of parseRetryAfter
• Log retry_after_clamped=true at the call site (Warn level) when the server-supplied header exceeds the cap, per logging-standard
• Update existing tests that asserted the old "no cap" behaviour; add new httptest.Server integration tests for: (a) oversized value clamped to cap, (b) value within cap honoured as-is, (c) missing header skips retryAfterSleep entirely

Test plan

• make build green
• make test green (all packages)
• make coverage green — 100% on both changed packages
• golangci-lint run clean on changed packages
• White-box ParseRetryAfter unit tests cover delay-seconds cap, HTTP-date cap, within-cap, past date, empty, negative
• Integration tests via httptest.Server verify end-to-end sleep behaviour

Closes bead bookshelf-0sha on merge.

## Summary - Add `maxRetryAfterWait = 60 * time.Second` const to both `googlebooks` and `hardcover` metadata providers (matching the existing `audnexus` cap from #530/bookshelf-3g0l) - Cap the parsed Retry-After duration at that limit in both the delay-seconds and HTTP-date forms of `parseRetryAfter` - Log `retry_after_clamped=true` at the call site (Warn level) when the server-supplied header exceeds the cap, per logging-standard - Update existing tests that asserted the old "no cap" behaviour; add new httptest.Server integration tests for: (a) oversized value clamped to cap, (b) value within cap honoured as-is, (c) missing header skips retryAfterSleep entirely ## Test plan - [x] `make build` green - [x] `make test` green (all packages) - [x] `make coverage` green — 100% on both changed packages - [x] `golangci-lint run` clean on changed packages - [x] White-box `ParseRetryAfter` unit tests cover delay-seconds cap, HTTP-date cap, within-cap, past date, empty, negative - [x] Integration tests via httptest.Server verify end-to-end sleep behaviour Closes bead bookshelf-0sha on merge.

zombor added 1 commit 2026-06-15 15:42:21 +00:00

fix(providers): cap Retry-After at 60s in googlebooks and hardcover ... ee40a92ef1

An unbounded Retry-After from a hostile server could pin a workflow
goroutine for an arbitrary duration. Add maxRetryAfterWait = 60s
(matching the audnexus provider cap from #530/bookshelf-3g0l) and
clamp parsed values in both delay-seconds and HTTP-date forms. Log
retry_after_clamped=true at the call site when the header exceeds the
cap. Tests updated to assert capped, within-cap, and missing-header
behaviour (httptest.Server integration + ParseRetryAfter white-box).

zombor commented 2026-06-15 16:00:48 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Phase 0: DEMO Verification

No DEMO block is expected for this resilience fix — the feature is verified through tests (CI is green). Proceeding directly to spec and code review.

Phase 1: Spec Compliance

Bead description: cap Retry-After at 60s in googlebooks + hardcover providers, clamping both delay-seconds and HTTP-date forms, with a clamp Warn log. 100% coverage on both packages.

All requirements are met:

• maxRetryAfterWait = 60 * time.Second constant added to both googlebooks/provider.go and hardcover/provider.go
• Both the integer-seconds path and HTTP-date path clamp correctly in parseRetryAfter
• retry_after_clamped=true Warn log arg appended at call site when clamped
• Tests cover: over-cap seconds, within-cap seconds, missing header, invalid header, far-future HTTP-date, near-future HTTP-date (within cap), negative/zero values
• CI green, PR mergeable

Phase 2: Code Quality

Security spot-check — hostile header values:

• Very large float (e.g. 1e300): secs * float64(time.Second) overflows float64 to +Inf; time.Duration(+Inf) yields math.MaxInt64 (Go's defined behavior for float→int conversion overflow). math.MaxInt64 > maxRetryAfterWait is true, so the clamp fires correctly. No panic, no negative wait.
• Negative floats: secs <= 0 branch returns 0 immediately. Safe.
• Far-future HTTP-date: time.Until(t) yields a large positive duration, caught by the > maxRetryAfterWait check. Safe.
• Past HTTP-date: d <= 0 returns 0. Safe.
• Garbage string: both strconv.ParseFloat and http.ParseTime fail, returns 0. Safe.

No secrets logged. redactKey on the URL is retained from the existing code.

Findings:

[MINOR] internal/metadata/googlebooks/provider.go:305 — retry_after_clamped log flag fires as a false positive for exactly-60 second header values

The condition retryAfter == maxRetryAfterWait && rawRetryAfter != "" is true both when a value was clamped (e.g. "9999") AND when the server sends exactly "60" (the cap boundary). In the latter case nothing was clamped but the log entry says it was. The correct approach is retryAfter == maxRetryAfterWait && parseRetryAfterRaw(rawRetryAfter) > maxRetryAfterWait, or comparing the raw parsed value to the cap before clamping. Low operational impact since 60s is an unlikely real-world value, but the log is inaccurate. Same issue in hardcover/provider.go:478.

[MINOR] internal/metadata/googlebooks/ratelimit_test.go — no test for retry_after_clamped log field being set/unset at the integration level

The unit tests for parseRetryAfter cover the clamp, and the ratelimit integration tests verify the clamped sleep duration, but no test checks that retry_after_clamped=true appears in the log for an over-cap value and is absent for a within-cap value. This is a minor test-coverage gap on the observability path (not a correctness gap since the log write path is trivial). Same gap in hardcover tests.

Conventions:

• var (...) grouping at top of function: not applicable here (no new multi-var blocks added).
• Curried func pattern: unchanged, no regression.
• Error wrapping: unchanged.
• Function length and nesting: both parseRetryAfter implementations are well within limits (<30 lines, nesting <4).

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## CODE REVIEW: APPROVED ### Phase 0: DEMO Verification No DEMO block is expected for this resilience fix — the feature is verified through tests (CI is green). Proceeding directly to spec and code review. ### Phase 1: Spec Compliance Bead description: cap `Retry-After` at 60s in googlebooks + hardcover providers, clamping both delay-seconds and HTTP-date forms, with a clamp Warn log. 100% coverage on both packages. All requirements are met: - `maxRetryAfterWait = 60 * time.Second` constant added to both `googlebooks/provider.go` and `hardcover/provider.go` - Both the integer-seconds path and HTTP-date path clamp correctly in `parseRetryAfter` - `retry_after_clamped=true` Warn log arg appended at call site when clamped - Tests cover: over-cap seconds, within-cap seconds, missing header, invalid header, far-future HTTP-date, near-future HTTP-date (within cap), negative/zero values - CI green, PR mergeable ### Phase 2: Code Quality **Security spot-check — hostile header values:** - Very large float (e.g. `1e300`): `secs * float64(time.Second)` overflows `float64` to `+Inf`; `time.Duration(+Inf)` yields `math.MaxInt64` (Go's defined behavior for float→int conversion overflow). `math.MaxInt64 > maxRetryAfterWait` is true, so the clamp fires correctly. No panic, no negative wait. - Negative floats: `secs <= 0` branch returns 0 immediately. Safe. - Far-future HTTP-date: `time.Until(t)` yields a large positive duration, caught by the `> maxRetryAfterWait` check. Safe. - Past HTTP-date: `d <= 0` returns 0. Safe. - Garbage string: both `strconv.ParseFloat` and `http.ParseTime` fail, returns 0. Safe. No secrets logged. `redactKey` on the URL is retained from the existing code. **Findings:** [MINOR] internal/metadata/googlebooks/provider.go:305 — `retry_after_clamped` log flag fires as a false positive for exactly-60 second header values The condition `retryAfter == maxRetryAfterWait && rawRetryAfter != ""` is true both when a value was clamped (e.g. `"9999"`) AND when the server sends exactly `"60"` (the cap boundary). In the latter case nothing was clamped but the log entry says it was. The correct approach is `retryAfter == maxRetryAfterWait && parseRetryAfterRaw(rawRetryAfter) > maxRetryAfterWait`, or comparing the raw parsed value to the cap before clamping. Low operational impact since 60s is an unlikely real-world value, but the log is inaccurate. Same issue in `hardcover/provider.go:478`. [MINOR] internal/metadata/googlebooks/ratelimit_test.go — no test for `retry_after_clamped` log field being set/unset at the integration level The unit tests for `parseRetryAfter` cover the clamp, and the ratelimit integration tests verify the clamped sleep duration, but no test checks that `retry_after_clamped=true` appears in the log for an over-cap value and is absent for a within-cap value. This is a minor test-coverage gap on the observability path (not a correctness gap since the log write path is trivial). Same gap in hardcover tests. **Conventions:** - `var (...)` grouping at top of function: not applicable here (no new multi-var blocks added). - Curried func pattern: unchanged, no regression. - Error wrapping: unchanged. - Function length and nesting: both `parseRetryAfter` implementations are well within limits (<30 lines, nesting <4). --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented 2026-06-15 16:23:16 +00:00

Author

Owner

Copy link

Security Review: bookshelf-0sha — Retry-After cap (googlebooks + hardcover)

Scope

Reviewed the diff for PR #569 (bd-bookshelf-0sha): parseRetryAfter in both internal/metadata/googlebooks/provider.go and internal/metadata/hardcover/provider.go now clamps server-supplied Retry-After values at 60 s; both delay-seconds and HTTP-date forms are clamped; a retry_after_clamped field is appended to the Warn log when the cap fires.

---

Float → Duration Overflow Analysis (delay-seconds path)

The conversion time.Duration(secs * float64(time.Second)) is the critical path. Go spec says converting a float to int when the value is outside the representable range is implementation-dependent; on amd64/arm64 (CVTTSS2SI/FCVTZS) +Inf converts to math.MinInt64 (most-negative int64).

For a hostile Retry-After: 1e300:

• secs = 1e300 > 0 — the secs <= 0 guard passes
• secs * float64(time.Second) overflows float64 to +Inf
• time.Duration(+Inf) wraps to math.MinInt64 (negative)
• d > maxRetryAfterWait is false (negative < 60 s) — cap check bypassed
• Returns the negative duration

However, the call site guard if retryAfter > 0 { retryAfterSleep(ctx, retryAfter) } prevents any sleep on a negative value. Net effect: an overflow value is silently treated as "no Retry-After header" (no sleep) rather than being clamped. This is safer than pre-patch (which would have produced a massive sleep) and does not cause a DoS. No panic, no negative-argument sleep. The security goal — bounding the maximum sleep — is achieved for the realistic hostile range (values up to ~9.2×10⁹ seconds, far past any plausible rate-limit signal).

Not a blocker. Minor semantic gap: a Retry-After of 1e300 is skipped rather than capped. A hardening fix (if secs > float64(maxRetryAfterWait)/float64(time.Second) { return maxRetryAfterWait } before the Duration conversion) would make intent explicit, but the security property holds either way.

---

Findings

[MINOR] internal/metadata/googlebooks/provider.go:440 — float→int64 overflow for astronomically large Retry-After values (> ~9.2×10⁹ s) silently bypasses the cap
The secs <= 0 guard runs before the cap guard. A value like 1e300 passes the secs <= 0 check, then secs * float64(time.Second) overflows to +Inf, which wraps to math.MinInt64 on conversion to time.Duration. The d > maxRetryAfterWait check is then false and the cap is not applied. The call-site if retryAfter > 0 guard prevents any sleep, so the result is "skip Retry-After" rather than "cap at 60 s." No DoS or panic; the security property holds for any realistic hostile value. Suggested hardening: add if secs > float64(maxRetryAfterWait)/float64(time.Second) { return maxRetryAfterWait } immediately after if secs <= 0. Same applies to internal/metadata/hardcover/provider.go:658.

[MINOR] internal/metadata/googlebooks/provider.go:305 — retry_after_clamped: true is a false positive when server sends exactly 60 s
Condition is retryAfter == maxRetryAfterWait && rawRetryAfter != "". A legitimate Retry-After: 60 parses to exactly maxRetryAfterWait without being clamped, yet the log records retry_after_clamped=true. This is a misleading observability signal, not a security issue. Fix: change the condition to retryAfter == maxRetryAfterWait && rawRetryAfter != "" && parsedSecsOrDateExceededCap(), or compare the raw seconds to the cap before conversion. Same issue at internal/metadata/hardcover/provider.go:478.

---

No Issues Found

• Key/token redaction intact: redactKey(reqURL) is applied consistently to all Google Books log sites (lines 278, 303, 323, 339, 347, 364). Hardcover passes the API key via Authorization: Bearer header (not URL), and the 429 log path logs only queryKey (a sanitized description from queryDescription, not the key) and rawRetryAfter — no credential exposure.
• No new request surface: only internal retry-delay clamping; no new endpoints, no new TLS/transport configuration.
• No injection surface: rawRetryAfter is a header value logged as a structured field (not interpolated into a string), so no log-injection risk.
• HTTP-date path is correct: time.Until(t) naturally returns negative for past dates (already guarded by d <= 0); future dates get clamped by the new d > maxRetryAfterWait check before return.
• No infinite loop introduced: retry loop is bounded by maxRetries = 3; the cap only reduces the sleep duration.
• DoS resistance improved: pre-patch a 429 with Retry-After: 9999 would sleep ~2.7 hours per attempt × 3 = ~8 hours pinning a goroutine. Post-patch: capped at 60 s per attempt.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Security Review: bookshelf-0sha — Retry-After cap (googlebooks + hardcover) ### Scope Reviewed the diff for PR #569 (`bd-bookshelf-0sha`): `parseRetryAfter` in both `internal/metadata/googlebooks/provider.go` and `internal/metadata/hardcover/provider.go` now clamps server-supplied Retry-After values at 60 s; both delay-seconds and HTTP-date forms are clamped; a `retry_after_clamped` field is appended to the Warn log when the cap fires. --- ### Float → Duration Overflow Analysis (delay-seconds path) The conversion `time.Duration(secs * float64(time.Second))` is the critical path. Go spec says converting a float to int when the value is outside the representable range is implementation-dependent; on amd64/arm64 (`CVTTSS2SI`/`FCVTZS`) +Inf converts to `math.MinInt64` (most-negative int64). For a hostile `Retry-After: 1e300`: - `secs = 1e300 > 0` — the `secs <= 0` guard passes - `secs * float64(time.Second)` overflows float64 to `+Inf` - `time.Duration(+Inf)` wraps to `math.MinInt64` (negative) - `d > maxRetryAfterWait` is **false** (negative < 60 s) — cap check bypassed - Returns the negative duration However, the call site guard `if retryAfter > 0 { retryAfterSleep(ctx, retryAfter) }` prevents any sleep on a negative value. Net effect: an overflow value is silently treated as "no Retry-After header" (no sleep) rather than being clamped. This is **safer than pre-patch** (which would have produced a massive sleep) and does not cause a DoS. No panic, no negative-argument sleep. The security goal — bounding the maximum sleep — is achieved for the realistic hostile range (values up to ~9.2×10⁹ seconds, far past any plausible rate-limit signal). **Not a blocker.** Minor semantic gap: a Retry-After of `1e300` is skipped rather than capped. A hardening fix (`if secs > float64(maxRetryAfterWait)/float64(time.Second) { return maxRetryAfterWait }` before the Duration conversion) would make intent explicit, but the security property holds either way. --- ### Findings [MINOR] internal/metadata/googlebooks/provider.go:440 — float→int64 overflow for astronomically large Retry-After values (> ~9.2×10⁹ s) silently bypasses the cap The `secs <= 0` guard runs before the cap guard. A value like `1e300` passes the `secs <= 0` check, then `secs * float64(time.Second)` overflows to `+Inf`, which wraps to `math.MinInt64` on conversion to `time.Duration`. The `d > maxRetryAfterWait` check is then `false` and the cap is not applied. The call-site `if retryAfter > 0` guard prevents any sleep, so the result is "skip Retry-After" rather than "cap at 60 s." No DoS or panic; the security property holds for any realistic hostile value. Suggested hardening: add `if secs > float64(maxRetryAfterWait)/float64(time.Second) { return maxRetryAfterWait }` immediately after `if secs <= 0`. Same applies to `internal/metadata/hardcover/provider.go:658`. [MINOR] internal/metadata/googlebooks/provider.go:305 — `retry_after_clamped: true` is a false positive when server sends exactly 60 s Condition is `retryAfter == maxRetryAfterWait && rawRetryAfter != ""`. A legitimate `Retry-After: 60` parses to exactly `maxRetryAfterWait` without being clamped, yet the log records `retry_after_clamped=true`. This is a misleading observability signal, not a security issue. Fix: change the condition to `retryAfter == maxRetryAfterWait && rawRetryAfter != "" && parsedSecsOrDateExceededCap()`, or compare the raw seconds to the cap before conversion. Same issue at `internal/metadata/hardcover/provider.go:478`. --- ### No Issues Found - **Key/token redaction intact**: `redactKey(reqURL)` is applied consistently to all Google Books log sites (lines 278, 303, 323, 339, 347, 364). Hardcover passes the API key via `Authorization: Bearer` header (not URL), and the 429 log path logs only `queryKey` (a sanitized description from `queryDescription`, not the key) and `rawRetryAfter` — no credential exposure. - **No new request surface**: only internal retry-delay clamping; no new endpoints, no new TLS/transport configuration. - **No injection surface**: `rawRetryAfter` is a header value logged as a structured field (not interpolated into a string), so no log-injection risk. - **HTTP-date path is correct**: `time.Until(t)` naturally returns negative for past dates (already guarded by `d <= 0`); future dates get clamped by the new `d > maxRetryAfterWait` check before return. - **No infinite loop introduced**: retry loop is bounded by `maxRetries = 3`; the cap only reduces the sleep duration. - **DoS resistance improved**: pre-patch a 429 with `Retry-After: 9999` would sleep ~2.7 hours per attempt × 3 = ~8 hours pinning a goroutine. Post-patch: capped at 60 s per attempt. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented 2026-06-15 16:23:42 +00:00

Author

Owner

Copy link

Security Review: bookshelf-0sha — Retry-After cap (googlebooks + hardcover)

Scope

Reviewed the diff for PR #569 (bd-bookshelf-0sha): parseRetryAfter in both internal/metadata/googlebooks/provider.go and internal/metadata/hardcover/provider.go now clamps server-supplied Retry-After values at 60 s; both delay-seconds and HTTP-date forms are clamped; a retry_after_clamped field is appended to the Warn log when the cap fires.

---

Float to Duration Overflow Analysis (delay-seconds path)

The conversion time.Duration(secs * float64(time.Second)) is the critical path. Go spec says converting a float to int when the value is outside the representable range is implementation-dependent; on amd64/arm64 +Inf converts to math.MinInt64 (most-negative int64).

For a hostile Retry-After: 1e300:

• secs = 1e300 > 0 — the secs <= 0 guard passes
• secs * float64(time.Second) overflows float64 to +Inf
• time.Duration(+Inf) wraps to math.MinInt64 (negative)
• d > maxRetryAfterWait is false (negative < 60 s) — cap check bypassed, returns negative

The call-site guard if retryAfter > 0 { retryAfterSleep(ctx, retryAfter) } prevents any sleep on a negative value. Net effect: an overflow value is silently treated as "no Retry-After header" (no sleep) rather than being clamped. This is safer than pre-patch (which would have produced a massive sleep) and does not cause a DoS. No panic. The security goal is achieved for the realistic hostile range.

---

Findings

[MINOR] internal/metadata/googlebooks/provider.go:440 — float->int64 overflow for extremely large Retry-After values silently bypasses the cap

The secs <= 0 guard runs before the cap guard. A value like 1e300 passes secs <= 0, then secs * float64(time.Second) overflows to +Inf, which wraps to math.MinInt64 on conversion to time.Duration. The d > maxRetryAfterWait check is then false and the cap is not applied. The call-site if retryAfter > 0 guard prevents any sleep, so the result is "skip Retry-After" rather than "cap at 60 s." No DoS or panic; security property holds for any realistic hostile value. Suggested hardening: add if secs > float64(maxRetryAfterWait)/float64(time.Second) { return maxRetryAfterWait } immediately after if secs <= 0. Same applies to internal/metadata/hardcover/provider.go:658.

[MINOR] internal/metadata/googlebooks/provider.go:305 — retry_after_clamped: true is a false positive when server sends exactly 60 s

Condition is retryAfter == maxRetryAfterWait && rawRetryAfter != "". A legitimate Retry-After: 60 parses to exactly maxRetryAfterWait without being clamped, yet the log records retry_after_clamped=true. Misleading observability signal, not a security issue. Same at internal/metadata/hardcover/provider.go:478.

---

No Issues Found

• Key/token redaction intact: redactKey(reqURL) applied consistently to all Google Books log sites. Hardcover passes the API key via Authorization header (not URL); the 429 log path logs only queryKey (sanitized description) and rawRetryAfter. No credential exposure.
• No new request surface: only internal retry-delay clamping; no new endpoints, no TLS/transport changes.
• No injection surface: rawRetryAfter is logged as a structured field, not interpolated into a string. No log-injection risk.
• HTTP-date path is correct: time.Until(t) naturally returns negative for past dates (guarded by d <= 0); future dates get the new d > maxRetryAfterWait clamp.
• No infinite loop introduced: retry loop bounded by maxRetries = 3; cap only reduces sleep duration.
• DoS resistance improved: pre-patch a 429 with Retry-After: 9999 slept ~2.7 hours per attempt x 3 attempts. Post-patch: capped at 60 s per attempt.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Security Review: bookshelf-0sha — Retry-After cap (googlebooks + hardcover) ### Scope Reviewed the diff for PR #569 (`bd-bookshelf-0sha`): `parseRetryAfter` in both `internal/metadata/googlebooks/provider.go` and `internal/metadata/hardcover/provider.go` now clamps server-supplied Retry-After values at 60 s; both delay-seconds and HTTP-date forms are clamped; a `retry_after_clamped` field is appended to the Warn log when the cap fires. --- ### Float to Duration Overflow Analysis (delay-seconds path) The conversion `time.Duration(secs * float64(time.Second))` is the critical path. Go spec says converting a float to int when the value is outside the representable range is implementation-dependent; on amd64/arm64 +Inf converts to math.MinInt64 (most-negative int64). For a hostile `Retry-After: 1e300`: - `secs = 1e300 > 0` — the `secs <= 0` guard passes - `secs * float64(time.Second)` overflows float64 to +Inf - `time.Duration(+Inf)` wraps to math.MinInt64 (negative) - `d > maxRetryAfterWait` is **false** (negative < 60 s) — cap check bypassed, returns negative The call-site guard `if retryAfter > 0 { retryAfterSleep(ctx, retryAfter) }` prevents any sleep on a negative value. Net effect: an overflow value is silently treated as "no Retry-After header" (no sleep) rather than being clamped. This is safer than pre-patch (which would have produced a massive sleep) and does not cause a DoS. No panic. The security goal is achieved for the realistic hostile range. --- ### Findings [MINOR] internal/metadata/googlebooks/provider.go:440 — float->int64 overflow for extremely large Retry-After values silently bypasses the cap The `secs <= 0` guard runs before the cap guard. A value like `1e300` passes `secs <= 0`, then `secs * float64(time.Second)` overflows to +Inf, which wraps to math.MinInt64 on conversion to `time.Duration`. The `d > maxRetryAfterWait` check is then false and the cap is not applied. The call-site `if retryAfter > 0` guard prevents any sleep, so the result is "skip Retry-After" rather than "cap at 60 s." No DoS or panic; security property holds for any realistic hostile value. Suggested hardening: add `if secs > float64(maxRetryAfterWait)/float64(time.Second) { return maxRetryAfterWait }` immediately after `if secs <= 0`. Same applies to internal/metadata/hardcover/provider.go:658. [MINOR] internal/metadata/googlebooks/provider.go:305 — `retry_after_clamped: true` is a false positive when server sends exactly 60 s Condition is `retryAfter == maxRetryAfterWait && rawRetryAfter != ""`. A legitimate `Retry-After: 60` parses to exactly `maxRetryAfterWait` without being clamped, yet the log records `retry_after_clamped=true`. Misleading observability signal, not a security issue. Same at internal/metadata/hardcover/provider.go:478. --- ### No Issues Found - **Key/token redaction intact**: `redactKey(reqURL)` applied consistently to all Google Books log sites. Hardcover passes the API key via Authorization header (not URL); the 429 log path logs only `queryKey` (sanitized description) and `rawRetryAfter`. No credential exposure. - **No new request surface**: only internal retry-delay clamping; no new endpoints, no TLS/transport changes. - **No injection surface**: `rawRetryAfter` is logged as a structured field, not interpolated into a string. No log-injection risk. - **HTTP-date path is correct**: `time.Until(t)` naturally returns negative for past dates (guarded by `d <= 0`); future dates get the new `d > maxRetryAfterWait` clamp. - **No infinite loop introduced**: retry loop bounded by `maxRetries = 3`; cap only reduces sleep duration. - **DoS resistance improved**: pre-patch a 429 with `Retry-After: 9999` slept ~2.7 hours per attempt x 3 attempts. Post-patch: capped at 60 s per attempt. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor added 1 commit 2026-06-15 16:29:38 +00:00

fix(providers): clamp overflow before Duration cast; fix clamp-log false positive ... a06665fbd3

Both googlebooks and hardcover parseRetryAfter had two issues:
1. Float-overflow: time.Duration(1e300 * float64(time.Second)) wraps to a
   large negative, bypassing the maxRetryAfterWait cap. Fix: compare secs
   against maxSecs = float64(maxRetryAfterWait)/float64(time.Second) in float
   space before converting, returning the cap immediately on overflow.
2. False-positive clamp log: the condition `retryAfter == maxRetryAfterWait &&
   rawRetryAfter != ""` fired when the server sent exactly "60" (nothing was
   clamped). Fix: return a bool from parseRetryAfter indicating whether the
   value was actually reduced; call sites use that bool instead of comparing
   the result to the cap.

Tests added: 1e300 asserts cap+clamped=true in both providers; "60" asserts
cap duration with clamped=false in both providers.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor force-pushed bd-bookshelf-0sha from a06665fbd3 to 130e7e3cbe 2026-06-15 16:52:43 +00:00 Compare

zombor merged commit bcd4023ae3 into main 2026-06-15 17:43:07 +00:00

zombor referenced this pull request from a commit 2026-06-15 17:43:09 +00:00

Merge pull request 'fix(providers): cap Retry-After in googlebooks/hardcover (bookshelf-0sha)' (#569) from bd-bookshelf-0sha into main

zombor referenced this pull request 2026-07-08 05:05:46 +00:00

test(metadata): harden Retry-After clamp boundary + log tests (bookshelf-8oj1) #1026

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!569

No description provided.