Enhancement: Correctly parsing flags with minimist and centrally validating them

[martinlingstuyl]

We're using minimist to parse command arguments. Recently, we've been updating the way the CLI interacts with minimist, to make sure we have consistent input. We've done this around booleans (#3853) and we've started it around strings and numbers (#5396)

However, flag options also have a potential issue.

Consider the following example:

m365 spo site list --includeOneDriveSites

This takes care of including a boolean value in an API call to sharepoint. (true or false) By default it's false, but if the flag is included, the value is true.

We generally do not validate these flags however, which means that a user might inadvertently add a string value after them:

m365 spo site list --includeOneDriveSites abc

The actual value that is parsed by minimist will now be a string abc, instead of the expected true. That's potentially a cause for issues. In the code we now need to make sure that the flag is never used directly, but is always used with a condition:

//this is rather safe
if (args.options.includeOneDriveSites) { 
} 

// this as well.
!!args.options.includeOneDriveSites

// this is not safe
const requestbody: {
  someProp: 'someValue',
  someOtherProp: args.options.includeOneDriveSites
}

Our current implementation puts the burden of checking the flag in the command itself, we should however handle it centrally.

Solution

I propose adding flags to this.types.booleans, to make sure the values are validated centrally and type safe.

Another solution would be to look at zod

[waldekmastykarz]

I'd suggest looking into zod. It would mean a significant refactoring, but the other solution feels like putting a bandaid and a temporary fix that doesn't address all cases and will until we ran into another limitation.

[Adam-it]

good find 👍

I propose adding flags to this.types.booleans, to make sure the values are validated centrally and type safe.

I wonder would this really take that much time? maybe we could do that to have this 'safe' before we move research with zod?

[waldekmastykarz]

I wonder would this really take that much time?

I think it's a shame to spend time on a solution that we know we'll remove it anyway. Going through all commands at our scale is not trivial and will easily get into hours, including doing the work and reviewing the PR. I'd rather we spend that time on reviewing PRs and providing new functionality.

If we need an interim solution, I wonder if there's a different approach that we could do centrally rather than for each command separately.

[Adam-it]

gotchya 👍.. and BTW I may relate to that, I also like adding new stuff more than refactoring the old stuff 😉