The CSRF check is done in the CGI->new constructor we overwrite. This is ugly and hazardous. We should move it to a Plack Middleware. However I didn't find a way to modify the request's parameters as CGI->param does not rely on plack's env parameters. Additionally Plack::Middleware does not provide methods to modify them directly and we would actually want to create our own Koha::Request that would inherit from Plack::Request to have our own query_parameters and body_parameters methods.
(In reply to Jonathan Druart from comment #0) > The CSRF check is done in the CGI->new constructor we overwrite. You mean these lines, right? Just to be clear :) my $original_op = $q->param('op'); my $request_method = $q->request_method // q{}; if ( $stateless_methods{$request_method} && defined $original_op && $original_op =~ m{^cud-} ) { warn "Programming error - op '$original_op' must not start with 'cud-' for $request_method"; $q->param( 'op', '' ); $q->param( 'debug_programming_error', "'$original_op' must not start with 'cud-' for $request_method" ); } elsif ( $stateful_methods{$request_method} && defined $q->param('op') && $original_op !~ m{^cud-} ) { warn "Programming error - op '$original_op' must start with 'cud-' for $request_method"; $q->param( 'op', '' ); $q->param( 'debug_programming_error', "'$original_op' must start with 'cud-' for $request_method" ); }
Cant we just postpone to get_template_and_user for most cases? Filter out unexpected methods used for regular scripts etc. Do not allow cud- with GET and perhaps always csrf check with POST?? Which cross domain posts should we actually allow?
Use this for inspiration! https://fastapi.metacpan.org/source/MATTP/Plack-Middleware-CSRFBlock-0.10/lib/Plack/Middleware/CSRFBlock.pm
(In reply to Marcel de Rooy from comment #1) > (In reply to Jonathan Druart from comment #0) > > The CSRF check is done in the CGI->new constructor we overwrite. > > You mean these lines, right? Just to be clear :) Yes. (In reply to Marcel de Rooy from comment #2) > Cant we just postpone to get_template_and_user for most cases? Filter out > unexpected methods used for regular scripts etc. Do not allow cud- with GET > and perhaps always csrf check with POST?? Which cross domain posts should we > actually allow? We don't always call get_template_and_user. The test was there at the beginning, see the history from 34478. See also further changes from bug 36084 (branch security/bug_34478_svc), what is on bug_34478 is not the final version.
Created attachment 162333 [details] [review] Bug 36148: Move CSRF check to a Plack middleware The easiest here is to not empty 'op' but instead redirect to an error page. Minor changes: to keep the patch simple it removed the 'dev only' error and display the error for non-dev installs. It should not be a problem anyway and will prevent errors to be hidden in the log. We could make KOHA_ERROR an arrayref, but later (we don't need it now anyway). Note that the OPAC still not benefit from a friendly specific error for invalid token.
To apply on top of security/bug_34478_svc (if not there yet).
Created attachment 162336 [details] [review] Bug 36148: Move CSRF check to a Plack middleware The easiest here is to not empty 'op' but instead redirect to an error page. Minor changes: to keep the patch simple it removed the 'dev only' error and display the error for non-dev installs. It should not be a problem anyway and will prevent errors to be hidden in the log. We could make KOHA_ERROR an arrayref, but later (we don't need it now anyway). Note that the OPAC still not benefit from a friendly specific error for invalid token.
Round two of testing... I'm testing the staff interface and while I see "Programming error - No CSRF token passed for POST" on the screen, I'm seeing a 404 status in the network tab, which is not ideal. I've come up with a solution which takes advantage of the HTTPExceptions and ErrorDocument middlewares. It appears that we can also pass environmental variables between middlewares (which I should've remembered from other Plack middlewares I've written for other apps). So if we want user-facing CSRF error messages, we can do that. I'm going to write up a little patch with my ideas...
Created attachment 162359 [details] [review] Bug 36148: Allow Koha::Middleware::CSRF to use error/exception middlewares This change allows Koha::Middleware::CSRF to use the ErrorDocument and HTTPExcetions middlewares to display the correct status codes and HTML documents. Leveraging Plack environmental variables, we're also able to pass along data to the error page handlers to show warnings indicating that there was a missing CSRF token.
I'm not considering my change to be the final version. I imagine that people might want to surface the CSRF error on the OPAC and Staff Interface differently. I basically just wanted to illustrate how to do it. I am actually going to submit another little follow-up to add some notes because there's 1 line which is super duper important and without it I nearly went crazy...
In fact, the check I do for $env->{'plack.middleware.Koha.CSRF'} happens too low. I just didn't want to make a white space change that would've made it impossible to see what I was changing...
Created attachment 162360 [details] [review] Bug 36148: Add explanatory notes
Note the change works for both the OPAC and Staff Interface. I think a case could be made for a 500 error instead of a 403 error. But I don't really care either way.
Thanks for your changes, David. They are great. I picked a generic "KOHA_ERROR" var on purpose. The idea was to make it an array and reusable for other potential low level errors. But I am fine with the plack.middleware.Koha.CSRF key.
Created attachment 162362 [details] [review] Bug 36148: Improve error handling and restore programming errors
Created attachment 162363 [details] [review] Bug 36148: Fix header name
We may want to return a JSON response instead of HTML is the client is asking for it.
The middleware seems to be working, the only thing I see is that the error pages aren't quite right. errno is not populated, so we don't see what type of error. On the staff side we are including 'messages.inc' which then includes 'blocking_errors.inc' which ends the template - so we never hit the csrf_error section of the template and we also don't get the 'What's next' section
(In reply to Nick Clemens from comment #19) > The middleware seems to be working, the only thing I see is that the error > pages aren't quite right. > > errno is not populated, so we don't see what type of error. > > On the staff side we are including 'messages.inc' which then includes > 'blocking_errors.inc' which ends the template - so we never hit the > csrf_error section of the template and we also don't get the 'What's next' > section Yeah looking at these latest changes I'm not sure that they're going in the right direction.
Created attachment 162428 [details] [review] Bug 36148: Do not deal with CSRF error in get_template_and_user We deal with that in the middleware, we don't want 403.pl to early exit. If we notice that we actually need it (for other scripts), we could eventually add a new flag to get_template_and_user to skip the CSRF check, or build the template without using get_template_and_user for errors/* scripts.
(In reply to Nick Clemens from comment #19) > The middleware seems to be working, the only thing I see is that the error > pages aren't quite right. > > errno is not populated, so we don't see what type of error. > > On the staff side we are including 'messages.inc' which then includes > 'blocking_errors.inc' which ends the template - so we never hit the > csrf_error section of the template and we also don't get the 'What's next' > section Indeed, should be fixed with the last patch. (In reply to David Cook from comment #20) > (In reply to Nick Clemens from comment #19) > > The middleware seems to be working, the only thing I see is that the error > > pages aren't quite right. > > > > errno is not populated, so we don't see what type of error. > > > > On the staff side we are including 'messages.inc' which then includes > > 'blocking_errors.inc' which ends the template - so we never hit the > > csrf_error section of the template and we also don't get the 'What's next' > > section > > Yeah looking at these latest changes I'm not sure that they're going in the > right direction. I think they do. Please provide an alternative approach if you still disagree.
403 is now looking like that: https://snipboard.io/Pr0cFd.jpg
(In reply to Jonathan Druart from comment #23) > 403 is now looking like that: https://snipboard.io/Pr0cFd.jpg That looks all right to me now. [U+1F44D]
Created attachment 162505 [details] [review] Bug 36148: Move CSRF check to a Plack middleware The easiest here is to not empty 'op' but instead redirect to an error page. Minor changes: to keep the patch simple it removed the 'dev only' error and display the error for non-dev installs. It should not be a problem anyway and will prevent errors to be hidden in the log. We could make KOHA_ERROR an arrayref, but later (we don't need it now anyway). Note that the OPAC still not benefit from a friendly specific error for invalid token. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162506 [details] [review] Bug 36148: Allow Koha::Middleware::CSRF to use error/exception middlewares This change allows Koha::Middleware::CSRF to use the ErrorDocument and HTTPExcetions middlewares to display the correct status codes and HTML documents. Leveraging Plack environmental variables, we're also able to pass along data to the error page handlers to show warnings indicating that there was a missing CSRF token. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162507 [details] [review] Bug 36148: Add explanatory notes Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162508 [details] [review] Bug 36148: Improve error handling and restore programming errors Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162509 [details] [review] Bug 36148: Fix header name Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162510 [details] [review] Bug 36148: Do not deal with CSRF error in get_template_and_user We deal with that in the middleware, we don't want 403.pl to early exit. If we notice that we actually need it (for other scripts), we could eventually add a new flag to get_template_and_user to skip the CSRF check, or build the template without using get_template_and_user for errors/* scripts. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 24.05.00.
Depends on Bug 34478 not in 23.11.x