Harden API input validation and bound rate-limiter memory#4
Merged
Conversation
The per-key bucket map grew without bound because entries were never removed, so requests under many distinct keys could exhaust memory. Track lastSeen per bucket and add EvictStale plus a RunEviction ticker loop that prunes idle buckets. Cutoff is passed in so the logic stays pure and testable without sleeps.
Wire the limiter's RunEviction loop into the app lifecycle next to the worker and pruner goroutines, so the eviction added in the previous commit actually runs. Add WHOOK_INGEST_EVICT_INTERVAL and WHOOK_INGEST_IDLE_TTL to tune the sweep, defaulting to a 10m sweep and a 1h idle TTL, and document them.
A source name becomes the ingest URL path segment and the rate-limiter key, but only non-emptiness was checked on register and nothing on update. Reject empty, over-long, and names containing spaces, control characters, or slashes so bad names are caught at registration instead of producing awkward ingest paths later.
Bad input was accepted and stored, then failed deep in the delivery worker or silently produced wrong results. Reject it with a 400 up front: - destination URL must be an absolute http/https URL, on create and update - per-destination backoff must have sane bounds (a zero max_attempts dead-letters on the first failure; zero delays cause a tight retry loop) - filter clauses that can never match (empty paths, empty header names, empty allow-lists) are rejected instead of silently dropping every event - unknown event status filters return 400 rather than an empty list - list pagination is clamped: limit is capped so a huge value cannot force an unbounded query, and a negative offset is floored to zero (it is a hard error on Postgres)
It is imported directly by the ratelimit package; go mod tidy moves it out of the indirect block.
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.
This picks up the two open PRs (#2 destination URL validation, #3 rate-limiter stale eviction), lands them correctly, and applies the same two ideas to the other places in the code where they were missing.
The two themes:
Rate-limiter eviction (supersedes #3)
lastSeenper bucket and addEvictStaleplus aRunEvictionticker loop.RunEvictioninto the app lifecycle next to the worker and pruner goroutines. Fix/ratelimit stale eviction #3 added the eviction method but nothing ever called it, so the leak it described stayed open. This starts the sweep so the map is actually bounded.WHOOK_INGEST_EVICT_INTERVAL(default 10m) andWHOOK_INGEST_IDLE_TTL(default 1h).The bucket map is keyed by source name and is populated before the source is validated, so hitting
POST /ingest/{random}with many names grows it without bound even though every request 404s. Eviction bounds it to the idle window.API boundary validation (supersedes #2)
max_attemptsdead-letters on the first failure and a zero delay causes a tight redelivery loop, both straight from unvalidated JSON.body_inlists) are rejected instead of silently dropping every event for that destination.statusfilters return 400 instead of an empty list.limitis capped at 500 so a huge value cannot force an unbounded query, and a negativeoffsetis floored to zero (it is a hard error on Postgres).Source names
All new logic has tests (pure-function unit tests plus handler-level integration tests over a real server and store).
go vetandgo test -race ./...pass.