Skip to content

Commit b2cc562

Browse files
committed
fix(webapp): compare bigint sanitizer boundaries via BigInt for exactness
Address review (CodeRabbit + Devin convergent finding): the literal `18446744073709551615` in JS source rounds to 2^64 in float64 (the spacing around 2^64 is 2048), so `value > 18446744073709551615` was effectively `value > 2**64` and missed the case where a JS Number's float64 value is *exactly* 2^64. `JSON.stringify(2**64)` emits "18446744073709552000" — a bare integer above UInt64.MAX that ClickHouse rejects — so the sanitizer would have let that row through unchanged and the batch would still drop. Switch to BigInt comparison against named `UINT64_MAX` / `INT64_MIN` constants. `BigInt(value)` is safe because we already gate on `Number.isInteger(value)`. The negative side is unaffected (Int64.MIN = -2^63 is exactly representable in float64) but the BigInt form keeps both sides symmetric and self-documenting. Regression test added: `sanitizeUnknownInPlace(2 ** 64)` must produce "18446744073709552000" with fixed=1. A naïve `>` literal comparison would not catch this. 23/23 sanitizer tests green; webapp typecheck clean.
1 parent 88c9f06 commit b2cc562

2 files changed

Lines changed: 22 additions & 1 deletion

File tree

apps/webapp/app/v3/eventRepository/sanitizeRowsOnParseError.server.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ export const INVALID_UTF16_SENTINEL = "[invalid-utf16]";
2727
* with an exponent and are accepted by CH at any magnitude, so they're
2828
* left alone.
2929
*/
30+
const UINT64_MAX = 18446744073709551615n;
31+
const INT64_MIN = -9223372036854775808n;
32+
3033
function isUnsafeJsonInteger(value: number): boolean {
3134
if (!Number.isFinite(value)) return false;
3235
if (!Number.isInteger(value)) return false;
@@ -36,7 +39,14 @@ function isUnsafeJsonInteger(value: number): boolean {
3639
// which CH accepts as Float64 at any magnitude. So the dangerous band
3740
// is strictly between the Int64/UInt64 boundary and 1e21.
3841
if (Math.abs(value) >= 1e21) return false;
39-
return value > 18446744073709551615 || value < -9223372036854775808;
42+
// Compare via BigInt for exactness. The Number literal 18446744073709551615
43+
// is rounded to 2**64 in float64 (the float spacing near 2^64 is 2048), so a
44+
// direct `value > 18446744073709551615` would miss a Number whose float64
45+
// value is exactly 2**64 — `JSON.stringify` of that emits
46+
// "18446744073709552000", which exceeds UInt64.MAX and ClickHouse rejects.
47+
// `BigInt(value)` is safe here because we already gated on Number.isInteger.
48+
const asBigInt = BigInt(value);
49+
return asBigInt > UINT64_MAX || asBigInt < INT64_MIN;
4050
}
4151

4252
export type SanitizeResult = {

apps/webapp/test/sanitizeRowsOnParseError.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,17 @@ describe("sanitizeUnknownInPlace", () => {
122122
expect(result.fixed).toBe(1);
123123
});
124124

125+
it("catches the float64 boundary at exactly 2**64 (UInt64.MAX + 1)", () => {
126+
// float64 cannot represent UInt64.MAX (2^64 - 1) exactly — the literal
127+
// 18446744073709551615 in JS source rounds to 2^64. JSON.stringify
128+
// emits this Number as "18446744073709552000", which exceeds UInt64.MAX
129+
// and trips ClickHouse. Regression for the BigInt-based comparison;
130+
// a naïve `value > 18446744073709551615` would let this pass.
131+
const result = sanitizeUnknownInPlace(2 ** 64);
132+
expect(result.value).toBe("18446744073709552000");
133+
expect(result.fixed).toBe(1);
134+
});
135+
125136
it("replaces an integer-valued Number below Int64.MIN with its string form", () => {
126137
// -9223372036854775809 is the first failing negative; in float64 it
127138
// rounds to the same representation as Int64.MIN (-9223372036854775808),

0 commit comments

Comments
 (0)