Harden local HTTP backend and fix settings loss

.github/workflows/build-test.yml

@@ -0,0 +1,24 @@
+name: Build and test
+
+on:
+  pull_request:
+  push:
+    branches: [master, main]
+
+jobs:
+  build-test:
+    runs-on: windows-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+
+      - name: Set up .NET
+        uses: actions/setup-dotnet@v4
+        with:
+          dotnet-version: '8.0.x'
+
+      - name: Build
+        run: dotnet build LuaToolsGui.sln -c Release
+
+      - name: Test
+        run: dotnet test LuaToolsGui.sln -c Release --no-build

src/LuaToolsGui/Services/HttpServerService.cs

@@ -1,4 +1,5 @@
 using System.Collections.Concurrent;
+using System.Globalization;
 using System.IO;
 using System.IO.Compression;
 using System.Net;
@@ -103,7 +104,10 @@ public Task StartAsync(CancellationToken cancellationToken)
             _log.LogWarning("HttpListener could not start on 127.0.0.1:6767, attempting netsh reservation");
             try
             {
-                var psi = new System.Diagnostics.ProcessStartInfo("netsh", "http add urlacl url=http://127.0.0.1:6767/ user=Everyone")
+                // Reserve the URL for the current user only. "Everyone" let any other account on the
+                // machine bind this port if the app was not running.
+                string user = $"{Environment.UserDomainName}\\{Environment.UserName}";
+                var psi = new System.Diagnostics.ProcessStartInfo("netsh", $"http add urlacl url=http://127.0.0.1:6767/ user=\"{user}\"")
                 {
                     Verb = "runas",
                     UseShellExecute = true,
@@ -151,32 +155,40 @@ private async Task HandleRequest(HttpListenerContext ctx)
         var req = ctx.Request;
         var resp = ctx.Response;
 
-        SetCors(resp);
         resp.ContentType = "application/json; charset=utf-8";
 
         try
         {
+            string? origin = req.Headers["Origin"];
+            if (!IsTrustedRequest(req.Headers["Host"], origin))
+            {
+                PluginLog.Log($"HTTP rejected {req.HttpMethod} {req.Url?.AbsolutePath} host='{req.Headers["Host"]}' origin='{origin}'");
+                resp.StatusCode = 403;
+                await resp.OutputStream.WriteAsync(Encoding.UTF8.GetBytes(JsonErr("Forbidden")));
+                return;
+            }
+            SetCors(resp, origin);
+
             string? path = req.Url?.AbsolutePath.TrimEnd('/');
+            string method = req.HttpMethod;
             // Log everything except the noisy status poll.
             if (path is not null && !path.StartsWith("/add-status/") && !path.StartsWith("/has/"))
-                PluginLog.Log($"HTTP {req.HttpMethod} {path}");
+                PluginLog.Log($"HTTP {method} {path}");
             (int status, string body) = path switch
             {
-                // Answer CORS preflight FIRST. Otherwise it matches a POST route (the
-                // matchers ignore method) and returns non-2xx, so the browser blocks the
-                // real request (this is why JSON POSTs like /add-source did nothing).
-                _ when req.HttpMethod == "OPTIONS" => (204, ""),
-                var p when MatchGet(p, "/has/{appid}", out var id) => await HandleHas(long.Parse(id)),
+                // Answer CORS preflight FIRST so it never falls through to a route.
+                _ when method == "OPTIONS" => (204, ""),
+                var p when Match(method, "GET", p, "/has/{appid}", out var id) => await HandleHas(id),
                 // Steam-plugin headless add: reflects the app's real DownloadViewModel.
-                var p when MatchPost(p, "/add/{appid}", out var id) => await HandleAdd(long.Parse(id), req),
-                var p when MatchGet(p, "/add-status/{appid}", out var id) => HandleAddStatus(long.Parse(id)),
-                var p when MatchPost(p, "/add-source/{appid}", out var id) => await HandleAddSource(long.Parse(id), req),
-                var p when MatchPost(p, "/check-sources/{appid}", out var id) => await HandleCheckSources(long.Parse(id)),
-                var p when MatchPost(p, "/download/{appid}", out var id) => await HandleDownload(long.Parse(id), req),
-                var p when MatchGet(p, "/download-status/{appid}", out var id) => HandleStatus(long.Parse(id)),
-                var p when MatchPost(p, "/cancel/{appid}", out var id) => HandleCancel(long.Parse(id)),
-                var p when MatchPost(p, "/remove/{appid}", out var id) => HandleRemove(long.Parse(id)),
-                var p when MatchPost(p, "/open/fix/{appid}", out var id) => HandleOpenFix(long.Parse(id)),
+                var p when Match(method, "POST", p, "/add/{appid}", out var id) => await HandleAdd(id, req),
+                var p when Match(method, "GET", p, "/add-status/{appid}", out var id) => HandleAddStatus(id),
+                var p when Match(method, "POST", p, "/add-source/{appid}", out var id) => await HandleAddSource(id, req),
+                var p when Match(method, "POST", p, "/check-sources/{appid}", out var id) => await HandleCheckSources(id),
+                var p when Match(method, "POST", p, "/download/{appid}", out var id) => await HandleDownload(id, req),
+                var p when Match(method, "GET", p, "/download-status/{appid}", out var id) => HandleStatus(id),
+                var p when Match(method, "POST", p, "/cancel/{appid}", out var id) => HandleCancel(id),
+                var p when Match(method, "POST", p, "/remove/{appid}", out var id) => HandleRemove(id),
+                var p when Match(method, "POST", p, "/open/fix/{appid}", out var id) => HandleOpenFix(id),
                 "/open/settings" when req.HttpMethod == "POST" => HandleOpenSettings(),
                 "/open-url" when req.HttpMethod == "POST" => await HandleOpenUrl(req),
                 "/restart-steam" when req.HttpMethod == "POST" => HandleRestartSteam(),
@@ -204,23 +216,53 @@ var p when MatchPost(p, "/open/fix/{appid}", out var id) => HandleOpenFix(long.P
         }
     }
 
-    private static bool MatchGet(string? path, string pattern, out string id)
+    /// <summary>
+    /// Match a route by HTTP method AND path, and parse its numeric {appid}. The method check matters:
+    /// without it a plain GET (an &lt;img src&gt; on any web page needs no CORS) reached state-changing
+    /// POST routes such as /remove/{appid}. A non-numeric id is simply not a match, so it ends in a
+    /// 404 instead of a 500 from long.Parse.
+    /// </summary>
+    internal static bool Match(string method, string expectedMethod, string? path, string pattern, out long id)
     {
-        id = "";
-        if (path is null) return false;
+        id = 0;
+        if (path is null || !string.Equals(method, expectedMethod, StringComparison.OrdinalIgnoreCase))
+            return false;
         var parts = pattern.TrimEnd('/').Split('/');
         var pathParts = path.Split('/');
         if (parts.Length != pathParts.Length) return false;
+        bool hasId = false;
         for (int i = 0; i < parts.Length; i++)
         {
-            if (parts[i].StartsWith("{")) { id = pathParts[i]; continue; }
+            if (parts[i].StartsWith("{"))
+            {
+                if (!long.TryParse(pathParts[i], NumberStyles.None, CultureInfo.InvariantCulture, out id))
+                    return false;
+                hasId = true;
+                continue;
+            }
             if (!string.Equals(parts[i], pathParts[i], StringComparison.OrdinalIgnoreCase)) return false;
         }
-        return !string.IsNullOrEmpty(id);
+        return hasId;
     }
 
-    private static bool MatchPost(string? path, string pattern, out string id) =>
-        MatchGet(path, pattern, out id);
+    // Steam's own surfaces that legitimately host the store-page plugin. Requests from the app itself
+    // (CefInjectorService's HttpClient) carry no Origin header at all.
+    private static readonly string[] TrustedOriginHosts =
+        ["store.steampowered.com", "steamcommunity.com", "steamloopback.host"];
+
+    /// <summary>
+    /// Reject what a hostile web page in the user's browser could send. The Host check defeats DNS
+    /// rebinding (a page whose own name resolves to 127.0.0.1 still sends its own name as Host); the
+    /// Origin check stops any other site's fetch/form from driving the backend.
+    /// </summary>
+    internal static bool IsTrustedRequest(string? host, string? origin)
+    {
+        if (host is not ("127.0.0.1:6767" or "localhost:6767")) return false;
+        if (string.IsNullOrEmpty(origin)) return true;
+        if (!Uri.TryCreate(origin, UriKind.Absolute, out var uri)) return false;
+        if (uri.Scheme != Uri.UriSchemeHttps) return false;
+        return TrustedOriginHosts.Any(h => string.Equals(uri.Host, h, StringComparison.OrdinalIgnoreCase));
+    }
 
     // ── Endpoint handlers ─────────────────────────────────────────────
 
@@ -601,9 +643,12 @@ private static (long BytesRead, long TotalBytes) LegacyBytes(Services.Downloads.
 
     // ── Helpers ───────────────────────────────────────────────────────
 
-    private static void SetCors(HttpListenerResponse resp)
+    /// <summary>Echo back only an origin that already passed <see cref="IsTrustedRequest"/>, never "*".</summary>
+    private static void SetCors(HttpListenerResponse resp, string? origin)
     {
-        resp.AddHeader("Access-Control-Allow-Origin", "*");
+        if (string.IsNullOrEmpty(origin)) return;
+        resp.AddHeader("Access-Control-Allow-Origin", origin);
+        resp.AddHeader("Vary", "Origin");
         resp.AddHeader("Access-Control-Allow-Methods", "GET, POST, OPTIONS");
         resp.AddHeader("Access-Control-Allow-Headers", "Content-Type");
     }

src/LuaToolsGui/Services/PluginAddService.cs

@@ -48,7 +48,12 @@ public class AddState
         public string? InstallStatus;
         public bool InstallFailed;
         public string? Error;
-        public bool Busy; // a download+install is running
+        // A download+install is running. An int behind Interlocked rather than a plain bool: Pick runs
+        // DownloadAsync on the thread pool, so two quick picks could both pass a read-then-write check.
+        private int _busy;
+        public bool Busy => Volatile.Read(ref _busy) == 1;
+        public bool TryEnterBusy() => Interlocked.CompareExchange(ref _busy, 1, 0) == 0;
+        public void ExitBusy() => Volatile.Write(ref _busy, 0);
     }
 
     private readonly ConcurrentDictionary<long, AddState> _states = new();
@@ -272,8 +277,7 @@ private async Task FillStandardBadgeAsync(List<SourceRow> rows)
     /// </remarks>
     private async Task DownloadAsync(long appId, AddState state, SourceRow row)
     {
-        if (state.Busy) return;
-        state.Busy = true;
+        if (!state.TryEnterBusy()) return;
         state.Error = null;
         state.InstallStatus = null;
         state.InstallFailed = false;
@@ -324,7 +328,7 @@ void OnChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e)
         {
             row.Downloading = false;
             row.Indeterminate = false;
-            state.Busy = false;
+            state.ExitBusy();
         }
     }
 }

src/LuaToolsGui/Services/SettingsService.cs

@@ -1,4 +1,5 @@
 using System.IO;
+using System.Reflection;
 using System.Text.Json;
 
 namespace LuaToolsGui.Services;
@@ -214,25 +215,40 @@ private static void PreserveCorrupt(string path)
         catch { /* best effort */ }
     }
 
+    private static readonly PropertyInfo[] PersistedProperties =
+        typeof(AppSettings).GetProperties(BindingFlags.Public | BindingFlags.Instance);
+
+    /// <summary>
+    /// True when no setting has been set. Derived from every public property rather than a hand-kept
+    /// list: a field missing from such a list made a user whose ONLY change was that setting lose it on
+    /// the next save (FixesPageSize, then DnsMode).
+    /// </summary>
+    internal static bool IsEmpty(AppSettings settings) =>
+        PersistedProperties.All(p => p.GetValue(settings) is null);
+
+    // Every setter saves, and setters run on the UI thread and on background services alike. All of them
+    // share one .tmp path, so two unsynchronized saves race on the final File.Move.
+    private readonly object _saveGate = new();
+
     private void Save()
+    {
+        lock (_saveGate)
+        {
+            try { SaveCore(); }
+            // A failed write must not throw out of a property setter into whatever UI changed it. The
+            // in-memory value still applies for this session, and the .bak/.tmp scheme keeps the
+            // previous file intact.
+            catch (Exception ex) when (ex is IOException or UnauthorizedAccessException)
+            {
+                PluginLog.Log($"Settings save failed: {ex.Message}");
+            }
+        }
+    }
+
+    private void SaveCore()
     {
         // Nothing worth persisting → don't leave a settings file behind.
-        // Every persisted field must be listed here. A field left out is treated as "nothing worth
-        // keeping", so a user whose ONLY change was that setting gets the file deleted and the setting
-        // silently lost on the next save. (FixesPageSize was missing.)
-        bool empty = _settings.SteamPathOverride is null
-            && _settings.SelectedMode is null
-            && _settings.AutoUpdateApps is null
-            && _settings.DonateKeys is null
-            && _settings.ManagePageSize is null
-            && _settings.FixesPageSize is null
-            && _settings.BuildsPageSize is null
-            && _settings.Language is null
-            && _settings.HubcapApiKey is null
-            && _settings.StartWithWindows is null
-            && _settings.MinimizeToTray is null
-            && _settings.FastFetch is null;
-        if (empty)
+        if (IsEmpty(_settings))
         {
             foreach (var p in new[] { FilePath, BakPath, TmpPath })
                 try { if (File.Exists(p)) File.Delete(p); } catch { /* best effort */ }

tests/LuaToolsGui.Tests/HttpServerSecurityTests.cs

@@ -0,0 +1,54 @@
+using LuaToolsGui.Services;
+using Xunit;
+
+namespace LuaToolsGui.Tests;
+
+/// <summary>
+/// The local backend is reachable by every web page the user opens. These pin down what it must refuse:
+/// a request from another site, a DNS-rebound host name, and a GET aimed at a state-changing route.
+/// </summary>
+public class HttpServerSecurityTests
+{
+    [Theory]
+    // The app's own HttpClient (CefInjectorService) sends no Origin.
+    [InlineData("127.0.0.1:6767", null)]
+    [InlineData("localhost:6767", null)]
+    [InlineData("127.0.0.1:6767", "")]
+    [InlineData("127.0.0.1:6767", "https://store.steampowered.com")]
+    [InlineData("127.0.0.1:6767", "https://steamcommunity.com")]
+    [InlineData("127.0.0.1:6767", "https://steamloopback.host")]
+    public void TrustedCallers_AreAccepted(string host, string? origin) =>
+        Assert.True(HttpServerService.IsTrustedRequest(host, origin));
+
+    [Theory]
+    // Any other site driving the backend through fetch or a form post.
+    [InlineData("127.0.0.1:6767", "https://evil.example")]
+    [InlineData("127.0.0.1:6767", "null")]
+    // Look-alike and downgraded Steam origins.
+    [InlineData("127.0.0.1:6767", "https://store.steampowered.com.evil.example")]
+    [InlineData("127.0.0.1:6767", "http://store.steampowered.com")]
+    // DNS rebinding: the page's own name resolves to 127.0.0.1 but is still sent as Host.
+    [InlineData("rebind.evil.example:6767", null)]
+    [InlineData(null, null)]
+    [InlineData("127.0.0.1:80", null)]
+    public void UntrustedCallers_AreRejected(string? host, string? origin) =>
+        Assert.False(HttpServerService.IsTrustedRequest(host, origin));
+
+    [Fact]
+    public void Match_ParsesTheAppId() =>
+        Assert.True(HttpServerService.Match("POST", "POST", "/remove/730", "/remove/{appid}", out var id) && id == 730);
+
+    [Fact]
+    public void Match_RejectsTheWrongMethod() =>
+        // An <img src="http://127.0.0.1:6767/remove/730"> is a GET and must not reach the POST route.
+        Assert.False(HttpServerService.Match("GET", "POST", "/remove/730", "/remove/{appid}", out _));
+
+    [Theory]
+    [InlineData("/remove/abc")]
+    [InlineData("/remove/-1")]
+    [InlineData("/remove/99999999999999999999")]
+    [InlineData("/remove/")]
+    [InlineData("/remove/730/extra")]
+    public void Match_RejectsMalformedIds(string path) =>
+        Assert.False(HttpServerService.Match("POST", "POST", path, "/remove/{appid}", out _));
+}

tests/LuaToolsGui.Tests/SettingsEmptinessTests.cs

@@ -0,0 +1,36 @@
+using System.Reflection;
+using LuaToolsGui.Services;
+using Xunit;
+
+namespace LuaToolsGui.Tests;
+
+/// <summary>
+/// SettingsService deletes settings.json when nothing is set. Any property the emptiness check misses is
+/// silently lost whenever it is the only thing the user changed, which already happened twice.
+/// </summary>
+public class SettingsEmptinessTests
+{
+    public static IEnumerable<object[]> EveryProperty() =>
+        typeof(AppSettings).GetProperties(BindingFlags.Public | BindingFlags.Instance)
+            .Select(p => new object[] { p.Name });
+
+    [Fact]
+    public void DefaultSettings_AreEmpty() =>
+        Assert.True(SettingsService.IsEmpty(new AppSettings()));
+
+    [Theory]
+    [MemberData(nameof(EveryProperty))]
+    public void SettingAnySingleProperty_MakesItNonEmpty(string propertyName)
+    {
+        var settings = new AppSettings();
+        var prop = typeof(AppSettings).GetProperty(propertyName)!;
+        var type = Nullable.GetUnderlyingType(prop.PropertyType) ?? prop.PropertyType;
+        object value = type == typeof(bool) ? false
+            : type == typeof(int) ? 0
+            : type == typeof(string) ? "x"
+            : throw new NotSupportedException($"Add a sample value for {type} ({propertyName}).");
+        prop.SetValue(settings, value);
+
+        Assert.False(SettingsService.IsEmpty(settings));
+    }
+}