Bug 36858 - Crash on wrong page number in opac-shelves
Summary: Crash on wrong page number in opac-shelves
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-14 12:01 UTC by Marcel de Rooy
Modified: 2024-05-31 13:22 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.06,24.05.12


Attachments
Bug 36858: Check page parameter for virtual shelves (3.53 KB, patch)
2024-05-21 14:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36858: Check page parameter for virtual shelves (3.54 KB, patch)
2024-05-22 00:44 UTC, David Nind
Details | Diff | Splinter Review
Bug 36858: Check page parameter for virtual shelves (3.61 KB, patch)
2024-05-22 07:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36858: Remove warnings (1.67 KB, patch)
2024-05-22 07:29 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2024-05-14 12:01:10 UTC
[WARN] No method pager found for Koha::Virtualshelves DBIx::Class::ResultSet::pager(): Invalid page number (page-numbers are 1-based) at /usr/share/koha/Koha/Objects.pm line 578
 at /usr/share/koha/opac/opac-shelves.pl line 477.

L477 my $pager = $shelves->pager;
L578 (Objects AUTOLOAD) my $r = eval { $self->_resultset->$method(@params) }; 
Returns a DBIx::Class::ResultSet::Pager object tied to the current resultset. Requires the page attribute to have been previously set on the resultset object, usually via a call to "page".

Looking at our logs, people are sending weird parameters to opac-shelves including non-numeric page numbers in 'page'.
Koha should probably not crash on that?
Comment 1 Jonathan Druart 2024-05-21 10:49:54 UTC
Looks like an attack? Trying to SQL inject something?
Comment 2 Jonathan Druart 2024-05-21 10:50:27 UTC
When passing non-integer ("x") to page I am getting "A supplied offset attribute must be a non-negative integer"
Comment 3 Marcel de Rooy 2024-05-21 13:43:13 UTC
(In reply to Jonathan Druart from comment #1)
> Looks like an attack? Trying to SQL inject something?

It does not seem to be a focused SQL injection but looked like a 'TestBuilder' generated URL with random values for known and unknown parameters.
But scrolling thru these logs never makes you happy ;)
Comment 4 Marcel de Rooy 2024-05-21 13:44:11 UTC
(In reply to Jonathan Druart from comment #2)
> When passing non-integer ("x") to page I am getting "A supplied offset
> attribute must be a non-negative integer"

Related crash:
PLACK-ERROR.LOG Can't call method "last_page" on an undefined value at /usr/share/koha/opac/opac-shelves.pl line 478
DBIx::Class::Storage::DBI::_select_args(): A supplied offset attribute must be a non-negative integer at /usr/share/koha/Koha/Objects.pm line 312
Comment 5 Marcel de Rooy 2024-05-21 14:38:38 UTC
(In reply to Jonathan Druart from comment #2)
> When passing non-integer ("x") to page I am getting "A supplied offset
> attribute must be a non-negative integer"

Yes, cant reproduce this invalid pager crash on master. Note that it comes from a 22.11 log, but that does still not explain it.
Comment 6 Marcel de Rooy 2024-05-21 14:39:54 UTC
Created attachment 166978 [details] [review]
Bug 36858: Check page parameter for virtual shelves

Test plan:
Try passing page=x now and verify different behavior (no 500).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 David Nind 2024-05-22 00:44:45 UTC
Created attachment 167002 [details] [review]
Bug 36858: Check page parameter for virtual shelves

Test plan:
Try passing page=x now and verify different behavior (no 500).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2024-05-22 00:46:10 UTC
Testing notes (using KTD):
1. Go to <opac-url>/cgi-bin/koha/opac-shelves.pl?op=list&public=0&page=X 
(or got to OPAC > Lists > Your lists, then add &page=x to the URL (no need to log in with an account))
2. You get an error trace: Can't call method "last_page" on an undefined value at /kohadevbox/koha/opac/opac-shelves.pl line 484 ......
3.Get an error in the /var/log/koha/kohadev/plack-opac-error.log log file: 
  [2024/05/22 00:33:44] [WARN] Argument "x" isn't numeric in numeric le (<=) at /usr/share/perl5/DBIx/Class/ResultSet.pm line 2523.
  2024/05/22 00:33:44] [WARN] No method pager found for Koha::Virtualshelves DBIx::Class::ResultSet::pager(): Invalid page number (page-numbers are 1-based) at /kohadevbox/koha/Koha/Objects.pm line 585 at /kohadevbox/koha/opac/opac-shelves.pl line 483.
4. Apply the patch.
5. Repeat step 1:
   ==> Warning in plack-opac-error.log: [2024/05/22 00:40:54] [WARN] Argument "x" isn't numeric in int at /kohadevbox/koha/opac/opac-shelves.pl line 90.
   ==> OPAC result: No error message, stays on the page with Lists (tabs with Your lists + Public lists, and a link to "Log in to create a new list")
Comment 9 Jonathan Druart 2024-05-22 07:29:25 UTC
Created attachment 167003 [details] [review]
Bug 36858: Check page parameter for virtual shelves

Test plan:
Try passing page=x now and verify different behavior (no 500).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2024-05-22 07:29:28 UTC
Created attachment 167004 [details] [review]
Bug 36858: Remove warnings

Argument "" isn't numeric in int

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2024-05-22 07:38:02 UTC
Removed another warning on bug 36914
Comment 12 Katrin Fischer 2024-05-22 14:20:13 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 13 Fridolin Somers 2024-05-29 14:14:34 UTC
Pushed to 23.11.x for 23.11.06
Comment 14 Lucas Gass 2024-05-31 13:22:23 UTC
Backported to 23.05.x for upcoming 23.05.12