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

OSX Clipboard handling fixes #2100

Merged
merged 33 commits into from Aug 16, 2019

Conversation

@molnard
Copy link
Collaborator

commented Aug 9, 2019

On OSX we encountered clipboard problems in some rare cases. The issue is high importance because users pasting their password and btc addresses through the clipboard.
There are some special characters which will interrupts the encoding process of the string resulting in the text being cut.
First occurence of the issue: #2079

We are currently working on the solution in Avalonia (GUI library) and in Wasabi as well.

  1. Special character - string being cut
    Character '¯' is a special character and it interrupts the encoding process.
    This was an encoding/length problem. The remaining part of the string is cut.

  2. Leading zero ('\0') characters
    The first Clipboard read contains leading zero characters. It is not caused any problems as Wasabi removes the leading and trailing whitespaces by default. It is fixed in Avalonia by making a dummy read on the clipboard after startup.

  3. Add more warnings and ensure consistent handling
    We need to add more warning messages if the pasted or entered password string modified. It is important to let the user know wasabi will not use exactly the same pw. As the wallet can be recovered by using the password, the user must know what was used exactly at wallet generation.

Avalonia fixes:
AvaloniaUI/Avalonia@release/0.8.1...release/0.8.2

- [ ] Recover doorpost-refold wallet. Check status at issue.

  • Maintain compatibility with the old password handling.
  • Add warning that the user will know Wasabi not exactly using the same pw which was pasted.
  • Write a lot of tests with strong password generator if the clipboard write/read gets corrupted and also for noparapasswrodbox.
  • Warning message not appears in some cases in pwbox. Test: insert > 100 Length pw with leading spaces.
  • Final test on OSX
@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

We could introduce an abstraction: "EncryptedSecretPasswordHandler" that could have tests, comment out the specific backwards compatibility issues why we are doing what and if someone touches it without a good reason we can break his hand:)

molnard added 2 commits Aug 9, 2019
@danwalmsley

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

Iv had a good look at the code here, and I think they changes are good.

Im assuming its not finished, because I cant see the integrations of GetPossibleCompatibilityPasswords

molnard added 7 commits Aug 12, 2019
@molnard

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 12, 2019

It is ready to review!

@molnard molnard requested review from nopara73 and lontivero Aug 12, 2019

@danwalmsley

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2019

@molnard Its not much of a big deal but I think the csproj should have just Avalonia.Native 0.8.2 not Avalonia 0.8.2.

@lontivero
Copy link
Collaborator

left a comment

It looks good. I didn't test it yet. I think it need at least one more test to verify the correct password is find.

@molnard

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2019

We could introduce an abstraction: "EncryptedSecretPasswordHandler" that could have tests, comment out the specific backwards compatibility issues why we are doing what and if someone touches it without a good reason we can break his hand:)

Did something similar. I have a function that gives back the list of possible passwords and indicate if a compatibility one was used. Modified one of the core functions in Keymanager which was used to decrypt the pw. It is a low level function so it is used everywhere and cannot be walkarounded. For example daemon will also use this.
https://github.com/zkSNACKs/WalletWasabi/pull/2100/files#diff-76400d6b99f6bb5082f33bdd8144c9d0R570
There will be a warning message in the logs and also on the GUI if password was used in compatibility mode.

@nopara73
Copy link
Collaborator

left a comment

Since I haven't been following what's going on, I was only looking for smells in my review.

WalletWasabi.Gui/WalletWasabi.Gui.csproj Outdated Show resolved Hide resolved
WalletWasabi.Tests/PasswordTests.cs Outdated Show resolved Hide resolved
WalletWasabi/Helpers/Constants.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/Controls/NoparaPasswordBox.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/Controls/NoparaPasswordBox.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/WalletWasabi.Gui.csproj Outdated Show resolved Hide resolved
@molnard

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2019

Avalonia ClipboardTest Removed.
According to @danwalmsley:

I think we cannot build this test without us merging some stuff into avalonia we have a Headless version of avalonia.

Failed ClipboardTestsAsync
Error Message:
System.InvalidOperationException : Call from invalid thread
Stack Trace:
at Avalonia.Threading.Dispatcher.VerifyAccess()
at Avalonia.Rendering.RenderLoop.Add(IRenderLoopTask i)
at Avalonia.Controls.AppBuilderBase1.Setup() at Avalonia.Controls.AppBuilderBase1.Start(AppMainDelegate main, String[] args)
at WalletWasabi.Tests.AvaloniaTest.RunTestAsync(Func`1 testCode) in /Users/vsts/agent/2.155.1/work/1/s/WalletWasabi.Tests/AvaloniaTests.cs:line 66
at WalletWasabi.Tests.AvaloniaTests.ClipboardTestsAsync() in /Users/vsts/agent/2.155.1/work/1/s/WalletWasabi.Tests/AvaloniaTests.cs:line 25
--- End of stack trace from previous location where exception was thrown ---

WalletWasabi.Gui/WalletWasabi.Gui.csproj Outdated Show resolved Hide resolved
molnard added 4 commits Aug 15, 2019
Merge branch 'master' into macpwfix
# Conflicts:
#	WalletWasabi/Services/WalletService.cs
@molnard

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 15, 2019

Code refactor

We decided to separate every Password related code to an individual class (PasswordHelper). This will provide formatting/verification and compatibility functions as well.
This will increase complexity but also ensures decoupling of password related functions. So any changes to this class can cause: if someone touches it without a good reason we can break his hand.

I had to separate every validation method because it is used in a very different way depends on the environment (NoparaPasswordBox, SendTab, TestPassword, WalletInfo, Deamon).

molnard added 4 commits Aug 16, 2019
WalletWasabi.Tests/WalletWasabi.Tests.csproj Outdated Show resolved Hide resolved
azure-pipelines-windows.yml Outdated Show resolved Hide resolved
azure-pipelines-osx.yml Outdated Show resolved Hide resolved
azure-pipelines-linux.yml Outdated Show resolved Hide resolved
molnard added 6 commits Aug 16, 2019
Merge branch 'master' into macpwfix
# Conflicts:
#	azure-pipelines-linux.yml
#	azure-pipelines-osx.yml
#	azure-pipelines-windows.yml

@nopara73 nopara73 merged commit d76e012 into zkSNACKs:master Aug 16, 2019

1 of 4 checks passed

Wasabi.Linux in progress
Details
Wasabi.Osx in progress
Details
Wasabi.Windows in progress
Details
CodeFactor No issues found.
Details

@molnard molnard deleted the molnard:macpwfix branch Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.