From 8a80bf7f4d2b0bc54dc9fad93527c6eaa9ce2d9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Sun, 15 Apr 2018 11:16:50 +0200 Subject: [PATCH 1/3] lib: introduce internal/validators Create a file to centralize argument validators that are used in multiple internal modules. Move validateInt32 and validateUint32 to this file. --- lib/fs.js | 16 +++--- lib/fs/promises.js | 12 +++-- lib/internal/fs.js | 45 +--------------- lib/internal/validators.js | 58 +++++++++++++++++++++ lib/internal/vm/module.js | 20 ++----- lib/vm.js | 24 +++------ node.gyp | 1 + test/parallel/test-vm-options-validation.js | 35 +++++++++---- 8 files changed, 112 insertions(+), 99 deletions(-) create mode 100644 lib/internal/validators.js diff --git a/lib/fs.js b/lib/fs.js index 6c8417030b7ac7..a8cd8a6260c081 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -51,7 +51,6 @@ const internalUtil = require('internal/util'); const { copyObject, getOptions, - isUint32, modeNum, nullCheck, preprocessSymlinkDestination, @@ -62,16 +61,19 @@ const { stringToSymlinkType, toUnixTimestamp, validateBuffer, - validateLen, validateOffsetLengthRead, validateOffsetLengthWrite, - validatePath, - validateUint32 + validatePath } = internalFS; const { CHAR_FORWARD_SLASH, CHAR_BACKWARD_SLASH, } = require('internal/constants'); +const { + isUint32, + validateInt32, + validateUint32 +} = require('internal/validators'); Object.defineProperty(exports, 'constants', { configurable: false, @@ -755,8 +757,8 @@ fs.ftruncate = function(fd, len = 0, callback) { // TODO(BridgeAR): This does not seem right. // There does not seem to be any validation before and if there is any, it // should work similar to validateUint32 or not have a upper cap at all. - // This applies to all usage of `validateLen`. - validateLen(len); + // This applies to all usage of `validateInt32(len, 'len')`. + validateInt32(len, 'len'); len = Math.max(0, len); const req = new FSReqWrap(); req.oncomplete = makeCallback(callback); @@ -765,7 +767,7 @@ fs.ftruncate = function(fd, len = 0, callback) { fs.ftruncateSync = function(fd, len = 0) { validateUint32(fd, 'fd'); - validateLen(len); + validateInt32(len, 'len'); len = Math.max(0, len); const ctx = {}; binding.ftruncate(fd, len, undefined, ctx); diff --git a/lib/fs/promises.js b/lib/fs/promises.js index ba6c2b7aa64855..6ccff6933bf106 100644 --- a/lib/fs/promises.js +++ b/lib/fs/promises.js @@ -24,7 +24,6 @@ const { copyObject, getOptions, getStatsFromBinding, - isUint32, modeNum, nullCheck, preprocessSymlinkDestination, @@ -32,12 +31,15 @@ const { stringToSymlinkType, toUnixTimestamp, validateBuffer, - validateLen, validateOffsetLengthRead, validateOffsetLengthWrite, - validatePath, - validateUint32 + validatePath } = require('internal/fs'); +const { + isUint32, + validateInt32, + validateUint32 +} = require('internal/validators'); const pathModule = require('path'); const kHandle = Symbol('handle'); @@ -275,7 +277,7 @@ async function truncate(path, len = 0) { async function ftruncate(handle, len = 0) { validateFileHandle(handle); - validateLen(len); + validateInt32(len, 'len'); len = Math.max(0, len); return binding.ftruncate(handle.fd, len, kUsePromises); } diff --git a/lib/internal/fs.js b/lib/internal/fs.js index b431cb910c2506..8a1d30b300fd8a 100644 --- a/lib/internal/fs.js +++ b/lib/internal/fs.js @@ -71,9 +71,6 @@ function getOptions(options, defaultOptions) { return options; } -function isInt32(n) { return n === (n | 0); } -function isUint32(n) { return n === (n >>> 0); } - function modeNum(m, def) { if (typeof m === 'number') return m; @@ -346,26 +343,6 @@ function validateBuffer(buffer) { } } -function validateLen(len) { - let err; - - if (!isInt32(len)) { - if (typeof len !== 'number') { - err = new ERR_INVALID_ARG_TYPE('len', 'number', len); - } else if (!Number.isInteger(len)) { - err = new ERR_OUT_OF_RANGE('len', 'an integer', len); - } else { - // 2 ** 31 === 2147483648 - err = new ERR_OUT_OF_RANGE('len', '> -2147483649 && < 2147483648', len); - } - } - - if (err !== undefined) { - Error.captureStackTrace(err, validateLen); - throw err; - } -} - function validateOffsetLengthRead(offset, length, bufferLength) { let err; @@ -415,28 +392,10 @@ function validatePath(path, propName = 'path') { } } -function validateUint32(value, propName) { - if (!isUint32(value)) { - let err; - if (typeof value !== 'number') { - err = new ERR_INVALID_ARG_TYPE(propName, 'number', value); - } else if (!Number.isInteger(value)) { - err = new ERR_OUT_OF_RANGE(propName, 'an integer', value); - } else { - // 2 ** 32 === 4294967296 - err = new ERR_OUT_OF_RANGE(propName, '>= 0 && < 4294967296', value); - } - Error.captureStackTrace(err, validateUint32); - throw err; - } -} - module.exports = { assertEncoding, copyObject, getOptions, - isInt32, - isUint32, modeNum, nullCheck, preprocessSymlinkDestination, @@ -449,9 +408,7 @@ module.exports = { SyncWriteStream, toUnixTimestamp, validateBuffer, - validateLen, validateOffsetLengthRead, validateOffsetLengthWrite, - validatePath, - validateUint32 + validatePath }; diff --git a/lib/internal/validators.js b/lib/internal/validators.js new file mode 100644 index 00000000000000..556bfb2dc08f5f --- /dev/null +++ b/lib/internal/validators.js @@ -0,0 +1,58 @@ +'use strict'; + +const { + ERR_INVALID_ARG_TYPE, + ERR_OUT_OF_RANGE +} = require('internal/errors').codes; + +function isInt32(value) { + return value === (value | 0); +} + +function isUint32(value) { + return value === (value >>> 0); +} + +function validateInt32(value, name) { + if (!isInt32(value)) { + let err; + if (typeof value !== 'number') { + err = new ERR_INVALID_ARG_TYPE(name, 'number', value); + } else if (!Number.isInteger(value)) { + err = new ERR_OUT_OF_RANGE(name, 'an integer', value); + } else { + // 2 ** 31 === 2147483648 + err = new ERR_OUT_OF_RANGE(name, '> -2147483649 && < 2147483648', value); + } + Error.captureStackTrace(err, validateInt32); + throw err; + } +} + +function validateUint32(value, name, positive) { + if (!isUint32(value)) { + let err; + if (typeof value !== 'number') { + err = new ERR_INVALID_ARG_TYPE(name, 'number', value); + } else if (!Number.isInteger(value)) { + err = new ERR_OUT_OF_RANGE(name, 'an integer', value); + } else { + const min = positive ? 1 : 0; + // 2 ** 32 === 4294967296 + err = new ERR_OUT_OF_RANGE(name, `>= ${min} && < 4294967296`, value); + } + Error.captureStackTrace(err, validateUint32); + throw err; + } else if (positive && value === 0) { + const err = new ERR_OUT_OF_RANGE(name, '>= 1 && < 4294967296', value); + Error.captureStackTrace(err, validateUint32); + throw err; + } +} + +module.exports = { + isInt32, + isUint32, + validateInt32, + validateUint32 +}; diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index 7284c8bd619901..9e945c45c3de8a 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -6,7 +6,6 @@ const { URL } = require('internal/url'); const { isContext } = process.binding('contextify'); const { ERR_INVALID_ARG_TYPE, - ERR_OUT_OF_RANGE, ERR_VM_MODULE_ALREADY_LINKED, ERR_VM_MODULE_DIFFERENT_CONTEXT, ERR_VM_MODULE_LINKING_ERRORED, @@ -19,6 +18,7 @@ const { customInspectSymbol, } = require('internal/util'); const { SafePromise } = require('internal/safe_globals'); +const { validateInt32, validateUint32 } = require('internal/validators'); const { ModuleWrap, @@ -92,8 +92,8 @@ class Module { perContextModuleId.set(context, 1); } - validateInteger(lineOffset, 'options.lineOffset'); - validateInteger(columnOffset, 'options.columnOffset'); + validateInt32(lineOffset, 'options.lineOffset'); + validateInt32(columnOffset, 'options.columnOffset'); if (initializeImportMeta !== undefined) { if (typeof initializeImportMeta === 'function') { @@ -203,9 +203,8 @@ class Module { let timeout = options.timeout; if (timeout === undefined) { timeout = -1; - } else if (!Number.isInteger(timeout) || timeout <= 0) { - throw new ERR_INVALID_ARG_TYPE('options.timeout', 'a positive integer', - timeout); + } else { + validateUint32(timeout, 'options.timeout', true); } const { breakOnSigint = false } = options; @@ -243,15 +242,6 @@ class Module { } } -function validateInteger(prop, propName) { - if (!Number.isInteger(prop)) { - throw new ERR_INVALID_ARG_TYPE(propName, 'integer', prop); - } - if ((prop >> 0) !== prop) { - throw new ERR_OUT_OF_RANGE(propName, '32-bit integer', prop); - } -} - module.exports = { Module, initImportMetaMap, diff --git a/lib/vm.js b/lib/vm.js index 5a5130d7c9c328..3ab50c81580365 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -28,11 +28,9 @@ const { isContext: _isContext, } = process.binding('contextify'); -const { - ERR_INVALID_ARG_TYPE, - ERR_OUT_OF_RANGE -} = require('internal/errors').codes; +const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes; const { isUint8Array } = require('internal/util/types'); +const { validateInt32, validateUint32 } = require('internal/validators'); class Script extends ContextifyScript { constructor(code, options = {}) { @@ -56,8 +54,8 @@ class Script extends ContextifyScript { if (typeof filename !== 'string') { throw new ERR_INVALID_ARG_TYPE('options.filename', 'string', filename); } - validateInteger(lineOffset, 'options.lineOffset'); - validateInteger(columnOffset, 'options.columnOffset'); + validateInt32(lineOffset, 'options.lineOffset'); + validateInt32(columnOffset, 'options.columnOffset'); if (cachedData !== undefined && !isUint8Array(cachedData)) { throw new ERR_INVALID_ARG_TYPE('options.cachedData', ['Buffer', 'Uint8Array'], cachedData); @@ -119,15 +117,6 @@ function validateContext(sandbox) { } } -function validateInteger(prop, propName) { - if (!Number.isInteger(prop)) { - throw new ERR_INVALID_ARG_TYPE(propName, 'integer', prop); - } - if ((prop >> 0) !== prop) { - throw new ERR_OUT_OF_RANGE(propName, '32-bit integer', prop); - } -} - function validateString(prop, propName) { if (prop !== undefined && typeof prop !== 'string') throw new ERR_INVALID_ARG_TYPE(propName, 'string', prop); @@ -151,9 +140,8 @@ function getRunInContextArgs(options = {}) { let timeout = options.timeout; if (timeout === undefined) { timeout = -1; - } else if (!Number.isInteger(timeout) || timeout <= 0) { - throw new ERR_INVALID_ARG_TYPE('options.timeout', 'a positive integer', - timeout); + } else { + validateUint32(timeout, 'options.timeout', true); } const { diff --git a/node.gyp b/node.gyp index 15475f689e3295..8e48f8956f8323 100644 --- a/node.gyp +++ b/node.gyp @@ -146,6 +146,7 @@ 'lib/internal/v8.js', 'lib/internal/v8_prof_polyfill.js', 'lib/internal/v8_prof_processor.js', + 'lib/internal/validators.js', 'lib/internal/stream_base_commons.js', 'lib/internal/vm/module.js', 'lib/internal/streams/lazy_transform.js', diff --git a/test/parallel/test-vm-options-validation.js b/test/parallel/test-vm-options-validation.js index e00a14e4b9e603..67a27d58fff805 100644 --- a/test/parallel/test-vm-options-validation.js +++ b/test/parallel/test-vm-options-validation.js @@ -17,7 +17,7 @@ common.expectsError(() => { new vm.Script('void 0', 42); }, invalidArgType); -[null, {}, [1], 'bad', true, 0.1].forEach((value) => { +[null, {}, [1], 'bad', true].forEach((value) => { common.expectsError(() => { new vm.Script('void 0', { lineOffset: value }); }, invalidArgType); @@ -27,6 +27,16 @@ common.expectsError(() => { }, invalidArgType); }); +[0.1, 2 ** 32].forEach((value) => { + common.expectsError(() => { + new vm.Script('void 0', { lineOffset: value }); + }, outOfRange); + + common.expectsError(() => { + new vm.Script('void 0', { columnOffset: value }); + }, outOfRange); +}); + common.expectsError(() => { new vm.Script('void 0', { lineOffset: Number.MAX_SAFE_INTEGER }); }, outOfRange); @@ -53,26 +63,31 @@ common.expectsError(() => { const script = new vm.Script('void 0'); const sandbox = vm.createContext(); - function assertErrors(options) { + function assertErrors(options, errCheck) { common.expectsError(() => { script.runInThisContext(options); - }, invalidArgType); + }, errCheck); common.expectsError(() => { script.runInContext(sandbox, options); - }, invalidArgType); + }, errCheck); common.expectsError(() => { script.runInNewContext({}, options); - }, invalidArgType); + }, errCheck); } - [null, 'bad', 42].forEach(assertErrors); - [{}, [1], 'bad', null, -1, 0, NaN].forEach((value) => { - assertErrors({ timeout: value }); + [null, 'bad', 42].forEach((value) => { + assertErrors(value, invalidArgType); + }); + [{}, [1], 'bad', null].forEach((value) => { + assertErrors({ timeout: value }, invalidArgType); + }); + [-1, 0, NaN].forEach((value) => { + assertErrors({ timeout: value }, outOfRange); }); [{}, [1], 'bad', 1, null].forEach((value) => { - assertErrors({ displayErrors: value }); - assertErrors({ breakOnSigint: value }); + assertErrors({ displayErrors: value }, invalidArgType); + assertErrors({ breakOnSigint: value }, invalidArgType); }); } From 0ea59295a9d3b6f8481f67854b35fe5a64804620 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 12 Apr 2018 11:54:19 +0200 Subject: [PATCH 2/3] process: migrate methods to throw errors with code Migrate some methods from node.cc to JS in order to properly throw errors with codes. --- doc/api/errors.md | 5 + lib/internal/bootstrap/node.js | 1 + lib/internal/errors.js | 1 + lib/internal/process/methods.js | 126 ++++++++++++++++++ node.gyp | 1 + src/node.cc | 126 ++++++++---------- test/parallel/test-process-chdir.js | 15 +-- test/parallel/test-process-euid-egid.js | 19 ++- test/parallel/test-process-uid-gid.js | 17 ++- .../{test-umask.js => test-process-umask.js} | 17 ++- 10 files changed, 235 insertions(+), 93 deletions(-) create mode 100644 lib/internal/process/methods.js rename test/parallel/{test-umask.js => test-process-umask.js} (82%) diff --git a/doc/api/errors.md b/doc/api/errors.md index 84a74c51e05093..eaa4dda5293021 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1597,6 +1597,11 @@ A string that contained unescaped characters was received. An unhandled error occurred (for instance, when an `'error'` event is emitted by an [`EventEmitter`][] but an `'error'` handler is not registered). + +### ERR_UNKNOWN_CREDENTIAL + +A Unix group or user identifier that does not exist was passed. + ### ERR_UNKNOWN_ENCODING diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index b74c194d07bab5..81db24be25603f 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -42,6 +42,7 @@ NativeModule.require('internal/process/warning').setup(); NativeModule.require('internal/process/next_tick').setup(); NativeModule.require('internal/process/stdio').setup(); + NativeModule.require('internal/process/methods').setup(); const perf = process.binding('performance'); const { diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 4823f0b65cb6ea..46f11467f05f9e 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1007,6 +1007,7 @@ E('ERR_UNHANDLED_ERROR', if (err === undefined) return msg; return `${msg} (${err})`; }, Error); +E('ERR_UNKNOWN_CREDENTIAL', '%s identifier does not exist: %s', Error); E('ERR_UNKNOWN_ENCODING', 'Unknown encoding: %s', TypeError); // This should probably be a `TypeError`. diff --git a/lib/internal/process/methods.js b/lib/internal/process/methods.js new file mode 100644 index 00000000000000..33fcac14b8e178 --- /dev/null +++ b/lib/internal/process/methods.js @@ -0,0 +1,126 @@ +'use strict'; + +const { + ERR_INVALID_ARG_TYPE, + ERR_INVALID_ARG_VALUE, + ERR_UNKNOWN_CREDENTIAL +} = require('internal/errors').codes; +const { + validateUint32 +} = require('internal/validators'); + +function setupProcessMethods() { + const { + chdir: _chdir, + initgroups: _initgroups, + setegid: _setegid, + seteuid: _seteuid, + setgid: _setgid, + setuid: _setuid, + setgroups: _setgroups, + umask: _umask, + } = process; + + process.chdir = chdir; + process.initgroups = initgroups; + process.setegid = setegid; + process.seteuid = seteuid; + process.setgid = setgid; + process.setuid = setuid; + process.setgroups = setgroups; + process.umask = umask; + + function chdir(directory) { + if (typeof directory !== 'string') { + throw new ERR_INVALID_ARG_TYPE('directory', 'string', directory); + } + return _chdir(directory); + } + + function initgroups(user, extraGroup) { + validateId(user, 'user'); + validateId(extraGroup, 'extraGroup'); + // Result is 0 on success, 1 if user is unknown, 2 if group is unknown. + const result = _initgroups(user, extraGroup); + if (result === 1) { + throw new ERR_UNKNOWN_CREDENTIAL('User', user); + } else if (result === 2) { + throw new ERR_UNKNOWN_CREDENTIAL('Group', extraGroup); + } + } + + function setegid(id) { + return execId(id, 'Group', _setegid); + } + + function seteuid(id) { + return execId(id, 'User', _seteuid); + } + + function setgid(id) { + return execId(id, 'Group', _setgid); + } + + function setuid(id) { + return execId(id, 'User', _setuid); + } + + function setgroups(groups) { + if (!Array.isArray(groups)) { + throw new ERR_INVALID_ARG_TYPE('groups', 'Array', groups); + } + for (var i = 0; i < groups.length; i++) { + validateId(groups[i], `groups[${i}]`); + } + // Result is 0 on success. A positive integer indicates that the + // corresponding group was not found. + const result = _setgroups(groups); + if (result > 0) { + throw new ERR_UNKNOWN_CREDENTIAL('Group', groups[result - 1]); + } + } + + const octalReg = /^[0-7]+$/; + function umask(mask) { + if (typeof mask === 'undefined') { + return _umask(mask); + } + + if (typeof mask === 'number') { + validateUint32(mask, 'mask'); + return _umask(mask); + } + + if (typeof mask === 'string') { + if (!octalReg.test(mask)) { + throw new ERR_INVALID_ARG_VALUE('mask', mask, + 'must be an octal string'); + } + const octal = Number.parseInt(mask, 8); + validateUint32(octal, 'mask'); + return _umask(octal); + } + + throw new ERR_INVALID_ARG_TYPE('mask', ['number', 'string', 'undefined'], + mask); + } + + function execId(id, type, method) { + validateId(id, 'id'); + // Result is 0 on success, 1 if credential is unknown. + const result = method(id); + if (result === 1) { + throw new ERR_UNKNOWN_CREDENTIAL(type, id); + } + } + + function validateId(id, name) { + if (typeof id === 'number') { + validateUint32(id, name); + } else if (typeof id !== 'string') { + throw new ERR_INVALID_ARG_TYPE(name, ['number', 'string'], id); + } + } +} + +exports.setup = setupProcessMethods; diff --git a/node.gyp b/node.gyp index 8e48f8956f8323..be88dd48060a28 100644 --- a/node.gyp +++ b/node.gyp @@ -118,6 +118,7 @@ 'lib/internal/net.js', 'lib/internal/os.js', 'lib/internal/process/esm_loader.js', + 'lib/internal/process/methods.js', 'lib/internal/process/next_tick.js', 'lib/internal/process/promises.js', 'lib/internal/process/stdio.js', diff --git a/src/node.cc b/src/node.cc index 6acad6870d81da..e6ce1493831102 100644 --- a/src/node.cc +++ b/src/node.cc @@ -164,6 +164,7 @@ using v8::ScriptOrigin; using v8::SealHandleScope; using v8::String; using v8::TryCatch; +using v8::Uint32; using v8::Uint32Array; using v8::Undefined; using v8::V8; @@ -1580,10 +1581,8 @@ static void Abort(const FunctionCallbackInfo& args) { static void Chdir(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() != 1 || !args[0]->IsString()) { - return env->ThrowTypeError("Bad argument."); - } - + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsString()); node::Utf8Value path(args.GetIsolate(), args[0]); int err = uv_chdir(*path); if (err) { @@ -1616,32 +1615,16 @@ static void Cwd(const FunctionCallbackInfo& args) { static void Umask(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); uint32_t old; - if (args.Length() < 1 || args[0]->IsUndefined()) { + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsUndefined() || args[0]->IsUint32()); + + if (args[0]->IsUndefined()) { old = umask(0); umask(static_cast(old)); - } else if (!args[0]->IsInt32() && !args[0]->IsString()) { - return env->ThrowTypeError("argument must be an integer or octal string."); } else { - int oct; - if (args[0]->IsInt32()) { - oct = args[0]->Uint32Value(); - } else { - oct = 0; - node::Utf8Value str(env->isolate(), args[0]); - - // Parse the octal string. - for (size_t i = 0; i < str.length(); i++) { - char c = (*str)[i]; - if (c > '7' || c < '0') { - return env->ThrowTypeError("invalid octal string"); - } - oct *= 8; - oct += c - '0'; - } - } + int oct = args[0].As()->Value(); old = umask(static_cast(oct)); } @@ -1779,18 +1762,18 @@ static void GetEGid(const FunctionCallbackInfo& args) { static void SetGid(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (!args[0]->IsUint32() && !args[0]->IsString()) { - return env->ThrowTypeError("setgid argument must be a number or a string"); - } + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsUint32() || args[0]->IsString()); gid_t gid = gid_by_name(env->isolate(), args[0]); if (gid == gid_not_found) { - return env->ThrowError("setgid group id does not exist"); - } - - if (setgid(gid)) { - return env->ThrowErrnoException(errno, "setgid"); + // Tells JS to throw ERR_INVALID_CREDENTIAL + args.GetReturnValue().Set(1); + } else if (setgid(gid)) { + env->ThrowErrnoException(errno, "setgid"); + } else { + args.GetReturnValue().Set(0); } } @@ -1798,18 +1781,18 @@ static void SetGid(const FunctionCallbackInfo& args) { static void SetEGid(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (!args[0]->IsUint32() && !args[0]->IsString()) { - return env->ThrowTypeError("setegid argument must be a number or string"); - } + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsUint32() || args[0]->IsString()); gid_t gid = gid_by_name(env->isolate(), args[0]); if (gid == gid_not_found) { - return env->ThrowError("setegid group id does not exist"); - } - - if (setegid(gid)) { - return env->ThrowErrnoException(errno, "setegid"); + // Tells JS to throw ERR_INVALID_CREDENTIAL + args.GetReturnValue().Set(1); + } else if (setegid(gid)) { + env->ThrowErrnoException(errno, "setegid"); + } else { + args.GetReturnValue().Set(0); } } @@ -1817,18 +1800,18 @@ static void SetEGid(const FunctionCallbackInfo& args) { static void SetUid(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (!args[0]->IsUint32() && !args[0]->IsString()) { - return env->ThrowTypeError("setuid argument must be a number or a string"); - } + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsUint32() || args[0]->IsString()); uid_t uid = uid_by_name(env->isolate(), args[0]); if (uid == uid_not_found) { - return env->ThrowError("setuid user id does not exist"); - } - - if (setuid(uid)) { - return env->ThrowErrnoException(errno, "setuid"); + // Tells JS to throw ERR_INVALID_CREDENTIAL + args.GetReturnValue().Set(1); + } else if (setuid(uid)) { + env->ThrowErrnoException(errno, "setuid"); + } else { + args.GetReturnValue().Set(0); } } @@ -1836,18 +1819,18 @@ static void SetUid(const FunctionCallbackInfo& args) { static void SetEUid(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (!args[0]->IsUint32() && !args[0]->IsString()) { - return env->ThrowTypeError("seteuid argument must be a number or string"); - } + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsUint32() || args[0]->IsString()); uid_t uid = uid_by_name(env->isolate(), args[0]); if (uid == uid_not_found) { - return env->ThrowError("seteuid user id does not exist"); - } - - if (seteuid(uid)) { - return env->ThrowErrnoException(errno, "seteuid"); + // Tells JS to throw ERR_INVALID_CREDENTIAL + args.GetReturnValue().Set(1); + } else if (seteuid(uid)) { + env->ThrowErrnoException(errno, "seteuid"); + } else { + args.GetReturnValue().Set(0); } } @@ -1893,9 +1876,8 @@ static void GetGroups(const FunctionCallbackInfo& args) { static void SetGroups(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (!args[0]->IsArray()) { - return env->ThrowTypeError("argument 1 must be an array"); - } + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsArray()); Local groups_list = args[0].As(); size_t size = groups_list->Length(); @@ -1906,7 +1888,9 @@ static void SetGroups(const FunctionCallbackInfo& args) { if (gid == gid_not_found) { delete[] groups; - return env->ThrowError("group name not found"); + // Tells JS to throw ERR_INVALID_CREDENTIAL + args.GetReturnValue().Set(static_cast(i + 1)); + return; } groups[i] = gid; @@ -1918,19 +1902,17 @@ static void SetGroups(const FunctionCallbackInfo& args) { if (rc == -1) { return env->ThrowErrnoException(errno, "setgroups"); } + + args.GetReturnValue().Set(0); } static void InitGroups(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (!args[0]->IsUint32() && !args[0]->IsString()) { - return env->ThrowTypeError("argument 1 must be a number or a string"); - } - - if (!args[1]->IsUint32() && !args[1]->IsString()) { - return env->ThrowTypeError("argument 2 must be a number or a string"); - } + CHECK_EQ(args.Length(), 2); + CHECK(args[0]->IsUint32() || args[0]->IsString()); + CHECK(args[1]->IsUint32() || args[1]->IsString()); node::Utf8Value arg0(env->isolate(), args[0]); gid_t extra_group; @@ -1946,7 +1928,8 @@ static void InitGroups(const FunctionCallbackInfo& args) { } if (user == nullptr) { - return env->ThrowError("initgroups user not found"); + // Tells JS to throw ERR_INVALID_CREDENTIAL + return args.GetReturnValue().Set(1); } extra_group = gid_by_name(env->isolate(), args[1]); @@ -1954,7 +1937,8 @@ static void InitGroups(const FunctionCallbackInfo& args) { if (extra_group == gid_not_found) { if (must_free) free(user); - return env->ThrowError("initgroups extra group not found"); + // Tells JS to throw ERR_INVALID_CREDENTIAL + return args.GetReturnValue().Set(2); } int rc = initgroups(user, extra_group); @@ -1966,6 +1950,8 @@ static void InitGroups(const FunctionCallbackInfo& args) { if (rc) { return env->ThrowErrnoException(errno, "initgroups"); } + + args.GetReturnValue().Set(0); } #endif // __POSIX__ && !defined(__ANDROID__) && !defined(__CloudABI__) diff --git a/test/parallel/test-process-chdir.js b/test/parallel/test-process-chdir.js index c0a245ffd3483b..998147dd43dfc8 100644 --- a/test/parallel/test-process-chdir.js +++ b/test/parallel/test-process-chdir.js @@ -1,6 +1,6 @@ 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const fs = require('fs'); const path = require('path'); @@ -33,10 +33,9 @@ process.chdir('..'); assert.strictEqual(process.cwd().normalize(), path.resolve(tmpdir.path).normalize()); -const errMessage = /^TypeError: Bad argument\.$/; -assert.throws(function() { process.chdir({}); }, - errMessage, 'Bad argument.'); -assert.throws(function() { process.chdir(); }, - errMessage, 'Bad argument.'); -assert.throws(function() { process.chdir('x', 'y'); }, - errMessage, 'Bad argument.'); +const err = { + code: 'ERR_INVALID_ARG_TYPE', + message: /The "directory" argument must be of type string/ +}; +common.expectsError(function() { process.chdir({}); }, err); +common.expectsError(function() { process.chdir(); }, err); diff --git a/test/parallel/test-process-euid-egid.js b/test/parallel/test-process-euid-egid.js index adae58abebdfb7..84fbd03e327b05 100644 --- a/test/parallel/test-process-euid-egid.js +++ b/test/parallel/test-process-euid-egid.js @@ -13,11 +13,18 @@ if (common.isWindows) { assert.throws(() => { process.seteuid({}); -}, /^TypeError: seteuid argument must be a number or string$/); +}, { + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "id" argument must be one of type number or string. ' + + 'Received type object' +}); assert.throws(() => { - process.seteuid('fhqwhgadshgnsdhjsdbkhsdabkfabkveybvf'); -}, /^Error: seteuid user id does not exist$/); + process.seteuid('fhqwhgadshgnsdhjsdbkhsdabkfabkveyb'); +}, { + code: 'ERR_UNKNOWN_CREDENTIAL', + message: 'User identifier does not exist: fhqwhgadshgnsdhjsdbkhsdabkfabkveyb' +}); // If we're not running as super user... if (process.getuid() !== 0) { @@ -27,11 +34,11 @@ if (process.getuid() !== 0) { assert.throws(() => { process.setegid('nobody'); - }, /^Error: (?:EPERM, .+|setegid group id does not exist)$/); + }, /(?:EPERM, .+|Group identifier does not exist: nobody)$/); assert.throws(() => { process.seteuid('nobody'); - }, /^Error: (?:EPERM, .+|seteuid user id does not exist)$/); + }, /^Error: (?:EPERM, .+|User identifier does not exist: nobody)$/); return; } @@ -41,7 +48,7 @@ const oldgid = process.getegid(); try { process.setegid('nobody'); } catch (err) { - if (err.message !== 'setegid group id does not exist') { + if (err.message !== 'Group identifier does not exist: nobody') { throw err; } else { process.setegid('nogroup'); diff --git a/test/parallel/test-process-uid-gid.js b/test/parallel/test-process-uid-gid.js index d3aac29decf0a6..42000955893241 100644 --- a/test/parallel/test-process-uid-gid.js +++ b/test/parallel/test-process-uid-gid.js @@ -35,11 +35,18 @@ if (common.isWindows) { assert.throws(() => { process.setuid({}); -}, /^TypeError: setuid argument must be a number or a string$/); +}, { + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "id" argument must be one of type ' + + 'number or string. Received type object' +}); assert.throws(() => { - process.setuid('fhqwhgadshgnsdhjsdbkhsdabkfabkveybvf'); -}, /^Error: setuid user id does not exist$/); + process.setuid('fhqwhgadshgnsdhjsdbkhsdabkfabkveyb'); +}, { + code: 'ERR_UNKNOWN_CREDENTIAL', + message: 'User identifier does not exist: fhqwhgadshgnsdhjsdbkhsdabkfabkveyb' +}); // If we're not running as super user... if (process.getuid() !== 0) { @@ -49,12 +56,12 @@ if (process.getuid() !== 0) { assert.throws( () => { process.setgid('nobody'); }, - /^Error: (?:EPERM, .+|setgid group id does not exist)$/ + /(?:EPERM, .+|Group identifier does not exist: nobody)$/ ); assert.throws( () => { process.setuid('nobody'); }, - /^Error: (?:EPERM, .+|setuid user id does not exist)$/ + /(?:EPERM, .+|User identifier does not exist: nobody)$/ ); return; } diff --git a/test/parallel/test-umask.js b/test/parallel/test-process-umask.js similarity index 82% rename from test/parallel/test-umask.js rename to test/parallel/test-process-umask.js index 1ac32cb7018906..869619f191a78b 100644 --- a/test/parallel/test-umask.js +++ b/test/parallel/test-process-umask.js @@ -43,8 +43,17 @@ assert.strictEqual(old, process.umask()); assert.throws(() => { process.umask({}); -}, /argument must be an integer or octal string/); +}, { + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "mask" argument must be one of type number, string, or ' + + 'undefined. Received type object' +}); -assert.throws(() => { - process.umask('123x'); -}, /invalid octal string/); +['123x', 'abc', '999'].forEach((value) => { + assert.throws(() => { + process.umask(value); + }, { + code: 'ERR_INVALID_ARG_VALUE', + message: `The argument 'mask' must be an octal string. Received '${value}'` + }); +}); From 6e43560b1c7cda24e7e2cd42bf94264e8c580e67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 19 Apr 2018 10:10:57 +0200 Subject: [PATCH 3/3] fixup! fix Windows --- lib/internal/process/methods.js | 83 +++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 36 deletions(-) diff --git a/lib/internal/process/methods.js b/lib/internal/process/methods.js index 33fcac14b8e178..503fd317f60395 100644 --- a/lib/internal/process/methods.js +++ b/lib/internal/process/methods.js @@ -10,32 +10,68 @@ const { } = require('internal/validators'); function setupProcessMethods() { + // Non-POSIX platforms like Windows don't have certain methods. + if (process.setgid !== undefined) { + setupPosixMethods(); + } + const { chdir: _chdir, + umask: _umask, + } = process; + + process.chdir = chdir; + process.umask = umask; + + function chdir(directory) { + if (typeof directory !== 'string') { + throw new ERR_INVALID_ARG_TYPE('directory', 'string', directory); + } + return _chdir(directory); + } + + const octalReg = /^[0-7]+$/; + function umask(mask) { + if (typeof mask === 'undefined') { + return _umask(mask); + } + + if (typeof mask === 'number') { + validateUint32(mask, 'mask'); + return _umask(mask); + } + + if (typeof mask === 'string') { + if (!octalReg.test(mask)) { + throw new ERR_INVALID_ARG_VALUE('mask', mask, + 'must be an octal string'); + } + const octal = Number.parseInt(mask, 8); + validateUint32(octal, 'mask'); + return _umask(octal); + } + + throw new ERR_INVALID_ARG_TYPE('mask', ['number', 'string', 'undefined'], + mask); + } +} + +function setupPosixMethods() { + const { initgroups: _initgroups, setegid: _setegid, seteuid: _seteuid, setgid: _setgid, setuid: _setuid, - setgroups: _setgroups, - umask: _umask, + setgroups: _setgroups } = process; - process.chdir = chdir; process.initgroups = initgroups; process.setegid = setegid; process.seteuid = seteuid; process.setgid = setgid; process.setuid = setuid; process.setgroups = setgroups; - process.umask = umask; - - function chdir(directory) { - if (typeof directory !== 'string') { - throw new ERR_INVALID_ARG_TYPE('directory', 'string', directory); - } - return _chdir(directory); - } function initgroups(user, extraGroup) { validateId(user, 'user'); @@ -80,31 +116,6 @@ function setupProcessMethods() { } } - const octalReg = /^[0-7]+$/; - function umask(mask) { - if (typeof mask === 'undefined') { - return _umask(mask); - } - - if (typeof mask === 'number') { - validateUint32(mask, 'mask'); - return _umask(mask); - } - - if (typeof mask === 'string') { - if (!octalReg.test(mask)) { - throw new ERR_INVALID_ARG_VALUE('mask', mask, - 'must be an octal string'); - } - const octal = Number.parseInt(mask, 8); - validateUint32(octal, 'mask'); - return _umask(octal); - } - - throw new ERR_INVALID_ARG_TYPE('mask', ['number', 'string', 'undefined'], - mask); - } - function execId(id, type, method) { validateId(id, 'id'); // Result is 0 on success, 1 if credential is unknown.