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

[aweebit]

Problem

opts() now returns the private _optionValues object, making changes to it such as property deletion possible, but I don't think they are supposed to be possible. For example, deleting a property would lead to an inconsistent state in which a source is defined in _optionValueSources for an option value that does not exist anymore.

program.setOptionValueWithSource('foo', 'bar', 'config');
const optionValues = program.opts();
console.log(program.getOptionValue('foo')); // bar
console.log(program.getOptionValueSource('foo')); // config
optionValues.foo = 'baz';
console.log(program.getOptionValue('foo')); // baz
console.log(program.getOptionValueSource('foo')); // still config, but should be undefined
delete optionValues.foo;
console.log(program.getOptionValue('foo')); // undefined
console.log(program.getOptionValueSource('foo')); // still config, but should be undefined

Solution

The PR originally fixed the problem by returning a shallow clone of _optionValues from opts() (the original #1920 did the same and has only been closed because of a branch rename).

However, to still support allowed changes to option values on the object returned from opts() and make getting / setting the values through that object consistent with how getOptionValue() / setOptionValue() behave by, for example, setting the source to undefined when updating the value (but that could also be relevant after #1919 is merged since the way option values are set would differ depending on whether the command is currently being parsed), thus making the change as "non-breaking" as possible and at the same time preventing inconsistencies, a different solution using a proxy with the get(), set(), deleteProperty() and defineProperty() traps as the return value of opts() has been adopted.

It is worth mentioning that no private object is exposed in the current library code when the legacy storeOptionsAsProperties mode is enabled. To preserve backwards-compatibility, the return value of opts() in that mode is left unchanged.

ChangeLog

Changed

• Breaking: prevent unsupported modifications such as property deletion to the object returned from opts() and make getting / setting option values through that object when storeOptionsAsProperties is disabled consistent with how .getOptionValue() / .setOptionValue() work

[shadowspawn]

Bit of a mind dump, take with salt. Leaning towards towards PR and giving this a go in next major version, although no issues reported so low priority.

I did wonder about making a shallow clone at the time the code was added (not mentioned): #1102

I have a vague performance concern about code like:

if (program.opts().foo)
   doSomething(program.opts().bar);

But:

• an options object is passed into the action handler, so easy to use that
• can take a copy
• I do not consider performance as a key factor

Thinking about traps, a possible trap is people writing mixed code like:

   .action((options, cmd) => {
      if (options)
         cmd.setOptionValueWithSource('foo', 'bar', 'my-source');
     doSomething(options); // does this include 'bar'? Change in behaviour.
   })

[aweebit]

Thinking about traps, a possible trap is people writing mixed code like:

   .action((options, cmd) => {
      if (options)
         cmd.setOptionValueWithSource('foo', 'bar', > 'my-source');
     doSomething(options); // does this include > 'bar'? Change in behaviour.
   })

I see two solutions:

• Do not pass options as parameter to action handlers (actually, the same problem is present for opts() calls)
• Throw an error when setting option values from within an action handler (and maybe also after the handler is called, effectively forbidding setting option values after preAction hooks). But since it doesn't make sense to forbid setting option values from within hooks, I think it should not be done for actions either

[shadowspawn]

A shallow clone does not protect against modification to non-trivial properties. I knew that was the case, and wondering how relevant. Yup!

The quick check I did was an array, like for a variadic option. A more complex example is some object created by a custom parser, like say an URL.

[aweebit]

Another idea: make opts() return an ES6 Proxy of _optionValues that throws errors when trying to make changes to the object.

It seems like all difficult problems can be solved by using proxies :)

[aweebit]

A shallow clone does not protect against modification to non-trivial properties. I knew that was the case, and wondering how relevant. Yup!

I don't think this is relevant, I think the user should expect to receive an object with actual option values and act accordingly (make deep clones if option value mutations are undesired). Also, it would mean too much work to do deep cloning every time opts() is called.

[aweebit]

For consistency, the object returned from opts() when options-as-properties are enabled would also need to be updated when option values change. I suggest:

1. cherry-picking 36fa763 829655d 633e4e4 be00f59 from #1919, making _optionValues the only true storage for option values,
2. changing the modified targets in the handlers for the proxy returned from the Command constructor due to the cherry-picked commits from _optionValues to _optionValuesProxy so that the set(), deleteProperty() and defineProperty() handlers are used when modifying options-as-properties
3. maintaining a getter with name equal to _versionOptionName in _optionValues when options-as-properties are enabled
4. treating the option with name _versionOptionName specially in setOptionValueWithSource() (and therefore in setOptionValue() and the set() handler, too) when options-as-properties are enabled: setting its value should update _version
5. unsetting values for options other than the one with name _versionOptionName and those stored in options when options-as-properties are being enabled
6. forbidding setting such options in setOptionValueWithSource() (and therefore in setOptionValue() and the set() handler, too) when options-as-properties are enabled so that the object returned from opts() really does return all options
7. simply returning _optionValuesProxy from opts() without losing backwards compatibility which is made possible by steps 1, 3, 5 and 6

[shadowspawn]

I did some light experimentation, and noticed one slightly odd output:

import { Command } from 'commander';
const program = new Command();
program.storeOptionsAsProperties();
program.option('--xyz', 'description');
program.parse();
console.log(program.opts());

1921 % node index.mjs --xyz
{ undefined: [Getter/Setter], xyz: true }

[shadowspawn]

Historically the way options were stored on the command object itself caused many problems: #933

The new way of storing the options is in a separate object: #1102.

I am not too concerned about exposing the underlying object via .opts(). In fact I initially wanted to make the new object available as a property, but the good names were already in use! There was an existing function called .opts() so adopting that pattern did allow a single API to work however the options are stored, which allows for a graceful transition from the old way to the new way.

A fully private implementation would allow changing the underlying representation (say to a map) without externally visible changes, but having the API at all offers the major benefits. Good enough.

[shadowspawn]

It seems like all difficult problems can be solved by using proxies :)

Gave it a go! 😆

I find it both clever, and slightly alarming, to return the proxy as this from the constructor.

This approach does allow some guardrails and do more than a free-for-all object without an explicit API to make it possible.

[aweebit]

I did some light experimentation, and noticed one slightly odd output:

import { Command } from 'commander';
const program = new Command();
program.storeOptionsAsProperties();
program.option('--xyz', 'description');
program.parse();
console.log(program.opts());

1921 % node index.mjs --xyz
{ undefined: [Getter/Setter], xyz: true }

I fixed this in a3f0e28, and even made the property look like a data property despite using getters and setters in the console output.

I am not too concerned about exposing the underlying object via .opts(). In fact I initially wanted to make the new object available as a property, but the good names were already in use!

It would be okay to expose the underlying object without a proxy if setting / deleting an option did not require a change to a different object, namely _optionValueSources. But with the proxy, it can be safely exposed even despite this.

There was an existing function called .opts() so adopting that pattern did allow a single API to work however the options are stored, which allows for a graceful transition from the old way to the new way.

With both proxies I've added, the options object could be made available as a property while still supporting both storage modes (opts() now simply returns _optionValuesProxy).

Gave it a go! 😆

Yes, and it worked!

I find it both clever, and slightly alarming, to return the proxy as this from the constructor.

Yet, it is supported, and I think even subclassing works as expected.

I am not sure why you closed the PR to be honest. Is preventing the unsafe exposure something you are not interested in?

I would like to open a new PR for this branch since the essence has changed and is now not to change the return value of opts() to something different, but to prevent undesired modifications to the current return value. The name of the new PR would be something like

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

Is it okay if I open it?

[shadowspawn]

I am not sure why you closed the PR to be honest. Is preventing the unsafe exposure something you are not interested in?

I didn't think just making comments would send a clear enough message. I'll explain at some length and hopefully give you some insight into my reasoning.

Commander is a widely used and long-lived library with many existing users. I am conservative about changes.

I see the goal of the PR as being of low value and more about theoretical correctness than a practical concern. Adding a spread operator: maybe. Changing the actual object returned from the Command constructor, and introducing a subtle layer for every use of the command object, and multiple handler "traps", and potential maintenance complications, and risk of unanticipated breaking changes, and explicit breaking changes with only being able to store known options as properties in legacy programs: no no no no no no.

Is it okay if I open it?

I don't think that is worthwhile based on my current understanding.

The PR template includes in particular:

What problem are you solving?
What Issues does this relate to?

To expand on that. What problem affecting users are you solving? Is this blocking you, or preventing some reasonable or best practice author patterns? If this supported by your own experience with writing a real program, or been reported as an issue? Is this something that will affect many users and we could help them learn how to do it correctly? Should this be enforced in code or just change documentation? Is this something the maintainers of the library may be concerned about causing more problems in the future?

To be clear: holes in the logical behaviour of the library are still of interest! (But making the program more opinionated and stricter will also break some existing users and usages. Are the benefits worthwhile?)

[shadowspawn]

I'll add a couple of examples of concerns about the depth of these changes.

[shadowspawn]

I think even subclassing works as expected.

You think subclassing works as expected? I expect this was just a casual and honest comment, but this is a big red flag. Neither of us have experience or expertise with Proxy. Command subclassing is allowed and supported, although in fact was being used by our users before we had any explicit support.

I did some reading about Proxy to see if it might break existing patterns or complicate changes in future. I found one good example. Suppose we want to make private properties actually private in the future:

// add private property to Command class
class Command extends EventEmitter {
  #sequenceNumber = 1;

  incrementSequence(value) {
    value = value ?? 1;
    this.#sequenceNumber += value;
  }

import { program } from 'commander';
program.incrementSequence();

% node private.mjs 
/Users/john/Documents/Sandpits/commander/my-fork/lib/command.js:20
    this.#sequenceNumber += value;
        ^

TypeError: Cannot read private member #sequenceNumber from an object whose class did not declare it
    at Proxy.incrementSequence (/Users/john/Documents/Sandpits/commander/my-fork/lib/command.js:20:9)
    at file:///Users/john/Documents/Sandpits/commander/issues/1919/private.mjs:2:9
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

Node.js v18.16.1

NB: I know that this can potentially be worked around by yet more work in the Proxy setup and traps.

My point is that adopting a sophisticated pattern has costs and unanticipated side-affects. Working with a plain Class is core JavaScript. I see Proxy as a sophisticated tool to make some hard things possible, not something to be dropped in lightly.

[shadowspawn]

Enough exposition! Hopefully you have a better idea of what I think about as a conservative library maintainer.

[aweebit]

I did some reading about Proxy to see if it might break existing patterns or complicate changes in future. I found one good example. Suppose we want to make private properties actually private in the future:

// add private property to Command class
class Command extends EventEmitter {
  #sequenceNumber = 1;

  incrementSequence(value) {
    value = value ?? 1;
    this.#sequenceNumber += value;
  }

import { program } from 'commander';
program.incrementSequence();

% node private.mjs 
/Users/john/Documents/Sandpits/commander/my-fork/lib/command.js:20
    this.#sequenceNumber += value;
        ^

TypeError: Cannot read private member #sequenceNumber from an object whose class did not declare it
    at Proxy.incrementSequence (/Users/john/Documents/Sandpits/commander/my-fork/lib/command.js:20:9)
    at file:///Users/john/Documents/Sandpits/commander/issues/1919/private.mjs:2:9
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

Node.js v18.16.1

A related issue for me to read later, and for you If you're interested:

• Private members break proxies tc39/proposal-class-fields#106

[aweebit]

A related issue for me to read later, and for you If you're interested:

• Private members break proxies tc39/proposal-class-fields#106

For interest, here is a solution I came up with even before reading into the issue:

class Instance {
  #secret = 'hedgehog';

  constructor() {
    return new Proxy(this, {
      get(target, key) {
        const value = Reflect.get(target, key);
        return value instanceof Function ? value.bind(target) : value;
      }
    });
  }

  revealSecret() {
    console.log(this.#secret);
  }
}

const instance = new Instance();
instance.revealSecret();

Update: There are some complications, though.

• instance.revealSecret === instance.revealSecret evaluates to false since a new function is created each time bind() is called (solved by caching the bound methods in a WeakMap)

• The bound methods returned from the get() handler cannot be rebound and thus visibly differ from the original methods (probably can be solved by using a wrapper function replacing the value of this from the proxy to target instead of binding)

• false is logged when running the following code:

  function someMethod() {}
  instance.someMethod = someMethod;
  console.log(instance.someMethod === someMethod);

  (seems unlikely something can be done to fix this)

Update: All the issues can be solved by returning a proxy further up the prototype chain!

class InstanceBase {
  constructor() {
    return new Proxy(this, {});
  }
}

class Instance extends InstanceBase {
  #secret = 'hedgehog';

  revealSecret() {
    console.log(this.#secret);
  }
}

const instance = new Instance();
instance.revealSecret(); // succeeds

console.log(instance.revealSecret === instance.revealSecret); // true

function someMethod() {}
instance.someMethod = someMethod;
console.log(instance.someMethod === someMethod); // true

[aweebit]

Update: All the issues can be solved by returning a proxy further up the prototype chain!

Now implemented on the branch I wanted this PR to merge, see eb142d8.

I think even subclassing works as expected.

You think subclassing works as expected? I expect this was just a casual and honest comment, but this is a big red flag. Neither of us have experience or expertise with Proxy. Command subclassing is allowed and supported, although in fact was being used by our users before we had any explicit support.

I admit my knowledge of proxies is not perfect, but I had already had some exposure to them before adding them here which I had got first and foremost from giving this answer on Stack Overflow 2 years ago, not even hoping it would ever be read by anyone because of how old and specific the question was, but just because I had fun experimenting with this stuff. Now, as I keep experimenting here, my knowledge gets better by the hour. For example, since now the Command class is itself a subclass of a class whose constructor returns a proxy, and since none of the tests failed after making this change, I can now tell you with certainty there should be no problems with subclasses of Command that we would not find in Command itself.

My point is that adopting a sophisticated pattern has costs and unanticipated side-affects. Working with a plain Class is core JavaScript. I see Proxy as a sophisticated tool to make some hard things possible, not something to be dropped in lightly.

As you may have noticed by now, I am used to the "code first" approach and let myself indulge in experiments before having a clear idea about what they should end in and unit tests ready. But that doesn't mean I am taking all of this lightly and don't understand the responsibility that comes with making changes to a project of such scale. It is great you pointed me in the right direction by finding the issue with private fields, and that is something that should happen when changes adopting sophisticated tools like this one are discussed.

It is definitely worth it to keep looking thoroughly for further problems / unanticipated side effects, but I am honestly beginning to doubt there will be any now when the problem with private fields is fixed, because the solution with a constructor returning the proxy further up the prototype chain seems to be a silver bullet in a way. I will be happy if you prove me wrong, though, and I challenge you to do so :) That would show me I am being a little too careless and would be a good lesson for me.

As for me, I personally don't see any way the users could tell they are dealing with a proxy except for the obvious util.types.isProxy() at the moment. I even spent some time learning about how the receiver parameter of Reflect.get(), Reflect.set() and the corresponding proxy handler methods works which I had not done properly before, and am pretty sure the parameter is used correctly in the current version of the code.

I see the goal of the PR as being of low value and more about theoretical correctness than a practical concern.

I am not sure I agree with you on that. I find the way the following code works quite problematic, and not only from the theoretical point of view:

program.setOptionValueWithSource('foo', 'bar', 'config');
const optionValues = program.opts();
console.log(program.getOptionValue('foo')); // bar
console.log(program.getOptionValueSource('foo')); // config
optionValues.foo = 'baz';
console.log(program.getOptionValue('foo')); // baz
console.log(program.getOptionValueSource('foo')); // still config, but should be undefined
delete optionValues.foo;
console.log(program.getOptionValue('foo')); // undefined
console.log(program.getOptionValueSource('foo')); // still config, but should be undefined

Note that only public methods are used here and, despite that, inconsistent states are reached. That should definitely not be allowed, but the only way to prevent it while keeping the object returned from opts() up-to-date at all times I see is by using proxies.

The first proxy it leads us to is _optionValuesProxy which would be enough if we did not want to support the options-as-properties mode. But since we do want that, and since the behavior of the object returned from opts() should ideally be the same in both modes, I've also added the second proxy returned from the constructor. However, to say that being able to store options as properties in legacy programs is the only meaning such a change would have would be a serious understatement. I can name at least two further advantages:

• The fact that _optionValues becomes the only true storage for option values makes it possible to never care about the options-as-properties mode anymore when adding code dealing with option values, see 36fa763 in Clean up state from previous parse call when calling parse() / parseAsync() #1919 for example.
• Support for option names conflicting with the instance's properties even when options-as-properties are enabled is added: values for such options just have to be set via setOptionValue() or setOptionValueWithSource() and accessed via getOptionValue(), opts() or optsWithGlobals(), rather than via what is masked as an instance property (this is of lesser importance than the previous point, but still nice to have).

To be clear: holes in the logical behaviour of the library are still of interest!

That is what this PR is all about. No, the issue does not affect many users and has probably never been reported, and deleting an option value like in the example above is not something I have thought of doing in a real application. However, I do think the library should not allow inconsistent states like the ones in the example to be reached, even if it almost never happens in practice.

(But making the program more opinionated and stricter will also break some existing users and usages. Are the benefits worthwhile?)

Setting or deleting an option value without caring about its saved source remaining unchanged is usage that should never have been made possible in the first place since it is simply wrong.

To sum up, I still think you gave up on this PR too early.

[aweebit]

The first proxy it leads us to is _optionValuesProxy which would be enough if we did not want to support the options-as-properties mode. But since we do want that, and since the behavior of the object returned from opts() should ideally be the same in both modes, I've also added the second proxy returned from the constructor.

The consistency between the modes in the way the return value of opts() behaves is actually something that is currently missing in the library. What I mean by that is that a private object is only exposed when options-as-properties are disabled. When they are enabled, a newly created object with option values is returned and as a result, mutating it has no effect on the actual option values.

If you say this discrepancy is desired because keeping the old approach as backwards compatible as possible is: weil, in that case, there is no need to return a proxy from a constructor as part of this PR, but I still think it's a reasonable change because of the advantages I've named.

[shadowspawn]

A related issue for me to read later, and for you If you're interested:

Very interesting and deep commentary on a controversial issue from multiple experts. Thanks. Some divergence between the envisaged uses of Proxy (i.e. membranes) and the way it has been described and used.

I admit my knowledge of proxies is not perfect, but I had already had some exposure to them before adding them here

I apologise that I did assume you had recently discovered proxies, and were seeing them as solutions everywhere.

As you may have noticed by now, I am used to the "code first" approach and let myself indulge in experiments before having a clear idea about what they should end in and unit tests ready.

I recommend using draft state for PRs while they are still evolving.

If you say this discrepancy is desired because keeping the old approach as backwards compatible as possible is

Effectively yes. The old approach is still available for backwards compatibility, not as an alternative mechanism for accessing options. (I suspect this may not be a direct response to your comment, but still relevant.)

Setting or deleting an option value without caring about its saved source remaining unchanged is usage that should never have been made possible in the first place since it is simply wrong.

Short version: I did that. I am still comfortable with it.

[aweebit]

If you say this discrepancy is desired because keeping the old approach as backwards compatible as possible is

Effectively yes. The old approach is still available for backwards compatibility, not as an alternative mechanism for accessing options. (I suspect this may not be a direct response to your comment, but still relevant.)

On the contrary, this is a very useful response. I now reverted 1b94efb (see aweebit@ef54142) to make opts() behavior backwards-compatible when options-as-proeprties are enabled. The only change I then cherry-picked from the commits made after that one is adding a get() trap so that the "get option value" behavior remains consistent even if getOptionValue() is to change in the future (see aweebit@64dfafd).

Check develop...aweebit:feature/opts-return to see the file changes that would be suggested by this PR if it were reopened now. There would be nothing new but a thin layer on top of _optionValues when it is returned from opts() that would prevent unwanted changes to option values and make getting / setting them consistent with how getOptionValue() / setOptionValue() behave. The consistency will become relevant if, for example, #1919 is merged, since the way option values are set would differ depending on whether the command is currently being parsed.

Do you think just this one proxy use is still problematic? I personally think it is highly unlikely to cause any trouble, including when changes to the language are made. After all, _optionValues is a simple object used for key-value storage that does not need to support subclassing, private fields etc. It is not even concerning the user could find out a proxy is returned by calling util.types.isProxy() (by contrast, finding that out for a Command instance where options-as-properties are disabled would make little sense because the proxy was only really needed for the case when they are on). So I think the use of proxies here is well-justified. What is your opinion? Could the PR be reopened now?

[aweebit]

Everything is now prepared for a potential reopen, see the updated PR title and description.

[shadowspawn]

Do you think just this one proxy use is still problematic?

This is a sensibly minor use of proxy.

Could the PR be reopened now?

However, still not something I am concerned about fixing at this time. But interesting to know that opts() could be made "read-only".

Copy of relevant code for possible future reference:

  this._optionValuesProxy = new Proxy(this._optionValues, {
      get: (_, key) => {
        return this.getOptionValue(key);
      },
      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`);
      }
    });