Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-event-expiration-validation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"nostream": patch
---

fix: reject expiration timestamp 0 and millisecond-scale values, and accept safe-integer second-based timestamps up to year 9999 in getEventExpiration()
7 changes: 6 additions & 1 deletion src/utils/event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,11 @@ export const isExpiredEvent = (event: Event): boolean => {
return expirationTime <= now
}

// 9999-12-31T23:59:59Z as Unix seconds. Millisecond-scale timestamps for any date
// from 2001 onward are at least 13 digits, well past this ceiling, so this rejects
// accidental millisecond values without capping legitimate far-future expirations.
const MAX_EXPIRATION_TIME = 253402300799

export const getEventExpiration = (event: Event): number | undefined => {
const [, rawExpirationTime] = event.tags.find((tag) => tag.length >= 2 && tag[0] === EventTags.Expiration) ?? []
if (!rawExpirationTime) {
Expand All @@ -253,7 +258,7 @@ export const getEventExpiration = (event: Event): number | undefined => {

const expirationTime = Number(rawExpirationTime)

if (Number.isSafeInteger(expirationTime) && Math.log10(expirationTime) < 10) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! The Math.log10(0) fix is a genuine improvement.

if (Number.isSafeInteger(expirationTime) && expirationTime > 0 && expirationTime <= MAX_EXPIRATION_TIME) {
return expirationTime
}
}
Expand Down
20 changes: 20 additions & 0 deletions test/unit/utils/event.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,26 @@ describe('NIP-40', () => {
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', () => {
Comment on lines 691 to 716

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
Loading