Bug 37783 - Fix form that looks like it would POST without an op in reserve/request.tt
Summary: Fix form that looks like it would POST without an op in reserve/request.tt
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P3 normal
Assignee: Phil Ringnalda
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 36192
Blocks: 37728 37817
  Show dependency treegraph
 
Reported: 2024-08-30 01:12 UTC by Phil Ringnalda
Modified: 2024-09-13 10:07 UTC (History)
6 users (show)

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


Attachments
Bug 37783: Fix form that looks like it would POST without an op in reserve/request.tt (2.03 KB, patch)
2024-08-30 01:25 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37783: Fix form that looks like it would POST without an op in reserve/request.tt (2.10 KB, patch)
2024-09-03 08:53 UTC, Jonathan Druart
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-08-30 01:12:14 UTC
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/7342209b34d642935651e7b2275c3ca14656c4b1/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt#L1443 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.
Comment 1 Phil Ringnalda 2024-08-30 01:25:16 UTC
Created attachment 170895 [details] [review]
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
Comment 2 Jonathan Druart 2024-09-03 08:49:13 UTC
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
Comment 3 Jonathan Druart 2024-09-03 08:50:30 UTC
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.
Comment 4 Jonathan Druart 2024-09-03 08:52:52 UTC
Created bug 37817.
Comment 5 Jonathan Druart 2024-09-03 08:53:55 UTC
Created attachment 170961 [details] [review]
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>
Comment 6 Jonathan Druart 2024-09-03 08:54:32 UTC
Trivial, skipping QA.
Comment 7 Katrin Fischer 2024-09-13 10:07:54 UTC
Pushed for 24.11!

Well done everyone, thank you!