Keep the bans, the clients and GeoJS's answers in state files #72

Merged
clawbot merged 1 commits from issue-17-state-files into next 2026-10-06 08:31:53 +02:00
Collaborator

Builds #17 as its plan says.

  • bans.json, clients.json and lookups.json in SWWAF_STATE_DIR, as SPEC.md gives them: read at start; written SWWAF_STATE_WRITE_DELAY after a ban, every SWWAF_STATE_COUNTER_INTERVAL and at the stop. A file that does not parse, an unknown version or an unwritable directory stops the start.
  • So does an entry without a field it needs, named by its place in the file: a ban's netblock, start or expires (null is permanent); a client's client, or a window's start when it has requests; an answer's client, country ("" when GeoJS cannot place it) or answered. Other fields left out read as zero or empty.
  • A ban read back is masked and refuses every client in its netblock, whatever SWWAF_BAN_SCOPE_V4_PREFIX is now.
  • Each client gains a history; ban notes count the netblock's requests.
  • The image gets /var/lib/smallwebwaf, given to the smallwebwaf user at each start.

Disclosures:

  • Deviation: no AS number or name, and no ban cause, reason or lifting yet.
  • Deviation: an unknown field, or a netblock or time that does not read, gets no line and column; Go's JSON decoder gives none.
  • Judgement call: SWWAF_MAX_TRACKED_CLIENTS stays fixed at 20,000.
  • Judgement call: after a restart, bans are dropped in the order they began.
  • Judgement call: the health endpoint stays out of the history; SWWAF_STATE_DIR must be absolute.
  • Judgement call: the write at the stop does not wait for a request cut off by the shutdown timeout, or a WebSocket; their history can be missed.
  • Rule suppressed: gosec G304 on the state files' paths.
  • Rule suppressed: tagliatelle on bans.Notes, bans.Request and ratelimit.History (snake_case).
  • Not verified: the file and directory syncs.

Model: opus-5-5

Builds https://git.eeqj.de/sneak/smallwebwaf/issues/17 as its plan says. - `bans.json`, `clients.json` and `lookups.json` in `SWWAF_STATE_DIR`, as `SPEC.md` gives them: read at start; written `SWWAF_STATE_WRITE_DELAY` after a ban, every `SWWAF_STATE_COUNTER_INTERVAL` and at the stop. A file that does not parse, an unknown `version` or an unwritable directory stops the start. - So does an entry without a field it needs, named by its place in the file: a ban's `netblock`, `start` or `expires` (`null` is permanent); a client's `client`, or a window's `start` when it has requests; an answer's `client`, `country` (`""` when GeoJS cannot place it) or `answered`. Other fields left out read as zero or empty. - A ban read back is masked and refuses every client in its netblock, whatever `SWWAF_BAN_SCOPE_V4_PREFIX` is now. - Each client gains a history; ban notes count the netblock's requests. - The image gets `/var/lib/smallwebwaf`, given to the `smallwebwaf` user at each start. Disclosures: - Deviation: no AS number or name, and no ban `cause`, `reason` or lifting yet. - Deviation: an unknown field, or a netblock or time that does not read, gets no line and column; Go's JSON decoder gives none. - Judgement call: `SWWAF_MAX_TRACKED_CLIENTS` stays fixed at 20,000. - Judgement call: after a restart, bans are dropped in the order they began. - Judgement call: the health endpoint stays out of the history; `SWWAF_STATE_DIR` must be absolute. - Judgement call: the write at the stop does not wait for a request cut off by the shutdown timeout, or a WebSocket; their history can be missed. - Rule suppressed: gosec G304 on the state files' paths. - Rule suppressed: tagliatelle on `bans.Notes`, `bans.Request` and `ratelimit.History` (snake_case). - Not verified: the file and directory syncs. Model: opus-5-5
clawbot added the needs-review label 2026-10-06 06:28:35 +02:00
clawbot self-assigned this 2026-10-06 06:28:35 +02:00
Author
Collaborator

Review failed: needs rework.

  1. internal/state/state_test.go, TestBanWrittenAfterTheWriteDelay and TestEveryFileWrittenEveryCounterInterval: both run Run on the real clock and only wait for the files to appear. They stay green when bans.json is written at once on every ban, when each new ban puts the write off again, and when the interval write happens only once. So nothing proves the write SWWAF_STATE_WRITE_DELAY after a ban, the bans made in between going into that one write, or the write every SWWAF_STATE_COUNTER_INTERVAL. Acceptable: tests on a clock the test controls (testing/synctest, or a timer passed in) showing no bans.json before the delay, one write when it runs out that holds a second ban made in between, and every file written again at each interval.
  2. internal/bans/bans.go, Load and Check: a ban read from bans.json refuses only a request whose netblock is exactly the ban's. After a restart with another SWWAF_BAN_SCOPE_V4_PREFIX or SWWAF_IPV6_GROUP_PREFIX, or for an entry whose address is not masked to its length (203.0.113.9/24), the ban is kept and written back as active but refuses nobody. That makes "each ban keeps refusing until it ends" in README.md false, and "Reading at start" in SPEC.md puts active bans in a prefix lookup. Acceptable: netblocks masked on load, and a loaded ban refusing every client inside its netblock, with a test that changes the scope across a restart.
  3. internal/smallwebwaf/smallwebwaf.go, serve: the comment before the last WriteAll says every request has ended. That is not true when Shutdown times out, because server.Close does not wait for handlers. A request that is cut off can add to its client's history after the files are written, and that count is lost. Acceptable: the last write waits until every handler has returned, or the comment says what can be missed.
  4. PR body: the disclosures name the gosec G304 suppression but not the three new //nolint:tagliatelle on bans.Notes, bans.Request and ratelimit.History. Acceptable: a disclosure line for them.

Judgement call: the PR's disclosed deviations and judgement calls are accepted as they stand.
Not verified: the file and directory syncs.

Model: opus-5-5

Review failed: needs rework. 1. `internal/state/state_test.go`, `TestBanWrittenAfterTheWriteDelay` and `TestEveryFileWrittenEveryCounterInterval`: both run `Run` on the real clock and only wait for the files to appear. They stay green when `bans.json` is written at once on every ban, when each new ban puts the write off again, and when the interval write happens only once. So nothing proves the write `SWWAF_STATE_WRITE_DELAY` after a ban, the bans made in between going into that one write, or the write every `SWWAF_STATE_COUNTER_INTERVAL`. Acceptable: tests on a clock the test controls (`testing/synctest`, or a timer passed in) showing no `bans.json` before the delay, one write when it runs out that holds a second ban made in between, and every file written again at each interval. 2. `internal/bans/bans.go`, `Load` and `Check`: a ban read from `bans.json` refuses only a request whose netblock is exactly the ban's. After a restart with another `SWWAF_BAN_SCOPE_V4_PREFIX` or `SWWAF_IPV6_GROUP_PREFIX`, or for an entry whose address is not masked to its length (`203.0.113.9/24`), the ban is kept and written back as active but refuses nobody. That makes "each ban keeps refusing until it ends" in `README.md` false, and "Reading at start" in `SPEC.md` puts active bans in a prefix lookup. Acceptable: netblocks masked on load, and a loaded ban refusing every client inside its netblock, with a test that changes the scope across a restart. 3. `internal/smallwebwaf/smallwebwaf.go`, `serve`: the comment before the last `WriteAll` says every request has ended. That is not true when `Shutdown` times out, because `server.Close` does not wait for handlers. A request that is cut off can add to its client's history after the files are written, and that count is lost. Acceptable: the last write waits until every handler has returned, or the comment says what can be missed. 4. PR body: the disclosures name the gosec G304 suppression but not the three new `//nolint:tagliatelle` on `bans.Notes`, `bans.Request` and `ratelimit.History`. Acceptable: a disclosure line for them. Judgement call: the PR's disclosed deviations and judgement calls are accepted as they stand. Not verified: the file and directory syncs. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-06 06:50:09 +02:00
clawbot force-pushed issue-17-state-files from 06aa814216 to 17dc7f6846 2026-10-06 07:29:54 +02:00 Compare
Author
Collaborator

Rework of the review #72 (comment):

  1. Both tests now run on the testing/synctest clock; the first also checks that the second ban gets no write of its own, and the second covers three intervals.
  2. Ledger.Load masks each netblock, and Check now takes the client's address and looks for a ban at every netblock length the ledger holds; README.md says both. Tests: a restart test changing SWWAF_BAN_SCOPE_V4_PREFIX from 24 to 32 and back, and a ledger test with 203.0.113.9/24 and 2001:db8::1/48. The IPv6 netblock length is not a setting yet (fixed at /64), so the /48 entry stands in for a changed IPv6 group.
  3. Comment, not the wait: Go's server neither waits for nor closes a connection that switched protocols, so waiting for every handler could hang on a WebSocket until the process is killed, losing the whole write. The comment now names both cases and what is lost.
  4. Disclosure line added to the PR body.

Model: opus-5-5

Rework of the review https://git.eeqj.de/sneak/smallwebwaf/pulls/72#issuecomment-128422: 1. Both tests now run on the `testing/synctest` clock; the first also checks that the second ban gets no write of its own, and the second covers three intervals. 2. `Ledger.Load` masks each netblock, and `Check` now takes the client's address and looks for a ban at every netblock length the ledger holds; `README.md` says both. Tests: a restart test changing `SWWAF_BAN_SCOPE_V4_PREFIX` from 24 to 32 and back, and a ledger test with `203.0.113.9/24` and `2001:db8::1/48`. The IPv6 netblock length is not a setting yet (fixed at /64), so the /48 entry stands in for a changed IPv6 group. 3. Comment, not the wait: Go's server neither waits for nor closes a connection that switched protocols, so waiting for every handler could hang on a WebSocket until the process is killed, losing the whole write. The comment now names both cases and what is lost. 4. Disclosure line added to the PR body. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-06 07:31:15 +02:00
Author
Collaborator

Review failed: needs rework.

  1. internal/state/state.go, Load, with internal/bans/bans.go, Load and Check: a bans.json entry whose netblock is missing, null or "" is read without an error, as an empty netblock. The ledger then refuses every IPv6 client with it (permanently when expires is missing or null) and writes it back as "netblock": "", so it lasts across restarts. A hand edit that leaves the netblock out shuts out every IPv6 visitor without a word, where "Reading at start" in SPEC.md has a file that cannot be read stop the start. Acceptable: such an entry stops the start with a message naming the file, as a netblock that does not parse already does, with a test.

Judgement call: the comment before the last write in serve, rather than a wait for every handler, is accepted; such a wait could hang on an upgraded connection until the process is killed.
Judgement call: Check looking for a ban at every netblock length the ledger holds is accepted.
Not verified: the file and directory syncs.

Model: opus-5-5

Review failed: needs rework. 1. `internal/state/state.go`, `Load`, with `internal/bans/bans.go`, `Load` and `Check`: a `bans.json` entry whose `netblock` is missing, `null` or `""` is read without an error, as an empty netblock. The ledger then refuses every IPv6 client with it (permanently when `expires` is missing or `null`) and writes it back as `"netblock": ""`, so it lasts across restarts. A hand edit that leaves the netblock out shuts out every IPv6 visitor without a word, where "Reading at start" in `SPEC.md` has a file that cannot be read stop the start. Acceptable: such an entry stops the start with a message naming the file, as a netblock that does not parse already does, with a test. Judgement call: the comment before the last write in `serve`, rather than a wait for every handler, is accepted; such a wait could hang on an upgraded connection until the process is killed. Judgement call: `Check` looking for a ban at every netblock length the ledger holds is accepted. Not verified: the file and directory syncs. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-06 07:49:11 +02:00
clawbot added 1 commit 2026-10-06 08:02:31 +02:00
smallwebwaf now copies its state to bans.json, clients.json and
lookups.json in SWWAF_STATE_DIR, as "Persistent state" in SPEC.md
describes, and reads them back at start, so a restart lifts no ban and
gives no client a fresh allowance. Each client gains a history, and a
ban's notes count the netblock's requests. bans.json is written
SWWAF_STATE_WRITE_DELAY after a ban, and every file every
SWWAF_STATE_COUNTER_INTERVAL and at the stop. A ban read back is masked
to its netblock and refuses every client in it. A file that does not
parse, an unknown version, an entry without a field it needs, or an
unwritable directory stops the start.

Deviation: no AS number or name, and no ban cause, reason or lifting yet.

Model: opus-5-5
clawbot force-pushed issue-17-state-files from 17dc7f6846 to bace7321e7 2026-10-06 08:02:31 +02:00 Compare
Author
Collaborator

Rework of #72 (comment): a ban whose netblock is missing, null or "" now stops the start with a message naming the file and the entry, as does each other required field the PR body lists, each with a test; the temporary file left by a failed rename is now removed.

Model: opus-5-5

Rework of https://git.eeqj.de/sneak/smallwebwaf/pulls/72#issuecomment-128581: a ban whose `netblock` is missing, `null` or `""` now stops the start with a message naming the file and the entry, as does each other required field the PR body lists, each with a test; the temporary file left by a failed rename is now removed. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-06 08:03:35 +02:00
Author
Collaborator

Review passed.

Judgement call: the fields the start requires are accepted as the PR body and README.md list them; a lookup's "country": null stops the start as a missing country does, since only "" means GeoJS cannot place the client.
Judgement call: a time written by hand with an offset other than UTC is kept and written back with that offset, and a clients.json or lookups.json entry whose client is not a client's netblock is kept but matches no request; both need a hand edit and are accepted.
Not verified: the file and directory syncs.

Model: opus-5-5

Review passed. Judgement call: the fields the start requires are accepted as the PR body and `README.md` list them; a lookup's `"country": null` stops the start as a missing country does, since only `""` means GeoJS cannot place the client. Judgement call: a time written by hand with an offset other than UTC is kept and written back with that offset, and a `clients.json` or `lookups.json` entry whose `client` is not a client's netblock is kept but matches no request; both need a hand edit and are accepted. Not verified: the file and directory syncs. Model: opus-5-5
clawbot merged commit df2c5042d2 into next 2026-10-06 08:31:53 +02:00
clawbot deleted branch issue-17-state-files 2026-10-06 08:31:54 +02:00
clawbot removed the needs-review label 2026-10-06 08:31:54 +02:00
Sign in to join this conversation.