From df15ba38785c9bafd820efe42e4d5487a21246f5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 16 Mar 2019 13:21:25 +0100 Subject: [PATCH 1/2] src: disallow constructor behaviour for native methods Disallow constructor behaviour and setting up prototypes for native methods that are not constructors (i.e. make them behave like ES6 class methods). --- src/env-inl.h | 8 ++------ test/parallel/test-process-chdir.js | 8 ++++++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index aff12e57bf2418..51605eb335d732 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -875,9 +875,7 @@ inline void Environment::SetMethod(v8::Local that, v8::Local context = isolate()->GetCurrentContext(); v8::Local function = NewFunctionTemplate(callback, v8::Local(), - // TODO(TimothyGu): Investigate if SetMethod is ever - // used for constructors. - v8::ConstructorBehavior::kAllow, + v8::ConstructorBehavior::kThrow, v8::SideEffectType::kHasSideEffect) ->GetFunction(context) .ToLocalChecked(); @@ -895,9 +893,7 @@ inline void Environment::SetMethodNoSideEffect(v8::Local that, v8::Local context = isolate()->GetCurrentContext(); v8::Local function = NewFunctionTemplate(callback, v8::Local(), - // TODO(TimothyGu): Investigate if SetMethod is ever - // used for constructors. - v8::ConstructorBehavior::kAllow, + v8::ConstructorBehavior::kThrow, v8::SideEffectType::kHasNoSideEffect) ->GetFunction(context) .ToLocalChecked(); diff --git a/test/parallel/test-process-chdir.js b/test/parallel/test-process-chdir.js index e66d366fb7874f..c1b0daaee6ec22 100644 --- a/test/parallel/test-process-chdir.js +++ b/test/parallel/test-process-chdir.js @@ -42,3 +42,11 @@ const err = { }; common.expectsError(function() { process.chdir({}); }, err); common.expectsError(function() { process.chdir(); }, err); + +// Check that our built-in methods do not have a prototype/constructor behaviour +// if they don't need to. This could be tested for any of our C++ methods. +assert.strictEqual(process.cwd.prototype, undefined); +assert.throws(() => new process.cwd(), /not a constructor/); +// Make sure that the above checks verify the right thing, i.e. that we're +// actually looking at a directly exposed C++ binding method. +assert.strictEqual(process.cwd.toString(), 'function cwd() { [native code] }'); From af88f4157e8eea68e994018c755e021beb001f25 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 17 Mar 2019 01:30:08 +0100 Subject: [PATCH 2/2] fixup! src: disallow constructor behaviour for native methods --- test/parallel/test-process-chdir.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/parallel/test-process-chdir.js b/test/parallel/test-process-chdir.js index c1b0daaee6ec22..005e17fac25a90 100644 --- a/test/parallel/test-process-chdir.js +++ b/test/parallel/test-process-chdir.js @@ -46,7 +46,4 @@ common.expectsError(function() { process.chdir(); }, err); // Check that our built-in methods do not have a prototype/constructor behaviour // if they don't need to. This could be tested for any of our C++ methods. assert.strictEqual(process.cwd.prototype, undefined); -assert.throws(() => new process.cwd(), /not a constructor/); -// Make sure that the above checks verify the right thing, i.e. that we're -// actually looking at a directly exposed C++ binding method. -assert.strictEqual(process.cwd.toString(), 'function cwd() { [native code] }'); +assert.throws(() => new process.cwd(), TypeError);