Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor wallet recover fixes #2201

Conversation

ignopeverell
Copy link
Contributor

Couple of those are security sensitive:

  1. Do not log the recovery phrase on error.
  2. Prompt for the phrase instead of passing it as a command arg so it doesn't appear in any command line history.

@ignopeverell ignopeverell added this to the Mainnet milestone Dec 21, 2018
@yeastplume
Copy link
Member

yeastplume commented Dec 21, 2018

Good catch on log. I tried to fix the recovery phrase entry issue earlier in the same way, but found it a bit problematic from a UX experience, see: #2174

Basically, (at least on my system) stdin ignores any keymaps, so any attempt to erase or hit arrow keys as you're typing just creates a bigger mess. Agreed doing it this way is better than leaving it in your shell history, but still.. hmm..

@yeastplume
Copy link
Member

yeastplume commented Dec 22, 2018

1 ) ____ 2) ____ 3) _____ (print to 12 or 24)
(Q to quit, 1-24 to edit a word)

Type word 1 > pants

1 ) pants 2) ____ 3) _____ (print to 12 or 24)
(Q to quit, 1-24 to edit a word)

Type word 2 > yeast

Something like this? And bonus points if we can figure out the equivalent of typing ‘stty erase ’, to ensure delete works

Fun little recovery phrase widget from your intro to programming with Pascal class.

@ignopeverell
Copy link
Contributor Author

Ah, missed #2174. Not sure what's this UX thing you're talking about though.

Btw, hate to keep adding more dependencies but isn't that what readline is for? We could pick a Rust port (like https://github.com/murarth/linefeed).

@yeastplume
Copy link
Member

Only problem with that linefeed crate is that I didn’t know it existed until now.

@ignopeverell
Copy link
Contributor Author

Superseded by #2276

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants