See test plan of bug 18014.
(In reply to Marcel de Rooy from comment #1) > See test plan of bug 18014. Oops, I mean bug 14026.
Created attachment 59969 [details] [review] Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) Without this patch, it is possible to delete authority records with URL manipulation. Like: /cgi-bin/koha/authorities/authorities-home.pl?op=delete&authid=[XXX] Test plan: [1] Go to Authorities. Search for some authorities (without links). [2] Delete an authority. Should work. [3] Construct an URL like above to delete another authority. Should fail. Under Plack this results in an internal server error, the log tells you: Wrong CSRF token. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59971 [details] [review] Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) Without this patch, it is possible to delete authority records with URL manipulation. Like: /cgi-bin/koha/authorities/authorities-home.pl?op=delete&authid=[XXX] Test plan: [1] Go to Authorities. Search for some authorities (without links). [2] Delete an authority. Should work. [3] Construct an URL like above to delete another authority. Should fail. Under Plack this results in an internal server error, the log tells you: Wrong CSRF token. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60417 [details] [review] Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) Without this patch, it is possible to delete authority records with URL manipulation. Like: /cgi-bin/koha/authorities/authorities-home.pl?op=delete&authid=[XXX] Test plan: [1] Go to Authorities. Search for some authorities (without links). [2] Delete an authority. Should work. [3] Construct an URL like above to delete another authority. Should fail. Under Plack this results in an internal server error, the log tells you: Wrong CSRF token. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Had to apply manually as wasn't permitted to git bz, but works for me Test plan should be: Test plan: [1] Go to Authorities. Search for some authorities (without links). [2] Delete an authority. Should work. [3] Apply patch [4] Construct an URL like above to delete another authority. Should fail. Under Plack this results in an internal server error, the log tells you: Wrong CSRF token.
Created attachment 61672 [details] [review] Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) Without this patch, it is possible to delete authority records with URL manipulation. Like: /cgi-bin/koha/authorities/authorities-home.pl?op=delete&authid=[XXX] Test plan: [1] Go to Authorities. Search for some authorities (without links). [2] Delete an authority. Should work. [3] Construct an URL like above to delete another authority. Should fail. Under Plack this results in an internal server error, the log tells you: Wrong CSRF token. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Nick Clemens from comment #6) > Had to apply manually as wasn't permitted to git bz, but works for me Fixed your signoff line, but forgot the test plan ;)
Created attachment 61673 [details] [review] Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) Without this patch, it is possible to delete authority records with URL manipulation. Like: /cgi-bin/koha/authorities/authorities-home.pl?op=delete&authid=[XXX] Test plan: [1] Go to Authorities. Search for some authorities (without links). [2] Delete an authority. Should work. [3] Apply patch. [4] Construct an URL like above to delete another authority. Should fail. Under Plack this results in an internal server error, the log tells you: Wrong CSRF token. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Amended the test plan.
I'd suggest to remove the JS function confirm_deletion in authorities/detail.tt. Apparently it is not used (no way to remove an authority record from the detail page), but there is a link to authorities/authorities-home.pl?op=delete
Ha yes there is one if the authority record is not linked to any bibliographic records. In that case I see the "Delete" button and as there is no token it fails with: Wrong CSRF token at /home/vagrant/kohaclone/authorities/authorities-home.pl line 63. See includes/authorities-toolbar.inc
Created attachment 61732 [details] [review] Bug 18019: [QA Follow-up] Also catch the delete from authorities detail Good catch from Jonathan. See comment11. Authorities detail should pass a CSRF token to authorities-home when deleting a record without linked biblios. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #11) > Ha yes there is one if the authority record is not linked to any > bibliographic records. > In that case I see the "Delete" button and as there is no token it fails > with: > > Wrong CSRF token at /home/vagrant/kohaclone/authorities/authorities-home.pl > line 63. > > > See includes/authorities-toolbar.inc Good catch. Thanks. Follow-up resolves it.
Created attachment 61740 [details] [review] Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) Without this patch, it is possible to delete authority records with URL manipulation. Like: /cgi-bin/koha/authorities/authorities-home.pl?op=delete&authid=[XXX] Test plan: [1] Go to Authorities. Search for some authorities (without links). [2] Delete an authority. Should work. [3] Apply patch. [4] Construct an URL like above to delete another authority. Should fail. Under Plack this results in an internal server error, the log tells you: Wrong CSRF token. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Amended the test plan. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 61741 [details] [review] Bug 18019: [QA Follow-up] Also catch the delete from authorities detail Good catch from Jonathan. See comment11. Authorities detail should pass a CSRF token to authorities-home when deleting a record without linked biblios. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Passing the token with GET is not a good way to do, but nothing quick to replace that.
Pushed to master for 17.05, thanks Marcel!
These patches have been pushed to 16.11.x and will be in 16.11.07.
Pushed to 16.05.x, for 16.05.12 release