Path rules and trap paths match the path decoded once too (closes #85)

A path rule matched only the path as the client sent it, so /%2eenv got
past the default env-file rule while an app that decodes the path
serves /.env. A path rule now matches the path as sent or decoded once,
as a uri rule already did, and so does a path in SWWAF_TRAP_PATHS:
/%77p-login.php matches /wp-login.php. The query is cut off before the
path is decoded, and the path is decoded only once, so /%252eenv is not
/.env. A ban's notes keep the path as sent. SPEC.md and README.md say
so.

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
@@ -741,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
@@ -1522,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/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/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"},