fix: strip newline and carriage return when password is read from stdin #1191
Conversation
nico1510
force-pushed
the
fix-newline-char-from-stdin-input
branch
from
526b32d
to
b8fd4f5
Compare
There was a problem hiding this comment.
Thanks for catching this and for taking the time to fix it. I've asked @radu-matei to review since he knows this code best, but apart from one (non-critical) question it looks fine to me.
src/commands/registry.rs
Outdated
| @@ -143,6 +143,9 @@ impl Login { | |||
| let mut buf = String::new(); | |||
| let mut stdin = std::io::stdin().lock(); | |||
| stdin.read_to_string(&mut buf)?; | |||
| // trim trailing newlines and carriage returns (see https://blog.v-gar.de/2019/04/rust-remove-trailing-newline-after-input) | |||
| let len = buf.trim_end_matches(&['\r', '\n'][..]).len(); | |||
| buf.truncate(len); | |||
There was a problem hiding this comment.
Is there a reason to use this length to truncate the original buf rather than return buf.trim_end_matches(...).to_owned()? That feels more readable to me and I'm not worried if it incurs an extra allocation. But I'm not familiar with this code and don't know the nuances of the string functions - I'll defer to you and Radu on this!
There was a problem hiding this comment.
I think the only justification for the length variant would be to "fix" the faulty buf by mutating it. But yeah I agree that yours is much more readable. Pushed a change 👍
nico1510
force-pushed
the
fix-newline-char-from-stdin-input
branch
2 times, most recently
from
6581c45
to
4537f2f
Compare
|
Hi, @nico1510! I haven't been able to reproduce this locally, so I assume there is something OS / shell specific happening here? Would you mind sharing your environment, please? Thanks, and thank you for the fix, really appreciate taking the time! |
nico1510
force-pushed
the
fix-newline-char-from-stdin-input
branch
from
4537f2f
to
4b579bc
Compare
Yep sure I'm on: zsh 5.8.1 (x86_64-apple-darwin22.0) |
Signed-off-by: Nico Beck <[email protected]>
nico1510
force-pushed
the
fix-newline-char-from-stdin-input
branch
from
4b579bc
to
4a36b24
Compare
|
@radu-matei How do you want to proceed with this one? Is there any risk or harm in merging it even though it might be environment-specific and we can't reproduce the issue? Or do you want to try and qualify the issue further? And @nico1510 my apologies for the long silence! |
|
@radu-matei bump |
After reading https://www.fermyon.com/blog/spin-v09#:~:text=Running%20Spin%20Applications%20From%20Registries I tried to login to docker hub but wasn't successful. I traced it down to a newline character which is apparently added when reading from stdin with rust (see https://blog.v-gar.de/2019/04/rust-remove-trailing-newline-after-input).
This command failed:
echo $DOCKER_PAT | spin registry login registry-1.docker.io --username $user --password-stdinI also had to manually create the
registry-auth.jsonwherespintries to store the saved credentials but that's just a fyi and is not fixed in this PR.