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
Merged
Changes from 18 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
066f42a
Change the logic, add more warning.
molnard Aug 9, 2019
27450ea
Tool to reproduce the cut password and tests.
molnard Aug 9, 2019
7d852d9
Clean up the Enter logic of Passwordbox
molnard Aug 9, 2019
5c37303
Add Avalonia directly so shell will use that
molnard Aug 12, 2019
b5ff074
Add pw compatibility.
molnard Aug 12, 2019
d0f8fb3
Better logic.
molnard Aug 12, 2019
ce02e0a
Add Avalonia.Native
molnard Aug 12, 2019
6398f0a
Add UI warn messages.
molnard Aug 12, 2019
7fcfacf
Fix display error of warn messages
molnard Aug 12, 2019
d8acef9
Code clean.
molnard Aug 12, 2019
0f31d07
Add PasswordTests to CI
molnard Aug 13, 2019
fc7c6ae
Remove unrequired Ref to Avalonia
molnard Aug 13, 2019
6ff65c6
Code clean.
molnard Aug 13, 2019
69d2475
Encoding.Default go home.
molnard Aug 13, 2019
b355f11
Update WalletWasabi/Helpers/Constants.cs
molnard Aug 13, 2019
ad2fe89
Add Avalonia Clipboard tests
molnard Aug 13, 2019
d74a560
Try to fix CI
molnard Aug 13, 2019
cb60045
Removed AvaloniaTests - cannot make it work.
molnard Aug 13, 2019
197cf83
Add OS selectivity
molnard Aug 14, 2019
d7fde2d
Code refactor - pw separation
molnard Aug 15, 2019
fe1dba6
Add functions to Daemon
molnard Aug 15, 2019
5a663a4
Merge branch 'master' into macpwfix
molnard Aug 15, 2019
3664f21
Remove Avalonia.Native
molnard Aug 15, 2019
10472f8
Overfee want overwritten fix
molnard Aug 16, 2019
8a26e10
Add readonly.
molnard Aug 16, 2019
6a758ee
Grammar.
molnard Aug 16, 2019
9258494
Add test.
molnard Aug 16, 2019
83e3d7f
Merge branch 'master' into macpwfix
molnard Aug 16, 2019
3e1d558
Remove GUI from tests.
molnard Aug 16, 2019
d79f64b
Guard when generating wallet.
molnard Aug 16, 2019
53d33cf
Add tests.
molnard Aug 16, 2019
392615a
Use PasswordHelper.IsTooLong.
molnard Aug 16, 2019
ca5099b
Refactor Enter on pwbox.
molnard Aug 16, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -35,10 +35,12 @@ public class NoparaPasswordBox : ExtendedTextBox, IStyleable
"如果你是只宠物小精灵,我就选你。" //If you were a Pokemon, I'd choose you.
};

private static string PasswordTooLongMessage = $"Password too long (Max {Constants.MaxPasswordLength} characters).";
This conversation was marked as resolved by molnard

This comment has been minimized.

Copy link
@yahiheb

yahiheb Aug 14, 2019

Collaborator

Should be camelCase.

This comment has been minimized.

Copy link
@molnard

molnard Aug 16, 2019

Author Collaborator

What do you mean?

This comment has been minimized.

Copy link
@yahiheb

yahiheb Aug 16, 2019

Collaborator

Forget it, it is correct as is.


private static Key[] SuppressedKeys { get; } =
{
Key.LeftCtrl, Key.RightCtrl, Key.LeftAlt, Key.RightAlt, Key.LeftShift, Key.RightShift, Key.Escape, Key.CapsLock, Key.NumLock, Key.LWin, Key.RWin,
Key.Left, Key.Right, Key.Up, Key.Down
Key.Left, Key.Right, Key.Up, Key.Down, Key.Enter
};

private bool _supressChanges;
@@ -114,7 +116,7 @@ public NoparaPasswordBox()
IsPasswordVisible = x;
});

this.WhenAnyValue(x => x.IsPasswordVisible).Subscribe(IsVisible =>
this.WhenAnyValue(x => x.IsPasswordVisible).Subscribe(_ =>
{
PaintText();
});
@@ -239,10 +241,6 @@ protected override async void OnKeyDown(KeyEventArgs e)
if (!string.IsNullOrEmpty(text))
{
e.Handled = OnTextInput(text, true);
if (!e.Handled)
{
_ = DisplayWarningAsync("Password too long (Max 150 characters)");
}
}
}
else if (Sb.Length > 0)
@@ -297,9 +295,17 @@ protected override async void OnKeyDown(KeyEventArgs e)

protected override void OnTextInput(TextInputEventArgs e)
{
e.Handled = OnTextInput(e.Text, false);
var isPaste = e.Text != null && e.Text.Length > 1; // if the ExtendedTextBox right click/Paste is used, OnTextInput will be called directly

e.Handled = OnTextInput(e.Text, isPaste);
}

/// <summary>
/// All text input operation (keydown/paste/delete) should call this.
/// </summary>
/// <param name="text"></param>
/// <param name="isPaste"></param>
/// <returns></returns>
private bool OnTextInput(string text, bool isPaste)
{
if (_supressChanges)
@@ -315,9 +321,10 @@ private bool OnTextInput(string text, bool isPaste)
SelectionStart = SelectionEnd = CaretIndex = 0;
_supressChanges = false;
}
if (isPaste && Sb.Length + text.Length > Constants.MaxPasswordLength) // Do not allow pastes that would be too long
if (Sb.Length + text.Length > Constants.MaxPasswordLength) // Do not allow insert that would be too long.
{
handledCorrectly = false;
_ = DisplayWarningAsync(PasswordTooLongMessage);
This conversation was marked as resolved by molnard

This comment has been minimized.

Copy link
@nopara73

nopara73 Aug 13, 2019

Collaborator

Any way to avoid this being called asynchronously?

This comment has been minimized.

Copy link
@molnard

molnard Aug 13, 2019

Author Collaborator

We are doing the same elsewhere so let it be, this will be replaced by notifications.

}
else if (CaretIndex == 0)
{
@@ -328,11 +335,13 @@ private bool OnTextInput(string text, bool isPaste)
Sb.Append(text);
}

if (handledCorrectly && Sb.Length > Constants.MaxPasswordLength) // Ensure the maximum length.
if (handledCorrectly && Sb.Length > Constants.MaxPasswordLength) // We should not get here, ensure the maximum length.
{
Sb.Remove(Constants.MaxPasswordLength, Sb.Length - Constants.MaxPasswordLength);
handledCorrectly = false; // Should play beep sound not working on windows.
_ = DisplayWarningAsync(PasswordTooLongMessage);
This conversation was marked as resolved by molnard

This comment has been minimized.

Copy link
@nopara73

nopara73 Aug 13, 2019

Collaborator

Any way to avoid this being called asynchronously?

This comment has been minimized.

Copy link
@molnard

molnard Aug 13, 2019

Author Collaborator

We are doing the same elsewhere so let it be, this will be replaced by notifications.

}

PaintText();
return handledCorrectly;
}
@@ -381,8 +390,21 @@ private void PaintText()
{
GenerateNewRandomSequence();
}
var password = Sb.ToString();

var beforeTrim = password.Length;

password = password.Trim();

if (beforeTrim != password.Length)
{
Sb.Clear();
Sb.Append(password);
_ = DisplayWarningAsync("Leading and trailing white spaces were removed!");
}

Password = password;

Password = Sb.ToString();
Text = _displayText.Substring(0, Sb.Length);
if (IsPasswordVisible)
{
@@ -9,7 +9,7 @@
<Setter Property="Template">
<ControlTemplate>
<Border Name="border" Background="{TemplateBinding Background}" BorderBrush="{TemplateBinding BorderBrush}" BorderThickness="{TemplateBinding BorderThickness}">
<DockPanel Margin="{TemplateBinding Padding}">
<DockPanel Margin="{TemplateBinding Padding}" LastChildFill ="true">
<TextBlock Name="floatingWatermark" Foreground="{DynamicResource ThemeAccentBrush}" FontSize="{DynamicResource FontSizeSmall}" Text="{TemplateBinding Watermark}" DockPanel.Dock="Top">
<TextBlock.IsVisible>
<MultiBinding Converter="{x:Static BoolConverters.And}">
@@ -34,6 +34,11 @@
<Binding RelativeSource="{RelativeSource TemplatedParent}" Path="Text" Converter="{x:Static StringConverters.IsNotNullOrEmpty}" />
</Button.IsVisible>
</Button>
<TextBlock DockPanel.Dock="Bottom"
Name="warningMessage"
Classes="warningMessage"
Text="{TemplateBinding WarningMessage}"
IsVisible="{TemplateBinding WarningMessage, Converter={x:Static StringConverters.IsNotNullOrEmpty}}" />
<DataValidationErrors>
<ScrollViewer
HorizontalScrollBarVisibility="{TemplateBinding (ScrollViewer.HorizontalScrollBarVisibility)}"
@@ -57,11 +62,6 @@
TextWrapping="{TemplateBinding TextWrapping}"
PasswordChar="{TemplateBinding PasswordChar}"
IsVisible="{TemplateBinding WarningMessage, Converter={x:Static StringConverters.IsNullOrEmpty}}" />
<TextBlock
Name="warningMessage"
Classes="warningMessage"
Text="{TemplateBinding WarningMessage}"
IsVisible="{TemplateBinding WarningMessage, Converter={x:Static StringConverters.IsNotNullOrEmpty}}" />
</Panel>
</ScrollViewer>
</DataValidationErrors>
@@ -180,27 +180,6 @@ public SendTabViewModel(WalletViewModel walletViewModel, bool isTransactionBuild
SetSendText();
});

this.WhenAnyValue(x => x.Password)
.ObserveOn(RxApp.MainThreadScheduler)
.Subscribe(x =>
{
try
{
if (x.NotNullAndNotEmpty())
{
char lastChar = x.Last();
if (lastChar == '\r' || lastChar == '\n') // If the last character is cr or lf then act like it'd be a sign to do the job.
{
Password = x.TrimEnd('\r', '\n');
}
}
}
catch (Exception ex)
{
Logging.Logger.LogTrace(ex);
}
});

this.WhenAnyValue(x => x.Label)
.ObserveOn(RxApp.MainThreadScheduler)
.Subscribe(UpdateSuggestions);
@@ -833,6 +812,11 @@ private void TryResetInputsOnSuccess(string successMessage)
ResetUi();

SetSuccessMessage(successMessage);

if (KeyManager.IsCompatibilityPasswordUsed)
{
WarningMessage = Constants.CompatibilityPasswordWarnMessage;
}
}
catch (Exception ex)
{
@@ -62,6 +62,11 @@ public WalletInfoViewModel(WalletViewModel walletViewModel) : base(walletViewMod
var secret = KeyManager.GetMasterExtKey(Password);
Password = "";

if (KeyManager.IsCompatibilityPasswordUsed)
{
SetWarningMessage(Constants.CompatibilityPasswordWarnMessage);
}

string master = secret.GetWif(Global.Network).ToWif();
string account = secret.Derive(KeyManager.AccountKeyPath).GetWif(Global.Network).ToWif();
string masterZ = secret.ToZPrv(Global.Network);
@@ -601,7 +601,13 @@ public async Task<KeyManager> LoadKeyManagerAsync(bool requirePassword, bool isH
{
if (keyManager.TestPassword(password))
{
SetSuccessMessage("Correct password.");
SuccessMessage = "Correct password.";
if (keyManager.IsCompatibilityPasswordUsed)
{
WarningMessage = Constants.CompatibilityPasswordWarnMessage;
ValidationMessage = "";
}

keyManager.SetPasswordVerified();
}
else
@@ -64,6 +64,7 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Avalonia.Native" Version="0.8.2" />
This conversation was marked as resolved by molnard

This comment has been minimized.

Copy link
@nopara73

nopara73 Aug 13, 2019

Collaborator

This is still not resolved.

This comment has been minimized.

Copy link
@molnard

molnard Aug 14, 2019

Author Collaborator

AvalonStudio.Shell references this library as well, so basically there is no new reference.

With Avalonia 0.9 we will need to do the same thing. Both Shell and Avalonia will be referenced.

This comment has been minimized.

Copy link
@nopara73

nopara73 Aug 14, 2019

Collaborator

That's not what we agreed on with @danwalmsley. We use everything Avalonia through the shell.

This comment has been minimized.

Copy link
@molnard

molnard Aug 16, 2019

Author Collaborator

Solved!

<PackageReference Include="AvalonStudio.Shell" Version="0.8.1" />
</ItemGroup>

@@ -0,0 +1,31 @@
using System;
using System.Collections.Generic;
using System.Text;
using WalletWasabi.Helpers;
using Xunit;

namespace WalletWasabi.Tests
{
public class PasswordTests
{
[Fact]
public void PasswordClipboardCutTest()
This conversation was marked as resolved by molnard

This comment has been minimized.

Copy link
@lontivero

lontivero Aug 15, 2019

Collaborator

I think this UT is not enough. The main method to test seems to be PasswordHelper::GetMasterExtKey.

This comment has been minimized.

Copy link
@molnard

molnard Aug 16, 2019

Author Collaborator

True, adding more tests.

{
string original = " w¾3AÍ-dCdï×¾M\\Øò¹ãÔÕýÈÝÁÐ9oEp¨}r:SR¦·ßNó±¥*W!¢ê#ikÇå<ðtÇf·a\\]§,à±H7«®È4nèNmæo4.qØ-¾ûda¯ºíö¾,¥¢½\\¹õèKeÁìÍSÈ@r±ØÙ2[r©UQÞ¶xN\"?:Ö@°&\n";
string desired = " w¾3AÍ-dCdï×¾M\\Øò¹ãÔÕýÈÝÁÐ9oEp¨}r:SR¦·ßNó±¥*W!¢ê#ikÇå<ðtÇf·a\\]§,à±H7«®È4nèNmæo4.qØ-¾ûda¯";
var results = PasswordHelper.GetPossiblePasswords(original);
var foundCorrectPassword = false;

foreach (var pw in results)
{
if (pw == desired)
{
foundCorrectPassword = true;
break;
}
}

Assert.True(foundCorrectPassword);
}
}
}
@@ -26,6 +26,7 @@

<ItemGroup>
<ProjectReference Include="..\WalletWasabi.Backend\WalletWasabi.Backend.csproj" />
<ProjectReference Include="..\WalletWasabi.Gui\WalletWasabi.Gui.csproj" />
This conversation was marked as resolved by molnard

This comment has been minimized.

Copy link
@nopara73

nopara73 Aug 16, 2019

Collaborator

Not needed anymore.

This comment has been minimized.

Copy link
@molnard

molnard Aug 16, 2019

Author Collaborator

Fixed.

<ProjectReference Include="..\WalletWasabi\WalletWasabi.csproj" />
</ItemGroup>

@@ -95,5 +95,7 @@ public static BitcoinWitPubKeyAddress GetCoordinatorAddress(Network network)
public const int DefaultRegTestBitcoinCoreRpcPort = 18443;

public const string Chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";

public const string CompatibilityPasswordWarnMessage = "Compatibility password was used! Please consider generating a new wallet to ensure recoverability!";
}
}
@@ -0,0 +1,31 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.InteropServices;
using System.Text;

namespace WalletWasabi.Helpers
{
public static class PasswordHelper
{
public static string[] GetPossiblePasswords(string originalPassword)
{
List<string> possiblePasswords = new List<string>()
{
originalPassword,
StringCutIssue(originalPassword)
This conversation was marked as resolved by molnard

This comment has been minimized.

Copy link
@lontivero

lontivero Aug 13, 2019

Collaborator

Q: should we do this for all the OS or only for OSX?

This comment has been minimized.

Copy link
@molnard

molnard Aug 14, 2019

Author Collaborator

Yes I agree. I fixed it.

};

return possiblePasswords.ToArray();
}

private static string StringCutIssue(string text)
{
// On OSX Avalonia gets the string from the Clipboard as byte[] and size.
// The size was mistakenly taken from the size of the original string which is not correct because of the UTF8 encoding.
byte[] bytes = Encoding.UTF8.GetBytes(text);
var myString = Encoding.UTF8.GetString(bytes.Take(text.Length).ToArray());
return text.Substring(0, myString.Length);
}
}
}
@@ -76,6 +76,8 @@ public class KeyManager

public const int AbsoluteMinGapLimit = 21;

public bool IsCompatibilityPasswordUsed { get; private set; }

[JsonConstructor]
public KeyManager(BitcoinEncryptedSecretNoEC encryptedSecret, byte[] chainCode, HDFingerprint? masterFingerprint, ExtPubKey extPubKey, bool? passwordVerified, int? minGapLimit, BlockchainState blockchainState, string filePath = null, KeyPath accountKeyPath = null)
{
@@ -552,6 +554,7 @@ public IEnumerable<(ExtKey secret, HdPubKey pubKey)> GetSecretsAndPubKeyPairs(st

public ExtKey GetMasterExtKey(string password)
{
IsCompatibilityPasswordUsed = false;
if (password is null)
{
password = "";
@@ -562,23 +565,37 @@ public ExtKey GetMasterExtKey(string password)
throw new SecurityException("This is a watchonly wallet.");
}

try
Exception resultException = null;

foreach (var pw in PasswordHelper.GetPossiblePasswords(password))
{
Key secret = EncryptedSecret.GetKey(password);
var extKey = new ExtKey(secret, ChainCode);
try
{
Key secret = EncryptedSecret.GetKey(pw);

var extKey = new ExtKey(secret, ChainCode);

// Backwards compatibility:
if (MasterFingerprint is null)
{
MasterFingerprint = secret.PubKey.GetHDFingerPrint();
}

// Backwards compatibility:
if (MasterFingerprint is null)
if (IsCompatibilityPasswordUsed)
{
Logger.LogError<KeyManager>(Constants.CompatibilityPasswordWarnMessage);
}
return extKey;
}
catch (SecurityException ex)
{
MasterFingerprint = secret.PubKey.GetHDFingerPrint();
resultException = ex;
}

return extKey;
}
catch (SecurityException ex)
{
throw new SecurityException("Invalid password.", ex);
IsCompatibilityPasswordUsed = true; // The first iteration will try the original password if we get here it was failed.
}

throw new SecurityException("Invalid password.", resultException);
}

public bool TestPassword(string password)
@@ -18,4 +18,4 @@ jobs:
inputs:
command: test
projects: 'WalletWasabi.Tests/WalletWasabi.Tests.csproj'
arguments: --configuration $(testConfiguration) --filter "CryptoTests | ExtPubKeyExplorerTests | KeyManagementTests | ModelTests | StoreTests | ParserTests | NBitcoinTests"
arguments: --configuration $(testConfiguration) --filter "CryptoTests | ExtPubKeyExplorerTests | KeyManagementTests | ModelTests | StoreTests | ParserTests | NBitcoinTests | PasswordTests"
This conversation was marked as resolved by molnard

This comment has been minimized.

Copy link
@nopara73

nopara73 Aug 16, 2019

Collaborator

Conflict.

This comment has been minimized.

Copy link
@molnard

molnard Aug 16, 2019

Author Collaborator

Fixed.

@@ -18,4 +18,4 @@ jobs:
inputs:
command: test
projects: 'WalletWasabi.Tests/WalletWasabi.Tests.csproj'
arguments: --configuration $(testConfiguration) --filter "CryptoTests | ExtPubKeyExplorerTests | KeyManagementTests | ModelTests | StoreTests | ParserTests| NBitcoinTests"
arguments: --configuration $(testConfiguration) --filter "CryptoTests | ExtPubKeyExplorerTests | KeyManagementTests | ModelTests | StoreTests | ParserTests| NBitcoinTests | PasswordTests"
This conversation was marked as resolved by molnard

This comment has been minimized.

Copy link
@nopara73

nopara73 Aug 16, 2019

Collaborator

Conflict.

This comment has been minimized.

Copy link
@molnard

molnard Aug 16, 2019

Author Collaborator

Fixed.

@@ -18,4 +18,4 @@ jobs:
inputs:
command: test
projects: 'WalletWasabi.Tests/WalletWasabi.Tests.csproj'
arguments: --configuration $(testConfiguration) --filter "CryptoTests | ExtPubKeyExplorerTests | KeyManagementTests | ModelTests | StoreTests | ParserTests| NBitcoinTests"
arguments: --configuration $(testConfiguration) --filter "CryptoTests | ExtPubKeyExplorerTests | KeyManagementTests | ModelTests | StoreTests | ParserTests| NBitcoinTests | PasswordTests"
This conversation was marked as resolved by molnard

This comment has been minimized.

Copy link
@nopara73

nopara73 Aug 16, 2019

Collaborator

Conflict.

This comment has been minimized.

Copy link
@molnard

molnard Aug 16, 2019

Author Collaborator

Fixed.

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.