Fix session and remote-input flood protection - #79
Merged
Conversation
ThreatCrush Security Scan66 finding(s) HIGH/CRITICAL: 13 | MEDIUM: 35 | LOW: 18
…and 16 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
The Lint job failed on one error, which stopped Type check and Format check from running at all — and both of those had problems waiting behind it. getClientIp used `||` where @typescript-eslint/prefer-nullish-coalescing wants `??`. Swapping the operator would have been a silent behaviour change: an `x-forwarded-for` of ", 1.2.3.4" trims its first hop to an empty string, and `??` returns that empty string rather than falling through. An empty rate-limit key is a *different* bucket from the shared 'unknown' fallback, so a client could double its own allowance just by prefixing a comma — in the one file whose whole job is to stop that. The check is now explicit about both the undefined and the empty case, which satisfies the rule and keeps the original semantics exactly. Covered by tests: a blank first hop from a leading comma, a whitespace-only header and an empty header all resolve to the shared bucket, plus x-real-ip precedence and whitespace trimming. Format check was failing on six files that had never been run through Prettier. Formatted, no logic touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
prune() examined a single entry and then broke out whenever the map sat below its cap, so expired entries accumulated until the table filled up instead of being cleaned as they aged. Once full it deleted whichever key came first in insertion order rather than the one nearest expiry, which could restart a throttle that still had most of its window left while an about-to-expire entry survived. The sweep is safe to run to completion because entries are written with one constant window, so iteration order is ascending resetAt: the first entry still inside its window means every entry behind it is too. That invariant had a hole — `Map.set` on a key that already exists keeps its original position, so a key whose window restarted stayed at the head with a new resetAt and stopped the sweep before it reached genuinely expired keys. check() now deletes before re-inserting so a refreshed key moves to the tail. Cost stays amortised O(1) per call: each entry is deleted once in its life plus at most one eviction, so this does not turn a burst of fresh keys into a full scan per request. Adds a `size` accessor for tests and diagnostics, and five tests. Two fail against the previous implementation: a table of 50 expired entries only shed one of them, and a restarted key left expired entries behind it unreachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Summary
Validation