FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

ensureSlash: Fix accidental string-to-NaN coercion by wchargin · Pull Request #4424 · react/create-react-app · GitHub

ensureSlash: Fix accidental string-to-NaN coercion - #4424

Merged
Timer merged 1 commit into
react:nextfrom
wchargin:wchargin-ensureslash-accidental-coercion
May 8, 2018
Merged

ensureSlash: Fix accidental string-to-NaN coercion#4424
Timer merged 1 commit into
react:nextfrom
wchargin:wchargin-ensureslash-accidental-coercion

Conversation

wchargin commented May 8, 2018

Copy link
Copy Markdown
Contributor

Summary:
The hasSlash method uses path.substr(path, path.length - 1) to
remove the last character from path. Clearly, the first parameter is
suspect; it should be 0. The code works as written, but only very
accidentally: the first parameter is coerced by ToNumber to NaN,
which is then coerced by ToInteger to +0, per the spec.

Test Plan:
Reading the spec should be sufficient. To verify in the Real World:

const path = "has-slash-but-does-not-need-slash/"
const a = path.substr(path, path.length - 1);
const b = path.substr(0, path.length - 1);
console.log(a === b);  // true
console.log(a);        // has-slash-but-does-not-need-slash

wchargin-branch: ensureslash-accidental-coercion

Summary:
The `hasSlash` method uses `path.substr(path, path.length - 1)` to
remove the last character from `path`. Clearly, the first parameter is
suspect; it should be `0`. The code works as written, but only very
accidentally: the first parameter is coerced by `ToNumber` to `NaN`,
which is then coerced by `ToInteger` to `+0`, per [the spec][1].

[1]: https://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.substr

Test Plan:
Reading the spec should be sufficient. To verify in the Real World:
```js
const path = "has-slash-but-does-not-need-slash/"
const a = path.substr(path, path.length - 1);
const b = path.substr(0, path.length - 1);
console.log(a === b);  // true
console.log(a);        // has-slash-but-does-not-need-slash
```

wchargin-branch: ensureslash-accidental-coercion
Timer added this to the 2.0.0 milestone May 8, 2018

Timer commented May 8, 2018

Copy link
Copy Markdown
Contributor

Wow, excellent catch and analysis. Thanks!

Timer merged commit 3aaddef into react:next May 8, 2018

wchargin commented May 8, 2018

Copy link
Copy Markdown
Contributor Author

You're welcome! Thanks for the quick response.

wchargin commented May 9, 2018

Copy link
Copy Markdown
Contributor Author

(Thanks to @decentralion for spotting this while adding Flow types to some of our code. :-) )

zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
Summary:
The `hasSlash` method uses `path.substr(path, path.length - 1)` to
remove the last character from `path`. Clearly, the first parameter is
suspect; it should be `0`. The code works as written, but only very
accidentally: the first parameter is coerced by `ToNumber` to `NaN`,
which is then coerced by `ToInteger` to `+0`, per [the spec][1].

[1]: https://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.substr

Test Plan:
Reading the spec should be sufficient. To verify in the Real World:
```js
const path = "has-slash-but-does-not-need-slash/"
const a = path.substr(path, path.length - 1);
const b = path.substr(0, path.length - 1);
console.log(a === b);  // true
console.log(a);        // has-slash-but-does-not-need-slash
```

wchargin-branch: ensureslash-accidental-coercion
lock Bot locked and limited conversation to collaborators Jan 19, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL