Description
Phil Ringnalda
2024-07-05 16:49:13 UTC
Created attachment 177131 [details] [review] Bug 37266: patron_lists/delete.pl should have CSRF protection This patch adds CSRF protection to patron list deletions. Also changed: The "Delete selected lists" button is now in a floating toolbar. To test, apply the patch and go to Tools -> Patron lists. - If necessary, create a few patron lists. - Test the two methods for list deletion available on the page: - Check one or more checkboxes and then click the "Delete selected lists" at the top of the page. - Click the "Actions" button for an individual list and choose "Delete list." - Open the checkout page for a patron. - Under the "Patron lists" tab, add the patron to a list. - Click the "Actions" button for an that list and choose "Delete list." - When you are taken to the patron lists page the list should have been deleted. - Perform the same test on the patron details page. Sponsored-by: Athens County Public Libraries Created attachment 177132 [details] [review] Bug 37266: patron_lists/delete.pl should have CSRF protection This patch adds CSRF protection to patron list deletions. Also changed: The "Delete selected lists" button is now in a floating toolbar. To test, apply the patch and go to Tools -> Patron lists. - If necessary, create a few patron lists. - Test the two methods for list deletion available on the page: - Check one or more checkboxes and then click the "Delete selected lists" at the top of the page. - Click the "Actions" button for an individual list and choose "Delete list." - Open the checkout page for a patron. - Under the "Patron lists" tab, add the patron to a list. - Click the "Actions" button for an that list and choose "Delete list." - When you are taken to the patron lists page the list should have been deleted. - Perform the same test on the patron details page. Sponsored-by: Athens County Public Libraries Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Created attachment 177352 [details] [review] Bug 37266: patron_lists/delete.pl should have CSRF protection This patch adds CSRF protection to patron list deletions. Also changed: The "Delete selected lists" button is now in a floating toolbar. To test, apply the patch and go to Tools -> Patron lists. - If necessary, create a few patron lists. - Test the two methods for list deletion available on the page: - Check one or more checkboxes and then click the "Delete selected lists" at the top of the page. - Click the "Actions" button for an individual list and choose "Delete list." - Open the checkout page for a patron. - Under the "Patron lists" tab, add the patron to a list. - Click the "Actions" button for an that list and choose "Delete list." - When you are taken to the patron lists page the list should have been deleted. - Perform the same test on the patron details page. Sponsored-by: Athens County Public Libraries Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Patch works well, changing manually the CSRF token correctly triggers CSRF protection, and code is clean and tidy. Great patch! Hi there, This patch does not apply cleanly on 24.05.x-security. See below conflicts. Please can you rebase against 24.05? ----- Conflict 1) kohadev-koha@kohadevbox:koha(24.05.x-security|AM 1/1)$ sed -n '/<<<<</,/>>>>>/p' koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt <<<<<<< HEAD <td> <input class="select_patron" type="checkbox" autocomplete="off" data-patron-list-id="[% l.patron_list_id | html %]"> <a href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]">[% l.name | html %]</a> </td> <td>[% l.patron_list_patrons_rs.count || 0 | html %]</td> <td> [% IF l.shared %] [% IF shared_by_other %] by <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% l.owner.id | uri %]">[% INCLUDE 'patron-title.inc' patron=l.owner %]</a> [% ELSE %] by you [% END %] [% END %] </td> <td> <div class="btn-group dropup"> <a class="btn btn-default btn-xs dropdown-toggle" id="listactions[% l.patron_list_id | html %]" role="button" data-toggle="dropdown" href="#"> Actions <b class="caret"></b> </a> <ul class="dropdown-menu pull-right" role="menu" aria-labelledby="listactions[% l.patron_list_id | html %]"> <li><a href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-user"></i> Add patrons</a></li> [% UNLESS shared_by_other %] <li><a href="/cgi-bin/koha/patron_lists/add-modify.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa-solid fa-pencil" aria-hidden="true"></i> Edit list</a></li> <li><a class="delete_patron" href="/cgi-bin/koha/patron_lists/delete.pl?patron_list_id=[% l.patron_list_id | html %]" data-list-name="[% l.name | html %]"><i class="fa fa-trash-can"></i> Delete list</a></li> [% END %] [% IF ( l.patron_list_patrons_rs.count ) %] <li class="divider"></li> <li> <a class="print_cards" href="/cgi-bin/koha/patroncards/print.pl?patronlist_id=[% l.patron_list_id | html %]" data-patron_list_id="[% l.patron_list_id | html %]"><i class="fa fa-print"></i> Print patron cards</a> </li> [% IF CAN_user_tools_edit_patrons %] <li> <a href="/cgi-bin/koha/tools/modborrowers.pl?patron_list_id=[% l.patron_list_id | uri %]&op=show"> <i class="fa-solid fa-pencil" aria-hidden="true"></i> Batch edit patrons </a> ======= <th class="NoSort"></th> <th>Name</th> <th>Patrons in list</th> <th>Shared</th> <th class="NoSort"> </th> </tr> </thead> <tbody> [% FOREACH l IN lists %] [% SET shared_by_other = l.owner.id != logged_in_user.id %] <tr> <td> <input class="select_list" name="patron_lists_ids" value="[% l.patron_list_id | html %]" type="checkbox" autocomplete="off" data-patron-list-id="[% l.patron_list_id | html %]" /> </td> <td> <a href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]">[% l.name | html %]</a> </td> <td>[% l.patron_list_patrons_rs.count || 0 | html %]</td> <td> [% IF l.shared %] [% IF shared_by_other %] by <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% l.owner.id | uri %]">[% INCLUDE 'patron-title.inc' patron=l.owner %]</a> [% ELSE %] by you [% END %] [% END %] </td> <td> <div class="btn-group dropup"> <a class="btn btn-default btn-xs dropdown-toggle" id="listactions[% l.patron_list_id | html %]" role="button" data-bs-toggle="dropdown" href="#"> Actions </a> <ul class="dropdown-menu" role="menu" aria-labelledby="listactions[% l.patron_list_id | html %]"> <li><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-user"></i> Add patrons</a></li> [% UNLESS shared_by_other %] <li><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/add-modify.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa-solid fa-pencil" aria-hidden="true"></i> Edit list</a></li> <li> <a class="dropdown-item submit-form-link" href="#" data-patron_list_id="[% l.patron_list_id | html %]" data-action="delete.pl" data-method="post" data-op="cud-delete" data-confirmation-msg="Are you sure you want to delete this list?"><i class="fa fa-trash-can"></i> Delete list</a> >>>>>>> Bug 37266: patron_lists/delete.pl should have CSRF protection <<<<<<< HEAD <a href="/cgi-bin/koha/tools/cleanborrowers.pl?step=2&patron_list_id=[% l.patron_list_id | uri %]&checkbox=borrower"> <i class="fa fa-trash-can"></i> Batch delete patrons </a> ======= <a class="print_cards dropdown-item" href="/cgi-bin/koha/patroncards/print.pl?patronlist_id=[% l.patron_list_id | html %]" data-patron_list_id="[% l.patron_list_id | html %]"><i class="fa fa-print"></i> Print patron cards</a> >>>>>>> Bug 37266: patron_lists/delete.pl should have CSRF protection Conflict 2) kohadev-koha@kohadevbox:koha(24.05.x-security|AM 1/1)$ sed -n '/<<<<</,/>>>>>/p' koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/patron-lists-tab.tt <<<<<<< HEAD <li><a href="/cgi-bin/koha/patron_lists/add-modify.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-pencil"></i> Edit list</a></li> <li><a class="delete_patron" href="/cgi-bin/koha/patron_lists/delete.pl?patron_list_id=[% l.patron_list_id | html %]" data-list-name="[% l.name | html %]"><i class="fa fa-trash"></i> Delete list</a></li> ======= <li><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/add-modify.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-pencil"></i> Edit list</a></li> <li> <a class="dropdown-item submit-form-link" href="#" data-patron_list_id="[% l.patron_list_id | html %]" data-action="/cgi-bin/koha/patron_lists/delete.pl" data-method="post" data-op="cud-delete" data-confirmation-msg="Are you sure you want to delete this list?"><i class="fa fa-trash-can"></i> Delete list</a> </li> >>>>>>> Bug 37266: patron_lists/delete.pl should have CSRF protection kohadev-koha@kohadevbox:koha(24.05.x-security|AM 1/1)$ Created attachment 177887 [details] [review] Bug 37266: patron_lists/delete.pl should have CSRF protection This patch adds CSRF protection to patron list deletions. Also changed: The "Delete selected lists" button is now in a floating toolbar. To test, apply the patch and go to Tools -> Patron lists. - If necessary, create a few patron lists. - Test the two methods for list deletion available on the page: - Check one or more checkboxes and then click the "Delete selected lists" at the top of the page. - Click the "Actions" button for an individual list and choose "Delete list." - Open the checkout page for a patron. - Under the "Patron lists" tab, add the patron to a list. - Click the "Actions" button for an that list and choose "Delete list." - When you are taken to the patron lists page the list should have been deleted. - Perform the same test on the patron details page. Sponsored-by: Athens County Public Libraries Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> (In reply to Owen Leonard from comment #6) > Created attachment 177887 [details] [review] [review] > Bug 37266: patron_lists/delete.pl should have CSRF protection > > This patch adds CSRF protection to patron list deletions. > > Also changed: The "Delete selected lists" button is now in a floating > toolbar. > > To test, apply the patch and go to Tools -> Patron lists. > > - If necessary, create a few patron lists. > - Test the two methods for list deletion available on the page: > - Check one or more checkboxes and then click the "Delete selected > lists" at the top of the page. > - Click the "Actions" button for an individual list and choose "Delete > list." > - Open the checkout page for a patron. > - Under the "Patron lists" tab, add the patron to a list. > - Click the "Actions" button for an that list and choose "Delete > list." > - When you are taken to the patron lists page the list should have > been deleted. > - Perform the same test on the patron details page. > > Sponsored-by: Athens County Public Libraries > Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> > Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Thanks Owen. Unfortunately that gives me an: error: sha1 information is lacking or useless. Could you please take a look? kohadev-koha@kohadevbox:koha(24.05.x-security)$ git bz apply 37266 Bug 37266 - patron_lists/delete.pl should have CSRF protection 177887 - Bug 37266: patron_lists/delete.pl should have CSRF protection Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 37266: patron_lists/delete.pl should have CSRF protection error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt). error: could not build fake ancestor Patch failed at 0001 Bug 37266: patron_lists/delete.pl should have CSRF protection hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-37266-patronlistsdeletepl-should-have-CSRF-pro-zirybmam.patch kohadev-koha@kohadevbox:koha(24.05.x-security|AM 1/1)$ Hi Owen, I resolved the sha1 error (comment #7) with help from mattermost. However, when I apply the patches on this bug report onto 24.05 I get conflicts. Could you please fix these conflicts? See the conflicts below: ------------------- kohadev-koha@kohadevbox:koha((89c6e32ee72...))$ git bz apply 37266 Bug 37266 - patron_lists/delete.pl should have CSRF protection 177887 - Bug 37266: patron_lists/delete.pl should have CSRF protection Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 37266: patron_lists/delete.pl should have CSRF protection Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt M koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt M koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt M koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/patron-lists-tab.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/patron-lists-tab.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/patron-lists-tab.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 37266: patron_lists/delete.pl should have CSRF protection hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-37266-patronlistsdeletepl-should-have-CSRF-pro-uil538ih.patch kohadev-koha@kohadevbox:koha((89c6e32ee72...)|AM 1/1)$ Specific files conflicting: kohadev-koha@kohadevbox:koha((89c6e32ee72...)|AM 1/1)$ sed -n '/<<<<</,/>>>>>/p' koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt <<<<<<< HEAD <div class="page-section"> <table id="patron-lists-table"> <thead> <tr> <input type="button" type="submit" class="btn btn-default btn-sm disabled" value="Delete selected lists" id="delete_selected_lists"/> <th>Name</th> <th>Patrons in list</th> <th>Shared</th> <th class="NoSort"> </th> </tr> </thead> <tbody> [% FOREACH l IN lists %] [% SET shared_by_other = l.owner.id != logged_in_user.id %] <tr> <td> <input class="select_patron" type="checkbox" autocomplete="off" data-patron-list-id="[% l.patron_list_id | html %]"> <a href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]">[% l.name | html %]</a> </td> <td>[% l.patron_list_patrons_rs.count || 0 | html %]</td> <td> [% IF l.shared %] [% IF shared_by_other %] by <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% l.owner.id | uri %]">[% INCLUDE 'patron-title.inc' patron=l.owner %]</a> [% ELSE %] by you [% END %] [% END %] </td> <td> <div class="btn-group dropup"> <a class="btn btn-default btn-xs dropdown-toggle" id="listactions[% l.patron_list_id | html %]" role="button" data-toggle="dropdown" href="#"> Actions <b class="caret"></b> </a> <ul class="dropdown-menu pull-right" role="menu" aria-labelledby="listactions[% l.patron_list_id | html %]"> <li><a href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-user"></i> Add patrons</a></li> [% UNLESS shared_by_other %] <li><a href="/cgi-bin/koha/patron_lists/add-modify.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa-solid fa-pencil" aria-hidden="true"></i> Edit list</a></li> <li><a class="delete_patron" href="/cgi-bin/koha/patron_lists/delete.pl?patron_list_id=[% l.patron_list_id | html %]" data-list-name="[% l.name | html %]"><i class="fa fa-trash-can"></i> Delete list</a></li> [% END %] [% IF ( l.patron_list_patrons_rs.count ) %] <li class="divider"></li> <li> <a class="print_cards" href="/cgi-bin/koha/patroncards/print.pl?patronlist_id=[% l.patron_list_id | html %]" data-patron_list_id="[% l.patron_list_id | html %]"><i class="fa fa-print"></i> Print patron cards</a> </li> [% IF CAN_user_tools_edit_patrons %] <li> <a href="/cgi-bin/koha/tools/modborrowers.pl?patron_list_id=[% l.patron_list_id | uri %]&op=show"> <i class="fa-solid fa-pencil" aria-hidden="true"></i> Batch edit patrons </a> ======= [% IF ( lists ) %] <form action="/cgi-bin/koha/patron_lists/delete.pl" method="post" id="patrons_lists_form"> [% INCLUDE 'csrf-token.inc' %] <input type="hidden" name="op" value="cud-delete" /> <div id="searchheader" class="searchheader noprint sticky"> <div class="btn-group"> <button type="submit" class="btn btn-default btn-sm disabled" id="delete_selected_lists"><i class="fa fa-trash" aria-hidden="true"></i> Delete selected lists</button> </div> </div> <div class="page-section"> <table id="patron-lists-table"> <thead> <tr> <th class="NoSort"></th> <th>Name</th> <th>Patrons in list</th> <th>Shared</th> <th class="NoSort"> </th> </tr> </thead> <tbody> [% FOREACH l IN lists %] [% SET shared_by_other = l.owner.id != logged_in_user.id %] <tr> <td> <input class="select_list" name="patron_lists_ids" value="[% l.patron_list_id | html %]" type="checkbox" autocomplete="off" data-patron-list-id="[% l.patron_list_id | html %]" /> </td> <td> <a href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]">[% l.name | html %]</a> </td> <td>[% l.patron_list_patrons_rs.count || 0 | html %]</td> <td> [% IF l.shared %] [% IF shared_by_other %] by <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% l.owner.id | uri %]">[% INCLUDE 'patron-title.inc' patron=l.owner %]</a> [% ELSE %] by you [% END %] [% END %] </td> <td> <div class="btn-group dropup"> <a class="btn btn-default btn-xs dropdown-toggle" id="listactions[% l.patron_list_id | html %]" role="button" data-bs-toggle="dropdown" href="#"> Actions </a> <ul class="dropdown-menu" role="menu" aria-labelledby="listactions[% l.patron_list_id | html %]"> <li ><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-user"></i> Add patrons</a></li > [% UNLESS shared_by_other %] <li ><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/add-modify.pl?patron_list_id=[% l.patron_list_id | uri %]" ><i class="fa-solid fa-pencil" aria-hidden="true"></i> Edit list</a ></li > <li> <a class="dropdown-item submit-form-link" href="#" data-patron_list_id="[% l.patron_list_id | html %]" data-action="delete.pl" data-method="post" data-op="cud-delete" data-confirmation-msg="Are you sure you want to delete this list?" ><i class="fa fa-trash-can"></i> Delete list</a > >>>>>>> Bug 37266: patron_lists/delete.pl should have CSRF protection <<<<<<< HEAD <a href="/cgi-bin/koha/tools/cleanborrowers.pl?step=2&patron_list_id=[% l.patron_list_id | uri %]&checkbox=borrower"> <i class="fa fa-trash-can"></i> Batch delete patrons </a> ======= <a class="print_cards dropdown-item" href="/cgi-bin/koha/patroncards/print.pl?patronlist_id=[% l.patron_list_id | html %]" data-patron_list_id="[% l.patron_list_id | html %]" ><i class="fa fa-print"></i> Print patron cards</a > >>>>>>> Bug 37266: patron_lists/delete.pl should have CSRF protection <<<<<<< HEAD [% END %] </ul> </div> </td> </tr> [% END %] </tbody> </table> </div> <!-- /.page-section --> ======= </ul> </div> </td> </tr> [% END %] </tbody> </table> </div> <!-- /.page-section --> </form> <!-- /#patron_lists_form --> >>>>>>> Bug 37266: patron_lists/delete.pl should have CSRF protection kohadev-koha@kohadevbox:koha((89c6e32ee72...)|AM 1/1)$ kohadev-koha@kohadevbox:koha((89c6e32ee72...)|AM 1/1)$ kohadev-koha@kohadevbox:koha((89c6e32ee72...)|AM 1/1)$ sed -n '/<<<<</,/>>>>>/p' koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/patron-lists-tab.tt <<<<<<< HEAD <div class="btn-group dropup"> <a class="btn btn-default btn-xs dropdown-toggle" id="listactions[% l.patron_list_id | html %]" role="button" data-toggle="dropdown" href="#"> Actions <b class="caret"></b> </a> <ul class="dropdown-menu pull-right" role="menu" aria-labelledby="listactions[% l.patron_list_id | html %]"> <li><a onclick="removeFromList('[% l.patron_list_id | html %]','[% list_id_lookup.${l.patron_list_id} | html %]')"><i class="fa fa-remove"></i> Remove patron from list</a></li> <li class="divider"></li> <li><a href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-user"></i> Add patrons</a></li> [% UNLESS shared_by_other %] <li><a href="/cgi-bin/koha/patron_lists/add-modify.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-pencil"></i> Edit list</a></li> <li><a class="delete_patron" href="/cgi-bin/koha/patron_lists/delete.pl?patron_list_id=[% l.patron_list_id | html %]" data-list-name="[% l.name | html %]"><i class="fa fa-trash"></i> Delete list</a></li> [% END %] [% IF ( l.patron_list_patrons_rs.count ) %] <li class="divider"></li> <li> <a class="print_cards" href="/cgi-bin/koha/patroncards/print.pl?patronlist_id=[% l.patron_list_id | html %]" data-patron_list_id="[% l.patron_list_id | html %]"><i class="fa fa-print"></i> Print patron cards</a> </li> [% IF CAN_user_tools_edit_patrons %] <li> <a href="/cgi-bin/koha/tools/modborrowers.pl?patron_list_id=[% l.patron_list_id | uri %]&op=show"> <i class="fa fa-pencil"></i> Batch edit patrons </a> ======= [% IF l.shared %] [% IF shared_by_other %] by <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% l.owner.id | uri %]">[% INCLUDE 'patron-title.inc' patron=l.owner %]</a> [% ELSE %] by you [% END %] [% END %] </td> [% IF CAN_user_tools_manage_patron_lists %] <td> <div class="btn-group dropup"> <a class="btn btn-default btn-xs dropdown-toggle" id="listactions[% l.patron_list_id | html %]" role="button" data-bs-toggle="dropdown" href="#"> Actions </a> <ul class="dropdown-menu" role="menu" aria-labelledby="listactions[% l.patron_list_id | html %]"> <li ><a class="dropdown-item" onclick="removeFromList('[% l.patron_list_id | html %]','[% list_id_lookup.${l.patron_list_id} | html %]')"><i class="fa fa-remove"></i> Remove patron from list</a></li > <li><hr class="dropdown-divider" /></li> <li ><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-user"></i> Add patrons</a></li > [% UNLESS shared_by_other %] <li ><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/add-modify.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-pencil"></i> Edit list</a></li > <li> <a class="dropdown-item submit-form-link" href="#" data-patron_list_id="[% l.patron_list_id | html %]" data-action="/cgi-bin/koha/patron_lists/delete.pl" data-method="post" data-op="cud-delete" data-confirmation-msg="Are you sure you want to delete this list?" ><i class="fa fa-trash-can"></i> Delete list</a > >>>>>>> Bug 37266: patron_lists/delete.pl should have CSRF protection kohadev-koha@kohadevbox:koha((89c6e32ee72...)|AM 1/1)$ Created attachment 178579 [details] [review] Bug 37266: [24.11.x] patron_lists/delete.pl should have CSRF protection This patch adds CSRF protection to patron list deletions. Also changed: The "Delete selected lists" button is now in a floating toolbar. To test, apply the patch and go to Tools -> Patron lists. - If necessary, create a few patron lists. - Test the two methods for list deletion available on the page: - Check one or more checkboxes and then click the "Delete selected lists" at the top of the page. - Click the "Actions" button for an individual list and choose "Delete list." - Open the checkout page for a patron. - Under the "Patron lists" tab, add the patron to a list. - Click the "Actions" button for an that list and choose "Delete list." - When you are taken to the patron lists page the list should have been deleted. - Perform the same test on the patron details page. Sponsored-by: Athens County Public Libraries Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Applied to 24.11.x-security Thanks for the rebase :) Hi Owen, Sorry, but can we please have a 24.05 rebase? Note: Applying either of the patches on this bug report conflict on 24.05.x-security branch. Thanks in advance! Alex Created attachment 178676 [details] [review] Bug 37266: [24.05.x] patron_lists/delete.pl should have CSRF protection This patch adds CSRF protection to patron list deletions. Also changed: The "Delete selected lists" button is now in a floating toolbar. To test, apply the patch and go to Tools -> Patron lists. - If necessary, create a few patron lists. - Test the two methods for list deletion available on the page: - Check one or more checkboxes and then click the "Delete selected lists" at the top of the page. - Click the "Actions" button for an individual list and choose "Delete list." - Open the checkout page for a patron. - Under the "Patron lists" tab, add the patron to a list. - Click the "Actions" button for an that list and choose "Delete list." - When you are taken to the patron lists page the list should have been deleted. - Perform the same test on the patron details page. Sponsored-by: Athens County Public Libraries Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Thank you very much Owen for the 24.05 rebase, appreciate it. We are checking it asap Applied to 24.05.x-security branch. Test plan works as expected. Comment on attachment 178579 [details] [review] Bug 37266: [24.11.x] patron_lists/delete.pl should have CSRF protection >From 6c321cdefe352c5369b36c34aafb7fa364cd33d4 Mon Sep 17 00:00:00 2001 >From: Owen Leonard <oleonard@myacpl.org> >Date: Mon, 24 Feb 2025 14:44:03 +0000 >Subject: [PATCH] Bug 37266: [24.11.x] patron_lists/delete.pl should have CSRF > protection > >This patch adds CSRF protection to patron list deletions. > >Also changed: The "Delete selected lists" button is now in a floating >toolbar. > >To test, apply the patch and go to Tools -> Patron lists. > >- If necessary, create a few patron lists. >- Test the two methods for list deletion available on the page: > - Check one or more checkboxes and then click the "Delete selected > lists" at the top of the page. > - Click the "Actions" button for an individual list and choose "Delete > list." >- Open the checkout page for a patron. > - Under the "Patron lists" tab, add the patron to a list. > - Click the "Actions" button for an that list and choose "Delete > list." > - When you are taken to the patron lists page the list should have > been deleted. >- Perform the same test on the patron details page. > >Sponsored-by: Athens County Public Libraries >Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> >Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> >--- > .../prog/en/modules/circ/circulation.tt | 1 + > .../prog/en/modules/members/moremember.tt | 1 + > .../prog/en/modules/patron_lists/lists.tt | 67 +++++++++++-------- > .../modules/patron_lists/patron-lists-tab.tt | 7 +- > patron_lists/delete.pl | 15 +++-- > 5 files changed, 56 insertions(+), 35 deletions(-) > >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt >index a3c9d73f1bb..fc77a84b8db 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt >@@ -1167,6 +1167,7 @@ > [% Asset.js("js/checkouts.js") | $raw %] > [% Asset.js("js/tables/bookings.js") | $raw %] > [% Asset.js("js/recalls.js") | $raw %] >+ [% Asset.js("js/form-submit.js") | $raw %] > [% END %] > > [% INCLUDE 'intranet-bottom.inc' %] >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt >index a12b9e3a504..c25adbf4b11 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt >@@ -776,6 +776,7 @@ > [% INCLUDE 'str/members-menu.inc' %] > [% Asset.js("js/members-menu.js") | $raw %] > [% Asset.js("js/recalls.js") | $raw %] >+ [% Asset.js("js/form-submit.js") | $raw %] > <script> > const LoadCheckoutsTableDelay = 0; > const AlwaysLoadCheckoutsTable = [% Koha.Preference('AlwaysLoadCheckoutsTable') | html %]; >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt >index 5e418df153d..38159d1afcb 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt >@@ -45,11 +45,20 @@ > > [% IF ( lists ) %] > >+ <form action="/cgi-bin/koha/patron_lists/delete.pl" method="post" id="patrons_lists_form"> >+ [% INCLUDE 'csrf-token.inc' %] >+ <input type="hidden" name="op" value="cud-delete" /> >+ <div id="searchheader" class="searchheader noprint sticky"> >+ <div class="btn-group"> >+ <button type="submit" class="btn btn-default btn-sm disabled" id="delete_selected_lists"><i class="fa fa-trash" aria-hidden="true"></i> Delete selected lists</button> >+ </div> >+ </div> >+ > <div class="page-section"> > <table id="patron-lists-table"> > <thead> > <tr> >- <input type="button" type="submit" class="btn btn-default btn-sm disabled" value="Delete selected lists" id="delete_selected_lists"/> >+ <th class="NoSort"></th> > <th>Name</th> > <th>Patrons in list</th> > <th>Shared</th> >@@ -62,7 +71,9 @@ > [% SET shared_by_other = l.owner.id != logged_in_user.id %] > <tr> > <td> >- <input class="select_patron" type="checkbox" autocomplete="off" data-patron-list-id="[% l.patron_list_id | html %]"> >+ <input class="select_list" name="patron_lists_ids" value="[% l.patron_list_id | html %]" type="checkbox" autocomplete="off" data-patron-list-id="[% l.patron_list_id | html %]" /> >+ </td> >+ <td> > <a href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]">[% l.name | html %]</a> > </td> > <td>[% l.patron_list_patrons_rs.count || 0 | html %]</td> >@@ -85,7 +96,9 @@ > <li><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-user"></i> Add patrons</a></li> > [% UNLESS shared_by_other %] > <li><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/add-modify.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa-solid fa-pencil" aria-hidden="true"></i> Edit list</a></li> >- <li><a class="delete_patron dropdown-item" href="/cgi-bin/koha/patron_lists/delete.pl?patron_list_id=[% l.patron_list_id | html %]" data-list-name="[% l.name | html %]"><i class="fa fa-trash-can"></i> Delete list</a></li> >+ <li> >+ <a class="dropdown-item submit-form-link" href="#" data-patron_list_id="[% l.patron_list_id | html %]" data-action="delete.pl" data-method="post" data-op="cud-delete" data-confirmation-msg="Are you sure you want to delete this list?" ><i class="fa fa-trash-can"></i> Delete list</a> >+ </li> > [% END %] > [% IF ( l.patron_list_patrons_rs.count ) %] > <li><hr class="dropdown-divider" /></li> >@@ -115,7 +128,8 @@ > </tbody> > </table> > </div> <!-- /.page-section --> >- >+ </form> >+ <!-- /#patron_lists_form --> > <!-- Modal to print patron cards --> > <div class="modal" id="patronExportModal" tabindex="-1" role="dialog" aria-labelledby="patronExportModal_label" aria-hidden="true"> > <div class="modal-dialog"> >@@ -147,6 +161,7 @@ > > [% MACRO jsinclude BLOCK %] > [% Asset.js("js/tools-menu.js") | $raw %] >+ [% Asset.js("js/form-submit.js") | $raw %] > [% INCLUDE 'datatables.inc' %] > > <script> >@@ -161,35 +176,31 @@ > "columnDefs": [ > { "orderable": false, "searchable": false, "targets": [ 'NoSort' ] } > ], >- "pagingType": "full" >+ "pagingType": "full", >+ "sorting": [[ 1, "asc" ]] > } )); >- $(".delete_patron").on("click", function(){ >- $(".dropdown").removeClass("open"); >- var list = $(this).data("list-name"); >- return confirmDelete( _("Are you sure you want to delete the list %s?").format(list)); >- }); > >- $("#delete_selected_lists").on("click", function() { >- if (selectedPatronLists.length != 0) { >- if (confirm(_("Are you sure you want to delete the selected lists ?"))) { >- var delete_lists_url = '/cgi-bin/koha/patron_lists/delete.pl?patron_lists_ids=' + selectedPatronLists.join("&patron_lists_ids="); >- window.location.href = delete_lists_url; >- } >+ $("#patrons_lists_form").submit(function(){ >+ var checkedItems = $("input[name=patron_lists_ids]:checked"); >+ if ( checkedItems.size() == 0) { >+ alert(_("You must select one or more lists to delete")); >+ return false; > } >+ if( confirm(_("Are you sure you want to delete the selected lists?")) ) { >+ return true; >+ } else { >+ return false; >+ } > }); > >- $(document).on("click", ".select_patron", function() { >- if($(this).is(':checked')){ >- $("#delete_selected_lists").attr("class","btn btn-default btn-sm"); >- selectedPatronLists.push($(this).data("patron-list-id")); >- } >- else { >- selectedPatronLists = selectedPatronLists.filter(item => item !== $(this).data("patron-list-id")); >- if(selectedPatronLists.length === 0){ >- $("#delete_selected_lists").attr("class","btn btn-default btn-sm disabled"); >- } >- } >- }); >+ $(document).on("click", ".select_list", function() { >+ var checkedItems = $("input[name=patron_lists_ids]:checked"); >+ if ( checkedItems.size() == 0 ) { >+ $("#delete_selected_lists").addClass("disabled").prop("disabled", true); >+ } else { >+ $("#delete_selected_lists").removeClass("disabled").prop("disabled", false); >+ } >+ }); > > $(".print_cards").on("click", function(e){ > e.preventDefault(); >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/patron-lists-tab.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/patron-lists-tab.tt >index d626d217107..de77c611def 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/patron-lists-tab.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/patron-lists-tab.tt >@@ -1,3 +1,6 @@ >+[% USE raw %] >+[% USE Koha %] >+[% USE Asset %] > [% USE KohaDates %] > > [% IF no_access_to_patron %] >@@ -53,7 +56,9 @@ > <li><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/list.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-user"></i> Add patrons</a></li> > [% UNLESS shared_by_other %] > <li><a class="dropdown-item" href="/cgi-bin/koha/patron_lists/add-modify.pl?patron_list_id=[% l.patron_list_id | uri %]"><i class="fa fa-pencil"></i> Edit list</a></li> >- <li><a class="delete_patron dropdown-item" href="/cgi-bin/koha/patron_lists/delete.pl?patron_list_id=[% l.patron_list_id | html %]" data-list-name="[% l.name | html %]"><i class="fa fa-trash"></i> Delete list</a></li> >+ <li> >+ <a class="dropdown-item submit-form-link" href="#" data-patron_list_id="[% l.patron_list_id | html %]" data-action="/cgi-bin/koha/patron_lists/delete.pl" data-method="post" data-op="cud-delete" data-confirmation-msg="Are you sure you want to delete this list?" ><i class="fa fa-trash-can"></i> Delete list</a> >+ </li> > [% END %] > [% IF ( l.patron_list_patrons_rs.count ) %] > <li><hr class="dropdown-divider" /></li> >diff --git a/patron_lists/delete.pl b/patron_lists/delete.pl >index e7c227ea1f7..45421acf554 100755 >--- a/patron_lists/delete.pl >+++ b/patron_lists/delete.pl >@@ -26,6 +26,7 @@ use C4::Output; > use Koha::List::Patron qw( DelPatronList ); > > my $cgi = CGI->new; >+my $op = $cgi->param('op') // q{}; > > my ( $template, $loggedinuser, $cookie ) = get_template_and_user( > { >@@ -39,12 +40,14 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( > my $id = $cgi->param('patron_list_id'); > my @lists_ids = $cgi->multi_param('patron_lists_ids'); > >-if ( defined $id && $id ne '' ) { >- DelPatronList( { patron_list_id => $id } ); >-} >-if (@lists_ids) { >- foreach my $list_id (@lists_ids) { >- DelPatronList( { patron_list_id => $list_id } ); >+if ( $op eq 'cud-delete' ) { >+ if ( defined $id && $id ne '' ) { >+ DelPatronList( { patron_list_id => $id } ); >+ } >+ if (@lists_ids) { >+ foreach my $list_id (@lists_ids) { >+ DelPatronList( { patron_list_id => $list_id } ); >+ } > } > } > >-- >2.39.5 Created attachment 179008 [details] [review] Bug 37266: patron_lists/delete.pl should have CSRF protection This patch adds CSRF protection to patron list deletions. Also changed: The "Delete selected lists" button is now in a floating toolbar. To test, apply the patch and go to Tools -> Patron lists. - If necessary, create a few patron lists. - Test the two methods for list deletion available on the page: - Check one or more checkboxes and then click the "Delete selected lists" at the top of the page. - Click the "Actions" button for an individual list and choose "Delete list." - Open the checkout page for a patron. - Under the "Patron lists" tab, add the patron to a list. - Click the "Actions" button for an that list and choose "Delete list." - When you are taken to the patron lists page the list should have been deleted. - Perform the same test on the patron details page. Sponsored-by: Athens County Public Libraries Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Pushed for 25.05! Well done everyone, thank you! |