lib: validate argument for punycode.encode and decode #10990

hiroppy · 2017-01-25T04:43:10Z

Purpose

Unify error wording and make it easy to understand.

encode

Current behavior

punycode.encode()

argument's type	result
number	`''`
string	`''`
boolean	`''`
undefined	error
null	error
object	`''`
array	`''`
function	`''`

punycode.encode is occurred an error when the argument is undefined and null.

Error log

TypeError: Cannot read property 'length' of undefined
    at ucs2decode (punycode.js:104:23)
    at Object.encode (punycode.js:291:10)
    at repl:1:10
    at realRunInThisContextScript (vm.js:22:35)
    at sigintHandlersWrap (vm.js:98:12)
    at ContextifyScript.Script.runInThisContext (vm.js:24:12)
    at REPLServer.defaultEval (repl.js:346:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:544:10)

Modify error's wording like below.

TypeError: Argument must not be "undefined" and "null"
    at Object.encode (punycode.js:292:11)
    at repl:1:10
    at ContextifyScript.Script.runInThisContext (vm.js:23:33)
    at REPLServer.defaultEval (repl.js:340:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:537:10)
    at emitOne (events.js:101:20)
    at REPLServer.emit (events.js:189:7)
    at REPLServer.Interface._onLine (readline.js:238:10)

decode

Current behavior

punycode.decode()

argument's type	result
number	error
string	`''`
boolean	error
undefined	error
null	error
object	error
array	`''`(only empty arrays are accepted)
function	error

punycode.decode is occurred an error when the argument isn't string and array.
We accpeted only a string.

Error log

undefined, null

TypeError: Cannot read property 'length' of undefined
    at Object.decode (punycode.js:199:27)
    at repl:1:10
    at realRunInThisContextScript (vm.js:22:35)
    at sigintHandlersWrap (vm.js:98:12)
    at ContextifyScript.Script.runInThisContext (vm.js:24:12)
    at REPLServer.defaultEval (repl.js:346:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:544:10)
    at emitOne (events.js:101:20)

number, boolean, object, function

TypeError: input.lastIndexOf is not a function
    at Object.decode (punycode.js:208:20)
    at repl:1:10
    at realRunInThisContextScript (vm.js:22:35)
    at sigintHandlersWrap (vm.js:98:12)
    at ContextifyScript.Script.runInThisContext (vm.js:24:12)
    at REPLServer.defaultEval (repl.js:346:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:544:10)
    at emitOne (events.js:101:20)

Modify error's wording like below.

TypeError: Argument must be a string
    at Object.decode (punycode.js:198:11)
    at repl:1:10
    at ContextifyScript.Script.runInThisContext (vm.js:23:33)
    at REPLServer.defaultEval (repl.js:340:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:537:10)
    at emitOne (events.js:101:20)
    at REPLServer.emit (events.js:189:7)
    at REPLServer.Interface._onLine (readline.js:238:10)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

punycode, test

sam-github · 2017-01-25T17:40:20Z

Could you do this as two commits, one which adds tests asserting the current behaviour, then a follow on commit showing the change in-behaviour? Its easier to track semver-major changes when they show up as changes in the test suites.

cjihrig · 2017-01-25T17:59:12Z

Shouldn't the lib/punycode.js changes be upstreamed first?

TimothyGu · 2017-01-25T19:08:23Z

lib/punycode.js

@@ -194,6 +194,9 @@ const adapt = function(delta, numPoints, firstTime) {
 * @returns {String} The resulting string of Unicode symbols.
 */
 const decode = function(input) {
+  if (typeof input !== 'string' && !Array.isArray(input)) {
+    throw new TypeError('Argument must be a string or an array');


For what reason are arrays allowed? As far as I can tell only the empty array would work as input.

Good catch.This is my overlook...

TimothyGu · 2017-01-25T21:25:51Z

lib/punycode.js

@@ -194,6 +194,9 @@ const adapt = function(delta, numPoints, firstTime) {
 * @returns {String} The resulting string of Unicode symbols.
 */
 const decode = function(input) {
+  if (!(typeof input === 'string' || Array.isArray(input) && input.length === 0)) {
+    throw new TypeError('Argument must be a string or an empty array');


Sorry for not being clear, but why are any arrays allowed? The documentation says only strings are allowed.

Currently, arrays are allowed behavior so I accepted it.
Can we disable it?
I think decode should be changed to accept only strings.

Yes, I agree.

OK, I accepted only a string.

hiroppy · 2017-01-26T22:47:45Z

@TimothyGu Updated, PTAL.

cjihrig · 2017-01-27T15:29:13Z

Just reiterating. Shouldn't the changes to lib/punycode.js be upstreamed first? Or, do we not take that file as is anymore?

Unify error wording and make it easy to understand. `punycode.encode` is occurred an error when the argument is `undefined` and `null`. `punycode.decode` is occurred an error when the argument isn't `string` and `array`. We accepted only a string.

hiroppy · 2017-01-27T15:58:31Z

@cjihrig Oops, sorry. Is this ok?

cjihrig · 2017-01-27T16:14:52Z

Historically, we have taken punycode.js as is from upstream and didn't really maintain or change it at all. If that is still the case, then I don't think we should make modifications. If that isn't the case anymore, then I have no strong opinion.

gibfahn · 2017-01-27T16:46:57Z

Just reiterating. Shouldn't the changes to lib/punycode.js be upstreamed first? Or, do we not take that file as is anymore?

Guessing from the history of that file, maybe @mathiasbynens ?

mathiasbynens · 2017-01-27T19:00:44Z

I can confirm that until now Node.js just used the upstream Punycode.js version. What if I don’t want those changes in Punycode.js, though? Would that be a dealbreaker for Node?

jasnell

I'm -1 on this. The punycode module is a vendored in dependency. This change, if made at all, should go into the upstream version. Also consider that the punycode module is considered to be docs-deprecated. Users should be moving to the userland version.

joyeecheung · 2017-02-06T07:00:43Z

Just curious: why isn't punycode.js in deps and then forwarded to userland in lib?

addaleax · 2017-02-06T07:52:05Z

Just curious: why isn't punycode.js in deps and then forwarded to userland in lib?

Yeah, we should do that. I can open a PR later this week if nobody else does. :)

jasnell · 2017-05-01T19:19:41Z

Closing this. Can discuss reopening if necessary.

nodejs-github-bot added the dont-land-on-v7.x label Jan 25, 2017

mscdex added punycode Issues and PRs related to the punycode module bundled in Node.js. and removed dont-land-on-v7.x labels Jan 25, 2017

sam-github added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 25, 2017

TimothyGu reviewed Jan 25, 2017

View reviewed changes

hiroppy force-pushed the feature/add-typeerror-to-punycode branch from 9433fce to ad37922 Compare January 25, 2017 20:42

nodejs-github-bot added the dont-land-on-v7.x label Jan 25, 2017

TimothyGu reviewed Jan 25, 2017

View reviewed changes

hiroppy force-pushed the feature/add-typeerror-to-punycode branch from ad37922 to c6596d7 Compare January 26, 2017 22:45

nodejs-github-bot added the dont-land-on-v7.x label Jan 26, 2017

hiroppy force-pushed the feature/add-typeerror-to-punycode branch from c6596d7 to a971d0e Compare January 26, 2017 22:50

nodejs-github-bot added the dont-land-on-v7.x label Jan 26, 2017

TimothyGu approved these changes Jan 27, 2017

View reviewed changes

hiroppy added 2 commits January 28, 2017 00:56

test: add exception tests to punycode

463f756

hiroppy force-pushed the feature/add-typeerror-to-punycode branch from a971d0e to 463f756 Compare January 27, 2017 15:58

nodejs-github-bot removed the dont-land-on-v7.x label Jan 27, 2017

jasnell requested changes Jan 27, 2017

View reviewed changes

joyeecheung mentioned this pull request Feb 6, 2017

test: improve punycode test coverage #11144

Closed

3 tasks

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

jasnell closed this May 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: validate argument for punycode.encode and decode #10990

lib: validate argument for punycode.encode and decode #10990

hiroppy commented Jan 25, 2017 •

edited

Loading

sam-github commented Jan 25, 2017

cjihrig commented Jan 25, 2017

TimothyGu Jan 25, 2017

hiroppy Jan 25, 2017

TimothyGu Jan 25, 2017

hiroppy Jan 26, 2017 •

edited

Loading

TimothyGu Jan 26, 2017

hiroppy Jan 26, 2017

hiroppy commented Jan 26, 2017

cjihrig commented Jan 27, 2017

hiroppy commented Jan 27, 2017

cjihrig commented Jan 27, 2017

gibfahn commented Jan 27, 2017

mathiasbynens commented Jan 27, 2017

jasnell left a comment

joyeecheung commented Feb 6, 2017

addaleax commented Feb 6, 2017

jasnell commented May 1, 2017

lib: validate argument for punycode.encode and decode #10990

lib: validate argument for punycode.encode and decode #10990

Conversation

hiroppy commented Jan 25, 2017 • edited Loading

Purpose

encode

Current behavior

Error log

decode

Current behavior

Error log

Checklist

Affected core subsystem(s)

sam-github commented Jan 25, 2017

cjihrig commented Jan 25, 2017

TimothyGu Jan 25, 2017

Choose a reason for hiding this comment

hiroppy Jan 25, 2017

Choose a reason for hiding this comment

TimothyGu Jan 25, 2017

Choose a reason for hiding this comment

hiroppy Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

TimothyGu Jan 26, 2017

Choose a reason for hiding this comment

hiroppy Jan 26, 2017

Choose a reason for hiding this comment

hiroppy commented Jan 26, 2017

cjihrig commented Jan 27, 2017

hiroppy commented Jan 27, 2017

cjihrig commented Jan 27, 2017

gibfahn commented Jan 27, 2017

mathiasbynens commented Jan 27, 2017

jasnell left a comment

Choose a reason for hiding this comment

joyeecheung commented Feb 6, 2017

addaleax commented Feb 6, 2017

jasnell commented May 1, 2017

hiroppy commented Jan 25, 2017 •

edited

Loading

hiroppy Jan 26, 2017 •

edited

Loading