From 66cca61b2144ff599c701c689c98b6c223403c33 Mon Sep 17 00:00:00 2001 From: peter Date: Tue, 7 Apr 2026 11:57:12 +0100 Subject: [PATCH] fix: address code review findings (critical + important + minor) Critical fixes: - C1: Fix GDI handle leak in IconGenerator (lazy singletons + DestroyIcon) - C2: Add appsettings.json to .gitignore, provide template - C3: Properly dispose IHost on exit Important fixes: - I1: Add SpamGuardOptionsValidator for startup config validation - I3: Use UID range queries instead of SearchQuery.All - I4: Only scan recent Sent messages after initial full scan - I5: Atomic file writes (write-to-temp, then rename) - I6: Set 30s HTTP timeout and base address on HttpClient - I7: Remove duplicate AddSingleton (DI conflict) - I8: Better HTML stripping (script/style removal, entity decoding) Minor fixes: - M1: Delete placeholder UnitTest1.cs - M2: Wire MaxActivityLogEntries config to ActivityLog constructor - M4: Log warnings instead of bare catch blocks - M5: Dispose JsonDocument instances - M8: Use AppContext.BaseDirectory for appsettings.json path - M9: Truncate NotifyIcon.Text to 127 chars Co-Authored-By: Claude Opus 4.6 (1M context) --- .gitignore | 3 + .../SpamGuardOptionsValidator.cs | 24 +++++++ src/SpamGuard/Program.cs | 30 ++++++-- src/SpamGuard/Services/EmailClassifier.cs | 6 +- src/SpamGuard/Services/InboxMonitorService.cs | 68 +++++++++++++++---- .../Services/TrustedSenderService.cs | 11 ++- src/SpamGuard/State/ProcessedUidStore.cs | 4 +- src/SpamGuard/State/TrustedSenderStore.cs | 4 +- src/SpamGuard/Tray/IconGenerator.cs | 25 +++++-- src/SpamGuard/Tray/TrayApplicationContext.cs | 3 +- src/SpamGuard/appsettings.template.json | 38 +++++++++++ tests/SpamGuard.Tests/UnitTest1.cs | 10 --- 12 files changed, 181 insertions(+), 45 deletions(-) create mode 100644 src/SpamGuard/Configuration/SpamGuardOptionsValidator.cs create mode 100644 src/SpamGuard/appsettings.template.json delete mode 100644 tests/SpamGuard.Tests/UnitTest1.cs diff --git a/.gitignore b/.gitignore index 104b544..6db5450 100644 --- a/.gitignore +++ b/.gitignore @@ -482,3 +482,6 @@ $RECYCLE.BIN/ # Vim temporary swap files *.swp + +# User configuration with secrets +src/SpamGuard/appsettings.json diff --git a/src/SpamGuard/Configuration/SpamGuardOptionsValidator.cs b/src/SpamGuard/Configuration/SpamGuardOptionsValidator.cs new file mode 100644 index 0000000..af03dd5 --- /dev/null +++ b/src/SpamGuard/Configuration/SpamGuardOptionsValidator.cs @@ -0,0 +1,24 @@ +namespace SpamGuard.Configuration; + +using Microsoft.Extensions.Options; + +public sealed class SpamGuardOptionsValidator : IValidateOptions +{ + public ValidateOptionsResult Validate(string? name, SpamGuardOptions options) + { + var errors = new List(); + + if (string.IsNullOrWhiteSpace(options.Imap.Host)) + errors.Add("SpamGuard:Imap:Host is required."); + if (string.IsNullOrWhiteSpace(options.Imap.Username)) + errors.Add("SpamGuard:Imap:Username is required."); + if (string.IsNullOrWhiteSpace(options.Imap.Password)) + errors.Add("SpamGuard:Imap:Password is required."); + if (string.IsNullOrWhiteSpace(options.Claude.ApiKey)) + errors.Add("SpamGuard:Claude:ApiKey is required."); + + return errors.Count > 0 + ? ValidateOptionsResult.Fail(errors) + : ValidateOptionsResult.Success; + } +} diff --git a/src/SpamGuard/Program.cs b/src/SpamGuard/Program.cs index dd9eced..179215f 100644 --- a/src/SpamGuard/Program.cs +++ b/src/SpamGuard/Program.cs @@ -1,7 +1,7 @@ -// src/SpamGuard/Program.cs using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Options; using Serilog; using SpamGuard.Configuration; using SpamGuard.Services; @@ -31,13 +31,16 @@ static class Program retainedFileCountLimit: 7) .CreateLogger(); + IHost? host = null; try { - var host = Host.CreateDefaultBuilder() + host = Host.CreateDefaultBuilder() .UseSerilog() .ConfigureAppConfiguration((context, config) => { - config.AddJsonFile("appsettings.json", optional: false); + config.AddJsonFile( + Path.Combine(AppContext.BaseDirectory, "appsettings.json"), + optional: false); config.AddEnvironmentVariables("SPAMGUARD_"); }) .ConfigureServices((context, services) => @@ -45,15 +48,28 @@ static class Program services.Configure( context.Configuration.GetSection(SpamGuardOptions.SectionName)); + // Validate required configuration on startup + services.AddSingleton, SpamGuardOptionsValidator>(); + services.AddOptionsWithValidateOnStart(); + // State stores services.AddSingleton(new ProcessedUidStore(dataDir)); services.AddSingleton(new TrustedSenderStore(dataDir)); // Services - services.AddSingleton(); + services.AddSingleton(sp => + { + var opts = sp.GetRequiredService>().Value; + return new ActivityLog(opts.Monitoring.MaxActivityLogEntries); + }); services.AddSingleton(); - services.AddSingleton(); - services.AddHttpClient(); + + // EmailClassifier with managed HttpClient (timeout + base address) + services.AddHttpClient(client => + { + client.Timeout = TimeSpan.FromSeconds(30); + client.BaseAddress = new Uri("https://api.anthropic.com/"); + }); // Background services services.AddSingleton(); @@ -72,6 +88,8 @@ static class Program } finally { + host?.StopAsync().GetAwaiter().GetResult(); + host?.Dispose(); Log.CloseAndFlush(); } } diff --git a/src/SpamGuard/Services/EmailClassifier.cs b/src/SpamGuard/Services/EmailClassifier.cs index 7e8395c..09832d5 100644 --- a/src/SpamGuard/Services/EmailClassifier.cs +++ b/src/SpamGuard/Services/EmailClassifier.cs @@ -74,7 +74,7 @@ public sealed partial class EmailClassifier }; var json = JsonSerializer.Serialize(requestBody); - var request = new HttpRequestMessage(HttpMethod.Post, "https://api.anthropic.com/v1/messages") + var request = new HttpRequestMessage(HttpMethod.Post, "v1/messages") { Content = new StringContent(json, Encoding.UTF8, "application/json") }; @@ -85,7 +85,7 @@ public sealed partial class EmailClassifier response.EnsureSuccessStatusCode(); var responseJson = await response.Content.ReadAsStringAsync(ct); - var doc = JsonDocument.Parse(responseJson); + using var doc = JsonDocument.Parse(responseJson); var text = doc.RootElement .GetProperty("content")[0] .GetProperty("text") @@ -110,7 +110,7 @@ public sealed partial class EmailClassifier try { - var doc = JsonDocument.Parse(cleaned); + using var doc = JsonDocument.Parse(cleaned); var root = doc.RootElement; return new ClassificationResult( diff --git a/src/SpamGuard/Services/InboxMonitorService.cs b/src/SpamGuard/Services/InboxMonitorService.cs index 36e734d..c7c2017 100644 --- a/src/SpamGuard/Services/InboxMonitorService.cs +++ b/src/SpamGuard/Services/InboxMonitorService.cs @@ -11,7 +11,7 @@ using SpamGuard.Configuration; using SpamGuard.Models; using SpamGuard.State; -public sealed class InboxMonitorService : BackgroundService +public sealed partial class InboxMonitorService : BackgroundService { private readonly ImapClientFactory _imapFactory; private readonly TrustedSenderStore _trustedSenders; @@ -22,6 +22,7 @@ public sealed class InboxMonitorService : BackgroundService private readonly ILogger _logger; private volatile bool _paused; + private uint _lastSeenUid; public bool IsPaused => _paused; public void Pause() => _paused = true; @@ -84,12 +85,25 @@ public sealed class InboxMonitorService : BackgroundService var inbox = client.Inbox; await inbox.OpenAsync(FolderAccess.ReadWrite, ct); - // Build search query: recent messages only - var since = _processedUids.Count == 0 - ? SearchQuery.DeliveredAfter(DateTime.UtcNow.AddDays(-_options.Monitoring.InitialScanDays)) - : SearchQuery.All; - - var uids = await inbox.SearchAsync(since, ct); + // Build search query: only fetch new messages + IList uids; + if (_lastSeenUid > 0) + { + var range = new UniqueIdRange(new UniqueId(_lastSeenUid + 1), UniqueId.MaxValue); + uids = await inbox.SearchAsync(range, SearchQuery.All, ct); + } + else if (_processedUids.Count > 0) + { + // Resuming from persisted state -- scan recent messages only + uids = await inbox.SearchAsync( + SearchQuery.DeliveredAfter(DateTime.UtcNow.AddDays(-1)), ct); + } + else + { + // First ever run -- initial scan window + uids = await inbox.SearchAsync( + SearchQuery.DeliveredAfter(DateTime.UtcNow.AddDays(-_options.Monitoring.InitialScanDays)), ct); + } _logger.LogDebug("Found {Count} messages in inbox", uids.Count); // Find the spam/junk folder @@ -98,7 +112,10 @@ public sealed class InboxMonitorService : BackgroundService foreach (var uid in uids) { if (_processedUids.Contains(uid.Id)) + { + if (uid.Id > _lastSeenUid) _lastSeenUid = uid.Id; continue; + } try { @@ -109,8 +126,10 @@ public sealed class InboxMonitorService : BackgroundService _logger.LogError(ex, "Error processing UID={Uid}", uid.Id); _activityLog.Add(new ActivityEntry( DateTime.UtcNow, "", $"UID {uid.Id}", Verdict.Error, null, ex.Message)); - _processedUids.Add(uid.Id); // Skip on next run to avoid infinite retry + _processedUids.Add(uid.Id); } + + if (uid.Id > _lastSeenUid) _lastSeenUid = uid.Id; } await client.DisconnectAsync(true, ct); @@ -182,15 +201,30 @@ public sealed class InboxMonitorService : BackgroundService private static string ExtractBodySnippet(MimeMessage message) { - var text = message.TextBody ?? message.HtmlBody ?? ""; + var text = message.TextBody; - // Strip HTML tags if we fell back to HTML body - if (message.TextBody == null && message.HtmlBody != null) - text = System.Text.RegularExpressions.Regex.Replace(text, "<[^>]+>", " "); + if (text == null && message.HtmlBody != null) + { + // Strip script and style blocks first, then remaining tags + text = StripScriptStyle().Replace(message.HtmlBody, " "); + text = StripHtmlTags().Replace(text, " "); + text = System.Net.WebUtility.HtmlDecode(text); + text = CollapseWhitespace().Replace(text, " ").Trim(); + } + text ??= ""; return text.Length > 2000 ? text[..2000] : text; } + [System.Text.RegularExpressions.GeneratedRegex(@"<(script|style)[^>]*>[\s\S]*?", System.Text.RegularExpressions.RegexOptions.IgnoreCase)] + private static partial System.Text.RegularExpressions.Regex StripScriptStyle(); + + [System.Text.RegularExpressions.GeneratedRegex(@"<[^>]+>")] + private static partial System.Text.RegularExpressions.Regex StripHtmlTags(); + + [System.Text.RegularExpressions.GeneratedRegex(@"\s{2,}")] + private static partial System.Text.RegularExpressions.Regex CollapseWhitespace(); + private async Task FindSpamFolderAsync(MailKit.Net.Imap.ImapClient client, CancellationToken ct) { // Try special folder first @@ -199,7 +233,10 @@ public sealed class InboxMonitorService : BackgroundService var junk = client.GetFolder(MailKit.SpecialFolder.Junk); if (junk != null) return junk; } - catch { } + catch (Exception ex) + { + _logger.LogDebug(ex, "Could not get Junk special folder"); + } // Fall back to configured folder name try @@ -209,7 +246,10 @@ public sealed class InboxMonitorService : BackgroundService return folders.FirstOrDefault(f => f.Name.Equals(_options.Monitoring.SpamFolderName, StringComparison.OrdinalIgnoreCase)); } - catch { } + catch (Exception ex) + { + _logger.LogDebug(ex, "Could not find spam folder by name '{FolderName}'", _options.Monitoring.SpamFolderName); + } return null; } diff --git a/src/SpamGuard/Services/TrustedSenderService.cs b/src/SpamGuard/Services/TrustedSenderService.cs index 7ca917a..e03aaf5 100644 --- a/src/SpamGuard/Services/TrustedSenderService.cs +++ b/src/SpamGuard/Services/TrustedSenderService.cs @@ -16,6 +16,7 @@ public sealed class TrustedSenderService : BackgroundService private readonly TrustedSenderStore _store; private readonly SpamGuardOptions _options; private readonly ILogger _logger; + private bool _initialScanDone; public TrustedSenderService( ImapClientFactory imapFactory, @@ -63,8 +64,13 @@ public sealed class TrustedSenderService : BackgroundService await sentFolder.OpenAsync(FolderAccess.ReadOnly, ct); - var uids = await sentFolder.SearchAsync(SearchQuery.All, ct); - _logger.LogDebug("Found {Count} messages in Sent folder", uids.Count); + // After initial full scan, only check messages from the last refresh period + var query = _initialScanDone + ? SearchQuery.DeliveredAfter(DateTime.UtcNow.AddMinutes(-_options.Monitoring.TrustedSenderRefreshMinutes)) + : SearchQuery.All; + + var uids = await sentFolder.SearchAsync(query, ct); + _logger.LogDebug("Found {Count} messages in Sent folder to scan", uids.Count); var addresses = new List(); foreach (var uid in uids) @@ -74,6 +80,7 @@ public sealed class TrustedSenderService : BackgroundService } _store.AddRange(addresses); + _initialScanDone = true; await client.DisconnectAsync(true, ct); } diff --git a/src/SpamGuard/State/ProcessedUidStore.cs b/src/SpamGuard/State/ProcessedUidStore.cs index da591b4..e582b59 100644 --- a/src/SpamGuard/State/ProcessedUidStore.cs +++ b/src/SpamGuard/State/ProcessedUidStore.cs @@ -52,7 +52,9 @@ public sealed class ProcessedUidStore lock (_lock) { var json = JsonSerializer.Serialize(_uids); - File.WriteAllText(_filePath, json); + var tempPath = _filePath + ".tmp"; + File.WriteAllText(tempPath, json); + File.Move(tempPath, _filePath, overwrite: true); } } diff --git a/src/SpamGuard/State/TrustedSenderStore.cs b/src/SpamGuard/State/TrustedSenderStore.cs index b52e005..af7dc11 100644 --- a/src/SpamGuard/State/TrustedSenderStore.cs +++ b/src/SpamGuard/State/TrustedSenderStore.cs @@ -50,7 +50,9 @@ public sealed class TrustedSenderStore lock (_lock) { var json = JsonSerializer.Serialize(_senders); - File.WriteAllText(_filePath, json); + var tempPath = _filePath + ".tmp"; + File.WriteAllText(tempPath, json); + File.Move(tempPath, _filePath, overwrite: true); } } diff --git a/src/SpamGuard/Tray/IconGenerator.cs b/src/SpamGuard/Tray/IconGenerator.cs index 08a9423..8cfa12a 100644 --- a/src/SpamGuard/Tray/IconGenerator.cs +++ b/src/SpamGuard/Tray/IconGenerator.cs @@ -2,10 +2,23 @@ namespace SpamGuard.Tray; using System.Drawing; using System.Drawing.Drawing2D; +using System.Runtime.InteropServices; public static class IconGenerator { - public static Icon CreateCircleIcon(Color color, int size = 16) + private static readonly Lazy _green = new(() => CreateCircleIcon(Color.FromArgb(76, 175, 80))); + private static readonly Lazy _yellow = new(() => CreateCircleIcon(Color.FromArgb(255, 193, 7))); + private static readonly Lazy _red = new(() => CreateCircleIcon(Color.FromArgb(244, 67, 54))); + + public static Icon Green => _green.Value; + public static Icon Yellow => _yellow.Value; + public static Icon Red => _red.Value; + + [DllImport("user32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + private static extern bool DestroyIcon(IntPtr handle); + + private static Icon CreateCircleIcon(Color color, int size = 16) { using var bitmap = new Bitmap(size, size); using var graphics = Graphics.FromImage(bitmap); @@ -15,14 +28,12 @@ public static class IconGenerator using var brush = new SolidBrush(color); graphics.FillEllipse(brush, 1, 1, size - 2, size - 2); - // Add a subtle border using var pen = new Pen(Color.FromArgb(100, 0, 0, 0), 1); graphics.DrawEllipse(pen, 1, 1, size - 3, size - 3); - return Icon.FromHandle(bitmap.GetHicon()); + var hIcon = bitmap.GetHicon(); + var icon = (Icon)Icon.FromHandle(hIcon).Clone(); + DestroyIcon(hIcon); + return icon; } - - public static Icon Green => CreateCircleIcon(Color.FromArgb(76, 175, 80)); - public static Icon Yellow => CreateCircleIcon(Color.FromArgb(255, 193, 7)); - public static Icon Red => CreateCircleIcon(Color.FromArgb(244, 67, 54)); } diff --git a/src/SpamGuard/Tray/TrayApplicationContext.cs b/src/SpamGuard/Tray/TrayApplicationContext.cs index ac552fe..9897594 100644 --- a/src/SpamGuard/Tray/TrayApplicationContext.cs +++ b/src/SpamGuard/Tray/TrayApplicationContext.cs @@ -48,7 +48,8 @@ public sealed class TrayApplicationContext : ApplicationContext { var checked_ = _activityLog.TodayChecked; var spam = _activityLog.TodaySpam; - _notifyIcon.Text = $"SpamGuard - {checked_} checked, {spam} spam caught today"; + var tooltip = $"SpamGuard - {checked_} checked, {spam} spam caught today"; + _notifyIcon.Text = tooltip.Length > 127 ? tooltip[..127] : tooltip; // Update icon based on state if (!_monitor.IsPaused) diff --git a/src/SpamGuard/appsettings.template.json b/src/SpamGuard/appsettings.template.json new file mode 100644 index 0000000..ffcd56a --- /dev/null +++ b/src/SpamGuard/appsettings.template.json @@ -0,0 +1,38 @@ +{ + "SpamGuard": { + "Imap": { + "Host": "imap.example.com", + "Port": 993, + "UseSsl": true, + "Username": "user@example.com", + "Password": "" + }, + "Claude": { + "ApiKey": "", + "Model": "claude-sonnet-4-6", + "MaxBodyLength": 2000 + }, + "Monitoring": { + "PollIntervalSeconds": 60, + "TrustedSenderRefreshMinutes": 60, + "SpamConfidenceThreshold": 0.7, + "SpamFolderName": "Junk", + "InitialScanDays": 7, + "MaxActivityLogEntries": 500 + } + }, + "Serilog": { + "MinimumLevel": "Information", + "WriteTo": [ + { "Name": "Console" }, + { + "Name": "File", + "Args": { + "path": "%APPDATA%/SpamGuard/logs/spamguard-.log", + "rollingInterval": "Day", + "retainedFileCountLimit": 7 + } + } + ] + } +} diff --git a/tests/SpamGuard.Tests/UnitTest1.cs b/tests/SpamGuard.Tests/UnitTest1.cs deleted file mode 100644 index b01984d..0000000 --- a/tests/SpamGuard.Tests/UnitTest1.cs +++ /dev/null @@ -1,10 +0,0 @@ -namespace SpamGuard.Tests; - -public class UnitTest1 -{ - [Fact] - public void Test1() - { - - } -} \ No newline at end of file