[Bug 42585] New: Add ability for Koha to analyze reports and warn for potentially dangerous reports
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Bug ID: 42585 Summary: Add ability for Koha to analyze reports and warn for potentially dangerous reports Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Reports Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: lisette@bywatersolutions.com It is quite possible to write reports in Koha that, when run, create huge temp tables that crash a database server or cause runreport.pl to use so much RAM that the kernel oom-kills Koha. It would be nice if we had a tool built in to Koha to help warn librarians about reports with the potential to bring down a Koha server. -- 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=42585 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=42585 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |phil@chetcolibrary.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #1 from David Cook <dcook@prosentient.com.au> --- I thought that there was a report open for this already but I can't find it with a quick search. Maybe I've just talked with people about this. Anyway, I think it's a good idea. And there's some options. The first one I was thinking of was doing EXPLAIN queries and seeing what the query planner thinks of the query. Another option (which would probably be best served by plugins) would be to have a LLM take a look at it. I've pointed AIs at Koha SQL queries in the past to see how well they would do, and it seemed like they did pretty good. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #2 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- (In reply to David Cook from comment #1)
I thought that there was a report open for this already but I can't find it with a quick search. Maybe I've just talked with people about this. Anyway, I think it's a good idea.
And there's some options. The first one I was thinking of was doing EXPLAIN queries and seeing what the query planner thinks of the query.
Yes! I've code something just nearing completion and it includes EXPLAIN and some static analysis tools.
Another option (which would probably be best served by plugins) would be to have a LLM take a look at it. I've pointed AIs at Koha SQL queries in the past to see how well they would do, and it seemed like they did pretty good.
This is actually where I started before I veered off into explain and static analysis. I've found https://github.com/mlc-ai/web-llm which is intriguing. I have no idea if it's practical yet, but if it is, that will be a followup bug. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 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=42585 Angela Berrett <angela.berrett@familysearch.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |angela.berrett@familysearch | |.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 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=42585 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|42643 | CC| |andrew@bywatersolutions.com 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=42585 --- Comment #3 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- FYI, I've got something I need to post up, but I've been re-mashing it this way and that to make it as grokkable as possible. I'll get it up asap! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 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=42585 --- Comment #4 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203792 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203792&action=edit Bug 42585: Add analyzer infrastructure Librarians write saved SQL reports that pull all of items, sort the whole borrowers table, or buffer millions of rows into the Plack worker. There's no warning. They click Run, the database or the app server falls over, and the report never finishes. This patch adds the runner, the analysis context, the check base class and the registry every check will hang off, along with the placeholder, EXPLAIN and server statistics helpers. No checks are registered yet, they come in the following patches. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/ t/db_dependent/Koha/Reports/Analyzer/ 3) In a koha-shell, run: perl -MKoha::Reports::Analyzer=analyze -MData::Dumper -e \ 'print Dumper analyze({ sql => "SELECT 1" })' 4) Note the empty findings list, the runner works but no checks are registered yet! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #5 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203793 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203793&action=edit Bug 42585: Add safety analyzers Koha won't run a saved report that fails its is_sql_valid whitelist, but the librarian doesn't find out until they click Run and the report errors out. This patch adds three checks that surface the same problems while the SQL is being written: forbidden_statement, missing_select and forbidden_column. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/Safety/ \ t/db_dependent/Koha/Reports/Analyzer.t 3) In a koha-shell, run: perl -MKoha::Reports::Analyzer=analyze -MData::Dumper -e \ 'print Dumper analyze({ sql => "UPDATE borrowers SET surname=1" })' 4) Note the forbidden_statement finding with severity high! 5) Repeat with "SELECT password FROM borrowers", note forbidden_column! 6) Repeat with "SELECT borrowernumber FROM borrowers LIMIT 1", note the empty findings list! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #6 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203794 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203794&action=edit Bug 42585: Add REST API for report analysis This patch adds POST /api/v1/reports/analyze, which runs the analyzer over the given SQL and returns the severity and findings as JSON. It takes the SQL and any placeholder parameters, and is guarded by the reports.create_reports permission. Test Plan: 1) Apply this patch 2) yarn api:bundle 3) Restart all the things! 4) prove t/db_dependent/api/v1/reports_analyze.t 5) Run: curl -s -X POST \ "http://koha:koha@kohadev-intra.localhost/api/v1/reports/analyze" \ -H "Content-Type: application/json" \ -d '{"sql":"UPDATE borrowers SET surname=1"}' | jq . 6) Note the forbidden_statement finding with severity high! 7) Repeat with "SELECT borrowernumber FROM borrowers LIMIT 1", note the empty findings list and a null severity! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #7 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203795 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203795&action=edit Bug 42585: Add Analyze button to the reports editor This patch adds an Analyze button to the SQL editor on guided_reports.pl. Clicking it opens a modal that prompts for any placeholder values, calls the REST endpoint and lists the findings, so a librarian can check a report before saving or running it. Placeholder dropdowns are filled from the matching lookup table for the common placeholder types (branches, categorycode, itemtypes) and from the authorised values for the rest, so the librarian picks a real value instead of typing a code. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Create a new report with the SQL "SELECT barcode FROM items" 4) Note the new Analyze button beside "Save report"! 5) Click it, then click "Run analysis", note the empty findings list! 6) Change the SQL to "UPDATE borrowers SET surname='X' WHERE 1=1" and analyze again, note the high severity forbidden_statement finding! 7) Change the SQL to "SELECT * FROM borrowers WHERE branchcode = <<Branch|branches>>" and analyze again, note the modal prompts for a branch with a dropdown of your real branches! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #8 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203796 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203796&action=edit Bug 42585: Add static correctness analyzers This patch adds five checks for SQL that's wrong no matter how much data is in the database: * cartesian_product, comma separated FROM with no WHERE * join_without_on, a JOIN missing its ON or USING * sleep_call, SLEEP() in a saved report * select_for_update, FOR UPDATE or LOCK IN SHARE MODE * having_without_group_by, HAVING with no GROUP BY None of them are scale dependent, so the runner never suppresses them on a small database. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/Static/ 3) Analyze a report for each of these: * SELECT * FROM borrowers, branches * SELECT * FROM borrowers JOIN branches * SELECT SLEEP(5) * SELECT * FROM borrowers FOR UPDATE * SELECT COUNT(*) FROM borrowers HAVING COUNT(*) > 5 4) Note each one gets the matching finding! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #9 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203797 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203797&action=edit Bug 42585: Add static performance analyzers This patch adds ten checks for SQL that's only a problem once there's real data behind it: order_by_rand, many_joins_no_where, order_by_no_limit_no_where, group_by_no_where, not_in_subquery, group_concat_no_where, leading_wildcard_like, correlated_subquery, union_not_all and function_on_column_in_where. All ten are scale dependent, so the runner drops them when every table the EXPLAIN plan touches is small. Dev instances stay quiet, production instances get the warning. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/Static/ 3) Analyze a report with the SQL "SELECT * FROM borrowers ORDER BY RAND() LIMIT 1" 4) On a dev database with few borrowers, note the finding is suppressed! 5) Load enough borrowers that the table isn't small any more and analyze again, note the order_by_rand finding now appears! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #10 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203798 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203798&action=edit Bug 42585: Add EXPLAIN-based analyzers This patch adds five checks that read the EXPLAIN plan instead of the SQL text: * large_temp_table_risk, a temp table bigger than the server's in memory cap, so it spills to disk * uses_join_buffer, the planner picked a Block Nested Loop because there's no index on the join column * dependent_subquery, the planner expects to run the subquery again for every outer row * full_table_scan, an access type of ALL on a table that isn't small * low_filter_efficiency, a full scan that keeps less than 10% of the rows it reads Scans of small lookup tables like branches and authorised_values are suppressed. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/Explain/ 3) Analyze a report with the SQL "SELECT b.*, ( SELECT COUNT(*) FROM issues i WHERE i.borrowernumber = b.borrowernumber ) FROM borrowers b" 4) Note the dependent_subquery finding! 5) Analyze a report with the SQL "SELECT borrowernumber FROM borrowers WHERE borrowernumber = 1" 6) Note there are no EXPLAIN findings at all! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #11 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203799 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203799&action=edit Bug 42585: Add schema-aware and meta analyzers This patch adds the last two checks: * select_star_on_large_payload_table, a wildcard SELECT whose plan touches one of Koha's big payload tables such as biblio_metadata or action_logs, dragging every row's full payload through the database, Plack and the download * explain_failed, emitted when EXPLAIN itself errored, with the trimmed database error in the message Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/SchemaAware/ \ t/Koha/Reports/Analyzer/Check/Meta/ 3) On an instance with a real sized catalog, analyze a report with the SQL "SELECT * FROM biblio_metadata" 4) Note the select_star_on_large_payload_table finding! 5) Analyze a report with the SQL "SELECT no_such_column FROM borrowers" 6) Note the explain_failed finding with the database error in it! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #12 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- (In reply to Kyle M Hall (khall) from comment #10)
3) Analyze a report with the SQL "SELECT b.*, ( SELECT COUNT(*) FROM issues i WHERE i.borrowernumber = b.borrowernumber ) FROM borrowers b" 4) Note the dependent_subquery finding!
We did not see any findings on this query. Failing QA for this. On your tests that depend on size of collection to determine whether or not to give an error, we still see the error when limiting the number of results with LIMIT or WHERE. It would be good for the analyzer to reflect all limitations on what can be saved in a query -- one cannot save a report that returns a password field, but the analyzer doesn't warn on it (while the analyzer does warn on DELETE or UPDATE) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43352 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43352 [Bug 43352] Optionally analyze all reports at creation and update and block saving of reports with warnings -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org