Repository navigation
Fail closed when the NIP-17 timestamp cannot be fuzzed - #69
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. WalkthroughNIP-17 timestamp generation now fails with ChangesNIP-17 error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nip17.c`:
- Around line 121-126: The timestamp-failure cleanup paths in src/nip17.c lines
121-126 and 236-241 must release all acquired resources before returning: in the
first path, free encrypted_rumor after destroying the partial seal; in the
second path, free encrypted_seal and wipe ephemeral_privkey before returning.
Update the corresponding cleanup logic around the affected seal/event
construction functions while preserving the existing error returns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
1 participant
Summary
random_timestamp_past_two_days()returned the exact current time whenRAND_bytesfailed. It now propagates the error and both callers abort.Why this is a real weakening, not a nit
NIP-17 back-dates seals and gift wraps by a random offset for one reason: so a relay observer cannot correlate send times. Collapsing to
now()on RNG failure removes exactly that property, and removes it silently. The event still builds, still validates, still sends. The only observable difference is that the timing metadata the construction exists to hide is published in the clear.Same shape as the RNG defects fixed in #63: a failure path that returns a plausible value instead of an error, so nothing downstream can tell the difference.
Both call sites, the seal at
nip17.c:121and the gift wrap at:236, now destroy the partially built event, null the out-param, and return the error rather than emitting an event whose timestamp is not fuzzed.On the error code
Returns
NOSTR_ERR_MEMORY, which is whatkey.c:229andevent.c:627already return for an RNG failure. Semantically loose for this case, but consistent with the codebase, and a dedicated code would be a public enum change that belongs in its own commit rather than riding along here.I first wrote
NOSTR_ERR_CRYPTO_FAILED, which does not exist. The compiler caught it; worth noting because the fix was to read what the codebase actually does rather than to invent what it ought to.Test plan
grepforreturn now;returns 0, so no path emits an unfuzzed timestampopenssl/err.hadded forERR_clear_error, matching how the OpenSSL branch inkey.cclears the error queue after a failed drawCloses libnostr-c-lp6.
Summary by CodeRabbit