Skip to content
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

Jump to bottom

Prevent opts() from unsafely exposing a private object and expose a proxy instead #1921

Closed
wants to merge 5 commits into from
Closed

Prevent opts() from unsafely exposing a private object and expose a proxy instead #1921

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e9b8856
Prevent opts() from exposing private object
aweebit Jul 30, 2023
3cfd9b1
Make opts() return proxy preventing mutations
aweebit Jul 30, 2023
88d0cf8
Allow setting properties directly on opts() return value
aweebit Jul 30, 2023
1b94efb
Return proxy redirecting keys for options-as-properties from Command …
aweebit Jul 30, 2023
a5afe93
Consistent opts() return value behavior
aweebit Jul 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
151 changes: 128 additions & 23 deletions lib/command.js
View file
Open in desktop
Expand Up @@ -77,6 +77,85 @@ class Command extends EventEmitter {
this._helpCommandnameAndArgs = 'help [command]';
this._helpCommandDescription = 'display help for command';
this._helpConfiguration = {};

this._optionValuesProxy = new Proxy(this._optionValues, {
set: (_, key, value) => {
this.setOptionValue(key, value);
return true;
},
deleteProperty: (_, key) => {
throw new Error(`Tried to delete value of option ${key}.
Option value deletion is not supported`);
},
defineProperty: (_, key) => {
throw new Error(`Tried to configure value of option ${key} using
- Object.defineProperty(),
- or Object.defineProperties(),
- or Reflect.defineProperty().
Options value configuration is not supported`);
}
});

// Because of how the returned proxy works, ideally, no prooerties should be defined outside the cinstructor.
// They can still be defined outside the constructor in subclasses, but only when _storeOptionsAsProperties is set to false.
this._version = undefined;
this._versionOptionName = undefined;

// The proxy only treats keys not present in the instance and its prototype chain as keys for _optionValues when _storeOptionsAsProperties is set to true.
// Setting option values for keys present in the instance and its prototype chain is still possible by calling .setOptionValue() or .setOptionValueWithSource(),
// but such values will not be accessible as instnace properties because the instance and its prototype chain has precedence.
// However, they will be accessible via .getOptionValue(), .opts() and .optsWithGlobals().
return new Proxy(this, {
get(target, key, receiver) {
if (target._storeOptionsAsProperties && !(key in target)) {
target = receiver = receiver._optionValuesProxy;
}
return Reflect.get(target, key, receiver);
},
set(target, key, value, receiver) {
if (target._storeOptionsAsProperties && !(key in target)) {
target = receiver = receiver._optionValuesProxy;
}
return Reflect.set(target, key, value, receiver);
},
has(target, key) {
if (target._storeOptionsAsProperties && !(key in target)) {
target = target._optionValuesProxy;
}
return Reflect.has(target, key);
},
deleteProperty(target, key) {
if (target._storeOptionsAsProperties && !(key in target)) {
target = target._optionValuesProxy;
}
return Reflect.deleteProperty(target, key);
},
defineProperty(target, key, descriptor) {
if (target._storeOptionsAsProperties && !(key in target)) {
target = target._optionValuesProxy;
}
return Reflect.defineProperty(target, key, descriptor);
},
getOwnPropertyDescriptor(target, key) {
if (target._storeOptionsAsProperties && !(key in target)) {
target = target._optionValuesProxy;
}
return Reflect.getOwnPropertyDescriptor(target, key);
},
ownKeys(target) {
const result = Reflect.ownKeys(target);
if (target._storeOptionsAsProperties) {
result.push(...Reflect.ownKeys(target._optionValuesProxy));
}
return result;
},
preventExtensions(target) {
if (target._storeOptionsAsProperties) {
Reflect.preventExtensions(target._optionValuesProxy);
}
return Reflect.preventExtensions(target);
}
});
}

/**
Expand Down Expand Up @@ -510,6 +589,9 @@ Expecting one of '${allowedValues.join("', '")}'`);
const oname = option.name();
const name = option.attributeName();

// register the option
this.options.push(option);

// store default value
if (option.negate) {
// --no-foo is special and defaults foo to true, unless a --foo option is already defined
Expand All @@ -521,9 +603,6 @@ Expecting one of '${allowedValues.join("', '")}'`);
this.setOptionValueWithSource(name, option.defaultValue, 'default');
}

// register the option
this.options.push(option);

// handler for cli and env supplied values
const handleOptionValue = (val, invalidValueMessage, valueSource) => {
// val is null for optional option used without an optional-argument.
Expand Down Expand Up @@ -750,10 +829,18 @@ Expecting one of '${allowedValues.join("', '")}'`);
*/

storeOptionsAsProperties(storeAsProperties = true) {
this._storeOptionsAsProperties = !!storeAsProperties;
if (this.options.length) {
throw new Error('call .storeOptionsAsProperties() before adding options');
}
if (Object.keys(this._optionValues).length) {
throw new Error('call .storeOptionsAsProperties() before setting option values');
}
if (!this._storeOptionsAsProperties && storeAsProperties) {
this._defineVersionOptionAsProperty();
} else if (this._storeOptionsAsProperties && !storeAsProperties) {
this._deleteVersionOptionProperty();
}
this._storeOptionsAsProperties = !!storeAsProperties;
return this;
}

Expand All @@ -765,9 +852,6 @@ Expecting one of '${allowedValues.join("', '")}'`);
*/

getOptionValue(key) {
if (this._storeOptionsAsProperties) {
return this[key];
}
return this._optionValues[key];
}

Expand All @@ -794,10 +878,20 @@ Expecting one of '${allowedValues.join("', '")}'`);

setOptionValueWithSource(key, value, source) {
if (this._storeOptionsAsProperties) {
this[key] = value;
} else {
this._optionValues[key] = value;
if (key === this._versionOptionName) {
throw new Error(`Tried to set value of option ${key} reserved for version number.
Set version number by calling .version() instead`);
}
const optionSupported = this.options.some(
option => key === option.attributeName()
);
if (!optionSupported) {
throw new Error(`Tried to set value of not supported option ${key}.
This is not allowed when option values are stored as instance properties.
Add support for option by calling .option() or .addOption() first`);
}
}
this._optionValues[key] = value;
this._optionValueSources[key] = source;
return this;
}
Expand Down Expand Up @@ -1555,19 +1649,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
* @return {Object}
*/
opts() {
if (this._storeOptionsAsProperties) {
// Preserve original behaviour so backwards compatible when still using properties
const result = {};
const len = this.options.length;

for (let i = 0; i < len; i++) {
const key = this.options[i].attributeName();
result[key] = key === this._versionOptionName ? this._version : this[key];
}
return result;
}

return this._optionValues;
return this._optionValuesProxy;
}

/**
Expand Down Expand Up @@ -1819,7 +1901,9 @@ Expecting one of '${allowedValues.join("', '")}'`);
flags = flags || '-V, --version';
description = description || 'output the version number';
const versionOption = this.createOption(flags, description);
if (this._storeOptionsAsProperties) this._deleteVersionOptionProperty();
this._versionOptionName = versionOption.attributeName();
if (this._storeOptionsAsProperties) this._defineVersionOptionAsProperty();
this.options.push(versionOption);
this.on('option:' + versionOption.name(), () => {
this._outputConfiguration.writeOut(`${str}\n`);
Expand All @@ -1828,6 +1912,27 @@ Expecting one of '${allowedValues.join("', '")}'`);
return this;
}

/**
* @api private
*/
_defineVersionOptionAsProperty() {
return Reflect.defineProperty(this._optionValues, this._versionOptionName, {
get: () => this._version,
set: (value) => {
this._version = value;
},
configurable: true,
enumerable: true
});
}

/**
* @api private
*/
_deleteVersionOptionProperty() {
return Reflect.deleteProperty(this._optionValues, this._versionOptionName);
}

/**
* Set the description.
*
Expand Down