Skip to content

fix(relay): share one overflow-free created_at skew check - #9231

Draft
ThePumpingLemma wants to merge 1 commit into
mainfrom
dgrochowski/shared-created-at-skew-check
Draft

ThePumpingLemma wants to merge 1 commit into
mainfrom
dgrochowski/shared-created-at-skew-check

Conversation

@ThePumpingLemma

@ThePumpingLemma ThePumpingLemma commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Five relay created_at freshness gates (general ingest drift ±900 s, NIP-43 leave ±120 s, moderation commands ±120 s, relay admin commands ±120 s, NIP-IA archive requests ±120 s) each re-implemented (created_at as i64 - now).abs() > max. With created_at = now + 2^63 the difference is exactly i64::MIN. Its abs() overflows: debug builds panic, and release builds wrap to a negative value that is never > max, so the gate accepts an extreme future timestamp. Nearby values overflow the subtraction itself, which panics in debug. The observer-frame gate (±300 s) used unsigned_abs but still did the i64 subtraction.

Change

  • New handlers::freshness module:
    • check_created_at_skew(Timestamp, now, max) compares in u64 with abs_diff, so no input can overflow it.
    • unix_now_secs() is the single source for the current time.
    • MAX_COMMAND_SKEW_SECS (120 s) is now one constant for all four command gates.
  • Every relay copy now uses the helper. Each site keeps its window and error text; the reported delta is widened to i128.
  • The general ingest gate change is kept to the comparison only. Its chrono now/event_ts locals stay because later code uses them.
  • buzz-auth already uses abs_diff and is not touched.
  • The local Postgres test URL already in identity_archive.rs gets the repo's usual sadscan:disable fixture marker. The commit hook flags the file once it is modified.

Accept/reject results only change for inputs that overflowed before. For every other created_at, abs_diff gives the same distance as the old code.

Testing

Failing before the fix (seam test through identity_archive::enforce_freshness):

$ cargo test -p buzz-relay --lib identity_archive::freshness_tests
---- ...freshness_rejects_created_at_that_wraps_i64_delta stdout ----
panicked at library/core/src/num/mod.rs:451:5:
attempt to negate with overflow
test result: FAILED. 1 passed; 1 failed

$ cargo test --release -p buzz-relay --lib identity_archive::freshness_tests
panicked at crates/buzz-relay/src/handlers/identity_archive.rs:788:14:
extreme future timestamp must be rejected: ()
test result: FAILED. 1 passed; 1 failed

After the fix:

  • cargo test -p buzz-relay --lib -- freshness moderation_commands relay_admin identity_archive ingest: 233 passed, 33 ignored (Postgres)
  • cargo test --release -p buzz-relay --lib -- freshness: 7 passed
  • cargo clippy -p buzz-relay --all-targets -- -D warnings: clean
  • cargo fmt --all: clean

The new tests are a helper table test (window edges, 0, u64::MAX, 2^63, now + 2^63 ± 1), a delta-widening test, an epoch-clock test, and the seam test.

Not covered

  • Postgres-backed tests were not run.
  • The canvas future-timestamp check still receives created_at as i64. The general gate now rejects extreme values before that check runs.

Review

No blockers were found. Every relay skew gate uses the shared helper, and each keeps its window and error text for in-range input. The canvas future check runs only after the ±900 s gate, so it cannot overflow. The sadscan marker matches the repo's existing fixture convention.

Optional:

  • Only the NIP-IA archive gate has a production-seam regression test. One wrap-point case per other gate would also catch a revert at those sites.
  • buzz-acp's observer-control freshness check has the same i64-subtraction pattern (debug panic only). It is out of scope here.
  • The general ingest gate could use unix_now_secs() and scope event_ts to the canvas branch.
  • The unix_now_secs doc says "fails every check closed", but created_at ≤ window still passes when now = 0.

Exercised on a local relay

Commit tested: ca729388a. Dev (debug) profile, so the old code would panic on overflow. Isolated harness relay on :3030 with BUZZ_REQUIRE_RELAY_MEMBERSHIP=true and the sender set as relay owner. Events were signed with exact created_at values, including ones above 2^53, and sent over WS EVENT.

Kind now+2^63, u64::MAX, now+2^63−1 just outside the window (+150 s commands, +930 s normal) just inside (+90 s / +870 s)
9040 moderation ban invalid: event timestamp too far from server time invalid: event timestamp out of range: … delta=150s (max ±120s) accepted
9031 relay admin remove same as above invalid: event timestamp out of range: … delta=150s (max ±120s) passed the gate (member not found at now)
28936 NIP-43 leave same as above invalid: leave request timestamp out of range (delta=150s, max ±120s) passed the gate (relay owner cannot leave)
9035 NIP-IA archive same as above invalid: event timestamp out of range: … delta=150s (max ±120s) passed the gate (later tag check)
9 normal message same as above invalid: event timestamp too far from server time accepted

The relay never panicked, and the socket stayed open.

Observation: over WS ingest, the extreme values never reach the per-site command gates. The general ±900 s gate runs first and rejects them with its own text. So the overflow fix in the command gates is shown only by the unit and seam tests. On this path, the per-site text appears only for small out-of-window deltas.

Postgres-gated tests: I ran cargo test -p buzz-relay --lib -- --ignored --test-threads=1 freshness moderation_commands relay_admin identity_archive ingest, pointing DATABASE_URL, BUZZ_TEST_DATABASE_URL and BUZZ_TEST_REDIS_URL at the harness: 32 passed, 1 failed. The failure is ingest_write_fence_follows_community_deletion_lifecycle. It runs sqlx migrations, which fail against the harness schema that pgschema already applied (type "channel_type" already exists). That is a test-environment problem and doesn't involve the freshness code. The 4 freshness/moderation/relay-admin/identity-archive tests are among the passes; the filter without ingest gave 9 passed.

Not exercised:

  • A release build of main to show the old wrap.
  • The observer-frame (±300 s) gate.
  • The HTTP /events transport.

Problem: five relay freshness gates (general ingest drift, NIP-43 leave,
moderation commands, relay admin commands, NIP-IA archive requests) each
re-implemented `(created_at as i64 - now).abs() > max`. For
`created_at = now + 2^63` the difference is exactly `i64::MIN`, whose
`abs()` overflows: debug builds panic, release builds wrap to a negative
number that is never `> max`, so an extreme future timestamp passes the
gate. Neighbouring values overflow the subtraction itself (debug panic).
The observer-frame gate used `unsigned_abs` but still did the i64
subtraction.

Change: add `handlers::freshness` with `check_created_at_skew`, which
compares the nostr `Timestamp` seconds against `now` with
`u64::abs_diff`, one `unix_now_secs` now source, and one
`MAX_COMMAND_SKEW_SECS` (120 s) for the command gates. Every relay copy
is migrated; the per-site windows (900 s, 300 s, 120 s) and error texts
are unchanged, with the reported delta widened to i128. The general
ingest gate keeps its existing chrono `now`/`event_ts` locals because
later code uses them. The pre-existing local Postgres test URL in
identity_archive.rs gets the repo's usual sadscan fixture marker.

Why safe: for every created_at that did not overflow, abs_diff gives the
same distance as before, so in-window events are accepted and
out-of-window events rejected exactly as today; only the overflowing
inputs change from accept/panic to reject.

Tests: table test of the helper (window edges, 0, u64::MAX, 2^63 and
now + 2^63 +/- 1) and a seam test through identity_archive's
`enforce_freshness`. Before the fix the seam test panicked with
"attempt to negate with overflow" in debug and failed with the extreme
event accepted in release.

Signed-off-by: David Grochowski <dgrochowski@squareup.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant