From 44cb06e3c4234372574d599dc531515743e9f87b Mon Sep 17 00:00:00 2001 From: nseaSeb Date: Sat, 18 Jul 2026 18:25:10 +0200 Subject: [PATCH] improvement: reject keyset pagination sorted on a field-policy-forbidden field Sorting a keyset-paginated read on a field a field policy forbids the actor from reading redacts that field on the returned rows, so the cursor could only be built from the redaction marker (or, for a record-dependent policy, from the real value it just leaked). The second page then seeks against that and silently returns no rows. Check the field-policy-resolved records before paging: if a sorted field comes back redacted, reject the read with `Ash.Error.Page.KeysetSortOnForbiddenField` instead of returning a page built on an unusable cursor. Because the check runs on the resolved result, it covers both static and record-dependent field policies. Sorts from `Ash.Query.sort_input/2` are rewritten to be policy-aware earlier (they encode `nil`, not the value), so their positions are skipped. Only reads that actually paginate are affected; a plain sort on a policy-protected field without pagination is unchanged. Closes #2786 Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/ash/actions/read/read.ex | 98 +++++++++---- .../page/keyset_sort_on_forbidden_field.ex | 30 ++++ lib/ash/page/keyset.ex | 29 ++++ .../field_policies/keyset_pagination_test.exs | 136 ++++++++++++++++++ 4 files changed, 266 insertions(+), 27 deletions(-) create mode 100644 lib/ash/error/page/keyset_sort_on_forbidden_field.ex diff --git a/lib/ash/actions/read/read.ex b/lib/ash/actions/read/read.ex index 3d004e1195..f235be5a9e 100644 --- a/lib/ash/actions/read/read.ex +++ b/lib/ash/actions/read/read.ex @@ -491,20 +491,25 @@ defmodule Ash.Actions.Read do opts[:authorize?], false ) do - data - |> Helpers.restrict_field_access(query) - |> add_tenant(new_query) - |> attach_fields(opts[:initial_data], initial_query, query, missing_pkeys?) - |> cleanup_field_auth(query) - |> add_page( - query.action, - count, - query.sort, - query, - new_query, - opts - ) - |> add_query(query, opts) + restricted = + data + |> Helpers.restrict_field_access(query) + |> add_tenant(new_query) + |> attach_fields(opts[:initial_data], initial_query, query, missing_pkeys?) + |> cleanup_field_auth(query) + + with :ok <- reject_forbidden_keyset_sort(restricted, query) do + restricted + |> add_page( + query.action, + count, + query.sort, + query, + new_query, + opts + ) + |> add_query(query, opts) + end else {:error, %Ash.Query{errors: errors} = query} -> {:error, Ash.Error.to_error_class(errors, query: query)} @@ -1108,19 +1113,24 @@ defmodule Ash.Actions.Read do opts[:authorize?], false ) do - data - |> Helpers.restrict_field_access(query) - |> add_tenant(query) - |> attach_fields(nil, initial_query, query, false) - |> cleanup_field_auth(query) - |> add_page( - query.action, - count, - query.sort, - initial_query, - query, - opts - ) + restricted = + data + |> Helpers.restrict_field_access(query) + |> add_tenant(query) + |> attach_fields(nil, initial_query, query, false) + |> cleanup_field_auth(query) + + with :ok <- reject_forbidden_keyset_sort(restricted, query) do + add_page( + restricted, + query.action, + count, + query.sort, + initial_query, + query, + opts + ) + end else {:error, %Ash.Query{errors: errors} = query} -> {:error, Ash.Error.to_error_class(errors, query: query)} @@ -3052,6 +3062,40 @@ defmodule Ash.Actions.Read do end end + # A keyset cursor seeks against the sort field's value, so sorting a + # keyset-paginated read on a field a field policy forbids the actor from + # reading can't produce a usable cursor. Checked on the field-policy-resolved + # records (after `restrict_field_access`), so it covers both static and + # record-dependent field policies. Sorts from `Ash.Query.sort_input/2` are + # rewritten to be policy-aware earlier and encode `nil`, so they are unaffected. + defp reject_forbidden_keyset_sort(records, query) do + pagination = query.action && query.action.pagination + + if query.page && pagination && pagination.keyset? && + !use_data_layer_keyset?(query, pagination) do + case Ash.Page.Keyset.forbidden_sort_field( + List.wrap(records), + query.sort || [], + query.sort_input_indices || [] + ) do + nil -> + :ok + + field -> + {:error, + Ash.Error.to_error_class( + Ash.Error.Page.KeysetSortOnForbiddenField.exception( + resource: query.resource, + field: field + ), + query: query + )} + end + else + :ok + end + end + defp remove_already_selected(fields, %struct{results: results}) when struct in [Ash.Page.Keyset, Ash.Page.Offset], do: remove_already_selected(fields, results) diff --git a/lib/ash/error/page/keyset_sort_on_forbidden_field.ex b/lib/ash/error/page/keyset_sort_on_forbidden_field.ex new file mode 100644 index 0000000000..dae9f3351c --- /dev/null +++ b/lib/ash/error/page/keyset_sort_on_forbidden_field.ex @@ -0,0 +1,30 @@ +# SPDX-FileCopyrightText: 2019 ash contributors +# +# SPDX-License-Identifier: MIT + +defmodule Ash.Error.Page.KeysetSortOnForbiddenField do + @moduledoc """ + Used when a keyset-paginated read is sorted on a field a field policy forbids + the current actor from reading. + + The field would be redacted on the returned rows, so the keyset cursor could + only be built from the redaction marker rather than the sort value, silently + breaking pagination. The read is rejected instead. + + If the sort comes from user input, use `Ash.Query.sort_input/2`, which takes + field policies into account. Otherwise, avoid sorting on a field this actor + cannot read while using keyset pagination. + """ + + use Splode.Error, fields: [:resource, :field], class: :invalid + + def message(%{resource: resource, field: field}) do + """ + Cannot keyset-paginate #{inspect(resource)} sorted on #{inspect(field)}: a field policy forbids the current actor from reading #{inspect(field)}. + + The field would be redacted on the returned rows, so the keyset cursor could only be built from the redaction marker rather than the sort value, which silently breaks pagination. + + If this sort comes from user input, use `Ash.Query.sort_input/2`. Otherwise, don't sort on #{inspect(field)} with keyset pagination for an actor that cannot read it. + """ + end +end diff --git a/lib/ash/page/keyset.ex b/lib/ash/page/keyset.ex index 2d5128d544..7b4db70ff1 100644 --- a/lib/ash/page/keyset.ex +++ b/lib/ash/page/keyset.ex @@ -107,6 +107,35 @@ defmodule Ash.Page.Keyset do end) end + @doc false + # Returns the name of the first sort field that is redacted (`%Ash.ForbiddenField{}`) + # on any of the given records, or `nil`. A keyset cursor seeks against the sort + # field's value, so a field the actor cannot read can't produce a usable cursor. + # This is checked on the field-policy-resolved records, so it covers both static + # and record-dependent field policies. + # + # Sort positions in `sort_input_indices` come from `Ash.Query.sort_input/2` and + # were already rewritten to be policy-aware (they encode `nil`, not the value), + # so they are skipped. + def forbidden_sort_field(records, sort, sort_input_indices) do + Enum.find_value(records, fn record -> + sort + |> Enum.zip(field_values(record, sort)) + |> Enum.with_index() + |> Enum.find_value(fn {{{field, _direction}, value}, index} -> + if index not in sort_input_indices and match?(%Ash.ForbiddenField{}, value) do + sort_field_name(field) + end + end) + end) + end + + defp sort_field_name(%{__struct__: struct, name: name}) + when struct in [Ash.Query.Calculation, Ash.Query.Aggregate], + do: name + + defp sort_field_name(field), do: field + @doc """ Creates filters on the query using the query for the Keyset. """ diff --git a/test/policy/field_policies/keyset_pagination_test.exs b/test/policy/field_policies/keyset_pagination_test.exs index f2d2ebd60c..1856322fcc 100644 --- a/test/policy/field_policies/keyset_pagination_test.exs +++ b/test/policy/field_policies/keyset_pagination_test.exs @@ -111,4 +111,140 @@ defmodule Ash.Test.Policy.FieldPolicy.KeysetPaginationTest do assert length(page2.results) == 1 assert Enum.uniq(all_ids) == all_ids end + + defmodule AdminDoc do + @moduledoc false + use Ash.Resource, + domain: Domain, + data_layer: Ash.DataLayer.Ets, + authorizers: [Ash.Policy.Authorizer] + + ets do + private? true + end + + attributes do + uuid_primary_key :id + + attribute :title, :string do + public? true + end + + attribute :secret_score, :integer do + public? true + end + end + + actions do + default_accept [:title, :secret_score] + defaults [:create] + + read :read do + primary? true + pagination keyset?: true, required?: false + end + end + + policies do + policy always() do + authorize_if always() + end + end + + field_policies do + field_policy :secret_score do + authorize_if actor_attribute_equals(:admin, true) + end + + field_policy :* do + authorize_if always() + end + end + end + + test "plain sort on a field the actor cannot read raises instead of silently breaking pagination" do + for i <- 1..4 do + AdminDoc + |> Ash.Changeset.for_create(:create, %{title: "t#{i}", secret_score: i * 10}, + authorize?: false + ) + |> Ash.create!() + end + + # a field policy forbids a non-admin from reading `secret_score`, so the + # cursor could only be built from the redaction marker. Rather than silently + # returning an empty second page, the read is rejected. + assert {:error, %Ash.Error.Invalid{errors: errors}} = + AdminDoc + |> Ash.Query.sort(secret_score: :asc) + |> Ash.read(actor: %{admin: false}, page: [limit: 2]) + + assert Enum.any?(errors, &match?(%Ash.Error.Page.KeysetSortOnForbiddenField{}, &1)) + + # an admin can read the field, so pagination works + page1 = + AdminDoc + |> Ash.Query.sort(secret_score: :asc) + |> Ash.read!(actor: %{admin: true}, page: [limit: 2]) + + cursor = List.last(page1.results).__metadata__.keyset + + page2 = + AdminDoc + |> Ash.Query.sort(secret_score: :asc) + |> Ash.read!(actor: %{admin: true}, page: [limit: 2, after: cursor]) + + assert length(page1.results) == 2 + assert length(page2.results) == 2 + end + + test "plain sort on a readable field still paginates under field policies" do + for {owner, score} <- [{"me", 10}, {"someone_else", 20}, {"someone_else", 30}] do + OwnedDoc + |> Ash.Changeset.for_create( + :create, + %{title: "doc-#{score}", owner_id: owner, secret_score: score}, + authorize?: false + ) + |> Ash.create!() + end + + page1 = + OwnedDoc + |> Ash.Query.sort(title: :asc) + |> Ash.read!(actor: %{id: "me"}, page: [limit: 2]) + + cursor = List.last(page1.results).__metadata__.keyset + + page2 = + OwnedDoc + |> Ash.Query.sort(title: :asc) + |> Ash.read!(actor: %{id: "me"}, page: [limit: 2, after: cursor]) + + assert length(page1.results) == 2 + assert length(page2.results) == 1 + end + + test "plain sort on a record-dependent forbidden field raises" do + for {owner, score} <- [{"me", 10}, {"someone_else", 20}, {"someone_else", 30}] do + OwnedDoc + |> Ash.Changeset.for_create( + :create, + %{title: "doc", owner_id: owner, secret_score: score}, + authorize?: false + ) + |> Ash.create!() + end + + # `secret_score` is readable for the actor's own rows but not others'. The + # field is loaded to sort, so the cursor would otherwise be built from the + # real value for rows the actor can't read. Checking the redacted result + # rejects the read rather than leaking those values through the cursor. + assert {:error, %Ash.Error.Invalid{errors: errors}} = + OwnedDoc + |> Ash.Query.sort(secret_score: :asc) + |> Ash.read(actor: %{id: "me"}, page: [limit: 2]) + + assert Enum.any?(errors, &match?(%Ash.Error.Page.KeysetSortOnForbiddenField{}, &1)) + end end