[Bug 37783] New: Fix form that looks like it would POST without an op in reserve/request.tt
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37783 Bug ID: 37783 Summary: Fix form that looks like it would POST without an op in reserve/request.tt Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P3 Component: Circulation Assignee: phil@chetcolibrary.org Reporter: phil@chetcolibrary.org QA Contact: testopia@bugs.koha-community.org CC: dcook@prosentient.com.au, gmcharlt@gmail.com, jonathan.druart@gmail.com, kyle.m.hall@gmail.com, oleonard@myacpl.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. One that the patched xt/find-missing-op-in-forms.t points at is the rather odd https://git.koha-community.org/Koha-community/Koha/src/commit/7342209b34d642... which isn't actually a standalone form: if you could submit it any way other than after the JavaScript at #L1851 alters it, it wouldn't actually do anything since it lacks both the op cud-cancel and also the reserve_id that it has to have to cancel a reserve. Assuming there's no real reason to have the JavaScript add the op, we could put it immediately after the [% INCLUDE 'csrf-token.inc' %] where it would usually be, but the funnier alternative is to put it inside the <div id="inputs"> for the test to see, and then since the first thing the JavaScript does is to .empty() that div before it puts it back in, the behavior would be unchanged but the test (and someone casually scanning the code) could see what the op will be without having to know that it will be deleted and then added right back. 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=37783 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=37783 --- Comment #1 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 170895 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170895&action=edit Bug 37783: Fix form that looks like it would POST without an op in reserve/request.tt 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 reserve/request.tt the modal for cancelling a hold looks like it is a form that will do a POST without an op input, but in fact it requires JavaScript to work at all, and with JavaScript it clears out the div where it stashes inputs and then inserts one with the op cud-cancel. To persuade the test at xt/find-missing-op-in-forms.t that there is an op, and to let a casual skimmer of the code see what that op will be, without actually changing the behavior in any way, we can just stick the op in the div which the JS will .empty() out before sticking the same thing back in. Test plan: 1. Search for any record with an item, click Place hold, place two holds 2. In the row for the second hold, click the trash can icon to delete 3. Nothing changed from normallly cancelling a hold, did it? It shouldn't have. 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=37783 --- Comment #2 from Jonathan Druart <jonathan.druart@gmail.com> --- This is not great and I was going to sign it off as it could be a quick solution for now. However it's buggy (and is ofc before your patch). If you select 2 holds then click on the cancel link (for the single line then), the modal is going to delete the 2 holds ("This action will cancel 2 hold(s)"). I let you decide if you want to fix this along with this patch. IMO if we fix the bug we will certainly -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37783 --- Comment #3 from Jonathan Druart <jonathan.druart@gmail.com> --- Outch, actually it displays "2 holds will be cancelled", but only one is cancelled. Also if you click "select all", then uncheck and click the cancel (single), the modal shows "This action will cancel 0 hold(s)". The way we deal with this message is totally broken. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37783 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37817 --- Comment #4 from Jonathan Druart <jonathan.druart@gmail.com> --- Created bug 37817. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37817 [Bug 37817] Cancellation holds alert not displayed the correct number of holds -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37783 Jonathan Druart <jonathan.druart@gmail.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=37783 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #170895|0 |1 is obsolete| | --- Comment #5 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 170961 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170961&action=edit Bug 37783: Fix form that looks like it would POST without an op in reserve/request.tt 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 reserve/request.tt the modal for cancelling a hold looks like it is a form that will do a POST without an op input, but in fact it requires JavaScript to work at all, and with JavaScript it clears out the div where it stashes inputs and then inserts one with the op cud-cancel. To persuade the test at xt/find-missing-op-in-forms.t that there is an op, and to let a casual skimmer of the code see what that op will be, without actually changing the behavior in any way, we can just stick the op in the div which the JS will .empty() out before sticking the same thing back in. Test plan: 1. Search for any record with an item, click Place hold, place two holds 2. In the row for the second hold, click the trash can icon to delete 3. Nothing changed from normallly cancelling a hold, did it? It shouldn't have. Sponsored-by: Chetco Community Public Library 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=37783 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #6 from Jonathan Druart <jonathan.druart@gmail.com> --- Trivial, skipping QA. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37783 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |jonathan.druart@gmail.com |y.org | CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37783 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |24.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37783 --- Comment #7 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=37783 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Pushed to main |RESOLVED Resolution|--- |FIXED --- Comment #8 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Merge conflicts 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=37783 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@ptfs-europe.co | |m --- Comment #9 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- This causes the "Cancel selected" button to not work. To reproduce: 1) Place a hold on a record 2) On the "existing holds" table, check a hold. 3) Click 'Cancel selected". Nothing happens. Ever. I believe this is because the payload now contains: ... op: cud-cancel biblionumber: 76 op: cud-cancel_bulk ... Revert this patch. Repeat the test plan. The selected hold is canceled as expected. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37783 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |38853 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38853 [Bug 38853] 'Cancel selected' on holds table does not work -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37783 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|38853 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38853 [Bug 38853] 'Cancel selected' on holds table does not work -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37783 Bug 37783 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