Skip to content

fix: validate sortable timestamp range and prevent state poisoning - #57

Open
jdymitarai wants to merge 2 commits into
suhailopensource:mainfrom
jdymitarai:fix-sortable-timestamp-poisoning
Open

jdymitarai wants to merge 2 commits into
suhailopensource:mainfrom
jdymitarai:fix-sortable-timestamp-poisoning

Conversation

@jdymitarai

Copy link
Copy Markdown

The sortable generator previously allowed timestamps exceeding SORTABLE_TIME_MAX to be encoded because it only checked whether the numeric value fit within the configured character width, leading to IDs that getTimestamp subsequently rejected as undefined. In addition, lastTime and lastRandom state were updated prior to validation, causing any transient out-of-range clock reading or monotonic spill-over to permanently poison subsequent ID generation in long-running processes. This change computes candidate timestamp and random values first, validates that the timestamp does not exceed SORTABLE_TIME_MAX and fits character width, throws a RangeError on invalid values, and defers saving generator state until validation passes. All local tests, typechecks, packaging audits, and smoke tests pass cleanly.

Fixes #55

Validate candidate timestamps against SORTABLE_TIME_MAX and defer generator state updates until validation passes, preventing out-of-range IDs and generator poisoning from transient clock glitches.

Fixes suhailopensource#55

Signed-off-by: jdymitarai <o10040115@gmail.com>
@suhailopensource

Copy link
Copy Markdown
Owner

Thanks @jdymitarai, the fix looks good!

One change please: remove the two lastTime = -1; lastRandom = []; lines, and the try/catch around encodeTime. When the time is bad, just don't save it.

Resetting makes the generator forget its last ID, so new IDs can come out in the wrong order.

Also, the issue was wrong about "recovering" after 2^48−1, sorry. It should keep throwing there.

Please replace the "recovers on subsequent calls" test with these two:

  it("keeps throwing after spilling past 2^48 - 1", () => {
    let now = SORTABLE_TIME_MAX;
    const gen = createSortableId({
      now: () => now,
      alphabet: "01",
      randomSize: 1,
    });

    expect(() => {
      for (let i = 0; i < 3; i++) gen("evt");
    }).toThrow(RangeError);

    now = Date.now();
    expect(() => gen("evt")).toThrow(RangeError);

    const fresh = createSortableId({
      now: () => now,
      alphabet: "01",
      randomSize: 1,
    });
    expect(() => fresh("evt")).not.toThrow();
  });

  it("keeps ids in order after a bad clock reading", () => {
    let now = 2_000;
    const gen = createSortableId({ now: () => now });

    const a = gen("evt");
    now = 1_000;
    const b = gen("evt");
    now = 2 ** 48 + 5;
    expect(() => gen("evt")).toThrow(RangeError);
    now = 1_000;
    const c = gen("evt");

    expect(b > a).toBe(true);
    expect(c > b).toBe(true);
  });

Both fail now and pass after the change. Thanks!

@jdymitarai

Copy link
Copy Markdown
Author

Updated as requested: removed the \lastTime/\lastRandom\ resets and the \ ry/catch\ around \encodeTime\ so bad timestamps leave existing generator state untouched, and replaced the spill test with the two new test cases. All 163 tests, typechecks, and package verifications pass cleanly.

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.

[bug] : Sortable generator makes timestamps its own decoder rejects, and one bad clock reading breaks the generator for good

2 participants