Bug 37192 - Can't print label from the item editor
Summary: Can't print label from the item editor
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Chris Nighswonger
QA Contact: Testopia
URL:
Keywords: regression
Depends on: 36192
Blocks:
  Show dependency treegraph
 
Reported: 2024-06-26 02:36 UTC by Phil Ringnalda
Modified: 2024-06-28 18:28 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 37192: Make label printing from the item editor work by accepting the op add (1.72 KB, patch)
2024-06-26 03:28 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37192: Make label printing from the item editor work by accepting the op add (1.77 KB, patch)
2024-06-26 14:46 UTC, Eric Garcia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Phil Ringnalda 2024-06-26 02:36:55 UTC
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.
Comment 1 Phil Ringnalda 2024-06-26 03:28:17 UTC
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.
Comment 2 Eric Garcia 2024-06-26 14:46:37 UTC
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>
Comment 3 Marcel de Rooy 2024-06-28 08:05:33 UTC
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.
Comment 4 Owen Leonard 2024-06-28 11:41:28 UTC
(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.
Comment 5 Phil Ringnalda 2024-06-28 18:28:35 UTC
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.