bilal-fazlani / commanddotnet

A modern framework for building modern CLI apps
https://commanddotnet.bilal-fazlani.com
MIT License
570 stars 29 forks source link

Stops displaying Password values in help text #326

Closed giuliov closed 3 years ago

giuliov commented 3 years ago

I found this problem, by adding an option of Password type with the EnvVar attribute. The default help text generator displays the default value with no consideration for the type

https://github.com/bilal-fazlani/commanddotnet/blob/14c8ca20ceef44db4dab3a6eb6c403dccf7c53b4/CommandDotNet/Help/HelpTextProvider.cs#L221

I verified that replacing the above line with

        : $"[{defaultValue!.ValueToString( argument.TypeInfo.Type == typeof(Password) )}]";

solves the issue.

bilal-fazlani commented 3 years ago

It would be nice to have tests for this functionality

drewburlingame commented 3 years ago

@giuliov thank you for reporting this. We shouldn't have to check the type. One of the main purposes of Password is to prevent this type of leak by default. Any .ToString() call should return and empty string or "*****" as seen in these tests. The goal is to avoid this kind of checking if the type is Password to display the value.

Edit: I remember now that the value isn't of type Password at this point. Your fix would be correct.

To test this, we can edit the DefaultFromEnvVarsTests by adding a new command to the App class in the test with a Password and EnvVar attr. I can't easily contribute to this PR because you've submitted it from your master branch. If you submit from a different branch, it would create a branch of the same name in this repo and I could check it out on our repo.

drewburlingame commented 3 years ago

@giuliov https://www.nuget.org/packages/CommandDotNet/4.1.4 should fix the issue. Let me know if you're still seeing it.

giuliov commented 3 years ago

Thanks @drewburlingame for the fast turnaround. I confirm that the problem disappeared in the new version.