Run every top-level path filter that matches the request

CHANGELOG.md

@@ -6,6 +6,16 @@
   - A filter refuses a handshake with `halt`, or with a status a registered `error` handler covers; the refusal closes the connection. A status alone still upgrades, as it still runs an HTTP route's handler.
   - `after` filters do not run for a handshake, since they would run once the `101` is on the wire - also where an HTTP route shares the path, which used to run them.
 - ***(BREAKING)*** A router's filters apply to the router's own routes only. A top-level route or `ws` on the same path no longer runs them; guard it with top-level filters. For a request no route serves, the router `before` filters of a route on the path still run, so a router guard or CORS preflight answer keeps covering the methods the router does not serve. Router filters run after the top-level filters of the same step.
+- ***(BREAKING)*** Run every top-level path filter whose path matches the request, in the order their paths were first registered, instead of only the most specific one. Global filters (`before_all` and friends without a path) still run first, router filters last. Before upgrading, check the filters whose paths overlap:
+  - A more specific filter path no longer exempts its paths from a broader guard. With `before_all "/api/*" { halt 401 unless authenticated }` and any `before_all "/api/login"` filter, `POST /api/login` used to get past the guard and now answers `401`. Skip such paths inside the guard itself, e.g. `next if env.request.path == "/api/login"`.
+  - Filters on overlapping paths all run, so side effects such as logging, counters or rate limits may run more than once per request: `before_get "/admin"` next to `before_get "/admin/"` both run for either spelling, since each matches both. One block registered for several paths at once, as with `before_all ["/admin", "/admin/*"]`, still runs once.
+  - The filter on the path registered later has the last word. `before_all "/api/html" { |env| env.response.content_type = "text/html" }` followed by `before_all "/api/*" { |env| env.response.content_type = "application/json" }` now answers `/api/html` with JSON; register the more specific path after the broader one. A block added to a path later joins that path's place, so registering `/api/html` again after `/api/*` does not help. `after` filters run in the same order, not reversed.
+  - Why: path filters shared one radix tree that ran the single best match, so a broader guard silently did not run wherever a more specific filter path matched too: with `before_all "/api/*" { halt 401 }` and `before_all "/api/public" { ... }`, `GET /api/public` skipped the guard and got `200`, and `before_get "/users/:id"` was skipped for `GET /users/me` once `before_get "/users/me"` existed. A filter path now matches what it matches on its own: the shared tree could also lose filters altogether, so `before_get "/users/:id"` and `before_get "/users/:uid/posts"` together ran for neither `/users/5` nor `/users/5/posts`, and it refused some combinations at boot with `Radix::Tree::SharedKeyError` - add `before_get "/users/me"` to those two - which are now accepted. `Kemal::FilterHandler#tree` no longer holds the path filters.
+- ***(BREAKING)*** Run no `before` filter after one that closed the response with `halt` or `redirect` - not later blocks on the same path, path filters after a halting `before_all`, the `before_get` step after the `before_all` one, or router filters - just as it already kept the route handler from running. `after` filters still all run. Before upgrading:
+  - A `before` filter that has to see refused requests - audit logging of failed logins, a metrics counter - no longer runs for them when it comes after the guard. Move it to an `after` filter.
+  - Refuse with `halt`. A guard that only sets a status for a registered `error` handler leaves the response open, so the filters after it still run and can change the status.
+  - An `after` filter that sets a header or the content type has to check `env.response.closed?` first: on a halted response it raises `IO::Error`, which skips the `after` filters behind it, `after_all` logging included.
+  - Why: the filters after a halt kept running on a response already sent. With `before_all "/api/*" { halt 401 }` and `before_get "/api/public" { |env| env.response.content_type = "application/json" }`, the client got its `401` but `content_type=` raised `IO::Error` on the closed response, which skipped `after_all`, and a later filter's work - a database lookup, a counter - ran for a request the guard had refused. With overlapping path filters now all running, a halting guard with a filter behind it is the common case.
 - ***(SECURITY)*** Never route a request-target that does not start with `/`, and refuse a malformed one ([GHSA-x97g-6h7h-q9w6](https://github.com/kemalcr/kemal/security/advisories/GHSA-x97g-6h7h-q9w6)). The route key is the method followed by the path with nothing in between, so the request line `GE T/admin/secret` built the key of `GET /admin/secret` and was served by that route, while every path-scoped guard read `T/admin/secret` and stood aside: `use "/admin", Auth`, `before_all "/admin/*"`, a `Kemal::Router` mounted at `/admin`, prefix `only`/`exclude` rules. Any verb worked - `DELET E/admin/users/42` ran the `delete` route. Such a request is now answered `400 Bad Request` with `Connection: close`, through a registered `error 400` like the refusal of a malformed method (#820). The route tree, `context.route` and the `only`/`exclude` rules of a `Kemal::Handler` no longer resolve a method and a path that cannot both be believed, so a handler that answers by itself behind `only ["/metrics"]` no longer served `GE T/metrics` - nor `GET/metrics /`, which the method check of #820 left reaching it. Absolute-form (`http://host/path`), authority-form (`CONNECT host:port`) and `OPTIONS *` targets are well formed, routed nowhere and still get `404`.
 - End the connection when an exception arrives after the response headers have gone out, instead of reading the next request on it. `Kemal::ExceptionHandler` could no longer render an error page at that point and returned as if the request had succeeded, so under keep-alive the server answered the next request on the same connection. A route that set `Content-Length: 100`, flushed `partial` and raised had the next pipelined response spliced into the 93 missing bytes - `partialHTTP/1.1 200 OK...` - which a client, or a proxy pooling the connection, read as one body. The connection now closes after the cut, as it does when an exception escapes the handlers altogether, so the client sees the response end short (`curl: (18) end of response with 93 bytes missing`). A chunked response, which the server still ends with its final chunk, and a response `halt` already closed keep their connection - an `after` filter touching a header of a large streamed response (#759) costs nothing. An error page for an exception raised before anything is sent is unchanged.
 - Leave a request path that names no static file to the routes instead of redirecting it. `StaticFileHandler` runs ahead of the routes and compared the *decoded* path with its canonical form, so an encoded `/` or a byte that is not UTF-8 in a route param never reached the route: with `get "/u/:id"`, `/u/a%2F%2Fb` answered `302 /u/a/b` and `/u/%FF` answered `302 /u/%EF%BF%BD`. A non-canonical path is now redirected only when it names a file under `public_folder`, or when it has a `.` or `..` segment - so `/u/..%2F..%2Fetc` still answers `302 /etc` and no route receives a dot segment the handler would have resolved away.

spec/filters_spec.cr

@@ -366,4 +366,214 @@ describe "Kemal::FilterHandler" do
       response.body.should_not contain("ACCOUNT-DATA")
     end
   end
+
+  # Path filters used to share one radix tree, which runs the single best
+  # match for the request path: a more specific filter path hid a broader
+  # guard on the same request. The specific filter is registered first below,
+  # so it runs before the guard halts and both are seen to run.
+  context "overlapping top-level path filters" do
+    it "runs a glob guard next to a more specific static filter" do
+      before_all "/api/public" do |env|
+        env.response.headers["X-Public"] = "1"
+      end
+      before_all "/api/*" do |env|
+        halt env, status_code: 401, response: "auth required"
+      end
+      get "/api/public" { "PUBLIC" }
+      get "/api/secret" { "SECRET" }
+
+      response = call_request_on_app(HTTP::Request.new("GET", "/api/public"))
+      response.status_code.should eq(401)
+      response.headers["X-Public"].should eq("1")
+      call_request_on_app(HTTP::Request.new("GET", "/api/secret")).status_code.should eq(401)
+    end
+
+    it "runs a param guard next to a static filter on one of its paths" do
+      before_get "/users/me" do |env|
+        env.response.headers["X-Me"] = "1"
+      end
+      before_get "/users/:id" do |env|
+        halt env, status_code: 401, response: "auth required"
+      end
+      get "/users/:id" { "USER" }
+
+      response = call_request_on_app(HTTP::Request.new("GET", "/users/me"))
+      response.status_code.should eq(401)
+      response.headers["X-Me"].should eq("1")
+    end
+
+    it "runs both filters on paths that differ by a trailing slash" do
+      before_all "/admin" do |env|
+        env.response.headers["X-Admin"] = "1"
+      end
+      before_all "/admin/" do |env|
+        halt env, status_code: 401, response: "auth required"
+      end
+      get "/admin" { "ADMIN" }
+
+      {"/admin", "/admin/"}.each do |path|
+        response = call_request_on_app(HTTP::Request.new("GET", path))
+        response.status_code.should eq(401)
+        response.headers["X-Admin"].should eq("1")
+      end
+    end
+
+    it "runs a glob guard next to a filter on its own prefix" do
+      before_all "/admin/" do |env|
+        env.response.headers["X-Admin"] = "1"
+      end
+      before_all "/admin/*" do |env|
+        halt env, status_code: 401, response: "auth required"
+      end
+      get "/admin" { "ADMIN" }
+
+      response = call_request_on_app(HTTP::Request.new("GET", "/admin"))
+      response.status_code.should eq(401)
+      response.headers["X-Admin"].should eq("1")
+    end
+
+    it "matches each filter path exactly where radix does on its own" do
+      filter_paths = ["/", "/admin", "/admin/", "/admin/*", "/users/:id", "/users/:id/posts", "/a//b", "/files/*path"]
+      request_paths = ["/", "/admin", "/admin/", "/admin/x", "/adminx", "/users", "/users/", "/users/5",
+                       "/users/5/", "/users/5/posts", "/a//b", "/a/b", "/files", "/files/x/y"]
+
+      filter_paths.each do |filter_path|
+        radix = Radix::Tree(Bool).new
+        radix.add "/before/ALL/#{filter_path}", true
+        ran = false
+        handler = Kemal::FilterHandler.new
+        handler._add_route_filter("ALL", filter_path, :before) { ran = true }
+
+        request_paths.each do |request_path|
+          ran = false
+          handler.call(HTTP::Server::Context.new(HTTP::Request.new("GET", request_path), HTTP::Server::Response.new(IO::Memory.new)))
+          {filter_path, request_path, ran}.should eq({filter_path, request_path, radix.find("/before/ALL/#{request_path}").found?})
+        end
+      end
+    end
+
+    it "runs a filter path whose key the shared tree split" do
+      calls = [] of String
+      before_get "/users/:id" { calls << "user" }
+      before_get "/users/:uid/posts" { calls << "posts" }
+      get "/users/:id" { "USER" }
+      get "/users/:id/posts" { "POSTS" }
+
+      call_request_on_app(HTTP::Request.new("GET", "/users/5"))
+      call_request_on_app(HTTP::Request.new("GET", "/users/5/posts"))
+      calls.should eq(["user", "posts"])
+    end
+
+    it "takes filter paths the shared tree refused" do
+      calls = [] of String
+      before_get "/users/me" { calls << "me" }
+      before_get "/users/:id" { calls << "user" }
+      before_get "/users/:uid/posts" { calls << "posts" }
+      get "/users/:id/posts" { "POSTS" }
+
+      call_request_on_app(HTTP::Request.new("GET", "/users/5/posts")).status_code.should eq(200)
+      calls.should eq(["posts"])
+    end
+
+    it "runs a block registered for several matching paths once" do
+      calls = 0
+      before_all ["/admin", "/admin/*"] { calls += 1 }
+      before_all ["/admin", "/admin"] { calls += 10 }
+      get "/admin" { "ADMIN" }
+      get "/admin/users" { "USERS" }
+
+      call_request_on_app(HTTP::Request.new("GET", "/admin"))
+      calls.should eq(11)
+      call_request_on_app(HTTP::Request.new("GET", "/admin/users"))
+      calls.should eq(12)
+    end
+
+    it "runs no before filter after one that halts, in any step" do
+      calls = [] of String
+      before_all "/api/*" do |env|
+        calls << "guard"
+        halt env, status_code: 401, response: "auth required"
+      end
+      before_all "/api/*" { calls << "same path" }
+      before_all "/api/public" { |env| calls << "all"; env.response.content_type = "application/json" }
+      before_get "*" { calls << "global get" }
+      before_get "/api/public" { |env| calls << "get"; env.response.content_type = "application/json" }
+      after_get "/api/public" { calls << "after get" }
+      after_all { calls << "after all" }
+
+      router = Kemal::Router.new
+      router.before { calls << "router" }
+      router.get "/public" { calls << "handler"; "PUBLIC" }
+      mount "/api", router
+
+      response = call_request_on_app(HTTP::Request.new("GET", "/api/public"))
+      response.status_code.should eq(401)
+      response.body.should eq("auth required")
+      calls.should eq(["guard", "after get", "after all"])
+    end
+
+    it "runs no path filter after a global filter that halts" do
+      ran = false
+      before_all { |env| halt env, status_code: 401, response: "auth required" }
+      before_all "/api/public" { ran = true }
+      get "/api/public" { "PUBLIC" }
+
+      call_request_on_app(HTTP::Request.new("GET", "/api/public")).status_code.should eq(401)
+      ran.should be_false
+    end
+
+    it "runs every matching after filter" do
+      calls = [] of String
+      after_get "/reports/*" { calls << "glob" }
+      after_get "/reports/:id" { calls << "param" }
+      after_get "/reports/daily" { calls << "static" }
+      get "/reports/:id" { calls << "handler"; "REPORT" }
+
+      call_request_on_app(HTTP::Request.new("GET", "/reports/daily"))
+      calls.should eq(["handler", "glob", "param", "static"])
+    end
+
+    it "runs every matching GET filter for a HEAD served by the GET route" do
+      before_get "/admin/users" do |env|
+        env.response.headers["X-Users"] = "1"
+      end
+      before_get "/admin/*" do |env|
+        halt env, status_code: 401, response: "auth required"
+      end
+      handler_ran = false
+      get "/admin/users" { handler_ran = true; "users" }
+
+      response = call_request_on_app(HTTP::Request.new("HEAD", "/admin/users"))
+      response.status_code.should eq(401)
+      response.headers["X-Users"].should eq("1")
+      handler_ran.should be_false
+    end
+
+    it "runs every matching before_all filter for an unrouted request" do
+      before_all "/api/nope" do |env|
+        env.response.headers["X-Nope"] = "1"
+      end
+      before_all "/api/*" do |env|
+        halt env, status_code: 401, response: "auth required"
+      end
+
+      response = call_request_on_app(HTTP::Request.new("GET", "/api/nope"))
+      response.status_code.should eq(401)
+      response.headers["X-Nope"].should eq("1")
+    end
+
+    it "runs global filters first, then path filters in the order their paths were registered" do
+      calls = [] of String
+      before_get "/orders/me" { calls << "static 1" }
+      before_get "/orders/*" { calls << "glob" }
+      before_get "*" { calls << "global" }
+      before_get "/orders/:id" { calls << "param" }
+      before_get "/orders/me" { calls << "static 2" }
+      before_get "/orders/other" { calls << "unmatched" }
+      get "/orders/:id" { calls << "handler"; "ORDER" }
+
+      call_request_on_app(HTTP::Request.new("GET", "/orders/me"))
+      calls.should eq(["global", "static 1", "static 2", "glob", "param", "handler"])
+    end
+  end
 end

src/kemal/dsl.cr

@@ -172,8 +172,10 @@ end
     end
 
     def {{ type.id }}_{{ method.id }}(paths : Enumerable(String), &block : HTTP::Server::Context -> _)
+      # One block for every path, so it runs once where several of them match.
+      filter = Kemal::FilterHandler::FilterBlock.new(shared: true, &block)
       paths.each do |path|
-        Kemal::FilterHandler::INSTANCE.{{ type.id }}({{ method }}.upcase, path, &block)
+        Kemal::FilterHandler::INSTANCE._add_route_filter({{ method }}.upcase, path, :{{ type.id }}, filter)
       end
     end
   {% end %}