fix: XSS, DPAPI scope, rate-limiter eviction (bug-bash batch 1) by erikdarlingdata · Pull Request #249 · erikdarlingdata/PerformanceStudio

erikdarlingdata · 2026-04-21T21:09:01Z

Summary

Three security / reliability fixes surfaced by a max-effort bug-bash review. Each is small, localized, verified end-to-end, and independent.

1. Stored XSS on the PlanShare dashboard

server/PlanShare/dashboard.html:282 rendered r.referrer via innerHTML string-concat. server/PlanShare/Program.cs:165 only sanitized referrers when Uri.TryCreate succeeded — non-URL strings flowed through raw.

Fix (defense in depth):

Server now sets referrer = null when TryCreate rejects the input, so raw attacker strings never reach storage.
Dashboard rebuilds the referrer table with document.createElement + textContent, so even any legacy row containing a hostile string can't execute.

2. DPAPI scope leaks SQL creds across Windows users

WindowsCredentialService.cs:19 was saving with CredentialPersistence.LocalMachine, which makes the credential decryptable by any account on the same box (shared dev VMs, jump hosts, Citrix).

Fix: one-word change to CredentialPersistence.Enterprise (per-user, DPAPI-scoped, roams with the domain profile). Pre-existing LocalMachine-scoped credentials remain readable; next re-save migrates them.

3. PlanShare rate limiter grows unbounded

server/PlanShare/Program.cs — the ConcurrentDictionary<string, List<DateTime>> keyed by IP pruned per-IP timestamp lists but never evicted the keys. Any unique-IP traffic over time grew memory forever.

Fix: added RateLimiter.Sweep() which prunes each list and TryRemoves keys whose list is empty. Both limiters are DI-registered via a RateLimiters container; CleanupService invokes Sweep() on its existing hourly tick and logs evictions.

Test plan

dotnet build on PlanViewer.Core and PlanShare: 0 errors, no new warnings.
XSS end-to-end: ran PlanShare locally, POSTed <img src=x onerror=...> and "><script>...</script> plus a legit URL to /api/event, then GET /api/stats. Only good.example.com landed in top_referrers; both XSS payloads dropped as expected.
DPAPI round-trip: SaveCredential → GetCredential → DeleteCredential with Enterprise scope and a password containing !, $, " — byte-for-byte match.
Rate limiter: 10 000 unique keys, window=1s. Fresh Sweep() evicted 0 (expected). Waited 1.5s, stale Sweep() evicted 10 000, 0 remaining. Post-sweep insert still works.

Dropped (false positives)

Bug-bash agents flagged several that didn't survive verification; not addressing these:

BenefitScorer.cs:584 "ms² unit bug" — dimensionally ms·ms/ms = ms, math is correct.
BenefitScorer.cs:367-378 Key-Lookup "double counts parent NL" — CostPercent is own-cost, not subtree (see ShowPlanParser.cs:1787).
PlanAnalyzer.cs:863 Rule 32 "fires on ActualRows=0" — that IS the scan-caused-by-bad-CE case the rule targets.
QueryStoreGridControl:1231 "duplicate Click handlers accumulate" — lines 1213-1219 explicitly -= before +=.
A handful of others (PlanShare DELETE "token leak", HKCU registry "injection", installer symlink, etc.) — detailed in the review thread.

🤖 Generated with Claude Code

- PlanShare dashboard: rebuild referrer table with DOM + textContent instead of innerHTML, so legacy rows containing hostile strings can't execute script. - PlanShare /api/event: drop the referrer entirely when Uri.TryCreate rejects it, instead of persisting the raw client-supplied string. - WindowsCredentialService: save credentials with Enterprise scope (current-user, DPAPI-encrypted) instead of LocalMachine (readable by every user on the box). - PlanShare RateLimiter: add Sweep() that prunes and TryRemoves empty keys, invoked from CleanupService's existing hourly tick so the dictionary no longer grows unbounded across unique IPs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

erikdarlingdata

Review summary — PR #249 (fix: XSS, DPAPI scope, rate-limiter eviction)

What the PR does
Three independent, localized fixes: (1) server drops non-URL referrers to null and dashboard rebuilds the referrer table via createElement/textContent (stored-XSS defense in depth); (2) WindowsCredentialService persistence flipped LocalMachine → Enterprise so DPAPI scope is per-user; (3) RateLimiter.Sweep() added and called from CleanupService's existing hourly tick to evict empty-list keys from the per-IP dictionary.

What's good

Base branch is dev. ✓
XSS fix is layered (server-side normalization + DOM API rendering), not just one side.
RateLimiters record + DI registration is cleaner than reaching into captured locals from the background service.
PR body is detailed, lists the drop/false-positive findings, and describes the manual verification for each change.
No Avalonia/XAML/PlanAnalyzer surface touched — the "keep PlanAnalyzer rules synced with PerformanceMonitor" concern does not apply here.

What needs attention

Program.cs:484 — minor race between Sweep() and IsAllowed() where a request timestamp can be dropped if the key is evicted mid-call. Bounded impact, worth knowing about. Inline comment with details.
Program.cs:169-174 — relative-URL referrers now drop to null instead of being stored. Almost certainly desired, but worth a sanity check against real client payloads.
WindowsCredentialService.cs:19 — Enterprise is correct; minor nit on the PR description's "roams with the domain profile" wording (it's opt-in via AD credential roaming policy). Existing LocalMachine creds still decrypt, so silent migration is fine.
dashboard.html:277 — replaceChildren() is modern-only; fine for a maintainer dashboard, flag if this page ever gets linked from user-facing surfaces.
Tests: src/PlanViewer.Core/Services/WindowsCredentialService.cs changed but nothing was added under tests/PlanViewer.Core.Tests/. The DPAPI change isn't easily unit-testable (Win32-only, touches real credential store) and the PR has a documented manual round-trip, so this is a reasonable skip. However, RateLimiter.Sweep() is pure in-process logic and is a good candidate for a unit test — the PR body describes exactly the 10k-key scenario you'd want captured as a regression test.

No changes requested — maintainer's call.

Generated by Claude Code

erikdarlingdata · 2026-04-21T21:10:40Z

+            }
+        }
+        return evicted;
+    }


Minor race between Sweep() and IsAllowed():

Thread A calls IsAllowed(key) — GetOrAdd returns existing list L (all entries expired, but not yet pruned).

Thread B (Sweep) acquires lock on L first, prunes to empty, TryRemoves the key, releases lock.

Thread A acquires lock on the now-detached L, adds now, returns true.

Thread A's next request: GetOrAdd creates a fresh empty list L2, so the earlier timestamp is lost.

Impact is bounded (hourly sweep, 60s window) — a client could get at most one "free" request per race — but if you want it airtight, re-check _requests.ContainsKey(kvp.Key) inside IsAllowed after locking, or switch to AddOrUpdate semantics. Not a blocker.

Generated by Claude Code

erikdarlingdata · 2026-04-21T21:10:43Z

+        referrer = Uri.TryCreate(referrer, UriKind.Absolute, out var refUri)
+            ? refUri.Host
+            : null;
+    }


Defense-in-depth fix looks right. One thing to double-check: a legitimate referrer coming in as a relative URL (e.g. /some-path) now gets dropped entirely instead of stored. Given the dashboard only shows hostnames, that's probably desired, but worth confirming against whatever clients actually POST here.

Generated by Claude Code

erikdarlingdata · 2026-04-21T21:10:49Z

                secret: password,
                comment: "planview credential",
-                persistence: CredentialPersistence.LocalMachine);
+                persistence: CredentialPersistence.Enterprise);


Enterprise is the correct scope for per-user DPAPI isolation. One call-out for the PR description: "roams with the domain profile" is slightly overstated — Enterprise means "roams via domain creds mgmt (credential roaming policy)" which is opt-in at the AD level. On non-domain / domain-without-roaming machines it's effectively identical to LocalComputer in terms of where the blob lives, but still DPAPI-scoped per user. End result for the user is correct either way; just noting in case someone asks.

Also: pre-existing LocalMachine creds will still decrypt (DPAPI machine key still present), so silent migration on next save is fine. No action needed.

Generated by Claude Code

erikdarlingdata · 2026-04-21T21:10:52Z

+            tr.appendChild(tdRef);
+            tr.appendChild(tdCount);
+            tbody.appendChild(tr);
+        });


tbody.replaceChildren() + createElement + textContent is the right shape. replaceChildren() is 2020+ evergreen-browser only; fine for a maintainer-facing dashboard but if this page is ever linked to from somewhere user-facing, be aware older Edge/Safari won't render the table. (Not changing; just flagging.)

Generated by Claude Code

erikdarlingdata merged commit 7fbdf3e into dev Apr 21, 2026
2 checks passed

erikdarlingdata deleted the fix/bug-bash-xss-dpapi-ratelimit branch April 21, 2026 21:09

erikdarlingdata commented Apr 21, 2026

View reviewed changes

erikdarlingdata mentioned this pull request Apr 22, 2026

Release v1.7.2 — operator self-time fix + decimal benefit % #256

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: XSS, DPAPI scope, rate-limiter eviction (bug-bash batch 1)#249

fix: XSS, DPAPI scope, rate-limiter eviction (bug-bash batch 1)#249
erikdarlingdata merged 1 commit intodevfrom
fix/bug-bash-xss-dpapi-ratelimit

erikdarlingdata commented Apr 21, 2026

Uh oh!

Uh oh!

erikdarlingdata left a comment

Uh oh!

erikdarlingdata Apr 21, 2026

Uh oh!

erikdarlingdata Apr 21, 2026

Uh oh!

erikdarlingdata Apr 21, 2026

Uh oh!

erikdarlingdata Apr 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

erikdarlingdata commented Apr 21, 2026

Summary

1. Stored XSS on the PlanShare dashboard

2. DPAPI scope leaks SQL creds across Windows users

3. PlanShare rate limiter grows unbounded

Test plan

Dropped (false positives)

Uh oh!

Uh oh!

erikdarlingdata left a comment

Choose a reason for hiding this comment

Uh oh!

erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant