[Bug 37779] New: Fix forms that POST without an op in tag moderation
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37779 Bug ID: 37779 Summary: Fix forms that POST without an op in tag moderation Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P3 Component: Tools Assignee: phil@chetcolibrary.org Reporter: phil@chetcolibrary.org QA Contact: testopia@bugs.koha-community.org Depends on: 36192 Blocks: 37728 We intend not to have forms with method="post" without an op variable (so we can check that the op starts with "cud-" as part of the CSRF protection), but because of bug 37728 some were missed. In tag moderation, there are two: the fallback form for testing whether a word has been approved or rejected when JavaScript is disabled so the normal AJAX code doesn't run (a brutal thing to write a test plan for), and the Filters form, which by POSTing prevents you from bookmarking the page for all reviews by a particular person, or all rejected tags, or anything else you might want to check without having to recreate the filter. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36192 [Bug 36192] [OMNIBUS] CSRF Protection for Koha https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37728 [Bug 37728] More "op" are missing in POSTed forms -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37779 --- Comment #1 from Phil Ringnalda <phil@chetcolibrary.org> --- Luckily, the no-JS fallback for test has only been broken for seven months, since a bug 34478 commit gave tags/review.pl actual op params with values to replace the params with names like "op-test" and a throwaway value that it used before, without noticing the submit button with the name op-test. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37779 Phil Ringnalda <phil@chetcolibrary.org> 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=37779 --- Comment #2 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 170919 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170919&action=edit Bug 37779: Fix forms that POST without an op in tag moderation We intend not to have forms with method="post" without an op variable (so we can check that the op starts with "cud-" as part of the CSRF protection), but because of bug 37728 some were missed. There are two in tags/review.tt: the filters for term, status, reviewer, and dates, which are better as a GET since you can then bookmark and link to a particular set of filters, and the no-JavaScript fallback for checking whether a term has been approved or rejected, which currently doesn't work at all, but with a working op param then works just fine as a GET. Test plan: If you have to use Chrome, you're on your own for the disabling JavaScript and getting rid of the body {display: none !important} style rule, my plan uses Firefox's devtools to do it 1. Without the patch, Tools - Tags - change the filter from the default status "pending" to "all", Apply, and bookmark the page 2. Open your bookmark, note that it's status "pending" 3. You can't test the no-JS fallback for term testing since it doesn't work, so apply patch and restart_all 4. Tools - Tags - change the filter from the default status "pending" to "all", Apply, and bookmark the page 5. Open your bookmark, note that it's status "all" 6. You need a couple of tags to test the Test feature, so open the OPAC, log in, search for any record and add the tags approveme, rejectme 7. Back in Tools - Tags, click the Reject button to reject rejectme 8. In the Check lists input, test that approveme shows "approveme is permitted!" and rejectme shows "rejectme is prohibited!" 8. Now to disable JavaScript, open Firefox's More tools - Web Developer Tools. You're going to need the Style Editor, so if it's not visible you'll want to enable it in Settings in the next step 9. Top bar, right side, there's a three-dots menu, with an option for Settings. In Advanced settings, click the checkbox for Disable JavaScript (which as hovering the * says, is only for that tab and only until you close the tab or the toolbox). 10. Checking Disable JavaScript turned your page to blank white, so go to the Style Editor, where the first thing, already selected, is an inline style sheet with the rule "body {display: none !important}" - select all the text of the rule and delete it, which you'll have to do on every page load 11. In the Check lists input, test that approveme still shows "approveme is permitted!" (after you delete the display: none !important rule, and with a rather annoying message about "Tested 1 Term(s)." replacing the list of terms). Sponsored-by: Chetco Community Public Library -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37779 Sukhmandeep <sukhmandeep.benipal@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #170919|0 |1 is obsolete| | --- Comment #3 from Sukhmandeep <sukhmandeep.benipal@inLibro.com> --- Created attachment 171214 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171214&action=edit Bug 37779: Fix forms that POST without an op in tag moderation We intend not to have forms with method="post" without an op variable (so we can check that the op starts with "cud-" as part of the CSRF protection), but because of bug 37728 some were missed. There are two in tags/review.tt: the filters for term, status, reviewer, and dates, which are better as a GET since you can then bookmark and link to a particular set of filters, and the no-JavaScript fallback for checking whether a term has been approved or rejected, which currently doesn't work at all, but with a working op param then works just fine as a GET. Test plan: If you have to use Chrome, you're on your own for the disabling JavaScript and getting rid of the body {display: none !important} style rule, my plan uses Firefox's devtools to do it 1. Without the patch, Tools - Tags - change the filter from the default status "pending" to "all", Apply, and bookmark the page 2. Open your bookmark, note that it's status "pending" 3. You can't test the no-JS fallback for term testing since it doesn't work, so apply patch and restart_all 4. Tools - Tags - change the filter from the default status "pending" to "all", Apply, and bookmark the page 5. Open your bookmark, note that it's status "all" 6. You need a couple of tags to test the Test feature, so open the OPAC, log in, search for any record and add the tags approveme, rejectme 7. Back in Tools - Tags, click the Reject button to reject rejectme 8. In the Check lists input, test that approveme shows "approveme is permitted!" and rejectme shows "rejectme is prohibited!" 8. Now to disable JavaScript, open Firefox's More tools - Web Developer Tools. You're going to need the Style Editor, so if it's not visible you'll want to enable it in Settings in the next step 9. Top bar, right side, there's a three-dots menu, with an option for Settings. In Advanced settings, click the checkbox for Disable JavaScript (which as hovering the * says, is only for that tab and only until you close the tab or the toolbox). 10. Checking Disable JavaScript turned your page to blank white, so go to the Style Editor, where the first thing, already selected, is an inline style sheet with the rule "body {display: none !important}" - select all the text of the rule and delete it, which you'll have to do on every page load 11. In the Check lists input, test that approveme still shows "approveme is permitted!" (after you delete the display: none !important rule, and with a rather annoying message about "Tested 1 Term(s)." replacing the list of terms). Sponsored-by: Chetco Community Public Library Signed-off-by: Sukhmandeep Benipal <sukhmandeep.benipal@inLibro.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37779 Sukhmandeep <sukhmandeep.benipal@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sukhmandeep.benipal@inLibro | |.com 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=37779 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37779 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171214|0 |1 is obsolete| | --- Comment #4 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 171333 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171333&action=edit Bug 37779: Fix forms that POST without an op in tag moderation We intend not to have forms with method="post" without an op variable (so we can check that the op starts with "cud-" as part of the CSRF protection), but because of bug 37728 some were missed. There are two in tags/review.tt: the filters for term, status, reviewer, and dates, which are better as a GET since you can then bookmark and link to a particular set of filters, and the no-JavaScript fallback for checking whether a term has been approved or rejected, which currently doesn't work at all, but with a working op param then works just fine as a GET. Test plan: If you have to use Chrome, you're on your own for the disabling JavaScript and getting rid of the body {display: none !important} style rule, my plan uses Firefox's devtools to do it 1. Without the patch, Tools - Tags - change the filter from the default status "pending" to "all", Apply, and bookmark the page 2. Open your bookmark, note that it's status "pending" 3. You can't test the no-JS fallback for term testing since it doesn't work, so apply patch and restart_all 4. Tools - Tags - change the filter from the default status "pending" to "all", Apply, and bookmark the page 5. Open your bookmark, note that it's status "all" 6. You need a couple of tags to test the Test feature, so open the OPAC, log in, search for any record and add the tags approveme, rejectme 7. Back in Tools - Tags, click the Reject button to reject rejectme 8. In the Check lists input, test that approveme shows "approveme is permitted!" and rejectme shows "rejectme is prohibited!" 8. Now to disable JavaScript, open Firefox's More tools - Web Developer Tools. You're going to need the Style Editor, so if it's not visible you'll want to enable it in Settings in the next step 9. Top bar, right side, there's a three-dots menu, with an option for Settings. In Advanced settings, click the checkbox for Disable JavaScript (which as hovering the * says, is only for that tab and only until you close the tab or the toolbox). 10. Checking Disable JavaScript turned your page to blank white, so go to the Style Editor, where the first thing, already selected, is an inline style sheet with the rule "body {display: none !important}" - select all the text of the rule and delete it, which you'll have to do on every page load 11. In the Check lists input, test that approveme still shows "approveme is permitted!" (after you delete the display: none !important rule, and with a rather annoying message about "Tested 1 Term(s)." replacing the list of terms). Sponsored-by: Chetco Community Public Library Signed-off-by: Sukhmandeep Benipal <sukhmandeep.benipal@inLibro.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37779 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37779 --- Comment #5 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37779 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to main |RESOLVED CC| |lucas@bywatersolutions.com --- Comment #6 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Doesn't apply cleanly in 24.05.x, please rebase if needed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37779 Bug 37779 depends on bug 36192, which changed state. Bug 36192 Summary: [OMNIBUS] CSRF Protection for Koha https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36192 What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon@bugs.koha-community.org