Harden PlanShare: storage limit, daily upload budget, one client key - #603
Merged
Merged
Conversation
Sharing is refused with 507 once the database uses 10 GB, counted as
(page_count - freelist_count) * page_size. /api/event skips the insert
in that state and still answers 200, so analytics never show an error.
Each client key can store 100 MB of plan data per UTC day. The budget
is in memory like the rate limiters and CleanupService sweeps it.
Every per-client limit (share, analytics, read, budget) now uses one
key: an IPv4 address, an IPv4-mapped IPv6 address as its IPv4 address,
and any other IPv6 address as its /64. The visitor hash still uses the
full address.
/api/share and /api/event answer 400 instead of 500 for a JSON root that
is not an object and for fields of the wrong type. /api/event refuses a
path over 512 characters. DELETE /api/plans/{id} takes the token in the
X-Delete-Token header and still accepts ?token=. Refusals carry an
"error" text in a JSON body.
The new classes are internal with InternalsVisibleTo, and the test
project now references server/PlanShare so CI builds and tests it. The
ci.yml path filter includes server/PlanShare.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
The web client reads the "error" text from a failed share (507, 429, 400) and shows it. A reply without that text, such as an HTML page from the proxy, keeps the generic message with the status code. DeleteAsync sends the token in X-Delete-Token instead of ?token=, which the proxy writes to its access log. The share dialog lists what the upload holds: the plan file name, the query text, operator details and warnings, compiled and runtime parameter values, missing index suggestions with database, schema and table names, and the full text report. The payload is unchanged. dashboard.html called history.replaceState, but a local array named history shadows window.history, so the call threw and left #token= in the address bar. It now calls window.history.replaceState. Tests: the endpoints run through WebApplicationFactory with the database in a temp folder and the limits passed as PlanShare:* settings, and the web share service runs against a stub HTTP handler. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
erikdarlingdata
marked this pull request as ready for review
September 28, 2026 21:20
|
Reviewed the diff. I found no blocking issues.
Two low-severity notes:
Test coverage for the changed behavior looks thorough. |
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
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.
What does this PR do?
This PR hardens the PlanShare server (
server/PlanShare) and the web app that uses it. It has no issue number.The server now stops taking shares when the database is full. It also limits how much plan data one client can store per day. All per-client limits use one client key. Bad input gets a 400 instead of a 500. The delete token moves from the URL to a header. The web app shows the server's reason when a share fails.
Storage limit
(PRAGMA page_count - PRAGMA freelist_count) x PRAGMA page_size.10 x 1024^3bytes) or more,/api/shareanswers 507 with{"error": "Plan sharing is full right now. Please try again later."}./api/eventmakes the same check. When the store is full, it does not store the event and still answers 200, so analytics never show an error.Daily upload budget
100 x 1024 x 1024bytes) of plan data per UTC day. The server counts the UTF-8 size of the request body./api/shareanswers 429 with{"error": "Daily sharing limit reached for your network. Please try again tomorrow."}. A refused share adds nothing to the count.CleanupServiceprunes it every hour, the same way it prunes the limiters. A restart resets it.Client key
Order of checks in /api/share
ttl_daysnot a number (400).A budget charge is not returned if the insert then fails. That is rare, and it costs a client at most one upload of its daily allowance.
Bad input
/api/shareand/api/eventanswer 400 instead of 500 when the JSON root is not an object or a field has the wrong type. The fields arettl_daysfor a share, andpathandreferrerfor an event. A JSONnullcounts as not sent./api/eventanswers 400 for apathlonger than 512 characters.{"error": "..."}. That includes the existing 400 answers and the per-minute 429 on/api/share, so the web app can show the text. The 429 on the other endpoints still has no body.Delete token
DELETE /api/plans/{id}accepts the token in theX-Delete-Tokenheader. It still accepts?token=for older clients. If both are present, the header wins.Web app
errortext from the server (507, 429 and 400). Before, it showed "Share failed: server returned N". That message is still the fallback when the reply has noerrortext, for example an HTML page from the proxy.AnalysisResultand the text report. The payload is unchanged.Dashboard
server/PlanShare/dashboard.htmlhas a local array namedhistory, which hideswindow.history. Thenhistory.replaceState(...)threw after the token was saved, and#token=stayed in the address bar. The call is nowwindow.history.replaceState(...). I reproduced the shadowing with a short Node script.Tests and CI
tests/PlanViewer.Core.Testsnow referencesserver/PlanShare/PlanShare.csproj, so the solution build and the test run cover the server. The server hasInternalsVisibleTofor the test project.codefilter in.github/workflows/ci.ymlnow includesserver/PlanShare/**. Without it, a PR that changes only the server skips the build and the tests.PlanShare:DataDir,PlanShare:MaxDatabaseBytesandPlanShare:DailyUploadBytes. Without them the server usesdata/next to the binary, the 10 GB limit and the 100 MB budget. The endpoint tests use them to run a server with a temp database and small limits..github/workflows/deploy-planshare.ymlis not changed.Which component(s) does this affect?
The template has no box for the PlanShare server or the web app. This PR changes both.
How was this tested?
Platform: Windows. This change has no plan files.
WebApplicationFactorywith a temp database. They check the 400 cases, the 507 answer, and both 429 answers. They check that/api/eventanswers 200 when the store is full, and that a path of 512 characters passes and one of 513 fails. They also check delete by header, delete by?token=, and the CORS preflight forX-Delete-Token. The budget tests check one key for an IPv4 address and its mapped form, and one key for two addresses in one /64.PlanShareServiceagainst a stub HTTP handler. They check the error text for 507, 429 and 400, and the fallback message. They also check that a delete request has the header and notokenin the URL. Nothing is sent to a real server.errortext. Both servers are stopped. Nothing was sent tostats.erikdarling.com.dotnet publish server/PlanShare/PlanShare.csproj -c Release -r linux-x64 --self-contained -p:PublishSingleFile=true) on this branch and onb34b1fb. Both outputs have the same files, includingPlanShareandlibe_sqlite3.so. The binary is about 5 KB larger.dotnet build PlanViewer.sln -c Release --no-incrementalgives 0 warnings and 0 errors. A Debug build gives the same.dotnet testrun (Release) had 1136 tests: 1134 passed, 2 skipped, 0 failed.Not done
All eight items are done. Three things I did not check:
X-Delete-Tokenheader.Checklist
dotnet build -c Debug)dotnet test)🤖 Generated with Claude Code
https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza