[Bug 42373] New: Add ability to disallow unbounded patron api searches
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Bug ID: 42373 Summary: Add ability to disallow unbounded patron api searches Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: tomascohen@gmail.com In a Koha instance with multiple hundreds of thousands of patrons, a patron api search with _per_page=-1 can cause a single worker to query the database for all patrons, then query the database multiple times for each of those patrons to decorate the resulting patrons ( fees owed, etc ). If the _per_page size is greater than the limit we specify, Koha should refuse the request and emit an http status code 400, with a clear error body explaining the max allowed _per_page value. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #1 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197512 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197512&action=edit Bug 42373: Enforce RESTMaxPatronsPageSize in patron list endpoint In a Koha instance with hundreds of thousands of patrons, an API request to /api/v1/patrons with _per_page=-1 can cause a single worker to query the database for all patrons, then make multiple additional queries per patron for decoration (fees owed, etc), effectively locking up that worker. This patch adds a new system preference RESTMaxPatronsPageSize. When set to a positive integer, requests to the /patrons list endpoint with _per_page=-1 or _per_page greater than the configured limit will be refused with HTTP 400 and error_code 'max_page_size_exceeded'. When left empty or set to 0, the existing behavior is preserved. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Go to Administration > System preferences > Web services > REST API 4) Note the new RESTMaxPatronsPageSize preference 5) Leave RESTMaxPatronsPageSize empty 6) Make an API request: GET /api/v1/patrons?_per_page=-1 7) Note the request succeeds as before! 8) Set RESTMaxPatronsPageSize to 100 9) Make an API request: GET /api/v1/patrons?_per_page=-1 10) Note the request is rejected with a 400 and a clear error message! 11) Make an API request: GET /api/v1/patrons?_per_page=500 12) Note the request is rejected with a 400! 13) Make an API request: GET /api/v1/patrons?_per_page=50 14) Note the request succeeds! 15) Make an API request: GET /api/v1/patrons?_per_page=100 16) Note the request succeeds! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #2 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197513 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197513&action=edit Bug 42373: Update database Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #3 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197514 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197514&action=edit Bug 42373: Add unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197512|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197513|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197514|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #4 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197516 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197516&action=edit Bug 42373: Enforce RESTMaxPatronsPageSize in patron list endpoint In a Koha instance with hundreds of thousands of patrons, an API request to /api/v1/patrons with _per_page=-1 can cause a single worker to query the database for all patrons, then make multiple additional queries per patron for decoration (fees owed, etc), effectively locking up that worker. This patch adds a new system preference RESTMaxPatronsPageSize. When set to a positive integer, requests to the /patrons list endpoint with _per_page=-1 or _per_page greater than the configured limit will be refused with HTTP 400 and error_code 'max_page_size_exceeded'. When left empty or set to 0, the existing behavior is preserved. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Go to Administration > System preferences > Web services > REST API 4) Note the new RESTMaxPatronsPageSize preference 5) Leave RESTMaxPatronsPageSize empty 6) Make an API request: GET /api/v1/patrons?_per_page=-1 7) Note the request succeeds as before! 8) Set RESTMaxPatronsPageSize to 100 9) Make an API request: GET /api/v1/patrons?_per_page=-1 10) Note the request is rejected with a 400 and a clear error message! 11) Make an API request: GET /api/v1/patrons?_per_page=500 12) Note the request is rejected with a 400! 13) Make an API request: GET /api/v1/patrons?_per_page=50 14) Note the request succeeds! 15) Make an API request: GET /api/v1/patrons?_per_page=100 16) Note the request succeeds! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #5 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197517 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197517&action=edit Bug 42373: Update database Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #6 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197518 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197518&action=edit Bug 42373: Add unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #7 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197525 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197525&action=edit Bug 42373: Don't show invalid options in the show entries pulldown Patch from commit 3d01888 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Status|Needs Signoff |Patch doesn't apply --- Comment #8 from David Nind <david@davidnind.com> --- I'm getting a sha1/fake ancestor error when trying to apply the patch: git bz apply 42373 📋 Bug 42373 - Add ability to disallow unbounded patron api searches • 197516 - Bug 42373: Enforce RESTMaxPatronsPageSize in patron list endpoint • 197517 - Bug 42373: Update database • 197518 - Bug 42373: Add unit tests • 197525 - Bug 42373: Don't show invalid options in the show entries pulldown Apply? [(y)es, (n)o, (i)nteractive] y Preparing 4 patch(es): ✓ Prepared 4 patch(es) Applying 4 patch(es): ✓ [1/4] Applied Bug 42373: Enforce RESTMaxPatronsPageSize in patron list en… ✓ [2/4] Applied Bug 42373: Update database ✓ [3/4] Applied Bug 42373: Add unit tests Patch application failed for attachment 197525 - Bug 42373: Don't show invalid options in the show entries pulldown Patches left in /tmp/Io01InWa6i for manual application if needed To resolve: 1. Fix conflicts (use 'git mergetool' or edit files manually) 2. Stage resolved files with 'git add' 3. Continue with 'git bz apply --continue' or 'git am --continue' 4. Or skip this patch with 'git bz apply --skip' 5. Or abort with 'git bz apply --abort' Error: Apply failed: Git command (git am -3 /tmp/Io01InWa6i/0004-197525.patch) failed: error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197525|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #9 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197655 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197655&action=edit Bug 42373: Don't show invalid values in the patron search Patch from commit 832d5b7 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #10 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Fixed! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42410 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42410 [Bug 42410] All patron-centric APIs should honor RESTMaxPatronsPageSize -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197516|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197517|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197518|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197655|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #11 from David Nind <david@davidnind.com> --- Created attachment 197666 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197666&action=edit Bug 42373: Enforce RESTMaxPatronsPageSize in patron list endpoint In a Koha instance with hundreds of thousands of patrons, an API request to /api/v1/patrons with _per_page=-1 can cause a single worker to query the database for all patrons, then make multiple additional queries per patron for decoration (fees owed, etc), effectively locking up that worker. This patch adds a new system preference RESTMaxPatronsPageSize. When set to a positive integer, requests to the /patrons list endpoint with _per_page=-1 or _per_page greater than the configured limit will be refused with HTTP 400 and error_code 'max_page_size_exceeded'. When left empty or set to 0, the existing behavior is preserved. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Go to Administration > System preferences > Web services > REST API 4) Note the new RESTMaxPatronsPageSize preference 5) Leave RESTMaxPatronsPageSize empty 6) Make an API request: GET /api/v1/patrons?_per_page=-1 7) Note the request succeeds as before! 8) Set RESTMaxPatronsPageSize to 100 9) Make an API request: GET /api/v1/patrons?_per_page=-1 10) Note the request is rejected with a 400 and a clear error message! 11) Make an API request: GET /api/v1/patrons?_per_page=500 12) Note the request is rejected with a 400! 13) Make an API request: GET /api/v1/patrons?_per_page=50 14) Note the request succeeds! 15) Make an API request: GET /api/v1/patrons?_per_page=100 16) Note the request succeeds! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #12 from David Nind <david@davidnind.com> --- Created attachment 197667 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197667&action=edit Bug 42373: Update database Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #13 from David Nind <david@davidnind.com> --- Created attachment 197668 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197668&action=edit Bug 42373: Add unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #14 from David Nind <david@davidnind.com> --- Created attachment 197669 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197669&action=edit Bug 42373: Don't show invalid values in the patron search Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 --- Comment #15 from David Nind <david@davidnind.com> --- Testing notes (using KTD and Bruno for API requests): 1. Did a yarn build as well, not sure if that is required or not. 2. Step 7. JSON returned, 4136 lines 3. Steps 10 and 12. 400 Bad Request returned: { "error": "_per_page exceeds the maximum allowed value of 100 for this endpoint", "error_code": "max_page_size_exceeded" } 4. Step 14. JSON returned, 3902 lines, patron ids 1 to 50 5. Step 16. JSON returned, 4136 lines, patron ids 1 to 53 6. Tests pass: prove t/db_dependent/api/v1/patrons.t Note: qa script says: Processing additional checks * Author 'John Doe' seems invalid * Email's author 'you@example.com' seems invalid -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Add ability to disallow |Add ability to disallow |unbounded patron api |unbounded patron API |searches |searches -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com Status|Signed Off |In Discussion --- Comment #16 from Jonathan Druart <jonathan.druart@gmail.com> --- I don't think a specific pref is a good idea. We should offer a flexible way to address the "max page size" need for the REST API. We could have a single RESTMaxPageSize pref (applied to all endpoints), or consider a JSON pref {patrons: 100, biblios: 50}. It will also helps to adjust the menuLength in a generic way (in datatables.js). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42643 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42643 [Bug 42643] [OMNIBUS] Assorted performance and stability work -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43016 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43016 [Bug 43016] [OMNIBUS] Server resource protection -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com Blocks|42643 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42643 [Bug 42643] [OMNIBUS] Assorted performance and stability work -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Add ability to disallow |Add ability to disallow |unbounded patron API |unbounded patron API |searches |searches via API -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|kyle@bywatersolutions.com |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42373 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |zoho.roboto@bywatersolution | |s.com -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org