From 18d5e9893d8deba17262b0a7badc2d0aa68697d9 Mon Sep 17 00:00:00 2001 From: Peter Abrahamsen Date: Sat, 25 Jul 2026 16:27:11 -0700 Subject: [PATCH] fix: reject page[limit] above max_page_size instead of truncating silently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ash clamps the SQL LIMIT to the action's max_page_size but keeps the *requested* limit for its "is there more?" bookkeeping: to_page/7 splits the rows on page_opts[:limit], so an over-large limit yields {all_rows, []} and more?: false. AshJsonApi then hits `Enum.count(results) < limit` and emits links.next: null. The result is a page that is both short and claims to be complete. On /api/json/detections, page[limit]=500 returns 251 rows (max_page_size 250, plus Ash's probe row) with no next link, so clients stop paginating and silently lose data. Passing page[count]=true does not help — the results-count branch is reached before the count is consulted. Present in ash 3.30.1 and ash_json_api 1.7.1 as well, so an upgrade does not fix it. Add OrcasiteWeb.Plugs.EnforceMaxPageSize, which resolves the JSON:API route ahead of dispatch and returns a 400 invalid_pagination error when page[limit] exceeds the matched action's max_page_size. This covers every index route, not just detections. It runs as a wrapper around the generated Ash router because AshJsonApi's before_dispatch hook calls the controller regardless of conn.halted. Also raise max_page_size to 1000 on Detection's :index and :by_category, matching Candidate and AudioImage. Fixes #992 Co-Authored-By: Claude Opus 5 (1M context) --- server/lib/orcasite/radio/detection.ex | 2 + server/lib/orcasite_web/json_api_router.ex | 19 ++- .../json_api_router/ash_router.ex | 11 ++ .../plugs/enforce_max_page_size.ex | 119 ++++++++++++++++++ .../json_api/max_page_size_test.exs | 71 +++++++++++ 5 files changed, 218 insertions(+), 4 deletions(-) create mode 100644 server/lib/orcasite_web/json_api_router/ash_router.ex create mode 100644 server/lib/orcasite_web/plugs/enforce_max_page_size.ex create mode 100644 server/test/orcasite_web/json_api/max_page_size_test.exs diff --git a/server/lib/orcasite/radio/detection.ex b/server/lib/orcasite/radio/detection.ex index 90e9d922f..985e439d5 100644 --- a/server/lib/orcasite/radio/detection.ex +++ b/server/lib/orcasite/radio/detection.ex @@ -128,6 +128,7 @@ defmodule Orcasite.Radio.Detection do offset? true countable true default_limit 100 + max_page_size 1000 end argument :feed_id, :string @@ -141,6 +142,7 @@ defmodule Orcasite.Radio.Detection do offset? true countable true default_limit 100 + max_page_size 1000 end argument :category, Orcasite.Types.DetectionCategory do diff --git a/server/lib/orcasite_web/json_api_router.ex b/server/lib/orcasite_web/json_api_router.ex index ad77b9d4a..9386e1af0 100644 --- a/server/lib/orcasite_web/json_api_router.ex +++ b/server/lib/orcasite_web/json_api_router.ex @@ -1,6 +1,17 @@ defmodule OrcasiteWeb.JsonApiRouter do - use AshJsonApi.Router, - domains: [Orcasite.Notifications, Orcasite.Radio], - json_schema: "/json_schema", - open_api: "/open_api" + @moduledoc """ + Entry point for the JSON:API. + + Wraps `OrcasiteWeb.JsonApiRouter.AshRouter` so our own plugs can run — and + halt — before dispatch. `AshJsonApi.Router`'s `before_dispatch` hook is not + usable for rejecting a request: it calls the route's controller regardless of + `conn.halted`. + """ + + use Plug.Builder + + plug OrcasiteWeb.Plugs.EnforceMaxPageSize, + domains: [Orcasite.Notifications, Orcasite.Radio] + + plug OrcasiteWeb.JsonApiRouter.AshRouter end diff --git a/server/lib/orcasite_web/json_api_router/ash_router.ex b/server/lib/orcasite_web/json_api_router/ash_router.ex new file mode 100644 index 000000000..22efde478 --- /dev/null +++ b/server/lib/orcasite_web/json_api_router/ash_router.ex @@ -0,0 +1,11 @@ +defmodule OrcasiteWeb.JsonApiRouter.AshRouter do + @moduledoc """ + The generated JSON:API router. Reached through `OrcasiteWeb.JsonApiRouter`, + which runs our own plugs first. + """ + + use AshJsonApi.Router, + domains: [Orcasite.Notifications, Orcasite.Radio], + json_schema: "/json_schema", + open_api: "/open_api" +end diff --git a/server/lib/orcasite_web/plugs/enforce_max_page_size.ex b/server/lib/orcasite_web/plugs/enforce_max_page_size.ex new file mode 100644 index 000000000..deaa3be8c --- /dev/null +++ b/server/lib/orcasite_web/plugs/enforce_max_page_size.ex @@ -0,0 +1,119 @@ +defmodule OrcasiteWeb.Plugs.EnforceMaxPageSize do + @moduledoc """ + Rejects JSON:API requests whose `page[limit]` exceeds the matched action's + `max_page_size`. + + Without this, an over-large limit is silently truncated and the response + claims to be complete. Ash clamps the SQL `LIMIT` to `max_page_size`, but + keeps the *requested* limit for its "is there more?" bookkeeping + (`Ash.Actions.Read.to_page/7` splits on `page[:limit]`), so the page comes + back short with `more?: false`. AshJsonApi then sees `Enum.count(results) < + limit` and emits `links.next: null`. A client asking for 500 detections gets + 251 and no reason to keep paginating. + + We can't serve what was asked for, so we say so rather than under-serving it + quietly. + + See https://github.com/orcasound/orcasite/issues/992. + """ + + @behaviour Plug + + import Plug.Conn + + @impl Plug + def init(opts), do: Keyword.validate!(opts, domains: []) + + @impl Plug + def call(conn, opts) do + conn = fetch_query_params(conn) + + with {:ok, limit} <- requested_limit(conn), + {:ok, resource, action_name} <- match_route(conn, Keyword.fetch!(opts, :domains)), + {:ok, max_page_size} <- max_page_size(resource, action_name), + true <- limit > max_page_size do + send_invalid_pagination(conn, limit, max_page_size) + else + _ -> conn + end + end + + defp requested_limit(%{query_params: %{"page" => %{"limit" => limit}}}) when is_binary(limit) do + case Integer.parse(limit) do + {limit, ""} -> {:ok, limit} + _ -> :error + end + end + + defp requested_limit(_conn), do: :error + + # Mirrors the route resolution in `AshJsonApi.Controllers.Router`. This plug + # runs ahead of that router (which does not honor `halt/1`), so it has to find + # the route itself to know which action's limits apply. + defp match_route(conn, domains) do + Enum.find_value(domains, :error, fn domain -> + case match_domain_route(domain, conn) do + {:ok, resource, route} -> + {:ok, resource, route.action} + + :error -> + domain + |> Ash.Domain.Info.resources() + |> Enum.filter(&(AshJsonApi.Resource in Spark.extensions(&1))) + |> Enum.find_value(fn resource -> + case resource.json_api_match_route(conn.method, conn.path_info) do + {:ok, route, _params} -> {:ok, resource, route.action} + :error -> nil + end + end) + end + end) + end + + defp match_domain_route(domain, conn) do + if Code.ensure_loaded?(domain) and function_exported?(domain, :json_api_match_route, 2) do + case domain.json_api_match_route(conn.method, conn.path_info) do + {:ok, resource, route, _params} -> {:ok, resource, route} + :error -> :error + end + else + :error + end + end + + # Only read actions carry `pagination`; anything else (creates, or a `related` + # route whose action lives on the destination resource) falls through and is + # left alone. + defp max_page_size(resource, action_name) do + case Ash.Resource.Info.action(resource, action_name) do + %{pagination: %{max_page_size: max_page_size}} when is_integer(max_page_size) -> + {:ok, max_page_size} + + _ -> + :error + end + end + + defp send_invalid_pagination(conn, limit, max_page_size) do + errors = + AshJsonApi.Serializer.serialize_errors(nil, [ + %AshJsonApi.Error{ + id: Ash.UUID.generate(), + status_code: 400, + code: "invalid_pagination", + title: "InvalidPagination", + detail: + "Invalid pagination: page[limit] of #{limit} exceeds the maximum page size of " <> + "#{max_page_size} for this endpoint. Request #{max_page_size} or fewer and " <> + "follow links.next to retrieve the rest.", + source_parameter: "page[limit]", + meta: %{} + } + ]) + + conn + |> put_resp_content_type("application/vnd.api+json") + |> send_resp(400, errors) + |> halt() + end +end diff --git a/server/test/orcasite_web/json_api/max_page_size_test.exs b/server/test/orcasite_web/json_api/max_page_size_test.exs new file mode 100644 index 000000000..da011f48c --- /dev/null +++ b/server/test/orcasite_web/json_api/max_page_size_test.exs @@ -0,0 +1,71 @@ +defmodule OrcasiteWeb.JsonApi.MaxPageSizeTest do + @moduledoc """ + Regression tests for https://github.com/orcasound/orcasite/issues/992. + + A `page[limit]` above the action's `max_page_size` used to be silently + clamped, returning a short page with `links.next: null` — which reads as + "that's everything". It must be an error instead. + """ + + use OrcasiteWeb.ConnCase, async: true + + describe "page[limit] above max_page_size" do + test "is rejected on an action with an explicit max_page_size", %{conn: conn} do + assert %{ + "errors" => [ + %{ + "code" => "invalid_pagination", + "detail" => detail, + "source" => %{"parameter" => "page[limit]"}, + "status" => "400", + "title" => "InvalidPagination" + } + ] + } = + conn + |> get("/api/json/detections?page[limit]=1001") + |> json_response(400) + + assert detail =~ "exceeds the maximum page size of 1000" + end + + test "is rejected on an action using Ash's default max_page_size", %{conn: conn} do + assert %{"errors" => [%{"code" => "invalid_pagination", "detail" => detail}]} = + conn + |> get("/api/json/feed_segments?page[limit]=251") + |> json_response(400) + + assert detail =~ "exceeds the maximum page size of 250" + end + end + + describe "page[limit] at or below max_page_size" do + test "is served for an action with an explicit max_page_size", %{conn: conn} do + assert %{"data" => _} = + conn + |> get("/api/json/detections?page[limit]=1000") + |> json_response(200) + end + + test "is served for an action using Ash's default max_page_size", %{conn: conn} do + assert %{"data" => _} = + conn + |> get("/api/json/feed_segments?page[limit]=250") + |> json_response(200) + end + + test "requests without page[limit] are untouched", %{conn: conn} do + assert %{"data" => _} = + conn + |> get("/api/json/detections") + |> json_response(200) + end + end + + test "unmatched routes still 404 rather than being swallowed", %{conn: conn} do + assert %{"errors" => [%{"code" => "no_route_found"}]} = + conn + |> get("/api/json/not_a_real_resource?page[limit]=1000000") + |> json_response(404) + end +end