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.
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.
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.
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.
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
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.
luck
Luckily, it would only affect plugins using objects.search for now, which hasn't been spotted in the wild.
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>
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>
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
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>
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>
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>
Well done catching this early guys, works as expected and is well tested.. passing qa.
Nice work everyone! Pushed to master for 20.05
Does not apply to 19.11.x Not backported
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>
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>
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>
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>
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>
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>
Thanks for the rebase! Backported to 19.11.x branch for 19.11.05