Skip to content

fix(parse): reject numeric strings with non-numbers #2729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 2, 2021
Merged

fix(parse): reject numeric strings with non-numbers #2729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55bdaaa
fix(parse): reject numeric strings with non-numbers
JordonPhillips Sep 1, 2021
98d43f9
chore: use more compact regex
JordonPhillips Sep 1, 2021
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
108 changes: 98 additions & 10 deletions packages/smithy-client/src/parse-utils.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,13 +320,44 @@ describe("strictParseDouble", () => {
expect(strictParseDouble("NaN")).toEqual(NaN);
});

it("rejects implicit NaN", () => {
expect(() => strictParseDouble("foo")).toThrowError();
describe("rejects implicit NaN", () => {
it.each([
"foo",
"123ABC",
"ABC123",
"12AB3C",
"1.A",
"1.1A",
"1.1A1",
"0xFF",
"0XFF",
"0b1111",
"0B1111",
"0777",
"0o777",
"0O777",
"1n",
"1N",
"1_000",
"e",
"e1",
".1",
])("rejects %s", (value) => {
expect(() => strictParseDouble(value)).toThrowError();
});
});

it("accepts numeric strings", () => {
expect(strictParseDouble("1")).toEqual(1);
expect(strictParseDouble("-1")).toEqual(-1);
expect(strictParseDouble("1.1")).toEqual(1.1);
expect(strictParseDouble("1e1")).toEqual(10);
expect(strictParseDouble("-1e1")).toEqual(-10);
expect(strictParseDouble("1e+1")).toEqual(10);
expect(strictParseDouble("1e-1")).toEqual(0.1);
expect(strictParseDouble("1E1")).toEqual(10);
expect(strictParseDouble("1E+1")).toEqual(10);
expect(strictParseDouble("1E-1")).toEqual(0.1);
});

describe("accepts numbers", () => {
Expand All @@ -347,8 +378,31 @@ describe("strictParseFloat32", () => {
expect(strictParseFloat32("NaN")).toEqual(NaN);
});

it("rejects implicit NaN", () => {
expect(() => strictParseFloat32("foo")).toThrowError();
describe("rejects implicit NaN", () => {
it.each([
"foo",
"123ABC",
"ABC123",
"12AB3C",
"1.A",
"1.1A",
"1.1A1",
"0xFF",
"0XFF",
"0b1111",
"0B1111",
"0777",
"0o777",
"0O777",
"1n",
"1N",
"1_000",
"e",
"e1",
".1",
])("rejects %s", (value) => {
expect(() => strictParseFloat32(value)).toThrowError();
});
});

describe("rejects doubles", () => {
Expand All @@ -359,7 +413,15 @@ describe("strictParseFloat32", () => {

it("accepts numeric strings", () => {
expect(strictParseFloat32("1")).toEqual(1);
expect(strictParseFloat32("-1")).toEqual(-1);
expect(strictParseFloat32("1.1")).toEqual(1.1);
expect(strictParseFloat32("1e1")).toEqual(10);
expect(strictParseFloat32("-1e1")).toEqual(-10);
expect(strictParseFloat32("1e+1")).toEqual(10);
expect(strictParseFloat32("1e-1")).toEqual(0.1);
expect(strictParseFloat32("1E1")).toEqual(10);
expect(strictParseFloat32("1E+1")).toEqual(10);
expect(strictParseFloat32("1E-1")).toEqual(0.1);
});

describe("accepts numbers", () => {
Expand Down Expand Up @@ -489,12 +551,26 @@ describe("strictParseLong", () => {
});

describe("rejects non-integers", () => {
it.each([1.1, "1.1", "NaN", "Infinity", "-Infinity", NaN, Infinity, -Infinity, true, false, [], {}])(
"rejects %s",
(value) => {
expect(() => strictParseLong(value as any)).toThrowError();
}
);
it.each([
1.1,
"1.1",
"NaN",
"Infinity",
"-Infinity",
NaN,
Infinity,
-Infinity,
true,
false,
[],
{},
"foo",
"123ABC",
"ABC123",
"12AB3C",
])("rejects %s", (value) => {
expect(() => strictParseLong(value as any)).toThrowError();
});
});
});

Expand Down Expand Up @@ -530,6 +606,10 @@ describe("strictParseInt32", () => {
-(2 ** 63 + 1),
2 ** 31,
-(2 ** 31 + 1),
"foo",
"123ABC",
"ABC123",
"12AB3C",
])("rejects %s", (value) => {
expect(() => strictParseInt32(value as any)).toThrowError();
});
Expand Down Expand Up @@ -570,6 +650,10 @@ describe("strictParseShort", () => {
-(2 ** 31 + 1),
2 ** 15,
-(2 ** 15 + 1),
"foo",
"123ABC",
"ABC123",
"12AB3C",
])("rejects %s", (value) => {
expect(() => strictParseShort(value as any)).toThrowError();
});
Expand Down Expand Up @@ -612,6 +696,10 @@ describe("strictParseByte", () => {
-(2 ** 15 + 1),
128,
-129,
"foo",
"123ABC",
"ABC123",
"12AB3C",
])("rejects %s", (value) => {
expect(() => strictParseByte(value as any)).toThrowError();
});
Expand Down
42 changes: 22 additions & 20 deletions packages/smithy-client/src/parse-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,15 +233,8 @@ export const expectString = (value: any): string | undefined => {
* @returns The value as a number, or undefined if it's null/undefined.
*/
export const strictParseDouble = (value: string | number): number | undefined => {
if (value === "NaN") {
return NaN;
}
if (typeof value == "string") {
const parsed: number = parseFloat(value);
if (Number.isNaN(parsed)) {
throw new TypeError(`Expected real number, got implicit NaN`);
}
return expectNumber(parsed);
return expectNumber(parseNumber(value));
}
return expectNumber(value);
};
Expand All @@ -262,19 +255,28 @@ export const strictParseFloat = strictParseDouble;
* @returns The value as a number, or undefined if it's null/undefined.
*/
export const strictParseFloat32 = (value: string | number): number | undefined => {
if (value === "NaN") {
return NaN;
}
if (typeof value == "string") {
const parsed: number = parseFloat(value);
if (Number.isNaN(parsed)) {
throw new TypeError(`Expected real number, got implicit NaN`);
}
return expectFloat32(parsed);
return expectFloat32(parseNumber(value));
}
return expectFloat32(value);
};

// This regex matches JSON-style numbers. In short:
// * The integral may start with a negative sign, but not a positive one
// * No leading 0 on the integral unless it's immediately followed by a '.'
// * Exponent indicated by a case-insensitive 'E' optionally followed by a
// positive/negative sign and some number of digits.
// It also matches both positive and negative infinity as well and explicit NaN.
const NUMBER_REGEX = /(-?(?:0|[1-9]\d*)(?:\.\d+)?(?:[eE][+-]?\d+)?)|(-?Infinity)|(NaN)/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nit: Is this too strict? parseFloat() handles strings leading with "0" just fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be overkill at this point (I think the concern was stuff like this) but we can always loosen this later. It's much more difficult to tighten it.


const parseNumber = (value: string): number => {
const matches = value.match(NUMBER_REGEX);
if (matches === null || matches[0].length !== value.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you anchor the regex so you can just use RegExp.test and not have to check the length of the match?

Copy link
Member Author

Choose a reason for hiding this comment

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

hahaha your faith in js is heartwarming, no it'll match partials anyway

throw new TypeError(`Expected real number, got implicit NaN`);
}
return parseFloat(value);
};

/**
* Asserts a value is a number and returns it. If the value is a string
* representation of a non-numeric number type (NaN, Infinity, -Infinity),
Expand Down Expand Up @@ -346,7 +348,7 @@ export const strictParseLong = (value: string | number): number | undefined => {
if (typeof value === "string") {
// parseInt can't be used here, because it will silently discard any
// existing decimals. We want to instead throw an error if there are any.
return expectLong(parseFloat(value));
return expectLong(parseNumber(value));
}
return expectLong(value);
};
Expand All @@ -370,7 +372,7 @@ export const strictParseInt32 = (value: string | number): number | undefined =>
if (typeof value === "string") {
// parseInt can't be used here, because it will silently discard any
// existing decimals. We want to instead throw an error if there are any.
return expectInt32(parseFloat(value));
return expectInt32(parseNumber(value));
}
return expectInt32(value);
};
Expand All @@ -389,7 +391,7 @@ export const strictParseShort = (value: string | number): number | undefined =>
if (typeof value === "string") {
// parseInt can't be used here, because it will silently discard any
// existing decimals. We want to instead throw an error if there are any.
return expectShort(parseFloat(value));
return expectShort(parseNumber(value));
}
return expectShort(value);
};
Expand All @@ -408,7 +410,7 @@ export const strictParseByte = (value: string | number): number | undefined => {
if (typeof value === "string") {
// parseInt can't be used here, because it will silently discard any
// existing decimals. We want to instead throw an error if there are any.
return expectByte(parseFloat(value));
return expectByte(parseNumber(value));
}
return expectByte(value);
};