sneak/smallwebwaf
1
0
Fork 0
Code Issues 58 Pull Requests 3 Actions 7 Packages Projects Releases Wiki Activity

Requests waiting for a client's first lookup are counted at the time they arrived, and one can wipe another's count #129

New Issue
Closed
opened 2026-10-14 18:13:37 +02:00 by clawbot · 1 comment
clawbot commented 2026-10-14 18:13:37 +02:00
Collaborator
Copy Link
Copy Source

internal/proxy/request.go, checkClient: now is read before lookUp, which can wait up to SWWAF_LOOKUP_TIMEOUT for a new client's first answer from GeoJS while a country list, a biased threshold or SWWAF_ADD_LOOKUP_HEADERS is set. limitBroken(now) then counts the request at that earlier time. Buckets.Add in internal/ratelimit/ratelimit.go takes a request dated more than a second before the bucket under way began for a clock set back, and empties that window's buckets.

Repro, with SWWAF_DENIED_COUNTRIES=kp, SWWAF_LOOKUP_TIMEOUT=1h, a stand-in for GeoJS that holds its answer, and a test clock:

  1. At 00:00:58 the client's first request arrives and waits for GeoJS.
  2. At 00:01:00.5 its second arrives and waits too.
  3. GeoJS answers, and both go on, in either order.
  4. At 00:01:01 a third request comes.

The third request's minute count should be about 2.98 (1 × 59/60 + 2). When the second request is counted before the first, the first empties the minute's buckets and the count is 1.98: the second request is lost. A test doing this 12 times got 1.98 in 6.

README, request log, counts: "the client's requests in the minute, the hour and the day as the rate limits count them, this request included: in each window, those in the bucket under way and a share of those in the bucket before". The comment on Buckets.Add keeps emptying the buckets for "the clock was set back"; here it was not.

The two requests must arrive more than a second apart across a minute boundary while both wait, so at the default SWWAF_LOOKUP_TIMEOUT of 1s this needs the process to be held up; a longer timeout makes it easy. Untested: reading now after lookUp should avoid it.

Severity: minor.

Model: opus-5-5

`internal/proxy/request.go`, `checkClient`: `now` is read before `lookUp`, which can wait up to `SWWAF_LOOKUP_TIMEOUT` for a new client's first answer from GeoJS while a country list, a biased threshold or `SWWAF_ADD_LOOKUP_HEADERS` is set. `limitBroken(now)` then counts the request at that earlier time. `Buckets.Add` in `internal/ratelimit/ratelimit.go` takes a request dated more than a second before the bucket under way began for a clock set back, and empties that window's buckets. Repro, with `SWWAF_DENIED_COUNTRIES=kp`, `SWWAF_LOOKUP_TIMEOUT=1h`, a stand-in for GeoJS that holds its answer, and a test clock: 1. At 00:00:58 the client's first request arrives and waits for GeoJS. 2. At 00:01:00.5 its second arrives and waits too. 3. GeoJS answers, and both go on, in either order. 4. At 00:01:01 a third request comes. The third request's minute count should be about 2.98 (1 × 59/60 + 2). When the second request is counted before the first, the first empties the minute's buckets and the count is 1.98: the second request is lost. A test doing this 12 times got 1.98 in 6. README, request log, `counts`: "the client's requests in the minute, the hour and the day as the rate limits count them, this request included: in each window, those in the bucket under way and a share of those in the bucket before". The comment on `Buckets.Add` keeps emptying the buckets for "the clock was set back"; here it was not. The two requests must arrive more than a second apart across a minute boundary while both wait, so at the default `SWWAF_LOOKUP_TIMEOUT` of `1s` this needs the process to be held up; a longer timeout makes it easy. Untested: reading `now` after `lookUp` should avoid it. Severity: minor. Model: opus-5-5
clawbot commented 2026-10-14 19:10:15 +02:00
Author
Collaborator
Copy Link
Copy Source

Fixed in #133: the checks after the lookup read the time again, so a request that waited for GeoJS is counted for the rate limits when it goes on, and no longer starts the minute afresh over another request of its client.

Model: opus-5-5

Fixed in https://git.eeqj.de/sneak/smallwebwaf/pulls/133: the checks after the lookup read the time again, so a request that waited for GeoJS is counted for the rate limits when it goes on, and no longer starts the minute afresh over another request of its client. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
Reviewed and ready for the owner to merge
 needs-checks
Checks must be fixed before review
 needs-rebase
Conflicts with next; rebase required
 needs-review
Review not yet done
 needs-rework
Review found issues to fix
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/smallwebwaf#129