Release v1.19.0 - #413
Merged
Merged
Conversation
AnalyzeCompact_PreservesHistoricalOutputBytes hashed raw stdout bytes. Console.WriteLine emits CRLF on Windows and LF elsewhere, so the golden hash was pinned to whichever OS produced it - the Linux CI runner. It passed in CI and failed on every Windows machine. Confirmed by hashing the same output twice: raw Windows stdout gives ed8183248519e72b..., and stripping the CR gives 0c609fed8e250d93..., which is exactly the expected constant. So the contract itself was correct; only the line endings differed. Normalizes CRLF to LF before hashing. The expected hash is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add read-only Repl plan exploration pilot
Two related fixes for a mistake in the original Dependabot config. target-branch was not set, so Dependabot defaulted to the repository's default branch - main. Its first two PRs (#407, #408) were therefore opened against main rather than dev, and merging them put dependency bumps directly onto main, bypassing the dev -> main release flow and leaving dev behind. Both entries now set target-branch: dev, with a comment explaining why it must stay. This branch also merges main back into dev so dev is no longer missing those bumps: SQLitePCLRaw.bundle_e_sqlite3 3.0.4 and Meziantou.Framework.Win32.CredentialManager 3.0.1. Merge was clean. Verified: clean Debug build 0 warnings, 288/288 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Point Dependabot at dev, and sync main back into dev
#388 brought Repl, Repl.Mcp and Repl.Testing onto dev at 0.11.0. They are pre-1.0, where a minor bump is allowed to break under semver - and Dependabot classifies 0.11 -> 0.12 as a *minor*, so the config's usual "majors get their own PR, minor/patch are grouped" split gives no protection at all here. A breaking bump would arrive inside a routine grouped batch. That matters more than for a typical dependency because the app ships an auto-updater, so whatever lands reaches users without them choosing it. Ignores semver-major and semver-minor for the three packages; patch bumps still flow so genuine fixes arrive. Mirrors the existing ignore rules for the other deliberate pins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Hold Repl packages to patch bumps only
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…anshare-hardening Never publish a release before signing; harden PlanShare
All four version sources: Directory.Build.props, the SSMS vsixmanifest and AssemblyInfo (not covered by Directory.Build.props, non-SDK project), and CITATION.cff. Verified: clean Release build 0 warnings, 288/288 tests, PlanShare builds clean, no stale 1.18.0 strings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bump version to 1.19.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release v1.19.0.
Security and reliability
release.ymlno longer publishes before signing.gh release createused to run as step 5, so a signing failure left a published, publicly visible release with no app assets - which is what happened to v1.17.0 when the SignPath approval window lapsed. Release creation and both SSMS uploads now sit after signing. This is the last release where a lapsed approval can leave an empty release published; from v1.20.0 a failed signing leaves no tag and nothing public.PlanShare hardening (auto-deploys to stats.erikdarling.com on merge):
visitor_hashis now HMAC-SHA256 with a 32-byte salt kept beside, not inside, the database. The previous unsalted truncated SHA256 was brute-forceable back to the source IP, so the adjacent "no PII stored" claim was untrue/api/statstakes an optionalSTATS_TOKEN, fixed-time compared. Opt-in, so deploying this changes nothing until you set itRemoteIpAddresswas127.0.0.1, so all three rate limiters shared one bucket and were a global cap, not per-IP.KnownProxiesrestricted to loopback soX-Forwarded-Forcannot be spoofedAlso included
dev(Point Dependabot at dev, and sync main back into dev #409) - it was defaulting tomain- andRepl*is held to patch bumps only (Hold Repl packages to patch bumps only #410), since 0.x minors may break and the app auto-updatesInfrastructure done outside this PR
main: force pushes and deletions blocked,build-and-testandcheck-versionrequired. No required reviews (sole maintainer cannot self-approve) andenforce_admins: falseso you can overridedeployuser withNoNewPrivileges,PrivateTmp,ProtectHome,ProtectSystem=strictVerification
mainat 1.18.0 socheck-versionpassesSigning
SignPath is manual-approval only. This run will pause at "Sign Windows build" and needs approving within 30 minutes.
🤖 Generated with Claude Code