|
Description
Julian Maurice
2024-04-12 13:18:51 UTC
Created attachment 164847 [details] [review] Bug 36598: Enable CSRF protection for Mojolicious apps Test plan: 1. Run bin/opac daemon -l http://*:3001/ 2. Go to http://localhost:3001/cgi-bin/koha/opac-user.pl 3. With browser devtools, locate csrf_token hidden input within the login form and remove it or modify it 4. Try to submit the form with correct credentials, it should fail ("Wrong CSRF token") 5. Reload the page, try to log in normally without modifying the DOM, it should succeed 6. Run bin/intranet daemon -l http://*:3002/ 7. Go to http://localhost:3002/cgi-bin/koha/mainpage.pl 8. With browser devtools, locate csrf_token hidden input within the login form and remove it or modify it 9. Try to submit the form with correct credentials, it should fail ("Wrong CSRF token") 10. Reload the page, try to log in normally without modifying the DOM, it should succeed 11. Run prove t/db_dependent/mojo/csrf.t Created attachment 164848 [details] [review] Bug 36598: Enable CSRF protection for Mojolicious apps Test plan: 1. Run bin/opac daemon -l http://*:3001/ 2. Go to http://localhost:3001/cgi-bin/koha/opac-user.pl 3. With browser devtools, locate csrf_token hidden input within the login form and remove it or modify it 4. Try to submit the form with correct credentials, it should fail ("Wrong CSRF token") 5. Reload the page, try to log in normally without modifying the DOM, it should succeed 6. Run bin/intranet daemon -l http://*:3002/ 7. Go to http://localhost:3002/cgi-bin/koha/mainpage.pl 8. With browser devtools, locate csrf_token hidden input within the login form and remove it or modify it 9. Try to submit the form with correct credentials, it should fail ("Wrong CSRF token") 10. Reload the page, try to log in normally without modifying the DOM, it should succeed 11. Run prove t/db_dependent/mojo/csrf.t Looks pretty good at a glance. I'd need a bit more time with it, but this should work. Setting status to ASSIGNED. It needs more work (adding the "cud-" prefix check) (In reply to Julian Maurice from comment #4) > Setting status to ASSIGNED. It needs more work (adding the "cud-" prefix > check) Ah right. Yes, we wouldn't need it for the API, but would more broadly I suppose... Created attachment 164870 [details] [review] Bug 36598: Prohibit CUD operations with safe HTTP methods (GET/HEAD/...) Created attachment 165075 [details] [review] Bug 36598: Enable CSRF protection for Mojolicious apps Test plan: 1. Run bin/opac daemon -l http://*:3001/ 2. Go to http://localhost:3001/cgi-bin/koha/opac-user.pl 3. With browser devtools, locate csrf_token hidden input within the login form and remove it or modify it 4. Try to submit the form with correct credentials, it should fail ("Wrong CSRF token") 5. Reload the page, try to log in normally without modifying the DOM, it should succeed 6. Run bin/intranet daemon -l http://*:3002/ 7. Go to http://localhost:3002/cgi-bin/koha/mainpage.pl 8. With browser devtools, locate csrf_token hidden input within the login form and remove it or modify it 9. Try to submit the form with correct credentials, it should fail ("Wrong CSRF token") 10. Reload the page, try to log in normally without modifying the DOM, it should succeed 11. Run prove t/db_dependent/mojo/csrf.t Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 165076 [details] [review] Bug 36598: Prohibit CUD operations with safe HTTP methods (GET/HEAD/...) Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 166094 [details] [review] Bug 36598: Fix CSRF header name (underscore -> hyphen) Comment on attachment 165075 [details] [review] Bug 36598: Enable CSRF protection for Mojolicious apps Review of attachment 165075 [details] [review]: ----------------------------------------------------------------- ::: Koha/App/Plugin/CSRF.pm @@ +45,5 @@ > + > +Defines an `around_action` hook that will return a 403 response if CSRF token > +is missing or invalid. > + > +This verification occurs only for HTTP methods POST, PUT, DELETE and PATCH. The commit "Prohibit CUD operations with safe HTTP methods" doesn't update this. Comment on attachment 165076 [details] [review] Bug 36598: Prohibit CUD operations with safe HTTP methods (GET/HEAD/...) Review of attachment 165076 [details] [review]: ----------------------------------------------------------------- ::: Koha/App/Plugin/CSRF.pm @@ +64,5 @@ > my $method = $c->req->method; > + if ( $method eq 'GET' || $method eq 'HEAD' || $method eq 'OPTIONS' || $method eq 'TRACE' ) { > + my $op = $c->req->param('op'); > + if ( $op && $op =~ /^cud-/ ) { > + return $c->reply->exception('Wrong HTTP method')->rendered(400); This will be confusing without basic explanation about the op cud- prefix depending on whether it's a stateful or stateless HTTP method. When helping someone in another ticket, it was the op that was wrong, not the method. What about something like «The `op` param starting with `cud-` goes only with a stateful HTTP method. Either the method or the `op` param is wrong.» (rephrasing welcome) testing notes: > 1. Run bin/opac daemon -l http://*:3001/ To use in koha-testing-docker, I did sudo killall apache2 # `service apache2 stop` had the expected output but apache was still running... bin/opac daemon -l http://*:8080/ # so it's a port mapped to the host --- Are those errors expected?: prove t/db_dependent/mojo/csrf.t t/db_dependent/mojo/csrf.t .. File not found : js/desk_selection.js at /kohadevbox/koha/Koha/Template/Plugin/Asset.pm line 84. File not found : js/register_selection.js at /kohadevbox/koha/Koha/Template/Plugin/Asset.pm line 84. File not found : js/desk_selection.js at /kohadevbox/koha/Koha/Template/Plugin/Asset.pm line 84. File not found : js/register_selection.js at /kohadevbox/koha/Koha/Template/Plugin/Asset.pm line 84. File not found : js/desk_selection.js at /kohadevbox/koha/Koha/Template/Plugin/Asset.pm line 84. File not found : js/register_selection.js at /kohadevbox/koha/Koha/Template/Plugin/Asset.pm line 84. That looks like mojo implementation having something incomplete in assets loading (only in the context of tests?) and not related to this ticket. Since they are still popping if I comment the usage of the CSRF plugin. (In reply to Victor Grousset/tuxayo from comment #12) > Are those errors expected?: > [...] > That looks like mojo implementation having something incomplete in assets > loading (only in the context of tests?) and not related to this ticket. > Since they are still popping if I comment the usage of the CSRF plugin. This is not related to mojo nor this bug. See bug 36582 comment 18 Created attachment 166634 [details] [review] Bug 36598: Improve documentation and error message in CSRF plugin Thanks for the changes :) Rechecking against the plack middleware (Koha/Middleware/CSRF.pm) I noticed that it also errors when with a stateful method, the op is lacking the `cud-` prefix. Which the mojo plugin here doesn't enforce. Another finding (less clear, at least in what to do): In the end, there will be duplication of the logic in the plack middleware and the mojo plugin. That logic doesn't look complex enough or enough likely to change much in the future to be worth refactoring. Maybe just add a comment at the start of the plack middleware logic that it must be kept in sync the with mojo plugin? Because any change in the middleware will very likely forget mojo. I'm not sure what the best way to handle that case. Created attachment 166673 [details] [review] Bug 36598: Prevent use of unsafe HTTP method with non-cud op parameter (In reply to Victor Grousset/tuxayo from comment #15) > Another finding (less clear, at least in what to do): > In the end, there will be duplication of the logic in the plack middleware > and the mojo plugin. That logic doesn't look complex enough or enough likely > to change much in the future to be worth refactoring. I agree. > Maybe just add a > comment at the start of the plack middleware logic that it must be kept in > sync the with mojo plugin? Because any change in the middleware will very > likely forget mojo. I'm not sure what the best way to handle that case. Ditch Plack/Starman in favor of Mojo/Hypnotoad (so there is no need for duplication) ? :D I'm not sure if a comment can help, but it is worth a try Created attachment 166675 [details] [review] Bug 36598: Add comments asking to keep both CSRF checks in sync Created attachment 166727 [details] [review] Bug 36598: Enable CSRF protection for Mojolicious apps Test plan: 1. Run bin/opac daemon -l http://*:3001/ 2. Go to http://localhost:3001/cgi-bin/koha/opac-user.pl 3. With browser devtools, locate csrf_token hidden input within the login form and remove it or modify it 4. Try to submit the form with correct credentials, it should fail ("Wrong CSRF token") 5. Reload the page, try to log in normally without modifying the DOM, it should succeed 6. Run bin/intranet daemon -l http://*:3002/ 7. Go to http://localhost:3002/cgi-bin/koha/mainpage.pl 8. With browser devtools, locate csrf_token hidden input within the login form and remove it or modify it 9. Try to submit the form with correct credentials, it should fail ("Wrong CSRF token") 10. Reload the page, try to log in normally without modifying the DOM, it should succeed 11. Run prove t/db_dependent/mojo/csrf.t Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 166728 [details] [review] Bug 36598: Prohibit CUD operations with safe HTTP methods (GET/HEAD/...) Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 166729 [details] [review] Bug 36598: Fix CSRF header name (underscore -> hyphen) Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 166730 [details] [review] Bug 36598: Improve documentation and error message in CSRF plugin Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 166731 [details] [review] Bug 36598: Prevent use of unsafe HTTP method with non-cud op parameter Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 166732 [details] [review] Bug 36598: Add comments asking to keep both CSRF checks in sync Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 166733 [details] [review] Bug 36598: (QA follow-up): tidy up code > Ditch Plack/Starman in favor of Mojo/Hypnotoad
> (so there is no need for duplication) ? :D
[U+1F638]
Seems to work as expected. Also tried this scenario: 1. open the opac form to edit one's detail 2. log out and log in from another tab 3. post the form 4. wrong CSRF token error as expected 5. same on the staff interface with the login form Messing with the op hidden field value to add or remove cud- gives the expected result. Code looks good, complies with the guidelines. I'm just not sure enough there isn't a blind spot somewhere since I didn't follow CSRF introduction. So if someone else can just confirm that, we can move forward. Created attachment 171119 [details] [review] Bug 36598: Enable CSRF protection for Mojolicious apps Test plan: 1. Run bin/opac daemon -l http://*:3001/ 2. Go to http://localhost:3001/cgi-bin/koha/opac-user.pl 3. With browser devtools, locate csrf_token hidden input within the login form and remove it or modify it 4. Try to submit the form with correct credentials, it should fail ("Wrong CSRF token") 5. Reload the page, try to log in normally without modifying the DOM, it should succeed 6. Run bin/intranet daemon -l http://*:3002/ 7. Go to http://localhost:3002/cgi-bin/koha/mainpage.pl 8. With browser devtools, locate csrf_token hidden input within the login form and remove it or modify it 9. Try to submit the form with correct credentials, it should fail ("Wrong CSRF token") 10. Reload the page, try to log in normally without modifying the DOM, it should succeed 11. Run prove t/db_dependent/mojo/csrf.t Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 171120 [details] [review] Bug 36598: Prohibit CUD operations with safe HTTP methods (GET/HEAD/...) Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 171121 [details] [review] Bug 36598: Fix CSRF header name (underscore -> hyphen) Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 171122 [details] [review] Bug 36598: Improve documentation and error message in CSRF plugin Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 171123 [details] [review] Bug 36598: Prevent use of unsafe HTTP method with non-cud op parameter Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 171124 [details] [review] Bug 36598: Add comments asking to keep both CSRF checks in sync Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 171125 [details] [review] Bug 36598: (QA follow-up): tidy up code Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Hi I am having trouble with testing this on 23.05 I run the following: kohadev-koha@kohadevbox:koha((ef654af7624...))$ bin/opac daemon -l http://*:3001/ [2024-09-11 23:59:24.10736] [22804] [info] Listening at "http://*:3001/" Web application available at http://127.0.0.1:3001/ But then http://127.0.0.1:3001/ is unable to connect Can I please get some help with testing this on ktd? (In reply to wainuiwitikapark from comment #35) > Can I please get some help with testing this on ktd? I'm not a user of ktd but you probably need to replace 127.0.0.1 by the container IP address (In reply to wainuiwitikapark from comment #35) > Hi I am having trouble with testing this on 23.05 The bug dependencies for this aren't on 23.05. So it shouldn't be backported. ---- As for testing on other branches that need this. I think I changed the port from 3001 to 8081. And before starting with Mojolicious I killed apache since it was already the port. (That's the lazy way) This is only for 24.11 and 24.05 right ? (In reply to Fridolin Somers from comment #38) > This is only for 24.11 and 24.05 right ? Yes Thanks - not needed for 23.05 so not backporting to 23.05 :) Backported to 24.05.x for 24.05.04 Pushed for 24.11! Well done everyone, thank you! |