Security & code audit — repojacking fix, crash fix, hardening

CLAUDE.md

@@ -73,7 +73,9 @@ Version comparison is **SemVer-only** (Major.Minor.Patch via `CompareSemVer`) 
 
 If a release lacks `SHA256SUMS.txt` (older releases), the update is allowed through with a logged warning rather than refused. New releases generated by `release.yml` always include it.
 
-The optional GitHub token read at line 50 is intentionally `Environment.GetEnvironmentVariable("")` — i.e. disabled. If you wire one up, do it through a real env-var name and treat it as a secret.
+An optional GitHub token is read from the `MONEYSHOT_GITHUB_TOKEN` environment variable (higher API rate limit). Treat it as a secret — never log it.
+
+**The repo/owner constants must track the repository's current name** (`Daolyap/Money-Shot` since the rename). GitHub redirects an old repo name only until someone re-registers it — at which point they own the update channel (repojacking). Update the constants immediately on any rename.
 
 ### Editor undo model
 

MoneyShot.Tests/SaveServiceTests.cs

@@ -0,0 +1,64 @@
+using System.IO;
+using MoneyShot.Services;
+using Xunit;
+
+namespace MoneyShot.Tests;
+
+public class SaveServiceTests
+{
+    private readonly SaveService _service = new();
+
+    // Path validation runs before the image is touched, so a null image never reaches the
+    // encoder for these rejection cases — no WPF/STA setup needed.
+
+    [Fact]
+    public void SaveToFile_InsideWindowsDirectory_IsRejected()
+    {
+        var windows = Environment.GetFolderPath(Environment.SpecialFolder.Windows);
+        var path = Path.Combine(windows, "moneyshot-test.png");
+        Assert.Throws<ArgumentException>(() => _service.SaveToFile(null!, path));
+    }
+
+    [Fact]
+    public void SaveToFile_NestedUnderSystemDirectory_IsRejected()
+    {
+        var system = Environment.GetFolderPath(Environment.SpecialFolder.System);
+        var path = Path.Combine(system, "drivers", "moneyshot-test.png");
+        Assert.Throws<ArgumentException>(() => _service.SaveToFile(null!, path));
+    }
+
+    [Theory]
+    [InlineData("")]
+    [InlineData("   ")]
+    public void SaveToFile_EmptyPath_IsRejected(string path)
+    {
+        Assert.Throws<ArgumentException>(() => _service.SaveToFile(null!, path));
+    }
+
+    [Fact]
+    public void SaveToFile_BareDirectoryPath_IsRejected()
+    {
+        Assert.Throws<ArgumentException>(() => _service.SaveToFile(null!, Path.GetTempPath()));
+    }
+
+    [Fact]
+    public void SaveToFile_UserWritablePath_PassesValidation()
+    {
+        // With a valid path, validation must NOT throw ArgumentException; the failure (if any)
+        // comes from the null image and is wrapped as InvalidOperationException.
+        var path = Path.Combine(Path.GetTempPath(), "moneyshot-test.png");
+        var ex = Record.Exception(() => _service.SaveToFile(null!, path));
+        Assert.IsType<InvalidOperationException>(ex);
+    }
+
+    [Theory]
+    [InlineData("PNG", ".png")]
+    [InlineData("JPG", ".jpg")]
+    [InlineData("BMP", ".bmp")]
+    public void GenerateFileName_UsesLowercaseExtension(string format, string expectedExtension)
+    {
+        var name = _service.GenerateFileName(format);
+        Assert.StartsWith("Screenshot_", name);
+        Assert.EndsWith(expectedExtension, name);
+    }
+}

MoneyShot.Tests/SettingsServiceTests.cs

@@ -10,6 +10,19 @@ public class SettingsServiceTests
     private static readonly string MyPictures =
         Environment.GetFolderPath(Environment.SpecialFolder.MyPictures);
 
+    [Theory]
+    [InlineData(-5, 0)]
+    [InlineData(0, 0)]
+    [InlineData(50, 50)]
+    [InlineData(500, 500)]
+    [InlineData(100_000, 500)]
+    public void HistoryRetentionCount_IsClampedToUiRange(int input, int expected)
+    {
+        var settings = new AppSettings { HistoryRetentionCount = input };
+        var sanitized = SettingsService.ValidateAndSanitizeSettings(settings);
+        Assert.Equal(expected, sanitized.HistoryRetentionCount);
+    }
+
     [Fact]
     public void EmptyDefaultSavePath_FallsBackToMyPictures()
     {

MoneyShot/App.xaml.cs

@@ -1,5 +1,3 @@
-using System.Configuration;
-using System.Data;
 using System.Windows;
 using System.Threading;
 
@@ -11,14 +9,16 @@ namespace MoneyShot;
 public partial class App : Application
 {
     private static Mutex? _mutex;
-
+    private static bool _ownsMutex;
+
     protected override void OnStartup(StartupEventArgs e)
     {
         // Create a unique mutex name for the application
         const string mutexName = "MoneyShot_SingleInstance_Mutex_3E6F8A2D";
-        
+
         _mutex = new Mutex(true, mutexName, out bool createdNew);
-
+        _ownsMutex = createdNew;
+
         if (!createdNew)
         {
             // Another instance is already running
@@ -30,15 +30,20 @@ protected override void OnStartup(StartupEventArgs e)
             Shutdown();
             return;
         }
-        
+
         base.OnStartup(e);
     }
-    
+
     protected override void OnExit(ExitEventArgs e)
     {
-        _mutex?.ReleaseMutex();
+        // Only the instance that actually acquired the mutex may release it — calling
+        // ReleaseMutex without ownership throws and would crash the "already running"
+        // second instance on its way out.
+        if (_ownsMutex)
+        {
+            _mutex?.ReleaseMutex();
+        }
         _mutex?.Dispose();
         base.OnExit(e);
     }
 }
-

MoneyShot/MainWindow.xaml.cs

@@ -392,36 +392,11 @@ private void OpenEditor(System.Windows.Media.Imaging.BitmapSource screenshot, st
         finally
         {
             // The editor's BitmapSource backings, RenderTargetBitmaps and pixelate brushes live in
-            // both managed and native heaps. Without forcing a collection plus a working-set trim,
-            // the process sits at several hundred MB until the next major GC — undesirable for a
-            // tray app that should hover near 80MB while idle.
-            ReleaseEditorMemory();
+            // both managed and native heaps — see MemoryTrimmer for why this is forced here.
+            MemoryTrimmer.TrimAfterEditorClose();
         }
     }
 
-    private static void ReleaseEditorMemory()
-    {
-        try
-        {
-            System.Runtime.GCSettings.LargeObjectHeapCompactionMode =
-                System.Runtime.GCLargeObjectHeapCompactionMode.CompactOnce;
-            GC.Collect(GC.MaxGeneration, GCCollectionMode.Aggressive, blocking: true, compacting: true);
-            GC.WaitForPendingFinalizers();
-            GC.Collect();
-
-            // Ask Windows to trim the working set. -1, -1 is the documented "trim now" sentinel.
-            using var process = System.Diagnostics.Process.GetCurrentProcess();
-            SetProcessWorkingSetSize(process.Handle, new IntPtr(-1), new IntPtr(-1));
-        }
-        catch (Exception ex)
-        {
-            MoneyShot.Services.Logger.Warn("Could not release editor memory", ex);
-        }
-    }
-
-    [System.Runtime.InteropServices.DllImport("kernel32.dll", SetLastError = true)]
-    private static extern bool SetProcessWorkingSetSize(IntPtr proc, IntPtr min, IntPtr max);
-
     private void ShowSettings()
     {
         var settings = new SettingsWindow();

MoneyShot/Services/AutoUpdateService.cs

@@ -16,8 +16,12 @@ namespace MoneyShot.Services;
 
 public sealed class AutoUpdateService
 {
+    // SECURITY: must track the repository's CURRENT name. The repo was renamed from
+    // "MoneyShot" to "Money-Shot"; GitHub redirects the old name only until someone
+    // re-registers it, at which point they would control this app's update channel
+    // (repojacking). If the repo is ever renamed again, update this immediately.
     private const string Owner = "Daolyap";
-    private const string Repository = "MoneyShot";
+    private const string Repository = "Money-Shot";
     private const string LatestReleaseUrl = $"https://api.github.com/repos/{Owner}/{Repository}/releases/latest";
     private const string AllReleasesUrl = $"https://api.github.com/repos/{Owner}/{Repository}/releases?per_page=1";
     private static readonly Regex VersionNumberRegex = new(@"\d+", RegexOptions.Compiled);
@@ -48,7 +52,10 @@ public AutoUpdateService(HttpClient? httpClient = null)
             _httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/vnd.github+json"));
         }
 
-        var token = Environment.GetEnvironmentVariable(""); // I can put a GitHub token in here if I want authenticated API requests
+        // Optional: authenticated API requests (higher rate limit). The 401 guidance message
+        // below already documents this variable name. Treat the value as a secret — it is only
+        // ever sent to api.github.com over HTTPS and never logged.
+        var token = Environment.GetEnvironmentVariable("MONEYSHOT_GITHUB_TOKEN");
         if (!string.IsNullOrWhiteSpace(token) && _httpClient.DefaultRequestHeaders.Authorization == null)
         {
             _httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", token.Trim());

MoneyShot/Services/HotKeyService.cs

@@ -33,6 +33,10 @@ public int RegisterHotKey(uint modifiers, uint key, Action action)
             _hotKeyActions[_currentId] = action;
             return _currentId;
         }
+
+        // Most often the combination is already claimed by another application (or by a
+        // still-running instance). Without this log the hotkey just silently does nothing.
+        Logger.Warn($"RegisterHotKey failed for modifiers=0x{modifiers:X} key=0x{key:X} — combination may be in use by another application.");
         return -1;
     }
 
@@ -59,7 +63,16 @@ private IntPtr HwndHook(IntPtr hwnd, int msg, IntPtr wParam, IntPtr lParam, ref
             var id = wParam.ToInt32();
             if (_hotKeyActions.TryGetValue(id, out var action))
             {
-                action?.Invoke();
+                try
+                {
+                    action?.Invoke();
+                }
+                catch (Exception ex)
+                {
+                    // An exception escaping a window-procedure hook can take down the process;
+                    // a failed capture should be logged, not fatal.
+                    Logger.Error("Hotkey action threw", ex);
+                }
                 handled = true;
             }
         }

MoneyShot/Services/Logger.cs

@@ -31,9 +31,13 @@ internal static class Logger
 
     private static void Write(string level, string message, Exception? exception)
     {
+        // Errors get the full exception (incl. stack trace) — "ERR ... :: IOException: access
+        // denied" alone is rarely enough to diagnose a user report. Warnings stay single-line.
         var line = exception == null
             ? $"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} [{level}] {message}"
-            : $"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} [{level}] {message} :: {exception.GetType().Name}: {exception.Message}";
+            : level == "ERR"
+                ? $"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} [{level}] {message} :: {exception}"
+                : $"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} [{level}] {message} :: {exception.GetType().Name}: {exception.Message}";
 
         System.Diagnostics.Debug.WriteLine(line);
 

MoneyShot/Services/MemoryTrimmer.cs

@@ -0,0 +1,35 @@
+using System.Runtime;
+using System.Runtime.InteropServices;
+
+namespace MoneyShot.Services;
+
+/// <summary>
+/// Releases the large native/managed bitmap backings the editor leaves behind and asks Windows
+/// to trim the working set. Without this a tray app that should idle near ~80 MB sits at several
+/// hundred MB after the editor closes, until the next major GC happens on its own schedule.
+/// Shared by every code path that closes an <c>EditorWindow</c> (capture flow and history).
+/// </summary>
+internal static class MemoryTrimmer
+{
+    public static void TrimAfterEditorClose()
+    {
+        try
+        {
+            GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnce;
+            GC.Collect(GC.MaxGeneration, GCCollectionMode.Aggressive, blocking: true, compacting: true);
+            GC.WaitForPendingFinalizers();
+            GC.Collect();
+
+            // Ask Windows to trim the working set. -1, -1 is the documented "trim now" sentinel.
+            using var process = System.Diagnostics.Process.GetCurrentProcess();
+            SetProcessWorkingSetSize(process.Handle, new IntPtr(-1), new IntPtr(-1));
+        }
+        catch (Exception ex)
+        {
+            Logger.Warn("Could not release editor memory", ex);
+        }
+    }
+
+    [DllImport("kernel32.dll", SetLastError = true)]
+    private static extern bool SetProcessWorkingSetSize(IntPtr proc, IntPtr min, IntPtr max);
+}

MoneyShot/Services/SaveService.cs

@@ -27,10 +27,12 @@ public void SaveToFile(BitmapSource image, string filePath, string format = "PNG
 
         try
         {
-            BitmapEncoder? encoder = format.ToUpper() switch
+            BitmapEncoder encoder = format.ToUpper() switch
             {
                 "PNG" => new PngBitmapEncoder(),
-                "JPG" or "JPEG" => new JpegBitmapEncoder(),
+                // Default JPEG quality (75) visibly smears text in screenshots; 90 keeps
+                // UI text legible at a still-reasonable file size.
+                "JPG" or "JPEG" => new JpegBitmapEncoder { QualityLevel = 90 },
                 "BMP" => new BmpBitmapEncoder(),
                 "GIF" => new GifBitmapEncoder(),
                 _ => new PngBitmapEncoder()
@@ -120,9 +122,16 @@ private void ValidateFilePath(string filePath)
 
             foreach (var sysDir in systemDirs)
             {
-                if (!string.IsNullOrEmpty(sysDir) && 
-                    !string.IsNullOrEmpty(directory) && 
-                    directory.StartsWith(sysDir, StringComparison.OrdinalIgnoreCase))
+                if (string.IsNullOrEmpty(sysDir) || string.IsNullOrEmpty(directory))
+                {
+                    continue;
+                }
+
+                // Compare with a trailing separator so "C:\Windows" blocks "C:\Windows\..."
+                // and "C:\Windows" itself, but not sibling folders like "C:\WindowsBackup".
+                var sysDirWithSeparator = Path.TrimEndingDirectorySeparator(sysDir) + Path.DirectorySeparatorChar;
+                var directoryWithSeparator = Path.TrimEndingDirectorySeparator(directory) + Path.DirectorySeparatorChar;
+                if (directoryWithSeparator.StartsWith(sysDirWithSeparator, StringComparison.OrdinalIgnoreCase))
                 {
                     throw new ArgumentException("Cannot save to system directories.", nameof(filePath));
                 }

MoneyShot/Services/SettingsService.cs

@@ -152,7 +152,11 @@ internal static AppSettings ValidateAndSanitizeSettings(AppSettings settings)
         {
             settings.DefaultLineThickness = 3;
         }
-
+
+        // Clamp history retention to the range the UI offers (0 disables retention
+        // enforcement) so a hand-edited settings.json can't request absurd values.
+        settings.HistoryRetentionCount = Math.Clamp(settings.HistoryRetentionCount, 0, 500);
+
         return settings;
     }
 

MoneyShot/Views/EditorWindow.xaml.cs

@@ -102,7 +102,6 @@ public EditorWindow(BitmapSource image)
         _originalImage = image;
         _saveService = new SaveService();
         DisplayImage();
-        SetupToolbar();
 
         // Add keyboard event handler for Delete key
         KeyDown += EditorWindow_KeyDown;
@@ -440,11 +439,6 @@ private void DisplayImage()
         DrawingCanvas.Height = _originalImage.PixelHeight;
     }
 
-    private void SetupToolbar()
-    {
-        // Tool buttons will be set up in XAML
-    }
-
     private Point ClampToCanvasBounds(Point point)
     {
         var clampedX = Math.Max(0, Math.Min(point.X, DrawingCanvas.Width));
@@ -1093,12 +1087,8 @@ private void UpdateArrow(Point currentPoint)
 
     private bool IsPointInElement(UIElement element, Point point)
     {
-        var left = Canvas.GetLeft(element);
-        var top = Canvas.GetTop(element);
-
-        // Handle NaN values (elements without explicit positioning)
-        if (double.IsNaN(left)) left = 0;
-        if (double.IsNaN(top)) top = 0;
+        var left = CanvasPosition.GetLeft(element);
+        var top = CanvasPosition.GetTop(element);
 
         if (element is Path path)
         {
@@ -1947,12 +1937,18 @@ private void Save_Click(object sender, RoutedEventArgs e)
         try
         {
             var finalImage = CaptureCanvasAsImage();
-
+
+            // Honour the user's configured save folder and default format — previously the
+            // dialog always opened wherever Windows last left it, defaulting to PNG.
+            var settings = new SettingsService().LoadSettings();
+            var defaultFormat = settings.DefaultFileFormat.ToUpperInvariant();
             var saveDialog = new Microsoft.Win32.SaveFileDialog
             {
                 Filter = "PNG Image|*.png|JPEG Image|*.jpg|Bitmap Image|*.bmp",
-                DefaultExt = ".png",
-                FileName = _saveService.GenerateFileName("PNG")
+                FilterIndex = defaultFormat switch { "JPG" or "JPEG" => 2, "BMP" => 3, _ => 1 },
+                DefaultExt = defaultFormat switch { "JPG" or "JPEG" => ".jpg", "BMP" => ".bmp", _ => ".png" },
+                InitialDirectory = settings.DefaultSavePath,
+                FileName = _saveService.GenerateFileName(settings.DefaultFileFormat)
             };
 
             if (saveDialog.ShowDialog() == true)