Bug 18019 - Add CSRF protection to authorities-home.pl (op==delete)
Summary: Add CSRF protection to authorities-home.pl (op==delete)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 18124
Blocks: 17096
  Show dependency treegraph
 
Reported: 2017-01-31 14:42 UTC by Marcel de Rooy
Modified: 2018-12-03 20:03 UTC (History)
10 users (show)

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


Attachments
Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) (5.16 KB, patch)
2017-02-07 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) (3.07 KB, patch)
2017-02-07 08:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) (2.74 KB, patch)
2017-02-17 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) (2.79 KB, patch)
2017-03-29 14:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) (2.83 KB, patch)
2017-03-29 14:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18019: [QA Follow-up] Also catch the delete from authorities detail (2.00 KB, patch)
2017-03-30 08:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18019: Add CSRF protection to authorities-home.pl (op==delete) (2.86 KB, patch)
2017-03-30 18:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18019: [QA Follow-up] Also catch the delete from authorities detail (2.12 KB, patch)
2017-03-30 18:13 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-01-31 14:42:00 UTC

    
Comment 1 Marcel de Rooy 2017-01-31 14:42:43 UTC
See test plan of bug 18014.
Comment 2 Marcel de Rooy 2017-01-31 14:43:55 UTC
(In reply to Marcel de Rooy from comment #1)
> See test plan of bug 18014.

Oops, I mean bug 14026.
Comment 3 Marcel de Rooy 2017-02-07 08:17:30 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2017-02-07 08:38:34 UTC
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>
Comment 5 Marcel de Rooy 2017-02-17 12:39:10 UTC
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>
Comment 6 Nick Clemens 2017-03-29 14:05:21 UTC
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.
Comment 7 Marcel de Rooy 2017-03-29 14:09:35 UTC
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>
Comment 8 Marcel de Rooy 2017-03-29 14:10:16 UTC
(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 ;)
Comment 9 Marcel de Rooy 2017-03-29 14:11:11 UTC
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.
Comment 10 Jonathan Druart 2017-03-29 15:42:12 UTC
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
Comment 11 Jonathan Druart 2017-03-29 15:44:41 UTC
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
Comment 12 Marcel de Rooy 2017-03-30 08:28:10 UTC
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>
Comment 13 Marcel de Rooy 2017-03-30 08:28:49 UTC
(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.
Comment 14 Jonathan Druart 2017-03-30 18:13:46 UTC
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>
Comment 15 Jonathan Druart 2017-03-30 18:13:51 UTC
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.
Comment 16 Kyle M Hall 2017-03-31 13:59:27 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 17 Katrin Fischer 2017-04-02 17:19:08 UTC
These patches have been pushed to 16.11.x and will be in 16.11.07.
Comment 18 Mason James 2017-05-03 01:52:36 UTC
Pushed to 16.05.x, for 16.05.12 release