Bug 24487 - build_query_params helper builds path parameter with matching criteria
Summary: build_query_params helper builds path parameter with matching criteria
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P1 - high critical (vote)
Assignee: Agustín Moyano
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 24502
  Show dependency treegraph
 
Reported: 2020-01-22 23:41 UTC by Agustín Moyano
Modified: 2020-11-30 21:46 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05


Attachments
Bub 24487: Regresion test (1.96 KB, patch)
2020-01-22 23:46 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bub 24487: Regresion test (2.19 KB, patch)
2020-01-23 03:40 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24487: Regresion test (2.19 KB, patch)
2020-01-23 04:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24487: Don't apply matching criteria to path parameters (5.63 KB, patch)
2020-01-23 04:59 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24487: Add 2 more tests (2.05 KB, patch)
2020-01-23 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24487: Regresion test (2.25 KB, patch)
2020-01-28 13:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24487: Don't apply matching criteria to path parameters (5.69 KB, patch)
2020-01-28 13:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24487: Add 2 more tests (2.13 KB, patch)
2020-01-28 13:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24487: Regresion test (2.31 KB, patch)
2020-01-30 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24487: Don't apply matching criteria to path parameters (5.75 KB, patch)
2020-01-30 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24487: Add 2 more tests (2.19 KB, patch)
2020-01-30 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
[19.11.x] Bug 24487: Regresion test (2.63 KB, patch)
2020-04-16 21:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[19.11.x] Bug 24487: Don't apply matching criteria to path parameters (5.75 KB, patch)
2020-04-16 21:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[19.11.x] Bug 24487: Add 2 more tests (3.14 KB, patch)
2020-04-16 21:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[19.11.x] Bug 24487: Regresion test (2.70 KB, patch)
2020-04-17 10:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
[19.11.x] Bug 24487: Don't apply matching criteria to path parameters (5.81 KB, patch)
2020-04-17 10:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
[19.11.x] Bug 24487: Add 2 more tests (3.20 KB, patch)
2020-04-17 10:55 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Agustín Moyano 2020-01-22 23:41:15 UTC
build_query_params helper in package Koha::REST::Plugin::Query builds dbic parameters with matching criteria including path parameters.

Lets asume we have an endpoint like the following:

> /api/v1/public/patrons/:patron_id/holds

where we should get the list of holds of a patron.

Lets asume also that the controller uses object.search helper like this:

> my $holds_set = Koha::Holds->new;
> my $holds     = $c->objects->search( $holds_set );


If someone should call this endpoint like this:

> GET /api/v1/public/patrons/1/holds?_match=contains

they would get the holds not only of patron 1, but any patron that has 1 in there id.

This is because build_query_params treats path parameters like any other parameter, and when it builds dbic's filter parameters it returns

> {borrowernumber => {like => '%1%'}}

because of the 'contains' criteria.

Similar thing happens with 'starts_with' and 'ends_with' criteria.

I've search the api in master, and I could not find a place where we have path parameters and object.search helper at the same time. We've been blessed so far (not in a Perl way).

This is critical, because currently if there is a plugin that extends the API, and which controller uses object.search helper, anyone can use this exploit to fetch unauthorised data.
Comment 1 Agustín Moyano 2020-01-22 23:46:27 UTC
Created attachment 97765 [details] [review]
Bub 24487: Regresion test

This patch introduces a regresion test where a path parameter is combined with 'contains' match criteria

To test:
1. apply this patch
2. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Test should fail at this point.
Comment 2 Agustín Moyano 2020-01-23 03:40:12 UTC
Created attachment 97772 [details] [review]
Bub 24487: Regresion test

This patch introduces a regresion test where a path parameter is combined with 'contains' match criteria

To test:
1. apply this patch
2. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Test should fail at this point.
Comment 3 Agustín Moyano 2020-01-23 04:59:41 UTC
Created attachment 97774 [details] [review]
Bug 24487: Regresion test

This patch introduces a regresion test where a path parameter is combined with 'contains' match criteria

To test:
1. apply this patch
2. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Test should fail at this point.
Comment 4 Agustín Moyano 2020-01-23 04:59:45 UTC
Created attachment 97775 [details] [review]
Bug 24487: Don't apply matching criteria to path parameters

This patch separates query parameters from path parameters, and uses exact matching for the later.

To test:
1. Apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
SUCCESS => tests ok
3. Sign off
Comment 5 Jonathan Druart 2020-01-23 09:49:59 UTC
Created attachment 97782 [details] [review]
Bug 24487: Add 2 more tests

And make sure tests pass if there is no patron with borrowernumber=10 in
DB.
Comment 6 Tomás Cohen Arazi 2020-01-24 13:05:17 UTC
luck
Comment 7 Tomás Cohen Arazi 2020-01-24 13:06:24 UTC
Luckily, it would only affect plugins using objects.search for now, which hasn't been spotted in the wild.
Comment 8 Tomás Cohen Arazi 2020-01-28 13:56:40 UTC
Created attachment 98029 [details] [review]
Bug 24487: Regresion test

This patch introduces a regresion test where a path parameter is combined with 'contains' match criteria

To test:
1. apply this patch
2. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Test should fail at this point.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2020-01-28 13:56:43 UTC
Created attachment 98030 [details] [review]
Bug 24487: Don't apply matching criteria to path parameters

This patch separates query parameters from path parameters, and uses exact matching for the later.

To test:
1. Apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
SUCCESS => tests ok
3. Sign off

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2020-01-28 13:56:47 UTC
Created attachment 98031 [details] [review]
Bug 24487: Add 2 more tests

And make sure tests pass if there is no patron with borrowernumber=10 in
DB.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Ammended test description
Comment 11 Martin Renvoize 2020-01-30 14:57:43 UTC
Created attachment 98164 [details] [review]
Bug 24487: Regresion test

This patch introduces a regresion test where a path parameter is combined with 'contains' match criteria

To test:
1. apply this patch
2. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Test should fail at this point.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-01-30 14:57:47 UTC
Created attachment 98165 [details] [review]
Bug 24487: Don't apply matching criteria to path parameters

This patch separates query parameters from path parameters, and uses exact matching for the later.

To test:
1. Apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
SUCCESS => tests ok
3. Sign off

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-01-30 14:57:50 UTC
Created attachment 98166 [details] [review]
Bug 24487: Add 2 more tests

And make sure tests pass if there is no patron with borrowernumber=10 in
DB.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Ammended test description
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-01-30 14:58:47 UTC
Well done catching this early guys, works as expected and is well tested.. passing qa.
Comment 15 Martin Renvoize 2020-01-30 15:46:04 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 16 Joy Nelson 2020-02-10 23:25:17 UTC
Does not apply to 19.11.x 
Not backported
Comment 17 Tomás Cohen Arazi 2020-04-16 21:53:58 UTC
Created attachment 103105 [details] [review]
[19.11.x] Bug 24487: Regresion test

This patch introduces a regresion test where a path parameter is combined with 'contains' match criteria

To test:
1. apply this patch
2. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Test should fail at this point.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Tomás Cohen Arazi 2020-04-16 21:54:14 UTC
Created attachment 103106 [details] [review]
[19.11.x] Bug 24487: Don't apply matching criteria to path parameters

This patch separates query parameters from path parameters, and uses exact matching for the later.

To test:
1. Apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
SUCCESS => tests ok
3. Sign off

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Tomás Cohen Arazi 2020-04-16 21:54:27 UTC
Created attachment 103107 [details] [review]
[19.11.x] Bug 24487: Add 2 more tests

And make sure tests pass if there is no patron with borrowernumber=10 in
DB.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Ammended test description
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Kyle M Hall 2020-04-17 10:54:45 UTC
Created attachment 103142 [details] [review]
[19.11.x] Bug 24487: Regresion test

This patch introduces a regresion test where a path parameter is combined with 'contains' match criteria

To test:
1. apply this patch
2. prove t/db_dependent/Koha/REST/Plugin/Objects.t

Test should fail at this point.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2020-04-17 10:54:58 UTC
Created attachment 103143 [details] [review]
[19.11.x] Bug 24487: Don't apply matching criteria to path parameters

This patch separates query parameters from path parameters, and uses exact matching for the later.

To test:
1. Apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
SUCCESS => tests ok
3. Sign off

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2020-04-17 10:55:02 UTC
Created attachment 103144 [details] [review]
[19.11.x] Bug 24487: Add 2 more tests

And make sure tests pass if there is no patron with borrowernumber=10 in
DB.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Ammended test description
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Joy Nelson 2020-04-17 18:48:10 UTC
Thanks for the rebase!  Backported to 19.11.x branch for 19.11.05