From 962d6f4e4967fb62eca194edeb51352a10377afb Mon Sep 17 00:00:00 2001 From: DeMoN288 <161434466+DeMoN288@users.noreply.github.com> Date: Sun, 25 Oct 2026 03:50:19 +0400 Subject: [PATCH] Harden local HTTP backend and fix settings loss Local backend (127.0.0.1:6767): - Routes now match on HTTP method. A plain GET (e.g. on any web page, no CORS needed) could reach POST routes like /remove/{appid}. - Reject requests whose Host is not 127.0.0.1/localhost:6767 (DNS rebinding) or whose Origin is not a Steam surface. - CORS echoes the trusted origin instead of "*". - Non-numeric appids are a 404 instead of a 500 from long.Parse. - netsh urlacl is reserved for the current user, not Everyone. Settings: - The "nothing set, delete the file" check is now derived from every AppSettings property. DnsMode was missing from the hand-kept list, so a user whose only change was the DNS mode lost it on the next save. - Save is serialized (all saves share one .tmp path) and IO failures are logged instead of thrown out of property setters. PluginAddService: Busy is entered atomically, so two quick picks can no longer both start a download. Adds tests for all of the above and a CI workflow running build + test. --- .github/workflows/build-test.yml | 24 +++++ src/LuaToolsGui/Services/HttpServerService.cs | 97 ++++++++++++++----- src/LuaToolsGui/Services/PluginAddService.cs | 12 ++- src/LuaToolsGui/Services/SettingsService.cs | 48 ++++++--- .../HttpServerSecurityTests.cs | 54 +++++++++++ .../SettingsEmptinessTests.cs | 36 +++++++ 6 files changed, 225 insertions(+), 46 deletions(-) create mode 100644 .github/workflows/build-test.yml create mode 100644 tests/LuaToolsGui.Tests/HttpServerSecurityTests.cs create mode 100644 tests/LuaToolsGui.Tests/SettingsEmptinessTests.cs diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml new file mode 100644 index 00000000..e7d9eade --- /dev/null +++ b/.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 diff --git a/src/LuaToolsGui/Services/HttpServerService.cs b/src/LuaToolsGui/Services/HttpServerService.cs index 4009248f..a3fa7966 100644 --- a/src/LuaToolsGui/Services/HttpServerService.cs +++ b/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) + /// + /// Match a route by HTTP method AND path, and parse its numeric {appid}. The method check matters: + /// without it a plain GET (an <img src> 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. + /// + 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"]; + + /// + /// 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. + /// + 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) + /// Echo back only an origin that already passed , never "*". + 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"); } diff --git a/src/LuaToolsGui/Services/PluginAddService.cs b/src/LuaToolsGui/Services/PluginAddService.cs index e19a04a6..7b9f41eb 100644 --- a/src/LuaToolsGui/Services/PluginAddService.cs +++ b/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 _states = new(); @@ -272,8 +277,7 @@ private async Task FillStandardBadgeAsync(List rows) /// 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(); } } } diff --git a/src/LuaToolsGui/Services/SettingsService.cs b/src/LuaToolsGui/Services/SettingsService.cs index 9ffe08ea..3316c21b 100644 --- a/src/LuaToolsGui/Services/SettingsService.cs +++ b/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); + + /// + /// 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). + /// + 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 */ } diff --git a/tests/LuaToolsGui.Tests/HttpServerSecurityTests.cs b/tests/LuaToolsGui.Tests/HttpServerSecurityTests.cs new file mode 100644 index 00000000..45022338 --- /dev/null +++ b/tests/LuaToolsGui.Tests/HttpServerSecurityTests.cs @@ -0,0 +1,54 @@ +using LuaToolsGui.Services; +using Xunit; + +namespace LuaToolsGui.Tests; + +/// +/// 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. +/// +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 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 _)); +} diff --git a/tests/LuaToolsGui.Tests/SettingsEmptinessTests.cs b/tests/LuaToolsGui.Tests/SettingsEmptinessTests.cs new file mode 100644 index 00000000..e23593f3 --- /dev/null +++ b/tests/LuaToolsGui.Tests/SettingsEmptinessTests.cs @@ -0,0 +1,36 @@ +using System.Reflection; +using LuaToolsGui.Services; +using Xunit; + +namespace LuaToolsGui.Tests; + +/// +/// 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. +/// +public class SettingsEmptinessTests +{ + public static IEnumerable 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)); + } +}