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

It's possible to unlock the wallet with an empty password #241

Closed
neura-sx opened this issue Sep 24, 2015 · 8 comments
Closed

It's possible to unlock the wallet with an empty password #241

neura-sx opened this issue Sep 24, 2015 · 8 comments
Assignees

Comments

@neura-sx
Copy link

When I lock the wallet (using the lock button in the top right corner) and then try to unlock it (using the same button in the top right corner) the application allows me to leave the password field empty and still unlock the wallet.

unlock-empty-bug

@svk31
Copy link
Contributor

svk31 commented Sep 24, 2015

@jcalfee This is because the password is never reset inside the unlockmodal, so if you unlock, lock then try to unlock again, the last password that you entered (which was the right one) is still stored in the the UnlockModal password_ui variable. If you don't try to enter a new password it won't get overwritten, so the unlock is successful. A simple fix is to just reset it on close events and successful login.

I noticed you aren't using this.state to save the password, for security reasons I guess?

@jcalfee jcalfee self-assigned this Sep 24, 2015
jcalfee pushed a commit that referenced this issue Sep 24, 2015
…tate). This fixes a bug that allowed a wallet to be reopened after being locked. #241
@jcalfee
Copy link

jcalfee commented Sep 24, 2015

The first issue should be fixed by removing the password from the state. Now only the modal password input field has the data.

That exposes something else:

The model package react-foundation-apps/src/modal does not trigger any events when the user clicks away from a model (only the X or cancel) .. This could be used to reset the password should the user click on the background (instead of the cancel button). Instead, after a click away then Unlock again the password is still in the dialog (as stars).

jcalfee pushed a commit that referenced this issue Sep 24, 2015
…rlayClose pending a bug fix in react-foundation-apps #241
@neura-sx
Copy link
Author

I can see that the main problem is now fixed but there is still a minor problem: when I try to unlock the wallet, enter a wrong password, then dismiss the pop-up and then try to unlock the wallet again - the pop-up still shows the error from my previous attempt.

unlock-redo-bug

@neura-sx
Copy link
Author

Also, I think it would be nice if the workflow was like this:
The "incorrect password" message should appear on my attempt to unlock the wallet with a wrong (or empty) password but then the pop-up should be reset, i.e. the text input field should be cleared and as soon as I start reentering the password the error message should be gone.

Otherwise it's a bit confusing - the error message is still visible but no longer applicable.

@jcalfee
Copy link

jcalfee commented Sep 29, 2015

Not sure why, my commit closed the case.. Guess github thought "close #241" meant close the case.

@jcalfee jcalfee reopened this Sep 29, 2015
@jcalfee
Copy link

jcalfee commented Sep 29, 2015

@valzav can you look into a way for PasswordInput.jsx to clear the Invalid Password error message just after the user changes the input text? As neura-sx says, once they start typing again that error message is no longer valid. I agree completely. I notice you created the PasswordInput so maybe you have an idea on how best to do that.

If you did want to clear the text as was also suggested I have seen the select all feature used after an invalid Enter event. This gives the user the opportunity to typeover everything or to keep it. I don't think this feature should consume any time though, only if it is very easy...

@neura-sx
Copy link
Author

neura-sx commented Oct 6, 2015

After the latest upgrade, when trying to unlock the wallet this error shows up in the console as soon as I start typing the password:

--ReferenceError: event is not defined

It happens only in Firefox (Win 7). Maybe it's not critical as it does not crash the app.

@jcalfee
Copy link

jcalfee commented Oct 8, 2015

The main issue is fixed. I have a mental note to improve dialog (with the other suggestion) the RED error behaviour so I'm closing the ticket now. If we end up missing this after a while please reopen under another ticket.

@jcalfee jcalfee closed this as completed Oct 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants