From f29201b68fedda4ffb590efbadc4ce1592d7133f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 19:28:29 -0400 Subject: [PATCH 1/4] Never publish a release before signing; harden PlanShare release.yml: `gh release create` ran before signing, so a signing failure left a published, publicly visible release with no app assets. That is exactly what happened to v1.17.0 when the SignPath approval window lapsed, and since SignPath offers manual approval only it would recur. The release is now created after signing succeeds, and the SSMS uploads move with it because uploading requires the release to exist. Nothing before the signing step publishes anything, so a failed or unapproved signing leaves no trace. PlanShare, from the maintenance security review: - Rate limit the plan GET (120/min per IP). It was the only unlimited endpoint, so nothing bounded scraping or id enumeration. - Salt visitor_hash. It was SHA256(ip|ua|day) truncated to 16 hex, which is brute-forceable straight back to the source IP - IPv4 is 2^32, the UA is guessable, the day is known - so the adjacent "no PII stored" claim was not true. Now HMAC-SHA256 with a 32-byte secret generated once into data/visitor-salt.bin, deliberately beside the DB rather than inside it so a copy of plans.db cannot reverse its own hashes. - Optional auth on /api/stats, which exposes share counts, traffic, unique visitors and top referrers. Enabled only when STATS_TOKEN is set, so deploying this cannot break the dashboard; accepts the token by header or query and compares in fixed time. Verified against a running instance: 401 without a token, 401 with a wrong one, 200 by header and by query; share and read still work; the rate limiter allowed 119 and rejected 11 of 130 reads; the salt file is created on first start. Co-Authored-By: Claude Fable 5 --- .github/workflows/release.yml | 66 +++++++++++++++++++++-------------- server/PlanShare/Program.cs | 55 ++++++++++++++++++++++++++--- 2 files changed, 90 insertions(+), 31 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 37183af..660f138 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -37,13 +37,11 @@ jobs: echo "EXISTS=false" >> $GITHUB_OUTPUT fi - - name: Create release - if: steps.check.outputs.EXISTS == 'false' - shell: bash - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - gh release create "v${{ steps.version.outputs.VERSION }}" --title "v${{ steps.version.outputs.VERSION }}" --generate-notes --target main + # NOTE: the release is deliberately NOT created here. It is created after + # signing succeeds, further down. Creating it up front meant a signing + # failure left a published, publicly-visible release with no app assets - + # which is exactly what happened to v1.17.0 when the SignPath approval + # window lapsed. Nothing before the signing step publishes anything now. - name: Setup .NET 10.0 uses: actions/setup-dotnet@v6 @@ -106,26 +104,9 @@ jobs: Write-Host "::warning::SSMS extension did not produce the expected artifacts - release published without the VSIX (issue 343)" } - - name: Upload SSMS extension to release - if: steps.ssms.outputs.BUILT == 'true' - shell: pwsh - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - VERSION: ${{ steps.version.outputs.VERSION }} - run: gh release upload "v$env:VERSION" releases/PlanViewer.Ssms.vsix releases/InstallSsmsExtension.exe --clobber - - # Publish to the SSMS Gallery (https://ssmsgallery.azurewebsites.net, issue 343). - # The gallery extracts version/description from the vsixmanifest, so the - # upload filename is irrelevant. continue-on-error: a gallery outage must - # never fail the release. bash so `curl` is curl.exe, not the pwsh alias. - - name: Publish SSMS extension to SSMS Gallery - if: steps.ssms.outputs.BUILT == 'true' - continue-on-error: true - shell: bash - run: | - echo "Uploading PlanViewer.Ssms.vsix to the SSMS Gallery..." - curl -sS -f "https://ssmsgallery.azurewebsites.net/api/upload" \ - -F "file=@releases/PlanViewer.Ssms.vsix" + # The VSIX is built above but NOT uploaded here — uploading requires the + # release to exist, and the release is not created until signing succeeds. + # Both uploads happen after the signing step. # ── SignPath code signing (Windows). Signing is REQUIRED for a release: # if the SignPath token is missing the job fails loudly rather than @@ -172,6 +153,37 @@ jobs: Remove-Item -Recurse -Force publish/win-x64 Copy-Item -Recurse signed/win-x64 publish/win-x64 + # ── Everything above this line is reversible: nothing has been published. + # The release is created only now that signed binaries exist. ── + - name: Create release + if: steps.check.outputs.EXISTS == 'false' + shell: bash + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + gh release create "v${{ steps.version.outputs.VERSION }}" --title "v${{ steps.version.outputs.VERSION }}" --generate-notes --target main + + - name: Upload SSMS extension to release + if: steps.ssms.outputs.BUILT == 'true' + shell: pwsh + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + VERSION: ${{ steps.version.outputs.VERSION }} + run: gh release upload "v$env:VERSION" releases/PlanViewer.Ssms.vsix releases/InstallSsmsExtension.exe --clobber + + # Publish to the SSMS Gallery (https://ssmsgallery.azurewebsites.net, issue 343). + # The gallery extracts version/description from the vsixmanifest, so the + # upload filename is irrelevant. continue-on-error: a gallery outage must + # never fail the release. bash so `curl` is curl.exe, not the pwsh alias. + - name: Publish SSMS extension to SSMS Gallery + if: steps.ssms.outputs.BUILT == 'true' + continue-on-error: true + shell: bash + run: | + echo "Uploading PlanViewer.Ssms.vsix to the SSMS Gallery..." + curl -sS -f "https://ssmsgallery.azurewebsites.net/api/upload" \ + -F "file=@releases/PlanViewer.Ssms.vsix" + # ── Velopack (uses signed Windows binaries) ─────────────────────── - name: Create Velopack release (Windows) shell: pwsh diff --git a/server/PlanShare/Program.cs b/server/PlanShare/Program.cs index 446c533..4e9185d 100644 --- a/server/PlanShare/Program.cs +++ b/server/PlanShare/Program.cs @@ -44,10 +44,31 @@ created_at TEXT NOT NULL cmd.ExecuteNonQuery(); } +// --- Visitor hash salt --- +// SHA256(ip|ua|day) truncated to 16 hex is brute-forceable straight back to the +// source IP: IPv4 is only 2^32, the User-Agent is guessable, and the day is +// known. Without a secret in the mix, "no PII stored" is not true. The salt is +// generated once and kept in a file beside the DB, never in the DB itself, so +// a copy of plans.db does not carry the means to reverse its own hashes. +var saltPath = Path.Combine(dataDir, "visitor-salt.bin"); +byte[] visitorSalt; +if (File.Exists(saltPath)) +{ + visitorSalt = File.ReadAllBytes(saltPath); +} +else +{ + visitorSalt = RandomNumberGenerator.GetBytes(32); + File.WriteAllBytes(saltPath, visitorSalt); +} + // --- Rate limiters (in-memory) --- // Created before Build() so they can be DI-registered and swept by CleanupService. var rateLimiter = new RateLimiter(maxRequests: 10, windowSeconds: 60); var analyticsRateLimiter = new RateLimiter(maxRequests: 30, windowSeconds: 60); +// Plan reads are cheap but unbounded reads let anyone enumerate or scrape the +// store; 120/min per IP is far above any human's browsing rate. +var readRateLimiter = new RateLimiter(maxRequests: 120, windowSeconds: 60); // Register the cleanup background service builder.Services.AddSingleton(new PlanDbConfig(connectionString)); @@ -118,8 +139,12 @@ created_at TEXT NOT NULL "application/json"); }); -app.MapGet("/api/plans/{id}", (string id) => +app.MapGet("/api/plans/{id}", (string id, HttpContext ctx) => { + var readIp = ctx.Connection.RemoteIpAddress?.ToString() ?? "unknown"; + if (!readRateLimiter.IsAllowed(readIp)) + return Results.StatusCode(429); + using var conn = new SqliteConnection(connectionString); conn.Open(); using var cmd = conn.CreateCommand(); @@ -173,11 +198,15 @@ created_at TEXT NOT NULL : null; } - // Visitor hash: SHA256(IP + User-Agent + date) — unique per day, no PII stored + // Visitor hash: HMAC-SHA256(secret salt, IP + User-Agent + date) — unique per + // day. The salt is what makes this one-way in practice: without it the input + // space (IPv4 = 2^32, guessable UA, known date) is small enough to brute + // force straight back to the source IP, so an unsalted digest would still be + // personal data despite looking like a hash. var ua = ctx.Request.Headers.UserAgent.FirstOrDefault() ?? ""; var day = DateTime.UtcNow.ToString("yyyy-MM-dd"); var visitorHash = Convert.ToHexString( - SHA256.HashData(Encoding.UTF8.GetBytes($"{ip}|{ua}|{day}"))).ToLower()[..16]; + HMACSHA256.HashData(visitorSalt, Encoding.UTF8.GetBytes($"{ip}|{ua}|{day}"))).ToLower()[..16]; using var conn = new SqliteConnection(connectionString); conn.Open(); @@ -192,8 +221,26 @@ created_at TEXT NOT NULL return Results.Ok(); }); -app.MapGet("/api/stats", () => +app.MapGet("/api/stats", (HttpContext ctx) => { + // Opt-in auth. /api/stats exposes share counts, daily traffic, unique + // visitors and top referrers — operational data, not something that needs + // to be world-readable. Set STATS_TOKEN to require it; leaving it unset + // keeps the current public behaviour so the dashboard cannot break just by + // deploying this. Compared with a fixed-time comparison so the token cannot + // be recovered a byte at a time. + var expectedToken = Environment.GetEnvironmentVariable("STATS_TOKEN"); + if (!string.IsNullOrEmpty(expectedToken)) + { + var supplied = ctx.Request.Headers["X-Stats-Token"].FirstOrDefault() + ?? ctx.Request.Query["token"].FirstOrDefault() + ?? ""; + var a = Encoding.UTF8.GetBytes(supplied); + var b = Encoding.UTF8.GetBytes(expectedToken); + if (a.Length != b.Length || !CryptographicOperations.FixedTimeEquals(a, b)) + return Results.StatusCode(401); + } + using var conn = new SqliteConnection(connectionString); conn.Open(); var now = DateTime.UtcNow.ToString("o"); From e6fc5646c3930cdf323b4054026b78255a1bbb33 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 19:47:43 -0400 Subject: [PATCH 2/4] Fix two rate-limiter defects the PR review caught Both found by the automated review on #411. 1. readRateLimiter was never registered in the RateLimiters record or passed to CleanupService, so unlike Share and Analytics it was never swept. Every unique client IP would leave a permanent dictionary entry for the process lifetime - the exact unbounded growth the sweep exists to prevent. My bug, introduced in the previous commit. 2. Pre-existing, and my read limiter turned it from latent into a real availability risk: the app never honored forwarded headers, so behind nginx every RemoteIpAddress was 127.0.0.1. All three limiters keyed on the same value, making them a single GLOBAL cap shared by every visitor rather than per-IP - one busy client would throttle everyone, and an abusive one could not be isolated. nginx already sets X-Real-IP and X-Forwarded-For; the app just was not reading them. KnownProxies is restricted to loopback on purpose. Trusting X-Forwarded-For from arbitrary sources would let any caller spoof its address and bypass the limits entirely, which is worse than the bug. Verified per-IP behaviour against a running instance: client A exhausted its own quota at exactly 120 (then 429), while a second client's first request still returned 200. Before the fix B would have been rejected. Also switched KnownNetworks to KnownIPNetworks - the former is obsolete in .NET 10 and emitted ASPDEPR005, and the build standard is 0 warnings. Co-Authored-By: Claude Fable 5 --- server/PlanShare/Program.cs | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/server/PlanShare/Program.cs b/server/PlanShare/Program.cs index 4e9185d..93148c7 100644 --- a/server/PlanShare/Program.cs +++ b/server/PlanShare/Program.cs @@ -2,6 +2,7 @@ using System.Security.Cryptography; using System.Text; using System.Text.Json; +using Microsoft.AspNetCore.HttpOverrides; using Microsoft.Data.Sqlite; var builder = WebApplication.CreateBuilder(args); @@ -72,13 +73,34 @@ created_at TEXT NOT NULL // Register the cleanup background service builder.Services.AddSingleton(new PlanDbConfig(connectionString)); -builder.Services.AddSingleton(new RateLimiters(rateLimiter, analyticsRateLimiter)); +builder.Services.AddSingleton(new RateLimiters(rateLimiter, analyticsRateLimiter, readRateLimiter)); builder.Services.AddHostedService(); // Request size limit (10 MB) builder.WebHost.ConfigureKestrel(o => o.Limits.MaxRequestBodySize = 10 * 1024 * 1024); var app = builder.Build(); + +// This runs behind nginx on loopback, which sets X-Real-IP and X-Forwarded-For. +// Without this middleware every request's RemoteIpAddress is 127.0.0.1, so all +// three rate limiters key on the same value and become a single GLOBAL cap +// shared by every visitor rather than per-IP limits — one busy client would +// throttle everyone, and one abusive client could not be isolated. +// +// KnownProxies is restricted to loopback deliberately: trusting X-Forwarded-For +// from arbitrary sources would let any caller spoof its address and bypass the +// limits entirely. Only the local nginx is believed. +var forwardedOptions = new ForwardedHeadersOptions +{ + ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto, + ForwardLimit = 1 +}; +forwardedOptions.KnownProxies.Clear(); +forwardedOptions.KnownIPNetworks.Clear(); +forwardedOptions.KnownProxies.Add(System.Net.IPAddress.Loopback); +forwardedOptions.KnownProxies.Add(System.Net.IPAddress.IPv6Loopback); +app.UseForwardedHeaders(forwardedOptions); + app.UseCors(); const int MaxTtlDays = 365; @@ -409,7 +431,7 @@ static string GenerateDeleteToken() record PlanDbConfig(string ConnectionString); -record RateLimiters(RateLimiter Share, RateLimiter Analytics); +record RateLimiters(RateLimiter Share, RateLimiter Analytics, RateLimiter Read); sealed class CleanupService : BackgroundService { @@ -463,13 +485,16 @@ private void Cleanup() _logger.LogInformation("Cleaned up {Count} old page views", deleted); } - // Evict stale rate-limiter keys so the dictionary doesn't grow forever. + // Evict stale rate-limiter keys so the dictionaries don't grow forever. + // Every limiter must be swept here: an unswept one keeps a permanent + // entry per unique client IP for the lifetime of the process. var shareEvicted = _rateLimiters.Share.Sweep(); var analyticsEvicted = _rateLimiters.Analytics.Sweep(); - if (shareEvicted + analyticsEvicted > 0) + var readEvicted = _rateLimiters.Read.Sweep(); + if (shareEvicted + analyticsEvicted + readEvicted > 0) _logger.LogInformation( - "Evicted {Share} share + {Analytics} analytics rate-limit keys", - shareEvicted, analyticsEvicted); + "Evicted {Share} share + {Analytics} analytics + {Read} read rate-limit keys", + shareEvicted, analyticsEvicted, readEvicted); } catch (Exception ex) { From 653af543d8a2eef0415e5eefa871710c9beeed24 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 20:04:30 -0400 Subject: [PATCH 3/4] Let the dashboard authenticate when STATS_TOKEN is set Second finding from the PR review. The bundled dashboard fetched /api/stats with no auth and swallowed any non-200 in a catch, rendering "No data yet". So the moment anyone enabled the STATS_TOKEN feature this PR adds, the dashboard would silently stop showing plan stats with no indication why - a fine way to lose an afternoon debugging the wrong thing. The fetch now sends X-Stats-Token. The token is bootstrapped once from ?token=... , stored in localStorage, and stripped from the URL via replaceState so it does not linger in the address bar or browser history on every later visit. A 401 now renders a distinct message naming the cause and the fix, rather than being indistinguishable from an empty database. Other non-2xx responses report their status code. Both go through textContent rather than innerHTML, since this dashboard already had an XSS fix (#249) and building HTML from a response-derived string invites the same mistake back. Verified against a running instance with STATS_TOKEN set: no header 401, X-Stats-Token header 200, ?token= 200. Extracted the inline script and ran node --check on it. Co-Authored-By: Claude Fable 5 --- server/PlanShare/dashboard.html | 34 ++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/server/PlanShare/dashboard.html b/server/PlanShare/dashboard.html index f0a7fd5..0829f55 100644 --- a/server/PlanShare/dashboard.html +++ b/server/PlanShare/dashboard.html @@ -219,10 +219,36 @@

Top Referrers (30 days)

}).join(""); updateCharts(); updateReleases(repo); } + // Token for /api/stats when the server has STATS_TOKEN set. Taken from + // ?token=... once and remembered, so the token is not left sitting in the + // address bar (and in browser history) on every subsequent visit. + function statsToken() { + var fromUrl = new URLSearchParams(window.location.search).get("token"); + if (fromUrl) { + try { localStorage.setItem("planshare_stats_token", fromUrl); } catch (e) { /* private mode */ } + history.replaceState(null, "", window.location.pathname); + return fromUrl; + } + try { return localStorage.getItem("planshare_stats_token") || ""; } catch (e) { return ""; } + } + var planStatsError = null; async function loadPlanStats() { + planStatsError = null; try { - const resp = await fetch("/api/stats"); - planStats = await resp.json(); + var token = statsToken(); + const resp = await fetch("/api/stats", token ? { headers: { "X-Stats-Token": token } } : undefined); + if (resp.status === 401) { + // Distinct from "no data": the server is fine, we are just not + // authorised. Swallowing this as an empty dashboard is how you + // spend an afternoon debugging the wrong thing. + planStatsError = "Stats require a token. Open this page once with ?token=YOUR_TOKEN"; + planStats = null; + } else if (!resp.ok) { + planStatsError = "Stats unavailable (HTTP " + resp.status + ")"; + planStats = null; + } else { + planStats = await resp.json(); + } } catch (e) { planStats = null; } @@ -231,7 +257,9 @@

Top Referrers (30 days)

function updatePlanView() { var row = document.getElementById("statsRow"); if (!planStats) { - row.innerHTML = "
-
No data yet
"; + var label = planStatsError || "No data yet"; + row.innerHTML = "
-
"; + row.querySelector(".stat-label").textContent = label; return; } var t = planStats.traffic; From 092b7f8d97df01a3bfa854e6fd374483e437b11a Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 20:21:13 -0400 Subject: [PATCH 4/4] Bootstrap the stats token from a URL fragment, not a query string Third round of review findings, both small. The dashboard took its token from ?token=..., which nginx writes to the access log the moment the page is requested - before any client-side replaceState could strip it. That would leave the secret in plaintext logs indefinitely, which rather defeats the point of adding auth. Fragments are never transmitted to the server, so #token=... cannot be logged at all. Still stored once and cleared from the address bar so it does not linger in browser history, and still sent as a header on the actual request. The server keeps accepting ?token= for curl convenience; it is only the browser path that changes. Also corrected the SSMS build-failure warning, which said the release "published without the VSIX". After the reorder no release exists at that point in the job, so it now says the release "will be published" without it. Verified: fragment parser returns the token for #token= and #a=1&token=, correctly does not match #tokenish=, handles percent-encoding, and returns empty for no fragment. Dashboard script passes node --check; release.yml still parses. Co-Authored-By: Claude Fable 5 --- .github/workflows/release.yml | 2 +- server/PlanShare/dashboard.html | 27 ++++++++++++++++++--------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 660f138..48d7596 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -101,7 +101,7 @@ jobs: "BUILT=true" >> $env:GITHUB_OUTPUT Write-Host "SSMS extension built: $vsix" } else { - Write-Host "::warning::SSMS extension did not produce the expected artifacts - release published without the VSIX (issue 343)" + Write-Host "::warning::SSMS extension did not produce the expected artifacts - the release will be published without the VSIX (issue 343)" } # The VSIX is built above but NOT uploaded here — uploading requires the diff --git a/server/PlanShare/dashboard.html b/server/PlanShare/dashboard.html index 0829f55..3b24ac0 100644 --- a/server/PlanShare/dashboard.html +++ b/server/PlanShare/dashboard.html @@ -219,15 +219,24 @@

Top Referrers (30 days)

}).join(""); updateCharts(); updateReleases(repo); } - // Token for /api/stats when the server has STATS_TOKEN set. Taken from - // ?token=... once and remembered, so the token is not left sitting in the - // address bar (and in browser history) on every subsequent visit. + // Token for /api/stats when the server has STATS_TOKEN set. + // + // Bootstrapped from the URL *fragment* (#token=...), not a query string. + // A fragment is never transmitted to the server, so the token cannot land + // in nginx access logs. A ?token= would be logged the moment the page is + // requested — before any client-side code could strip it — which would + // leave the secret sitting in plaintext logs forever. + // + // Stored once, then cleared from the address bar so it does not persist in + // browser history either. Requests always send it as a header. function statsToken() { - var fromUrl = new URLSearchParams(window.location.search).get("token"); - if (fromUrl) { - try { localStorage.setItem("planshare_stats_token", fromUrl); } catch (e) { /* private mode */ } - history.replaceState(null, "", window.location.pathname); - return fromUrl; + var hash = window.location.hash || ""; + var m = hash.match(/(?:^#|&)token=([^&]+)/); + if (m) { + var t = decodeURIComponent(m[1]); + try { localStorage.setItem("planshare_stats_token", t); } catch (e) { /* private mode */ } + history.replaceState(null, "", window.location.pathname + window.location.search); + return t; } try { return localStorage.getItem("planshare_stats_token") || ""; } catch (e) { return ""; } } @@ -241,7 +250,7 @@

Top Referrers (30 days)

// Distinct from "no data": the server is fine, we are just not // authorised. Swallowing this as an empty dashboard is how you // spend an afternoon debugging the wrong thing. - planStatsError = "Stats require a token. Open this page once with ?token=YOUR_TOKEN"; + planStatsError = "Stats require a token. Open this page once with #token=YOUR_TOKEN"; planStats = null; } else if (!resp.ok) { planStatsError = "Stats unavailable (HTTP " + resp.status + ")";