Bug 1763: Validation of input boxes prevented cancelling dialogs using Cancel button

https://winscp.net/tracker/1763

Source commit: 7b6375bef21ce787a2382434b901501fc4696409

source/forms/CopyParamPreset.cpp

@@ -167,9 +167,14 @@ TCopyParamRule * __fastcall TCopyParamPresetDialog::GetRule()
   if (HasRuleCheck->Checked)
   {
     TCopyParamRuleData Data;
+    // Last resort check, in case the mask escapes validation in OnExit by IsCancelButtonBeingClicked
+    ValidateMask(HostNameEdit->Text);
     Data.HostName = HostNameEdit->Text;
+    ValidateMask(UserNameEdit->Text);
     Data.UserName = UserNameEdit->Text;
+    ValidateMask(RemoteDirectoryEdit->Text);
     Data.RemoteDirectory = RemoteDirectoryEdit->Text;
+    ValidateMask(LocalDirectoryEdit->Text);
     Data.LocalDirectory = LocalDirectoryEdit->Text;
     Rule = new TCopyParamRule(Data);
   }

source/forms/CopyParams.cpp

@@ -311,7 +311,7 @@ void __fastcall TCopyParamsFrame::UpdateRightsByStr()
 //---------------------------------------------------------------------------
 void __fastcall TCopyParamsFrame::RightsEditExit(TObject * /*Sender*/)
 {
-  if (RightsEdit->Modified)
+  if (RightsEdit->Modified && !IsCancelButtonBeingClicked(this))
   {
     try
     {
@@ -334,16 +334,19 @@ void __fastcall TCopyParamsFrame::RightsEditContextPopup(TObject * Sender,
 //---------------------------------------------------------------------------
 void __fastcall TCopyParamsFrame::SpeedComboExit(TObject * /*Sender*/)
 {
-  try
+  if (!IsCancelButtonBeingClicked(this))
   {
-    SpeedCombo->Text = SetSpeedLimit(GetSpeedLimit(SpeedCombo->Text));
-  }
-  catch (Exception & E)
-  {
-    ShowExtendedException(&E);
-    SpeedCombo->SetFocus();
-    SpeedCombo->SelectAll();
-    Abort();
+    try
+    {
+      SpeedCombo->Text = SetSpeedLimit(GetSpeedLimit(SpeedCombo->Text));
+    }
+    catch (Exception & E)
+    {
+      ShowExtendedException(&E);
+      SpeedCombo->SetFocus();
+      SpeedCombo->SelectAll();
+      Abort();
+    }
   }
 }
 //---------------------------------------------------------------------------

source/forms/Preferences.cpp

@@ -937,7 +937,8 @@ void __fastcall TPreferencesDialog::SaveConfiguration()
     Configuration->LogFileName = LogFileNameEdit3->Text;
     Configuration->LogFileAppend = LogFileAppendButton->Checked;
     __int64 LogMaxSize;
-    if (LogMaxSizeCheck->Checked && DebugAlwaysTrue(TryStrToSize(LogMaxSizeCombo->Text, LogMaxSize)))
+    // TryStrToSize can fail, only if LogMaxSizeComboExit is bypassed due to IsCancelButtonBeingClicked
+    if (LogMaxSizeCheck->Checked && TryStrToSize(LogMaxSizeCombo->Text, LogMaxSize))
     {
       Configuration->LogMaxSize = LogMaxSize;
     }
@@ -2495,40 +2496,43 @@ void __fastcall TPreferencesDialog::PanelFontLabelDblClick(TObject * Sender)
 //---------------------------------------------------------------------------
 void __fastcall TPreferencesDialog::UpdatesAuthenticationEmailEditExit(TObject * /*Sender*/)
 {
-  if (FVerifiedUpdatesAuthenticationEmail != UpdatesAuthenticationEmailEdit->Text)
+  if (!IsCancelButtonBeingClicked(this))
   {
-    if (!UpdatesAuthenticationEmailEdit->Text.IsEmpty())
+    if (FVerifiedUpdatesAuthenticationEmail != UpdatesAuthenticationEmailEdit->Text)
     {
-      TUpdatesConfiguration Updates = SaveUpdates();
-
+      if (!UpdatesAuthenticationEmailEdit->Text.IsEmpty())
       {
-        TInstantOperationVisualizer Visualizer;
-        QueryUpdates(Updates);
-      }
+        TUpdatesConfiguration Updates = SaveUpdates();
 
-      UnicodeString AuthenticationError = Updates.Results.AuthenticationError;
-      if (!AuthenticationError.IsEmpty())
-      {
-        AuthenticationError = FormatUpdatesMessage(AuthenticationError);
-        if (HasParagraphs(AuthenticationError))
-        {
-          AuthenticationError = MainInstructionsFirstParagraph(AuthenticationError);
-        }
-        else
         {
-          AuthenticationError = MainInstructions(AuthenticationError);
+          TInstantOperationVisualizer Visualizer;
+          QueryUpdates(Updates);
         }
 
-        unsigned int Result =
-          MoreMessageDialog(AuthenticationError, NULL, qtError, qaIgnore | qaAbort, HELP_AUTOMATIC_UPDATE);
-        if (Result == qaAbort)
+        UnicodeString AuthenticationError = Updates.Results.AuthenticationError;
+        if (!AuthenticationError.IsEmpty())
         {
-          Abort();
+          AuthenticationError = FormatUpdatesMessage(AuthenticationError);
+          if (HasParagraphs(AuthenticationError))
+          {
+            AuthenticationError = MainInstructionsFirstParagraph(AuthenticationError);
+          }
+          else
+          {
+            AuthenticationError = MainInstructions(AuthenticationError);
+          }
+
+          unsigned int Result =
+            MoreMessageDialog(AuthenticationError, NULL, qtError, qaIgnore | qaAbort, HELP_AUTOMATIC_UPDATE);
+          if (Result == qaAbort)
+          {
+            Abort();
+          }
         }
       }
-    }
 
-    FVerifiedUpdatesAuthenticationEmail = UpdatesAuthenticationEmailEdit->Text;
+      FVerifiedUpdatesAuthenticationEmail = UpdatesAuthenticationEmailEdit->Text;
+    }
   }
 }
 //---------------------------------------------------------------------------
@@ -2915,29 +2919,35 @@ void __fastcall TPreferencesDialog::LanguagesViewCustomDrawItem(
 void __fastcall TPreferencesDialog::LogMaxSizeComboExit(TObject * /*Sender*/)
 {
   __int64 Size;
-  if (!TryStrToSize(LogMaxSizeCombo->Text, Size))
-  {
-    LogMaxSizeCombo->SetFocus();
-    throw Exception(FMTLOAD(SIZE_INVALID, (LogMaxSizeCombo->Text)));
-  }
-  else
+  if (!IsCancelButtonBeingClicked(this))
   {
-    LogMaxSizeCombo->Text = SizeToStr(Size);
+    if (!TryStrToSize(LogMaxSizeCombo->Text, Size))
+    {
+      LogMaxSizeCombo->SetFocus();
+      throw Exception(FMTLOAD(SIZE_INVALID, (LogMaxSizeCombo->Text)));
+    }
+    else
+    {
+      LogMaxSizeCombo->Text = SizeToStr(Size);
+    }
   }
 }
 //---------------------------------------------------------------------------
 void __fastcall TPreferencesDialog::PuttyPathEditExit(TObject * /*Sender*/)
 {
-  try
-  {
-    UnicodeString Program, AParams, Dir;
-    SplitCommand(PuttyPathEdit->Text, Program, AParams, Dir);
-  }
-  catch(...)
+  if (!IsCancelButtonBeingClicked(this))
   {
-    PuttyPathEdit->SelectAll();
-    PuttyPathEdit->SetFocus();
-    throw;
+    try
+    {
+      UnicodeString Program, AParams, Dir;
+      SplitCommand(PuttyPathEdit->Text, Program, AParams, Dir);
+    }
+    catch(...)
+    {
+      PuttyPathEdit->SelectAll();
+      PuttyPathEdit->SetFocus();
+      throw;
+    }
   }
 }
 //---------------------------------------------------------------------------
@@ -2993,7 +3003,15 @@ void __fastcall TPreferencesDialog::CustomIniFileStorageChanged()
 //---------------------------------------------------------------------------
 void __fastcall TPreferencesDialog::CustomIniFileStorageEditExit(TObject * /*Sender*/)
 {
-  CustomIniFileStorageChanged();
+  if (!IsCancelButtonBeingClicked(this))
+  {
+    CustomIniFileStorageChanged();
+  }
+  else
+  {
+    // Reset the value to prevent accidental overwide of an INI file, in case the dialog cancel does not complete
+    CustomIniFileStorageEdit->Text = FCustomIniFileStorageName;
+  }
 }
 //---------------------------------------------------------------------------
 void __fastcall TPreferencesDialog::CustomIniFileStorageEditAfterDialog(TObject * Sender, UnicodeString & Name, bool & Action)

source/forms/Properties.cpp

@@ -734,31 +734,34 @@ void __fastcall TPropertiesDialog::ChecksumViewContextPopup(
   MenuPopup(Sender, MousePos, Handled);
 }
 //---------------------------------------------------------------------------
-void __fastcall TPropertiesDialog::ResolveRemoteToken(
+void __fastcall TPropertiesDialog::ValidateRemoteToken(
   const TRemoteToken & Orig, int Message, TComboBox * ComboBox,
   const TRemoteTokenList * List)
 {
-  try
-  {
-    ComboBox->Text =
-      LoadRemoteToken(StoreRemoteToken(Orig, ComboBox->Text, Message, List));
-  }
-  catch(...)
+  if (!IsCancelButtonBeingClicked(this))
   {
-    ComboBox->SetFocus();
-    throw;
+    try
+    {
+      ComboBox->Text =
+        LoadRemoteToken(StoreRemoteToken(Orig, ComboBox->Text, Message, List));
+    }
+    catch(...)
+    {
+      ComboBox->SetFocus();
+      throw;
+    }
   }
 }
 //---------------------------------------------------------------------------
 void __fastcall TPropertiesDialog::GroupComboBoxExit(TObject * Sender)
 {
-  ResolveRemoteToken(FOrigProperties.Group, PROPERTIES_INVALID_GROUP,
+  ValidateRemoteToken(FOrigProperties.Group, PROPERTIES_INVALID_GROUP,
     dynamic_cast<TComboBox *>(Sender), FGroupList);
 }
 //---------------------------------------------------------------------------
 void __fastcall TPropertiesDialog::OwnerComboBoxExit(TObject * Sender)
 {
-  ResolveRemoteToken(FOrigProperties.Owner, PROPERTIES_INVALID_OWNER,
+  ValidateRemoteToken(FOrigProperties.Owner, PROPERTIES_INVALID_OWNER,
     dynamic_cast<TComboBox *>(Sender), FUserList);
 }
 //---------------------------------------------------------------------------

source/forms/Properties.h

@@ -111,7 +111,7 @@ protected:
     TRemoteProperties & Properties);
   void __fastcall StoreRemoteToken(unsigned int ID, const UnicodeString & Text,
     const TRemoteTokenList * List, TRemoteToken & Result);
-  void __fastcall ResolveRemoteToken(
+  void __fastcall ValidateRemoteToken(
     const TRemoteToken & Orig, int Message, TComboBox * ComboBox,
     const TRemoteTokenList * List);
   void __fastcall UpdateControls();

source/forms/Rights.cpp

@@ -675,11 +675,10 @@ void __fastcall TRightsFrame::OctalEditChange(TObject *)
 //---------------------------------------------------------------------------
 void __fastcall TRightsFrame::OctalEditExit(TObject *)
 {
-  if (!Visible)
+  if ((!Visible && DebugAlwaysTrue(Popup)) || // Popup assert: should happen only if popup is closed by esc key
+      IsCancelButtonBeingClicked(this) || // CloseButton
+      ((FCancelButton != NULL) && IsButtonBeingClicked(FCancelButton)))
   {
-    // should happen only if popup is closed by esc key
-    DebugAssert(Popup);
-
     // cancel changes
     ForceUpdate();
   }

source/forms/SiteAdvanced.cpp

@@ -417,6 +417,10 @@ TSshProt __fastcall TSiteAdvancedDialog::GetSshProt()
 //---------------------------------------------------------------------
 void __fastcall TSiteAdvancedDialog::SaveSession()
 {
+  // Last resort validation,
+  // in case the test in EncryptKeyEditExit was previously bypassed due to IsCancelButtonBeingClicked
+  EncryptKeyEditExit(GetEncryptKeyEdit());
+
   // SSH page
   FSessionData->Compression = CompressionCheck->Checked;
   FSessionData->Ssh2DES = Ssh2LegacyDESCheck->Checked;
@@ -1622,10 +1626,13 @@ void __fastcall TSiteAdvancedDialog::GenerateKeyButtonClick(TObject * /*Sender*/
 //---------------------------------------------------------------------------
 void __fastcall TSiteAdvancedDialog::EncryptKeyEditExit(TObject * /*Sender*/)
 {
-  UnicodeString HexKey = GetEncryptKeyEdit()->Text;
-  if (!HexKey.IsEmpty())
+  if (!IsCancelButtonBeingClicked(this))
   {
-    ValidateEncryptKey(HexToBytes(HexKey));
+    UnicodeString HexKey = GetEncryptKeyEdit()->Text;
+    if (!HexKey.IsEmpty())
+    {
+      ValidateEncryptKey(HexToBytes(HexKey));
+    }
   }
 }
 //---------------------------------------------------------------------------

source/windows/Tools.cpp

@@ -641,24 +641,32 @@ IShellLink * __fastcall CreateDesktopSessionShortCut(
       InfoTip, SpecialFolder, IconIndex, Return);
 }
 //---------------------------------------------------------------------------
+void ValidateMask(const UnicodeString & Mask, int ForceDirectoryMasks)
+{
+  TFileMasks Masks(ForceDirectoryMasks);
+  Masks = Mask;
+}
+//---------------------------------------------------------------------------
 template<class TEditControl>
 void __fastcall ValidateMaskEditT(const UnicodeString & Mask, TEditControl * Edit, int ForceDirectoryMasks)
 {
   DebugAssert(Edit != NULL);
-  TFileMasks Masks(ForceDirectoryMasks);
-  try
-  {
-    Masks = Mask;
-  }
-  catch(EFileMasksException & E)
+  if (!IsCancelButtonBeingClicked(Edit))
   {
-    ShowExtendedException(&E);
-    Edit->SetFocus();
-    // This does not work for TEdit and TMemo (descendants of TCustomEdit) anymore,
-    // as it re-selects whole text on exception in TCustomEdit.CMExit
-    Edit->SelStart = E.ErrorStart - 1;
-    Edit->SelLength = E.ErrorLen;
-    Abort();
+    try
+    {
+      ValidateMask(Mask, ForceDirectoryMasks);
+    }
+    catch(EFileMasksException & E)
+    {
+      ShowExtendedException(&E);
+      Edit->SetFocus();
+      // This does not work for TEdit and TMemo (descendants of TCustomEdit) anymore,
+      // as it re-selects whole text on exception in TCustomEdit.CMExit
+      Edit->SelStart = E.ErrorStart - 1;
+      Edit->SelLength = E.ErrorLen;
+      Abort();
+    }
   }
 }
 //---------------------------------------------------------------------------
@@ -674,8 +682,11 @@ void __fastcall ValidateMaskEdit(TEdit * Edit)
 //---------------------------------------------------------------------------
 void __fastcall ValidateMaskEdit(TMemo * Edit, bool Directory)
 {
-  UnicodeString Mask = TFileMasks::ComposeMaskStr(GetUnwrappedMemoLines(Edit), Directory);
-  ValidateMaskEditT(Mask, Edit, Directory ? 1 : 0);
+  if (!IsCancelButtonBeingClicked(Edit))
+  {
+    UnicodeString Mask = TFileMasks::ComposeMaskStr(GetUnwrappedMemoLines(Edit), Directory);
+    ValidateMaskEditT(Mask, Edit, Directory ? 1 : 0);
+  }
 }
 //---------------------------------------------------------------------------
 TStrings * __fastcall GetUnwrappedMemoLines(TMemo * Memo)

source/windows/Tools.h

@@ -38,6 +38,7 @@ TColor __fastcall GetWindowTextColor(TColor BackgroundColor, TColor Color = stat
 TColor __fastcall GetWindowColor(TColor Color = static_cast<TColor>(0));
 TColor __fastcall GetBtnFaceColor();
 TColor __fastcall GetNonZeroColor(TColor Color);
+void ValidateMask(const UnicodeString & Mask, int ForceDirectoryMasks = -1);
 void __fastcall ValidateMaskEdit(TComboBox * Edit);
 void __fastcall ValidateMaskEdit(TEdit * Edit);
 void __fastcall ValidateMaskEdit(TMemo * Edit, bool Directory);

source/windows/VCLCommon.cpp

@@ -2434,7 +2434,7 @@ bool __fastcall SupportsSplitButton()
   return (Win32MajorVersion >= 6);
 }
 //---------------------------------------------------------------------------
-static TButton * __fastcall FindDefaultButton(TWinControl * Control)
+static TButton * __fastcall FindStandardButton(TWinControl * Control, bool Default)
 {
   TButton * Result = NULL;
   int Index = 0;
@@ -2442,7 +2442,7 @@ static TButton * __fastcall FindDefaultButton(TWinControl * Control)
   {
     TControl * ChildControl = Control->Controls[Index];
     TButton * Button = dynamic_cast<TButton *>(ChildControl);
-    if ((Button != NULL) && Button->Default)
+    if ((Button != NULL) && (Default ? Button->Default : Button->Cancel))
     {
       Result = Button;
     }
@@ -2451,7 +2451,7 @@ static TButton * __fastcall FindDefaultButton(TWinControl * Control)
       TWinControl * WinControl = dynamic_cast<TWinControl *>(ChildControl);
       if (WinControl != NULL)
       {
-        Result = FindDefaultButton(WinControl);
+        Result = FindStandardButton(WinControl, Default);
       }
     }
     Index++;
@@ -2468,7 +2468,7 @@ TModalResult __fastcall DefaultResult(TCustomForm * Form, TButton * DefaultButto
   // ModalResult being mrNone, when Windows session is being logged off.
   // We interpreted mrNone as OK, causing lots of troubles.
   TModalResult Result = mrNone;
-  TButton * Button = FindDefaultButton(Form);
+  TButton * Button = FindStandardButton(Form, true);
   if (DebugAlwaysTrue(Button != NULL))
   {
     Result = Button->ModalResult;
@@ -2799,3 +2799,27 @@ TPanel * __fastcall CreateBlankPanel(TComponent * Owner)
   Panel->BevelKind = bkNone;
   return Panel;
 }
+//---------------------------------------------------------------------------
+bool IsButtonBeingClicked(TButtonControl * Button)
+{
+  class TPublicButtonControl : public TButtonControl
+  {
+  public:
+    __property ClicksDisabled;
+  };
+  TPublicButtonControl * PublicButton = reinterpret_cast<TPublicButtonControl *>(Button);
+  // HACK ClicksDisabled is set in TButtonControl.WndProc while changing focus as response to WM_LBUTTONDOWN.
+  return PublicButton->ClicksDisabled;
+}
+//---------------------------------------------------------------------------
+// When using this in OnExit handers, it's still possible that the user does not actually click the
+// CanceButton (for example, when the button is released out of the button).
+// Then the validation is bypassed. Consequently, all dialogs that uses this must still
+// gracefully handle submission with non-validated data.
+bool IsCancelButtonBeingClicked(TControl * Control)
+{
+  TCustomForm * Form = GetParentForm(Control);
+  TButtonControl * CancelButton = FindStandardButton(Form, false);
+  // Find dialog has no Cancel button
+  return (CancelButton != NULL) && IsButtonBeingClicked(CancelButton);
+}

source/windows/VCLCommon.h

@@ -85,5 +85,7 @@ void __fastcall SetRescaleFunction(
   TComponent * Component, TRescaleEvent OnRescale, TObject * Token = NULL, bool OwnsToken = false);
 void __fastcall RecordFormImplicitRescale(TForm * Form);
 void __fastcall CountClicksForWindowPrint(TForm * Form);
+bool IsButtonBeingClicked(TButtonControl * Button);
+bool IsCancelButtonBeingClicked(TControl * Control);
 //---------------------------------------------------------------------------
 #endif  // VCLCommonH