Leave SWWAF_RATE_LIMIT_EXEMPT_PATHS out of the request rate limits (closes #77)

A request is neither counted nor refused by the request rate limits
when its path, percent-decoded, starts with one of the comma-separated
prefixes in SWWAF_RATE_LIMIT_EXEMPT_PATHS. A request whose decoded path
contains .. or a backslash, or whose path as sent holds an encoded
slash, is never exempt, since an app may act on it as a path outside
every prefix, such as /assets/..%2Flogin as /login. The static lists,
bans and the country lists still apply. The setting is empty by
default, and a prefix that does not start with / stops the start.
README.md documents it.

Model: opus-5-5

README.md

@@ -81,13 +81,14 @@ in `bin/state` unless `SWWAF_STATE_DIR` is set.
   takes the client over one of the rate limits below is refused with
   `SWWAF_BAN_RESPONSE`, `403` by default, before anything reaches the app, and
   bans the client. A request whose path starts with one of
-  `SWWAF_RATE_LIMIT_EXEMPT_PATHS` is neither counted nor refused by the rate
+  `SWWAF_RATE_LIMIT_EXEMPT_PATHS`, as that setting below describes, is neither
-  limits; the static lists, bans and the country lists still apply to it. A
+  counted nor refused by the rate limits; the static lists, bans and the country
-  client is one IPv4 address, or one IPv6 /64, since one abuser usually holds a
+  lists still apply to it. A client is one IPv4 address, or one IPv6 /64, since
-  whole /64. Each window is counted in two fixed buckets, the earlier one
+  one abuser usually holds a whole /64. Each window is counted in two fixed
-  weighted by how much of it the window still covers. At most 20,000 clients are
+  buckets, the earlier one weighted by how much of it the window still covers.
-  kept, the least recently seen dropped first, with their history, and a restart
+  At most 20,000 clients are kept, the least recently seen dropped first, with
-  gives no client a fresh allowance (see "State files" below).
+  their history, and a restart gives no client a fresh allowance (see "State
+  files" below).
 - Bans a client that breaks a rate limit, as "Bans" in [`SPEC.md`](SPEC.md)
   describes: the first ban lasts an hour, and a limit broken again within a day
   of a ban ending bans for three times as long as that ban, so 1, 3, 9, 27 and
@@ -196,14 +197,14 @@ it, and the effective settings are logged at start.
   page makes a few hundred requests and several people often share one address.
 - `SWWAF_RATE_LIMIT_EXEMPT_PATHS` (default empty): path prefixes whose requests
   the rate limits neither count nor refuse, such as `/assets/` for static
-  assets; each starts with `/`. A prefix is compared, character for character,
+  assets; each starts with `/`. A request whose path, percent-decoded, contains
-  with the start of the path the app will act on: the request's path, before any
+  `..` anywhere or a backslash, or whose path as sent holds an encoded slash
-  query string, percent-decoded, with its `.` and `..` segments and repeated
+  (`%2F` or `%2f`), is never exempt, since the app may act on it as a path
-  slashes resolved and without a trailing slash, which is not always what the
+  outside every prefix: `/assets/..%2Flogin` as `/login`. Any other request is
-  request log's `path` shows. `/assets/` matches `/assets/app.js`,
+  exempt when its path, percent-decoded and before any query string, starts with
-  `/assets//img/logo.png` and `/static/../assets/app.js`, but not `/assets/`
+  a prefix, character for character. `/assets/` matches `/assets/app.js` and
-  itself, `/assets`, `/Assets/app.js`, `/static/assets/app.js` or
+  `/assets/`, but not `/assets`, `/Assets/app.js`, `/static/assets/app.js`,
-  `/assets/..%2Flogin`, which is `/login`. A prefix is written without
+  `/static/../assets/app.js` or `/assets%2Fapp.js`. A prefix is written without
   percent-encoding, and there are no wildcards: `*` is a character like any
   other.
 - `SWWAF_DENIED_COUNTRIES` (default empty): countries whose clients are refused,
@@ -751,9 +752,9 @@ so that they run in minimal containers.
 ## TODO
 
 - The rest of milestone 3: taking in an admin's edits to the state files
-  (https://git.eeqj.de/sneak/smallwebwaf/issues/68), exemptions and the rest of
+  (https://git.eeqj.de/sneak/smallwebwaf/issues/68) and the rest of the request
-  the request log's fields; then the rest of the design, in the order of the
+  log's fields; then the rest of the design, in the order of the build order in
-  build order in [`SPEC.md`](SPEC.md).
+  [`SPEC.md`](SPEC.md).
 
 ## Documents
 

internal/proxy/ratelimits_test.go

@@ -92,12 +92,9 @@ func TestRateLimitExemptPathsAreNeitherCountedNorRefused(t *testing.T) {
 	// With a limit of one request a minute, the requests for paths under a
 	// prefix are not counted, so client's first request for / is within
 	// the limit; and once client has reached it, they are not refused.
-	// The app acts on /static/../assets/app.js as /assets/app.js.
 	s.request(client, "/assets/app.js", http.StatusOK, requestlog.ActionForward)
 	s.request(client, "/favicon.ico?v=2", http.StatusOK, requestlog.ActionForward)
 	s.get(client, http.StatusOK, requestlog.ActionForward)
-	s.request(client, "/static/../assets/app.js",
-		http.StatusOK, requestlog.ActionForward)
 
 	line := s.request(client, "/assets/app.js", http.StatusOK, requestlog.ActionForward)
 	if line.LimitHit != "" {
@@ -116,17 +113,24 @@ func TestRateLimitExemptPathsAreNeitherCountedNorRefused(t *testing.T) {
 		http.StatusForbidden, requestlog.ActionCountryDenied)
 }
 
-func TestRateLimitCountsPathsOutsideEveryExemptPrefix(t *testing.T) {
+func TestRateLimitCountsPathsThatAreNotExempt(t *testing.T) {
 	t.Parallel()
 
-	// A prefix matches only at the start of the path, and the app acts on
-	// the last three paths as /login, once they are percent-decoded and
-	// their .. segments resolved.
 	for _, sent := range []string{
+		// A prefix matches only at the start of the path.
 		"/static/assets/app.js",
+		// .. once percent-decoded: an app may act on these as /login, the
+		// last as a path under /sneak/app/ or as /assets/x.
 		"/assets/../login",
 		"/assets/%2e%2e/login",
 		"/assets/..%2Flogin",
+		"/assets/..;/login",
+		"/sneak/app/src/branch/main/..%2F..%2F..%2F..%2F..%2F..%2Fassets/x",
+		// An encoded slash or a backslash: Go's router takes /assets%2Fx
+		// for one path segment, not a path under /assets/.
+		"/assets%2Fx",
+		"/assets%2fx",
+		`/assets/x\y`,
 	} {
 		t.Run(sent, func(t *testing.T) {
 			t.Parallel()

internal/proxy/request.go

@@ -7,8 +7,8 @@ import (
 	"net/http/httptrace"
 	"net/http/httputil"
 	"net/netip"
+	"net/url"
 	"os"
-	"path"
 	"slices"
 	"strings"
 	"sync"
@@ -148,9 +148,9 @@ func (rq *request) check(ctx context.Context) *refusal {
 // client either refuses is not looked up, and then the country lists; a
 // request any of them refuses is not counted for the rate limits. Then
 // come the rate limits, unless the client is in
-// SWWAF_RATE_LIMIT_EXEMPT_NETS or the path the app will act on starts
+// SWWAF_RATE_LIMIT_EXEMPT_NETS or the request's path is exempt under
-// with one of SWWAF_RATE_LIMIT_EXEMPT_PATHS, so that every other request
+// SWWAF_RATE_LIMIT_EXEMPT_PATHS, so that every other request is counted.
-// is counted. ctx is the request's own context.
+// ctx is the request's own context.
 func (rq *request) checkClient(ctx context.Context) string {
 	cfg := rq.h.config
 	if isInside(rq.client, cfg.AllowNets) {
@@ -171,13 +171,8 @@ func (rq *request) checkClient(ctx context.Context) string {
 		return requestlog.ActionCountryDenied
 	}
 
-	// The prefixes are matched against the path the app will act on:
-	// URL.Path is the request's path percent-decoded, and path.Clean
-	// resolves its . and .. segments and repeated slashes, and drops a
-	// trailing slash. So /assets/..%2Flogin is /login, outside /assets/,
-	// whatever the log line's path shows.
 	exempt := isInside(rq.client, cfg.RateLimitExemptNets) ||
-		startsWithAny(path.Clean(rq.in.URL.Path), cfg.RateLimitExemptPaths)
+		pathExempt(rq.in.URL, cfg.RateLimitExemptPaths)
 	if !exempt && rq.limitBroken(now) {
 		return requestlog.ActionRateLimited
 	}
@@ -185,10 +180,25 @@ func (rq *request) checkClient(ctx context.Context) string {
 	return ""
 }
 
-// startsWithAny reports whether s starts with one of prefixes.
+// pathExempt reports whether the rate limits leave out a request for u
-func startsWithAny(s string, prefixes []string) bool {
+// because of SWWAF_RATE_LIMIT_EXEMPT_PATHS: whether its path,
+// percent-decoded, starts with one of prefixes. A request whose decoded
+// path contains .. anywhere or a backslash, or whose path as sent holds
+// an encoded slash (%2F or %2f), never is, since an app may act on it as
+// a path outside every prefix: /assets/..%2Flogin as /login, or
+// /assets%2Fx as one path segment, as Go's router does.
+func pathExempt(u *url.URL, prefixes []string) bool {
+	decoded := u.Path
+	// EscapedPath is the path as the app receives it, not decoded.
+	sent := strings.ToLower(u.EscapedPath())
+
+	if strings.Contains(decoded, "..") || strings.Contains(decoded, `\`) ||
+		strings.Contains(sent, "%2f") {
+		return false
+	}
+
 	return slices.ContainsFunc(prefixes, func(prefix string) bool {
-		return strings.HasPrefix(s, prefix)
+		return strings.HasPrefix(decoded, prefix)
 	})
 }