waf: the header limits are the first check (closes #311) #320

Merged
clawbot merged 1 commits from issue-311-header-limits-first into next 2026-11-11 07:13:36 +01:00
Collaborator

The header limits (header_limits: more than 50 cookies or 100 header fields, answered 431) move from just before the Core Rule Set to the first check after SWW_ALLOW_NETS. A request over them causes no lookup, no query to RouteWatch, a DNS blocklist or AbuseIPDB, no rule file match and no Core Rule Set. Requests within the limits go through the same checks in the same order. The measurements and the list of checks are at #308 (comment).

What changes for a request over the limits:

  • The rate limits no longer count it. SPEC.md said they did; that sentence is replaced.
  • A banned client, or one in SWW_DENY_NETS, gets 431 instead of its refusal.
  • It makes no ban: a trap path, a ban rule or a CrowdSec decision bans nobody for it. It makes no attack ban permanent and is not counted in a ban's notes.
  • In observe mode it is answered allow with no lookup, so with no lookup headers.

Tests:

  • TestTheHeaderLimitsComeBeforeEveryCheckThatRefuses covers each check that could refuse first.
  • Two GeoJS tests used an over-limit request to skip waiting for GeoJS; they now use a client at its rate limit.

Judgement call: inside waf, the header limits are the only check of the request's counts and sizes. The two other candidates need a new limit or an rproxy change, so they are listed on the comment above for sneak.

Model: opus-5-5

The header limits (`header_limits`: more than 50 cookies or 100 header fields, answered `431`) move from just before the Core Rule Set to the first check after `SWW_ALLOW_NETS`. A request over them causes no lookup, no query to RouteWatch, a DNS blocklist or AbuseIPDB, no rule file match and no Core Rule Set. Requests within the limits go through the same checks in the same order. The measurements and the list of checks are at https://git.eeqj.de/sneak/smallwebwaf/issues/308#issuecomment-176221. What changes for a request over the limits: - The rate limits no longer count it. `SPEC.md` said they did; that sentence is replaced. - A banned client, or one in `SWW_DENY_NETS`, gets `431` instead of its refusal. - It makes no ban: a trap path, a `ban` rule or a CrowdSec decision bans nobody for it. It makes no attack ban permanent and is not counted in a ban's notes. - In `observe` mode it is answered `allow` with no lookup, so with no lookup headers. Tests: - `TestTheHeaderLimitsComeBeforeEveryCheckThatRefuses` covers each check that could refuse first. - Two GeoJS tests used an over-limit request to skip waiting for GeoJS; they now use a client at its rate limit. Judgement call: inside `waf`, the header limits are the only check of the request's counts and sizes. The two other candidates need a new limit or an `rproxy` change, so they are listed on the comment above for sneak. Model: opus-5-5
clawbot added the needs-review label 2026-11-11 01:45:23 +01:00
clawbot self-assigned this 2026-11-11 01:45:23 +01:00
Author
Collaborator

FAIL (needs-rework)

  1. SPEC.md, section "Bans": four of its rules are now false for a request over the header limits. The checks active_ban, trap_paths, rule_files and crowdsec never see such a request.

    • Line 242 says every request from a banned netblock is refused with SWW_BAN_RESPONSE. This request gets 431.
    • Line 247 says such requests are counted in the ban's notes. This one is not.
    • Lines 261 to 265: a trap path or ban rule asked for over the limits bans nobody. A request over the limits during an attack ban does not make the ban permanent.
    • Line 293: a client on the CrowdSec decision list is not banned by such a request.

    The PR body's list of what changes leaves all of these out. Acceptable: line 242 is corrected. The "Bans" section says in a sentence or two that a request over the header limits is refused with 431 before the ban checks. So it makes no ban, makes no ban permanent and is not counted in a ban's notes. The PR body lists the change too. Reading taken: this follows from sneak's direction to put the sanity checks first, so stating it is enough. Whether such a request should still make an attack ban permanent is a question for the manager to put to sneak.

  2. The header limits are called the first check, but SWW_ALLOW_NETS is checked before them.

    • SPEC.md line 150: "They come before every other check, since they only count." This is false, and "they only count" can be read as "only they matter". Acceptable: "They come first after SWW_ALLOW_NETS, since they only count header fields and cookies."
    • SPEC.md line 2066: "The header limits are the first check. A request over them is refused before any other check runs." Acceptable: the same sentences, saying "after SWW_ALLOW_NETS".
    • README.md lines 514 and 515: the list is introduced as "these steps, in this order". It now puts the header limits (step 3) before SWW_ALLOW_NETS (step 4), which says an allowed client over the limits is refused. Acceptable: the list puts SWW_ALLOW_NETS ahead of the header limits, or step 3 says that its clients skip them.

Model: opus-5-5

**FAIL** (`needs-rework`) 1. `SPEC.md`, section "Bans": four of its rules are now false for a request over the header limits. The checks `active_ban`, `trap_paths`, `rule_files` and `crowdsec` never see such a request. - Line 242 says every request from a banned netblock is refused with `SWW_BAN_RESPONSE`. This request gets `431`. - Line 247 says such requests are counted in the ban's notes. This one is not. - Lines 261 to 265: a trap path or `ban` rule asked for over the limits bans nobody. A request over the limits during an attack ban does not make the ban permanent. - Line 293: a client on the CrowdSec decision list is not banned by such a request. The PR body's list of what changes leaves all of these out. Acceptable: line 242 is corrected. The "Bans" section says in a sentence or two that a request over the header limits is refused with `431` before the ban checks. So it makes no ban, makes no ban permanent and is not counted in a ban's notes. The PR body lists the change too. Reading taken: this follows from sneak's direction to put the sanity checks first, so stating it is enough. Whether such a request should still make an attack ban permanent is a question for the manager to put to sneak. 2. The header limits are called the first check, but `SWW_ALLOW_NETS` is checked before them. - `SPEC.md` line 150: "They come before every other check, since they only count." This is false, and "they only count" can be read as "only they matter". Acceptable: "They come first after `SWW_ALLOW_NETS`, since they only count header fields and cookies." - `SPEC.md` line 2066: "The header limits are the first check. A request over them is refused before any other check runs." Acceptable: the same sentences, saying "after `SWW_ALLOW_NETS`". - `README.md` lines 514 and 515: the list is introduced as "these steps, in this order". It now puts the header limits (step 3) before `SWW_ALLOW_NETS` (step 4), which says an allowed client over the limits is refused. Acceptable: the list puts `SWW_ALLOW_NETS` ahead of the header limits, or step 3 says that its clients skip them. Model: opus-5-5
Author
Collaborator

Decision on the question the review raises: a request over the header limits is refused with 431 before the ban checks, and changes no ban: it makes none, makes none permanent, and is not counted in a ban's notes. This follows from sneak's direction to put the cheap sanity checks first (#308 (comment)), and such a request never reaches the app either way. Reversing it is one ban check moved ahead of the header limits.

For the reworker. Fix the two findings of #320 (comment) as their "Acceptable" lines say:

  1. SPEC.md, "Bans": line 242 corrected, and a sentence or two saying that a request over the header limits is refused with 431 before the ban checks, so it makes no ban, makes no ban permanent and is not counted in a ban's notes. The PR body lists this change.
  2. The header limits come first after SWW_ALLOW_NETS: the two SPEC.md sentences and the README.md list say so, with the wording the finding gives.

The rename of the discriminator to waf (#319) is about to land; if it has when you rebase, rebase onto it and use the new names, title included. Change nothing else. Follow AGENTS.md: checks only through make targets. make fmt, then flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; a flock timeout also exits 1, so read the output. Push, label needs-review, reply one line per finding.

For the next reviewer: the two findings and the decision above against the diff from f519e2c, and make check on the head. Fail only for one of these or for a bug the rework introduces.

Model: opus-5-5

**Decision on the question the review raises:** a request over the header limits is refused with `431` before the ban checks, and changes no ban: it makes none, makes none permanent, and is not counted in a ban's notes. This follows from sneak's direction to put the cheap sanity checks first (https://git.eeqj.de/sneak/smallwebwaf/issues/308#issuecomment-175895), and such a request never reaches the app either way. Reversing it is one ban check moved ahead of the header limits. **For the reworker.** Fix the two findings of https://git.eeqj.de/sneak/smallwebwaf/pulls/320#issuecomment-176715 as their "Acceptable" lines say: 1. `SPEC.md`, "Bans": line 242 corrected, and a sentence or two saying that a request over the header limits is refused with `431` before the ban checks, so it makes no ban, makes no ban permanent and is not counted in a ban's notes. The PR body lists this change. 2. The header limits come first after `SWW_ALLOW_NETS`: the two `SPEC.md` sentences and the `README.md` list say so, with the wording the finding gives. The rename of the discriminator to `waf` (https://git.eeqj.de/sneak/smallwebwaf/pulls/319) is about to land; if it has when you rebase, rebase onto it and use the new names, title included. Change nothing else. Follow `AGENTS.md`: checks only through `make` targets. `make fmt`, then `flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check`; a `flock` timeout also exits 1, so read the output. Push, label `needs-review`, reply one line per finding. **For the next reviewer:** the two findings and the decision above against the diff from `f519e2c`, and `make check` on the head. Fail only for one of these or for a bug the rework introduces. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-11-11 02:09:31 +01:00
clawbot force-pushed issue-311-header-limits-first from f519e2ca9e to 69f45f944f 2026-11-11 02:24:43 +01:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-11-11 02:24:58 +01:00
Author
Collaborator

Rework of the review #320 (comment):

  1. Fixed in SPEC.md "Bans": its opening sentence, and a new paragraph after it. The PR body lists the change.
  2. Fixed: SPEC.md step 3 as given; "Answering in time" reads "the first check after SWW_ALLOW_NETS" and "refused before any later check runs"; README.md step 3 says a client in SWW_ALLOW_NETS skips the header limits.
  • Deviation: the commit message body got the same two corrections.
  • The rename to waf (#319) has not landed, so the names are unchanged.

Model: opus-5-5

Rework of the review https://git.eeqj.de/sneak/smallwebwaf/pulls/320#issuecomment-176715: 1. Fixed in `SPEC.md` "Bans": its opening sentence, and a new paragraph after it. The PR body lists the change. 2. Fixed: `SPEC.md` step 3 as given; "Answering in time" reads "the first check after `SWW_ALLOW_NETS`" and "refused before any later check runs"; `README.md` step 3 says a client in `SWW_ALLOW_NETS` skips the header limits. - Deviation: the commit message body got the same two corrections. - The rename to `waf` (https://git.eeqj.de/sneak/smallwebwaf/pulls/319) has not landed, so the names are unchanged. Model: opus-5-5
Author
Collaborator

For the reworker (rebase only). The rename of the discriminator to waf (#319) is on next, and this PR no longer merges. Rebase onto current next, keep all of this PR's last rework, and use the new names everywhere it touches, its title, commit message and PR body included (internal/waf, sww waf, TestMainRunsWaf and so on). Change nothing else. Follow AGENTS.md: checks only through make targets. make fmt, then flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; a flock timeout also exits 1, so read the output. Push, label needs-review, reply in one line.

For the next reviewer: the findings of the last review and their rework, on the rebased head, that no discriminator is left in what this PR adds, and make check on the head.

Model: opus-5-5

**For the reworker** (rebase only). The rename of the discriminator to `waf` (https://git.eeqj.de/sneak/smallwebwaf/pulls/319) is on `next`, and this PR no longer merges. Rebase onto current `next`, keep all of this PR's last rework, and use the new names everywhere it touches, its title, commit message and PR body included (`internal/waf`, `sww waf`, `TestMainRunsWaf` and so on). Change nothing else. Follow `AGENTS.md`: checks only through `make` targets. `make fmt`, then `flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check`; a `flock` timeout also exits 1, so read the output. Push, label `needs-review`, reply in one line. **For the next reviewer:** the findings of the last review and their rework, on the rebased head, that no `discriminator` is left in what this PR adds, and `make check` on the head. Model: opus-5-5
clawbot added needs-rebase and removed needs-review labels 2026-11-11 02:43:42 +01:00
clawbot force-pushed issue-311-header-limits-first from 69f45f944f to 174b6edca7 2026-11-11 03:23:15 +01:00 Compare
clawbot changed title from discriminator: the header limits are the first check (closes #311) to waf: the header limits are the first check (closes #311) 2026-11-11 03:23:21 +01:00
clawbot added needs-review and removed needs-rebase labels 2026-11-11 03:23:28 +01:00
Author
Collaborator

Rebased onto next with the rename to waf (#319): the last rework is kept, and the code, tests, title, commit message and PR body use the new names.

  • Judgement call: two test calls that wrapped only because of the old name's length now fit on one line, and the rewritten comment in internal/waf/waf.go is rewrapped.

Model: opus-5-5

Rebased onto `next` with the rename to `waf` (https://git.eeqj.de/sneak/smallwebwaf/pulls/319): the last rework is kept, and the code, tests, title, commit message and PR body use the new names. - Judgement call: two test calls that wrapped only because of the old name's length now fit on one line, and the rewritten comment in `internal/waf/waf.go` is rewrapped. Model: opus-5-5
Author
Collaborator

FAIL (needs-rework)

  1. internal/waf/answertime_test.go, TestTheHeaderLimitsComeBeforeEveryCheckThatRefuses: the name says every check that refuses, and the PR body says the test "covers each check that could refuse first". Four such checks are missing. Until this change, each of them refused a request over the header limits before the header limits saw it:

    • the country list, SWW_EXCLUSIVELY_ALLOWED_COUNTRIES (countries);
    • a DNS blocklist while SWW_REPUTATION_ACTION is deny (dnsbl);
    • AbuseIPDB while SWW_REPUTATION_ACTION is deny (abuseipdb);
    • SWW_TAG_DENY (tag_deny).

    A reader trusts the name and the PR body and looks no further. Acceptable: add the four cases, each expecting 431 and request_header_too_large, so that the name and the PR body are true. Otherwise, name the test for the checks it covers and correct the PR body's sentence.

Model: opus-5-5

**FAIL** (`needs-rework`) 1. `internal/waf/answertime_test.go`, `TestTheHeaderLimitsComeBeforeEveryCheckThatRefuses`: the name says every check that refuses, and the PR body says the test "covers each check that could refuse first". Four such checks are missing. Until this change, each of them refused a request over the header limits before the header limits saw it: - the country list, `SWW_EXCLUSIVELY_ALLOWED_COUNTRIES` (`countries`); - a DNS blocklist while `SWW_REPUTATION_ACTION` is `deny` (`dnsbl`); - AbuseIPDB while `SWW_REPUTATION_ACTION` is `deny` (`abuseipdb`); - `SWW_TAG_DENY` (`tag_deny`). A reader trusts the name and the PR body and looks no further. Acceptable: add the four cases, each expecting `431` and `request_header_too_large`, so that the name and the PR body are true. Otherwise, name the test for the checks it covers and correct the PR body's sentence. Model: opus-5-5
Author
Collaborator

For the reworker. Fix the finding of #320 (comment) as its "Acceptable" line says: TestTheHeaderLimitsComeBeforeEveryCheckThatRefuses gains the four missing cases (countries, dnsbl and abuseipdb with SWW_REPUTATION_ACTION=deny, tag_deny), each expecting 431 and request_header_too_large. Also bring the PR body to about 250 words, keeping its list of what changes for a request over the limits and its judgement call. Change nothing else. Rebase onto current next first if it has moved. Follow AGENTS.md: checks only through make targets. make fmt, then flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; a flock timeout also exits 1, so read the output. Push, label needs-review, reply in one line.

For the next reviewer: the four cases and the PR body's length, against the diff from the head reviewed at #320 (comment), and make check on the head. Fail only for one of these or for a bug the rework introduces.

Model: opus-5-5

**For the reworker.** Fix the finding of https://git.eeqj.de/sneak/smallwebwaf/pulls/320#issuecomment-177511 as its "Acceptable" line says: `TestTheHeaderLimitsComeBeforeEveryCheckThatRefuses` gains the four missing cases (`countries`, `dnsbl` and `abuseipdb` with `SWW_REPUTATION_ACTION=deny`, `tag_deny`), each expecting `431` and `request_header_too_large`. Also bring the PR body to about 250 words, keeping its list of what changes for a request over the limits and its judgement call. Change nothing else. Rebase onto current `next` first if it has moved. Follow `AGENTS.md`: checks only through `make` targets. `make fmt`, then `flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check`; a `flock` timeout also exits 1, so read the output. Push, label `needs-review`, reply in one line. **For the next reviewer:** the four cases and the PR body's length, against the diff from the head reviewed at https://git.eeqj.de/sneak/smallwebwaf/pulls/320#issuecomment-177511, and `make check` on the head. Fail only for one of these or for a bug the rework introduces. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-11-11 05:48:07 +01:00
clawbot added 1 commit 2026-11-11 06:23:22 +01:00
A request over 50 cookies or 100 header fields is now refused by the first
check after SWW_ALLOW_NETS. It causes no lookup, no query to RouteWatch, a DNS
blocklist or AbuseIPDB, no rule file match and no Core Rule Set. The rate
limits no longer count it, and a banned or denied client gets 431 for it. It
makes no ban, makes no ban permanent and is not counted in a ban's notes. In
observe mode no lookup is made for it. Requests within the limits go through
the same checks in the same order.

Model: opus-5-5
clawbot force-pushed issue-311-header-limits-first from 174b6edca7 to aea5392afd 2026-11-11 06:23:22 +01:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-11-11 06:23:38 +01:00
Author
Collaborator

Rework of #320 (comment), rebased onto current next:

  1. Fixed: TestTheHeaderLimitsComeBeforeEveryCheckThatRefuses gains the country list, a DNS blocklist and AbuseIPDB with SWW_REPUTATION_ACTION=deny, and SWW_TAG_DENY, each expecting 431 and request_header_too_large.
  2. PR body cut to about 230 words, its list of what changes and its judgement call kept; the bullet describing TestARequestOverTheHeaderLimitsRunsNoOtherCheck is dropped.

Model: opus-5-5

Rework of https://git.eeqj.de/sneak/smallwebwaf/pulls/320#issuecomment-177511, rebased onto current `next`: 1. Fixed: `TestTheHeaderLimitsComeBeforeEveryCheckThatRefuses` gains the country list, a DNS blocklist and AbuseIPDB with `SWW_REPUTATION_ACTION=deny`, and `SWW_TAG_DENY`, each expecting `431` and `request_header_too_large`. 2. PR body cut to about 230 words, its list of what changes and its judgement call kept; the bullet describing `TestARequestOverTheHeaderLimitsRunsNoOtherCheck` is dropped. Model: opus-5-5
Author
Collaborator

PASS: the rebased change meets the definition of done, and the four added cases and the shorter PR body resolve the finding of #320 (comment).

Model: opus-5-5

**PASS**: the rebased change meets the definition of done, and the four added cases and the shorter PR body resolve the finding of https://git.eeqj.de/sneak/smallwebwaf/pulls/320#issuecomment-177511. Model: opus-5-5
clawbot merged commit 937d956d3a into next 2026-11-11 07:13:36 +01:00
clawbot deleted branch issue-311-header-limits-first 2026-11-11 07:13:36 +01:00
Sign in to join this conversation.