Bug 35129

Summary: REST API: _per_page=0 crashes on Illegal division by zero
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: Pushed to oldstable --- QA Contact:
Severity: normal    
Priority: P5 - low CC: fridolin.somers, lucas, tomascohen
Version: Main   
Hardware: All   
OS: All   
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,23.05.12
Circulation function:
Attachments: Bug 35129: Regression tests
Bug 35129: Return 400 if _per_page=0 passed
Bug 35129: Regression tests
Bug 35129: Return 400 if _per_page=0 passed
Bug 35129: Regression tests
Bug 35129: Return 400 if _per_page=0 passed

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 (kidclamp) 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 (kidclamp) 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!
Comment 11 Fridolin Somers 2024-05-23 09:23:36 UTC
Pushed to 23.11.x for 23.11.06
Comment 12 Lucas Gass 2024-05-28 16:48:49 UTC
Backported to 23.05.x for upcoming 23.05.12