A client that stops sending after the app answers is timed out (closes #130)

When the app answered before the client had sent its whole body, and its
answer ended with its last bytes, every timeout was stopped,
SWWAF_CLIENT_REQUEST_TIMEOUT included. The transport went on waiting to
read the body, and Go's server cannot send the answer until that read
returns, so a client that stopped sending held the request until it
closed the connection. The timeouts now run on until the client has sent
its body, as they do before the app answers: once
SWWAF_CLIENT_REQUEST_TIMEOUT runs out the client is sent the app's
answer, the connection is closed, and the log line says timed_out.

Model: opus-5-5

README.md

@@ -197,11 +197,12 @@ in `bin/state` unless `SWWAF_STATE_DIR` is set, and the default rule file of
   rate limits and before the rule files, which it does not need. A request for
   one is refused with `SWWAF_BAN_RESPONSE` and bans the client's netblock for a
   clear sign of attack, as a `ban` rule's match does. A trap path is matched
-  against the path a `path` rule sees: as the client sent it, before any
-  decoding and without the query, the whole of it, character for character.
-  `/wp-login.php` matches `/wp-login.php?redirect_to=x`, but not
-  `/wp-login.php/`, `/WP-LOGIN.PHP`, `/blog/wp-login.php` or `/%77p-login.php`.
-  A client in `SWWAF_ALLOW_NETS` is not checked.
+  against the path a `path` rule sees: without the query, as the client sent it
+  and once percent-decoded, the whole of it, character for character.
+  `/wp-login.php` matches `/wp-login.php?redirect_to=x` and `/%77p-login.php`,
+  but not `/wp-login.php/`, `/WP-LOGIN.PHP`, `/blog/wp-login.php` or
+  `/%2577p-login.php`. The ban's notes give the path as the client sent it. A
+  client in `SWWAF_ALLOW_NETS` is not checked.
 - Inspects each request with the OWASP Core Rule Set 4.25.0, run by Coraza,
   after the rule files, unless `SWWAF_WAF_MODE` is `off`: its method, its URL
   with the query, and its headers, and, once `SWWAF_WAF_BODY_LIMIT` is set, its
@@ -714,15 +715,19 @@ effective settings are logged at start, unless `SWWAF_LOG_LEVEL` is `warn` or
   request body. A size, such as `128K`, at most `1G`, has it read a body of form
   data or multipart up to that size, the rest of a longer one passing on to the
   app as it arrives, without being held, and a JSON or XML body no larger than
-  that size, since those cannot be read in part. A body whose type ends in
-  `+json`, or is `text/json`, is JSON, and one whose type ends in `+xml` is XML.
-  Any other body reaches the app uninspected, and so does a larger JSON or XML
-  body: the Core Rule Set would read any other body as form data, where binary
-  content such as a git push trips rules written for text. A body it reads that
-  Coraza cannot parse (rule 900440), and a multipart body that fails Coraza's
-  strict checks (rule 900450), add 5 to the score, as a rule rated critical
-  does, since no rule reads what comes after the fault; a multipart body that
-  reaches the limit does not, since the limit can cut it inside a part's header.
+  that size, since those cannot be read in part. A body is JSON when its type is
+  `application/json` or `text/json`, or an `application/` or `text/` type ending
+  in `+json`, and XML when its type is `application/xml` or `text/xml`, or an
+  `application/` or `text/` type ending in `+xml`. Any other body reaches the
+  app uninspected, and so does a larger JSON or XML body: the Core Rule Set
+  would read any other body as form data, where binary content such as a git
+  push trips rules written for text. A body it reads that Coraza cannot parse
+  (rule 900440), and a multipart body that fails Coraza's strict checks (rule
+  900450), add 5 to the score, as a rule rated critical does, since no rule
+  reads what comes after the fault. So does a multipart body the limit cuts
+  before the colon of a part's header line, or between the carriage return and
+  the line feed that end a part's header line or the empty line after its
+  headers, since Coraza takes the line the limit cuts for a malformed header.
   The client has until `SWWAF_CLIENT_REQUEST_TIMEOUT` runs out to send the part
   that is read, and a request whose body passes `SWWAF_REQUEST_MAX_BYTES` within
   it is refused before anything reaches the app. Of a file in a multipart body,
@@ -737,8 +742,8 @@ effective settings are logged at start, unless `SWWAF_LOG_LEVEL` is `warn` or
   scanners ask for, such as `/wp-login.php,/xmlrpc.php` in front of gitea; a
   request for one bans its client for a clear sign of attack, as a `ban` rule
   does, with or without rule files (see "What it does so far" above). A path
-  that does not start with `/`, or holds a `?`, which no path a `path` rule sees
-  holds, stops the start.
+  that does not start with `/`, or holds a `?`, as one written with a query
+  does, stops the start.
 - `SWWAF_ERROR_BURST_THRESHOLD` (default `30`): the most requests of a client in
   a minute that `smallwebwaf` may refuse after a rule file match, a trap path or
   a Core Rule Set match, or for a missing or wrong token; one more breaks the
@@ -1518,14 +1523,15 @@ scanner-agent  user_agent  ban     (?i)\b(sqlmap|nikto|nuclei|wpscan)\b
 
 - The id is letters, digits, `-` and `_`, and no two rules share one. The
   request log, the metrics and a ban's notes name the rule by it.
-- The target is what the regex is matched against: `path` or `query`, as the
-  client sent it, before any decoding; `uri`, the path and the query together,
-  both as sent and once percent-decoded, so that an encoded probe does not slip
-  past; `method`; `host`; `user_agent`; `referer`; or `header:<Name>`, any one
-  request header but `Host` and `Transfer-Encoding`, which Go's HTTP server
-  takes out of every request; the request's host is the target `host`. A header
-  sent more than once is matched with its values joined by `, `, and one not
-  sent as empty text. No body is read.
+- The target is what the regex is matched against: `path`, the path without the
+  query, and `uri`, the path and the query together, each both as sent and once
+  percent-decoded, so that an encoded probe does not slip past; `query`, as the
+  client sent it, before any decoding; `method`; `host`; `user_agent`;
+  `referer`; or `header:<Name>`, any one request header but `Host` and
+  `Transfer-Encoding`, which Go's HTTP server takes out of every request; the
+  request's host is the target `host`. A header sent more than once is matched
+  with its values joined by `, `, and one not sent as empty text. No body is
+  read.
 - The action is `log`, `block` or `ban` (see "What it does so far" above). Keep
   `ban` for requests no real visitor sends, and anchor a path at the site root
   with `^/`: a file of the same name deeper in a site can be ordinary content,

SPEC.md

@@ -825,7 +825,8 @@ a file edited, added or removed there takes effect while it runs.
 - `id`: a short name of letters, digits, `-` and `_`, unique across all files.
   It appears in the request log, metrics, alerts and ban notes.
 - `target`: what the regex is matched against.
-    - `path`: the URL path as received, before any decoding.
+    - `path`: the URL path without the query, both as received and once
+      percent-decoded, so an encoded probe cannot slip past.
     - `query`: the raw query string.
     - `uri`: path and query together, both as received and once percent-decoded,
       so an encoded probe cannot slip past.

internal/bans/bans.go

@@ -123,7 +123,8 @@ type Notes struct {
 	LimitPercentSetting string `json:"limit_percent_setting,omitempty"`
 	// RuleID and Target are, for a ban for a clear sign of attack, the id
 	// of the rule file rule that matched, and its target; TrapPath is, for
-	// one for a request for a path in SWWAF_TRAP_PATHS, that path.
+	// one for a request for a path in SWWAF_TRAP_PATHS, that path as the
+	// client sent it.
 	RuleID   string `json:"rule_id,omitempty"`
 	Target   string `json:"target,omitempty"`
 	TrapPath string `json:"trap_path,omitempty"`

internal/config/config.go

@@ -1665,7 +1665,8 @@ func parsePathPrefixes(value string) ([]string, error) {
 
 // parseTrapPaths reads a comma-separated list of trap paths. Each is
 // matched against a request's path as a path rule is, without the query,
-// so a path that does not start with / or holds a ? would never match.
+// so a path that does not start with / would never match, and one that
+// holds a ? is written with a query.
 func parseTrapPaths(value string) ([]string, error) {
 	paths, err := parseList(value)
 	if err != nil {

internal/proxy/request.go

@@ -789,9 +789,19 @@ func (rq *request) responseTimedOut() {
 	}
 }
 
-// responseReceived is called once the app has sent its whole answer.
+// responseReceived is called once the app has sent its whole answer. An
+// app may answer before the client has sent its whole body, as with a 404
+// it sends without reading it. The timeouts then run on: until the
+// transport's read of the body returns, Go's server cannot send the
+// answer, and SWWAF_CLIENT_REQUEST_TIMEOUT is what ends that read for a
+// client that stops sending.
 func (rq *request) responseReceived() {
 	rq.complete = true
+
+	if rq.body != nil && !rq.body.received.Load() {
+		return
+	}
+
 	rq.stopTimers()
 }
 

internal/proxy/rules.go

@@ -10,13 +10,15 @@ import (
 )
 
 // trapPath reports whether the request asks for a path in
-// SWWAF_TRAP_PATHS: its path as a path rule sees it, before any decoding
-// and without the query, is one of them. Such a request is a clear sign of
-// attack, as a ban rule's match is: it bans the client's netblock, or in
-// observe mode raises the alert for the ban it would have made.
+// SWWAF_TRAP_PATHS: its path as a path rule sees it, without the query,
+// as sent or once percent-decoded, is one of them. Such a request is a
+// clear sign of attack, as a ban rule's match is: it bans the client's
+// netblock, or in observe mode raises the alert for the ban it would have
+// made. The ban's notes give the path as sent.
 func (rq *request) trapPath(now time.Time) bool {
 	path := rules.Path(rq.in)
-	if !slices.Contains(rq.h.config.TrapPaths, path) {
+	if !slices.Contains(rq.h.config.TrapPaths, path) &&
+		!slices.Contains(rq.h.config.TrapPaths, rules.DecodeOnce(path)) {
 		return false
 	}
 

internal/proxy/timeouts_test.go

@@ -194,6 +194,28 @@ func sendLargeBody(t *testing.T, addr string) net.Conn {
 	return conn
 }
 
+func TestClientStopsSendingAfterTheAppHasAnswered(t *testing.T) {
+	t.Parallel()
+
+	// The app answers at once without reading the body, as an app does
+	// for a path it does not serve.
+	app := startApp(t, http.NotFound)
+	addr, out := startProxy(t, app.URL, map[string]string{
+		clientRequestTimeout: shortTimeoutSetting,
+		metricsToken:         token,
+	})
+
+	start := time.Now()
+	got := readResponse(t, sendPartOfBody(t, addr))
+	wantTimedOut(t, start)
+
+	// The app's answer came before the time ran out, so the client is
+	// sent that answer, not smallwebwaf's 408.
+	wantStatus(t, got, http.StatusNotFound)
+	wantLine(t, out.requestLine(t), http.StatusNotFound, requestlog.ActionTimedOut)
+	wantLimitHits(t, addr, clientRequestTimeout, 1)
+}
+
 func TestAppTooSlowToAnswer(t *testing.T) {
 	t.Parallel()
 

internal/proxy/trappaths_test.go

@@ -32,10 +32,9 @@ func TestTrapPathBansAsABanRuleDoes(t *testing.T) {
 	})
 	start := clk.Now()
 
-	// Only the path itself, as the client sent it, is a trap path.
-	for _, path := range []string{
-		"/wp-login.php/", "/WP-LOGIN.PHP", "/blog/xmlrpc.php", "/%77p-login.php",
-	} {
+	// Only the path itself, the whole of it in the same case, is a trap
+	// path.
+	for _, path := range []string{"/wp-login.php/", "/WP-LOGIN.PHP", "/blog/xmlrpc.php"} {
 		s.request(otherClient, path, http.StatusOK, requestlog.ActionForward)
 	}
 
@@ -84,6 +83,22 @@ func TestTrapPathBansAsABanRuleDoes(t *testing.T) {
 	}
 }
 
+func TestTrapPathMatchesThePathDecodedOnce(t *testing.T) {
+	t.Parallel()
+
+	s, _, server := startWithClock(t, "", map[string]string{trapPaths: trapPathList})
+
+	// %2577 decodes once to %77, not to w.
+	s.request(otherClient, "/%2577p-login.php", http.StatusOK, requestlog.ActionForward)
+	s.request(client, "/%77p-login.php", http.StatusForbidden, requestlog.ActionBanned)
+
+	// The ban's notes give the path as the client sent it.
+	got := server.Ledger.Bans(netip.MustParsePrefix(client + "/32"))
+	if len(got) != 1 || got[0].Notes.TrapPath != "/%77p-login.php" {
+		t.Fatalf("bans %+v, want one whose notes give the trap path /%%77p-login.php", got)
+	}
+}
+
 func TestTrapPathsNeedNoRuleFiles(t *testing.T) {
 	t.Parallel()
 

internal/rules/rules.go

@@ -50,6 +50,13 @@ const quietTime = 2 * time.Second
 // header:<Name>.
 const headerTarget = "header:"
 
+// pathTarget and uriTarget are the targets matched both as received and
+// once percent-decoded: the path, and the path and query together.
+const (
+	pathTarget = "path"
+	uriTarget  = "uri"
+)
+
 // escapeLength is the length of a percent escape, such as %2e.
 const escapeLength = 3
 
@@ -374,7 +381,7 @@ func parse(line string) (Rule, bool, error) {
 // isTarget reports whether target is one a rule may have.
 func isTarget(target string) bool {
 	switch target {
-	case "path", "query", "uri", "method", "host", "user_agent", "referer":
+	case pathTarget, "query", uriTarget, "method", "host", "user_agent", "referer":
 		return true
 	}
 
@@ -384,35 +391,38 @@ func isTarget(target string) bool {
 }
 
 // matches reports whether the rule's regex matches its target in r. For
-// uri it is matched against the path and query as received, and against
-// them once percent-decoded, so that an encoded probe cannot slip past.
+// path and uri it is matched against the target as received, and against
+// it once percent-decoded, so that an encoded probe cannot slip past.
 func (rule Rule) matches(r *http.Request) bool {
-	if rule.Target == "uri" {
-		uri := pathAndQuery(r)
+	target := value(rule.Target, r)
 
-		return rule.regex.MatchString(uri) || rule.regex.MatchString(decodeOnce(uri))
+	if rule.Target == pathTarget || rule.Target == uriTarget {
+		return rule.regex.MatchString(target) || rule.regex.MatchString(DecodeOnce(target))
 	}
 
-	return rule.regex.MatchString(value(rule.Target, r))
+	return rule.regex.MatchString(target)
 }
 
 // Path returns r's path as the client sent it, before any decoding or
-// re-encoding, up to the first ?: what a path rule is matched against.
+// re-encoding, up to the first ?: what a path rule is matched against, as
+// it is and once percent-decoded.
 func Path(r *http.Request) string {
 	path, _, _ := strings.Cut(pathAndQuery(r), "?")
 
 	return path
 }
 
-// value returns what a rule with target, other than uri, is matched
-// against in r: the path, as Path gives it, and the query as the client
-// sent it, before any decoding or re-encoding, after the first ?, and a
-// header's values joined by ", ", as HTTP joins those of a header sent
-// more than once.
+// value returns what a rule with target is matched against in r: the
+// path, as Path gives it, the path and query, as pathAndQuery gives
+// them, and the query as the client sent it, before any decoding or
+// re-encoding, after the first ?, and a header's values joined by ", ",
+// as HTTP joins those of a header sent more than once.
 func value(target string, r *http.Request) string {
 	switch target {
-	case "path":
+	case pathTarget:
 		return Path(r)
+	case uriTarget:
+		return pathAndQuery(r)
 	case "query":
 		_, query, _ := strings.Cut(pathAndQuery(r), "?")
 
@@ -464,11 +474,11 @@ func header(r *http.Request, name string) string {
 	return strings.Join(r.Header.Values(name), ", ")
 }
 
-// decodeOnce returns s with each percent escape, such as %2e, replaced by
+// DecodeOnce returns s with each percent escape, such as %2e, replaced by
 // the byte it stands for. A % that is not followed by two hex digits is
 // left as it is, so that a malformed escape cannot keep the rest of s
 // from being decoded.
-func decodeOnce(s string) string {
+func DecodeOnce(s string) string {
 	var decoded strings.Builder
 
 	for i := 0; i < len(s); i++ {

internal/rules/rules_test.go

@@ -49,7 +49,9 @@ func TestEachTargetMatchesWhatItNames(t *testing.T) {
 		want   bool
 	}{
 		{"path as received", `path log ^/%2eenv$`, "/%2eenv", nil, true},
-		{"path not decoded", `path log ^/\.env$`, "/%2eenv", nil, false},
+		{"path decoded", `path log ^/\.env$`, "/%2Eenv", nil, true},
+		{"path decoded only once", `path log ^/\.env$`, "/%252eenv", nil, false},
+		{"path decoded without the query", `path log ^/a\?b$`, "/a%3fb?c", nil, true},
 		{"path without the query", `path log ^/a$`, "/a?b=c", nil, true},
 		{"query as received", `query log ^b=%2e$`, "/a?b=%2e", nil, true},
 		{"uri as received", `uri log ^/a\?b=%2e$`, "/a?b=%2e", nil, true},
@@ -418,7 +420,7 @@ func TestDefaultFileBansProbesAtTheSiteRootAlone(t *testing.T) {
 
 	// Probes sent by a browser, by the rule that refuses them.
 	for rule, targets := range map[string][]string{
-		"env-file":    {"/.env", "/.env.production", "/.ENV"},
+		"env-file":    {"/.env", "/.env.production", "/.ENV", "/%2eenv"},
 		"vcs-dir":     {"/.git/config", "/.git", "/.svn/entries"},
 		"secrets-dir": {"/.aws/credentials", "/.ssh/id_rsa"},
 		"secret-file": {"/.htpasswd", "/.DS_Store", "/.git-credentials"},

internal/waf/waf.go

@@ -105,9 +105,10 @@ SecRuleUpdateTargetById 944110 "!REQUEST_HEADERS:Referer"
 // bodyDirectives have the Core Rule Set read the part of a request body
 // Inspect gives it, which is at most one byte longer than the limit, up to
 // the limit, %d bytes, and read JSON and XML as Coraza's recommended
-// configuration has it in its rules 200000, 200001 and 200006, with any
-// type ending in +xml, and text/json, besides; form data and multipart
-// Coraza knows by itself. %% stands for a % Coraza reads.
+// configuration has it in its rules 200000, 200001 and 200006, with
+// text/json, and any application or text type ending in +xml or +json,
+// besides; form data and multipart Coraza knows by itself. %% stands for
+// a % Coraza reads.
 const bodyDirectives = `
 SecRequestBodyAccess On
 SecRequestBodyLimit %d
@@ -132,13 +133,11 @@ SecRule REQBODY_PROCESSOR "@rx ^(?:URLENCODED|MULTIPART|JSON|XML)$" \
 # its strict checks (900450), each add 5 to the score, as a rule the Core
 # Rule Set rates critical does: no rule reads what comes after the fault,
 # which the app may still read. Coraza's recommended configuration refuses
-# them in its rules 200002 and 200003. A multipart body that reaches the
-# limit is let off both (900430), since the limit can cut it inside a
-# part's header, which Coraza takes for such a fault. Coraza parses any
-# form data body.
-SecRule INBOUND_DATA_ERROR "@eq 1" "id:900430,phase:2,pass,nolog,chain"
-    SecRule REQBODY_PROCESSOR "@streq MULTIPART" \
-        "ctl:ruleRemoveById=900440,ctl:ruleRemoveById=900450"
+# them in its rules 200002 and 200003. A multipart body the limit cuts
+# before the colon of a part's header line, or between the carriage return
+# and the line feed that end a part's header line or the empty line after
+# its headers, adds 5 too, since Coraza takes the line the limit cuts for a
+# malformed header. Coraza parses any form data body.
 SecRule REQBODY_ERROR "!@eq 0" "id:900440,phase:2,pass,severity:'CRITICAL',\
     setvar:'tx.inbound_anomaly_score_pl1=+5'"
 SecRule MULTIPART_STRICT_ERROR "!@eq 0" "id:900450,phase:2,pass,\

internal/waf/waf_test.go

@@ -613,29 +613,57 @@ func TestBodyCorazaCannotParseIsAMatch(t *testing.T) {
 
 	// The multipart bodies Coraza cannot parse fail its strict checks too:
 	// one whose type names its boundary twice, and one with a part header
-	// that has no colon, before the attack.
-	body = field("q", injection) + end
-	wantBody(t, crs, post(multipart+"; boundary=c"), body, matched(900440, 900450),
-		body)
+	// that has no colon, before the attack. They do so padded past the
+	// limit too, which cuts them in the padding.
+	noColon := "--b\r\nContent-Disposition form-data; name=\"a\"\r\n\r\n1\r\n"
+	pad := field("pad", strings.Repeat("a", bodyLimit))
 
-	body = "--b\r\nContent-Disposition form-data; name=\"a\"\r\n\r\n1\r\n" +
-		field("q", injection) + end
-	wantBody(t, crs, post(multipart), body, matched(900440, 900450), body)
+	for _, tc := range []struct{ header, head string }{
+		{multipart + "; boundary=c", ""},
+		{multipart, noColon},
+	} {
+		body = tc.head + field("q", injection) + end
+		wantBody(t, crs, post(tc.header), body, matched(900440, 900450), body)
+
+		body = tc.head + field("q", injection) + pad + end
+		wantBody(t, crs, post(tc.header), body, matched(900440, 900450),
+			body[:bodyLimit+1])
+	}
 }
 
-func TestMultipartBodyCutInsideAPartHeaderIsNotAMatch(t *testing.T) {
+func TestMultipartBodyCutBeforeAPartHeadersColonIsAMatch(t *testing.T) {
 	t.Parallel()
 
 	// The limit falls in the middle of the name of the second part's
-	// header, which Coraza, reading up to the limit, finds without a colon.
+	// header, which Coraza, reading up to the limit, cannot tell from a
+	// header without a colon.
 	cut := "--b\r\nContent-Di"
 	first := field("pad", strings.Repeat("a", bodyLimit-len(field("pad", ""))-len(cut)))
 	body := first + cut + "sposition: form-data; name=\"q\"\r\n\r\n1\r\n" + end
 
-	wantBody(t, readingBodies(t), post(multipart), body, waf.Result{},
+	wantBody(t, readingBodies(t), post(multipart), body, matched(900440, 900450),
 		body[:bodyLimit+1])
 }
 
+func TestMultipartBodyCutBeforeALineFeedInAPartsHeadersIsAMatch(t *testing.T) {
+	t.Parallel()
+
+	crs := readingBodies(t)
+	headerLine := "--b\r\nContent-Disposition: form-data; name=\"q\"\r"
+
+	// The limit falls between the carriage return and the line feed that
+	// end the second part's header line, and then between those that end
+	// the empty line after it. Coraza, reading up to the limit, takes the
+	// line ending in a lone carriage return for a malformed header.
+	for _, cut := range []int{len(headerLine), len(headerLine + "\n\r")} {
+		first := field("pad", strings.Repeat("a", bodyLimit-len(field("pad", ""))-cut))
+		body := first + field("q", "1") + end
+
+		wantBody(t, crs, post(multipart), body, matched(900440, 900450),
+			body[:bodyLimit+1])
+	}
+}
+
 // TestCorazaWritesNoFile is not parallel, since it sets TMPDIR, the
 // system's temporary directory, for the whole test process.
 func TestCorazaWritesNoFile(t *testing.T) {