BlogRateLimiter: HashSet counters collapse same-millisecond increments #796

New Issue

2026-07-22T08:45:13Z

spikerj commented

2026-07-22 08:45:13 +00:00

Summary

BlogRateLimiter.IncrementCounters stores DateTime.UtcNow in HashSet<DateTime> for minute/hour/day windows. Rapid successive increments in the same millisecond collapse to a single entry, so limits are effectively unenforceable under burst traffic.

Repro

Pinned characterization in BlogRateLimiterTests on test/coverage-wave-2026-07 / PR #454: calling IncrementPostCounters MaxPostsPerHour times then CheckPostRateLimit still allows when all stamps share one tick.

Fix

Use a List<DateTime> (or counter + bucket timestamps), or ensure uniqueness (e.g. stopwatch ticks / sequence) so N increments yield N counted events.

Impact

Post/comment rate limits can be bypassed by clients that burst within one millisecond.

## Summary `BlogRateLimiter.IncrementCounters` stores `DateTime.UtcNow` in `HashSet<DateTime>` for minute/hour/day windows. Rapid successive increments in the same millisecond collapse to a single entry, so limits are effectively unenforceable under burst traffic. ## Repro Pinned characterization in `BlogRateLimiterTests` on `test/coverage-wave-2026-07` / PR #454: calling `IncrementPostCounters` `MaxPostsPerHour` times then `CheckPostRateLimit` still allows when all stamps share one tick. ## Fix Use a `List<DateTime>` (or counter + bucket timestamps), or ensure uniqueness (e.g. stopwatch ticks / sequence) so N increments yield N counted events. ## Impact Post/comment rate limits can be bypassed by clients that burst within one millisecond.

spikerj commented

2026-07-22 08:54:42 +00:00

Same HashSet<DateTime> same-ms collapse also affects CustomerRateLimiter (IncrementCreateCounters / IncrementUpdateCounters). Characterization pinned in CustomerRateLimiterTests.IncrementCreateCounters_SameMillisecondBurst_DoesNotEnforceHourLimit on PR #454 / test/coverage-wave-2026-07.

Same HashSet<DateTime> same-ms collapse also affects `CustomerRateLimiter` (`IncrementCreateCounters` / `IncrementUpdateCounters`). Characterization pinned in `CustomerRateLimiterTests.IncrementCreateCounters_SameMillisecondBurst_DoesNotEnforceHourLimit` on PR #454 / `test/coverage-wave-2026-07`.

spikerj commented

2026-07-29 15:48:26 +00:00

Audited against origin/master — NOT DONE. The PR that references this ticket did not fix it. Correcting the record, because the git trail is actively misleading here.

The production bug is untouched:

SpikerSoft.Business/Domain/Blog/Services/BlogRateLimiter.cs:207-209 — MinuteCounts / HourCounts / DayCounts are still HashSet<DateTime>.
:179-185 — IncrementCounters still calls data.MinuteCounts.Add(now) with a single now value, so N increments within the same instant still collapse to 1.

PR #455 (6cdfc029) is a test-determinism change, not the fix. Its own commit message says so: "the sole production change is the constructor/clock seam" and "BlogRateLimiterTests (the named #796 origin) already pinned this deterministically … so it is left unchanged." It injected TimeProvider into LessonRateLimiter and CustomerRateLimiter so the burst tests reproduce the collapse reliably on CI. Its file list does not include BlogRateLimiter.cs at all.

Remaining:

BlogRateLimiter.cs:207-209 → List<DateTime> or a counter+bucket structure (the ticket's subject).
CustomerRateLimiter.cs:204-207 → same change; this was added to scope by comment 10661 and is still HashSet<DateTime>.
Invert the characterization tests when fixing. They currently assert the buggy behaviour — CustomerRateLimiterTests.cs:80 literally reads "HashSet<DateTime> collapses same-instant increments (same as #796)". Leaving those green after the fix would mean the fix didn't take.

One adjacent note, not this ticket's scope: BookUploadRateLimiter.cs:152-153 and LessonRateLimiter.cs:134-135 share the identical pattern (LessonRateLimiterTests.cs:70 already pins it). Worth folding into the same pass since it's the same three-line change in four files.

Audited against `origin/master` — **NOT DONE. The PR that references this ticket did not fix it.** Correcting the record, because the git trail is actively misleading here. **The production bug is untouched:** - `SpikerSoft.Business/Domain/Blog/Services/BlogRateLimiter.cs:207-209` — `MinuteCounts` / `HourCounts` / `DayCounts` are still `HashSet<DateTime>`. - `:179-185` — `IncrementCounters` still calls `data.MinuteCounts.Add(now)` with a single `now` value, so N increments within the same instant still collapse to 1. **PR #455 (`6cdfc029`) is a test-determinism change, not the fix.** Its own commit message says so: *"the sole production change is the constructor/clock seam"* and *"BlogRateLimiterTests (the named #796 origin) already pinned this deterministically … so it is left unchanged."* It injected `TimeProvider` into `LessonRateLimiter` and `CustomerRateLimiter` so the burst tests reproduce the collapse reliably on CI. **Its file list does not include `BlogRateLimiter.cs` at all.** **Remaining:** 1. `BlogRateLimiter.cs:207-209` → `List<DateTime>` or a counter+bucket structure (the ticket's subject). 2. `CustomerRateLimiter.cs:204-207` → same change; this was added to scope by comment 10661 and is still `HashSet<DateTime>`. 3. **Invert the characterization tests when fixing.** They currently *assert the buggy behaviour* — `CustomerRateLimiterTests.cs:80` literally reads `"HashSet<DateTime> collapses same-instant increments (same as #796)"`. Leaving those green after the fix would mean the fix didn't take. One adjacent note, not this ticket's scope: `BookUploadRateLimiter.cs:152-153` and `LessonRateLimiter.cs:134-135` share the identical pattern (`LessonRateLimiterTests.cs:70` already pins it). Worth folding into the same pass since it's the same three-line change in four files.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: spikerj/spikersoft-issues#796