Change Read-Host -MaskInput to use existing SecureString path, but return as plain text (#13256)

SteveL-MSFT · web-flow · commit 99b3bfaa34c6 · 2020-07-30T22:16:37.000-07:00
diff --git a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/ReadConsoleCmdlet.cs b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/ReadConsoleCmdlet.cs
@@ -130,9 +130,9 @@ protected override void BeginProcessing()
                 }
 
                 FieldDescription fd = new FieldDescription(promptString);
-                if (AsSecureString)
+                if (AsSecureString || MaskInput)
                 {
-                    fd.SetParameterType(typeof(System.Security.SecureString));
+                    fd.SetParameterType(typeof(SecureString));
                 }
                 else
                 {
@@ -149,27 +149,37 @@ protected override void BeginProcessing()
                 {
                     foreach (PSObject o in result.Values)
                     {
-                        WriteObject(o);
+                        if (MaskInput && o?.BaseObject is SecureString secureString)
+                        {
+                            WriteObject(Utils.GetStringFromSecureString(secureString));
+                        }
+                        else
+                        {
+                            WriteObject(o);
+                        }
                     }
                 }
             }
             else
             {
                 object result;
-                if (AsSecureString)
+                if (AsSecureString || MaskInput)
                 {
                     result = Host.UI.ReadLineAsSecureString();
                 }
-                else if (MaskInput)
-                {
-                    result = Host.UI.ReadLineMaskedAsString();
-                }
                 else
                 {
                     result = Host.UI.ReadLine();
                 }
 
-                WriteObject(result);
+                if (MaskInput)
+                {
+                    WriteObject(Utils.GetStringFromSecureString((SecureString)result));
+                }
+                else
+                {
+                    WriteObject(result);
+                }
             }
         }
 
diff --git a/src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs b/src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
@@ -112,17 +112,6 @@ public override string ReadLine()
             throw new PSNotImplementedException();
         }
 
-        /// <summary>
-        /// Null implementation of ReadLineMaskedAsString.
-        /// </summary>
-        /// <returns>
-        /// It throws an exception.
-        /// </returns>
-        public override string ReadLineMaskedAsString()
-        {
-            throw new PSNotImplementedException();
-        }
-
         /// <summary>
         /// ReadLineAsSecureString.
         /// </summary>
diff --git a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs
@@ -179,39 +179,6 @@ public override string ReadLine()
             return ReadLine(false, string.Empty, out unused, true, true);
         }
 
-        /// <summary>
-        /// See base class.
-        /// </summary>
-        /// <returns>
-        /// The characters typed by the user.
-        /// </returns>
-        /// <exception cref="HostException">
-        /// If obtaining a handle to the active screen buffer failed
-        ///    OR
-        ///    Win32's setting input buffer mode to disregard window and mouse input failed.
-        ///    OR
-        ///    Win32's ReadConsole failed.
-        /// </exception>
-        /// <exception cref="PipelineStoppedException">
-        /// If Ctrl-C is entered by user.
-        /// </exception>
-        public override string ReadLineMaskedAsString()
-        {
-            HandleThrowOnReadAndPrompt();
-
-            // we lock here so that multiple threads won't interleave the various reads and writes here.
-            object result = null;
-            lock (_instanceLock)
-            {
-                result = ReadLineSafe(false, PrintToken);
-            }
-
-            StringBuilder resultSb = result as StringBuilder;
-            Dbg.Assert(resultSb != null, "ReadLineMaskedAsString did not return a stringBuilder");
-
-            return resultSb.ToString();
-        }
-
         /// <summary>
         /// See base class.
         /// </summary>
diff --git a/src/System.Management.Automation/engine/hostifaces/InternalHostUserInterface.cs b/src/System.Management.Automation/engine/hostifaces/InternalHostUserInterface.cs
@@ -120,47 +120,6 @@ public override
             return result;
         }
 
-        /// <summary>
-        /// See base class.
-        /// </summary>
-        /// <returns>
-        /// The characters typed by the user.
-        /// </returns>
-        /// <exception cref="HostException">
-        /// If the UI property of the external host is null, possibly because the PSHostUserInterface is not
-        /// implemented by the external host.
-        /// </exception>
-        public override
-        string
-        ReadLineMaskedAsString()
-        {
-            if (_externalUI == null)
-            {
-                ThrowNotInteractive();
-            }
-
-            string result = null;
-
-            try
-            {
-                result = _externalUI.ReadLineMaskedAsString();
-            }
-            catch (PipelineStoppedException)
-            {
-                // PipelineStoppedException is thrown by host when it wants
-                // to stop the pipeline.
-                LocalPipeline lpl = (LocalPipeline)((RunspaceBase)_parent.Context.CurrentRunspace).GetCurrentlyRunningPipeline();
-                if (lpl == null)
-                {
-                    throw;
-                }
-
-                lpl.Stopper.Stop();
-            }
-
-            return result;
-        }
-
         /// <summary>
         /// See base class.
         /// </summary>
diff --git a/src/System.Management.Automation/engine/hostifaces/MshHostUserInterface.cs b/src/System.Management.Automation/engine/hostifaces/MshHostUserInterface.cs
@@ -61,29 +61,6 @@ public abstract System.Management.Automation.Host.PSHostRawUserInterface RawUI
         /// <seealso cref="System.Management.Automation.Host.PSHostUserInterface.Prompt"/>
         public abstract string ReadLine();
 
-        /// <summary>
-        /// Same as ReadLine except that the input is not echoed to the user while it is collected
-        /// or is echoed in some obfuscated way, such as showing a dot for each character.
-        /// </summary>
-        /// <returns>
-        /// The characters typed by the user.
-        /// </returns>
-        /// <remarks>
-        /// Note that credentials (a user name and password) should be gathered with
-        /// <see cref="System.Management.Automation.Host.PSHostUserInterface.PromptForCredential(string, string, string, string)"/>
-        /// <see cref="System.Management.Automation.Host.PSHostUserInterface.PromptForCredential(string, string, string, string, System.Management.Automation.PSCredentialTypes, System.Management.Automation.PSCredentialUIOptions)"/>
-        /// </remarks>
-        /// <seealso cref="System.Management.Automation.Host.PSHostUserInterface.ReadLine"/>
-        /// <seealso cref="System.Management.Automation.Host.PSHostUserInterface.PromptForCredential(string, string, string, string)"/>
-        /// <seealso cref="System.Management.Automation.Host.PSHostUserInterface.PromptForCredential(string, string, string, string, System.Management.Automation.PSCredentialTypes, System.Management.Automation.PSCredentialUIOptions)"/>
-        /// <seealso cref="System.Management.Automation.Host.PSHostUserInterface.PromptForChoice"/>
-        /// <seealso cref="System.Management.Automation.Host.PSHostUserInterface.Prompt"/>
-        public virtual string ReadLineMaskedAsString()
-        {
-            // Default implementation of the function to maintain backwards compatibility of the base class.
-            throw new PSNotImplementedException();
-        }
-
         /// <summary>
         /// Same as ReadLine, except that the result is a SecureString, and that the input is not echoed to the user while it is
         /// collected (or is echoed in some obfuscated way, such as showing a dot for each character).
diff --git a/src/System.Management.Automation/engine/remoting/server/ServerRemoteHostUserInterface.cs b/src/System.Management.Automation/engine/remoting/server/ServerRemoteHostUserInterface.cs
@@ -197,17 +197,6 @@ public override void WriteWarningLine(string message)
             _serverMethodExecutor.ExecuteVoidMethod(RemoteHostMethodId.WriteWarningLine, new object[] { message });
         }
 
-        /// <summary>
-        /// Read line as string masked.
-        /// </summary>
-        /// <returns>
-        /// Not implemented. It throws an exception.
-        /// </returns>
-        public override string ReadLineMaskedAsString()
-        {
-            throw new PSNotImplementedException();
-        }
-
         /// <summary>
         /// Read line as secure string.
         /// </summary>
diff --git a/test/powershell/Modules/Microsoft.PowerShell.Utility/Read-Host.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Utility/Read-Host.Tests.ps1
@@ -12,6 +12,7 @@ Describe "Read-Host Test" -Tag "CI" {
 
     AfterEach {
         $ps.Commands.Clear()
+        $th.UI.Streams.Clear()
     }
 
     AfterAll {
@@ -30,17 +31,26 @@ Describe "Read-Host Test" -Tag "CI" {
         $prompt = $th.ui.streams.prompt[0]
         $prompt | Should -Not -BeNullOrEmpty
         $prompt.split(":")[-1] | Should -Be myprompt
+        $result | Should -BeExactly 'this is a prompt response'
     }
 
     It "Read-Host returns a secure string when using -AsSecureString parameter" {
         $result = $ps.AddScript("Read-Host -AsSecureString").Invoke() | Select-Object -First 1
         $result | Should -BeOfType SecureString
-        [pscredential]::New("foo",$result).GetNetworkCredential().Password | Should -BeExactly TEST
+        $result | ConvertFrom-SecureString -AsPlainText | Should -BeExactly 'TEST'
     }
 
     It "Read-Host returns a string when using -MaskInput parameter" {
         $result = $ps.AddScript("Read-Host -MaskInput").Invoke()
-        $result | Should -Be $th.UI.ReadLineData
+        $result | Should -BeExactly 'TEST'
+    }
+
+    It "Read-Host returns a string when using -MaskInput parameter used with -Prompt" {
+        $result = $ps.AddScript("Read-Host -MaskInput -Prompt Test").Invoke()
+        $prompt = $th.ui.streams.prompt[0]
+        $prompt | Should -Not -BeNullOrEmpty
+        $prompt.split(":")[-1] | Should -BeExactly 'Test'
+        $result | Should -BeExactly 'this is a prompt response'
     }
 
     It "Read-Host throws an error when both -AsSecureString parameter and -MaskInput parameter are used" {
diff --git a/test/tools/Modules/HelpersHostCS/HelpersHostCS.psm1 b/test/tools/Modules/HelpersHostCS/HelpersHostCS.psm1
@@ -183,11 +183,6 @@ namespace TestHost
             return ReadLineData;
         }
 
-        public override string ReadLineMaskedAsString()
-        {
-            return ReadLineData;
-        }
-
         public override SecureString ReadLineAsSecureString()
         {
             SecureString ss = new SecureString();

Original file line number	Diff line number	Diff line change
`@@ -130,9 +130,9 @@ protected override void BeginProcessing()`
`130`	`130`	`}`
`131`	`131`
`132`	`132`	`FieldDescription fd = new FieldDescription(promptString);`
`133`		`- if (AsSecureString)`
	`133`	`+ if (AsSecureString \|\| MaskInput)`
`134`	`134`	`{`
`135`		`- fd.SetParameterType(typeof(System.Security.SecureString));`
	`135`	`+ fd.SetParameterType(typeof(SecureString));`
`136`	`136`	`}`
`137`	`137`	`else`
`138`	`138`	`{`
`@@ -149,27 +149,37 @@ protected override void BeginProcessing()`
`149`	`149`	`{`
`150`	`150`	`foreach (PSObject o in result.Values)`
`151`	`151`	`{`
`152`		`- WriteObject(o);`
	`152`	`+ if (MaskInput && o?.BaseObject is SecureString secureString)`
	`153`	`+ {`
	`154`	`+ WriteObject(Utils.GetStringFromSecureString(secureString));`
	`155`	`+ }`
	`156`	`+ else`
	`157`	`+ {`
	`158`	`+ WriteObject(o);`
	`159`	`+ }`
`153`	`160`	`}`
`154`	`161`	`}`
`155`	`162`	`}`
`156`	`163`	`else`
`157`	`164`	`{`
`158`	`165`	`object result;`
`159`		`- if (AsSecureString)`
	`166`	`+ if (AsSecureString \|\| MaskInput)`
`160`	`167`	`{`
`161`	`168`	`result = Host.UI.ReadLineAsSecureString();`
`162`	`169`	`}`
`163`		`- else if (MaskInput)`
`164`		`- {`
`165`		`- result = Host.UI.ReadLineMaskedAsString();`
`166`		`- }`
`167`	`170`	`else`
`168`	`171`	`{`
`169`	`172`	`result = Host.UI.ReadLine();`
`170`	`173`	`}`
`171`	`174`
`172`		`- WriteObject(result);`
	`175`	`+ if (MaskInput)`
	`176`	`+ {`
	`177`	`+ WriteObject(Utils.GetStringFromSecureString((SecureString)result));`
	`178`	`+ }`
	`179`	`+ else`
	`180`	`+ {`
	`181`	`+ WriteObject(result);`
	`182`	`+ }`
`173`	`183`	`}`
`174`	`184`	`}`
`175`	`185`
Original file line number	Diff line number	Diff line change
`@@ -183,11 +183,6 @@ namespace TestHost`
`183`	`183`	`return ReadLineData;`
`184`	`184`	`}`
`185`	`185`
`186`		`- public override string ReadLineMaskedAsString()`
`187`		`- {`
`188`		`- return ReadLineData;`
`189`		`- }`
`190`		`-`
`191`	`186`	`public override SecureString ReadLineAsSecureString()`
`192`	`187`	`{`
`193`	`188`	`SecureString ss = new SecureString();`