feat: add master password support, credential sets UI, and FrmPassword fixes by yosale2011 · Pull Request #3229 · mRemoteNG/mRemoteNG · GitHub
Skip to content

feat: add master password support, credential sets UI, and FrmPassword fixes#3229

Open
yosale2011 wants to merge 3 commits into
mRemoteNG:v1.78.2-devfrom
yosale2011:feature/master-password
Open

feat: add master password support, credential sets UI, and FrmPassword fixes#3229
yosale2011 wants to merge 3 commits into
mRemoteNG:v1.78.2-devfrom
yosale2011:feature/master-password

Conversation

@yosale2011

Copy link
Copy Markdown

Summary

  • Add master password support with encrypted verifier storage and startup unlock flow
  • Add credential sets manager UI for managing internal credential sets
  • Add master password manager form for setting/changing/removing master password
  • Fix FrmPassword CS0535 build error (missing IKeyProvider.GetKey())
  • Bring password dialog to foreground on startup
  • Improve error handling in credential deserialization

Master Password Feature

  • New MasterPasswordService class for setting, verifying, and removing master passwords
  • New StartupUnlockService to prompt for master password on application startup
  • XmlKeyValidator for validating encryption keys against XML files
  • Session-based encryption key management in Runtime.cs

Credential Sets Feature

  • New CredentialSetsManager form for viewing and managing credential sets
  • Added "Credential Sets" menu item under Tools menu
  • Internal credential set support with default repository handling
  • CredentialSetTypeConverter for property grid integration

FrmPassword UI Fixes

  • Implemented parameterless GetKey() to satisfy IKeyProvider interface (build fix)
  • Added TopMost/BringToFront/Activate to ensure dialog appears in foreground
  • Changed StartPosition from CenterParent to CenterScreen
  • Fixed OK button to properly validate and close

Test plan

  • Build succeeds without errors
  • Password dialog appears in foreground when application starts
  • Master password can be set, verified, and removed
  • Credential sets can be managed through the new UI
  • Existing connections load correctly without master password configured

…FrmPassword fixes

- Add MasterPasswordService with encrypted verifier storage
- Add StartupUnlockService for master password prompt on startup
- Add XmlKeyValidator for encryption key validation
- Add credential sets manager UI and master password manager form
- Fix FrmPassword CS0535 build error (missing IKeyProvider.GetKey())
- Bring password dialog to foreground on startup
- Improve error handling in credential deserialization

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

@qodo-code-review

qodo-code-review Bot commented Mar 21, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

Grey Divider


Action required

1. AxMSTSCLib version missing🐞 Bug ✓ Correctness
Description
Central package management is enabled, but the new PackageReference to AxMSTSCLib has no
corresponding PackageVersion, which will break non-VisualStudio (dotnet CLI) restores/builds.
Code

mRemoteNG/mRemoteNG.csproj[R129-132]

+  <!-- NuGet package for dotnet CLI builds -->
+  <ItemGroup Condition="'$(BuildingInsideVisualStudio)' != 'true'">
+    <PackageReference Include="AxMSTSCLib" />
+  </ItemGroup>
Evidence
The project uses central package management, but adds a versionless AxMSTSCLib PackageReference only
for non-VS builds; without a matching PackageVersion entry, restore/build will fail under central
management.

mRemoteNG/mRemoteNG.csproj[117-132]
Directory.Packages.props[1-5]
Directory.Packages.props[7-40]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`mRemoteNG/mRemoteNG.csproj` adds `&amp;amp;lt;PackageReference Include=&amp;amp;quot;AxMSTSCLib&amp;amp;quot; /&amp;amp;gt;` for dotnet CLI builds while the repo uses central package management (`ManagePackageVersionsCentrally=true`). There is no `PackageVersion` entry for `AxMSTSCLib`, which typically causes NuGet restore/build failures.
### Issue Context
Central package management requires every `PackageReference` to have a corresponding `PackageVersion` in `Directory.Packages.props` (or specify `Version=` inline).
### Fix Focus Areas
- mRemoteNG/mRemoteNG.csproj[129-132]
- Directory.Packages.props[1-40]
### Suggested fix
Add a central version entry, e.g.:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Credentials can be wiped🐞 Bug ⛯ Reliability
Description
Credential repository loading/decryption now swallows all exceptions and returns an empty set, and
Runtime’s key migration/sync logic can later save that empty set back to disk, silently overwriting
valid credentials after a wrong key/corruption.
Code

mRemoteNG/Config/CredentialRecordLoader.cs[R28-40]

      public IEnumerable<ICredentialRecord> Load(SecureString key)
      {
-            string serializedCredentials = _dataProvider.Load();
-            return _deserializer.Deserialize(serializedCredentials, key);
+            try
+            {
+                string serializedCredentials = _dataProvider.Load();
+                return _deserializer.Deserialize(serializedCredentials, key);
+            }
+            catch (Exception)
+            {
+                // If loading fails for any reason (file missing, corrupt, etc.),
+                // return empty collection to allow the system to initialize properly.
+                return Array.Empty<ICredentialRecord>();
+            }
Evidence
Multiple credential load/decrypt/parse components now return empty on any error, making a decryption
failure indistinguishable from a truly empty repository. Runtime then calls
SaveCredentials(EncryptionKey) during repository key migration/synchronization, which persists the
current in-memory records (possibly empty) via the saver.

mRemoteNG/Config/CredentialRecordLoader.cs[28-40]
mRemoteNG/Config/Serializers/CredentialSerializer/XmlCredentialPasswordDecryptorDecorator.cs[26-42]
mRemoteNG/Config/Serializers/CredentialSerializer/XmlCredentialPasswordDecryptorDecorator.cs[45-59]
mRemoteNG/Config/Serializers/CredentialSerializer/XmlCredentialRecordDeserializer.cs[16-43]
mRemoteNG/App/Runtime.cs[213-245]
mRemoteNG/Config/CredentialRecordSaver.cs[27-31]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Credential loading/decryption/parsing now catches broad exceptions and returns an empty credential set. This can convert a *failure* (wrong key, corrupted file, invalid format) into “no credentials”. Later, `Runtime` can call `repository.SaveCredentials(EncryptionKey)` during key migration/sync, which will persist the empty in-memory set and overwrite the on-disk repository.
### Issue Context
A load/decrypt failure should be surfaced and should prevent marking the repository as successfully loaded or prevent subsequent save/migration operations.
### Fix Focus Areas
- mRemoteNG/Config/CredentialRecordLoader.cs[28-40]
- mRemoteNG/Config/Serializers/CredentialSerializer/XmlCredentialPasswordDecryptorDecorator.cs[26-42]
- mRemoteNG/Config/Serializers/CredentialSerializer/XmlCredentialPasswordDecryptorDecorator.cs[45-59]
- mRemoteNG/Config/Serializers/CredentialSerializer/XmlCredentialRecordDeserializer.cs[16-43]
- mRemoteNG/App/Runtime.cs[213-245]
### Suggested fix
- Only treat *file missing/uninitialized* as empty; for decrypt/parse/schema errors:
- log/show an error,
- return a failure indicator (e.g., throw a specific exception or return a Result type),
- ensure the repository is not set to `IsLoaded=true` on failure,
- and/or guard `SaveCredentials`/migration to skip repositories whose last load failed.
- Consider distinguishing “uninitialized credentials file” from “invalid credentials file” instead of returning `string.Empty`/empty collection for both.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. TypeConverter clears UserViaAPI🐞 Bug ✓ Correctness
Description
CredentialSetTypeConverter.ConvertFrom returns an empty string for values that are neither GUIDs nor
known credential titles, so entering a custom UserViaAPI value will be converted to empty and saved,
breaking non-internal credential provider configurations.
Code

mRemoteNG/Credential/CredentialSetTypeConverter.cs[R64-91]

+        public override bool GetStandardValuesExclusive(ITypeDescriptorContext? context)
+        {
+            // Allow custom values for other external credential providers (like DelineaSecretServer)
+            // The dropdown will still show available credential sets for InternalCredentialSet
+            return false;
+        }
+
+        public override bool GetStandardValuesSupported(ITypeDescriptorContext? context)
+        {
+            return true;
+        }
+
+        public override object? ConvertFrom(ITypeDescriptorContext? context, CultureInfo? culture, object value)
+        {
+            if (value is string stringValue)
+            {
+                // If it's already a GUID, return it as-is
+                if (Guid.TryParse(stringValue, out _))
+                    return stringValue;
+
+                // Otherwise, look up the GUID from the title
+                Dictionary<string, string> map = GetCredentialSetMap();
+                if (map.TryGetValue(stringValue, out string? guid))
+                    return guid;
+
+                // Return empty string if not found
+                return string.Empty;
+            }
Evidence
The type converter explicitly allows non-exclusive values, but its ConvertFrom path drops any
unknown string to string.Empty. The converter is applied to UserViaAPI and
RDGatewayUserViaAPI, so editing those fields in the property grid can erase values that are not
credential-set titles/GUIDs.

mRemoteNG/Credential/CredentialSetTypeConverter.cs[64-91]
mRemoteNG/Connection/AbstractConnectionRecord.cs[233-240]
mRemoteNG/Connection/AbstractConnectionRecord.cs[640-648]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`CredentialSetTypeConverter.ConvertFrom(...)` returns `string.Empty` when the provided string is not a GUID and not found in the credential title→GUID map. In the property grid, this will replace a user-entered custom value with an empty string.
### Issue Context
The converter sets `GetStandardValuesExclusive` to `false` (custom values allowed), but `ConvertFrom` contradicts that by clearing unknown inputs.
### Fix Focus Areas
- mRemoteNG/Credential/CredentialSetTypeConverter.cs[64-91]
- mRemoteNG/Connection/AbstractConnectionRecord.cs[233-240]
- mRemoteNG/Connection/AbstractConnectionRecord.cs[640-648]
### Suggested fix
- In `ConvertFrom`, if the string is not a GUID and not a known title, **return the original stringValue** rather than `string.Empty`.
- Optionally, only perform title→GUID mapping when the instance’s provider is `InternalCredentialSet` (inspect `context?.Instance` as `ConnectionInfo`/record and check `ExternalCredentialProvider` / `RDGatewayExternalCredentialProvider`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. PuTTY orphan risk on crash 🐞 Bug ⛯ Reliability
Description
PuTTY process startup no longer registers the spawned process with ChildProcessTracker, so
crash/abrupt-exit cleanup via the Job Object is no longer applied and PuTTY can be orphaned.
Code

mRemoteNG/Connection/Protocol/PuttyBase.cs[R302-306]

              PuttyProcess.EnableRaisingEvents = true;
              PuttyProcess.Exited += ProcessExited;
-                // Start the process minimized for non-PuTTYNG so the window
-                // does not flash at its default position on screen before
-                // being reparented into the mRemoteNG panel.
-                if (!_isPuttyNg)
-                {
-                    PuttyProcess.StartInfo.WindowStyle = ProcessWindowStyle.Minimized;
-                }
-
              PuttyProcess.Start();
-                ChildProcessTracker.AddProcess(PuttyProcess);
              PuttyProcess.WaitForInputIdle(Properties.OptionsAdvancedPage.Default.MaxPuttyWaitTime * 1000);
Evidence
ChildProcessTracker is explicitly designed to ensure child processes are terminated when the parent
exits by assigning them to a Job Object. PuttyBase still spawns the PuTTY process but no longer adds
it to this tracker, and there are no other references to ChildProcessTracker usage elsewhere in the
codebase.

mRemoteNG/Connection/Protocol/PuttyBase.cs[302-306]
mRemoteNG/Tools/ChildProcessTracker.cs[8-54]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PuttyBase.Connect()` starts `PuttyProcess` but does not add it to `ChildProcessTracker`. `ChildProcessTracker` exists to put child processes into a Windows Job Object with `KILL_ON_JOB_CLOSE` so they are terminated if mRemoteNG crashes/exits.
### Issue Context
Without this registration, PuTTY processes may remain running if mRemoteNG terminates unexpectedly.
### Fix Focus Areas
- mRemoteNG/Connection/Protocol/PuttyBase.cs[302-306]
- mRemoteNG/Tools/ChildProcessTracker.cs[8-54]
### Suggested fix
Call `ChildProcessTracker.AddProcess(PuttyProcess);` immediately after `PuttyProcess.Start()` (or as soon as the handle is available), unless there is a documented replacement cleanup mechanism.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. HighDPI set after dialogs 🐞 Bug ⛯ Reliability
Description
Application.SetHighDpiMode is now called in StartApplication, but runtime-check UI (download/cancel
dialogs) can be shown earlier, so those dialogs may render without the intended PerMonitorV2 DPI
configuration.
Code

mRemoteNG/App/ProgramRoot.cs[R52-55]

          var (latestRuntimeVersion, downloadUrl) = DotNetRuntimeCheck.GetLatestAvailableDotNetVersionAsync().GetAwaiter().GetResult();
-            bool validDownloadUrl = Uri.TryCreate(downloadUrl, UriKind.Absolute, out var downloadUri) && downloadUri.Scheme == Uri.UriSchemeHttps;
          if (string.IsNullOrEmpty(installedVersion))
          {
              try
Evidence
MainAsync shows dialogs during runtime checks before calling StartApplication. High DPI mode is only
set inside StartApplication, after these dialogs can already have been displayed.

mRemoteNG/App/ProgramRoot.cs[49-77]
mRemoteNG/App/ProgramRoot.cs[150-155]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Application.SetHighDpiMode(PerMonitorV2)` is called in `StartApplication()`, but runtime checks in `MainAsync()` can show dialogs first. Those earlier dialogs may not benefit from the intended DPI mode.
### Issue Context
This impacts UX (scaling/hit-testing) for the startup/runtime-check dialogs.
### Fix Focus Areas
- mRemoteNG/App/ProgramRoot.cs[49-77]
- mRemoteNG/App/ProgramRoot.cs[150-155]
### Suggested fix
Move `Application.SetHighDpiMode(HighDpiMode.PerMonitorV2);` back to the earliest possible point in `Main()` (before any dialogs or other WinForms UI is shown), or ensure an equivalent early initialization path runs before runtime-check dialogs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment thread mRemoteNG/mRemoteNG.csproj
Comment thread mRemoteNG/Config/CredentialRecordLoader.cs Outdated
Comment thread mRemoteNG/Credential/CredentialSetTypeConverter.cs
Yosale2011 and others added 2 commits March 21, 2026 14:27
… converter

- Add AxMSTSCLib PackageVersion (1.0.1) to Directory.Packages.props for
  central package management compatibility with dotnet CLI builds
- Remove catch-all exception handlers in credential loading pipeline to
  prevent silent data loss: decryption/parse errors now propagate,
  keeping IsLoaded=false and preventing SaveCredentials from overwriting
  valid credentials with an empty set
- Fix CredentialSetTypeConverter.ConvertFrom to return original value
  instead of empty string for unknown inputs, preserving custom
  credential provider values

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…CredentialSet enum

- Create CredentialSetsManager.Designer.cs with Form inheritance and
  all UI controls referenced by CredentialSetsManager.cs
- Create CredentialSetResolver class for resolving credentials by
  GUID or title from the credential catalog
- Add InternalCredentialSet value to ExternalCredentialProvider enum

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@yosale2011

Copy link
Copy Markdown
Author

Update: Build fixes added

Added missing files to fix build errors:

  • CredentialSetsManager.Designer.cs — Form designer with all UI controls
  • CredentialSetResolver.cs — Resolves credentials by GUID or title
  • ExternalCredentialProviderSelector.cs — Added InternalCredentialSet enum value

Build now succeeds with 0 errors.

@sonarqubecloud

Copy link
Copy Markdown

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

Successfully merging this pull request may close these issues.

1 participant