Never publish a release before signing; harden PlanShare

[erikdarlingdata]

Two things: stop release.yml publishing before signing, and the three remaining PlanShare items from the maintenance security review.

release.yml: nothing is published until signing succeeds

gh release create ran as step 5, before signing at step 15. So when signing failed, the release was already public - with no app assets on it. That is exactly what happened to v1.17.0 when the SignPath approval window lapsed: a published release carrying only the VSIX.

Since SignPath offers manual approval only, this was guaranteed to recur.

The release is now created after signing succeeds. The two SSMS uploads moved with it, because uploading requires the release to exist - that dependency is the reason the original ordering existed, and it is handled by moving both rather than by using a draft (a draft has no git tag until published, and vpk upload github resolves the release by tag).

New order:

 8. Build SSMS extension        (built early, not uploaded)
11. Sign Windows build          <- everything above is reversible
13. Create release              <- first thing that publishes
14. Upload SSMS extension
15. Publish to SSMS Gallery
16. Create Velopack release
17. Package and upload

A failed or unapproved signing now leaves no trace - no tag, no release, nothing for users to stumble into.

PlanShare hardening

Rate limit the plan GET (120/min per IP). It was the only unlimited endpoint, so nothing bounded scraping or id enumeration. 120/min is far above any human browsing rate.

Salt visitor_hash. It was SHA256(ip|ua|day) truncated to 16 hex - brute-forceable straight back to the source IP, since IPv4 is only 2^32, the User-Agent is guessable and the day is known. The adjacent "no PII stored" comment was therefore not true. Now HMAC-SHA256 with a 32-byte secret generated once into data/visitor-salt.bin - kept beside the database rather than inside it, so a copy of plans.db does not carry the means to reverse its own hashes.

Optional auth on /api/stats, which exposes share counts, daily traffic, unique visitors and top referrers. Deliberately opt-in: enabled only when STATS_TOKEN is set, so deploying this cannot break the dashboard. Accepts the token by X-Stats-Token header or ?token=, compared with CryptographicOperations.FixedTimeEquals so it cannot be recovered a byte at a time.

Verified against a running instance

Check	Result
/api/stats no token	401
/api/stats wrong token	401
/api/stats correct token (header)	200
/api/stats correct token (query)	200
share + read still work	200
GET rate limiter, 130 requests	119 x 200, 11 x 429
salt file on first start	created, 32 bytes

Plus a clean Release build with 0 warnings, and release.yml step ordering asserted programmatically.

Deployment

server/PlanShare/** auto-deploys on push to main (#399), so these ship with the next release. STATS_TOKEN is not set on the server, so /api/stats stays public until you choose to set it - deliberate, so this deploy cannot break anything.

🤖 Generated with Claude Code

[claude]

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().

[claude]

Reviewed. Both fixes are sound in intent:

• release.yml reorder: the new step order is correct — Create release now sits after Sign Windows build succeeds, and both SSMS uploads moved with it. No dangling if: conditions or output references broken by the move; steps.ssms.outputs.BUILT and steps.check.outputs.EXISTS are still set upstream of where they're consumed.
• Salt-file HMAC for visitor_hash: correctly moves the secret out of the SQLite file and fixes the actual weakness (brute-forceable unsalted digest). data/ is already gitignored so visitor-salt.bin won't leak into source control.
• /api/stats opt-in token auth: constant-time comparison via CryptographicOperations.FixedTimeEquals, defaults to today's (public) behavior when STATS_TOKEN is unset — reasonable given the low-stakes single-operator deployment.

One real bug, flagged inline: the new readRateLimiter (120 req/min for GET /api/plans/{id}) is never added to the RateLimiters record or passed into CleanupService, so unlike Share and Analytics it never gets Sweep()'d. Its ConcurrentDictionary<string, List<DateTime>> accumulates one permanent entry per unique IP ever seen for the lifetime of the process — the exact unbounded-growth problem the sweep loop exists to prevent for the other two limiters.

Minor/non-blocking:

• Given deploy-planshare.yml fronts this with nginx, ctx.Connection.RemoteIpAddress may resolve to the proxy's address rather than the real client for all three rate limiters (pre-existing pattern, not introduced here, but worth confirming nginx forwards X-Forwarded-For and the app trusts it — otherwise the new 120/min read limit is effectively a global cap shared by every visitor, not per-IP).
• The PR description mentions release.yml step ordering was "asserted programmatically," but no such test/script is part of this diff — assume that check was run locally and not persisted; not blocking.

[claude]

Review

Both changes do what the description says and are scoped correctly. release.yml reordering is correct: Create release, both SSMS uploads, and the gallery publish now all sit after the signing step, and nothing before it can leave a public trace on failure. The PlanShare hardening (salt file kept beside not inside the DB, ForwardedHeaders restricted to loopback KnownProxies so IP-based rate limiting can't be trivially defeated by a spoofed header from outside, fixed-time token comparison, opt-in STATS_TOKEN) is sound and appropriately scoped for a publicly-reachable service.

One concrete gap:

dashboard.html's own /api/stats fetch has no way to supply STATS_TOKEN. loadPlanStats() (dashboard.html:224) does fetch("/api/stats") with no auth header/query param, and a non-200 response is swallowed by the catch and rendered as "No data yet" rather than surfaced as an auth failure. The PR frames STATS_TOKEN as opt-in specifically so "deploying this cannot break the dashboard" today (it's unset in prod) — but the moment anyone actually turns on the feature this PR exists to add, the bundled dashboard silently stops showing plan stats with no indication why. Worth either wiring a token into the dashboard fetch (e.g. a ?token= prompt/config) or noting in the PR/README that enabling STATS_TOKEN requires a corresponding dashboard change.

Minor, non-blocking: the PR description says release.yml step ordering is "asserted programmatically," but no such test/script is in this diff or the repo — worth double-checking that assertion lives somewhere before relying on it as regression coverage.

[claude]

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.

[erikdarlingdata]

Both review findings addressed, plus a correction to my own PR description.

1. readRateLimiter never swept — real bug, mine, introduced in this PR. It was not in the RateLimiters record so CleanupService never called Sweep() on it, leaving one permanent dictionary entry per unique client IP for the process lifetime. Registered and swept alongside the other two.

2. Rate limiters were a global cap, not per-IP — pre-existing, but my read limiter turned it from latent into an availability risk. Confirmed on the server: nginx does set X-Real-IP and X-Forwarded-For, but the app never read them, so every RemoteIpAddress was 127.0.0.1 and all three limiters shared one bucket. One busy client would have throttled everyone.

Added UseForwardedHeaders with KnownProxies restricted to loopback — trusting X-Forwarded-For from arbitrary sources would let any caller spoof its address and bypass the limits entirely, which is worse than the original bug.

Verified per-IP behaviour:

Client	Result
A (203.0.113.10), 125 requests	120 x 200, then 5 x 429
B (198.51.100.77), first request	200 — unaffected by A's exhausted quota

Before the fix, B would have been rejected.

3. Dashboard could not authenticate — also correct, and the failure mode was the bad part: loadPlanStats() swallowed non-200s and rendered "No data yet", identical to an empty database. Enabling the very feature this PR adds would have silently blanked the dashboard.

The fetch now sends X-Stats-Token, bootstrapped once from ?token=..., stored in localStorage, and stripped from the URL with replaceState so it does not linger in the address bar or history. A 401 renders a distinct message naming the cause and the fix; other non-2xx report their status. Both via textContent, not innerHTML — this dashboard already had an XSS fix (#249) and building HTML from response-derived strings invites it back.

Verified with STATS_TOKEN set: no header 401, header 200, ?token= 200. Inline script extracted and node --checked.

Correction to my description: I wrote that the release.yml step ordering was "asserted programmatically". That overstates it — it was a one-off script I ran locally, not persisted as a test. There is no regression coverage for that ordering, and the reviewer was right to question it. Treat the ordering as verified once, by inspection, not guarded.

Also switched KnownNetworks to KnownIPNetworks; the former is obsolete in .NET 10 and emitted ASPDEPR005, and the standard here is 0 warnings.

[claude]

This warning message is now stale after the reorder: at this point in the job (before "Create release" further down), no release exists yet, so "release published without the VSIX" no longer describes reality. Worth rewording to something like "release will be published without the VSIX" so the annotation stays accurate.

[claude]

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]

Review summary

Both changes are sound and address real, concrete problems (the v1.17.0 signing-window incident, brute-forceable visitor hashing, an unbounded read endpoint, and an unauthenticated stats endpoint). Went through this with the "untrusted plan XML" and "T-SQL injection" lenses from the review brief — neither applies here, this PR doesn't touch plan parsing or T-SQL generation at all, just the release pipeline and the PlanShare server.

release.yml reorder — verified the new step order is self-consistent: steps.check.outputs.EXISTS, steps.version.outputs.VERSION, and steps.ssms.outputs.BUILT are all set early in the job and referenced correctly by the steps that moved down. The "everything above this line is reversible" claim holds for the signing gate specifically. Left one inline note: the SSMS-build-failure warning message ("release published without the VSIX") is now stale, since no release exists yet at the point that step runs.

PlanShare hardening — the ForwardedHeadersOptions setup is done carefully (KnownProxies restricted to loopback, ForwardLimit = 1), which is exactly what's needed to make the new per-IP read rate limiter actually be per-IP instead of a single global bucket once behind nginx. HMAC-salting the visitor hash with a secret kept beside (not inside) plans.db is the right fix for the brute-forceable SHA256 hash. CryptographicOperations.FixedTimeEquals for the stats token is correctly used (the redundant length pre-check is harmless — FixedTimeEquals already short-circuits safely on length mismatch). Left one inline note on the query-string ?token= fallback: it'll land in nginx access logs before dashboard.html's history.replaceState ever runs client-side, worth a one-line acknowledgment if that matters for this deployment.

No test coverage added for the new server behavior (read rate limiter, salted hash, stats auth), but there's no existing automated-test precedent for server/PlanShare to follow, and the PR's manual verification table against a running instance covers the actual new behavior reasonably well.

[claude]

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.

[claude]

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).

[claude]

Review

Two independent fixes, both well-reasoned and correctly scoped.

release.yml reorder — solid. Verified the relative ordering: Create release now sits after Sign Windows build, and the SSMS upload/gallery-publish steps moved with it since they depend on the release existing. The Check if release already exists / EXISTS gate and the Velopack delta-download step's position relative to Create release are both unchanged, so this doesn't introduce a new race with delta packaging. One gap: this ordering is enforced only by a code comment ("Everything above this line is reversible"). Nothing in CI would catch a future edit that moves Create release back above signing — which is precisely the failure mode this PR exists to prevent. The PR description mentions the ordering was "asserted programmatically," but no such check is part of this diff; worth committing a small script/test for it if one exists.

PlanShare hardening — the ForwardedHeadersOptions addition is the important fix here, arguably more than the two features it's bundled with: without it, ctx.Connection.RemoteIpAddress would be 127.0.0.1 for every request behind nginx, meaning the share/analytics rate limiters (already in production) were keying on a single global bucket rather than per-IP. Restricting KnownProxies to loopback is the right call to avoid trusting a spoofable header from arbitrary callers.

Left two inline nits on Program.cs: the /api/stats token's query-string fallback reintroduces the exact log-leak this PR's own dashboard-side comment warns about, and visitor-salt.bin is written with default file permissions with no explicit lockdown, which matters since this is an internet-facing service rather than a local single-user tool.

Nothing else stood out — the T-SQL generation, version-file, and PlanViewer.Core/Web linkage conventions don't apply to this diff (no .sqlplan/T-SQL or SDK-project changes here).