Bug 35129 - REST API: _per_page=0 crashes on Illegal division by zero
Summary: REST API: _per_page=0 crashes on Illegal division by zero
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-10-23 12:11 UTC by Marcel de Rooy
Modified: 2024-04-04 13:25 UTC (History)
1 user (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


Attachments
Bug 35129: Regression tests (1.15 KB, patch)
2024-03-18 15:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35129: Return 400 if _per_page=0 passed (1.42 KB, patch)
2024-03-18 15:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35129: Regression tests (1.22 KB, patch)
2024-03-21 15:46 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 35129: Return 400 if _per_page=0 passed (1.49 KB, patch)
2024-03-21 15:46 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 35129: Regression tests (1.27 KB, patch)
2024-04-01 17:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35129: Return 400 if _per_page=0 passed (1.54 KB, patch)
2024-04-01 17:50 UTC, Nick Clemens
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 2023-10-23 12:11:42 UTC
From IRC:

[14:08] <marcelr> Nice /api/v1/patrons?_page=1&_per_page=0 just returns me a 500 (ISE)
[14:09] <marcelr> not tested yet :)
[14:10] <marcelr> [ERROR] GET /api/v1/patrons: unhandled exception (Mojo::Exception)<<Illegal division by zero at /usr/share/koha/Koha/REST/Plugin/Pagination.pm line 106.>>
Comment 1 Marcel de Rooy 2023-10-23 12:33:13 UTC
To prohibit _per_page=0 we could use OpenAPI 3.0 with

not:
    const: 0
Comment 2 Marcel de Rooy 2023-10-23 12:48:59 UTC
Sorry no quick simple solution found yet. Take a try!
Comment 3 Tomás Cohen Arazi 2024-03-18 15:40:02 UTC
Nice catch.
Comment 4 Tomás Cohen Arazi 2024-03-18 15:53:55 UTC
Created attachment 163342 [details] [review]
Bug 35129: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2024-03-18 15:53:58 UTC
Created attachment 163343 [details] [review]
Bug 35129: Return 400 if _per_page=0 passed

This patch adds a safe guard for when consumers pass _per_page=0 to
endpoints. This condition is checked for on a centralized place and
avoid reaching the controller in such scenarios that would provoke a
division by zero exception.

To test:
1. Apply the regression tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/pagination.t
=> FAIL: We expect a 400, but get a 500 instead
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! No more explosions for this!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Laura Escamilla 2024-03-21 15:46:21 UTC
Created attachment 163637 [details] [review]
Bug 35129: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Comment 7 Laura Escamilla 2024-03-21 15:46:32 UTC
Created attachment 163638 [details] [review]
Bug 35129: Return 400 if _per_page=0 passed

This patch adds a safe guard for when consumers pass _per_page=0 to
endpoints. This condition is checked for on a centralized place and
avoid reaching the controller in such scenarios that would provoke a
division by zero exception.

To test:
1. Apply the regression tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/pagination.t
=> FAIL: We expect a 400, but get a 500 instead
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! No more explosions for this!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Comment 8 Nick Clemens 2024-04-01 17:50:17 UTC
Created attachment 164222 [details] [review]
Bug 35129: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2024-04-01 17:50:19 UTC
Created attachment 164223 [details] [review]
Bug 35129: Return 400 if _per_page=0 passed

This patch adds a safe guard for when consumers pass _per_page=0 to
endpoints. This condition is checked for on a centralized place and
avoid reaching the controller in such scenarios that would provoke a
division by zero exception.

To test:
1. Apply the regression tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/pagination.t
=> FAIL: We expect a 400, but get a 500 instead
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! No more explosions for this!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Katrin Fischer 2024-04-04 13:25:17 UTC
Pushed for 24.05!

Well done everyone, thank you!