Steps to reproduce: 1. Search the catalog for anything with results, click Edit items on any bib with an existing item. 2. Click the Actions menu at the left of any item, select Print label That 500 error you just saw happened because the Print label action opens a popup at /cgi-bin/koha/labels/label-edit-batch.pl?op=add&number_type=itemnumber&number_list=633 which used to create a new label batch with that item in it, but now falls through the code past the "$op eq cud-add" because other parts of the label batch UI want to POST things to add so the op changed to cud-add, and the attempt to add winds up in code that thinks you are editing an existing batch, and makes crashing assumptions about what will be defined. Given that we don't believe that creating a label batch is worthy of CSRF protection, and we don't believe that removing an item from an existing batch is worthy of CSRF protection, I would argue that we should just make the if for add be elsif ($op eq 'cud-add' || $op eq 'add') rather than go through all the gyrations necessary to open a popup and have it immediately and silently POST a form just to add an item to a newly created batch.
Created attachment 168117 [details] [review] Bug 37192: Make label printing from the item editor work by accepting the op add Some parts of the UI for adding items to a label batch (and there's lots of different UI to do it) want to POST items to add to a batch, so CSRF protection switched the op to add an item to cud-add. However, the item editor needs to create a batch of one from its Actions menu by just opening a popup, so it wants to GET ?op=add. Accepting both ops will keep all the variety of UIs happy. Test plan: 1. Search the catalog for something that will return results, like Perl 2. On any bib that has at least one item, click Edit items 3. In the Actions menu to the left of any item, choose Print label 4. That opened a popup with a 500 error rather than a label batch editor 5. Apply patch, restart_all 6. Repeat step 3, but this time you'll get a label batch editor with your item in it.
Created attachment 168151 [details] [review] Bug 37192: Make label printing from the item editor work by accepting the op add Some parts of the UI for adding items to a label batch (and there's lots of different UI to do it) want to POST items to add to a batch, so CSRF protection switched the op to add an item to cud-add. However, the item editor needs to create a batch of one from its Actions menu by just opening a popup, so it wants to GET ?op=add. Accepting both ops will keep all the variety of UIs happy. Test plan: 1. Search the catalog for something that will return results, like Perl 2. On any bib that has at least one item, click Edit items 3. In the Actions menu to the left of any item, choose Print label 4. That opened a popup with a 500 error rather than a label batch editor 5. Apply patch, restart_all 6. Repeat step 3, but this time you'll get a label batch editor with your item in it. Signed-off-by: Eric Garcia <cubingguy714@gmail.com>
Security always has a price ;) I understand that it is easier to do it like this than adding much more code to achieve the same. But before we open the doors again for exceptions here and there, and compromise our security somewhere along the way, the release team should discuss and have concensus about that. Moving to ID Copying Martin as new QAM and Jonathan as CSRF expert.
(In reply to Phil Ringnalda from comment #0) > we don't believe that removing an item from an existing > batch is worthy of CSRF protection I believe so, and filed Bug 37206.
Okay, I think I've got it, but unassigning because I don't have enough time to do it right before I go on vacation. I should be old enough by now to know not to ever touch printing, because in every project it's canonically a tar-baby that's going to stick other bugs to you. What this Print label feature should be doing is not "add", because cud-add is actually two ops with one name: cud_add with a non-zero batch_id is add, cud_add with batch_id=0 or batch_id omitted is create-and-add, because new is very much not cud_new, it is show-blank-form. What this feature should be doing is calling new, with added behavior for new to stick a passed &item_id=n into the add textarea, where the user can then click Add to create-and-add, an awkward but necessary step. And ugh, someone really needs to fix bug 37223 because that's horrid behavior.
Meh, that's wrong. The user doesn't need to sit considering the repercussions of cud-add writing a line to creator_batches, they just need to show their csrf_token visa to be allowed in, and the link should just cud-add with form-submit.js. We just need bug 37267 to let the link keep opening a new window.
*** Bug 37297 has been marked as a duplicate of this bug. ***
Created attachment 168839 [details] [review] Bug 37192: Add ability to pass data-new_tab to form-submit.js and open in new tab
Created attachment 168840 [details] [review] Bug 37192: Adjust template for additem.tt Test plan: 1. Search the catalog for something that will return results, like Perl 2. On any bib that has at least one item, click Edit items 3. In the Actions menu to the left of any item, choose Print label 4. That opened a popup with a 500 error rather than a label batch editor 5. Apply patch, restart_all 6. Repeat step 3, but this time you'll get a label batch editor with your item in it.
Created attachment 168851 [details] [review] Bug 37192: Add ability to pass data-new_tab to form-submit.js and open in new tab Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Created attachment 168852 [details] [review] Bug 37192: Adjust template for additem.tt Test plan: 1. Search the catalog for something that will return results, like Perl 2. On any bib that has at least one item, click Edit items 3. In the Actions menu to the left of any item, choose Print label 4. That opened a popup with a 500 error rather than a label batch editor 5. Apply patch, restart_all 6. Repeat step 3, but this time you'll get a label batch editor with your item in it. Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
*** Bug 37267 has been marked as a duplicate of this bug. ***
Created attachment 169089 [details] [review] Bug 37192: Add ability to pass data-new_tab to form-submit.js and open in new tab Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169090 [details] [review] Bug 37192: Adjust template for additem.tt Test plan: 1. Search the catalog for something that will return results, like Perl 2. On any bib that has at least one item, click Edit items 3. In the Actions menu to the left of any item, choose Print label 4. That opened a popup with a 500 error rather than a label batch editor 5. Apply patch, restart_all 6. Repeat step 3, but this time you'll get a label batch editor with your item in it. Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pragmatic solution without re-writing the whole of Koha. Passing QA
Maybe this has already been done somewhere else but if not, we should document how to use form-submit.js somewhere.
Pushed for 24.11! Well done everyone, thank you!
Could this be backported to 24.05?
Backported to 24.05.x for upcoming 24.05.04
Not for 23.11.x
fixes a bug, nothing to document