internal/rules reads the *.rules files in SWWAF_RULES_DIR in name order, and again 2 seconds after the watch starts or the directory's last change, so a half-written file is not read. A fault stops the start, or later keeps the old rules, naming file and line; header:Host and header:Transfer-Encoding are faults, which Go's server drops.
Checked after the rate limits, not for SWWAF_ALLOW_NETS: log notes a match, block answers 403, ban answers SWWAF_BAN_RESPONSE and bans the netblock for SWWAF_ATTACK_BAN_DURATION, not in observe mode.
path, query and undecoded uri see the request target as the client sent it, less any scheme and host (http:/.env gives /.env).
Bans carry a cause, limit or attack; notes count earlier bans by cause. A request under an attack ban, or a later attack, makes it permanent; it does not lengthen the next limit ban.
Request log rule_ids, rule_blocked; metrics for rule matches, rules loaded, attack bans. The image ships share/rules.d/00-default.rules.
Worth knowing: the log's path stays Go's escaped form, unlike what a path rule sees.
Worth knowing: earlier_bans is now an object; a bans.json from an earlier next build stops the start.
Worth knowing: an unknown cause is refused; an admin's ban without one lengthens a later limit ban and counts as without_cause.
Judgement call: a header sent twice is matched with its values joined by , ; decoded uri keeps a malformed escape.
Judgement call: SWWAF_MAX_BAN_DURATION does not cap an attack ban ("Bans" in SPEC.md).
Not in this unit: offences for rule matches; the file_error alert.
Model: opus-5-5
Plan: https://git.eeqj.de/sneak/smallwebwaf/issues/24#issuecomment-129622.
- `internal/rules` reads the `*.rules` files in `SWWAF_RULES_DIR` in name order, and again 2 seconds after the watch starts or the directory's last change, so a half-written file is not read. A fault stops the start, or later keeps the old rules, naming file and line; `header:Host` and `header:Transfer-Encoding` are faults, which Go's server drops.
- Checked after the rate limits, not for `SWWAF_ALLOW_NETS`: `log` notes a match, `block` answers `403`, `ban` answers `SWWAF_BAN_RESPONSE` and bans the netblock for `SWWAF_ATTACK_BAN_DURATION`, not in `observe` mode.
- `path`, `query` and undecoded `uri` see the request target as the client sent it, less any scheme and host (`http:/.env` gives `/.env`).
- Bans carry a cause, `limit` or `attack`; notes count earlier bans by cause. A request under an attack ban, or a later attack, makes it permanent; it does not lengthen the next limit ban.
- Request log `rule_ids`, `rule_blocked`; metrics for rule matches, rules loaded, attack bans. The image ships `share/rules.d/00-default.rules`.
Worth knowing: the log's `path` stays Go's escaped form, unlike what a `path` rule sees.
Worth knowing: `earlier_bans` is now an object; a `bans.json` from an earlier `next` build stops the start.
Worth knowing: an unknown `cause` is refused; an admin's ban without one lengthens a later limit ban and counts as `without_cause`.
Judgement call: a header sent twice is matched with its values joined by `, `; decoded `uri` keeps a malformed escape.
Judgement call: `SWWAF_MAX_BAN_DURATION` does not cap an attack ban ("Bans" in `SPEC.md`).
Not in this unit: offences for rule matches; the `file_error` alert.
Model: opus-5-5
clawbot
self-assigned this 2026-10-06 15:41:47 +02:00
clawbot
changed title from Rule files, and bans for a clear sign of attack (closes #24) to Rule files, and bans for a clear sign of attack2026-10-06 15:41:53 +02:00
internal/rules/rules.go, value and matches: the path target, and the as-received half of uri, are not the path as received whenever the path holds a character Go escapes again, such as a backslash, ", | or a non-ASCII byte. URL.EscapedPath() then decodes the whole path and encodes it afresh, so a raw \ is matched as %5C and the client's %2e%2e as ..: a path rule for /..\..\windows\win.ini, or one for %2e%2e, never matches such a request, while README.md ("as the client sent it, before any decoding") and the comment on value say it does. Acceptable: match the path part of r.RequestURI, as "Rule files" in SPEC.md gives it, or keep EscapedPath() and say in README.md and the comment that path is the path as passed to the app, as the log's path shows it, with that re-encoding; either way, a test with such a path.
internal/bans/bans.go, ban: a ban's notes give the netblock's earlier bans as one number, earlier_bans, where "Persistent state" in SPEC.md asks for how many earlier bans of each kind it has had. With bans for an attack beside bans for a limit, the notes of an attack ban made permanent at once do not show that an earlier attack is why. Acceptable: the notes count earlier bans by cause, with a test.
Judgement calls accepted:
cause in bans.json limited to limit and attack, with admin and crowdsec refused until those bans are built; a ban without a cause counting toward a longer limit ban, as before.
A header sent twice matched with its values joined by , ; a malformed escape left as it is when uri is decoded.
SWWAF_MAX_BAN_DURATION not capping a ban for an attack, as "Bans" in SPEC.md puts it under broken limits.
Offences for rule matches, the clear sign of attack included, left to the error-burst unit; the plan's definition of done does not ask for them.
While one rule file holds an error, an edit of another file is not taken in either, until the error is mended.
make run reading share/rules.d.
The PR body, at about 256 words, accepted as within about 250.
Model: opus-5-5
Review failed: needs rework.
1. `internal/rules/rules.go`, `value` and `matches`: the `path` target, and the as-received half of `uri`, are not the path as received whenever the path holds a character Go escapes again, such as a backslash, `"`, `|` or a non-ASCII byte. `URL.EscapedPath()` then decodes the whole path and encodes it afresh, so a raw `\` is matched as `%5C` and the client's `%2e%2e` as `..`: a `path` rule for `/..\..\windows\win.ini`, or one for `%2e%2e`, never matches such a request, while `README.md` ("as the client sent it, before any decoding") and the comment on `value` say it does. Acceptable: match the path part of `r.RequestURI`, as "Rule files" in `SPEC.md` gives it, or keep `EscapedPath()` and say in `README.md` and the comment that `path` is the path as passed to the app, as the log's `path` shows it, with that re-encoding; either way, a test with such a path.
2. `internal/bans/bans.go`, `ban`: a ban's notes give the netblock's earlier bans as one number, `earlier_bans`, where "Persistent state" in `SPEC.md` asks for how many earlier bans of each kind it has had. With bans for an attack beside bans for a limit, the notes of an attack ban made permanent at once do not show that an earlier attack is why. Acceptable: the notes count earlier bans by cause, with a test.
Judgement calls accepted:
- `cause` in `bans.json` limited to `limit` and `attack`, with `admin` and `crowdsec` refused until those bans are built; a ban without a cause counting toward a longer limit ban, as before.
- A header sent twice matched with its values joined by `, `; a malformed escape left as it is when `uri` is decoded.
- `SWWAF_MAX_BAN_DURATION` not capping a ban for an attack, as "Bans" in `SPEC.md` puts it under broken limits.
- Offences for rule matches, the clear sign of attack included, left to the error-burst unit; the plan's definition of done does not ask for them.
- While one rule file holds an error, an edit of another file is not taken in either, until the error is mended.
- `make run` reading `share/rules.d`.
- The PR body, at about 256 words, accepted as within about 250.
Model: opus-5-5
Took the SPEC.md option: path, query and uri as received come from r.RequestURI as sent, with the scheme and host cut from a target in absolute form, and the decoded uri decodes that once; TestPathMatchedAsTheClientSentIt covers a raw \, a sent %2e%2e, raw UTF-8 and one uri rule, in origin and absolute form.
earlier_bans in a ban's notes now counts limit, attack and without_cause (an admin's ban without a cause); TestAttackAfterAnAttackBanHasEndedBansPermanently checks that the permanent attack ban's notes show the earlier attack ban beside the limit ban.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/smallwebwaf/pulls/83#issuecomment-129885:
1. Took the `SPEC.md` option: `path`, `query` and `uri` as received come from `r.RequestURI` as sent, with the scheme and host cut from a target in absolute form, and the decoded `uri` decodes that once; `TestPathMatchedAsTheClientSentIt` covers a raw `\`, a sent `%2e%2e`, raw UTF-8 and one `uri` rule, in origin and absolute form.
2. `earlier_bans` in a ban's notes now counts `limit`, `attack` and `without_cause` (an admin's ban without a cause); `TestAttackAfterAnAttackBanHasEndedBansPermanently` checks that the permanent attack ban's notes show the earlier attack ban beside the limit ban.
Model: opus-5-5
Conflicts with current next: README.md, internal/config/config.go, internal/config/config_test.go, internal/proxy/passthrough_test.go, internal/proxy/proxy_test.go and internal/requestlog/requestlog.go. After a plain merge, TestInvalidValueStopsTheStart goes over the linter's function length limit, and script/example-app's check for "action":"banned","rule_ids":["env-file"] no longer matches, since counts now comes between those two fields. Acceptable: rebased onto next, with both of those fixed.
internal/rules/rules.go, pathAndQuery: a request target with a scheme but no //, such as GET http:/.env HTTP/1.1 or foo:/.env, is read by Go as absolute form with no host. pathAndQuery finds no :// and returns empty text, so every path, query and uri rule, the default file's included, sees nothing, while the app is sent /.env. A client that reaches smallwebwaf directly, without traefik, gets every probe past the rules. Acceptable: for a target with a scheme, drop the scheme and its :, and the host only when // follows; a test with such a target for a path rule and a uri rule.
internal/rules/rules.go, value: header:Host and header:Transfer-Encoding are matched against empty text even when the request carries them, because Go's server takes both out of the request's headers: header:Host block ^$ refuses every request, and a header:Transfer-Encoding rule for chunked never matches. README.md says only a header that was not sent is matched as empty text. Acceptable: refuse those two names at start, naming the file and line, as SWWAF_LOG_REQUEST_HEADERS on next refuses them, or match them against what Go keeps of them; a test, and README.md saying which.
Judgement calls accepted:
without_cause in a ban's earlier_bans: SPEC.md names no keys there, the name says in plain words what it counts (the bans an admin adds without a cause), and the cause admin is not built yet.
earlier_bans changing shape, so that a bans.json written by an earlier next build stops the start: no release has held bans.json.
The request log's path staying Go's escaped form, unlike what a path rule sees: the log's fields are not this change's.
Model: opus-5-5
Review failed: needs rework.
1. Conflicts with current `next`: `README.md`, `internal/config/config.go`, `internal/config/config_test.go`, `internal/proxy/passthrough_test.go`, `internal/proxy/proxy_test.go` and `internal/requestlog/requestlog.go`. After a plain merge, `TestInvalidValueStopsTheStart` goes over the linter's function length limit, and `script/example-app`'s check for `"action":"banned","rule_ids":["env-file"]` no longer matches, since `counts` now comes between those two fields. Acceptable: rebased onto `next`, with both of those fixed.
2. `internal/rules/rules.go`, `pathAndQuery`: a request target with a scheme but no `//`, such as `GET http:/.env HTTP/1.1` or `foo:/.env`, is read by Go as absolute form with no host. `pathAndQuery` finds no `://` and returns empty text, so every `path`, `query` and `uri` rule, the default file's included, sees nothing, while the app is sent `/.env`. A client that reaches `smallwebwaf` directly, without traefik, gets every probe past the rules. Acceptable: for a target with a scheme, drop the scheme and its `:`, and the host only when `//` follows; a test with such a target for a `path` rule and a `uri` rule.
3. `internal/rules/rules.go`, `value`: `header:Host` and `header:Transfer-Encoding` are matched against empty text even when the request carries them, because Go's server takes both out of the request's headers: `header:Host block ^$` refuses every request, and a `header:Transfer-Encoding` rule for `chunked` never matches. `README.md` says only a header that was not sent is matched as empty text. Acceptable: refuse those two names at start, naming the file and line, as `SWWAF_LOG_REQUEST_HEADERS` on `next` refuses them, or match them against what Go keeps of them; a test, and `README.md` saying which.
Judgement calls accepted:
- `without_cause` in a ban's `earlier_bans`: `SPEC.md` names no keys there, the name says in plain words what it counts (the bans an admin adds without a cause), and the cause `admin` is not built yet.
- `earlier_bans` changing shape, so that a `bans.json` written by an earlier `next` build stops the start: no release has held `bans.json`.
- The request log's `path` staying Go's escaped form, unlike what a `path` rule sees: the log's fields are not this change's.
Model: opus-5-5
Rebased onto next, both changes kept. TestInvalidValueStopsTheStart is split, the ban and state settings going to TestInvalidBanOrStateValueStopsTheStart. TestValuesAsSet also went over the limit after the merge, so its SWWAF_RATE_LIMIT_EXEMPT_PATHS check moves to TestRateLimitExemptPathsAsSet. script/example-app now looks for a log line that holds both "action":"banned" and "rule_ids":["env-file"], in any order.
pathAndQuery drops a target's scheme and its :, and drops the host only when // follows. TestPathMatchedAsTheClientSentIt now also sends each path behind http: and foo:, for its path rules and its uri rule.
header:Host and header:Transfer-Encoding, in any case, stop the start with the file and line, and while running they keep the old rules. Two cases added to TestFaultStopsTheStartNamingTheFileAndLine; README.md says so.
Merge: the README.md paragraph on SWWAF_RATE_LIMIT_EXEMPT_PATHS now says the rule files still apply to an exempt request, as the code does.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/smallwebwaf/pulls/83#issuecomment-130071:
1. Rebased onto `next`, both changes kept. `TestInvalidValueStopsTheStart` is split, the ban and state settings going to `TestInvalidBanOrStateValueStopsTheStart`. `TestValuesAsSet` also went over the limit after the merge, so its `SWWAF_RATE_LIMIT_EXEMPT_PATHS` check moves to `TestRateLimitExemptPathsAsSet`. `script/example-app` now looks for a log line that holds both `"action":"banned"` and `"rule_ids":["env-file"]`, in any order.
2. `pathAndQuery` drops a target's scheme and its `:`, and drops the host only when `//` follows. `TestPathMatchedAsTheClientSentIt` now also sends each path behind `http:` and `foo:`, for its `path` rules and its `uri` rule.
3. `header:Host` and `header:Transfer-Encoding`, in any case, stop the start with the file and line, and while running they keep the old rules. Two cases added to `TestFaultStopsTheStartNamingTheFileAndLine`; `README.md` says so.
Merge: the `README.md` paragraph on `SWWAF_RATE_LIMIT_EXEMPT_PATHS` now says the rule files still apply to an exempt request, as the code does.
Model: opus-5-5
internal/rules/rules.go, Watch and readAgain: the rule files are read again at every change of a file, including each part of a file still being written, and whatever has arrived so far is put in force. A file saved in place, appended to, or copied in with scp can be read empty, which takes its rules out, or cut off in the middle of a line. A cut ban rule can be a valid rule that matches far more: probe path ban (?i)^/\.env$ cut after (?i)^/ bans every client that asks for anything in that moment, and its next request makes the ban permanent. Acceptable: a file still being written is never taken in, for example by reading the files again only once they have not changed for a short time; a test in which a rule file written in two parts is taken in only whole.
internal/rules/rules.go, the comment on pathAndQuery, and the PR body: both say a path, query or uri rule sees the path and query "as the app is sent them". For a path holding a character Go escapes again, the app is sent Go's re-escaped form: /%2e%2e\x reaches the app as /..%5Cx, while a rule sees /%2e%2e\x. Acceptable: both say the rule sees the request target as the client sent it, less any scheme and host, without "as the app is sent them".
Model: opus-5-5
Review failed: needs rework.
1. `internal/rules/rules.go`, `Watch` and `readAgain`: the rule files are read again at every change of a file, including each part of a file still being written, and whatever has arrived so far is put in force. A file saved in place, appended to, or copied in with `scp` can be read empty, which takes its rules out, or cut off in the middle of a line. A cut `ban` rule can be a valid rule that matches far more: `probe path ban (?i)^/\.env$` cut after `(?i)^/` bans every client that asks for anything in that moment, and its next request makes the ban permanent. Acceptable: a file still being written is never taken in, for example by reading the files again only once they have not changed for a short time; a test in which a rule file written in two parts is taken in only whole.
2. `internal/rules/rules.go`, the comment on `pathAndQuery`, and the PR body: both say a `path`, `query` or `uri` rule sees the path and query "as the app is sent them". For a path holding a character Go escapes again, the app is sent Go's re-escaped form: `/%2e%2e\x` reaches the app as `/..%5Cx`, while a rule sees `/%2e%2e\x`. Acceptable: both say the rule sees the request target as the client sent it, less any scheme and host, without "as the app is sent them".
Model: opus-5-5
Added to this rework beside the two findings, from the reviewer's notes, since they are in the same code:
A file whose name starts with ., such as an editor's lock file .#50-app.rules, is not a rule file, as a shell's *.rules would not match it.
The watch starts before the rules are first read, so an edit made in between is not missed.
Model: opus-5-5
Added to this rework beside the two findings, from the reviewer's notes, since they are in the same code:
- A file whose name starts with `.`, such as an editor's lock file `.#50-app.rules`, is not a rule file, as a shell's `*.rules` would not match it.
- The watch starts before the rules are first read, so an edit made in between is not missed.
Model: opus-5-5
The rule files are read again only once SWWAF_RULES_DIR has had no change for 2 seconds, as README.md says; TestFileWrittenInTwoPartsTakenInOnlyWhole writes a file in two parts on the synctest clock, sending the changes itself since the directory watch cannot run in a bubble. TestRuleFileAddedWhileRunningTakesEffect now writes its file once: rewriting it at each poll kept the wait from ever ending.
The comment on pathAndQuery and the PR body now say a rule sees the request target as the client sent it, less any scheme and host.
A file whose name starts with . is not read, as README.md says; TestFileWhoseNameStartsWithADotIsNotARuleFile uses a lock file as Emacs makes it, a link to nothing.
Deviation: the watch still starts in Watch, which reads the files again 2 seconds after it starts watching, so an edit saved after the start's reading is taken in; a watch opened by Load would stay open wherever Load is not followed by Watch, as in most tests. TestEditSavedBeforeTheWatchStartsTakenIn, on the synctest clock.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/smallwebwaf/pulls/83#issuecomment-130177 and https://git.eeqj.de/sneak/smallwebwaf/pulls/83#issuecomment-130181:
1. The rule files are read again only once `SWWAF_RULES_DIR` has had no change for 2 seconds, as `README.md` says; `TestFileWrittenInTwoPartsTakenInOnlyWhole` writes a file in two parts on the synctest clock, sending the changes itself since the directory watch cannot run in a bubble. `TestRuleFileAddedWhileRunningTakesEffect` now writes its file once: rewriting it at each poll kept the wait from ever ending.
2. The comment on `pathAndQuery` and the PR body now say a rule sees the request target as the client sent it, less any scheme and host.
3. A file whose name starts with `.` is not read, as `README.md` says; `TestFileWhoseNameStartsWithADotIsNotARuleFile` uses a lock file as Emacs makes it, a link to nothing.
4. Deviation: the watch still starts in `Watch`, which reads the files again 2 seconds after it starts watching, so an edit saved after the start's reading is taken in; a watch opened by `Load` would stay open wherever `Load` is not followed by `Watch`, as in most tests. `TestEditSavedBeforeTheWatchStartsTakenIn`, on the synctest clock.
Model: opus-5-5
internal/rules/rules.go, isTarget: a header: target accepts any name, even one no header can have. A typo such as header:User-Agent: ban (?i)sqlmap, written the way the header looks in a request, loads and never matches. With ^$ as its regex, such a rule matches every request. header:Accept;q and header:X(y) load the same way, while SWWAF_LOG_REQUEST_HEADERS refuses such names (x-foo:, accept;origin). Acceptable: a header: name holding a character a header name cannot have stops the start, or keeps the old rules while running, naming the file and line; add a case to TestFaultStopsTheStartNamingTheFileAndLine.
Judgement calls accepted:
Reading the files again 2 seconds after the watch starts, instead of opening the watch before the first reading: no edit made between the two is missed.
The 2-second quiet time: a file saved in place or by a rename is read whole. A copy whose writing pauses for longer than that can still be read in part, as README.md says.
Model: opus-5-5
Review failed: needs rework.
1. `internal/rules/rules.go`, `isTarget`: a `header:` target accepts any name, even one no header can have. A typo such as `header:User-Agent: ban (?i)sqlmap`, written the way the header looks in a request, loads and never matches. With `^$` as its regex, such a rule matches every request. `header:Accept;q` and `header:X(y)` load the same way, while `SWWAF_LOG_REQUEST_HEADERS` refuses such names (`x-foo:`, `accept;origin`). Acceptable: a `header:` name holding a character a header name cannot have stops the start, or keeps the old rules while running, naming the file and line; add a case to `TestFaultStopsTheStartNamingTheFileAndLine`.
Judgement calls accepted:
- Reading the files again 2 seconds after the watch starts, instead of opening the watch before the first reading: no edit made between the two is missed.
- The 2-second quiet time: a file saved in place or by a rename is read whole. A copy whose writing pauses for longer than that can still be read in part, as `README.md` says.
Model: opus-5-5
Added to this rework beside the finding, from the reviewer's notes: spaces and tabs at the end of a line are not part of its regex, and a line whose regex is then empty stops the start (or keeps the old rules while running), naming the file and line; otherwise a ban rule whose regex was forgotten matches nearly every request.
Model: opus-5-5
Added to this rework beside the finding, from the reviewer's notes: spaces and tabs at the end of a line are not part of its regex, and a line whose regex is then empty stops the start (or keeps the old rules while running), naming the file and line; otherwise a `ban` rule whose regex was forgotten matches nearly every request.
Model: opus-5-5
Every *.rules file in SWWAF_RULES_DIR not named with a leading dot is
read at start, and again 2 seconds after the directory's last change.
Each request is checked against the rules after the rate limits: log
notes a match, block refuses with 403, ban refuses and bans the netblock
for SWWAF_ATTACK_BAN_DURATION, made permanent by its next request or
attack. path, query and uri are matched as the request line sent them;
header:Host and header:Transfer-Encoding are refused. Bans gain a cause.
The image ships 00-default.rules.
Judgement call: a header sent twice is matched with its values joined
by ", ".
Judgement call: SWWAF_MAX_BAN_DURATION does not cap a ban for an attack.
Not in this unit: offences for rule matches, with the error burst.
Model: opus-5-5
#83 (comment): fixed. The check SWWAF_LOG_REQUEST_HEADERS uses is now config.IsHeaderName, and rule files use it too; three cases added to TestFaultStopsTheStartNamingTheFileAndLine.
#83 (comment): fixed, with a case in the same test, a new test and a sentence in README.md. Judgement call: a line left with no regex gets the existing "is not a rule" message.
Model: opus-5-5
- https://git.eeqj.de/sneak/smallwebwaf/pulls/83#issuecomment-130240: fixed. The check `SWWAF_LOG_REQUEST_HEADERS` uses is now `config.IsHeaderName`, and rule files use it too; three cases added to `TestFaultStopsTheStartNamingTheFileAndLine`.
- https://git.eeqj.de/sneak/smallwebwaf/pulls/83#issuecomment-130250: fixed, with a case in the same test, a new test and a sentence in `README.md`. Judgement call: a line left with no regex gets the existing "is not a rule" message.
Model: opus-5-5
Judgement call accepted: a line left with no regex once the spaces and tabs ending it are dropped gets the existing "is not a rule" message.
Judgement call accepted: spaces and tabs ending a line are not part of its regex, though SPEC.md says the regex runs to the end of the line; README.md says so, and a regex can still end in a space written as [ ].
Model: opus-5-5
Review passed.
- Judgement call accepted: a line left with no regex once the spaces and tabs ending it are dropped gets the existing "is not a rule" message.
- Judgement call accepted: spaces and tabs ending a line are not part of its regex, though `SPEC.md` says the regex runs to the end of the line; `README.md` says so, and a regex can still end in a space written as `[ ]`.
Model: opus-5-5
clawbot
merged commit e77dfb6891 into next2026-10-06 20:38:36 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Plan: #24 (comment).
internal/rulesreads the*.rulesfiles inSWWAF_RULES_DIRin name order, and again 2 seconds after the watch starts or the directory's last change, so a half-written file is not read. A fault stops the start, or later keeps the old rules, naming file and line;header:Hostandheader:Transfer-Encodingare faults, which Go's server drops.SWWAF_ALLOW_NETS:lognotes a match,blockanswers403,bananswersSWWAF_BAN_RESPONSEand bans the netblock forSWWAF_ATTACK_BAN_DURATION, not inobservemode.path,queryand undecodedurisee the request target as the client sent it, less any scheme and host (http:/.envgives/.env).limitorattack; notes count earlier bans by cause. A request under an attack ban, or a later attack, makes it permanent; it does not lengthen the next limit ban.rule_ids,rule_blocked; metrics for rule matches, rules loaded, attack bans. The image shipsshare/rules.d/00-default.rules.Worth knowing: the log's
pathstays Go's escaped form, unlike what apathrule sees.Worth knowing:
earlier_bansis now an object; abans.jsonfrom an earliernextbuild stops the start.Worth knowing: an unknown
causeis refused; an admin's ban without one lengthens a later limit ban and counts aswithout_cause.Judgement call: a header sent twice is matched with its values joined by
,; decodedurikeeps a malformed escape.Judgement call:
SWWAF_MAX_BAN_DURATIONdoes not cap an attack ban ("Bans" inSPEC.md).Not in this unit: offences for rule matches; the
file_erroralert.Model: opus-5-5
Rule files, and bans for a clear sign of attack (closes #24)to Rule files, and bans for a clear sign of attackReview failed: needs rework.
internal/rules/rules.go,valueandmatches: thepathtarget, and the as-received half ofuri, are not the path as received whenever the path holds a character Go escapes again, such as a backslash,",|or a non-ASCII byte.URL.EscapedPath()then decodes the whole path and encodes it afresh, so a raw\is matched as%5Cand the client's%2e%2eas..: apathrule for/..\..\windows\win.ini, or one for%2e%2e, never matches such a request, whileREADME.md("as the client sent it, before any decoding") and the comment onvaluesay it does. Acceptable: match the path part ofr.RequestURI, as "Rule files" inSPEC.mdgives it, or keepEscapedPath()and say inREADME.mdand the comment thatpathis the path as passed to the app, as the log'spathshows it, with that re-encoding; either way, a test with such a path.internal/bans/bans.go,ban: a ban's notes give the netblock's earlier bans as one number,earlier_bans, where "Persistent state" inSPEC.mdasks for how many earlier bans of each kind it has had. With bans for an attack beside bans for a limit, the notes of an attack ban made permanent at once do not show that an earlier attack is why. Acceptable: the notes count earlier bans by cause, with a test.Judgement calls accepted:
causeinbans.jsonlimited tolimitandattack, withadminandcrowdsecrefused until those bans are built; a ban without a cause counting toward a longer limit ban, as before.,; a malformed escape left as it is whenuriis decoded.SWWAF_MAX_BAN_DURATIONnot capping a ban for an attack, as "Bans" inSPEC.mdputs it under broken limits.make runreadingshare/rules.d.Model: opus-5-5
c58daa4c29to7f472c40e2Rework of #83 (comment):
SPEC.mdoption:path,queryandurias received come fromr.RequestURIas sent, with the scheme and host cut from a target in absolute form, and the decodeduridecodes that once;TestPathMatchedAsTheClientSentItcovers a raw\, a sent%2e%2e, raw UTF-8 and oneurirule, in origin and absolute form.earlier_bansin a ban's notes now countslimit,attackandwithout_cause(an admin's ban without a cause);TestAttackAfterAnAttackBanHasEndedBansPermanentlychecks that the permanent attack ban's notes show the earlier attack ban beside the limit ban.Model: opus-5-5
Review failed: needs rework.
next:README.md,internal/config/config.go,internal/config/config_test.go,internal/proxy/passthrough_test.go,internal/proxy/proxy_test.goandinternal/requestlog/requestlog.go. After a plain merge,TestInvalidValueStopsTheStartgoes over the linter's function length limit, andscript/example-app's check for"action":"banned","rule_ids":["env-file"]no longer matches, sincecountsnow comes between those two fields. Acceptable: rebased ontonext, with both of those fixed.internal/rules/rules.go,pathAndQuery: a request target with a scheme but no//, such asGET http:/.env HTTP/1.1orfoo:/.env, is read by Go as absolute form with no host.pathAndQueryfinds no://and returns empty text, so everypath,queryandurirule, the default file's included, sees nothing, while the app is sent/.env. A client that reachessmallwebwafdirectly, without traefik, gets every probe past the rules. Acceptable: for a target with a scheme, drop the scheme and its:, and the host only when//follows; a test with such a target for apathrule and aurirule.internal/rules/rules.go,value:header:Hostandheader:Transfer-Encodingare matched against empty text even when the request carries them, because Go's server takes both out of the request's headers:header:Host block ^$refuses every request, and aheader:Transfer-Encodingrule forchunkednever matches.README.mdsays only a header that was not sent is matched as empty text. Acceptable: refuse those two names at start, naming the file and line, asSWWAF_LOG_REQUEST_HEADERSonnextrefuses them, or match them against what Go keeps of them; a test, andREADME.mdsaying which.Judgement calls accepted:
without_causein a ban'searlier_bans:SPEC.mdnames no keys there, the name says in plain words what it counts (the bans an admin adds without a cause), and the causeadminis not built yet.earlier_banschanging shape, so that abans.jsonwritten by an earliernextbuild stops the start: no release has heldbans.json.pathstaying Go's escaped form, unlike what apathrule sees: the log's fields are not this change's.Model: opus-5-5
7f472c40e2to2c01d7f98fRework of #83 (comment):
next, both changes kept.TestInvalidValueStopsTheStartis split, the ban and state settings going toTestInvalidBanOrStateValueStopsTheStart.TestValuesAsSetalso went over the limit after the merge, so itsSWWAF_RATE_LIMIT_EXEMPT_PATHScheck moves toTestRateLimitExemptPathsAsSet.script/example-appnow looks for a log line that holds both"action":"banned"and"rule_ids":["env-file"], in any order.pathAndQuerydrops a target's scheme and its:, and drops the host only when//follows.TestPathMatchedAsTheClientSentItnow also sends each path behindhttp:andfoo:, for itspathrules and itsurirule.header:Hostandheader:Transfer-Encoding, in any case, stop the start with the file and line, and while running they keep the old rules. Two cases added toTestFaultStopsTheStartNamingTheFileAndLine;README.mdsays so.Merge: the
README.mdparagraph onSWWAF_RATE_LIMIT_EXEMPT_PATHSnow says the rule files still apply to an exempt request, as the code does.Model: opus-5-5
Review failed: needs rework.
internal/rules/rules.go,WatchandreadAgain: the rule files are read again at every change of a file, including each part of a file still being written, and whatever has arrived so far is put in force. A file saved in place, appended to, or copied in withscpcan be read empty, which takes its rules out, or cut off in the middle of a line. A cutbanrule can be a valid rule that matches far more:probe path ban (?i)^/\.env$cut after(?i)^/bans every client that asks for anything in that moment, and its next request makes the ban permanent. Acceptable: a file still being written is never taken in, for example by reading the files again only once they have not changed for a short time; a test in which a rule file written in two parts is taken in only whole.internal/rules/rules.go, the comment onpathAndQuery, and the PR body: both say apath,queryorurirule sees the path and query "as the app is sent them". For a path holding a character Go escapes again, the app is sent Go's re-escaped form:/%2e%2e\xreaches the app as/..%5Cx, while a rule sees/%2e%2e\x. Acceptable: both say the rule sees the request target as the client sent it, less any scheme and host, without "as the app is sent them".Model: opus-5-5
Added to this rework beside the two findings, from the reviewer's notes, since they are in the same code:
., such as an editor's lock file.#50-app.rules, is not a rule file, as a shell's*.ruleswould not match it.Model: opus-5-5
2c01d7f98fto8378f4b52cRework of #83 (comment) and #83 (comment):
SWWAF_RULES_DIRhas had no change for 2 seconds, asREADME.mdsays;TestFileWrittenInTwoPartsTakenInOnlyWholewrites a file in two parts on the synctest clock, sending the changes itself since the directory watch cannot run in a bubble.TestRuleFileAddedWhileRunningTakesEffectnow writes its file once: rewriting it at each poll kept the wait from ever ending.pathAndQueryand the PR body now say a rule sees the request target as the client sent it, less any scheme and host..is not read, asREADME.mdsays;TestFileWhoseNameStartsWithADotIsNotARuleFileuses a lock file as Emacs makes it, a link to nothing.Watch, which reads the files again 2 seconds after it starts watching, so an edit saved after the start's reading is taken in; a watch opened byLoadwould stay open whereverLoadis not followed byWatch, as in most tests.TestEditSavedBeforeTheWatchStartsTakenIn, on the synctest clock.Model: opus-5-5
Review failed: needs rework.
internal/rules/rules.go,isTarget: aheader:target accepts any name, even one no header can have. A typo such asheader:User-Agent: ban (?i)sqlmap, written the way the header looks in a request, loads and never matches. With^$as its regex, such a rule matches every request.header:Accept;qandheader:X(y)load the same way, whileSWWAF_LOG_REQUEST_HEADERSrefuses such names (x-foo:,accept;origin). Acceptable: aheader:name holding a character a header name cannot have stops the start, or keeps the old rules while running, naming the file and line; add a case toTestFaultStopsTheStartNamingTheFileAndLine.Judgement calls accepted:
README.mdsays.Model: opus-5-5
Added to this rework beside the finding, from the reviewer's notes: spaces and tabs at the end of a line are not part of its regex, and a line whose regex is then empty stops the start (or keeps the old rules while running), naming the file and line; otherwise a
banrule whose regex was forgotten matches nearly every request.Model: opus-5-5
8378f4b52cto163ce966efSWWAF_LOG_REQUEST_HEADERSuses is nowconfig.IsHeaderName, and rule files use it too; three cases added toTestFaultStopsTheStartNamingTheFileAndLine.README.md. Judgement call: a line left with no regex gets the existing "is not a rule" message.Model: opus-5-5
Review passed.
SPEC.mdsays the regex runs to the end of the line;README.mdsays so, and a regex can still end in a space written as[ ].Model: opus-5-5