(#3503, #3513) Incorporate review feedback

- Use OrdinalIgnoreCase for command name comparison.
- Log with Warn or Error where appropriate and emit to log file only as
needed.
- Use explicitly named parameters instead of true/false.
- Remove comment that's no longer valid.

Author: corbob

src/chocolatey/infrastructure.app/services/NugetService.cs

@@ -223,8 +223,8 @@ it is possible that incomplete package lists are returned from a command
                     // This is done by default on the `info` command, and by request on the `list` command. As such, we are going to validate it's that scenario
                     // to avoid needlessly decrypting the arguments file.
                     var shouldDecryptArguments = (
-                            config.CommandName.Equals("info", StringComparison.InvariantCultureIgnoreCase) ||
-                            config.CommandName.Equals("list", StringComparison.InvariantCultureIgnoreCase)
+                            config.CommandName.Equals("info", StringComparison.OrdinalIgnoreCase) ||
+                            config.CommandName.Equals("list", StringComparison.OrdinalIgnoreCase)
                         ) &&
                         config.Verbose &&
                         !string.IsNullOrWhiteSpace(packageInfo.Arguments);
@@ -359,16 +359,12 @@ Package url{6}
                 var failedPackages = string.Join(", ", decryptionFailures.Select(f => "{0} - {1}".FormatWith(f.Package.Id, f.Package.Version)));
                 var failureMessage = "There were some failures decrypting package arguments.";
                 var failedPackagesMessage = "Failed packages: {0}".FormatWith(failedPackages);
-                if (config.RegularOutput)
-                {
-                    this.Log().Warn(failureMessage);
-                    this.Log().Warn(failedPackagesMessage);
-                }
-                else
-                {
-                    this.Log().Debug(failureMessage);
-                    this.Log().Debug(failedPackagesMessage);
-                }
+                this.Log().Warn(config.RegularOutput ?
+                    ChocolateyLoggers.Normal :
+                    ChocolateyLoggers.LogFileOnly, failureMessage);
+                this.Log().Warn(config.RegularOutput ?
+                    ChocolateyLoggers.Normal :
+                    ChocolateyLoggers.LogFileOnly, failedPackagesMessage);
             }
         }
 

src/chocolatey/infrastructure.app/utility/ArgumentsUtility.cs

@@ -55,7 +55,7 @@ public static bool SensitiveArgumentsProvided(string commandArguments)
 
         public static IEnumerable<string> DecryptPackageArgumentsFile(IFileSystem fileSystem, string id, string version)
         {
-            return DecryptPackageArgumentsFile(fileSystem, id, version, true, false);
+            return DecryptPackageArgumentsFile(fileSystem, id, version, redactSensitiveArguments: true, throwOnFailure: false);
         }
 
         public static IEnumerable<string> DecryptPackageArgumentsFile(
@@ -93,8 +93,6 @@ public static IEnumerable<string> DecryptPackageArgumentsFile(
 
             try
             {
-                // The following code is borrowed from the Chocolatey codebase, should
-                // be extracted to a separate location in choco executable so we can re-use it.
                 packageArgumentsUnencrypted = arguments.Contains(" --") && arguments.ToStringSafe().Length > 4
                     ? arguments
                     : NugetEncryptionUtility.DecryptString(arguments);
@@ -103,17 +101,18 @@ public static IEnumerable<string> DecryptPackageArgumentsFile(
             catch (Exception ex)
             {
                 var firstMessage = "There was an error attempting to decrypt the contents of the .arguments file for version '{0}' of package '{1}'.  See log file for more information.".FormatWith(version, id);
-                var secondMessage = "We failed to decrypt {0}. Error from decryption: {1}".FormatWith(argumentsFile, ex.Message);
-                
+                var secondMessage = "We failed to decrypt '{0}'. Error from decryption:{1}  '{2}'".FormatWith(argumentsFile, Environment.NewLine, ex.Message);
+                "chocolatey".Log().Error(throwOnFailure ? 
+                    logging.ChocolateyLoggers.Normal :
+                    logging.ChocolateyLoggers.LogFileOnly, firstMessage);
+                "chocolatey".Log().Error(throwOnFailure ?
+                    logging.ChocolateyLoggers.Normal :
+                    logging.ChocolateyLoggers.LogFileOnly, secondMessage);
+
                 if (throwOnFailure)
                 {
-                    "chocolatey".Log().Error(firstMessage);
-                    "chocolatey".Log().Error(secondMessage);
                     throw;
                 }
-
-                "chocolatey".Log().Debug(firstMessage);
-                "chocolatey".Log().Debug(secondMessage);
             }
 
             // Lets do a global check first to see if there are any sensitive arguments

tests/pester-tests/features/ArgumentsDecryption.Tests.ps1

@@ -77,12 +77,13 @@
         @{
             ErrorType = 'Base64 invalid'
             DecryptionError = 'The input is not a valid Base-64 string as it contains a non-base 64 character, more than two padding characters, or an illegal character among the padding characters.'
+            # `!` is an invalid Base64 character: https://en.wikipedia.org/wiki/Base64#Base64_table_from_RFC_4648
             FileContents = 'InvalidBase64!'
         }
         @{
             ErrorType = 'Invalid decryption'
             DecryptionError = 'Key not valid for use in specified state.'
-            # The contents of this was taken from a throw away VM.
+            # The contents of this was taken from a throw away VM. As such, DPAPI will not be able to decrypt it, and will error.
             FileContents = 'AQAAANCMnd8BFdERjHoAwE/Cl+sBAAAAn1/taDnOFUqGb17fBymxHQQAAAACAAAAAAAQZgAAAAEAACAAAAAU8gmqznJYKdkuj8bgk8sgg6Le3sbGoGkZOV3YtRFfwwAAAAAOgAAAAAIAACAAAAD1I9LYxrEhx9m71eF3VqyAike+XJTePhDAcrOilAFjQlAAAAA8lfiMR5Ns/AntLdVR3eBQSduCnipRCbdu/er/+YABMTzJDMGqnXuIsKwWoNIhrB14Yit4jVPipt3a/Nx18xx+YsnUewI4P6GlDL5do1y8mkAAAABMxvyPgCtN36BwAOXvJghIh9Hs8jUZOJtQIlWci8BnJkBmaaoSZ6pTGULk4TbFXMf/FK1NPo2mPM0YVL8QgJyK'
         }
     ) {
@@ -110,7 +111,8 @@
 
         It 'Outputs expected messages' {
             $shouldContain = -not ($CommandDescription -eq 'verbose' -or $CommandDescription -eq 'remember')
-            $Output.Lines | Should -Not:$shouldContain -Contain "We failed to decrypt $($env:ChocolateyInstall)\.chocolatey\upgradepackage.1.0.0\.arguments. Error from decryption: $DecryptionError" -Because $Output.String
+            $Output.Lines | Should -Not:$shouldContain -Contain "We failed to decrypt '$($env:ChocolateyInstall)\.chocolatey\upgradepackage.1.0.0\.arguments'. Error from decryption:" -Because $Output.String
+            $Output.Lines | Should -Not:$shouldContain -Contain "'$DecryptionError'" -Because $Output.String
         }
     }
 }