Bug 37266

Summary: patron_lists/delete.pl should have CSRF protection
Product: Koha Reporter: Phil Ringnalda <phil>
Component: ToolsAssignee: Owen Leonard <oleonard>
Status: RESOLVED FIXED QA Contact: Julian Maurice <julian.maurice>
Severity: normal    
Priority: P5 - low CC: alexbuckley, dcook, didier.gautheron, fridolin.somers, jonathan.druart, julian.maurice, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, paul.derscheid, philippe.blouin, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00,24.11.02,24.05.07
Circulation function:
Attachments: Bug 37266: patron_lists/delete.pl should have CSRF protection
Bug 37266: patron_lists/delete.pl should have CSRF protection
Bug 37266: patron_lists/delete.pl should have CSRF protection
Bug 37266: patron_lists/delete.pl should have CSRF protection
Bug 37266: [24.11.x] patron_lists/delete.pl should have CSRF protection
Bug 37266: [24.05.x] patron_lists/delete.pl should have CSRF protection
Bug 37266: patron_lists/delete.pl should have CSRF protection

Description Phil Ringnalda 2024-07-05 16:49:13 UTC
If patron lists were normal, deleting them would be at patron_lists/lists.pl?op=delete and we would have changed that to op=cud-delete and put in a CSRF token, but they have their own script for delete, which accepts a GET.
Comment 1 Owen Leonard 2025-01-24 18:31:38 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
Comment 2 Phil Ringnalda 2025-01-24 18:54:45 UTC
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>
Comment 3 Julian Maurice 2025-01-30 12:22:16 UTC
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>
Comment 4 Julian Maurice 2025-01-30 12:24:41 UTC
Patch works well, changing manually the CSRF token correctly triggers CSRF protection, and code is clean and tidy.
Great patch!
Comment 5 Alex Buckley 2025-02-12 02:14:44 UTC
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">&nbsp;</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)$
Comment 6 Owen Leonard 2025-02-12 13:45:59 UTC
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>
Comment 7 Alex Buckley 2025-02-12 20:56:40 UTC
(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)$
Comment 8 Alex Buckley 2025-02-20 02:13:59 UTC
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">&nbsp;</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">&nbsp;</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)$
Comment 9 Owen Leonard 2025-02-24 14:52:56 UTC
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>
Comment 10 Paul Derscheid 2025-02-24 18:58:08 UTC
Applied to 24.11.x-security

Thanks for the rebase :)
Comment 11 Alex Buckley 2025-02-24 20:56:48 UTC
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
Comment 12 Owen Leonard 2025-02-25 13:01:41 UTC
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>
Comment 13 Alex Buckley 2025-02-25 16:34:05 UTC
Thank you very much Owen for the 24.05 rebase, appreciate it.

We are checking it asap
Comment 14 Alex Buckley 2025-02-25 19:23:37 UTC
Applied to 24.05.x-security branch.

Test plan works as expected.
Comment 15 Katrin Fischer 2025-03-06 11:51:21 UTC
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
Comment 16 Owen Leonard 2025-03-06 12:17:11 UTC
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>
Comment 17 Katrin Fischer 2025-03-06 12:56:07 UTC
Pushed for 25.05!

Well done everyone, thank you!