-
Notifications
You must be signed in to change notification settings - Fork 30
fix: XSS, DPAPI scope, rate-limiter eviction (bug-bash batch 1) #249
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,8 +44,14 @@ created_at TEXT NOT NULL | |
| cmd.ExecuteNonQuery(); | ||
| } | ||
|
|
||
| // --- 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); | ||
|
|
||
| // Register the cleanup background service | ||
| builder.Services.AddSingleton(new PlanDbConfig(connectionString)); | ||
| builder.Services.AddSingleton(new RateLimiters(rateLimiter, analyticsRateLimiter)); | ||
| builder.Services.AddHostedService<CleanupService>(); | ||
|
|
||
| // Request size limit (10 MB) | ||
|
|
@@ -54,10 +60,6 @@ created_at TEXT NOT NULL | |
| var app = builder.Build(); | ||
| app.UseCors(); | ||
|
|
||
| // --- Rate limiters (in-memory) --- | ||
| var rateLimiter = new RateLimiter(maxRequests: 10, windowSeconds: 60); | ||
| var analyticsRateLimiter = new RateLimiter(maxRequests: 30, windowSeconds: 60); | ||
|
|
||
| const int MaxTtlDays = 365; | ||
|
|
||
| // --- Endpoints --- | ||
|
|
@@ -161,9 +163,15 @@ created_at TEXT NOT NULL | |
| return Results.BadRequest("Invalid JSON"); | ||
| } | ||
|
|
||
| // Strip referrer to domain only (no full URLs with query params) | ||
| if (!string.IsNullOrEmpty(referrer) && Uri.TryCreate(referrer, UriKind.Absolute, out var refUri)) | ||
| referrer = refUri.Host; | ||
| // Strip referrer to domain only (no full URLs with query params). | ||
| // If it doesn't parse as an absolute URL, drop it — never persist raw | ||
| // client-supplied strings, since the dashboard renders referrers in HTML. | ||
| if (!string.IsNullOrEmpty(referrer)) | ||
| { | ||
| referrer = Uri.TryCreate(referrer, UriKind.Absolute, out var refUri) | ||
| ? refUri.Host | ||
| : null; | ||
| } | ||
|
|
||
| // Visitor hash: SHA256(IP + User-Agent + date) — unique per day, no PII stored | ||
| var ua = ctx.Request.Headers.UserAgent.FirstOrDefault() ?? ""; | ||
|
|
@@ -352,14 +360,18 @@ static string GenerateDeleteToken() | |
|
|
||
| record PlanDbConfig(string ConnectionString); | ||
|
|
||
| record RateLimiters(RateLimiter Share, RateLimiter Analytics); | ||
|
|
||
| sealed class CleanupService : BackgroundService | ||
| { | ||
| private readonly PlanDbConfig _config; | ||
| private readonly RateLimiters _rateLimiters; | ||
| private readonly ILogger<CleanupService> _logger; | ||
|
|
||
| public CleanupService(PlanDbConfig config, ILogger<CleanupService> logger) | ||
| public CleanupService(PlanDbConfig config, RateLimiters rateLimiters, ILogger<CleanupService> logger) | ||
| { | ||
| _config = config; | ||
| _rateLimiters = rateLimiters; | ||
| _logger = logger; | ||
| } | ||
|
|
||
|
|
@@ -401,6 +413,14 @@ private void Cleanup() | |
| if (deleted > 0) | ||
| _logger.LogInformation("Cleaned up {Count} old page views", deleted); | ||
| } | ||
|
|
||
| // Evict stale rate-limiter keys so the dictionary doesn't grow forever. | ||
| var shareEvicted = _rateLimiters.Share.Sweep(); | ||
| var analyticsEvicted = _rateLimiters.Analytics.Sweep(); | ||
| if (shareEvicted + analyticsEvicted > 0) | ||
| _logger.LogInformation( | ||
| "Evicted {Share} share + {Analytics} analytics rate-limit keys", | ||
| shareEvicted, analyticsEvicted); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
|
|
@@ -441,4 +461,25 @@ public bool IsAllowed(string key) | |
| return true; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Evicts keys whose timestamp lists have gone empty. Call periodically | ||
| /// so the dictionary doesn't grow forever across unique IPs. | ||
| /// Returns the number of keys evicted. | ||
| /// </summary> | ||
| public int Sweep() | ||
| { | ||
| var cutoff = DateTime.UtcNow.AddSeconds(-_windowSeconds); | ||
| var evicted = 0; | ||
| foreach (var kvp in _requests) | ||
| { | ||
| lock (kvp.Value) | ||
| { | ||
| kvp.Value.RemoveAll(t => t < cutoff); | ||
| if (kvp.Value.Count == 0 && _requests.TryRemove(kvp)) | ||
| evicted++; | ||
| } | ||
| } | ||
| return evicted; | ||
| } | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor race between
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 Generated by Claude Code |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -275,13 +275,28 @@ <h3>Top Referrers (30 days)</h3> | |
| } | ||
| function updateReferrers() { | ||
| var tbody = document.querySelector("#referrersTable tbody"); | ||
| tbody.replaceChildren(); | ||
| if (!planStats || !planStats.traffic.top_referrers.length) { | ||
| tbody.innerHTML = "<tr><td colspan='2' style='color:#484f58'>No referrer data yet</td></tr>"; | ||
| var tr = document.createElement("tr"); | ||
| var td = document.createElement("td"); | ||
| td.setAttribute("colspan", "2"); | ||
| td.style.color = "#484f58"; | ||
| td.textContent = "No referrer data yet"; | ||
| tr.appendChild(td); | ||
| tbody.appendChild(tr); | ||
| return; | ||
| } | ||
| tbody.innerHTML = planStats.traffic.top_referrers.map(function(r) { | ||
| return "<tr><td>" + r.referrer + "</td><td class='num'>" + fmt(r.count) + "</td></tr>"; | ||
| }).join(""); | ||
| planStats.traffic.top_referrers.forEach(function(r) { | ||
| var tr = document.createElement("tr"); | ||
| var tdRef = document.createElement("td"); | ||
| tdRef.textContent = r.referrer; | ||
| var tdCount = document.createElement("td"); | ||
| tdCount.className = "num"; | ||
| tdCount.textContent = fmt(r.count); | ||
| tr.appendChild(tdRef); | ||
| tr.appendChild(tdCount); | ||
| tbody.appendChild(tr); | ||
| }); | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Generated by Claude Code |
||
| } | ||
| function updateCharts() { | ||
| updateChart("starsChart", "line", { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,12 +11,12 @@ | |
| { | ||
| try | ||
| { | ||
| CredentialManager.WriteCredential( | ||
|
Check warning on line 14 in src/PlanViewer.Core/Services/WindowsCredentialService.cs
|
||
| applicationName: Prefix + serverId, | ||
| userName: username, | ||
| secret: password, | ||
| comment: "planview credential", | ||
| persistence: CredentialPersistence.LocalMachine); | ||
| persistence: CredentialPersistence.Enterprise); | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also: pre-existing Generated by Claude Code |
||
| return true; | ||
| } | ||
| catch { return false; } | ||
|
|
@@ -24,7 +24,7 @@ | |
|
|
||
| public (string Username, string Password)? GetCredential(string serverId) | ||
| { | ||
| var cred = CredentialManager.ReadCredential(Prefix + serverId); | ||
|
Check warning on line 27 in src/PlanViewer.Core/Services/WindowsCredentialService.cs
|
||
| if (cred == null) return null; | ||
| return (cred.UserName ?? "", cred.Password ?? ""); | ||
| } | ||
|
|
@@ -33,7 +33,7 @@ | |
| { | ||
| try | ||
| { | ||
| CredentialManager.DeleteCredential(Prefix + serverId); | ||
|
Check warning on line 36 in src/PlanViewer.Core/Services/WindowsCredentialService.cs
|
||
| return true; | ||
| } | ||
| catch { return false; } | ||
|
|
@@ -41,7 +41,7 @@ | |
|
|
||
| public bool CredentialExists(string serverId) | ||
| { | ||
| return CredentialManager.ReadCredential(Prefix + serverId) != null; | ||
|
Check warning on line 44 in src/PlanViewer.Core/Services/WindowsCredentialService.cs
|
||
| } | ||
|
|
||
| public bool UpdateCredential(string serverId, string username, string password) | ||
|
|
@@ -54,7 +54,7 @@ | |
| /// </summary> | ||
| public IReadOnlyList<(string ServerName, string Username)> ListAll() | ||
| { | ||
| return CredentialManager.EnumerateCredentials() | ||
|
Check warning on line 57 in src/PlanViewer.Core/Services/WindowsCredentialService.cs
|
||
| .Where(c => c.ApplicationName.StartsWith(Prefix, StringComparison.Ordinal)) | ||
| .Select(c => (c.ApplicationName[Prefix.Length..], c.UserName ?? "")) | ||
| .ToList(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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