feat: harden updater error handling and input validation#50
Conversation
- Validate plugin/new-plugin/updater paths and extensions in PluginUpdater.TryScheduleUpdate. - Validate KeePass process ID is non-negative. - Add distinct exit codes and argument validation in KeePassAutoReload.Updater Program.cs. - Add directory creation before copying and verify restart executable exists. - Add 5 unit tests for invalid PluginUpdater inputs; dotnet test passes 71/71. Closes #46.
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
|
|
||
| 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)) | ||
| { |
There was a problem hiding this comment.
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;
}|
|
||
| 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; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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;
}| if (!File.Exists(newPluginPath)) return false; | ||
| if (!File.Exists(updaterExePath)) return false; |
There was a problem hiding this comment.
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;
1 participant
Summary\r\nImprove input validation and error reporting in the external updater workflow to catch misconfiguration early and surface clear failures.\r\n\r\n## Changes\r\n- now validates file extensions for the plugin DLL, the update file, and the updater EXE.\r\n- Validates that the KeePass process ID is non-negative.\r\n- Validates the KeePass executable path extension when provided.\r\n- The updater now uses distinct exit codes (, , , ).\r\n- The updater validates arguments, checks source file existence, creates the destination directory if needed, and verifies the restart executable before launching it.\r\n\r\n## Tests\r\n- Added 5 unit tests for invalid inputs.\r\n- MSBUILD : error MSB1003: Specify a project or solution file. The current working directory does not contain a project or solution file. passes: 71/71.\r\n\r\nCloses #46.