From 157d52af2f7d5e22b05daa780b83248f0c7b84df Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 20 Oct 2026 15:26:59 +0000 Subject: [PATCH] A ban in bans.json that ends at or before its start is refused, as other malformed entries are (closes #150) A netblock's next ban for a broken limit within SWWAF_LIMIT_BAN_REPEAT_WINDOW lasts three times its last limit or admin ban. A hand-written entry with an expires at or before its start made that length zero or less, so the next limit ban refused nothing. The bans.json checks now refuse such an entry: it stops the start, and an edit holding one is set aside as bans.json.bad while running. README's "State files" lists it with the other refused entries. Model: opus-5-5 --- README.md | 28 +++++++++++++++------------- internal/state/state.go | 9 +++++++-- internal/state/state_test.go | 15 +++++++++++++++ 3 files changed, 37 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index d1cbdf4..4d188b5 100644 --- a/README.md +++ b/README.md @@ -1449,11 +1449,12 @@ whole service, its `asn`, for an AS number, its `name`, for a named netblock, or the `start` of a window in which it has requests or bytes; a list's `url`, `fetched` or `lines`, which is `[]` for an empty list; a verdict's `zone`, `client`, `listed`, which is `false` for a client the zone does not list, or -`fetched`. So does a ban whose `cause` is not `limit`, `attack`, `admin` or -`crowdsec`, alerts waiting for a destination that is not `webhook`, `slack` or -`ntfy`, an anomaly counter whose `scope` is not `client`, `net`, `asn`, `total` -or `watch`, and a copy of a list with a line that would make its fetch fail. An -answer's `asn` or `as_name` left out reads as empty. +`fetched`. So does a ban whose `expires` is not after its `start` or whose +`cause` is not `limit`, `attack`, `admin` or `crowdsec`, alerts waiting for a +destination that is not `webhook`, `slack` or `ntfy`, an anomaly counter whose +`scope` is not `client`, `net`, `asn`, `total` or `watch`, and a copy of a list +with a line that would make its fetch fail. An answer's `asn` or `as_name` left +out reads as empty. While it runs, `smallwebwaf` watches `SWWAF_STATE_DIR` and takes in your edit of a state file as soon as you save it: what the file then holds replaces what @@ -1463,14 +1464,15 @@ writes a file it takes in any edit made since, so your edit is not overwritten; a change `smallwebwaf` made after you opened the file, such as a new ban, is lost when you save over it. An edit that would stop the start, because it does not parse, has another `version`, leaves out a field an entry needs, gives a ban -another `cause`, names another destination, gives an anomaly counter another -`scope` or gives a list's copy a line that would make its fetch fail, does not -stop the running `smallwebwaf`: it keeps what it holds, and at the file's next -write renames your file to `.bad`, such as `bans.json.bad`, writes the -file again from memory, logs the file and where the error is, and raises a -`file_error` alert for it. It waits for that write because an editor's file can -be read before the editor has finished writing it. Mend the `.bad` file and move -it back. A file you remove is written again at its next write. +an `expires` not after its `start` or another `cause`, names another +destination, gives an anomaly counter another `scope` or gives a list's copy a +line that would make its fetch fail, does not stop the running `smallwebwaf`: it +keeps what it holds, and at the file's next write renames your file to +`.bad`, such as `bans.json.bad`, writes the file again from memory, logs +the file and where the error is, and raises a `file_error` alert for it. It +waits for that write because an editor's file can be read before the editor has +finished writing it. Mend the `.bad` file and move it back. A file you remove is +written again at its next write. To ban a netblock, add an entry to `bans.json` with its `netblock`, its `start` and its `expires`, `null` for a ban that never ends; its `reason` and its diff --git a/internal/state/state.go b/internal/state/state.go index b662075..a6321c4 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -61,6 +61,7 @@ var ( // errMissing is for an entry without a field it needs. errMissing = errors.New("has no") errCause = errors.New("is not limit, attack, admin or crowdsec") + errExpires = errors.New("is not after its start") errDestination = errors.New("is not webhook, slack or ntfy") errScope = errors.New("is not client, net, asn, total or watch") errWaitingList = errors.New(`waiting is a list, but now lists the alerts by ` + @@ -646,8 +647,10 @@ func (e BanEntry) ban() bans.Ban { // client, a start, from which the length of the netblock's next ban is // worked out, or an expires, which would make it permanent. A permanent // ban's expires is null, which Bans cannot tell from a missing one, so -// each expires is read again as written. A cause other than limit, -// attack, admin or crowdsec, most likely misspelt, is refused too. +// each expires is read again as written. An expires not after the start +// is refused too, since the netblock's next ban for a broken limit, three +// times as long as this one, would then refuse nothing; and so is a cause +// other than limit, attack, admin or crowdsec, most likely misspelt. func (f *bansFile) check(data []byte) error { var written struct { Bans []struct { @@ -668,6 +671,8 @@ func (f *bansFile) check(data []byte) error { return missing(i, "start") case written.Bans[i].Expires == nil: return missing(i, "expires") + case entry.Expires != nil && !entry.Expires.After(entry.Start): + return fmt.Errorf("entry %d's expires %w", i+1, errExpires) case entry.Cause != "" && entry.Cause != bans.CauseLimit && entry.Cause != bans.CauseAttack && entry.Cause != bans.CauseAdmin && entry.Cause != bans.CauseCrowdSec: diff --git a/internal/state/state_test.go b/internal/state/state_test.go index 0a95f2e..0b98d94 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -865,6 +865,21 @@ func TestBanWithAnotherCauseStopsTheStart(t *testing.T) { `: entry 4's cause "atack" is not limit, attack, admin or crowdsec`) } +func TestBanThatDoesNotEndAfterItsStartStopsTheStart(t *testing.T) { + t.Parallel() + + // The first ban ends a second after its start; the second ends as it + // starts, or an hour before. + for _, expires := range []string{`"2026-10-06T00:00:00Z"`, `"2026-10-05T23:00:00Z"`} { + wantRefused(t, bansJSON, `{"version": 1, "bans": [`+ + `{"netblock": "203.0.113.9/32", "start": "2026-10-06T00:00:00Z", `+ + `"expires": "2026-10-06T00:00:01Z"}, `+ + `{"netblock": "203.0.113.10/32", "start": "2026-10-06T00:00:00Z", `+ + `"expires": `+expires+`}]}`, + `: entry 2's expires is not after its start`) + } +} + func TestUnknownVersionStopsTheStart(t *testing.T) { t.Parallel()