Skip to content

feat: harden updater error handling and input validation #50

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.

Sign up for GitHub

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

Merged
hieuck merged 1 commit into from
Jul 8, 2026
Merged

feat: harden updater error handling and input validation #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 43 additions & 6 deletions src/KeePassAutoReload.Updater/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ namespace KeePassAutoReload.Updater
{
internal static class Program
{
internal const int ExitSuccess = 0;
internal const int ExitInvalidArguments = 1;
internal const int ExitUpdateFailed = 2;
internal const int ExitRestartFailed = 3;

internal static int Main(string[] args)
{
int processId = 0;
Expand All @@ -22,7 +27,11 @@ internal static int Main(string[] args)

if (string.Equals(current, "--process-id", StringComparison.OrdinalIgnoreCase))
{
int.TryParse(value, out processId);
if (!int.TryParse(value, out processId) || processId < 0)
{
Console.Error.WriteLine("Invalid process ID.");
return ExitInvalidArguments;
}
}
else if (string.Equals(current, "--source", StringComparison.OrdinalIgnoreCase))
{
Comment on lines 27 to 37

@codereviewbot-ai codereviewbot-ai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argument Value Validation Weakness

The argument parsing logic assumes that every flag is followed by a value, but does not check if the value is another flag (e.g., --source --destination ...). This can lead to incorrect assignments and subtle bugs if the user omits a value or provides flags in an unexpected order.

Recommendation:
Add a check to ensure that value does not start with -- before assigning it to a variable. For example:

if (value.StartsWith("--")) {
    Console.Error.WriteLine($"Missing value for argument: {current}");
    return ExitInvalidArguments;
}

Expand All @@ -41,7 +50,25 @@ internal static int Main(string[] args)
if (string.IsNullOrWhiteSpace(source) || string.IsNullOrWhiteSpace(destination))
{
Console.Error.WriteLine("Usage: KeePassAutoReload.Updater --source <path> --destination <path> [--process-id <pid>] [--restart <path>]");
return 1;
return ExitInvalidArguments;
}

if (!source.EndsWith(".new", StringComparison.OrdinalIgnoreCase))
{
Console.Error.WriteLine("Source file must have a .new extension.");
return ExitInvalidArguments;
}

if (!destination.EndsWith(".dll", StringComparison.OrdinalIgnoreCase))
{
Console.Error.WriteLine("Destination file must have a .dll extension.");
return ExitInvalidArguments;
}

if (!File.Exists(source))
{
Console.Error.WriteLine("Source file does not exist: " + source);
return ExitInvalidArguments;
}

try
Expand All @@ -63,20 +90,30 @@ internal static int Main(string[] args)

Thread.Sleep(1000);

string destinationDirectory = Path.GetDirectoryName(destination);
if (!string.IsNullOrWhiteSpace(destinationDirectory) && !Directory.Exists(destinationDirectory))
{
Directory.CreateDirectory(destinationDirectory);
}

File.Copy(source, destination, overwrite: true);
File.Delete(source);

if (!string.IsNullOrWhiteSpace(restart) && File.Exists(restart))
if (string.IsNullOrWhiteSpace(restart)) return ExitSuccess;

if (!File.Exists(restart))
{
Process.Start(restart);
Console.Error.WriteLine("KeePass executable not found: " + restart);
return ExitRestartFailed;
}

return 0;
Process.Start(restart);
return ExitSuccess;
}
catch (Exception ex)
{
Console.Error.WriteLine("Update failed: " + ex.Message);
return 2;
return ExitUpdateFailed;
}
}
}
Comment on lines 90 to 119

@codereviewbot-ai codereviewbot-ai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File Operation Robustness and Error Handling

The file operations (File.Copy, File.Delete, Process.Start) are performed without handling specific exceptions or implementing retry logic. If the destination file is locked, or there are permission issues, the update will fail immediately. The catch-all exception handler only logs the error message, which may not provide sufficient diagnostic information.

Recommendation:

  • Implement more granular exception handling for file operations to provide clearer error messages (e.g., catch IOException, UnauthorizedAccessException).
  • Consider adding retry logic for file operations in case of transient errors (e.g., file temporarily locked).
  • Log the stack trace or more detailed error information to aid in troubleshooting.

Example:

try {
    File.Copy(source, destination, overwrite: true);
} catch (IOException ioEx) {
    Console.Error.WriteLine($"File copy failed: {ioEx.Message}");
    return ExitUpdateFailed;
}

Expand Down
10 changes: 10 additions & 0 deletions src/PluginUpdater.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ public static bool TryScheduleUpdate(
if (string.IsNullOrWhiteSpace(newPluginPath)) throw new ArgumentException("newPluginPath");
if (string.IsNullOrWhiteSpace(updaterExePath)) throw new ArgumentException("updaterExePath");
if (starter == null) throw new ArgumentNullException("starter");
if (keepassProcessId < 0) throw new ArgumentOutOfRangeException("keepassProcessId");

if (!pluginPath.EndsWith(".dll", StringComparison.OrdinalIgnoreCase))
throw new ArgumentException("pluginPath must end with .dll", "pluginPath");
if (!newPluginPath.EndsWith(".new", StringComparison.OrdinalIgnoreCase))
throw new ArgumentException("newPluginPath must end with .new", "newPluginPath");
if (!updaterExePath.EndsWith(".exe", StringComparison.OrdinalIgnoreCase))
throw new ArgumentException("updaterExePath must end with .exe", "updaterExePath");
if (!string.IsNullOrWhiteSpace(keepassExecutablePath) && !keepassExecutablePath.EndsWith(".exe", StringComparison.OrdinalIgnoreCase))
throw new ArgumentException("keepassExecutablePath must end with .exe", "keepassExecutablePath");

if (!File.Exists(newPluginPath)) return false;
if (!File.Exists(updaterExePath)) return false;
Comment on lines 49 to 50

@codereviewbot-ai codereviewbot-ai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Insufficient Error Reporting for File Existence Checks

The method returns false if either newPluginPath or updaterExePath does not exist, but does not indicate which file was missing. This lack of detail can hinder debugging and error reporting. Consider returning a more informative result, such as an enum or error message, to specify which file check failed.

Example improvement:

if (!File.Exists(newPluginPath)) return Result.NewPluginMissing;
if (!File.Exists(updaterExePath)) return Result.UpdaterExeMissing;

Expand Down
40 changes: 40 additions & 0 deletions tests/Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,46 @@ public void TryScheduleUpdate_ThrowsWhenUpdaterPathIsInvalid(string updaterPath)
PluginUpdater.TryScheduleUpdate(@"C:\plugin.dll", @"C:\new.dll", updaterPath, 1234, @"C:\KeePass.exe", starter));
}

[Fact]
public void TryScheduleUpdate_ThrowsWhenPluginPathDoesNotEndWithDll()
{
FakeProcessStarter starter = new FakeProcessStarter();
Assert.Throws<ArgumentException>(() =>
PluginUpdater.TryScheduleUpdate(@"C:\plugin.txt", @"C:\new.dll.new", @"C:\updater.exe", 1234, @"C:\KeePass.exe", starter));
}

[Fact]
public void TryScheduleUpdate_ThrowsWhenNewPluginPathDoesNotEndWithNew()
{
FakeProcessStarter starter = new FakeProcessStarter();
Assert.Throws<ArgumentException>(() =>
PluginUpdater.TryScheduleUpdate(@"C:\plugin.dll", @"C:\new.dll", @"C:\updater.exe", 1234, @"C:\KeePass.exe", starter));
}

[Fact]
public void TryScheduleUpdate_ThrowsWhenUpdaterPathDoesNotEndWithExe()
{
FakeProcessStarter starter = new FakeProcessStarter();
Assert.Throws<ArgumentException>(() =>
PluginUpdater.TryScheduleUpdate(@"C:\plugin.dll", @"C:\new.dll.new", @"C:\updater.bat", 1234, @"C:\KeePass.exe", starter));
}

[Fact]
public void TryScheduleUpdate_ThrowsWhenKeePassExecutablePathHasInvalidExtension()
{
FakeProcessStarter starter = new FakeProcessStarter();
Assert.Throws<ArgumentException>(() =>
PluginUpdater.TryScheduleUpdate(@"C:\plugin.dll", @"C:\new.dll.new", @"C:\updater.exe", 1234, @"C:\KeePass.txt", starter));
}

[Fact]
public void TryScheduleUpdate_ThrowsWhenProcessIdIsNegative()
{
FakeProcessStarter starter = new FakeProcessStarter();
Assert.Throws<ArgumentOutOfRangeException>(() =>
PluginUpdater.TryScheduleUpdate(@"C:\plugin.dll", @"C:\new.dll.new", @"C:\updater.exe", -1, @"C:\KeePass.exe", starter));
}

[Fact]
public void TryScheduleUpdate_ThrowsWhenStarterIsNull()
{
Expand Down
Loading