pnp / cli-microsoft365

Manage Microsoft 365 and SharePoint Framework projects on any platform
https://aka.ms/cli-m365
MIT License
882 stars 312 forks source link

Validation fails to detect empty string for "id" Parameter in "m365 spo list get" command. #6055

Open Saurabh7019 opened 1 month ago

Saurabh7019 commented 1 month ago

Priority

(Low) Something is a little off

Description

The validation for the id parameter in them365 spo list get command does not properly handle an empty string. This results in the validation passing incorrectly and returning same result as m365 spo web get

Similar issues exists in a few other commands within the CLI.

Steps to reproduce

Expected results

The validation for the id parameter should properly handle an empty string.

Actual results

The command executes without proper validation and returns spo web get result.

Diagnostics

No response

CLI for Microsoft 365 version

v7.9.0

nodejs version

v20.9.0

Operating system (environment)

Windows

Shell

PowerShell

cli doctor

No response

Additional Info

image

The issue is with unhandled empty string in command validation.

milanholemans commented 1 month ago

Yes, unfortunately, this is a "known" issue. Currently, we mostly check on if (args.options.id) but in fact, we should if (args.options.id !== undefined) because an empty string validates to false. Unfortunately, the majority of all commands suffer from this. @waldekmastykarz I'm wondering if this is solved within ZOD? If that's the case, I don't think it's worth fixing it.

waldekmastykarz commented 1 month ago

Yes, unfortunately, this is a "known" issue. Currently, we mostly check on if (args.options.id) but in fact, we should if (args.options.id !== undefined) because an empty string validates to false. Unfortunately, the majority of all commands suffer from this. @waldekmastykarz I'm wondering if this is solved within ZOD? If that's the case, I don't think it's worth fixing it.

Yes, this should be fixed in zod, because it has a separate required/optional check. I suggest, that we prioritize fixing this issue by migrating this command to zod as soon as possible so that we can confirm the fix.