search

conradkleinespel / rpassword

Cross platform Rust library to read a password in the terminal (Linux, BSD, OSX, Windows, WASM).
Apache License 2.0
244 stars 38 forks source link

`read_password` fails on Windows when stdin is piped #50

Closed Heliozoa closed 4 years ago

Heliozoa commented 4 years ago

Hi! I ran into the following issue on Windows 10. Could be related to #49 but I'm not sure. With the main.rs below

fn main() {
    rpassword::read_password().unwrap();
}

running echo "asd" | cargo run results in thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 6, kind: Other, message: "The handle is invalid." }'.

The cause is the GetConsoleMode call at https://github.com/conradkleinespel/rpassword/blob/096c3522ddcd809ece27df6e4ac3ee6cfb3175ed/src/lib.rs#L184-L188 which fails because the handle has the wrong type when stdin is piped (I found this question on the topic https://comp.os.ms-windows.programmer.win32.narkive.com/xiPamaWm/getconsolemode-fails-when-input-piped). Subsequent Get/SetConsoleMode calls would also fail.

A simple solution could be to wrap the console mode calls in something like

if unsafe { winapi::um::fileapi::GetFileType(handle) } != winapi::um::winbase::FILE_TYPE_PIPE {
    // ...
}

but I'm not sure if there are any other implications to doing so. If this sounds acceptable, I can make a PR for it. Alternatively it could check that the handle is of type FILE_TYPE_CHAR (related SO answer https://stackoverflow.com/a/3453272) but I felt like making a special case for pipe would be less likely to cause any other issues.

For additional context, I originally bumped into this issue when trying to test a CLI app that asks for a password using rpassword, which tried to run the CLI with Command::new(cli).stdin(Stdio::piped()).spawn() and write into the stdin handle. Also, I don't really know anything about the Windows API so it's possible I've misunderstood or overlooked something. Thanks for your work on rpassword!

conradkleinespel commented 4 years ago

Hey @Heliozoa ! Thanks for reporting this and proposing a PR ! :pray: :+1:

@equalsraf Since you wrote the GetConsoleMode part, would you by any chance be able to review @Heliozoa 's proposal ? Ideally, you'd both agree on a fix, make a PR and I'd then merge.

equalsraf commented 4 years ago

Sure.

I agree with his proposal. Checking the handle type for FILE_TYPE_PIPE has the least potential for disruption even if later we change our minds on this. Any other option requires some research on how FILE_TYPE_CHAR or other types behave. But since this function targets stdin specifically i dont think that is required.