fix: reject expiration timestamp 0 and accept far-future timestamps in getEventExpiration() - #708
Conversation
…n getEventExpiration()
🦋 Changeset detectedLatest commit: 3457bae The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
coverage: 68.723% (+0.02%) from 68.708% — Priyanshubhartistm:fix/event-expiration-validation-allows-zero into cameri:main |
|
|
||
| const expirationTime = Number(rawExpirationTime) | ||
|
|
||
| if (Number.isSafeInteger(expirationTime) && Math.log10(expirationTime) < 10) { |
There was a problem hiding this comment.
Thanks for the PR! The Math.log10(0) fix is a genuine improvement.
| event.tags = [['expiration', 'a']] | ||
| expect(getEventExpiration(event)).to.be.undefined | ||
| }) | ||
|
|
||
| it('returns false if expiration is 0', () => { | ||
| event.tags = [['expiration', '0']] | ||
| expect(getEventExpiration(event)).to.be.undefined | ||
| }) | ||
|
|
||
| it('returns true if expiration is a safe integer beyond year 2287', () => { | ||
| event.tags = [['expiration', '10000000000']] | ||
| expect(getEventExpiration(event)).to.equal(10000000000) | ||
| }) | ||
|
|
||
| it('returns true if expiration is the maximum representable Unix seconds timestamp', () => { | ||
| event.tags = [['expiration', '253402300799']] | ||
| expect(getEventExpiration(event)).to.equal(253402300799) | ||
| }) | ||
|
|
||
| it('returns false if expiration looks like a millisecond-scale timestamp', () => { | ||
| event.tags = [['expiration', '1700000000000']] | ||
| expect(getEventExpiration(event)).to.be.undefined | ||
| }) | ||
| }) | ||
|
|
||
| describe('isExpiredEvent', () => { |
There was a problem hiding this comment.
There's a problem with the new ceiling though.
events.expires_at is a plain PostgreSQL integer. The .unsigned() in the migration is MySQL specific, and Knex ignores it on PostgreSQL. That means we're capped at 2147483647 (January 2038), so both values your tests accept (10000000000 and 253402300799) are too large for the column. In fact, timestamps from 2038 to 2286 already fail on main today for the same reason, since the old log10 < 10 check allowed them through as well.
The INSERT then fails, create() catches the error and returns 0, and DefaultEventStrategy interprets that as a duplicate. The client ends up getting ["OK", id, true, "duplicate:"] for an event that was never stored.
On main, the same event is at least stored with expires_at = NULL. Ignoring the expiration is the bug we're trying to fix, but the note itself is still preserved. With this change, the note is silently dropped instead. So it doesn't quite fix #707; it changes the failure mode from "expiration ignored" to "event silently lost."
The tests only exercise the validation function and never hit the database, so there wasn't really a way for this to be caught by the current test suite. It's an easy thing to miss.
|
@Priyanshubhartistm To be clear this isn't on you. 2038–2286 already fails on main for the same reason, the old @cameri i think your ms-scale concern still holds, just at a lower ceiling. |
|
@Priyanshubhartistm Flip the two far-future tests to expect undefined and add one for Actual year-9999 support needs |
5 participants
Description
getEventExpiration()insrc/utils/event.tsusedMath.log10(expirationTime) < 10as a bounds check on the NIP-40expirationtag. This has two bugs:Math.log10(0) === -Infinity, which is< 10, so an expiration of0(Unix epoch) was accepted as valid.> 9,999,999,999) failed the< 10check and was silently discarded, treated as "no expiration."Replaced the check with
expirationTime > 0, which accepts any positive safe integer and rejects non-positive values, with no arbitrary digit-count ceiling.Related Issue
Closes #707
Motivation and Context
An expiration of
0should never be treated as a valid future expiration, and a relay shouldn't silently ignore legitimate (if far-future) expiration timestamps just because they cross an arbitrary digit-count boundary.How Has This Been Tested?
test/unit/utils/event.spec.tsunder the existingNIP-40 > getEventExpirationsuite:expiration: '0'→undefinedexpiration: '10000000000'(year 2287+) → returned as-ispnpm run test:unit(1395 passing) andpnpm run test:cli(73 passing)pnpm run lint,pnpm run check:deps,pnpm run build,pnpm run verify:cli:build,pnpm run cover:unit— all pass.Screenshots (if appropriate):
N/A — pure logic fix, no UI/API surface change.
Types of changes
Checklist: