improvement: raise when a keyset sort field is forbidden by a field policy - #2791
Open
nseaSeb wants to merge 1 commit into
Open
improvement: raise when a keyset sort field is forbidden by a field policy#2791nseaSeb wants to merge 1 commit into
nseaSeb wants to merge 1 commit into
Conversation
zachdaniel
reviewed
Jul 22, 2026
| # deselected and the cursor would be built from the redaction marker, silently | ||
| # breaking pagination. Raise instead. Sorts coming from `Ash.Query.sort_input/2` | ||
| # are rewritten to be policy-aware before this point, so they are not affected. | ||
| defp validate_keyset_sort_readable( |
Contributor
There was a problem hiding this comment.
I think this has to happen on the resulting data actually. Because a field might not be statically forbidden, but forbidden via a calculation in the query. So when generating the keyset on a result we should raise the error there.
…den 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 ash-project#2786 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nseaSeb
force-pushed
the
fix/keyset-redacted-sort-field
branch
from
July 23, 2026 07:31
3645b8d to
44cb06e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2786.
Following the discussion there —
Ash.Query.sortis opting in, but the actor still can't read the field, so the field policy redacts it and keyset pagination silently breaks.What happens today
A keyset-paginated read sorted on a field a field policy forbids the actor from reading:
The field is deselected, so the cursor is built from the redaction marker (
%Ash.NotLoaded{}) instead of the value. Page 1 is fine (2 rows, correct order); page 2 seeks against that marker and returns 0 of the remaining rows — no error, pagination just ends early.The change
Rather than building an unusable cursor, this detects a statically-forbidden keyset sort field and raises
Ash.Error.Page.KeysetSortOnForbiddenField. Two things worth calling out about how:deselect_known_forbidden_fields(which already computes the statically-forbidden fields — I extracted that list intoknown_forbidden_fields/4). So there's no "detect the marker on the built cursor" code to look for; the read is rejected upstream and the error threads through the existingwith(soAsh.readreturns{:error, _}andAsh.read!raises).sort_inputis unaffected. Sorts fromAsh.Query.sort_input/2are already rewritten to be policy-aware (they encodenil, not a marker), so they keep paginating.Scope
This covers static field policies — the case in #2786's repro. Record-dependent field policies (e.g.
owner_id == actor(:id)) behave differently and are not handled here: the field is loaded to sort, so the cursor is built from the real value. That's a leak rather than an empty page, and a separate concern — flagging it so this isn't read as fully closing that class.One deliberate choice for review: the check is gated on the read actually paginating (
query.page). A keyset-action read with no page opts sorted on a forbidden field still attaches a marker cursor as metadata without raising. Happy to drop that gate if you'd rather it be unconditional.Tests
test/policy/field_policies/keyset_pagination_test.exs: forbidden plain sort raises, admin (who can read the field) still paginates, a readable field still paginates, and the existingsort_inputtest still passes. Full suite green.