mirror of
https://github.com/block/buzz.git
synced 2026-11-05 08:51:10 +00:00
f54d728e25
Red-team pass against the auth lane surfaced one real bug and two robustness gaps. All three caught by tests, the bug verified by temporarily reverting the fix and watching the test fail with the real Redis error. 1. Real bug: a caller passing ttl_secs > i64::MAX (e.g. u64::MAX from a config bug) caused Redis to return "ResponseError: value is not an integer or out of range" from `SET NX EX <ttl>`. RedisNip98ReplayGuard then returned Err, the trait contract forces callers to fail closed, and every NIP-98-gated request from that point would have errored with no visible link back to the bad config. Fix: introduce MAX_REPLAY_TTL_SECS (1 hour — 30× the natural physical maximum, well inside i64::MAX) and clamp ttl_secs into [DEFAULT, MAX] before the SET. New ignored-Redis test `above_ceiling_ttl_is_clamped` exercises the path with u64::MAX and asserts the claim+replay sequence succeeds, which it only does with the clamp. 2. Robustness: pin "all rate-limit and replay key components are lowercase ASCII" as a unit-level invariant. If pubkey::to_hex, Uuid::Display, or LimitType::key_suffix ever started emitting uppercase, the same logical (community, pubkey/event_id) would map to two distinct Redis keys — silently doubling the rate-limit quota or breaking the seen-set's identity. Two new tests (`rate_limit_key_components_are_lowercase`, `key_components_are_lowercase`) catch the regression in CI rather than production. 3. Robustness: structured tracing on every Redis failure path with `community = %ctx.community()` as a structured field, so ops can group log alerts by tenant without needing the community id to be embedded in the AuthError string. The user-facing AuthError::Internal payload stays the existing convention (consistent with rate_limit.rs neighbors); the per-tenant context lives in tracing fields, not in the error string. Also: add `ttl_floor_below_ceiling` and `max_ttl_fits_in_redis_signed_ex` unit tests so the two TTL constants can't drift past each other or above Redis's signed-EX limit in a future edit. Out of scope for this lane (flagged to other lane owners): - AuthError::Internal generally embeds raw downstream error strings (existing pattern across rate_limit.rs and nip98_replay.rs). Could leak community/tenant identifiers if those strings ever surface to clients. Audit lane (Quinn) owns the error-message safety rule per Eva's [6] lane split. - check_ip_connection MUST be called before host resolution / on every connection (including failed-host-resolution attempts). Otherwise an attacker who picks a non-matching host header bypasses the IP cap. Wiring lives in the relay-wiring lane (Eva). Validation: - cargo test -p buzz-auth --lib: 44 passed (4 new red-team tests). - cargo test -p buzz-pubsub --lib: 3 passed, 10 Redis-required ignored. - cargo test -p buzz-pubsub --lib nip98_replay -- --ignored against local Redis: 4 passed (1 new ceiling-clamp test). - Bug verified: with the clamp temporarily reverted, the above_ceiling_ttl_is_clamped test fails with the real Redis error "value is not an integer or out of range" — proving the test catches the regression, not just the fix. Co-authored-by: Tyler Longwell <tlongwell@block.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz>