From 6272af8ea918f3d5d7374b195dba7ae61a7ec75a Mon Sep 17 00:00:00 2001
From: Jim Brandt <jbrandt@bestpractical.com>
Date: Tue, 7 Nov 2023 17:09:31 -0500
Subject: [PATCH] Set next_page and prev_page without leaking any info

c2d5d72cd removed total and pages if a user doesn't have
sufficient rights for security, and this makes paging
difficult because the caller doesn't know if there are
more results.

Refactor next_page to set it based on looking ahead,
but still be secure and not reveal any extra
information about counts. If next_page exists,
a caller now knows they should call for more records.

Fixes: I##37712
---
 lib/RT/REST2/Resource/Collection.pm | 41 ++++++++++++----
 t/rest2/tickets.t                   | 74 +++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 8 deletions(-)

diff --git a/lib/RT/REST2/Resource/Collection.pm b/lib/RT/REST2/Resource/Collection.pm
index 7653d5695c6..6cd18bcee64 100644
--- a/lib/RT/REST2/Resource/Collection.pm
+++ b/lib/RT/REST2/Resource/Collection.pm
@@ -208,18 +208,29 @@ sub serialize {
     $results{pages} = defined $results{total} ? ceil($results{total} / $results{per_page}) : undef;
     if ( $results{pages} ) {
         if ($results{page} < $results{pages}) {
-            my $page = $results{page} + 1;
-            $uri->query_form( @query_form, page => $results{page} + 1 );
-            $results{next_page} = $uri->as_string;
+            $results{next_page} = $self->get_next_page( results_ref => \%results, uri => $uri, query_form_ref => \@query_form );
         }
-        if ($results{page} > 1) {
-            # If we're beyond the last page, set prev_page as the last page
-            # available, otherwise, the previous page.
-            $uri->query_form( @query_form, page => ($results{page} > $results{pages} ? $results{pages} : $results{page} - 1) );
-            $results{prev_page} = $uri->as_string;
+    }
+
+    if ( (not exists $results{next_page}) && (not defined $results{total}) ) {
+        # If total is undef, this collection checks ACLs in code so we can't
+        # use the collection count directly. Try to peek ahead to see if there
+        # are more records available so we can return next_page without giving
+        # away specific counts.
+
+        $collection->NextPage();
+        while (my $item = $collection->Next) {
+            # As soon as we get one record, we know there is a next page
+            $results{next_page} = $self->get_next_page( results_ref => \%results, uri => $uri, query_form_ref => \@query_form );
+            last;
         }
     }
 
+    if ( (not exists $results{prev_page}) && $results{page} > 1 ) {
+        $uri->query_form( @query_form, page => ($results{page} - 1) );
+        $results{prev_page} = $uri->as_string;
+    }
+
     return \%results;
 }
 
@@ -229,6 +240,20 @@ sub serialize_record {
     return expand_uid($record);
 }
 
+sub get_next_page {
+    my $self = shift;
+    my %args = (
+        results_ref => undef,
+        uri => undef,
+        query_form_ref => undef,
+        @_
+    );
+
+    my $page = $args{results_ref}->{page} + 1;
+    $args{uri}->query_form( @{$args{query_form_ref}}, page => $args{results_ref}->{page} + 1 );
+    return $args{uri}->as_string;
+}
+
 # XXX TODO: Bulk update via DELETE/PUT on a collection resource?
 
 sub charsets_provided { [ 'utf-8' ] }
diff --git a/t/rest2/tickets.t b/t/rest2/tickets.t
index fd563c82e73..a880ef47bc7 100644
--- a/t/rest2/tickets.t
+++ b/t/rest2/tickets.t
@@ -405,6 +405,80 @@ my ($ticket_url, $ticket_id);
     }
 }
 
+# Transactions with paging
+{
+    my $res = $mech->get($ticket_url,
+        'Authorization' => $auth,
+    );
+    is($res->code, 200);
+
+    my $history_pages_url = $mech->url_for_hypermedia('history');
+
+    # Set low per_page to get at least two pages
+    $history_pages_url .= '?per_page=10';
+    $res = $mech->get($history_pages_url,
+        'Authorization' => $auth,
+    );
+    is($res->code, 200);
+
+    my $content = $mech->json_response;
+    is($content->{count}, 9);
+    is($content->{page}, 1);
+    is($content->{per_page}, 10);
+    is($content->{prev_page}, undef, 'No prev_page');
+    is($content->{next_page}, $history_pages_url . '&page=2');
+
+    is($content->{total}, undef, 'No total');
+    is(scalar @{$content->{items}}, 9);
+
+    for my $txn (@{ $content->{items} }) {
+        is($txn->{type}, 'transaction');
+        like($txn->{_url}, qr{$rest_base_path/transaction/\d+$});
+    }
+
+    # Get next_page
+    $res = $mech->get($content->{next_page},
+        'Authorization' => $auth,
+    );
+    is($res->code, 200);
+
+    $content = $mech->json_response;
+    is($content->{count}, 5);
+    is($content->{page}, 2);
+    is($content->{per_page}, 10);
+    is($content->{next_page}, undef, 'No next_page');
+    is($content->{prev_page}, $history_pages_url . '&page=1');
+
+    is($content->{total}, undef, 'No total');
+    is(scalar @{$content->{items}}, 5);
+
+    for my $txn (@{ $content->{items} }) {
+        is($txn->{type}, 'transaction');
+        like($txn->{_url}, qr{$rest_base_path/transaction/\d+$});
+    }
+
+    # Back to prev_page
+    $res = $mech->get($content->{prev_page},
+        'Authorization' => $auth,
+    );
+    is($res->code, 200);
+
+    $content = $mech->json_response;
+    is($content->{count}, 9);
+    is($content->{page}, 1);
+    is($content->{per_page}, 10);
+    is($content->{prev_page}, undef, 'No prev_page');
+    is($content->{next_page}, $history_pages_url . '&page=2');
+
+    is($content->{total}, undef, 'No total');
+    is(scalar @{$content->{items}}, 9);
+
+    for my $txn (@{ $content->{items} }) {
+        is($txn->{type}, 'transaction');
+        like($txn->{_url}, qr{$rest_base_path/transaction/\d+$});
+    }
+}
+
 # Ticket Reply
 {
     # we know from earlier tests that look at hypermedia without ReplyToTicket