Skip to content

Never publish a release before signing; harden PlanShare #411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erikdarlingdata merged 4 commits into from
Jul 26, 2026
Merged

Never publish a release before signing; harden PlanShare #411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f29201b
Never publish a release before signing; harden PlanShare
erikdarlingdata Jul 25, 2026
e6fc564
Fix two rate-limiter defects the PR review caught
erikdarlingdata Jul 25, 2026
653af54
Let the dashboard authenticate when STATS_TOKEN is set
erikdarlingdata Jul 26, 2026
092b7f8
Bootstrap the stats token from a URL fragment, not a query string
erikdarlingdata Jul 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 40 additions & 28 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -103,29 +101,12 @@ 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)"
}

- 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
Expand Down Expand Up @@ -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
Expand Down
92 changes: 82 additions & 10 deletions server/PlanShare/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -44,20 +45,62 @@ 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);
Comment on lines +55 to +63

@claude claude Bot Jul 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

visitor-salt.bin is written with the process's default umask (typically 644 on Linux), so any other local account on the host can read it and reverse visitor_hash back to source IPs — the exact attack this salt is meant to prevent. Since PlanShare is an internet-facing service (not the single-user desktop app), worth locking this down, e.g. File.SetUnixFileMode(saltPath, UnixFileMode.UserRead | UnixFileMode.UserWrite) after writing (net7+ API, cross-platform no-op on Windows).

}

// --- 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);

@claude claude Bot Jul 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readRateLimiter is never registered in the RateLimiters record (line 412) or passed to AddSingleton(new RateLimiters(...)) (line 75), so CleanupService.Cleanup() (lines 467-472) only sweeps Share and Analytics — it never calls readRateLimiter.Sweep().

Every unique IP that ever hits GET /api/plans/{id} leaves a ConcurrentDictionary entry behind permanently (the per-key timestamp list gets emptied by IsAllowed, but the key itself is only removed by Sweep). This is exactly the unbounded-growth problem the sweep mechanism was built to prevent for the other two limiters — it just wasn't wired up for the new one.

Fix: extend RateLimiters to (RateLimiter Share, RateLimiter Analytics, RateLimiter Read), pass readRateLimiter through, and add _rateLimiters.Read.Sweep() alongside the other two in CleanupService.Cleanup().


// 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<CleanupService>();

// 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;
Expand Down Expand Up @@ -118,8 +161,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();
Expand Down Expand Up @@ -173,11 +220,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();
Expand All @@ -192,8 +243,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))

@claude claude Bot Jul 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dashboard.html's loadPlanStats() does fetch("/api/stats") with no X-Stats-Token header or ?token= param, and swallows any non-200 response as "No data yet." Today that's harmless since STATS_TOKEN is unset in prod, but the moment it's actually turned on — the scenario this code exists for — the bundled dashboard silently stops showing plan stats with no visible auth error. Worth wiring a token into the dashboard fetch, or documenting that enabling STATS_TOKEN requires a matching dashboard change.

{
var supplied = ctx.Request.Headers["X-Stats-Token"].FirstOrDefault()
?? ctx.Request.Query["token"].FirstOrDefault()

@claude claude Bot Jul 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since /api/stats is reachable over the public internet (behind nginx), accepting STATS_TOKEN via ?token= means the secret gets written to nginx/access logs and briefly sits in browser history before dashboard.html's history.replaceState scrubs the client-side URL — server-side logging already happened by then. Given this is opt-in and low-value operational data, it's probably fine as-is, but worth a one-line note (or restricting the query-param fallback) if log retention/exposure matters to you.

@claude claude Bot Jul 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ?token= fallback undercuts the threat model this same PR states for the dashboard: the dashboard.html comment explains a query-string token would land in nginx access logs, which is why the token is bootstrapped from a URL fragment instead. Any caller that uses ?token= against this endpoint (curl, a bookmark, a monitoring script) reintroduces exactly that log-leak. Since the dashboard JS already only ever sends the header, consider dropping the query-string fallback here, or at least noting in the comment that it's a known weaker path kept for convenience.

?? "";
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");
Expand Down Expand Up @@ -362,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
{
Expand Down Expand Up @@ -416,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)
{
Expand Down
43 changes: 40 additions & 3 deletions server/PlanShare/dashboard.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,45 @@ <h3>Top Referrers (30 days)</h3>
}).join("");
updateCharts(); updateReleases(repo);
}
// 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 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 ""; }
}
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;
}
Expand All @@ -231,7 +266,9 @@ <h3>Top Referrers (30 days)</h3>
function updatePlanView() {
var row = document.getElementById("statsRow");
if (!planStats) {
row.innerHTML = "<div class='stat-card'><div class='stat-value'>-</div><div class='stat-label'>No data yet</div></div>";
var label = planStatsError || "No data yet";
row.innerHTML = "<div class='stat-card'><div class='stat-value'>-</div><div class='stat-label'></div></div>";
row.querySelector(".stat-label").textContent = label;
return;
}
var t = planStats.traffic;
Expand Down